[PATCH v15 0/3] VT-d Device-TLB flush issue

[PATCH v15 0/3] VT-d Device-TLB flush issue

[Jan Beulich]

From: Quan Xu <quan.xu@intel.com>

these patches fix current timeout concern and also allow limited ATS support.

these patches are the rest ones:
1. move the domain crash logic up to the generic IOMMU layer

2. If Device-TLB flush timed out, we hide the target ATS device
   immediately. By hiding the device, we make sure it can't be
   assigned to any domain any longer (see device_assigned).

Quan Xu (3):
  IOMMU/ATS: use a struct pci_dev * instead of SBDF
  IOMMU: add domain crash logic
  VT-d: fix Device-TLB flush timeout issue
---
v15: Carried out my own review comments now that Quan left Intel.
---
Not covered in this series:

    a) Eliminate the panic() in IOMMU_WAIT_OP, used only in VT-d register read/write.
       Further discussion is required on whether and how to improve it.
    b) Handle IOTLB/Context/IEC flush timeout.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v15 1/3] IOMMU/ATS: use a struct pci_dev * instead of SBDF

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 13479 bytes --]

From: Quan Xu <quan.xu@intel.com>

Do away with struct pci_ats_dev; integrate the few bits of information
in struct pci_dev (and as a result drop get_ats_device() altogether).
Hook ATS devices onto a linked list off of each IOMMU instead of on a
global one. 

Signed-off-by: Quan Xu <quan.xu@intel.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v15: See rewritten description. Ditch changes to vtd/intremap.c.

--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -289,35 +289,29 @@ void amd_iommu_flush_iotlb(u8 devfn, con
     unsigned long flags;
     struct amd_iommu *iommu;
     unsigned int req_id, queueid, maxpend;
-    struct pci_ats_dev *ats_pdev;
 
     if ( !ats_enabled )
         return;
 
-    ats_pdev = get_ats_device(pdev->seg, pdev->bus, pdev->devfn);
-    if ( ats_pdev == NULL )
+    if ( !pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) )
         return;
 
-    if ( !pci_ats_enabled(ats_pdev->seg, ats_pdev->bus, ats_pdev->devfn) )
-        return;
-
-    iommu = find_iommu_for_device(ats_pdev->seg,
-                                  PCI_BDF2(ats_pdev->bus, ats_pdev->devfn));
+    iommu = find_iommu_for_device(pdev->seg, PCI_BDF2(pdev->bus, pdev->devfn));
 
     if ( !iommu )
     {
         AMD_IOMMU_DEBUG("%s: Can't find iommu for %04x:%02x:%02x.%u\n",
-                        __func__, ats_pdev->seg, ats_pdev->bus,
-                        PCI_SLOT(ats_pdev->devfn), PCI_FUNC(ats_pdev->devfn));
+                        __func__, pdev->seg, pdev->bus,
+                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
         return;
     }
 
     if ( !iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
         return;
 
-    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(ats_pdev->bus, devfn));
+    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(pdev->bus, devfn));
     queueid = req_id;
-    maxpend = ats_pdev->ats_queue_depth & 0xff;
+    maxpend = pdev->ats.queue_depth & 0xff;
 
     /* send INVALIDATE_IOTLB_PAGES command */
     spin_lock_irqsave(&iommu->lock, flags);
--- a/xen/drivers/passthrough/amd/iommu_detect.c
+++ b/xen/drivers/passthrough/amd/iommu_detect.c
@@ -128,6 +128,7 @@ int __init amd_iommu_detect_one_acpi(
     }
 
     spin_lock_init(&iommu->lock);
+    INIT_LIST_HEAD(&iommu->ats_devices);
 
     iommu->seg = ivhd_block->pci_segment_group;
     iommu->bdf = ivhd_block->header.device_id;
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -162,7 +162,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, iommu);
+            enable_ats_device(pdev, &iommu->ats_devices);
 
         amd_iommu_flush_iotlb(devfn, pdev, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
     }
