diff mbox

[v13,1/3] IOMMU/x86: use a struct pci_dev* instead of SBDF

Message ID 1467179974-57317-2-git-send-email-quan.xu@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quan Xu June 29, 2016, 5:59 a.m. UTC 
From: Quan Xu <quan.xu@intel.com>

A struct pci_dev* instead of SBDF is stored inside struct
pci_ats_dev and parameter to *_ats_device().

Also use ats_dev for "struct pci_ats_dev" variable, while
pdev (_pdev, if there is already a pdev) for "struct pci_dev"
in the scope of IOMMU.

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

CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

---
v13:
   1. use ats_dev for "struct pci_ats_dev" variable, while
      pdev (_pdev, if there is already a pdev) for "struct pci_dev"
      in the scope of IOMMU.
   2. add local variables seg, bus, and devfn, which will
      greatly reduce the number of changes in *_ats_device().
   3. convert SBDF into struct pci_dev* to disable_ats_device()
      get_ats_device() as well.
      (
       enable_ats_device() and disable_ats_device() can't have
       the const added to 'struct pci_dev *pdev', otherwise then
       compilation fails. also Afaict that would in turn eliminate
       the need for some of the changes further up. but get_ats_device()
       can.
      )
---
 xen/drivers/passthrough/amd/iommu_cmd.c     | 13 +++---
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 +-
 xen/drivers/passthrough/ats.h               | 10 ++---
 xen/drivers/passthrough/vtd/intremap.c      |  8 ++--
 xen/drivers/passthrough/vtd/iommu.c         | 14 +++---
 xen/drivers/passthrough/vtd/x86/ats.c       | 24 +++++++----
 xen/drivers/passthrough/x86/ats.c           | 67 ++++++++++++++++++-----------
 7 files changed, 81 insertions(+), 59 deletions(-)

Comments

Tian, Kevin July 4, 2016, 5:35 a.m. UTC | #1 
> From: Xu, Quan
> Sent: Wednesday, June 29, 2016 2:00 PM
> 
> From: Quan Xu <quan.xu@intel.com>
> 
> A struct pci_dev* instead of SBDF is stored inside struct
> pci_ats_dev and parameter to *_ats_device().
> 
> Also use ats_dev for "struct pci_ats_dev" variable, while
> pdev (_pdev, if there is already a pdev) for "struct pci_dev"
> in the scope of IOMMU.
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> 
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Feng Wu <feng.wu@intel.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>, with one minor comment:

> diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c
> b/xen/drivers/passthrough/amd/iommu_cmd.c
> index 7c9d9be..934977a 100644
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -294,28 +294,27 @@ void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev
> *pdev,
>      if ( !ats_enabled )
>          return;
> 
> -    ats_pdev = get_ats_device(pdev->seg, pdev->bus, pdev->devfn);
> +    ats_pdev = get_ats_device(pdev);
>      if ( ats_pdev == NULL )
>          return;
> 

