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

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

[Knut Omang]

This patch set has been completely reimplemented according to ideas from the
discussion of v2.

It still solves the same problem, but does so only within the Intel IOMMU code and Q35,
without changing the IOMMU interface. This eliminates the need for any separate
interface change patch.

This is the thread following v2 of the patch set:

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

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 v3:
  - Replaced use of g_hash_table_add with g_hash_table_insert
    to support compiling on older versions of glib.

Changes from v2:
  - Completely reimplemented fix to avoid API change and further
    logical deviation from how hardware works.
    API change no longer necessary, so just a single patch.

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

Knut Omang (1):
  intel_iommu: Add support for translation for devices behind bridges

 hw/i386/intel_iommu.c         | 90 +++++++++++++++++++++++++++++++++++--------
 hw/pci-host/q35.c             | 25 ++----------
 include/hw/i386/intel_iommu.h | 16 +++++++-
 3 files changed, 91 insertions(+), 40 deletions(-)

--
2.4.3

[Qemu-devel] [PATCH v4 1/1] intel_iommu: Add support for translation for devices behind bridges

[Knut Omang]

- Use a hash table indexed on bus pointers to store information about buses
  instead of using the bus numbers.
  Bus pointers are stored in a new VTDBus struct together with the vector
  of device address space pointers indexed by devfn.
- The bus number is still used for lookup for selective SID based invalidate,
  in which case the bus number is lazily resolved from the bus hash table and
  cached in a separate index.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 hw/i386/intel_iommu.c         | 90 +++++++++++++++++++++++++++++++++++--------
 hw/pci-host/q35.c             | 25 ++----------
 include/hw/i386/intel_iommu.h | 16 +++++++-
 3 files changed, 91 insertions(+), 40 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 08055a8..d677a28 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -22,6 +22,7 @@
 #include "hw/sysbus.h"
 #include "exec/address-spaces.h"
 #include "intel_iommu_internal.h"
+#include "hw/pci/pci.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -166,19 +167,17 @@ 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;
+    VTDBus *vtd_bus;
+    GHashTableIter bus_it;
     uint32_t devfn_it;
 
+    g_hash_table_iter_init(&bus_it, s->vtd_as_by_busptr);
+
     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;
-        }
+    while (g_hash_table_iter_next (&bus_it, NULL, (void**)&vtd_bus)) {
         for (devfn_it = 0; devfn_it < VTD_PCI_DEVFN_MAX; ++devfn_it) {
-            vtd_as = pvtd_as[devfn_it];
+            vtd_as = vtd_bus->dev_as[devfn_it];
             if (!vtd_as) {
                 continue;
             }
@@ -754,12 +753,13 @@ static inline bool vtd_is_interrupt_addr(hwaddr addr)
  * @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,
+static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
                                    uint8_t devfn, hwaddr addr, bool is_write,
                                    IOMMUTLBEntry *entry)
 {
     IntelIOMMUState *s = vtd_as->iommu_state;
     VTDContextEntry ce;
+    uint8_t bus_num = pci_bus_num(bus);
     VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;
     uint64_t slpte;
     uint32_t level;
@@ -874,6 +874,30 @@ static void vtd_context_global_invalidate(IntelIOMMUState *s)
     }
 }
 
+
+/* Find the VTD address space currently associated with a given bus number,
+ */
+static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
+{
+    VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
+    if (!vtd_bus) {
+        /* Iterate over the registered buses to find the one
+         * which currently hold this bus number, and update the bus_num lookup table:
+         */
+        GHashTableIter iter;
+        uint64_t key;
+
+        g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
+        while (g_hash_table_iter_next (&iter, (void**)&key, (void**)&vtd_bus)) {
+            if (pci_bus_num(vtd_bus->bus) == bus_num) {
+                s->vtd_as_by_bus_num[bus_num] = vtd_bus;
+                return vtd_bus;
+            }
+        }
+    }
+    return vtd_bus;
+}
+
 /* Do a context-cache device-selective invalidation.
  * @func_mask: FM field after shifting
  */
