[Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Knut Omang]

This patch set changes the data structure used to handle address spaces within
the emulated Intel iommu to support traversal also if bus numbers are dynamically
allocated, as is the case for devices that sit behind root ports or downstream switches.
This means that we cannot use bus number as index, instead a QLIST is used.

This requires a change in the API for setup of IOMMUs which is taken care of by
the first patch. The second patch implements the fix.

The initial patch set had some discussion related to whether this fix, applied to the
bridge code, was applicable to all bridges. No clear conclusion arised as far as I understood,
in the meantime a number of people have run into the same issue as I did which lead me
to implement this, so I gather it might be a useful intermediate solution that works until
a better approach can be found? I believe the IOMMU emulation code has limited usefulness
if it only supports devices sitting directly on the root complex.

This is the thread following the initial patch set:

  http://thread.gmane.org/gmane.comp.emulators.qemu/302246

The patch set was also discussed in this thread:

  http://thread.gmane.org/gmane.comp.emulators.qemu/316949

Changes from v1:
  - Rebased to current master
  - Fixed minor syntax issues

Knut Omang (2):
  iommu: Replace bus+devfn arguments with PCIDevice* in PCIIOMMUFunc
  intel_iommu: Add support for translation for devices behind bridges.

 hw/alpha/typhoon.c            |  2 +-
 hw/i386/intel_iommu.c         | 56 +++++++++++++++++++------------------------
 hw/pci-host/apb.c             |  2 +-
 hw/pci-host/prep.c            |  3 +--
 hw/pci-host/q35.c             | 42 +++++++++++++-------------------
 hw/pci/pci.c                  |  7 +++---
 hw/pci/pci_bridge.c           |  6 +++++
 hw/ppc/spapr_pci.c            |  2 +-
 include/hw/i386/intel_iommu.h |  6 +++--
 include/hw/pci/pci.h          |  5 +++-
 10 files changed, 62 insertions(+), 69 deletions(-)

--
2.4.3

[Qemu-devel] [PATCH v2 1/2] iommu: Replace bus+devfn arguments with PCIDevice* in PCIIOMMUFunc

[Knut Omang]

The dev pointer is needed by intel_iommu to enable it to store the dma address pointer
with the device.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 hw/alpha/typhoon.c   | 2 +-
 hw/pci-host/apb.c    | 2 +-
 hw/pci-host/prep.c   | 3 +--
 hw/pci-host/q35.c    | 5 +++--
 hw/pci/pci.c         | 7 +++----
 hw/ppc/spapr_pci.c   | 2 +-
 include/hw/pci/pci.h | 4 +++-
 7 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 421162e..7e9a521 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -726,7 +726,7 @@ static const MemoryRegionIOMMUOps typhoon_iommu_ops = {
     .translate = typhoon_translate_iommu,
 };
 
-static AddressSpace *typhoon_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
+static AddressSpace *typhoon_pci_dma_iommu(PCIDevice *dev, void *opaque)
 {
     TyphoonState *s = opaque;
     return &s->pchip.iommu_as;
diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 599768e..6c78887 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -198,7 +198,7 @@ static inline void pbm_clear_request(APBState *s, unsigned int irq_num)
     s->irq_request = NO_IRQ_REQUEST;
 }
 
-static AddressSpace *pbm_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
+static AddressSpace *pbm_pci_dma_iommu(PCIDevice *dev, void *opaque)
 {
     IOMMUState *is = opaque;
 
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index c63f45d..cfe5594 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -196,8 +196,7 @@ static void raven_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(pic[irq_num] , level);
 }
 
-static AddressSpace *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
-                                             int devfn)
+static AddressSpace *raven_pcihost_set_iommu(PCIDevice *dev, void *opaque)
 {
     PREPPCIState *s = opaque;
 
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index bd74094..7892482 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -423,11 +423,12 @@ static void mch_reset(DeviceState *qdev)
     mch_update(mch);
 }
 
-static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
+static AddressSpace *q35_host_dma_iommu(PCIDevice *dev, void *opaque)
 {
     IntelIOMMUState *s = opaque;
     VTDAddressSpace **pvtd_as;
-    int bus_num = pci_bus_num(bus);
+    int bus_num = pci_bus_num(dev->bus);
+    int devfn = dev->devfn;
 
     assert(0 <= bus_num && bus_num <= VTD_PCI_BUS_MAX);
     assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 251bcbf..5ece4c0 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -870,7 +870,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     PCIConfigReadFunc *config_read = pc->config_read;
     PCIConfigWriteFunc *config_write = pc->config_write;
     Error *local_err = NULL;
-    AddressSpace *dma_as;
 
     if (devfn < 0) {
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
@@ -892,11 +891,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
 
     pci_dev->bus = bus;
     pci_dev->devfn = devfn;
-    dma_as = pci_device_iommu_address_space(pci_dev);
+    pci_dev->dma_as = pci_device_iommu_address_space(pci_dev);
 
     memory_region_init_alias(&pci_dev->bus_master_enable_region,
                              OBJECT(pci_dev), "bus master",
-                             dma_as->root, 0, memory_region_size(dma_as->root));
+                             pci_dev->dma_as->root, 0, memory_region_size(pci_dev->dma_as->root));
     memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
     address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region,
                        name);
