[PATCH v8 5/6] AMD/IOMMU: pull ATS disabling earlier

Jan Beulich posted 6 patches 3 years, 2 months ago
[PATCH v8 5/6] AMD/IOMMU: pull ATS disabling earlier
Posted by Jan Beulich 3 years, 2 months ago
Disabling should be done in the opposite order of enabling: ATS wants to
be turned off before adjusting the DTE, just like it gets enabled only
after the DTE was suitably prepared. Note that we want ATS to be
disabled as soon as any of the DTEs involved in the handling of a device
(including phantom devices) gets adjusted respectively. For this reason
the "devfn == pdev->devfn" of the original conditional gets dropped.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: This points out that for phantom devices the ordering in
     amd_iommu_setup_domain_device() may also not be fully suitable: ATS
     would better be enabled on the device only after all involved DTEs
     have got prepared. This would be a less straightforward change,
     though.
---
v8: New.

--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -313,6 +313,12 @@ static void amd_iommu_disable_domain_dev
     if ( QUARANTINE_SKIP(domain) )
         return;
 
+    ASSERT(pcidevs_locked());
+
+    if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
+         pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
+        disable_ats_device(pdev);
+
     BUG_ON ( iommu->dev_table.buffer == NULL );
     req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
     table = iommu->dev_table.buffer;
@@ -348,13 +354,6 @@ static void amd_iommu_disable_domain_dev
     }
     else
         spin_unlock_irqrestore(&iommu->lock, flags);
-
-    ASSERT(pcidevs_locked());
-
-    if ( devfn == pdev->devfn &&
-         pci_ats_device(iommu->seg, bus, devfn) &&
-         pci_ats_enabled(iommu->seg, bus, devfn) )
-        disable_ats_device(pdev);
 }
 
 static int reassign_device(struct domain *source, struct domain *target,


Re: [PATCH v8 5/6] AMD/IOMMU: pull ATS disabling earlier
Posted by Durrant, Paul 3 years, 2 months ago
On 22/09/2021 15:38, Jan Beulich wrote:
> Disabling should be done in the opposite order of enabling: ATS wants to
> be turned off before adjusting the DTE, just like it gets enabled only
> after the DTE was suitably prepared. Note that we want ATS to be
> disabled as soon as any of the DTEs involved in the handling of a device
> (including phantom devices) gets adjusted respectively. For this reason
> the "devfn == pdev->devfn" of the original conditional gets dropped.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul@xen.org>