@@ -356,7 +356,7 @@ void amd_iommu_disable_domain_device(str
     if ( devfn == pdev->devfn &&
          pci_ats_device(iommu->seg, bus, devfn) &&
          pci_ats_enabled(iommu->seg, bus, devfn) )
-        disable_ats_device(iommu->seg, bus, devfn);
+        disable_ats_device(pdev);
 }
 
 static int reassign_device(struct domain *source, struct domain *target,
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -17,26 +17,15 @@
 
 #include <xen/pci_regs.h>
 
-struct pci_ats_dev {
-    struct list_head list;
-    u16 seg;
-    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
 #define ATS_REG_CTL    6
 #define ATS_QUEUE_DEPTH_MASK     0x1f
 #define ATS_ENABLE               (1<<15)
 
-extern struct list_head ats_devices;
 extern bool_t ats_enabled;
 
-int enable_ats_device(int seg, int bus, int devfn, const void *iommu);
-void disable_ats_device(int seg, int bus, int devfn);
-struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn);
+int enable_ats_device(struct pci_dev *pdev, struct list_head *ats_list);
+void disable_ats_device(struct pci_dev *pdev);
 
 static inline int pci_ats_enabled(int seg, int bus, int devfn)
 {
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1162,6 +1162,7 @@ int __init iommu_alloc(struct acpi_drhd_
         return -ENOMEM;
 
     iommu->msi.irq = -1; /* No irq assigned yet. */
+    INIT_LIST_HEAD(&iommu->ats_devices);
 
     iommu->intel = alloc_intel_iommu();
     if ( iommu->intel == NULL )
@@ -1461,8 +1462,8 @@ int domain_context_mapping_one(
     return rc;
 }
 
-static int domain_context_mapping(
-    struct domain *domain, u8 devfn, const struct pci_dev *pdev)
+static int domain_context_mapping(struct domain *domain, u8 devfn,
+                                  struct pci_dev *pdev)
 {
     struct acpi_drhd_unit *drhd;
     int ret = 0;
@@ -1498,7 +1499,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, drhd->iommu);
+            enable_ats_device(pdev, &drhd->iommu->ats_devices);
 
         break;
 
@@ -1611,8 +1612,8 @@ int domain_context_unmap_one(
     return rc;
 }
 
-static int domain_context_unmap(
-    struct domain *domain, u8 devfn, const struct pci_dev *pdev)
+static int domain_context_unmap(struct domain *domain, u8 devfn,
+                                struct pci_dev *pdev)
 {
     struct acpi_drhd_unit *drhd;
     struct iommu *iommu;
@@ -1648,7 +1649,7 @@ static int domain_context_unmap(
                    PCI_SLOT(devfn), PCI_FUNC(devfn));
         ret = domain_context_unmap_one(domain, iommu, bus, devfn);
         if ( !ret && devfn == pdev->devfn && ats_device(pdev, drhd) > 0 )
-            disable_ats_device(seg, bus, devfn);
+            disable_ats_device(pdev);
 
         break;
 
@@ -1994,7 +1995,7 @@ static int intel_iommu_enable_device(str
     if ( ret <= 0 )
         return ret;
 
-    ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu);
+    ret = enable_ats_device(pdev, &drhd->iommu->ats_devices);
 
     return ret >= 0 ? 0 : ret;
 }
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -542,6 +542,7 @@ struct iommu {
     u64 root_maddr; /* root entry machine address */
     struct msi_desc msi;
     struct intel_iommu *intel;
+    struct list_head ats_devices;
     unsigned long *domid_bitmap;  /* domain id bitmap */
     u16 *domid_map;               /* domain id mapping array */
 };
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -71,7 +71,8 @@ int ats_device(const struct pci_dev *pde
     return pos;
 }
 
-static int device_in_domain(struct iommu *iommu, struct pci_ats_dev *pdev, u16 did)
+static int device_in_domain(const struct iommu *iommu,
+                            const struct pci_dev *pdev, u16 did)
 {
     struct root_entry *root_entry = NULL;
     struct context_entry *ctxt_entry = NULL;
@@ -108,22 +109,18 @@ out:
 int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
     u64 addr, unsigned int size_order, u64 type)
 {
-    struct pci_ats_dev *pdev;
+    const struct pci_dev *pdev;
     int ret = 0;
 
     if ( !ecap_dev_iotlb(iommu->ecap) )
         return ret;
 
-    list_for_each_entry( pdev, &ats_devices, list )
+    list_for_each_entry( pdev, &iommu->ats_devices, ats.list )
     {
         u16 sid = PCI_BDF2(pdev->bus, pdev->devfn);
         bool_t sbit;
         int rc = 0;
 
-        /* Only invalidate devices that belong to this IOMMU */
-        if ( pdev->iommu != iommu )
-            continue;
-
         switch ( type )
         {
         case DMA_TLB_DSI_FLUSH:
@@ -134,7 +131,7 @@ int dev_invalidate_iotlb(struct iommu *i
             /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
             sbit = 1;
             addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats.queue_depth,
                                           sid, sbit, addr);
             break;
         case DMA_TLB_PSI_FLUSH:
@@ -154,7 +151,7 @@ int dev_invalidate_iotlb(struct iommu *i
                 addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
             }
 
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats.queue_depth,
                                           sid, sbit, addr);
             break;
         default:
--- a/xen/drivers/passthrough/x86/ats.c
+++ b/xen/drivers/passthrough/x86/ats.c
@@ -17,15 +17,14 @@
 #include <xen/pci_regs.h>
 #include "../ats.h"
 
-LIST_HEAD(ats_devices);
-
 bool_t __read_mostly ats_enabled = 0;
 boolean_param("ats", ats_enabled);
 
-int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
+int enable_ats_device(struct pci_dev *pdev, struct list_head *ats_list)
 {
-    struct pci_ats_dev *pdev = NULL;
     u32 value;
+    u16 seg = pdev->seg;
+    u8 bus = pdev->bus, devfn = pdev->devfn;
     int pos;
 
     pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
@@ -39,19 +38,15 @@ int enable_ats_device(int seg, int bus,
                             PCI_FUNC(devfn), pos + ATS_REG_CTL);
     if ( value & ATS_ENABLE )
     {
-        list_for_each_entry ( pdev, &ats_devices, list )
-        {
-            if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+        struct pci_dev *other;
+
+        list_for_each_entry ( other, ats_list, ats.list )
+            if ( other == pdev )
             {
                 pos = 0;
                 break;
             }
-        }
     }
-    if ( pos )
-        pdev = xmalloc(struct pci_ats_dev);
-    if ( !pdev )
-        return -ENOMEM;
 
     if ( !(value & ATS_ENABLE) )
     {
@@ -62,15 +57,12 @@ int enable_ats_device(int seg, int bus,
 
     if ( pos )
     {
-        pdev->seg = seg;
-        pdev->bus = bus;
-        pdev->devfn = devfn;
-        pdev->iommu = iommu;
+        pdev->ats.cap_pos = pos;
         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 ?:
+        pdev->ats.queue_depth = value & ATS_QUEUE_DEPTH_MASK ?:
                                 ATS_QUEUE_DEPTH_MASK + 1;
-        list_add(&pdev->list, &ats_devices);
+        list_add(&pdev->ats.list, ats_list);
     }
 
     if ( iommu_verbose )
@@ -81,48 +73,23 @@ int enable_ats_device(int seg, int bus,
     return pos;
 }
 
-void disable_ats_device(int seg, int bus, int devfn)
+void disable_ats_device(struct pci_dev *pdev)
 {
-    struct pci_ats_dev *pdev;
     u32 value;
-    int pos;
+    u16 seg = pdev->seg;
+    u8 bus = pdev->bus, devfn = pdev->devfn;
 
-    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
-    BUG_ON(!pos);
+    BUG_ON(!pdev->ats.cap_pos);
 
-    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
-                            PCI_FUNC(devfn), pos + ATS_REG_CTL);
+    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+                            pdev->ats.cap_pos + ATS_REG_CTL);
     value &= ~ATS_ENABLE;
     pci_conf_write16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                     pos + ATS_REG_CTL, value);
+                     pdev->ats.cap_pos + ATS_REG_CTL, value);
 
-    list_for_each_entry ( pdev, &ats_devices, list )
-    {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
-        {
-            list_del(&pdev->list);
-            xfree(pdev);
-            break;
-        }
-    }
+    list_del(&pdev->ats.list);
 
     if ( iommu_verbose )
         dprintk(XENLOG_INFO, "%04x:%02x:%02x.%u: ATS is disabled\n",
                 seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
-
-struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn)
-{
-    struct pci_ats_dev *pdev;
-
-    if ( !pci_ats_device(seg, bus, devfn) )
-        return NULL;
-
-    list_for_each_entry ( pdev, &ats_devices, list )
-    {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
-            return pdev;
-    }
-
-    return NULL;
-}
--- a/xen/include/asm-x86/amd-iommu.h
+++ b/xen/include/asm-x86/amd-iommu.h
@@ -104,6 +104,8 @@ struct amd_iommu {
     uint64_t exclusion_limit;
 
     int enabled;
+
+    struct list_head ats_devices;
 };
 
 struct ivrs_mappings {
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -78,6 +78,11 @@ struct pci_dev {
     struct pci_dev_info info;
     struct arch_pci_dev arch;
     struct {
+        struct list_head list;
+        unsigned int cap_pos;
+        unsigned int queue_depth;
+    } ats;
+    struct {
         s_time_t time;
         unsigned int count;
 #define PT_FAULT_THRESHOLD 10



[-- Attachment #2: IOMMU-ATS-info-reorg.patch --]
[-- Type: text/plain, Size: 13528 bytes --]

IOMMU/ATS: use a struct pci_dev * instead of SBDF

From: Quan Xu <quan.xu@intel.com>

Do away with struct pci_ats_dev; integrate the few bits of information
in struct pci_dev (and as a result drop get_ats_device() altogether).
Hook ATS devices onto a linked list off of each IOMMU instead of on a
global one. 

Signed-off-by: Quan Xu <quan.xu@intel.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v15: See rewritten description. Ditch changes to vtd/intremap.c.

--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -289,35 +289,29 @@ void amd_iommu_flush_iotlb(u8 devfn, con
     unsigned long flags;
     struct amd_iommu *iommu;
     unsigned int req_id, queueid, maxpend;
-    struct pci_ats_dev *ats_pdev;
 
     if ( !ats_enabled )
         return;
 
-    ats_pdev = get_ats_device(pdev->seg, pdev->bus, pdev->devfn);
-    if ( ats_pdev == NULL )
+    if ( !pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) )
         return;
 
-    if ( !pci_ats_enabled(ats_pdev->seg, ats_pdev->bus, ats_pdev->devfn) )
-        return;
-
-    iommu = find_iommu_for_device(ats_pdev->seg,
-                                  PCI_BDF2(ats_pdev->bus, ats_pdev->devfn));
+    iommu = find_iommu_for_device(pdev->seg, PCI_BDF2(pdev->bus, pdev->devfn));
 
     if ( !iommu )
     {
         AMD_IOMMU_DEBUG("%s: Can't find iommu for %04x:%02x:%02x.%u\n",
-                        __func__, ats_pdev->seg, ats_pdev->bus,
-                        PCI_SLOT(ats_pdev->devfn), PCI_FUNC(ats_pdev->devfn));
+                        __func__, pdev->seg, pdev->bus,
+                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
         return;
     }
 
     if ( !iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
         return;
 
-    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(ats_pdev->bus, devfn));
+    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(pdev->bus, devfn));
     queueid = req_id;
-    maxpend = ats_pdev->ats_queue_depth & 0xff;
+    maxpend = pdev->ats.queue_depth & 0xff;
 
     /* send INVALIDATE_IOTLB_PAGES command */
     spin_lock_irqsave(&iommu->lock, flags);
--- a/xen/drivers/passthrough/amd/iommu_detect.c
+++ b/xen/drivers/passthrough/amd/iommu_detect.c
@@ -128,6 +128,7 @@ int __init amd_iommu_detect_one_acpi(
     }
 
     spin_lock_init(&iommu->lock);
+    INIT_LIST_HEAD(&iommu->ats_devices);
 
     iommu->seg = ivhd_block->pci_segment_group;
     iommu->bdf = ivhd_block->header.device_id;
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -162,7 +162,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, iommu);
+            enable_ats_device(pdev, &iommu->ats_devices);
 
         amd_iommu_flush_iotlb(devfn, pdev, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
     }
@@ -356,7 +356,7 @@ void amd_iommu_disable_domain_device(str
     if ( devfn == pdev->devfn &&
          pci_ats_device(iommu->seg, bus, devfn) &&
          pci_ats_enabled(iommu->seg, bus, devfn) )
-        disable_ats_device(iommu->seg, bus, devfn);
+        disable_ats_device(pdev);
 }
 
 static int reassign_device(struct domain *source, struct domain *target,
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -17,26 +17,15 @@
 
 #include <xen/pci_regs.h>
 
-struct pci_ats_dev {
-    struct list_head list;
-    u16 seg;
-    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
 #define ATS_REG_CTL    6
 #define ATS_QUEUE_DEPTH_MASK     0x1f
 #define ATS_ENABLE               (1<<15)
 
-extern struct list_head ats_devices;
 extern bool_t ats_enabled;
 
-int enable_ats_device(int seg, int bus, int devfn, const void *iommu);
-void disable_ats_device(int seg, int bus, int devfn);
-struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn);
+int enable_ats_device(struct pci_dev *pdev, struct list_head *ats_list);
+void disable_ats_device(struct pci_dev *pdev);
 
 static inline int pci_ats_enabled(int seg, int bus, int devfn)
 {
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1162,6 +1162,7 @@ int __init iommu_alloc(struct acpi_drhd_
         return -ENOMEM;
 
     iommu->msi.irq = -1; /* No irq assigned yet. */
+    INIT_LIST_HEAD(&iommu->ats_devices);
 
     iommu->intel = alloc_intel_iommu();
     if ( iommu->intel == NULL )
@@ -1461,8 +1462,8 @@ int domain_context_mapping_one(
     return rc;
 }
 
-static int domain_context_mapping(
-    struct domain *domain, u8 devfn, const struct pci_dev *pdev)
+static int domain_context_mapping(struct domain *domain, u8 devfn,
+                                  struct pci_dev *pdev)
 {
     struct acpi_drhd_unit *drhd;
     int ret = 0;
@@ -1498,7 +1499,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, drhd->iommu);
+            enable_ats_device(pdev, &drhd->iommu->ats_devices);
 
         break;
 
@@ -1611,8 +1612,8 @@ int domain_context_unmap_one(
     return rc;
 }
 
-static int domain_context_unmap(
-    struct domain *domain, u8 devfn, const struct pci_dev *pdev)
+static int domain_context_unmap(struct domain *domain, u8 devfn,
+                                struct pci_dev *pdev)
 {
     struct acpi_drhd_unit *drhd;
     struct iommu *iommu;
@@ -1648,7 +1649,7 @@ static int domain_context_unmap(
                    PCI_SLOT(devfn), PCI_FUNC(devfn));
         ret = domain_context_unmap_one(domain, iommu, bus, devfn);
         if ( !ret && devfn == pdev->devfn && ats_device(pdev, drhd) > 0 )
-            disable_ats_device(seg, bus, devfn);
+            disable_ats_device(pdev);
 
         break;
 
@@ -1994,7 +1995,7 @@ static int intel_iommu_enable_device(str
     if ( ret <= 0 )
         return ret;
 
-    ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu);
+    ret = enable_ats_device(pdev, &drhd->iommu->ats_devices);
 
     return ret >= 0 ? 0 : ret;
 }
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -542,6 +542,7 @@ struct iommu {
     u64 root_maddr; /* root entry machine address */
     struct msi_desc msi;
     struct intel_iommu *intel;
+    struct list_head ats_devices;
     unsigned long *domid_bitmap;  /* domain id bitmap */
     u16 *domid_map;               /* domain id mapping array */
 };
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -71,7 +71,8 @@ int ats_device(const struct pci_dev *pde
     return pos;
 }
 
-static int device_in_domain(struct iommu *iommu, struct pci_ats_dev *pdev, u16 did)
+static int device_in_domain(const struct iommu *iommu,
+                            const struct pci_dev *pdev, u16 did)
 {
     struct root_entry *root_entry = NULL;
     struct context_entry *ctxt_entry = NULL;
@@ -108,22 +109,18 @@ out:
 int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
     u64 addr, unsigned int size_order, u64 type)
 {
-    struct pci_ats_dev *pdev;
+    const struct pci_dev *pdev;
     int ret = 0;
 
     if ( !ecap_dev_iotlb(iommu->ecap) )
         return ret;
 
-    list_for_each_entry( pdev, &ats_devices, list )
+    list_for_each_entry( pdev, &iommu->ats_devices, ats.list )
     {
         u16 sid = PCI_BDF2(pdev->bus, pdev->devfn);
         bool_t sbit;
         int rc = 0;
 
-        /* Only invalidate devices that belong to this IOMMU */
-        if ( pdev->iommu != iommu )
-            continue;
-
         switch ( type )
         {
         case DMA_TLB_DSI_FLUSH:
@@ -134,7 +131,7 @@ int dev_invalidate_iotlb(struct iommu *i
             /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
             sbit = 1;
             addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats.queue_depth,
                                           sid, sbit, addr);
             break;
         case DMA_TLB_PSI_FLUSH:
@@ -154,7 +151,7 @@ int dev_invalidate_iotlb(struct iommu *i
                 addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
             }
 
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
+            rc = qinval_device_iotlb_sync(iommu, pdev->ats.queue_depth,
                                           sid, sbit, addr);
             break;
         default:
--- a/xen/drivers/passthrough/x86/ats.c
+++ b/xen/drivers/passthrough/x86/ats.c
@@ -17,15 +17,14 @@
 #include <xen/pci_regs.h>
 #include "../ats.h"
 
-LIST_HEAD(ats_devices);
-
 bool_t __read_mostly ats_enabled = 0;
 boolean_param("ats", ats_enabled);
 
-int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
+int enable_ats_device(struct pci_dev *pdev, struct list_head *ats_list)
 {
-    struct pci_ats_dev *pdev = NULL;
     u32 value;
+    u16 seg = pdev->seg;
+    u8 bus = pdev->bus, devfn = pdev->devfn;
     int pos;
 
     pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
@@ -39,19 +38,15 @@ int enable_ats_device(int seg, int bus,
                             PCI_FUNC(devfn), pos + ATS_REG_CTL);
     if ( value & ATS_ENABLE )
     {
-        list_for_each_entry ( pdev, &ats_devices, list )
-        {
-            if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+        struct pci_dev *other;
+
+        list_for_each_entry ( other, ats_list, ats.list )
+            if ( other == pdev )
             {
                 pos = 0;
                 break;
             }
-        }
     }
-    if ( pos )
-        pdev = xmalloc(struct pci_ats_dev);
-    if ( !pdev )
-        return -ENOMEM;
 
     if ( !(value & ATS_ENABLE) )
     {
@@ -62,15 +57,12 @@ int enable_ats_device(int seg, int bus,
 
     if ( pos )
     {
-        pdev->seg = seg;
-        pdev->bus = bus;
-        pdev->devfn = devfn;
-        pdev->iommu = iommu;
+        pdev->ats.cap_pos = pos;
         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 ?:
+        pdev->ats.queue_depth = value & ATS_QUEUE_DEPTH_MASK ?:
                                 ATS_QUEUE_DEPTH_MASK + 1;
-        list_add(&pdev->list, &ats_devices);
+        list_add(&pdev->ats.list, ats_list);
     }
 
     if ( iommu_verbose )
@@ -81,48 +73,23 @@ int enable_ats_device(int seg, int bus,
     return pos;
 }
 
-void disable_ats_device(int seg, int bus, int devfn)
+void disable_ats_device(struct pci_dev *pdev)
 {
-    struct pci_ats_dev *pdev;
     u32 value;
-    int pos;
+    u16 seg = pdev->seg;
+    u8 bus = pdev->bus, devfn = pdev->devfn;
 
-    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
-    BUG_ON(!pos);
+    BUG_ON(!pdev->ats.cap_pos);
 
-    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
-                            PCI_FUNC(devfn), pos + ATS_REG_CTL);
+    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+                            pdev->ats.cap_pos + ATS_REG_CTL);
     value &= ~ATS_ENABLE;
     pci_conf_write16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                     pos + ATS_REG_CTL, value);
+                     pdev->ats.cap_pos + ATS_REG_CTL, value);
 
-    list_for_each_entry ( pdev, &ats_devices, list )
-    {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
-        {
-            list_del(&pdev->list);
-            xfree(pdev);
-            break;
-        }
-    }
+    list_del(&pdev->ats.list);
 
     if ( iommu_verbose )
         dprintk(XENLOG_INFO, "%04x:%02x:%02x.%u: ATS is disabled\n",
                 seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
-
-struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn)
-{
-    struct pci_ats_dev *pdev;
-
-    if ( !pci_ats_device(seg, bus, devfn) )
-        return NULL;
-
-    list_for_each_entry ( pdev, &ats_devices, list )
-    {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
-            return pdev;
-    }
-
-    return NULL;
-}
--- a/xen/include/asm-x86/amd-iommu.h
+++ b/xen/include/asm-x86/amd-iommu.h
@@ -104,6 +104,8 @@ struct amd_iommu {
     uint64_t exclusion_limit;
 
     int enabled;
+
+    struct list_head ats_devices;
 };
 
 struct ivrs_mappings {
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -78,6 +78,11 @@ struct pci_dev {
     struct pci_dev_info info;
     struct arch_pci_dev arch;
     struct {
+        struct list_head list;
+        unsigned int cap_pos;
+        unsigned int queue_depth;
+    } ats;
+    struct {
         s_time_t time;
         unsigned int count;
 #define PT_FAULT_THRESHOLD 10

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v15 1/3] IOMMU/ATS: use a struct pci_dev * instead of SBDF

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 08, 2016 2:44 PM
> 
> From: Quan Xu <quan.xu@intel.com>
> 
> Do away with struct pci_ats_dev; integrate the few bits of information
> in struct pci_dev (and as a result drop get_ats_device() altogether).
> Hook ATS devices onto a linked list off of each IOMMU instead of on a
> global one.
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v15: See rewritten description. Ditch changes to vtd/intremap.c.
> 

Thanks. It's a good enhancement:

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v15 2/3] IOMMU: add domain crash logic

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]

From: Quan Xu <quan.xu@intel.com>

Add domain crash logic to the generic IOMMU layer to benefit
all platforms.

No spamming of the log can occur. For DomU, we avoid logging any
message for already dying domains. For Dom0, that'll still be more
verbose than we'd really like, but it at least wouldn't outright
flood the console.

Signed-off-by: Quan Xu <quan.xu@intel.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -318,21 +318,47 @@ int iommu_iotlb_flush(struct domain *d,
                       unsigned int page_count)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    int rc;
 
     if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush )
         return 0;
 
-    return hd->platform_ops->iotlb_flush(d, gfn, page_count);
+    rc = hd->platform_ops->iotlb_flush(d, gfn, page_count);
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR
+                   "d%d: IOMMU IOTLB flush failed: %d, gfn %#lx, page count %u\n",
+                   d->domain_id, rc, gfn, page_count);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
+    return rc;
 }
 
 int iommu_iotlb_flush_all(struct domain *d)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    int rc;
 
     if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush_all )
         return 0;
 
-    return hd->platform_ops->iotlb_flush_all(d);
+    rc = hd->platform_ops->iotlb_flush_all(d);
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR
+                   "d%d: IOMMU IOTLB flush all failed: %d\n",
+                   d->domain_id, rc);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
+    return rc;
 }
 
 int __init iommu_setup(void)
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1848,6 +1848,17 @@ int iommu_pte_flush(struct domain *d, u6
         }
     }
 
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR VTDPREFIX
+                   " d%d: IOMMU pages flush failed: %d\n",
+                   d->domain_id, rc);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
     return rc;
 }
 




[-- Attachment #2: IOMMU-crash-guest-upon-flush-failure.patch --]
[-- Type: text/plain, Size: 2600 bytes --]

IOMMU: add domain crash logic

From: Quan Xu <quan.xu@intel.com>

Add domain crash logic to the generic IOMMU layer to benefit
all platforms.

No spamming of the log can occur. For DomU, we avoid logging any
message for already dying domains. For Dom0, that'll still be more
verbose than we'd really like, but it at least wouldn't outright
flood the console.

Signed-off-by: Quan Xu <quan.xu@intel.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -318,21 +318,47 @@ int iommu_iotlb_flush(struct domain *d,
                       unsigned int page_count)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    int rc;
 
     if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush )
         return 0;
 
-    return hd->platform_ops->iotlb_flush(d, gfn, page_count);
+    rc = hd->platform_ops->iotlb_flush(d, gfn, page_count);
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR
+                   "d%d: IOMMU IOTLB flush failed: %d, gfn %#lx, page count %u\n",
+                   d->domain_id, rc, gfn, page_count);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
+    return rc;
 }
 
 int iommu_iotlb_flush_all(struct domain *d)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    int rc;
 
     if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush_all )
         return 0;
 
-    return hd->platform_ops->iotlb_flush_all(d);
+    rc = hd->platform_ops->iotlb_flush_all(d);
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR
+                   "d%d: IOMMU IOTLB flush all failed: %d\n",
+                   d->domain_id, rc);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
+    return rc;
 }
 
 int __init iommu_setup(void)
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1848,6 +1848,17 @@ int iommu_pte_flush(struct domain *d, u6
         }
     }
 
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR VTDPREFIX
+                   " d%d: IOMMU pages flush failed: %d\n",
+                   d->domain_id, rc);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
     return rc;
 }
 

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v15 3/3] VT-d: fix Device-TLB flush timeout issue

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 9129 bytes --]