@@ -2442,7 +2441,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
     PCIBus *bus = PCI_BUS(dev->bus);
 
     if (bus->iommu_fn) {
-        return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
+        return bus->iommu_fn(dev, bus->iommu_opaque);
     }
 
     if (bus->parent_dev) {
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index a8f79d8..7d39317 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -747,7 +747,7 @@ static const MemoryRegionOps spapr_msi_ops = {
 /*
  * PHB PCI device
  */
-static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
+static AddressSpace *spapr_pci_dma_iommu(PCIDevice *dev, void *opaque)
 {
     sPAPRPHBState *phb = opaque;
 
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 2e9d8ba..e142641 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -247,6 +247,7 @@ struct PCIDevice {
     char name[64];
     PCIIORegion io_regions[PCI_NUM_REGIONS];
     AddressSpace bus_master_as;
+    AddressSpace *dma_as;
     MemoryRegion bus_master_enable_region;
 
     /* do not access the following fields */
@@ -415,7 +416,8 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range);
 
 void pci_device_deassert_intx(PCIDevice *dev);
 
-typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
+typedef AddressSpace *(*PCIIOMMUFunc)(PCIDevice *, void *);
+void pci_set_dma_address_space(AddressSpace *dma_address_space);
 
 AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
 void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
-- 
2.4.3

[Qemu-devel] [PATCH v2 2/2] intel_iommu: Add support for translation for devices behind bridges.

[Knut Omang]

- Add call to pci_setup_iommu for the secondary bus in a bridge.
- Refactor IntelIOMMUState to use a list instead of tables based on
  bus/devfn, as bus numbers can change dynamically.
- Instead store reference to the VTDAddressSpace as an AddressSpace
  pointer, dma_as within PCIDevice.
- Use NULL dev to q35_host_dma_iommu to indicate a special (non-pci)
  device (needed by interrupt remapping logic)

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 hw/i386/intel_iommu.c         | 56 +++++++++++++++++++------------------------
 hw/pci-host/q35.c             | 41 +++++++++++++------------------
 hw/pci/pci_bridge.c           |  6 +++++
 include/hw/i386/intel_iommu.h |  6 +++--
 include/hw/pci/pci.h          |  3 ++-
 5 files changed, 52 insertions(+), 60 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 08055a8..aaac24e 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -20,6 +20,7 @@
  */
 
 #include "hw/sysbus.h"
+#include "hw/pci/pci.h"
 #include "exec/address-spaces.h"
 #include "intel_iommu_internal.h"
 
@@ -30,6 +31,7 @@ enum {
     DEBUG_CACHE,
 };
 #define VTD_DBGBIT(x)   (1 << DEBUG_##x)
+
 static int vtd_dbgflags = VTD_DBGBIT(GENERAL) | VTD_DBGBIT(CSR);
 
 #define VTD_DPRINTF(what, fmt, ...) do { \
@@ -166,24 +168,11 @@ static gboolean vtd_hash_remove_by_page(gpointer key, gpointer value,
  */
 static void vtd_reset_context_cache(IntelIOMMUState *s)
 {
-    VTDAddressSpace **pvtd_as;
     VTDAddressSpace *vtd_as;
-    uint32_t bus_it;
-    uint32_t devfn_it;
 
     VTD_DPRINTF(CACHE, "global context_cache_gen=1");
-    for (bus_it = 0; bus_it < VTD_PCI_BUS_MAX; ++bus_it) {
-        pvtd_as = s->address_spaces[bus_it];
-        if (!pvtd_as) {
-            continue;
-        }
-        for (devfn_it = 0; devfn_it < VTD_PCI_DEVFN_MAX; ++devfn_it) {
-            vtd_as = pvtd_as[devfn_it];
-            if (!vtd_as) {
-                continue;
-            }
-            vtd_as->context_cache_entry.context_cache_gen = 0;
-        }
+    QLIST_FOREACH(vtd_as, &s->address_spaces, iommu_next) {
+        vtd_as->context_cache_entry.context_cache_gen = 0;
     }
     s->context_cache_gen = 1;
 }
@@ -751,11 +740,11 @@ static inline bool vtd_is_interrupt_addr(hwaddr addr)
  *
  * @bus_num: The bus number
  * @devfn: The devfn, which is the  combined of device and function number
+ * @vtd_as: The address space to translate against
  * @is_write: The access is a write operation
  * @entry: IOMMUTLBEntry that contain the addr to be translated and result
  */
-static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, uint8_t bus_num,
-                                   uint8_t devfn, hwaddr addr, bool is_write,
+static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, hwaddr addr, bool is_write,
                                    IOMMUTLBEntry *entry)
 {
     IntelIOMMUState *s = vtd_as->iommu_state;
@@ -763,6 +752,8 @@ static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, uint8_t bus_num,
     VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;
     uint64_t slpte;
     uint32_t level;
+    uint8_t bus_num = pci_bus_num(vtd_as->dev->bus);
+    uint8_t devfn = vtd_as->devfn;
     uint16_t source_id = vtd_make_source_id(bus_num, devfn);
     int ret_fr;
     bool is_fpd_set = false;
@@ -882,10 +873,10 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,
                                           uint16_t func_mask)
 {
     uint16_t mask;
-    VTDAddressSpace **pvtd_as;
     VTDAddressSpace *vtd_as;
     uint16_t devfn;
-    uint16_t devfn_it;
+    uint16_t devfn_it = 0;
+    uint8_t bus_num, bus_num_it;
 
     switch (func_mask & 3) {
     case 0:
@@ -903,16 +894,18 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,
     }
     VTD_DPRINTF(INV, "device-selective invalidation source 0x%"PRIx16
                     " mask %"PRIu16, source_id, mask);
-    pvtd_as = s->address_spaces[VTD_SID_TO_BUS(source_id)];
-    if (pvtd_as) {
-        devfn = VTD_SID_TO_DEVFN(source_id);
-        for (devfn_it = 0; devfn_it < VTD_PCI_DEVFN_MAX; ++devfn_it) {
-            vtd_as = pvtd_as[devfn_it];
-            if (vtd_as && ((devfn_it & mask) == (devfn & mask))) {
-                VTD_DPRINTF(INV, "invalidate context-cahce of devfn 0x%"PRIx16,
-                            devfn_it);
-                vtd_as->context_cache_entry.context_cache_gen = 0;
-            }
+    bus_num = VTD_SID_TO_BUS(source_id);
+    devfn = VTD_SID_TO_DEVFN(source_id);
+
+    QLIST_FOREACH(vtd_as, &s->address_spaces, iommu_next) {
+        bus_num_it = pci_bus_num(vtd_as->dev->bus);
+        if (bus_num_it != bus_num)
+            continue;
+        if ((devfn_it & mask) == (devfn & mask)) {
+            VTD_DPRINTF(INV, "invalidate context-cahce of devfn 0x%"PRIx16,
+                        devfn_it);
+            vtd_as->context_cache_entry.context_cache_gen = 0;
+            break;
         }
     }
 }
@@ -1805,8 +1798,7 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr,
         return ret;
     }
 
-    vtd_do_iommu_translate(vtd_as, vtd_as->bus_num, vtd_as->devfn, addr,
-                           is_write, &ret);
+    vtd_do_iommu_translate(vtd_as, addr, is_write, &ret);
     VTD_DPRINTF(MMU,
                 "bus %"PRIu8 " slot %"PRIu8 " func %"PRIu8 " devfn %"PRIu8
                 " gpa 0x%"PRIx64 " hpa 0x%"PRIx64, vtd_as->bus_num,
@@ -1931,7 +1923,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
 
     VTD_DPRINTF(GENERAL, "");
-    memset(s->address_spaces, 0, sizeof(s->address_spaces));
+    QLIST_INIT(&s->address_spaces);
     memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,
                           "intel_iommu", DMAR_REG_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->csrmem);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 7892482..35d4032 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -425,33 +425,24 @@ static void mch_reset(DeviceState *qdev)
 
 static AddressSpace *q35_host_dma_iommu(PCIDevice *dev, void *opaque)
 {
-    IntelIOMMUState *s = opaque;
-    VTDAddressSpace **pvtd_as;
-    int bus_num = pci_bus_num(dev->bus);
-    int devfn = dev->devfn;
-
-    assert(0 <= bus_num && bus_num <= VTD_PCI_BUS_MAX);
-    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
-
-    pvtd_as = s->address_spaces[bus_num];
-    if (!pvtd_as) {
-        /* No corresponding free() */
-        pvtd_as = g_malloc0(sizeof(VTDAddressSpace *) * VTD_PCI_DEVFN_MAX);
-        s->address_spaces[bus_num] = pvtd_as;
-    }
-    if (!pvtd_as[devfn]) {
-        pvtd_as[devfn] = g_malloc0(sizeof(VTDAddressSpace));
-
-        pvtd_as[devfn]->bus_num = (uint8_t)bus_num;
-        pvtd_as[devfn]->devfn = (uint8_t)devfn;
-        pvtd_as[devfn]->iommu_state = s;
-        pvtd_as[devfn]->context_cache_entry.context_cache_gen = 0;
-        memory_region_init_iommu(&pvtd_as[devfn]->iommu, OBJECT(s),
+    VTDAddressSpace *as = NULL;
+    struct IntelIOMMUState *s = opaque;
+
+    if (dev && dev->dma_as)
+        as = container_of(dev->dma_as, VTDAddressSpace, as);
+    if (!as) {
+        as = g_malloc0(sizeof(VTDAddressSpace));
+        as->dev = dev;
+        as->devfn = dev->devfn;
+        as->iommu_state = s;
+        as->context_cache_entry.context_cache_gen = 0;
+        memory_region_init_iommu(&as->iommu, OBJECT(s),
                                  &s->iommu_ops, "intel_iommu", UINT64_MAX);
-        address_space_init(&pvtd_as[devfn]->as,
-                           &pvtd_as[devfn]->iommu, "intel_iommu");
+        address_space_init(&as->as,
+                           &as->iommu, "intel_iommu");
+        QLIST_INSERT_HEAD(&s->address_spaces, as, iommu_next);
     }
-    return &pvtd_as[devfn]->as;
+    return &as->as;
 }
 
 static void mch_init_dmar(MCHPCIState *mch)
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 40c97b1..e6832c4 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -376,8 +376,14 @@ int pci_bridge_initfn(PCIDevice *dev, const char *typename)
     sec_bus->address_space_io = &br->address_space_io;
     memory_region_init(&br->address_space_io, OBJECT(br), "pci_bridge_io", 65536);
     br->windows = pci_bridge_region_init(br);
+
     QLIST_INIT(&sec_bus->child);
     QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
+
+    if (dev->bus->iommu_opaque) {
+        pci_setup_iommu(sec_bus, dev->bus->iommu_fn, dev->bus->iommu_opaque);
+    }
+
     return 0;
 }
 
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index e321ee4..1b838a5 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -21,6 +21,7 @@
 
 #ifndef INTEL_IOMMU_H
 #define INTEL_IOMMU_H
+#include "qemu/queue.h"
 #include "hw/qdev.h"
 #include "sysemu/dma.h"
 
@@ -65,11 +66,12 @@ struct VTDContextCacheEntry {
 };
 
 struct VTDAddressSpace {
-    uint8_t bus_num;
+    PCIDevice *dev;
     uint8_t devfn;
     AddressSpace as;
     MemoryRegion iommu;
     IntelIOMMUState *iommu_state;
+    QLIST_ENTRY(VTDAddressSpace) iommu_next; /* List for traversal by the iommu */
     VTDContextCacheEntry context_cache_entry;
 };
 
@@ -114,7 +116,7 @@ struct IntelIOMMUState {
     GHashTable *iotlb;              /* IOTLB */
 
     MemoryRegionIOMMUOps iommu_ops;
-    VTDAddressSpace **address_spaces[VTD_PCI_BUS_MAX];
+    QLIST_HEAD(,VTDAddressSpace) address_spaces;
 };
 
 #endif
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index e142641..044a3aa 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -416,9 +416,10 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range);
 
 void pci_device_deassert_intx(PCIDevice *dev);
 
-typedef AddressSpace *(*PCIIOMMUFunc)(PCIDevice *, void *);
 void pci_set_dma_address_space(AddressSpace *dma_address_space);
 
+typedef AddressSpace *(*PCIIOMMUFunc)(PCIDevice *, void *);
+
 AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
 void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Marcel Apfelbaum]

On 09/01/2015 09:48 PM, Knut Omang wrote:
> This patch set changes the data structure used to handle address spaces within
> the emulated Intel iommu to support traversal also if bus numbers are dynamically
> allocated, as is the case for devices that sit behind root ports or downstream switches.
> This means that we cannot use bus number as index, instead a QLIST is used.
>
> This requires a change in the API for setup of IOMMUs which is taken care of by
> the first patch. The second patch implements the fix.
>
> The initial patch set had some discussion related to whether this fix, applied to the
> bridge code, was applicable to all bridges. No clear conclusion arised as far as I understood,
> in the meantime a number of people have run into the same issue as I did which lead me
> to implement this, so I gather it might be a useful intermediate solution that works until
> a better approach can be found? I believe the IOMMU emulation code has limited usefulness
> if it only supports devices sitting directly on the root complex.
Hi,

Thank you for (re)sending the patches!

While I believe you are perfectly right and IOMMU
would benefit from this addition, I saw that are some reviews
in the  prev thread that are not purely theoretical/philosophical.
E.g. PCIDevice *dev field in VTDAddressSpace and maybe a few more.

I would suggest to address them so it will be easier to continue the review process.

Thank you,
Marcel

>
> This is the thread following the initial patch set:
>
>    http://thread.gmane.org/gmane.comp.emulators.qemu/302246
>
> The patch set was also discussed in this thread:
>
>    http://thread.gmane.org/gmane.comp.emulators.qemu/316949
>
> Changes from v1:
>    - Rebased to current master
>    - Fixed minor syntax issues
>
> Knut Omang (2):
>    iommu: Replace bus+devfn arguments with PCIDevice* in PCIIOMMUFunc
>    intel_iommu: Add support for translation for devices behind bridges.
>
>   hw/alpha/typhoon.c            |  2 +-
>   hw/i386/intel_iommu.c         | 56 +++++++++++++++++++------------------------
>   hw/pci-host/apb.c             |  2 +-
>   hw/pci-host/prep.c            |  3 +--
>   hw/pci-host/q35.c             | 42 +++++++++++++-------------------
>   hw/pci/pci.c                  |  7 +++---
>   hw/pci/pci_bridge.c           |  6 +++++
>   hw/ppc/spapr_pci.c            |  2 +-
>   include/hw/i386/intel_iommu.h |  6 +++--
>   include/hw/pci/pci.h          |  5 +++-
>   10 files changed, 62 insertions(+), 69 deletions(-)
>
> --
> 2.4.3
>

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Knut Omang]

On Wed, 2015-09-02 at 15:30 +0300, Marcel Apfelbaum wrote:
> On 09/01/2015 09:48 PM, Knut Omang wrote:
> > This patch set changes the data structure used to handle address 
> > spaces within
> > the emulated Intel iommu to support traversal also if bus numbers 
> > are dynamically
> > allocated, as is the case for devices that sit behind root ports or 
> > downstream switches.
> > This means that we cannot use bus number as index, instead a QLIST 
> > is used.
> > 
> > This requires a change in the API for setup of IOMMUs which is 
> > taken care of by
> > the first patch. The second patch implements the fix.
> > 
> > The initial patch set had some discussion related to whether this 
> > fix, applied to the
> > bridge code, was applicable to all bridges. No clear conclusion 
> > arised as far as I understood,
> > in the meantime a number of people have run into the same issue as 
> > I did which lead me
> > to implement this, so I gather it might be a useful intermediate 
> > solution that works until
> > a better approach can be found? I believe the IOMMU emulation code 
> > has limited usefulness
> > if it only supports devices sitting directly on the root complex.
> Hi,
> 
> Thank you for (re)sending the patches!
> 
> While I believe you are perfectly right and IOMMU
> would benefit from this addition, I saw that are some reviews
> in the  prev thread that are not purely theoretical/philosophical.
> E.g. PCIDevice *dev field in VTDAddressSpace and maybe a few more.

Yes, in principle I agree, but the problem is that there is no other
good reference that would uniquely identify the device as the requester
IDs (devfn) cannot be used since it changes during enumeration, 
as argued better for here:

http://article.gmane.org/gmane.comp.emulators.qemu/317201

> I would suggest to address them so it will be easier to continue the 
> review process.

Knut

> Thank you,
> Marcel
> 
> > 
> > This is the thread following the initial patch set:
> > 
> >    http://thread.gmane.org/gmane.comp.emulators.qemu/302246
> > 
> > The patch set was also discussed in this thread:
> > 
> >    http://thread.gmane.org/gmane.comp.emulators.qemu/316949
> > 
> > Changes from v1:
> >    - Rebased to current master
> >    - Fixed minor syntax issues
> > 
> > Knut Omang (2):
> >    iommu: Replace bus+devfn arguments with PCIDevice* in 
> > PCIIOMMUFunc
> >    intel_iommu: Add support for translation for devices behind 
> > bridges.
> > 
> >   hw/alpha/typhoon.c            |  2 +-
> >   hw/i386/intel_iommu.c         | 56 +++++++++++++++++++-----------
> > -------------
> >   hw/pci-host/apb.c             |  2 +-
> >   hw/pci-host/prep.c            |  3 +--
> >   hw/pci-host/q35.c             | 42 +++++++++++++-----------------
> > --
> >   hw/pci/pci.c                  |  7 +++---
> >   hw/pci/pci_bridge.c           |  6 +++++
> >   hw/ppc/spapr_pci.c            |  2 +-
> >   include/hw/i386/intel_iommu.h |  6 +++--
> >   include/hw/pci/pci.h          |  5 +++-
> >   10 files changed, 62 insertions(+), 69 deletions(-)
> > 
> > --
> > 2.4.3
> > 
> 

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Alex Williamson]

On Wed, 2015-09-02 at 15:10 +0200, Knut Omang wrote:
> On Wed, 2015-09-02 at 15:30 +0300, Marcel Apfelbaum wrote:
> > On 09/01/2015 09:48 PM, Knut Omang wrote:
> > > This patch set changes the data structure used to handle address 
> > > spaces within
> > > the emulated Intel iommu to support traversal also if bus numbers 
> > > are dynamically
> > > allocated, as is the case for devices that sit behind root ports or 
> > > downstream switches.
> > > This means that we cannot use bus number as index, instead a QLIST 
> > > is used.
> > > 
> > > This requires a change in the API for setup of IOMMUs which is 
> > > taken care of by
> > > the first patch. The second patch implements the fix.
> > > 
> > > The initial patch set had some discussion related to whether this 
> > > fix, applied to the
> > > bridge code, was applicable to all bridges. No clear conclusion 
> > > arised as far as I understood,
> > > in the meantime a number of people have run into the same issue as 
> > > I did which lead me
> > > to implement this, so I gather it might be a useful intermediate 
> > > solution that works until
> > > a better approach can be found? I believe the IOMMU emulation code 
> > > has limited usefulness
> > > if it only supports devices sitting directly on the root complex.
> > Hi,
> > 
> > Thank you for (re)sending the patches!
> > 
> > While I believe you are perfectly right and IOMMU
> > would benefit from this addition, I saw that are some reviews
> > in the  prev thread that are not purely theoretical/philosophical.
> > E.g. PCIDevice *dev field in VTDAddressSpace and maybe a few more.
> 
> Yes, in principle I agree, but the problem is that there is no other
> good reference that would uniquely identify the device as the requester
> IDs (devfn) cannot be used since it changes during enumeration, 
> as argued better for here:
> 
> http://article.gmane.org/gmane.comp.emulators.qemu/317201

There are very specific rules for translating requester IDs across
bridges.  Bus numbers can change during enumeration, devfn cannot.
devfn can however be masked by topology changes from PCIe to PCI.  If we
pretend that the IOMMU can distinguish requester IDs where it can't on
real hardware, we're going to break the guest.  Thanks,

Alex

> > I would suggest to address them so it will be easier to continue the 
> > review process.
> 
> Knut
> 
> > Thank you,
> > Marcel
> > 
> > > 
> > > This is the thread following the initial patch set:
> > > 
> > >    http://thread.gmane.org/gmane.comp.emulators.qemu/302246
> > > 
> > > The patch set was also discussed in this thread:
> > > 
> > >    http://thread.gmane.org/gmane.comp.emulators.qemu/316949
> > > 
> > > Changes from v1:
> > >    - Rebased to current master
> > >    - Fixed minor syntax issues
> > > 
> > > Knut Omang (2):
> > >    iommu: Replace bus+devfn arguments with PCIDevice* in 
> > > PCIIOMMUFunc
> > >    intel_iommu: Add support for translation for devices behind 
> > > bridges.
> > > 
> > >   hw/alpha/typhoon.c            |  2 +-
> > >   hw/i386/intel_iommu.c         | 56 +++++++++++++++++++-----------
> > > -------------
> > >   hw/pci-host/apb.c             |  2 +-
> > >   hw/pci-host/prep.c            |  3 +--
> > >   hw/pci-host/q35.c             | 42 +++++++++++++-----------------
> > > --
> > >   hw/pci/pci.c                  |  7 +++---
> > >   hw/pci/pci_bridge.c           |  6 +++++
> > >   hw/ppc/spapr_pci.c            |  2 +-
> > >   include/hw/i386/intel_iommu.h |  6 +++--
> > >   include/hw/pci/pci.h          |  5 +++-
> > >   10 files changed, 62 insertions(+), 69 deletions(-)
> > > 
> > > --
> > > 2.4.3
> > > 
> > 

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Benjamin Herrenschmidt]

On Wed, 2015-09-02 at 10:12 -0600, Alex Williamson wrote:

> There are very specific rules for translating requester IDs across
> bridges.  Bus numbers can change during enumeration, devfn cannot.
> devfn can however be masked by topology changes from PCIe to PCI.  If 
> we pretend that the IOMMU can distinguish requester IDs where it 
> can't on real hardware, we're going to break the guest.  Thanks,

Note that whether a PCI / PCI-X bridge will mask devfn, bus# or both or
even mask it partially (number of bits) or replace some transfers with
its own RID ... depends on a given bridge implementation.

Another thing is while I agree that the bus number is problematic,
since it changes, it is still what the HW actually uses to match the
requester in practice, at least on PHB and I would think on Intel.

The problem is more fundamental. qemu is trying to bind devices to
address spaces in a fixed way at device creation time, while this is
lazily resolved in HW at the point of the DMA occurring.

One way to fix it is to effectively have an address space per device,
and have the iommu translate function figure out the binding
dynamically and flush things if it detects a change. But that is tricky
for vfio and it means invalidations will have to iterate all address
spaces.

The other option is to create Address Spaces on the fly as we lookup
domains, and bind them to devices lazily, but again, we need to deal
with changes/invalidations and that can be nasty with VFIO.

Sadly, I can't think of a silver bullet.

Cheers,
Ben.

> Alex
> 
> > > I would suggest to address them so it will be easier to continue
> > > the 
> > > review process.
> > 
> > Knut
> > 
> > > Thank you,
> > > Marcel
> > > 
> > > > 
> > > > This is the thread following the initial patch set:
> > > > 
> > > >    http://thread.gmane.org/gmane.comp.emulators.qemu/302246
> > > > 
> > > > The patch set was also discussed in this thread:
> > > > 
> > > >    http://thread.gmane.org/gmane.comp.emulators.qemu/316949
> > > > 
> > > > Changes from v1:
> > > >    - Rebased to current master
> > > >    - Fixed minor syntax issues
> > > > 
> > > > Knut Omang (2):
> > > >    iommu: Replace bus+devfn arguments with PCIDevice* in 
> > > > PCIIOMMUFunc
> > > >    intel_iommu: Add support for translation for devices behind 
> > > > bridges.
> > > > 
> > > >   hw/alpha/typhoon.c            |  2 +-
> > > >   hw/i386/intel_iommu.c         | 56 +++++++++++++++++++-------
> > > > ----
> > > > -------------
> > > >   hw/pci-host/apb.c             |  2 +-
> > > >   hw/pci-host/prep.c            |  3 +--
> > > >   hw/pci-host/q35.c             | 42 +++++++++++++-------------
> > > > ----
> > > > --
> > > >   hw/pci/pci.c                  |  7 +++---
> > > >   hw/pci/pci_bridge.c           |  6 +++++
> > > >   hw/ppc/spapr_pci.c            |  2 +-
> > > >   include/hw/i386/intel_iommu.h |  6 +++--
> > > >   include/hw/pci/pci.h          |  5 +++-
> > > >   10 files changed, 62 insertions(+), 69 deletions(-)
> > > > 
> > > > --
> > > > 2.4.3
> > > > 
> > > 
> 
> 

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Knut Omang]

