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>