From: Quan Xu <quan.xu@intel.com>

If Device-TLB flush timed out, we hide the target ATS device
immediately. By hiding the device, we make sure it can't be
assigned to any domain any longer (see device_assigned).

Signed-off-by: Quan Xu <quan.xu@intel.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v15: Re-base on heavily changed patch 1. Call disable_ats_device() and
     domain_crash() from iommu_dev_iotlb_flush_timeout() and move the
     function to passthrough/pci.c to fix the ARM build. As a result of
     calling disable_ats_device() also use list_for_each_entry_safe()
     in dev_invalidate_iotlb().

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -32,6 +32,7 @@
 #include <xen/tasklet.h>
 #include <xsm/xsm.h>
 #include <asm/msi.h>
+#include "ats.h"
 
 struct pci_seg {
     struct list_head alldevs_list;
@@ -1504,6 +1505,34 @@ static int iommu_get_device_group(
     return i;
 }
 
+void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev)
+{
+    pcidevs_lock();
+
+    disable_ats_device(pdev);
+
+    ASSERT(pdev->domain);
+    if ( d != pdev->domain )
+    {
+        pcidevs_unlock();
+        return;
+    }
+
+    list_del(&pdev->domain_list);
+    pdev->domain = NULL;
+    _pci_hide_device(pdev);
+
+    if ( !d->is_shutting_down && printk_ratelimit() )
+        printk(XENLOG_ERR
+               "dom%d: ATS device %04x:%02x:%02x.%u flush failed\n",
+               d->domain_id, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+               PCI_FUNC(pdev->devfn));
+    if ( !is_hardware_domain(d) )
+        domain_crash(d);
+
+    pcidevs_unlock();
+}
+
 int iommu_do_pci_domctl(
     struct xen_domctl *domctl, struct domain *d,
     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -25,6 +25,7 @@
 
 #define VTDPREFIX "[VT-D]"
 
+struct pci_ats_dev;
 extern bool_t rwbf_quirk;
 
 void print_iommu_regs(struct acpi_drhd_unit *drhd);
@@ -60,8 +61,8 @@ int dev_invalidate_iotlb(struct iommu *i
                          u64 addr, unsigned int size_order, u64 type);
 
 int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
-                                          u32 max_invs_pend,
-                                          u16 sid, u16 size, u64 addr);
+                                          struct pci_dev *pdev,
+                                          u16 did, u16 size, u64 addr);
 
 unsigned int get_cache_line_size(void);
 void cacheline_flush(char *);
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -27,11 +27,11 @@
 #include "dmar.h"
 #include "vtd.h"
 #include "extern.h"