To align with other places, better to change ats_pdev to ats_dev too.
Quan Xu July 4, 2016, 5:41 a.m. UTC | #2 
On July 04, 2016 1:35 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Wednesday, June 29, 2016 2:00 PM
> >
> > From: Quan Xu <quan.xu@intel.com>
> >
> > A struct pci_dev* instead of SBDF is stored inside struct pci_ats_dev
> > and parameter to *_ats_device().
> >
> > Also use ats_dev for "struct pci_ats_dev" variable, while pdev (_pdev,
> > if there is already a pdev) for "struct pci_dev"
> > in the scope of IOMMU.
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Kevin Tian <kevin.tian@intel.com>
> > CC: Feng Wu <feng.wu@intel.com>
> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >
> 
> Acked-by: Kevin Tian <kevin.tian@intel.com>, with one minor comment:
> 
> > diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c
> > b/xen/drivers/passthrough/amd/iommu_cmd.c
> > index 7c9d9be..934977a 100644
> > --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> > +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> > @@ -294,28 +294,27 @@ void amd_iommu_flush_iotlb(u8 devfn, const
> > struct pci_dev *pdev,
> >      if ( !ats_enabled )
> >          return;
> >
> > -    ats_pdev = get_ats_device(pdev->seg, pdev->bus, pdev->devfn);
> > +    ats_pdev = get_ats_device(pdev);
> >      if ( ats_pdev == NULL )
> >          return;
> >
> 
> To align with other places, better to change ats_pdev to ats_dev too.


Indeed,  thanks very much!!
I will update it soon.
- Quan
diff mbox

Patch

diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c
index 7c9d9be..934977a 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -294,28 +294,27 @@  void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev *pdev,
     if ( !ats_enabled )
         return;
 
-    ats_pdev = get_ats_device(pdev->seg, pdev->bus, pdev->devfn);
+    ats_pdev = get_ats_device(pdev);
     if ( ats_pdev == NULL )
         return;
 
-    if ( !pci_ats_enabled(ats_pdev->seg, ats_pdev->bus, ats_pdev->devfn) )
+    if ( !pci_ats_enabled(pdev->seg, pdev->bus, 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, pdev->devfn));
     queueid = req_id;
     maxpend = ats_pdev->ats_queue_depth & 0xff;
 
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 7761241..dad4a71 100644
--- 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_device(
          !pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
     {
         if ( devfn == pdev->devfn )
-            enable_ats_device(iommu->seg, bus, devfn, iommu);
+            enable_ats_device(iommu, pdev);
 
         amd_iommu_flush_iotlb(devfn, pdev, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
     }
@@ -356,7 +356,7 @@  void amd_iommu_disable_domain_device(struct domain *domain,
     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,
diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h
index 5c91572..47ff22d 100644
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -19,9 +19,7 @@ 
 
 struct pci_ats_dev {
     struct list_head list;
-    u16 seg;
-    u8 bus;
-    u8 devfn;
+    struct pci_dev *pdev;
     u16 ats_queue_depth;    /* ATS device invalidation queue depth */
     const void *iommu;      /* No common IOMMU struct so use void pointer */
 };
@@ -34,9 +32,9 @@  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, 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(const void *iommu, struct pci_dev *pdev);
+void disable_ats_device(struct pci_dev *pdev);
+struct pci_ats_dev *get_ats_device(const struct pci_dev *pdev);
 
 static inline int pci_ats_enabled(int seg, int bus, int devfn)
 {
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
index 7c11565..28e3cf4 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -952,7 +952,7 @@  int pi_update_irte(const struct vcpu *v, const struct pirq *pirq,
     const struct msi_desc *msi_desc;
     int remap_index;
     int rc = 0;
-    const struct pci_dev *pci_dev;
+    const struct pci_dev *pdev;
     const struct acpi_drhd_unit *drhd;
     struct iommu *iommu;
     struct ir_ctrl *ir_ctrl;
@@ -972,8 +972,8 @@  int pi_update_irte(const struct vcpu *v, const struct pirq *pirq,
         goto unlock_out;
     }
 
-    pci_dev = msi_desc->dev;
-    if ( !pci_dev )
+    pdev = msi_desc->dev;
+    if ( !pdev )
     {
         rc = -ENODEV;
         goto unlock_out;
@@ -990,7 +990,7 @@  int pi_update_irte(const struct vcpu *v, const struct pirq *pirq,
      * 'struct msi_desc' in some other place, so we don't need to waste
      * time searching it here.
      */
-    drhd = acpi_find_matched_drhd_unit(pci_dev);
+    drhd = acpi_find_matched_drhd_unit(pdev);
     if ( !drhd )
         return -ENODEV;
 
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index f010612..cc34497 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1461,8 +1461,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 +1498,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(drhd->iommu, pdev);
 
         break;
 
@@ -1611,8 +1611,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 +1648,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 +1994,7 @@  static int intel_iommu_enable_device(struct pci_dev *pdev)
     if ( ret <= 0 )
         return ret;
 
-    ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu);
+    ret = enable_ats_device(drhd->iommu, pdev);
 
     return ret >= 0 ? 0 : ret;
 }
diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c
index dfa4d30..11fe9bb 100644
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -71,12 +71,14 @@  int ats_device(const struct pci_dev *pdev, const struct acpi_drhd_unit *drhd)
     return pos;
 }
 
-static int device_in_domain(struct iommu *iommu, struct pci_ats_dev *pdev, u16 did)
+static int device_in_domain(struct iommu *iommu, struct pci_ats_dev *ats_dev, u16 did)
 {
     struct root_entry *root_entry = NULL;
     struct context_entry *ctxt_entry = NULL;
     int tt, found = 0;
+    struct pci_dev *pdev = ats_dev->pdev;
 
+    ASSERT(pdev);
     root_entry = (struct root_entry *) map_vtd_domain_page(iommu->root_maddr);
     if ( !root_entry || !root_present(root_entry[pdev->bus]) )
         goto out;
@@ -108,37 +110,41 @@  out:
 int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
     u64 addr, unsigned int size_order, u64 type)
 {
-    struct pci_ats_dev *pdev;
+    struct pci_ats_dev *ats_dev;
     int ret = 0;
 
     if ( !ecap_dev_iotlb(iommu->ecap) )
         return ret;
 
-    list_for_each_entry( pdev, &ats_devices, list )
+    list_for_each_entry( ats_dev, &ats_devices, list )
     {
-        u16 sid = PCI_BDF2(pdev->bus, pdev->devfn);
+        struct pci_dev *pdev = ats_dev->pdev;
+        u16 sid;
         bool_t sbit;
         int rc = 0;
 
+        ASSERT(pdev);
+        sid = PCI_BDF2(pdev->bus, pdev->devfn);
+
         /* Only invalidate devices that belong to this IOMMU */
-        if ( pdev->iommu != iommu )
+        if ( ats_dev->iommu != iommu )
             continue;
 
         switch ( type )
         {
         case DMA_TLB_DSI_FLUSH:
-            if ( !device_in_domain(iommu, pdev, did) )
+            if ( !device_in_domain(iommu, ats_dev, did) )
                 break;
             /* fall through if DSI condition met */
         case DMA_TLB_GLOBAL_FLUSH:
             /* 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, ats_dev->ats_queue_depth,
                                           sid, sbit, addr);
             break;
         case DMA_TLB_PSI_FLUSH:
-            if ( !device_in_domain(iommu, pdev, did) )
+            if ( !device_in_domain(iommu, ats_dev, did) )
                 break;
 
             /* if size <= 4K, set sbit = 0, else set sbit = 1 */
@@ -154,7 +160,7 @@  int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                 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, ats_dev->ats_queue_depth,
                                           sid, sbit, addr);
             break;
         default:
diff --git a/xen/drivers/passthrough/x86/ats.c b/xen/drivers/passthrough/x86/ats.c
index 40c9f40..627b9e3 100644
--- a/xen/drivers/passthrough/x86/ats.c
+++ b/xen/drivers/passthrough/x86/ats.c
@@ -22,10 +22,12 @@  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(const void *iommu, struct pci_dev *pdev)
 {
-    struct pci_ats_dev *pdev = NULL;
+    struct pci_ats_dev *ats_dev = 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,9 +41,14 @@  int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
                             PCI_FUNC(devfn), pos + ATS_REG_CTL);
     if ( value & ATS_ENABLE )
     {
-        list_for_each_entry ( pdev, &ats_devices, list )
+        list_for_each_entry ( ats_dev, &ats_devices, list )
         {
-            if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+            struct pci_dev *_pdev = ats_dev->pdev;
+
+            ASSERT(_pdev);
+            if ( _pdev->seg == seg &&
+                 _pdev->bus == bus &&
+                 _pdev->devfn == devfn )
             {
                 pos = 0;
                 break;
@@ -49,8 +56,8 @@  int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
         }
     }
     if ( pos )
-        pdev = xmalloc(struct pci_ats_dev);
-    if ( !pdev )
+        ats_dev = xmalloc(struct pci_ats_dev);
+    if ( !ats_dev )
         return -ENOMEM;
 
     if ( !(value & ATS_ENABLE) )
@@ -62,15 +69,13 @@  int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
 
     if ( pos )
     {
-        pdev->seg = seg;
-        pdev->bus = bus;
-        pdev->devfn = devfn;
-        pdev->iommu = iommu;
+        ats_dev->pdev = pdev;
+        ats_dev->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 ?:
-                                ATS_QUEUE_DEPTH_MASK + 1;
-        list_add(&pdev->list, &ats_devices);
+        ats_dev->ats_queue_depth = value & ATS_QUEUE_DEPTH_MASK ?:
+                                   ATS_QUEUE_DEPTH_MASK + 1;
+        list_add(&ats_dev->list, &ats_devices);
     }
 
     if ( iommu_verbose )
@@ -81,10 +86,12 @@  int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
     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;
+    struct pci_ats_dev *ats_dev;
     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);
@@ -96,12 +103,17 @@  void disable_ats_device(int seg, int bus, int devfn)
     pci_conf_write16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
                      pos + ATS_REG_CTL, value);
 
-    list_for_each_entry ( pdev, &ats_devices, list )
+    list_for_each_entry ( ats_dev, &ats_devices, list )
     {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+        struct pci_dev *_pdev = ats_dev->pdev;
+
+        ASSERT(_pdev);
+        if ( _pdev->seg == seg &&
+             _pdev->bus == bus &&
+             _pdev->devfn == devfn )
         {
-            list_del(&pdev->list);
-            xfree(pdev);
+            list_del(&ats_dev->list);
+            xfree(ats_dev);
             break;
         }
     }
@@ -111,17 +123,24 @@  void disable_ats_device(int seg, int bus, int devfn)
                 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 *get_ats_device(const struct pci_dev *pdev)
 {
-    struct pci_ats_dev *pdev;
+    struct pci_ats_dev *ats_dev;
+    u16 seg = pdev->seg;
+    u8 bus = pdev->bus, devfn = pdev->devfn;
 
     if ( !pci_ats_device(seg, bus, devfn) )
         return NULL;
 
-    list_for_each_entry ( pdev, &ats_devices, list )
+    list_for_each_entry ( ats_dev, &ats_devices, list )
     {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
-            return pdev;
+        struct pci_dev *_pdev = ats_dev->pdev;
+
+        ASSERT(_pdev);
+        if ( _pdev->seg == seg &&
+             _pdev->bus == bus &&
+             _pdev->devfn == devfn )
+            return ats_dev;
     }
 
     return NULL;