On Thu, 2015-09-03 at 08:33 +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2015-09-02 at 10:12 -0600, Alex Williamson wrote:
> 
> > There are very specific rules for translating requester IDs across
> > bridges.  Bus numbers can change during enumeration, devfn cannot.

Thanks for clarifying that point, Alex, I realize I was a bit imprecise
in my last mail,

> > devfn can however be masked by topology changes from PCIe to PCI.  If 
> > we pretend that the IOMMU can distinguish requester IDs where it 
> > can't on real hardware, we're going to break the guest.  Thanks,
> 
> Note that whether a PCI / PCI-X bridge will mask devfn, bus# or both or
> even mask it partially (number of bits) or replace some transfers with
> its own RID ... depends on a given bridge implementation.
> 
> Another thing is while I agree that the bus number is problematic,
> since it changes, it is still what the HW actually uses to match the
> requester in practice, at least on PHB and I would think on Intel.
> 
> The problem is more fundamental. qemu is trying to bind devices to
> address spaces in a fixed way at device creation time, while this is
> lazily resolved in HW at the point of the DMA occurring.

So let me try to sum up my understanding in context of the patch in
terms of these two approaches,

> One way to fix it is to effectively have an address space per device,
> and have the iommu translate function figure out the binding
> dynamically and flush things if it detects a change. But that is tricky
> for vfio and it means invalidations will have to iterate all address
> spaces.

So my patch is along these lines by actually moving the address space
pointer into the device struct.
The benefit is that:
  * The data structure for the DMA address space can be reused across 
    IOMMUs, and the address spaces can be set up before bus numbers are
    
    assigned, and the implementation is fairly simple.
  * The IOMMU does not have to be notified of bus changes, except for
    invalidation purposes (but wouldn't a new enumeration cause a full 
    IOMMU invalidate anyway?)

The drawbacks are:
  * The IOMMUs get to know explicitly about devices behind a bridge, 
    which logically deviates from how hardware works and 
    complicates future attempts to implement bridges that 
    translate RIDs.
  * Each device can have only one DMA address space mapping associated 
    with it (I suppose it might be possible to have a topology that 
    would allow multiple paths to a device, but do we care at this 
    stage?)

> The other option is to create Address Spaces on the fly as we lookup
> domains, and bind them to devices lazily, but again, we need to deal
> with changes/invalidations and that can be nasty with VFIO.

We could get here without changing the interfaces, by refining the
current implementation to just cache bus pointers at setup, then lazily
add address spaces for each device. This approach would yield IOMMU
device specific implementations, but would still in practice associate
devices with address spaces.

> Sadly, I can't think of a silver bullet.

I agree,

The latter approach is better handled restarting from the current code,
as my patch depends too much on the interface change.