+#include "../ats.h"
 
 #define VTD_QI_TIMEOUT	1
 
-static int __must_check invalidate_sync(struct iommu *iommu,
-                                        bool_t flush_dev_iotlb);
+static int __must_check invalidate_sync(struct iommu *iommu);
 
 static void print_qi_regs(struct iommu *iommu)
 {
@@ -103,7 +103,7 @@ static int __must_check queue_invalidate
 
     unmap_vtd_domain_page(qinval_entries);
 
-    return invalidate_sync(iommu, 0);
+    return invalidate_sync(iommu);
 }
 
 static int __must_check queue_invalidate_iotlb_sync(struct iommu *iommu,
@@ -140,7 +140,7 @@ static int __must_check queue_invalidate
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    return invalidate_sync(iommu, 0);
+    return invalidate_sync(iommu);
 }
 
 static int __must_check queue_invalidate_wait(struct iommu *iommu,
@@ -199,25 +199,53 @@ static int __must_check queue_invalidate
     return -EOPNOTSUPP;
 }
 
-static int __must_check invalidate_sync(struct iommu *iommu,
-                                        bool_t flush_dev_iotlb)
+static int __must_check invalidate_sync(struct iommu *iommu)
 {
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
     ASSERT(qi_ctrl->qinval_maddr);
 
-    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
+    return queue_invalidate_wait(iommu, 0, 1, 1, 0);
 }
 