@@ -882,7 +906,7 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,
                                           uint16_t func_mask)
 {
     uint16_t mask;
-    VTDAddressSpace **pvtd_as;
+    VTDBus *vtd_bus;
     VTDAddressSpace *vtd_as;
     uint16_t devfn;
     uint16_t devfn_it;
@@ -903,11 +927,11 @@ 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) {
+    vtd_bus = vtd_find_as_from_bus_num(s, VTD_SID_TO_BUS(source_id));
+    if (vtd_bus) {
         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];
+            vtd_as = vtd_bus->dev_as[devfn_it];
             if (vtd_as && ((devfn_it & mask) == (devfn & mask))) {
                 VTD_DPRINTF(INV, "invalidate context-cahce of devfn 0x%"PRIx16,
                             devfn_it);
@@ -1805,11 +1829,11 @@ 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,
+    vtd_do_iommu_translate(vtd_as, vtd_as->bus, vtd_as->devfn, 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,
+                " gpa 0x%"PRIx64 " hpa 0x%"PRIx64, pci_bus_num(vtd_as->bus),
                 VTD_PCI_SLOT(vtd_as->devfn), VTD_PCI_FUNC(vtd_as->devfn),
                 vtd_as->devfn, addr, ret.translated_addr);
     return ret;
@@ -1839,6 +1863,38 @@ static Property vtd_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+
+VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
+{
+    uint64_t key = (uint64_t)bus;
+    VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key);
+    VTDAddressSpace *vtd_dev_as;
+
+    if (!vtd_bus) {
+        /* No corresponding free() */
+        vtd_bus = g_malloc0(sizeof(VTDBus) + sizeof(VTDAddressSpace *) * VTD_PCI_DEVFN_MAX);
+        vtd_bus->bus = bus;
+        key = (uint64_t)bus;
+        g_hash_table_insert(s->vtd_as_by_busptr, &key, vtd_bus);
+    }
+
+    vtd_dev_as = vtd_bus->dev_as[devfn];
+
+    if (!vtd_dev_as) {
+        vtd_bus->dev_as[devfn] = vtd_dev_as = g_malloc0(sizeof(VTDAddressSpace));
+
+        vtd_dev_as->bus = bus;
+        vtd_dev_as->devfn = (uint8_t)devfn;
+        vtd_dev_as->iommu_state = s;
+        vtd_dev_as->context_cache_entry.context_cache_gen = 0;
+        memory_region_init_iommu(&vtd_dev_as->iommu, OBJECT(s),
+                                 &s->iommu_ops, "intel_iommu", UINT64_MAX);
+        address_space_init(&vtd_dev_as->as,
+                           &vtd_dev_as->iommu, "intel_iommu");
+    }
+    return vtd_dev_as;
+}
+
 /* Do the initialization. It will also be called when reset, so pay
  * attention when adding new initialization stuff.
  */
@@ -1931,13 +1987,15 @@ 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));
+    memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
     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);
     /* No corresponding destroy */
     s->iotlb = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
                                      g_free, g_free);
+    s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
+                                              g_free, g_free);
     vtd_init(s);
 }
 
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index bd74094..c81507d 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -426,31 +426,12 @@ static void mch_reset(DeviceState *qdev)
 static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 {
     IntelIOMMUState *s = opaque;
-    VTDAddressSpace **pvtd_as;
-    int bus_num = pci_bus_num(bus);
+    VTDAddressSpace *vtd_as;
 
-    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),
-                                 &s->iommu_ops, "intel_iommu", UINT64_MAX);
-        address_space_init(&pvtd_as[devfn]->as,
-                           &pvtd_as[devfn]->iommu, "intel_iommu");
-    }
-    return &pvtd_as[devfn]->as;
+    vtd_as = vtd_find_add_as(s, bus, devfn);
+    return &vtd_as->as;
 }
 
 static void mch_init_dmar(MCHPCIState *mch)
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index e321ee4..5dbadb7 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -49,6 +49,7 @@ typedef struct VTDContextCacheEntry VTDContextCacheEntry;
 typedef struct IntelIOMMUState IntelIOMMUState;
 typedef struct VTDAddressSpace VTDAddressSpace;
 typedef struct VTDIOTLBEntry VTDIOTLBEntry;