Thanks,
Knut

> Cheers,
> Ben.
> 
> > Alex
> > 
> > > > I would suggest to address them so it will be easier to continue
> > > > the 
> > > > review process.
> > > 
> > > Knut
> > > 
> > > > Thank you,
> > > > Marcel
> > > > 
> > > > > 
> > > > > This is the thread following the initial patch set:
> > > > > 
> > > > >    http://thread.gmane.org/gmane.comp.emulators.qemu/302246
> > > > > 
> > > > > The patch set was also discussed in this thread:
> > > > > 
> > > > >    http://thread.gmane.org/gmane.comp.emulators.qemu/316949
> > > > > 
> > > > > Changes from v1:
> > > > >    - Rebased to current master
> > > > >    - Fixed minor syntax issues
> > > > > 
> > > > > Knut Omang (2):
> > > > >    iommu: Replace bus+devfn arguments with PCIDevice* in 
> > > > > PCIIOMMUFunc
> > > > >    intel_iommu: Add support for translation for devices behind 
> > > > > bridges.
> > > > > 
> > > > >   hw/alpha/typhoon.c            |  2 +-
> > > > >   hw/i386/intel_iommu.c         | 56 +++++++++++++++++++-------
> > > > > ----
> > > > > -------------
> > > > >   hw/pci-host/apb.c             |  2 +-
> > > > >   hw/pci-host/prep.c            |  3 +--
> > > > >   hw/pci-host/q35.c             | 42 +++++++++++++-------------
> > > > > ----
> > > > > --
> > > > >   hw/pci/pci.c                  |  7 +++---
> > > > >   hw/pci/pci_bridge.c           |  6 +++++
> > > > >   hw/ppc/spapr_pci.c            |  2 +-
> > > > >   include/hw/i386/intel_iommu.h |  6 +++--
> > > > >   include/hw/pci/pci.h          |  5 +++-
> > > > >   10 files changed, 62 insertions(+), 69 deletions(-)
> > > > > 
> > > > > --
> > > > > 2.4.3
> > > > > 
> > > > 
> > 
> > 

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Knut Omang]

On Thu, 2015-09-03 at 07:26 +0200, Knut Omang wrote:
> On Thu, 2015-09-03 at 08:33 +1000, Benjamin Herrenschmidt wrote:
> > On Wed, 2015-09-02 at 10:12 -0600, Alex Williamson wrote:
> > 
> > > There are very specific rules for translating requester IDs across
> > > bridges.  Bus numbers can change during enumeration, devfn cannot.
> 
> Thanks for clarifying that point, Alex, I realize I was a bit imprecise
> in my last mail,
> 
> > > devfn can however be masked by topology changes from PCIe to PCI.  If 
> > > we pretend that the IOMMU can distinguish requester IDs where it 
> > > can't on real hardware, we're going to break the guest.  Thanks,
> > 
> > Note that whether a PCI / PCI-X bridge will mask devfn, bus# or both or
> > even mask it partially (number of bits) or replace some transfers with
> > its own RID ... depends on a given bridge implementation.
> > 
> > Another thing is while I agree that the bus number is problematic,
> > since it changes, it is still what the HW actually uses to match the
> > requester in practice, at least on PHB and I would think on Intel.
> > 
> > The problem is more fundamental. qemu is trying to bind devices to
> > address spaces in a fixed way at device creation time, while this is
> > lazily resolved in HW at the point of the DMA occurring.
> 
> So let me try to sum up my understanding in context of the patch in
> terms of these two approaches,
> 
> > One way to fix it is to effectively have an address space per device,
> > and have the iommu translate function figure out the binding
> > dynamically and flush things if it detects a change. But that is tricky
> > for vfio and it means invalidations will have to iterate all address
> > spaces.
> 
> So my patch is along these lines by actually moving the address space
> pointer into the device struct.
> The benefit is that:
>   * The data structure for the DMA address space can be reused across 
>     IOMMUs, and the address spaces can be set up before bus numbers are
>     
>     assigned, and the implementation is fairly simple.
>   * The IOMMU does not have to be notified of bus changes, except for
>     invalidation purposes (but wouldn't a new enumeration cause a full 
>     IOMMU invalidate anyway?)
> 
> The drawbacks are:
>   * The IOMMUs get to know explicitly about devices behind a bridge, 
>     which logically deviates from how hardware works and 
>     complicates future attempts to implement bridges that 
>     translate RIDs.
>   * Each device can have only one DMA address space mapping associated 
>     with it (I suppose it might be possible to have a topology that 
>     would allow multiple paths to a device, but do we care at this 
>     stage?)
> 
> > The other option is to create Address Spaces on the fly as we lookup
> > domains, and bind them to devices lazily, but again, we need to deal
> > with changes/invalidations and that can be nasty with VFIO.
> 
> We could get here without changing the interfaces, by refining the
> current implementation to just cache bus pointers at setup, then lazily
> add address spaces for each device. This approach would yield IOMMU
> device specific implementations, but would still in practice associate
> devices with address spaces.