-int qinval_device_iotlb_sync(struct iommu *iommu,
-                             u32 max_invs_pend,
-                             u16 sid, u16 size, u64 addr)
+static int __must_check dev_invalidate_sync(struct iommu *iommu,
+                                            struct pci_dev *pdev, u16 did)
+{
+    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
+    int rc;
+
+    ASSERT(qi_ctrl->qinval_maddr);
+    rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
+    if ( rc == -ETIMEDOUT )
+    {
+        struct domain *d = NULL;
+
+        if ( test_bit(did, iommu->domid_bitmap) )
+            d = rcu_lock_domain_by_id(iommu->domid_map[did]);
+
+        /*
+         * In case the domain has been freed or the IOMMU domid bitmap is
+         * not valid, the device no longer belongs to this domain.
+         */
+        if ( d == NULL )
+            return rc;
+
+        iommu_dev_iotlb_flush_timeout(d, pdev);
+        rcu_unlock_domain(d);
+    }
+
+    return rc;
+}
+
+int qinval_device_iotlb_sync(struct iommu *iommu, struct pci_dev *pdev,
+                             u16 did, u16 size, u64 addr)
 {
     unsigned long flags;
     unsigned int index;
     u64 entry_base;
     struct qinval_entry *qinval_entry, *qinval_entries;
 
+    ASSERT(pdev);
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
     entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
@@ -227,9 +255,9 @@ int qinval_device_iotlb_sync(struct iomm
 
     qinval_entry->q.dev_iotlb_inv_dsc.lo.type = TYPE_INVAL_DEVICE_IOTLB;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0;
-    qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = max_invs_pend;
+    qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = pdev->ats.queue_depth;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_2 = 0;
-    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = sid;
+    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = PCI_BDF2(pdev->bus, pdev->devfn);
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_3 = 0;
 
     qinval_entry->q.dev_iotlb_inv_dsc.hi.size = size;
@@ -240,7 +268,7 @@ int qinval_device_iotlb_sync(struct iomm
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    return invalidate_sync(iommu, 1);
+    return dev_invalidate_sync(iommu, pdev, did);
 }
 
 static int __must_check queue_invalidate_iec_sync(struct iommu *iommu,
@@ -271,7 +299,7 @@ static int __must_check queue_invalidate
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    ret = invalidate_sync(iommu, 0);
+    ret = invalidate_sync(iommu);
 
     /*
      * reading vt-d architecture register will ensure
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -109,15 +109,14 @@ out:
 int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
     u64 addr, unsigned int size_order, u64 type)
 {
-    const struct pci_dev *pdev;
+    struct pci_dev *pdev, *temp;
     int ret = 0;
 
     if ( !ecap_dev_iotlb(iommu->ecap) )
         return ret;
 
-    list_for_each_entry( pdev, &iommu->ats_devices, ats.list )
+    list_for_each_entry_safe( pdev, temp, &iommu->ats_devices, ats.list )
     {
-        u16 sid = PCI_BDF2(pdev->bus, pdev->devfn);
         bool_t sbit;
         int rc = 0;
 
@@ -131,8 +130,7 @@ int dev_invalidate_iotlb(struct iommu *i
             /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
             sbit = 1;
             addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats.queue_depth,
-                                          sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev, did, sbit, addr);
             break;
         case DMA_TLB_PSI_FLUSH:
             if ( !device_in_domain(iommu, pdev, did) )
@@ -151,8 +149,7 @@ int dev_invalidate_iotlb(struct iommu *i
                 addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
             }
 
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats.queue_depth,
-                                          sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev, did, sbit, addr);
             break;
         default:
             dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush type\n");
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -207,6 +207,8 @@ int __must_check iommu_iotlb_flush(struc
                                    unsigned int page_count);
 int __must_check iommu_iotlb_flush_all(struct domain *d);
 
+void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev);
+
 /*
  * The purpose of the iommu_dont_flush_iotlb optional cpu flag is to
  * avoid unecessary iotlb_flush in the low level IOMMU code.



[-- Attachment #2: VT-d-fix-flush-timeout.patch --]
[-- Type: text/plain, Size: 9169 bytes --]

VT-d: fix Device-TLB flush timeout issue

From: Quan Xu <quan.xu@intel.com>

If Device-TLB flush timed out, we hide the target ATS device
immediately. By hiding the device, we make sure it can't be
assigned to any domain any longer (see device_assigned).

Signed-off-by: Quan Xu <quan.xu@intel.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v15: Re-base on heavily changed patch 1. Call disable_ats_device() and
     domain_crash() from iommu_dev_iotlb_flush_timeout() and move the
     function to passthrough/pci.c to fix the ARM build. As a result of
     calling disable_ats_device() also use list_for_each_entry_safe()
     in dev_invalidate_iotlb().

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -32,6 +32,7 @@
 #include <xen/tasklet.h>
 #include <xsm/xsm.h>
 #include <asm/msi.h>
+#include "ats.h"
 
 struct pci_seg {
     struct list_head alldevs_list;
@@ -1504,6 +1505,34 @@ static int iommu_get_device_group(
     return i;
 }
 
+void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev)
+{
+    pcidevs_lock();
+
+    disable_ats_device(pdev);
+
+    ASSERT(pdev->domain);
+    if ( d != pdev->domain )
+    {
+        pcidevs_unlock();
+        return;
+    }
+
+    list_del(&pdev->domain_list);
+    pdev->domain = NULL;
+    _pci_hide_device(pdev);
+
+    if ( !d->is_shutting_down && printk_ratelimit() )
+        printk(XENLOG_ERR
+               "dom%d: ATS device %04x:%02x:%02x.%u flush failed\n",
+               d->domain_id, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+               PCI_FUNC(pdev->devfn));
+    if ( !is_hardware_domain(d) )
+        domain_crash(d);
+
+    pcidevs_unlock();
+}
+
 int iommu_do_pci_domctl(
     struct xen_domctl *domctl, struct domain *d,
     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -25,6 +25,7 @@
 
 #define VTDPREFIX "[VT-D]"
 
+struct pci_ats_dev;
 extern bool_t rwbf_quirk;
 
 void print_iommu_regs(struct acpi_drhd_unit *drhd);
@@ -60,8 +61,8 @@ int dev_invalidate_iotlb(struct iommu *i
                          u64 addr, unsigned int size_order, u64 type);
 
 int __must_check qinval_device_iotlb_sync(struct iommu *iommu,
-                                          u32 max_invs_pend,
-                                          u16 sid, u16 size, u64 addr);
+                                          struct pci_dev *pdev,
+                                          u16 did, u16 size, u64 addr);
 
 unsigned int get_cache_line_size(void);
 void cacheline_flush(char *);
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -27,11 +27,11 @@
 #include "dmar.h"
 #include "vtd.h"
 #include "extern.h"
+#include "../ats.h"
 
 #define VTD_QI_TIMEOUT	1
 
-static int __must_check invalidate_sync(struct iommu *iommu,
-                                        bool_t flush_dev_iotlb);
+static int __must_check invalidate_sync(struct iommu *iommu);
 
 static void print_qi_regs(struct iommu *iommu)
 {
@@ -103,7 +103,7 @@ static int __must_check queue_invalidate
 
     unmap_vtd_domain_page(qinval_entries);
 
-    return invalidate_sync(iommu, 0);
+    return invalidate_sync(iommu);
 }
 
 static int __must_check queue_invalidate_iotlb_sync(struct iommu *iommu,
@@ -140,7 +140,7 @@ static int __must_check queue_invalidate
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    return invalidate_sync(iommu, 0);
+    return invalidate_sync(iommu);
 }
 
 static int __must_check queue_invalidate_wait(struct iommu *iommu,
@@ -199,25 +199,53 @@ static int __must_check queue_invalidate
     return -EOPNOTSUPP;
 }
 
-static int __must_check invalidate_sync(struct iommu *iommu,
-                                        bool_t flush_dev_iotlb)
+static int __must_check invalidate_sync(struct iommu *iommu)
 {
     struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 
     ASSERT(qi_ctrl->qinval_maddr);
 
-    return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb);
+    return queue_invalidate_wait(iommu, 0, 1, 1, 0);
 }
 
-int qinval_device_iotlb_sync(struct iommu *iommu,
-                             u32 max_invs_pend,
-                             u16 sid, u16 size, u64 addr)
+static int __must_check dev_invalidate_sync(struct iommu *iommu,
+                                            struct pci_dev *pdev, u16 did)
+{
+    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
+    int rc;
+
+    ASSERT(qi_ctrl->qinval_maddr);
+    rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
+    if ( rc == -ETIMEDOUT )
+    {
+        struct domain *d = NULL;
+
+        if ( test_bit(did, iommu->domid_bitmap) )
+            d = rcu_lock_domain_by_id(iommu->domid_map[did]);
+
+        /*
+         * In case the domain has been freed or the IOMMU domid bitmap is
+         * not valid, the device no longer belongs to this domain.
+         */
+        if ( d == NULL )
+            return rc;
+
+        iommu_dev_iotlb_flush_timeout(d, pdev);
+        rcu_unlock_domain(d);
+    }
+
+    return rc;
+}
+
+int qinval_device_iotlb_sync(struct iommu *iommu, struct pci_dev *pdev,
+                             u16 did, u16 size, u64 addr)
 {
     unsigned long flags;
     unsigned int index;
     u64 entry_base;
     struct qinval_entry *qinval_entry, *qinval_entries;
 
+    ASSERT(pdev);
     spin_lock_irqsave(&iommu->register_lock, flags);
     index = qinval_next_index(iommu);
     entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
@@ -227,9 +255,9 @@ int qinval_device_iotlb_sync(struct iomm
 
     qinval_entry->q.dev_iotlb_inv_dsc.lo.type = TYPE_INVAL_DEVICE_IOTLB;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0;
-    qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = max_invs_pend;
+    qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = pdev->ats.queue_depth;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_2 = 0;
-    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = sid;
+    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = PCI_BDF2(pdev->bus, pdev->devfn);
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_3 = 0;
 
     qinval_entry->q.dev_iotlb_inv_dsc.hi.size = size;
@@ -240,7 +268,7 @@ int qinval_device_iotlb_sync(struct iomm
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    return invalidate_sync(iommu, 1);
+    return dev_invalidate_sync(iommu, pdev, did);
 }
 
 static int __must_check queue_invalidate_iec_sync(struct iommu *iommu,
@@ -271,7 +299,7 @@ static int __must_check queue_invalidate
     qinval_update_qtail(iommu, index);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    ret = invalidate_sync(iommu, 0);
+    ret = invalidate_sync(iommu);
 
     /*
      * reading vt-d architecture register will ensure
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -109,15 +109,14 @@ out:
 int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
     u64 addr, unsigned int size_order, u64 type)
 {
-    const struct pci_dev *pdev;
+    struct pci_dev *pdev, *temp;
     int ret = 0;
 
     if ( !ecap_dev_iotlb(iommu->ecap) )
         return ret;
 
-    list_for_each_entry( pdev, &iommu->ats_devices, ats.list )
+    list_for_each_entry_safe( pdev, temp, &iommu->ats_devices, ats.list )
     {
-        u16 sid = PCI_BDF2(pdev->bus, pdev->devfn);
         bool_t sbit;
         int rc = 0;
 
@@ -131,8 +130,7 @@ int dev_invalidate_iotlb(struct iommu *i
             /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
             sbit = 1;
             addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats.queue_depth,
-                                          sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev, did, sbit, addr);
             break;
         case DMA_TLB_PSI_FLUSH:
             if ( !device_in_domain(iommu, pdev, did) )
@@ -151,8 +149,7 @@ int dev_invalidate_iotlb(struct iommu *i
                 addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
             }
 
-            rc = qinval_device_iotlb_sync(iommu, pdev->ats.queue_depth,
-                                          sid, sbit, addr);
+            rc = qinval_device_iotlb_sync(iommu, pdev, did, sbit, addr);
             break;
         default:
             dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush type\n");
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -207,6 +207,8 @@ int __must_check iommu_iotlb_flush(struc
                                    unsigned int page_count);
 int __must_check iommu_iotlb_flush_all(struct domain *d);
 
+void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev);
+
 /*
  * The purpose of the iommu_dont_flush_iotlb optional cpu flag is to
  * avoid unecessary iotlb_flush in the low level IOMMU code.

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v15 3/3] VT-d: fix Device-TLB flush timeout issue

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 08, 2016 2:46 PM
> 
> From: Quan Xu <quan.xu@intel.com>
> 
> If Device-TLB flush timed out, we hide the target ATS device
> immediately. By hiding the device, we make sure it can't be
> assigned to any domain any longer (see device_assigned).
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v15: Re-base on heavily changed patch 1. Call disable_ats_device() and
>      domain_crash() from iommu_dev_iotlb_flush_timeout() and move the
>      function to passthrough/pci.c to fix the ARM build. As a result of
>      calling disable_ats_device() also use list_for_each_entry_safe()
>      in dev_invalidate_iotlb().
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v15 0/3] VT-d Device-TLB flush issue

[Jan Beulich]

>>> On 08.07.16 at 08:39, <JBeulich@suse.com> wrote:
> From: Quan Xu <quan.xu@intel.com>
> 
> these patches fix current timeout concern and also allow limited ATS 
> support.
> 
> these patches are the rest ones:
> 1. move the domain crash logic up to the generic IOMMU layer
> 
> 2. If Device-TLB flush timed out, we hide the target ATS device
>    immediately. By hiding the device, we make sure it can't be
>    assigned to any domain any longer (see device_assigned).
> 
> Quan Xu (3):
>   IOMMU/ATS: use a struct pci_dev * instead of SBDF
>   IOMMU: add domain crash logic
>   VT-d: fix Device-TLB flush timeout issue

I should probably have said somewhere that obviously I didn't test
this for the actual ATS case, as I don't have access to any suitable
device. Therefore, alongside the necessary ack-s, it would be nice
if we could also get a Tested-by from someone who does have an
ATS device.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v15 0/3] VT-d Device-TLB flush issue

[Xu, Quan]

On July 08, 2016 2:52 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 08.07.16 at 08:39, <JBeulich@suse.com> wrote:
> > From: Quan Xu <quan.xu@intel.com>
> >
> > these patches fix current timeout concern and also allow limited ATS
> > support.
> >
> > these patches are the rest ones:
> > 1. move the domain crash logic up to the generic IOMMU layer
> >
> > 2. If Device-TLB flush timed out, we hide the target ATS device
> >    immediately. By hiding the device, we make sure it can't be
> >    assigned to any domain any longer (see device_assigned).
> >
> > Quan Xu (3):
> >   IOMMU/ATS: use a struct pci_dev * instead of SBDF
> >   IOMMU: add domain crash logic
> >   VT-d: fix Device-TLB flush timeout issue
> 
> I should probably have said somewhere that obviously I didn't test this for the
> actual ATS case, as I don't have access to any suitable device. Therefore,
> alongside the necessary ack-s, it would be nice if we could also get a Tested-by
> from someone who does have an ATS device.
> 

(today I just come back to return my laptop and while test it) thanks for your work and education :):)...

Tested-by: Quan Xu <quan.xu@intel.com>

Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v15 0/3] VT-d Device-TLB flush issue

[Andrew Cooper]

On 08/07/16 07:39, Jan Beulich wrote:
> From: Quan Xu <quan.xu@intel.com>
>
> these patches fix current timeout concern and also allow limited ATS support.
>
> these patches are the rest ones:
> 1. move the domain crash logic up to the generic IOMMU layer
>
> 2. If Device-TLB flush timed out, we hide the target ATS device
>    immediately. By hiding the device, we make sure it can't be
>    assigned to any domain any longer (see device_assigned).
>
> Quan Xu (3):
>   IOMMU/ATS: use a struct pci_dev * instead of SBDF
>   IOMMU: add domain crash logic
>   VT-d: fix Device-TLB flush timeout issue

Committed, thanks.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel