[Qemu-devel] [PATCH v3] fix pci_requester_id()

[Qemu-devel] [PATCH v3] fix pci_requester_id()

[Peter Xu]

This is v3 to fix pci_requester_id() issue.

I avoided detecting root bus type since it seems not matter much if
we will directly drop them for non-pcie cases.

Also, I used zero to indicate uncached requester_id (assuming that
BDF 0x0000 is never used by any device).

v3 changes:
- add empty line after local var defines
- enhance comments to explain reason than what is done, also fix
  some english errors in comments
- add requester_id cache: Here I didn't cache the PCI device but
  requester ID directly, since for pcie-to-pci case we are not
  returning BDF but (secondary bus num, 0) pair. So if to cache with
  device, we need one more field, which is complicated. This is
  based on the assumption that requester_id will not change for a
  device lifecycle (is it possible that guest kernel modify
  bus number during runtime?). Anyway, please let me know if I am
  wrong.

Test done: IOMMU IR v7 (not yet posted) work with pci-pci bridges.

Thanks,

Peter Xu (1):
  pci: fix pci_requester_id()

 hw/i386/kvm/pci-assign.c |  2 +-
 hw/pci/pci.c             | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h     | 11 +++++++++--
 3 files changed, 56 insertions(+), 3 deletions(-)

-- 
2.4.11

[Qemu-devel] [PATCH v3] pci: fix pci_requester_id()

[Peter Xu]

This fix SID verification failure when IOMMU IR is enabled with PCI
bridges. Existing pci_requester_id() is more like getting BDF info
only. Renaming it to pci_get_bdf(). Meanwhile, we provide the correct
implementation to get requester ID. VT-d spec 5.1.1 is a good reference
to go, though it talks only about interrupt delivery, the rule works
exactly the same for non-interrupt cases.

Currently, there are three use cases for pci_requester_id():

- PCIX status bits: here we need BDF only, not requester ID. Replacing
  with pci_get_bdf().
- PCIe Error injection and MSI delivery: for both these cases, we are
  looking for requester IDs. Here we should use the new impl.

To avoid a PCI walk every time we send MSI message, one requester_id
field is added to PCIDevice to cache the result when we use it the first
time.  Here assumption is made that requester_id will never change
during device lifecycle.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/kvm/pci-assign.c |  2 +-
 hw/pci/pci.c             | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h     | 11 +++++++++--
 3 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index bf425a2..c40ab36 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1481,7 +1481,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
          * error bits, leave the rest. */
         status = pci_get_long(pci_dev->config + pos + PCI_X_STATUS);
         status &= ~(PCI_X_STATUS_BUS | PCI_X_STATUS_DEVFN);
-        status |= pci_requester_id(pci_dev);
+        status |= pci_get_bdf(pci_dev);
         status &= ~(PCI_X_STATUS_SPL_DISC | PCI_X_STATUS_UNX_SPL |
                     PCI_X_STATUS_SPL_ERR);
         pci_set_long(pci_dev->config + pos + PCI_X_STATUS, status);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bb605ef..0a35255 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -885,6 +885,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     }
 
     pci_dev->devfn = devfn;
+    pci_dev->requester_id = 0;  /* Not cached */
     dma_as = pci_device_iommu_address_space(pci_dev);
 
     memory_region_init_alias(&pci_dev->bus_master_enable_region,
@@ -2498,6 +2499,51 @@ PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
     }
 }
 