As the thread went silent after our conclusions, I have made a second
implementation for the Intel IOMMU according to this alternate scheme,
It keeps the current API and handles the bus number resolution lazily
within the IOMMU implementation, I will post the (single) patch as v3
of this. 

Hopefully this is acceptable and can be leveraged to do a similar
rework, or be abstracted as generic functionality (?) for the other
architectures,..

Thanks,

Knut

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Benjamin Herrenschmidt]

On Sat, 2015-09-12 at 20:37 +0200, Knut Omang wrote:
> As the thread went silent after our conclusions, I have made a second
> implementation for the Intel IOMMU according to this alternate scheme,
> It keeps the current API and handles the bus number resolution lazily
> within the IOMMU implementation, I will post the (single) patch as v3
> of this. 
> 
> Hopefully this is acceptable and can be leveraged to do a similar
> rework, or be abstracted as generic functionality (?) for the other
> architectures,..

Ah sorry, I meant to look at your email in more details and respond but
it fell through the cracks.

I'm happy to have a look at your work and see how it applies to me, you
can see my powernv code which also supports translation for devices
behind bridges here (but doesn't do as much caching as q35 does):

https://github.com/ozbenh/qemu/commit/4e0ed1002f98fd97aa7ca3a48c74933d0343dd42

Which depends on:

https://github.com/ozbenh/qemu/commit/facedeba8811985ca20ac3dbad5d07e1a10ea9b2

(Which I think Michael merged recently, I haven't checked).

Cheers,
Ben.

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: Add support for translation for devices behind bridges

[Knut Omang]

On Sun, 2015-09-13 at 09:12 +1000, Benjamin Herrenschmidt wrote:
> On Sat, 2015-09-12 at 20:37 +0200, Knut Omang wrote:
> > As the thread went silent after our conclusions, I have made a
> > second
> > implementation for the Intel IOMMU according to this alternate
> > scheme,
> > It keeps the current API and handles the bus number resolution
> > lazily
> > within the IOMMU implementation, I will post the (single) patch as
> > v3
> > of this. 
> > 
> > Hopefully this is acceptable and can be leveraged to do a similar
> > rework, or be abstracted as generic functionality (?) for the other
> > architectures,..
> 
> Ah sorry, I meant to look at your email in more details and respond
> but
> it fell through the cracks.

I know how it is.. :-)

> I'm happy to have a look at your work and see how it applies to me,
> you
> can see my powernv code which also supports translation for devices
> behind bridges here (but doesn't do as much caching as q35 does):
> 
> https://github.com/ozbenh/qemu/commit/4e0ed1002f98fd97aa7ca3a48c74933
> d0343dd42

Looks conceptually similar - the caching part is probably not that
important, it just came natural as it evolved from the original
implementation. We are probably so far talking about max a few buses
and a few devices per bus, I suppose.

I suppose the best then now would be to stick to your suggestion of
getting the functionality in and let it mature, then look for any
optimization whether structural or performance wise,

> Which depends on:
> 
> https://github.com/ozbenh/qemu/commit/facedeba8811985ca20ac3dbad5d07e
> 1a10ea9b2
> 
> (Which I think Michael merged recently, I haven't checked).

Yes, I noticed while rebasing.

> Cheers,
> Ben.

Cheers,
Knut