+typedef struct VTDBus VTDBus;
 
 /* Context-Entry */
 struct VTDContextEntry {
@@ -65,7 +66,7 @@ struct VTDContextCacheEntry {
 };
 
 struct VTDAddressSpace {
-    uint8_t bus_num;
+    PCIBus *bus;
     uint8_t devfn;
     AddressSpace as;
     MemoryRegion iommu;
@@ -73,6 +74,11 @@ struct VTDAddressSpace {
     VTDContextCacheEntry context_cache_entry;
 };
 
+struct VTDBus {
+    PCIBus* bus;		/* A reference to the bus to provide translation for */
+    VTDAddressSpace *dev_as[0];	/* A table of VTDAddressSpace objects indexed by devfn */
+};
+
 struct VTDIOTLBEntry {
     uint64_t gfn;
     uint16_t domain_id;
@@ -114,7 +120,13 @@ struct IntelIOMMUState {
     GHashTable *iotlb;              /* IOTLB */
 
     MemoryRegionIOMMUOps iommu_ops;
-    VTDAddressSpace **address_spaces[VTD_PCI_BUS_MAX];
+    GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by PCIBus* reference */
+    VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */
 };
 
+/* Find the VTD Address space associated with the given bus pointer,
+ * create a new one if none exists
+ */
+VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn);
+
 #endif
-- 
2.4.3

Re: [Qemu-devel] [PATCH v4 1/1] intel_iommu: Add support for translation for devices behind bridges

[Michael S. Tsirkin]

On Sat, Sep 26, 2015 at 08:09:56AM +0200, Knut Omang wrote:
> - Use a hash table indexed on bus pointers to store information about buses
>   instead of using the bus numbers.
>   Bus pointers are stored in a new VTDBus struct together with the vector
>   of device address space pointers indexed by devfn.
> - The bus number is still used for lookup for selective SID based invalidate,
>   in which case the bus number is lazily resolved from the bus hash table and
>   cached in a separate index.
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>

Fails on 32 bit:
/scm/qemu/hw/i386/intel_iommu.c: In function ‘vtd_find_add_as’:
/scm/qemu/hw/i386/intel_iommu.c:1869:20: error: cast from pointer to
integer of different size [-Werror=pointer-to-int-cast]
     uint64_t key = (uint64_t)bus;
                    ^
/scm/qemu/hw/i386/intel_iommu.c:1877:15: error: cast from pointer to
integer of different size [-Werror=pointer-to-int-cast]
         key = (uint64_t)bus;
               ^

You need to cast things to uintptr_t.

> ---
>  hw/i386/intel_iommu.c         | 90 +++++++++++++++++++++++++++++++++++--------
>  hw/pci-host/q35.c             | 25 ++----------
>  include/hw/i386/intel_iommu.h | 16 +++++++-
>  3 files changed, 91 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 08055a8..d677a28 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -22,6 +22,7 @@
>  #include "hw/sysbus.h"
>  #include "exec/address-spaces.h"
>  #include "intel_iommu_internal.h"
> +#include "hw/pci/pci.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -166,19 +167,17 @@ 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;
> +    VTDBus *vtd_bus;
> +    GHashTableIter bus_it;
>      uint32_t devfn_it;
>  
> +    g_hash_table_iter_init(&bus_it, s->vtd_as_by_busptr);
> +
>      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;
> -        }
> +    while (g_hash_table_iter_next (&bus_it, NULL, (void**)&vtd_bus)) {
>          for (devfn_it = 0; devfn_it < VTD_PCI_DEVFN_MAX; ++devfn_it) {
> -            vtd_as = pvtd_as[devfn_it];
> +            vtd_as = vtd_bus->dev_as[devfn_it];
>              if (!vtd_as) {
>                  continue;
>              }
> @@ -754,12 +753,13 @@ static inline bool vtd_is_interrupt_addr(hwaddr addr)
>   * @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,
> +static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
>                                     uint8_t devfn, hwaddr addr, bool is_write,
>                                     IOMMUTLBEntry *entry)
>  {
>      IntelIOMMUState *s = vtd_as->iommu_state;
>      VTDContextEntry ce;
> +    uint8_t bus_num = pci_bus_num(bus);
>      VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;
>      uint64_t slpte;
>      uint32_t level;
> @@ -874,6 +874,30 @@ static void vtd_context_global_invalidate(IntelIOMMUState *s)
>      }
>  }
>  
> +
> +/* Find the VTD address space currently associated with a given bus number,
> + */
> +static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s, uint8_t bus_num)
> +{
> +    VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
> +    if (!vtd_bus) {
> +        /* Iterate over the registered buses to find the one
> +         * which currently hold this bus number, and update the bus_num lookup table:
> +         */
> +        GHashTableIter iter;
> +        uint64_t key;
> +
> +        g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
> +        while (g_hash_table_iter_next (&iter, (void**)&key, (void**)&vtd_bus)) {
> +            if (pci_bus_num(vtd_bus->bus) == bus_num) {
> +                s->vtd_as_by_bus_num[bus_num] = vtd_bus;
> +                return vtd_bus;
> +            }
> +        }
> +    }
> +    return vtd_bus;
> +}
> +
>  /* Do a context-cache device-selective invalidation.
>   * @func_mask: FM field after shifting
>   */
> @@ -882,7 +906,7 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s,
>                                            uint16_t func_mask)
>  {
>      uint16_t mask;
> -    VTDAddressSpace **pvtd_as;
> +    VTDBus *vtd_bus;
>      VTDAddressSpace *vtd_as;
>      uint16_t devfn;
>      uint16_t devfn_it;
> @@ -903,11 +927,11 @@ 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) {
> +    vtd_bus = vtd_find_as_from_bus_num(s, VTD_SID_TO_BUS(source_id));
> +    if (vtd_bus) {
>          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];
> +            vtd_as = vtd_bus->dev_as[devfn_it];
>              if (vtd_as && ((devfn_it & mask) == (devfn & mask))) {
>                  VTD_DPRINTF(INV, "invalidate context-cahce of devfn 0x%"PRIx16,
>                              devfn_it);
> @@ -1805,11 +1829,11 @@ 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,
> +    vtd_do_iommu_translate(vtd_as, vtd_as->bus, vtd_as->devfn, 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,
> +                " gpa 0x%"PRIx64 " hpa 0x%"PRIx64, pci_bus_num(vtd_as->bus),
>                  VTD_PCI_SLOT(vtd_as->devfn), VTD_PCI_FUNC(vtd_as->devfn),
>                  vtd_as->devfn, addr, ret.translated_addr);
>      return ret;
> @@ -1839,6 +1863,38 @@ static Property vtd_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +
> +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
> +{
> +    uint64_t key = (uint64_t)bus;
> +    VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key);
> +    VTDAddressSpace *vtd_dev_as;
> +
> +    if (!vtd_bus) {
> +        /* No corresponding free() */
> +        vtd_bus = g_malloc0(sizeof(VTDBus) + sizeof(VTDAddressSpace *) * VTD_PCI_DEVFN_MAX);
> +        vtd_bus->bus = bus;
> +        key = (uint64_t)bus;
> +        g_hash_table_insert(s->vtd_as_by_busptr, &key, vtd_bus);
> +    }
> +
> +    vtd_dev_as = vtd_bus->dev_as[devfn];
> +
> +    if (!vtd_dev_as) {
> +        vtd_bus->dev_as[devfn] = vtd_dev_as = g_malloc0(sizeof(VTDAddressSpace));
> +
> +        vtd_dev_as->bus = bus;
> +        vtd_dev_as->devfn = (uint8_t)devfn;
> +        vtd_dev_as->iommu_state = s;
> +        vtd_dev_as->context_cache_entry.context_cache_gen = 0;
> +        memory_region_init_iommu(&vtd_dev_as->iommu, OBJECT(s),
> +                                 &s->iommu_ops, "intel_iommu", UINT64_MAX);
> +        address_space_init(&vtd_dev_as->as,
> +                           &vtd_dev_as->iommu, "intel_iommu");
> +    }
> +    return vtd_dev_as;
> +}
> +
>  /* Do the initialization. It will also be called when reset, so pay
>   * attention when adding new initialization stuff.
>   */
> @@ -1931,13 +1987,15 @@ 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));
> +    memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
>      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);
>      /* No corresponding destroy */
>      s->iotlb = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
>                                       g_free, g_free);
> +    s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
> +                                              g_free, g_free);
>      vtd_init(s);
>  }
>  
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index bd74094..c81507d 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -426,31 +426,12 @@ static void mch_reset(DeviceState *qdev)
>  static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>  {
>      IntelIOMMUState *s = opaque;
> -    VTDAddressSpace **pvtd_as;
> -    int bus_num = pci_bus_num(bus);
> +    VTDAddressSpace *vtd_as;
>  
> -    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),
> -                                 &s->iommu_ops, "intel_iommu", UINT64_MAX);
> -        address_space_init(&pvtd_as[devfn]->as,
> -                           &pvtd_as[devfn]->iommu, "intel_iommu");
> -    }
> -    return &pvtd_as[devfn]->as;
> +    vtd_as = vtd_find_add_as(s, bus, devfn);
> +    return &vtd_as->as;
>  }
>  
>  static void mch_init_dmar(MCHPCIState *mch)
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index e321ee4..5dbadb7 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -49,6 +49,7 @@ typedef struct VTDContextCacheEntry VTDContextCacheEntry;
>  typedef struct IntelIOMMUState IntelIOMMUState;
>  typedef struct VTDAddressSpace VTDAddressSpace;
>  typedef struct VTDIOTLBEntry VTDIOTLBEntry;
> +typedef struct VTDBus VTDBus;
>  
>  /* Context-Entry */
>  struct VTDContextEntry {
> @@ -65,7 +66,7 @@ struct VTDContextCacheEntry {
>  };
>  
>  struct VTDAddressSpace {
> -    uint8_t bus_num;
> +    PCIBus *bus;
>      uint8_t devfn;
>      AddressSpace as;
>      MemoryRegion iommu;
> @@ -73,6 +74,11 @@ struct VTDAddressSpace {
>      VTDContextCacheEntry context_cache_entry;
>  };
>  
> +struct VTDBus {
> +    PCIBus* bus;		/* A reference to the bus to provide translation for */
> +    VTDAddressSpace *dev_as[0];	/* A table of VTDAddressSpace objects indexed by devfn */
> +};
> +
>  struct VTDIOTLBEntry {
>      uint64_t gfn;
>      uint16_t domain_id;
> @@ -114,7 +120,13 @@ struct IntelIOMMUState {
>      GHashTable *iotlb;              /* IOTLB */
>  
>      MemoryRegionIOMMUOps iommu_ops;
> -    VTDAddressSpace **address_spaces[VTD_PCI_BUS_MAX];
> +    GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by PCIBus* reference */
> +    VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */
>  };
>  
> +/* Find the VTD Address space associated with the given bus pointer,
> + * create a new one if none exists
> + */
> +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn);
> +
>  #endif
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v4 1/1] intel_iommu: Add support for translation for devices behind bridges

[Knut Omang]

On Sun, 2015-09-27 at 13:07 +0300, Michael S. Tsirkin wrote:
> On Sat, Sep 26, 2015 at 08:09:56AM +0200, Knut Omang wrote:
> > - Use a hash table indexed on bus pointers to store information
> > about buses
> >   instead of using the bus numbers.
> >   Bus pointers are stored in a new VTDBus struct together with the
> > vector
> >   of device address space pointers indexed by devfn.
> > - The bus number is still used for lookup for selective SID based
> > invalidate,
> >   in which case the bus number is lazily resolved from the bus hash
> > table and
> >   cached in a separate index.
> > 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> 
> Fails on 32 bit:
> /scm/qemu/hw/i386/intel_iommu.c: In function ‘vtd_find_add_as’:
> /scm/qemu/hw/i386/intel_iommu.c:1869:20: error: cast from pointer to
> integer of different size [-Werror=pointer-to-int-cast]
>      uint64_t key = (uint64_t)bus;
>                     ^
> /scm/qemu/hw/i386/intel_iommu.c:1877:15: error: cast from pointer to
> integer of different size [-Werror=pointer-to-int-cast]
>          key = (uint64_t)bus;
>                ^
> 
> You need to cast things to uintptr_t.

Sorry - everything around me has become 64 bit these days, will be more
careful - I'll post a v5 with this.

Knut

> > ---
> >  hw/i386/intel_iommu.c         | 90
> > +++++++++++++++++++++++++++++++++++--------
> >  hw/pci-host/q35.c             | 25 ++----------
> >  include/hw/i386/intel_iommu.h | 16 +++++++-
> >  3 files changed, 91 insertions(+), 40 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 08055a8..d677a28 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -22,6 +22,7 @@
> >  #include "hw/sysbus.h"
> >  #include "exec/address-spaces.h"
> >  #include "intel_iommu_internal.h"
> > +#include "hw/pci/pci.h"
> >  
> >  /*#define DEBUG_INTEL_IOMMU*/
> >  #ifdef DEBUG_INTEL_IOMMU
> > @@ -166,19 +167,17 @@ 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;
> > +    VTDBus *vtd_bus;
> > +    GHashTableIter bus_it;
> >      uint32_t devfn_it;
> >  
> > +    g_hash_table_iter_init(&bus_it, s->vtd_as_by_busptr);
> > +
> >      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;
> > -        }
> > +    while (g_hash_table_iter_next (&bus_it, NULL,
> > (void**)&vtd_bus)) {
> >          for (devfn_it = 0; devfn_it < VTD_PCI_DEVFN_MAX;
> > ++devfn_it) {
> > -            vtd_as = pvtd_as[devfn_it];
> > +            vtd_as = vtd_bus->dev_as[devfn_it];
> >              if (!vtd_as) {
> >                  continue;
> >              }
> > @@ -754,12 +753,13 @@ static inline bool
> > vtd_is_interrupt_addr(hwaddr addr)
> >   * @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,
> > +static void vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus
> > *bus,
> >                                     uint8_t devfn, hwaddr addr,
> > bool is_write,
> >                                     IOMMUTLBEntry *entry)
> >  {
> >      IntelIOMMUState *s = vtd_as->iommu_state;
> >      VTDContextEntry ce;
> > +    uint8_t bus_num = pci_bus_num(bus);
> >      VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;
> >      uint64_t slpte;
> >      uint32_t level;
> > @@ -874,6 +874,30 @@ static void
> > vtd_context_global_invalidate(IntelIOMMUState *s)
> >      }
> >  }
> >  
> > +
> > +/* Find the VTD address space currently associated with a given
> > bus number,
> > + */
> > +static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState *s,
> > uint8_t bus_num)
> > +{
> > +    VTDBus *vtd_bus = s->vtd_as_by_bus_num[bus_num];
> > +    if (!vtd_bus) {
> > +        /* Iterate over the registered buses to find the one
> > +         * which currently hold this bus number, and update the
> > bus_num lookup table:
> > +         */
> > +        GHashTableIter iter;
> > +        uint64_t key;
> > +
> > +        g_hash_table_iter_init(&iter, s->vtd_as_by_busptr);
> > +        while (g_hash_table_iter_next (&iter, (void**)&key,
> > (void**)&vtd_bus)) {
> > +            if (pci_bus_num(vtd_bus->bus) == bus_num) {
> > +                s->vtd_as_by_bus_num[bus_num] = vtd_bus;
> > +                return vtd_bus;
> > +            }
> > +        }
> > +    }
> > +    return vtd_bus;
> > +}
> > +
> >  /* Do a context-cache device-selective invalidation.
> >   * @func_mask: FM field after shifting
> >   */
> > @@ -882,7 +906,7 @@ static void
> > vtd_context_device_invalidate(IntelIOMMUState *s,
> >                                            uint16_t func_mask)
> >  {
> >      uint16_t mask;
> > -    VTDAddressSpace **pvtd_as;
> > +    VTDBus *vtd_bus;
> >      VTDAddressSpace *vtd_as;
> >      uint16_t devfn;
> >      uint16_t devfn_it;
> > @@ -903,11 +927,11 @@ 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) {
> > +    vtd_bus = vtd_find_as_from_bus_num(s,
> > VTD_SID_TO_BUS(source_id));
> > +    if (vtd_bus) {
> >          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];
> > +            vtd_as = vtd_bus->dev_as[devfn_it];
> >              if (vtd_as && ((devfn_it & mask) == (devfn & mask))) {
> >                  VTD_DPRINTF(INV, "invalidate context-cahce of
> > devfn 0x%"PRIx16,
> >                              devfn_it);
> > @@ -1805,11 +1829,11 @@ 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,
> > +    vtd_do_iommu_translate(vtd_as, vtd_as->bus, vtd_as->devfn,
> > 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,
> > +                " gpa 0x%"PRIx64 " hpa 0x%"PRIx64,
> > pci_bus_num(vtd_as->bus),
> >                  VTD_PCI_SLOT(vtd_as->devfn), VTD_PCI_FUNC(vtd_as
> > ->devfn),
> >                  vtd_as->devfn, addr, ret.translated_addr);
> >      return ret;
> > @@ -1839,6 +1863,38 @@ static Property vtd_properties[] = {
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > +
> > +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
> > int devfn)
> > +{
> > +    uint64_t key = (uint64_t)bus;
> > +    VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr,
> > &key);
> > +    VTDAddressSpace *vtd_dev_as;
> > +
> > +    if (!vtd_bus) {
> > +        /* No corresponding free() */
> > +        vtd_bus = g_malloc0(sizeof(VTDBus) +
> > sizeof(VTDAddressSpace *) * VTD_PCI_DEVFN_MAX);
> > +        vtd_bus->bus = bus;
> > +        key = (uint64_t)bus;
> > +        g_hash_table_insert(s->vtd_as_by_busptr, &key, vtd_bus);
> > +    }
> > +
> > +    vtd_dev_as = vtd_bus->dev_as[devfn];
> > +
> > +    if (!vtd_dev_as) {
> > +        vtd_bus->dev_as[devfn] = vtd_dev_as =
> > g_malloc0(sizeof(VTDAddressSpace));
> > +
> > +        vtd_dev_as->bus = bus;
> > +        vtd_dev_as->devfn = (uint8_t)devfn;
> > +        vtd_dev_as->iommu_state = s;
> > +        vtd_dev_as->context_cache_entry.context_cache_gen = 0;
> > +        memory_region_init_iommu(&vtd_dev_as->iommu, OBJECT(s),
> > +                                 &s->iommu_ops, "intel_iommu",
> > UINT64_MAX);
> > +        address_space_init(&vtd_dev_as->as,
> > +                           &vtd_dev_as->iommu, "intel_iommu");
> > +    }
> > +    return vtd_dev_as;
> > +}
> > +
> >  /* Do the initialization. It will also be called when reset, so
> > pay
> >   * attention when adding new initialization stuff.
> >   */
> > @@ -1931,13 +1987,15 @@ 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));
> > +    memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
> >      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);
> >      /* No corresponding destroy */
> >      s->iotlb = g_hash_table_new_full(vtd_uint64_hash,
> > vtd_uint64_equal,
> >                                       g_free, g_free);
> > +    s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash,
> > vtd_uint64_equal,
> > +                                              g_free, g_free);
> >      vtd_init(s);
> >  }
> >  
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index bd74094..c81507d 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -426,31 +426,12 @@ static void mch_reset(DeviceState *qdev)
> >  static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque,
> > int devfn)
> >  {
> >      IntelIOMMUState *s = opaque;
> > -    VTDAddressSpace **pvtd_as;
> > -    int bus_num = pci_bus_num(bus);
> > +    VTDAddressSpace *vtd_as;
> >  
> > -    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),
> > -                                 &s->iommu_ops, "intel_iommu",
> > UINT64_MAX);
> > -        address_space_init(&pvtd_as[devfn]->as,
> > -                           &pvtd_as[devfn]->iommu, "intel_iommu");
> > -    }
> > -    return &pvtd_as[devfn]->as;
> > +    vtd_as = vtd_find_add_as(s, bus, devfn);
> > +    return &vtd_as->as;
> >  }
> >  
> >  static void mch_init_dmar(MCHPCIState *mch)
> > diff --git a/include/hw/i386/intel_iommu.h
> > b/include/hw/i386/intel_iommu.h
> > index e321ee4..5dbadb7 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -49,6 +49,7 @@ typedef struct VTDContextCacheEntry
> > VTDContextCacheEntry;
> >  typedef struct IntelIOMMUState IntelIOMMUState;
> >  typedef struct VTDAddressSpace VTDAddressSpace;
> >  typedef struct VTDIOTLBEntry VTDIOTLBEntry;
> > +typedef struct VTDBus VTDBus;
> >  
> >  /* Context-Entry */
> >  struct VTDContextEntry {
> > @@ -65,7 +66,7 @@ struct VTDContextCacheEntry {
> >  };
> >  
> >  struct VTDAddressSpace {
> > -    uint8_t bus_num;
> > +    PCIBus *bus;
> >      uint8_t devfn;
> >      AddressSpace as;
> >      MemoryRegion iommu;
> > @@ -73,6 +74,11 @@ struct VTDAddressSpace {
> >      VTDContextCacheEntry context_cache_entry;
> >  };
> >  
> > +struct VTDBus {
> > +    PCIBus* bus;		/* A reference to the bus to
> > provide translation for */
> > +    VTDAddressSpace *dev_as[0];	/* A table of
> > VTDAddressSpace objects indexed by devfn */
> > +};
> > +
> >  struct VTDIOTLBEntry {
> >      uint64_t gfn;
> >      uint16_t domain_id;
> > @@ -114,7 +120,13 @@ struct IntelIOMMUState {
> >      GHashTable *iotlb;              /* IOTLB */
> >  
> >      MemoryRegionIOMMUOps iommu_ops;
> > -    VTDAddressSpace **address_spaces[VTD_PCI_BUS_MAX];
> > +    GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by
> > PCIBus* reference */
> > +    VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects
> > indexed by bus number */
> >  };
> >  
> > +/* Find the VTD Address space associated with the given bus
> > pointer,
> > + * create a new one if none exists
> > + */
> > +VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
> > int devfn);
> > +
> >  #endif