+/* Parse bridges up to the root complex and get final Requester ID
+ * for this device.  For full PCIe topology, this works exactly as
+ * what pci_get_bdf() does. However, several tricks are required
+ * when mixed up with legacy PCI devices and PCIe-to-PCI bridges. */
+static uint16_t pci_requester_id_no_cache(PCIDevice *dev)
+{
+    uint8_t bus_n;
+    uint16_t result = pci_get_bdf(dev);
+
+    while (!pci_bus_is_root(dev->bus)) {
+        /* We are under PCI/PCIe bridges, fetch bus number of
+         * current bus, which is the secondary bus number of
+         * parent bridge. */
+        bus_n = pci_bus_num(dev->bus);
+        dev = dev->bus->parent_dev;
+        if (pci_is_express(dev)) {
+            if (pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
+                /* When we pass through PCIe-to-PCI/PCIX bridges, we
+                 * override the requester ID using secondary bus
+                 * number of parent bridge with zeroed devfn
+                 * (pcie-to-pci bridge spec chap 2.3). */
+                result = PCI_BUILD_BDF(bus_n, 0);
+            }
+        } else {
+            /* Legacy PCI, override requester ID with the bridge's
+             * BDF upstream.  When the root complex connects to
+             * legacy PCI devices (including buses), it can only
+             * obtain requester ID info from directly attached
+             * devices.  If devices are attached under bridges, only
+             * the requester ID of the bridge that is directly
+             * attached to the root complex can be recognized. */
+            result = pci_get_bdf(dev);
+        }
+    }
+    return result;
+}
+
+uint16_t pci_requester_id(PCIDevice *dev)
+{
+    if (unlikely(!dev->requester_id)) {
+        dev->requester_id = pci_requester_id_no_cache(dev);
+    }
+    return dev->requester_id;
+}
+
 static const TypeInfo pci_device_type_info = {
     .name = TYPE_PCI_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index ef6ba51..cb3ab3b 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -15,6 +15,7 @@
 #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
 #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
 #define PCI_FUNC(devfn)         ((devfn) & 0x07)
+#define PCI_BUILD_BDF(bus, devfn)     ((bus << 8) | (devfn))
 #define PCI_SLOT_MAX            32
 #define PCI_FUNC_MAX            8
 
@@ -252,6 +253,10 @@ struct PCIDevice {
     /* the following fields are read only */
     PCIBus *bus;
     int32_t devfn;
+    /* Cached requester ID, to avoid the PCI tree walking every time
+     * we invoke PCI request (e.g., MSI).  For conventional PCI root
+     * complex, this field is meaningless. */
+    uint16_t requester_id;
     char name[64];
     PCIIORegion io_regions[PCI_NUM_REGIONS];
     AddressSpace bus_master_as;
@@ -685,11 +690,13 @@ static inline uint32_t pci_config_size(const PCIDevice *d)
     return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
 }
 
-static inline uint16_t pci_requester_id(PCIDevice *dev)
+static inline uint16_t pci_get_bdf(PCIDevice *dev)
 {
-    return (pci_bus_num(dev->bus) << 8) | dev->devfn;
+    return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
 }
 
+uint16_t pci_requester_id(PCIDevice *dev);
+
 /* DMA access functions */
 static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
 {
-- 
2.4.11

Re: [Qemu-devel] [PATCH v3] pci: fix pci_requester_id()

[Michael S. Tsirkin]

On Tue, May 17, 2016 at 02:45:07PM +0800, Peter Xu wrote:
> This fix SID verification failure when IOMMU IR is enabled with PCI
> bridges. Existing pci_requester_id() is more like getting BDF info
> only. Renaming it to pci_get_bdf(). Meanwhile, we provide the correct
> implementation to get requester ID. VT-d spec 5.1.1 is a good reference
> to go, though it talks only about interrupt delivery, the rule works
> exactly the same for non-interrupt cases.
> 
> Currently, there are three use cases for pci_requester_id():
> 
> - PCIX status bits: here we need BDF only, not requester ID. Replacing
>   with pci_get_bdf().
> - PCIe Error injection and MSI delivery: for both these cases, we are
>   looking for requester IDs. Here we should use the new impl.
> 
> To avoid a PCI walk every time we send MSI message, one requester_id
> field is added to PCIDevice to cache the result when we use it the first
> time.  Here assumption is made that requester_id will never change
> during device lifecycle.

That's wrong though. It can change if bus number changes.

That's why I said add a pointer to the actual requester,
set it up during initialization, not on first use.


> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/kvm/pci-assign.c |  2 +-
>  hw/pci/pci.c             | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h     | 11 +++++++++--
>  3 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index bf425a2..c40ab36 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -1481,7 +1481,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
>           * error bits, leave the rest. */
>          status = pci_get_long(pci_dev->config + pos + PCI_X_STATUS);
>          status &= ~(PCI_X_STATUS_BUS | PCI_X_STATUS_DEVFN);
> -        status |= pci_requester_id(pci_dev);
> +        status |= pci_get_bdf(pci_dev);
>          status &= ~(PCI_X_STATUS_SPL_DISC | PCI_X_STATUS_UNX_SPL |
>                      PCI_X_STATUS_SPL_ERR);
>          pci_set_long(pci_dev->config + pos + PCI_X_STATUS, status);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bb605ef..0a35255 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -885,6 +885,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      }
>  
>      pci_dev->devfn = devfn;
> +    pci_dev->requester_id = 0;  /* Not cached */
>      dma_as = pci_device_iommu_address_space(pci_dev);
>  
>      memory_region_init_alias(&pci_dev->bus_master_enable_region,
> @@ -2498,6 +2499,51 @@ PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
>      }
>  }
>  
> +/* Parse bridges up to the root complex and get final Requester ID
> + * for this device.  For full PCIe topology, this works exactly as
> + * what pci_get_bdf() does. However, several tricks are required
> + * when mixed up with legacy PCI devices and PCIe-to-PCI bridges. */
> +static uint16_t pci_requester_id_no_cache(PCIDevice *dev)
> +{
> +    uint8_t bus_n;
> +    uint16_t result = pci_get_bdf(dev);
> +
> +    while (!pci_bus_is_root(dev->bus)) {
> +        /* We are under PCI/PCIe bridges, fetch bus number of
> +         * current bus, which is the secondary bus number of
> +         * parent bridge. */
> +        bus_n = pci_bus_num(dev->bus);
> +        dev = dev->bus->parent_dev;
> +        if (pci_is_express(dev)) {
> +            if (pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> +                /* When we pass through PCIe-to-PCI/PCIX bridges, we
> +                 * override the requester ID using secondary bus
> +                 * number of parent bridge with zeroed devfn
> +                 * (pcie-to-pci bridge spec chap 2.3). */
> +                result = PCI_BUILD_BDF(bus_n, 0);
> +            }
> +        } else {
> +            /* Legacy PCI, override requester ID with the bridge's
> +             * BDF upstream.  When the root complex connects to
> +             * legacy PCI devices (including buses), it can only
> +             * obtain requester ID info from directly attached
> +             * devices.  If devices are attached under bridges, only
> +             * the requester ID of the bridge that is directly
> +             * attached to the root complex can be recognized. */
> +            result = pci_get_bdf(dev);
> +        }
> +    }
> +    return result;
> +}
> +
> +uint16_t pci_requester_id(PCIDevice *dev)
> +{
> +    if (unlikely(!dev->requester_id)) {
> +        dev->requester_id = pci_requester_id_no_cache(dev);
> +    }
> +    return dev->requester_id;
> +}
> +
>  static const TypeInfo pci_device_type_info = {
>      .name = TYPE_PCI_DEVICE,
>      .parent = TYPE_DEVICE,
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index ef6ba51..cb3ab3b 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -15,6 +15,7 @@
>  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>  #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
>  #define PCI_FUNC(devfn)         ((devfn) & 0x07)
> +#define PCI_BUILD_BDF(bus, devfn)     ((bus << 8) | (devfn))
>  #define PCI_SLOT_MAX            32
>  #define PCI_FUNC_MAX            8
>  
> @@ -252,6 +253,10 @@ struct PCIDevice {
>      /* the following fields are read only */
>      PCIBus *bus;
>      int32_t devfn;
> +    /* Cached requester ID, to avoid the PCI tree walking every time
> +     * we invoke PCI request (e.g., MSI).  For conventional PCI root
> +     * complex, this field is meaningless. */
> +    uint16_t requester_id;
>      char name[64];
>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>      AddressSpace bus_master_as;
> @@ -685,11 +690,13 @@ static inline uint32_t pci_config_size(const PCIDevice *d)
>      return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
>  }
>  
> -static inline uint16_t pci_requester_id(PCIDevice *dev)
> +static inline uint16_t pci_get_bdf(PCIDevice *dev)
>  {
> -    return (pci_bus_num(dev->bus) << 8) | dev->devfn;
> +    return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
>  }
>  
> +uint16_t pci_requester_id(PCIDevice *dev);
> +
>  /* DMA access functions */
>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>  {
> -- 
> 2.4.11

Re: [Qemu-devel] [PATCH v3] pci: fix pci_requester_id()

[Peter Xu]

On Tue, May 17, 2016 at 10:46:12AM +0300, Michael S. Tsirkin wrote:
> On Tue, May 17, 2016 at 02:45:07PM +0800, Peter Xu wrote:
> > This fix SID verification failure when IOMMU IR is enabled with PCI
> > bridges. Existing pci_requester_id() is more like getting BDF info
> > only. Renaming it to pci_get_bdf(). Meanwhile, we provide the correct
> > implementation to get requester ID. VT-d spec 5.1.1 is a good reference
> > to go, though it talks only about interrupt delivery, the rule works
> > exactly the same for non-interrupt cases.
> > 
> > Currently, there are three use cases for pci_requester_id():
> > 
> > - PCIX status bits: here we need BDF only, not requester ID. Replacing
> >   with pci_get_bdf().
> > - PCIe Error injection and MSI delivery: for both these cases, we are
> >   looking for requester IDs. Here we should use the new impl.
> > 
> > To avoid a PCI walk every time we send MSI message, one requester_id
> > field is added to PCIDevice to cache the result when we use it the first
> > time.  Here assumption is made that requester_id will never change
> > during device lifecycle.
> 
> That's wrong though. It can change if bus number changes.
> 
> That's why I said add a pointer to the actual requester,
> set it up during initialization, not on first use.

Ah... So finally we need one more field for that... Will fix in
v4. Thanks!

-- peterx