Re: [PATCH v4 3/4] iommu/s390: handle IOAT registration based on domain

[Niklas Schnelle]

On Fri, 2025-02-07 at 15:53 -0500, Matthew Rosato wrote:
> At this point, the dma_table is really a property of the s390-iommu
> domain.  Rather than checking its contents elsewhere in the codebase,
> move the code that registers the table with firmware into
> s390-iommu and make a decision what to register with firmware based
> upon the type of domain in use for the device in question.
> 
> Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> ---
>  arch/s390/include/asm/pci.h |  2 ++
>  arch/s390/kvm/pci.c         | 17 ++-------------
>  arch/s390/pci/pci.c         | 35 +++++++++++++++++-------------
>  arch/s390/pci/pci_sysfs.c   | 11 +---------
>  drivers/iommu/s390-iommu.c  | 43 +++++++++++++++++++++++++++++++++++--
>  5 files changed, 66 insertions(+), 42 deletions(-)
> 
> 
--8<---
>  int zpci_hot_reset_device(struct zpci_dev *zdev)
>  {
> -	u8 status;
>  	int rc;
>  
>  	lockdep_assert_held(&zdev->state_lock);
> @@ -758,19 +773,9 @@ int zpci_hot_reset_device(struct zpci_dev *zdev)
>  			return rc;
>  	}
>  
> -	rc = zpci_enable_device(zdev);
> -	if (rc)
> -		return rc;
> -
> -	if (zdev->dma_table)
> -		rc = zpci_register_ioat(zdev, 0, zdev->start_dma, zdev->end_dma,
> -					virt_to_phys(zdev->dma_table), &status);
> -	if (rc) {
> -		zpci_disable_device(zdev);
> -		return rc;
> -	}
> +	rc = zpci_reenable_device(zdev);
>  
> -	return 0;
> +	return rc;

I can confirm that this change to re-do the I/O address translation re-
registration fixes the below zpci_hot_reset_device() test which  in an
iommu.passthrough=1 guest was causing host IOMMU violations in v3:

# echo 'bus' > /sys/bus/pci/devices/2003\:00\:00.0/reset_method
# echo 1 > /sys/bus/pci/devices/2003\:00\:00.0/reset

>  }
> 
---8<---
>  
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index fbdeded3d48b..007ccfdad495 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -381,6 +381,46 @@ static void zdev_s390_domain_update(struct zpci_dev *zdev,
>  	spin_unlock_irqrestore(&zdev->dom_lock, flags);
>  }
>  
> +static int s390_iommu_domain_reg_ioat(struct zpci_dev *zdev,
> +				      struct iommu_domain *domain, u8 *status)
> +{
> +	struct s390_domain *s390_domain;
> +	int rc = 0;
> +	u64 iota;
> +
> +	switch (domain->type) {
> +	case IOMMU_DOMAIN_IDENTITY:
> +		rc = zpci_register_ioat(zdev, 0, zdev->start_dma,
> +					zdev->end_dma, 0, status);
> +		break;
> +	case IOMMU_DOMAIN_BLOCKED:
> +		/* Nothing to do in this case */
> +		break;
> +	default:
> +		s390_domain = to_s390_domain(domain);
> +		iota = virt_to_phys(s390_domain->dma_table) |
> +		       ZPCI_IOTA_RTTO_FLAG;
> +		rc = zpci_register_ioat(zdev, 0, zdev->start_dma,
> +					zdev->end_dma, iota, status);
> +	}
> +
> +	return rc;
> +}
> +
> +int zpci_iommu_register_ioat(struct zpci_dev *zdev, u8 *status)
> +{
> +	unsigned long flags;
> +	int rc;
> +
> +	spin_lock_irqsave(&zdev->dom_lock, flags);
> +
> +	rc = s390_iommu_domain_reg_ioat(zdev, zdev->s390_domain, status);
> +
> +	spin_unlock_irqrestore(&zdev->dom_lock, flags);
> +
> +	return rc;
> +}
> +
> 

I really like how this takes out more of the IOMMU details from non-
IOMMU code. Definitely an improvement in terms of on its own. As stated
above I tested that this fixes the one issue I stumbled over in testing
the previous version. So feel free to add:

Tested-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
Reviewed-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>