All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU
@ 2014-06-17 14:37 Malcolm Crossley
  2014-06-17 15:06 ` Jan Beulich
  0 siblings, 1 reply; 6+ messages in thread
From: Malcolm Crossley @ 2014-06-17 14:37 UTC (permalink / raw)
  To: xen-devel
  Cc: kevin.tian, jbeulich, andrew.cooper3, paul.durrant,
	Aravind.Gopalakrishnan, suravee.suthikulpanit, yang.z.zhang

PCIe ATS allows for devices to contain IOTLBs, the VT-d code was iterating
around all ATS capable devices and issuing IOTLB operations for all IOMMUs,
even though each ATS device is only accessible via one particular IOMMU.

Issuing an IOMMU operation to a device not accessible via that IOMMU results
in an IOMMU timeout because the device does not reply. VT-d IOMMU timeouts
result in a Xen panic.

Therefore this bug prevents any Intel system with 2 or more ATS enabled IOMMUs,
each with an ATS device connected to them, from booting Xen.

The patch adds a IOMMU pointer to the ATS device struct so the VT-d code can
ensure it does not issue IOMMU ATS operations on the wrong IOMMU. A void
pointer has to be used because AMD and Intel IOMMU implementations do not have
a common IOMMU structure or indexing mechanism.

Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>

diff -r 4708591d8aa8 -r 0ec71ff08df1 xen/drivers/passthrough/amd/pci_amd_iommu.c
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -164,7 +164,7 @@ static void amd_iommu_setup_domain_devic
          !pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
     {
         if ( devfn == pdev->devfn )
-            enable_ats_device(iommu->seg, bus, devfn);
+            enable_ats_device(iommu, iommu->seg, bus, devfn);
 
         amd_iommu_flush_iotlb(devfn, pdev, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
     }
diff -r 4708591d8aa8 -r 0ec71ff08df1 xen/drivers/passthrough/ats.h
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -24,6 +24,7 @@ struct pci_ats_dev {
     u8 bus;
     u8 devfn;
     u16 ats_queue_depth;    /* ATS device invalidation queue depth */
+    const void *iommu;      /* No common IOMMU struct so use void pointer */
 };
 
 #define ATS_REG_CAP    4
@@ -34,7 +35,7 @@ struct pci_ats_dev {
 extern struct list_head ats_devices;
 extern bool_t ats_enabled;
 
-int enable_ats_device(int seg, int bus, int devfn);
+int enable_ats_device(const void *iommu, int seg, int bus, int devfn);
 void disable_ats_device(int seg, int bus, int devfn);
 struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn);
 
diff -r 4708591d8aa8 -r 0ec71ff08df1 xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1442,7 +1442,7 @@ static int domain_context_mapping(
         ret = domain_context_mapping_one(domain, drhd->iommu, bus, devfn,
                                          pdev);
         if ( !ret && devfn == pdev->devfn && ats_device(pdev, drhd) > 0 )
-            enable_ats_device(seg, bus, devfn);
+            enable_ats_device(drhd->iommu, seg, bus, devfn);
 
         break;
 
@@ -1930,7 +1930,7 @@ static int intel_iommu_enable_device(str
     if ( ret <= 0 )
         return ret;
 
-    ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn);
+    ret = enable_ats_device(drhd->iommu, pdev->seg, pdev->bus, pdev->devfn);
 
     return ret >= 0 ? 0 : ret;
 }
diff -r 4708591d8aa8 -r 0ec71ff08df1 xen/drivers/passthrough/vtd/x86/ats.c
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -120,6 +120,10 @@ int dev_invalidate_iotlb(struct iommu *i
     {
         sid = (pdev->bus << 8) | pdev->devfn;
 
+        /* Only invalidate devices that belong to this IOMMU */
+        if ( !pdev->iommu || pdev->iommu != iommu )
+            continue;
+
         switch ( type ) {
         case DMA_TLB_DSI_FLUSH:
             if ( !device_in_domain(iommu, pdev, did) )
diff -r 4708591d8aa8 -r 0ec71ff08df1 xen/drivers/passthrough/x86/ats.c
--- a/xen/drivers/passthrough/x86/ats.c
+++ b/xen/drivers/passthrough/x86/ats.c
@@ -23,7 +23,7 @@ LIST_HEAD(ats_devices);
 bool_t __read_mostly ats_enabled = 1;
 boolean_param("ats", ats_enabled);
 
-int enable_ats_device(int seg, int bus, int devfn)
+int enable_ats_device(const void *iommu, int seg, int bus, int devfn)
 {
     struct pci_ats_dev *pdev = NULL;
     u32 value;
@@ -66,6 +66,7 @@ int enable_ats_device(int seg, int bus, 
         pdev->seg = seg;
         pdev->bus = bus;
         pdev->devfn = devfn;
+        pdev->iommu = iommu;
         value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
                                 PCI_FUNC(devfn), pos + ATS_REG_CAP);
         pdev->ats_queue_depth = value & ATS_QUEUE_DEPTH_MASK ?:

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-18 10:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-17 14:37 [PATCH v3] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU Malcolm Crossley
2014-06-17 15:06 ` Jan Beulich
2014-06-17 16:15   ` Malcolm Crossley
2014-06-18 10:18     ` Jan Beulich
2014-06-18 10:24       ` Malcolm Crossley
2014-06-18 10:36         ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.