The AMD IOMMU is set up at boot time and uses PCI bus numbers + devfn
for indexing into DTE. The problem is that before the guest started,
all PCI bus numbers are 0 as no PCI discovery happened yet (BIOS or/and
kernel will do that later) so relying on the bus number is wrong.
The immediate effect is emulated devices cannot do DMA when places on
a bus other that 0.

Replace static array of address_space with hash table which uses devfn and
PCIBus* for key as it is not going to change after the guest is booted.

Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 hw/i386/amd_iommu.c | 134 ++++++++++++++++++++++++++------------------
 hw/i386/amd_iommu.h |   2 +-
 2 files changed, 79 insertions(+), 57 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 378e0cb55eab..b194e3294dd7 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -59,7 +59,7 @@ const char *amdvi_mmio_high[] = {
 };
 
 struct AMDVIAddressSpace {
-    uint8_t bus_num;            /* bus number                           */
+    PCIBus *bus;                /* PCIBus (for bus number)              */
     uint8_t devfn;              /* device function                      */
     AMDVIState *iommu_state;    /* AMDVI - one per machine              */
     MemoryRegion root;          /* AMDVI Root memory map region         */
@@ -101,6 +101,11 @@ typedef enum AMDVIFaultReason {
     AMDVI_FR_PT_ENTRY_INV,      /* Failure to read PTE from guest memory */
 } AMDVIFaultReason;
 
+typedef struct amdvi_as_key {
+    PCIBus *bus;
+    uint8_t devfn;
+} amdvi_as_key;
+
 uint64_t amdvi_extended_feature_register(AMDVIState *s)
 {
     uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
@@ -382,6 +387,44 @@ static guint amdvi_uint64_hash(gconstpointer v)
     return (guint)*(const uint64_t *)v;
 }
 
+static gboolean amdvi_as_equal(gconstpointer v1, gconstpointer v2)
+{
+    const struct amdvi_as_key *key1 = v1;
+    const struct amdvi_as_key *key2 = v2;
+
+    return key1->bus == key2->bus && key1->devfn == key2->devfn;
+}
+
+static guint amdvi_as_hash(gconstpointer v)
+{
+    const struct amdvi_as_key *key = v;
+    guint bus = (guint)(uintptr_t)key->bus;
+
+    return (guint)(bus << 8 | (uint)key->devfn);
+}
+
+static AMDVIAddressSpace *amdvi_as_lookup(AMDVIState *s, PCIBus *bus,
+                                          uint8_t devfn)
+{
+    amdvi_as_key key = { .bus = bus, .devfn = devfn };
+    return g_hash_table_lookup(s->address_spaces, &key);
+}
+
+gboolean amdvi_find_as_by_devid(gpointer key, gpointer value,
+                                  gpointer user_data)
+{
+    amdvi_as_key *as = (struct amdvi_as_key *)key;
+    uint16_t devid = *((uint16_t *)user_data);
+
+    return devid == PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
+}
+
+static AMDVIAddressSpace *amdvi_get_as_by_devid(AMDVIState *s, uint16_t devid)
+{
+    return g_hash_table_find(s->address_spaces,
+                             amdvi_find_as_by_devid, &devid);
+}
+
 static AMDVIIOTLBEntry *amdvi_iotlb_lookup(AMDVIState *s, hwaddr addr,
                                            uint64_t devid)
 {
@@ -551,7 +594,7 @@ static inline uint64_t amdvi_get_pte_entry(AMDVIState *s, uint64_t pte_addr,
 
 static int amdvi_as_to_dte(AMDVIAddressSpace *as, uint64_t *dte)
 {
-    uint16_t devid = PCI_BUILD_BDF(as->bus_num, as->devfn);
+    uint16_t devid = PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
     AMDVIState *s = as->iommu_state;
 
     if (!amdvi_get_dte(s, devid, dte)) {
@@ -1011,25 +1054,15 @@ static void amdvi_switch_address_space(AMDVIAddressSpace *amdvi_as)
  */
 static void amdvi_reset_address_translation_all(AMDVIState *s)
 {
-    AMDVIAddressSpace **iommu_as;
-
-    for (int bus_num = 0; bus_num < PCI_BUS_MAX; bus_num++) {
+    AMDVIAddressSpace *iommu_as;
+    GHashTableIter as_it;
 
-        /* Nothing to do if there are no devices on the current bus */
-        if (!s->address_spaces[bus_num]) {
-            continue;
-        }
-        iommu_as = s->address_spaces[bus_num];
+    g_hash_table_iter_init(&as_it, s->address_spaces);
 
-        for (int devfn = 0; devfn < PCI_DEVFN_MAX; devfn++) {
-
-            if (!iommu_as[devfn]) {
-                continue;
-            }
-            /* Use passthrough as default mode after reset */
-            iommu_as[devfn]->addr_translation = false;
-            amdvi_switch_address_space(iommu_as[devfn]);
-        }
+    while (g_hash_table_iter_next(&as_it, NULL, (void **)&iommu_as)) {
+        /* Use passhthrough as default mode after reset */
+        iommu_as->addr_translation = false;
+        amdvi_switch_address_space(iommu_as);
     }
 }
 
@@ -1089,27 +1122,15 @@ static void enable_nodma_mode(AMDVIAddressSpace *as)
  */
 static void amdvi_update_addr_translation_mode(AMDVIState *s, uint16_t devid)
 {
-    uint8_t bus_num, devfn, dte_mode;
+    uint8_t dte_mode;
     AMDVIAddressSpace *as;
     uint64_t dte[4] = { 0 };
     int ret;
 
-    /*
-     * Convert the devid encoded in the command to a bus and devfn in
-     * order to retrieve the corresponding address space.
-     */
-    bus_num = PCI_BUS_NUM(devid);
-    devfn = devid & 0xff;
-
-    /*
-     * The main buffer of size (AMDVIAddressSpace *) * (PCI_BUS_MAX) has already
-     * been allocated within AMDVIState, but must be careful to not access
-     * unallocated devfn.
-     */
-    if (!s->address_spaces[bus_num] || !s->address_spaces[bus_num][devfn]) {
+    as = amdvi_get_as_by_devid(s, devid);
+    if (!as) {
         return;
     }
-    as = s->address_spaces[bus_num][devfn];
 
     ret = amdvi_as_to_dte(as, dte);
 
@@ -1783,7 +1804,7 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr,
                                bool is_write, IOMMUTLBEntry *ret)
 {
     AMDVIState *s = as->iommu_state;
-    uint16_t devid = PCI_BUILD_BDF(as->bus_num, as->devfn);
+    uint16_t devid = PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
     AMDVIIOTLBEntry *iotlb_entry = amdvi_iotlb_lookup(s, addr, devid);
     uint64_t entry[4];
     int dte_ret;
@@ -1858,7 +1879,7 @@ static IOMMUTLBEntry amdvi_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
     }
 
     amdvi_do_translate(as, addr, flag & IOMMU_WO, &ret);
-    trace_amdvi_translation_result(as->bus_num, PCI_SLOT(as->devfn),
+    trace_amdvi_translation_result(pci_bus_num(as->bus), PCI_SLOT(as->devfn),
             PCI_FUNC(as->devfn), addr, ret.translated_addr);
     return ret;
 }
@@ -2222,30 +2243,28 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 {
     char name[128];
     AMDVIState *s = opaque;
-    AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
-    int bus_num = pci_bus_num(bus);
+    AMDVIAddressSpace *amdvi_dev_as;
+    amdvi_as_key *key;
 
-    iommu_as = s->address_spaces[bus_num];
+    amdvi_dev_as = amdvi_as_lookup(s, bus, devfn);
 
     /* allocate memory during the first run */
-    if (!iommu_as) {
-        iommu_as = g_new0(AMDVIAddressSpace *, PCI_DEVFN_MAX);
-        s->address_spaces[bus_num] = iommu_as;
-    }
-
-    /* set up AMD-Vi region */
-    if (!iommu_as[devfn]) {
+    if (!amdvi_dev_as) {
         snprintf(name, sizeof(name), "amd_iommu_devfn_%d", devfn);
 
-        iommu_as[devfn] = g_new0(AMDVIAddressSpace, 1);
-        iommu_as[devfn]->bus_num = (uint8_t)bus_num;
-        iommu_as[devfn]->devfn = (uint8_t)devfn;
-        iommu_as[devfn]->iommu_state = s;
-        iommu_as[devfn]->notifier_flags = IOMMU_NOTIFIER_NONE;
-        iommu_as[devfn]->iova_tree = iova_tree_new();
-        iommu_as[devfn]->addr_translation = false;
+        amdvi_dev_as = g_new0(AMDVIAddressSpace, 1);
+        key = g_new0(amdvi_as_key, 1);
 
-        amdvi_dev_as = iommu_as[devfn];
+        amdvi_dev_as->bus = bus;
+        amdvi_dev_as->devfn = (uint8_t)devfn;
+        amdvi_dev_as->iommu_state = s;
+        amdvi_dev_as->notifier_flags = IOMMU_NOTIFIER_NONE;
+        amdvi_dev_as->iova_tree = iova_tree_new();
+        amdvi_dev_as->addr_translation = false;
+        key->bus = bus;
+        key->devfn = devfn;
+
+        g_hash_table_insert(s->address_spaces, key, amdvi_dev_as);
 
         /*
          * Memory region relationships looks like (Address range shows
@@ -2288,7 +2307,7 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 
         amdvi_switch_address_space(amdvi_dev_as);
     }
-    return &iommu_as[devfn]->as;
+    return &amdvi_dev_as->as;
 }
 
 static const PCIIOMMUOps amdvi_iommu_ops = {
@@ -2329,7 +2348,7 @@ static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
     if (!s->dma_remap && (new & IOMMU_NOTIFIER_MAP)) {
         error_setg_errno(errp, ENOTSUP,
             "device %02x.%02x.%x requires dma-remap=1",
-            as->bus_num, PCI_SLOT(as->devfn), PCI_FUNC(as->devfn));
+            pci_bus_num(as->bus), PCI_SLOT(as->devfn), PCI_FUNC(as->devfn));
         return -ENOTSUP;
     }
 
@@ -2510,6 +2529,9 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
     s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
                                      amdvi_uint64_equal, g_free, g_free);
 
+    s->address_spaces = g_hash_table_new_full(amdvi_as_hash,
+                                     amdvi_as_equal, g_free, g_free);
+
     /* set up MMIO */
     memory_region_init_io(&s->mr_mmio, OBJECT(s), &mmio_mem_ops, s,
                           "amdvi-mmio", AMDVI_MMIO_SIZE);
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index daf82fc85f96..38471b95d153 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -408,7 +408,7 @@ struct AMDVIState {
     bool mmio_enabled;
 
     /* for each served device */
-    AMDVIAddressSpace **address_spaces[PCI_BUS_MAX];
+    GHashTable *address_spaces;
 
     /* list of address spaces with registered notifiers */
     QLIST_HEAD(, AMDVIAddressSpace) amdvi_as_with_notifiers;
-- 
2.34.1

On 10/13/25 1:00 AM, Sairaj Kodilkar wrote:
> The AMD IOMMU is set up at boot time and uses PCI bus numbers + devfn
> for indexing into DTE. The problem is that before the guest started,
> all PCI bus numbers are 0 as no PCI discovery happened yet (BIOS or/and
> kernel will do that later) so relying on the bus number is wrong.
> The immediate effect is emulated devices cannot do DMA when places on
> a bus other that 0.
> 
> Replace static array of address_space with hash table which uses devfn and
> PCIBus* for key as it is not going to change after the guest is booted.
> 
> Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> ---
>   hw/i386/amd_iommu.c | 134 ++++++++++++++++++++++++++------------------
>   hw/i386/amd_iommu.h |   2 +-
>   2 files changed, 79 insertions(+), 57 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 378e0cb55eab..b194e3294dd7 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -59,7 +59,7 @@ const char *amdvi_mmio_high[] = {
>   };
>   
>   struct AMDVIAddressSpace {
> -    uint8_t bus_num;            /* bus number                           */
> +    PCIBus *bus;                /* PCIBus (for bus number)              */
>       uint8_t devfn;              /* device function                      */
>       AMDVIState *iommu_state;    /* AMDVI - one per machine              */
>       MemoryRegion root;          /* AMDVI Root memory map region         */
> @@ -101,6 +101,11 @@ typedef enum AMDVIFaultReason {
>       AMDVI_FR_PT_ENTRY_INV,      /* Failure to read PTE from guest memory */
>   } AMDVIFaultReason;
>   
> +typedef struct amdvi_as_key {
> +    PCIBus *bus;
> +    uint8_t devfn;
> +} amdvi_as_key;
> +
>   uint64_t amdvi_extended_feature_register(AMDVIState *s)
>   {
>       uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
> @@ -382,6 +387,44 @@ static guint amdvi_uint64_hash(gconstpointer v)
>       return (guint)*(const uint64_t *)v;
>   }
>   
> +static gboolean amdvi_as_equal(gconstpointer v1, gconstpointer v2)
> +{
> +    const struct amdvi_as_key *key1 = v1;
> +    const struct amdvi_as_key *key2 = v2;
> +
> +    return key1->bus == key2->bus && key1->devfn == key2->devfn;
> +}
> +
> +static guint amdvi_as_hash(gconstpointer v)
> +{
> +    const struct amdvi_as_key *key = v;
> +    guint bus = (guint)(uintptr_t)key->bus;
> +
> +    return (guint)(bus << 8 | (uint)key->devfn);
> +}
> +
> +static AMDVIAddressSpace *amdvi_as_lookup(AMDVIState *s, PCIBus *bus,
> +                                          uint8_t devfn)
> +{
> +    amdvi_as_key key = { .bus = bus, .devfn = devfn };
> +    return g_hash_table_lookup(s->address_spaces, &key);
> +}
> +
> +gboolean amdvi_find_as_by_devid(gpointer key, gpointer value,

This causes a build failure.

Please continue using static for amdvi_find_as_by_devid() i.e.:

static gboolean amdvi_find_as_by_devid(...

This local helper is only needed within this file.

Alejandro

> +                                  gpointer user_data)
> +{
> +    amdvi_as_key *as = (struct amdvi_as_key *)key;
> +    uint16_t devid = *((uint16_t *)user_data);
> +
> +    return devid == PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
> +}
> +
> +static AMDVIAddressSpace *amdvi_get_as_by_devid(AMDVIState *s, uint16_t devid)
> +{
> +    return g_hash_table_find(s->address_spaces,
> +                             amdvi_find_as_by_devid, &devid);
> +}
> +
>   static AMDVIIOTLBEntry *amdvi_iotlb_lookup(AMDVIState *s, hwaddr addr,
>                                              uint64_t devid)
>   {
> @@ -551,7 +594,7 @@ static inline uint64_t amdvi_get_pte_entry(AMDVIState *s, uint64_t pte_addr,
>   
>   static int amdvi_as_to_dte(AMDVIAddressSpace *as, uint64_t *dte)
>   {
> -    uint16_t devid = PCI_BUILD_BDF(as->bus_num, as->devfn);
> +    uint16_t devid = PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
>       AMDVIState *s = as->iommu_state;
>   
>       if (!amdvi_get_dte(s, devid, dte)) {
> @@ -1011,25 +1054,15 @@ static void amdvi_switch_address_space(AMDVIAddressSpace *amdvi_as)
>    */
>   static void amdvi_reset_address_translation_all(AMDVIState *s)
>   {
> -    AMDVIAddressSpace **iommu_as;
> -
> -    for (int bus_num = 0; bus_num < PCI_BUS_MAX; bus_num++) {
> +    AMDVIAddressSpace *iommu_as;
> +    GHashTableIter as_it;
>   
> -        /* Nothing to do if there are no devices on the current bus */
> -        if (!s->address_spaces[bus_num]) {
> -            continue;
> -        }
> -        iommu_as = s->address_spaces[bus_num];
> +    g_hash_table_iter_init(&as_it, s->address_spaces);
>   
> -        for (int devfn = 0; devfn < PCI_DEVFN_MAX; devfn++) {
> -
> -            if (!iommu_as[devfn]) {
> -                continue;
> -            }
> -            /* Use passthrough as default mode after reset */
> -            iommu_as[devfn]->addr_translation = false;
> -            amdvi_switch_address_space(iommu_as[devfn]);
> -        }
> +    while (g_hash_table_iter_next(&as_it, NULL, (void **)&iommu_as)) {
> +        /* Use passhthrough as default mode after reset */
> +        iommu_as->addr_translation = false;
> +        amdvi_switch_address_space(iommu_as);
>       }
>   }
>   
> @@ -1089,27 +1122,15 @@ static void enable_nodma_mode(AMDVIAddressSpace *as)
>    */
>   static void amdvi_update_addr_translation_mode(AMDVIState *s, uint16_t devid)
>   {
> -    uint8_t bus_num, devfn, dte_mode;
> +    uint8_t dte_mode;
>       AMDVIAddressSpace *as;
>       uint64_t dte[4] = { 0 };
>       int ret;
>   
> -    /*
> -     * Convert the devid encoded in the command to a bus and devfn in
> -     * order to retrieve the corresponding address space.
> -     */
> -    bus_num = PCI_BUS_NUM(devid);
> -    devfn = devid & 0xff;
> -
> -    /*
> -     * The main buffer of size (AMDVIAddressSpace *) * (PCI_BUS_MAX) has already
> -     * been allocated within AMDVIState, but must be careful to not access
> -     * unallocated devfn.
> -     */
> -    if (!s->address_spaces[bus_num] || !s->address_spaces[bus_num][devfn]) {
> +    as = amdvi_get_as_by_devid(s, devid);
> +    if (!as) {
>           return;
>       }
> -    as = s->address_spaces[bus_num][devfn];
>   
>       ret = amdvi_as_to_dte(as, dte);
>   
> @@ -1783,7 +1804,7 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr,
>                                  bool is_write, IOMMUTLBEntry *ret)
>   {
>       AMDVIState *s = as->iommu_state;
> -    uint16_t devid = PCI_BUILD_BDF(as->bus_num, as->devfn);
> +    uint16_t devid = PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
>       AMDVIIOTLBEntry *iotlb_entry = amdvi_iotlb_lookup(s, addr, devid);
>       uint64_t entry[4];
>       int dte_ret;
> @@ -1858,7 +1879,7 @@ static IOMMUTLBEntry amdvi_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
>       }
>   
>       amdvi_do_translate(as, addr, flag & IOMMU_WO, &ret);
> -    trace_amdvi_translation_result(as->bus_num, PCI_SLOT(as->devfn),
> +    trace_amdvi_translation_result(pci_bus_num(as->bus), PCI_SLOT(as->devfn),
>               PCI_FUNC(as->devfn), addr, ret.translated_addr);
>       return ret;
>   }
> @@ -2222,30 +2243,28 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>   {
>       char name[128];
>       AMDVIState *s = opaque;
> -    AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
> -    int bus_num = pci_bus_num(bus);
> +    AMDVIAddressSpace *amdvi_dev_as;
> +    amdvi_as_key *key;
>   
> -    iommu_as = s->address_spaces[bus_num];
> +    amdvi_dev_as = amdvi_as_lookup(s, bus, devfn);
>   
>       /* allocate memory during the first run */
> -    if (!iommu_as) {
> -        iommu_as = g_new0(AMDVIAddressSpace *, PCI_DEVFN_MAX);
> -        s->address_spaces[bus_num] = iommu_as;
> -    }
> -
> -    /* set up AMD-Vi region */
> -    if (!iommu_as[devfn]) {
> +    if (!amdvi_dev_as) {
>           snprintf(name, sizeof(name), "amd_iommu_devfn_%d", devfn);
>   
> -        iommu_as[devfn] = g_new0(AMDVIAddressSpace, 1);
> -        iommu_as[devfn]->bus_num = (uint8_t)bus_num;
> -        iommu_as[devfn]->devfn = (uint8_t)devfn;
> -        iommu_as[devfn]->iommu_state = s;
> -        iommu_as[devfn]->notifier_flags = IOMMU_NOTIFIER_NONE;
> -        iommu_as[devfn]->iova_tree = iova_tree_new();
> -        iommu_as[devfn]->addr_translation = false;
> +        amdvi_dev_as = g_new0(AMDVIAddressSpace, 1);
> +        key = g_new0(amdvi_as_key, 1);
>   
> -        amdvi_dev_as = iommu_as[devfn];
> +        amdvi_dev_as->bus = bus;
> +        amdvi_dev_as->devfn = (uint8_t)devfn;
> +        amdvi_dev_as->iommu_state = s;
> +        amdvi_dev_as->notifier_flags = IOMMU_NOTIFIER_NONE;
> +        amdvi_dev_as->iova_tree = iova_tree_new();
> +        amdvi_dev_as->addr_translation = false;
> +        key->bus = bus;
> +        key->devfn = devfn;
> +
> +        g_hash_table_insert(s->address_spaces, key, amdvi_dev_as);
>   
>           /*
>            * Memory region relationships looks like (Address range shows
> @@ -2288,7 +2307,7 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>   
>           amdvi_switch_address_space(amdvi_dev_as);
>       }
> -    return &iommu_as[devfn]->as;
> +    return &amdvi_dev_as->as;
>   }
>   
>   static const PCIIOMMUOps amdvi_iommu_ops = {
> @@ -2329,7 +2348,7 @@ static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>       if (!s->dma_remap && (new & IOMMU_NOTIFIER_MAP)) {
>           error_setg_errno(errp, ENOTSUP,
>               "device %02x.%02x.%x requires dma-remap=1",
> -            as->bus_num, PCI_SLOT(as->devfn), PCI_FUNC(as->devfn));
> +            pci_bus_num(as->bus), PCI_SLOT(as->devfn), PCI_FUNC(as->devfn));
>           return -ENOTSUP;
>       }
>   
> @@ -2510,6 +2529,9 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>       s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
>                                        amdvi_uint64_equal, g_free, g_free);
>   
> +    s->address_spaces = g_hash_table_new_full(amdvi_as_hash,
> +                                     amdvi_as_equal, g_free, g_free);
> +
>       /* set up MMIO */
>       memory_region_init_io(&s->mr_mmio, OBJECT(s), &mmio_mem_ops, s,
>                             "amdvi-mmio", AMDVI_MMIO_SIZE);
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index daf82fc85f96..38471b95d153 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -408,7 +408,7 @@ struct AMDVIState {
>       bool mmio_enabled;
>   
>       /* for each served device */
> -    AMDVIAddressSpace **address_spaces[PCI_BUS_MAX];
> +    GHashTable *address_spaces;
>   
>       /* list of address spaces with registered notifiers */
>       QLIST_HEAD(, AMDVIAddressSpace) amdvi_as_with_notifiers;

On Mon, Oct 13, 2025 at 10:30:45AM +0530, Sairaj Kodilkar wrote:
> The AMD IOMMU is set up at boot time and uses PCI bus numbers + devfn
> for indexing into DTE. The problem is that before the guest started,
> all PCI bus numbers are 0 as no PCI discovery happened yet (BIOS or/and
> kernel will do that later) so relying on the bus number is wrong.
> The immediate effect is emulated devices cannot do DMA when places on
> a bus other that 0.
> 
> Replace static array of address_space with hash table which uses devfn and
> PCIBus* for key as it is not going to change after the guest is booted.

I am curious whether this has any measureable impact on
performance.


> Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>


love the patch! yet something to improve:

> ---
>  hw/i386/amd_iommu.c | 134 ++++++++++++++++++++++++++------------------
>  hw/i386/amd_iommu.h |   2 +-
>  2 files changed, 79 insertions(+), 57 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 378e0cb55eab..b194e3294dd7 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -59,7 +59,7 @@ const char *amdvi_mmio_high[] = {
>  };
>  
>  struct AMDVIAddressSpace {
> -    uint8_t bus_num;            /* bus number                           */
> +    PCIBus *bus;                /* PCIBus (for bus number)              */
>      uint8_t devfn;              /* device function                      */
>      AMDVIState *iommu_state;    /* AMDVI - one per machine              */
>      MemoryRegion root;          /* AMDVI Root memory map region         */
> @@ -101,6 +101,11 @@ typedef enum AMDVIFaultReason {
>      AMDVI_FR_PT_ENTRY_INV,      /* Failure to read PTE from guest memory */
>  } AMDVIFaultReason;
>  
> +typedef struct amdvi_as_key {
> +    PCIBus *bus;
> +    uint8_t devfn;
> +} amdvi_as_key;
> +
>  uint64_t amdvi_extended_feature_register(AMDVIState *s)
>  {
>      uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;


Pls fix structure and typedef names according to the QEMU
coding style. Thanks!


> @@ -382,6 +387,44 @@ static guint amdvi_uint64_hash(gconstpointer v)
>      return (guint)*(const uint64_t *)v;
>  }
>  
> +static gboolean amdvi_as_equal(gconstpointer v1, gconstpointer v2)
> +{
> +    const struct amdvi_as_key *key1 = v1;
> +    const struct amdvi_as_key *key2 = v2;
> +
> +    return key1->bus == key2->bus && key1->devfn == key2->devfn;
> +}
> +
> +static guint amdvi_as_hash(gconstpointer v)
> +{
> +    const struct amdvi_as_key *key = v;
> +    guint bus = (guint)(uintptr_t)key->bus;
> +
> +    return (guint)(bus << 8 | (uint)key->devfn);
> +}
> +
> +static AMDVIAddressSpace *amdvi_as_lookup(AMDVIState *s, PCIBus *bus,
> +                                          uint8_t devfn)
> +{
> +    amdvi_as_key key = { .bus = bus, .devfn = devfn };
> +    return g_hash_table_lookup(s->address_spaces, &key);
> +}
> +
> +gboolean amdvi_find_as_by_devid(gpointer key, gpointer value,
> +                                  gpointer user_data)
> +{
> +    amdvi_as_key *as = (struct amdvi_as_key *)key;

this assignment does not need a cast I think.

> +    uint16_t devid = *((uint16_t *)user_data);

would be better like this:
	    uint16_t * devidp = user_data;
then just use *devidp instead of devid.


> +
> +    return devid == PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
> +}
> +
> +static AMDVIAddressSpace *amdvi_get_as_by_devid(AMDVIState *s, uint16_t devid)
> +{
> +    return g_hash_table_find(s->address_spaces,
> +                             amdvi_find_as_by_devid, &devid);
> +}
> +
>  static AMDVIIOTLBEntry *amdvi_iotlb_lookup(AMDVIState *s, hwaddr addr,
>                                             uint64_t devid)
>  {
> @@ -551,7 +594,7 @@ static inline uint64_t amdvi_get_pte_entry(AMDVIState *s, uint64_t pte_addr,
>  
>  static int amdvi_as_to_dte(AMDVIAddressSpace *as, uint64_t *dte)
>  {
> -    uint16_t devid = PCI_BUILD_BDF(as->bus_num, as->devfn);
> +    uint16_t devid = PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
>      AMDVIState *s = as->iommu_state;
>  
>      if (!amdvi_get_dte(s, devid, dte)) {
> @@ -1011,25 +1054,15 @@ static void amdvi_switch_address_space(AMDVIAddressSpace *amdvi_as)
>   */
>  static void amdvi_reset_address_translation_all(AMDVIState *s)
>  {
> -    AMDVIAddressSpace **iommu_as;
> -
> -    for (int bus_num = 0; bus_num < PCI_BUS_MAX; bus_num++) {
> +    AMDVIAddressSpace *iommu_as;
> +    GHashTableIter as_it;
>  
> -        /* Nothing to do if there are no devices on the current bus */
> -        if (!s->address_spaces[bus_num]) {
> -            continue;
> -        }
> -        iommu_as = s->address_spaces[bus_num];
> +    g_hash_table_iter_init(&as_it, s->address_spaces);
>  
> -        for (int devfn = 0; devfn < PCI_DEVFN_MAX; devfn++) {
> -
> -            if (!iommu_as[devfn]) {
> -                continue;
> -            }
> -            /* Use passthrough as default mode after reset */
> -            iommu_as[devfn]->addr_translation = false;
> -            amdvi_switch_address_space(iommu_as[devfn]);
> -        }
> +    while (g_hash_table_iter_next(&as_it, NULL, (void **)&iommu_as)) {
> +        /* Use passhthrough as default mode after reset */
> +        iommu_as->addr_translation = false;
> +        amdvi_switch_address_space(iommu_as);
>      }
>  }
>  
> @@ -1089,27 +1122,15 @@ static void enable_nodma_mode(AMDVIAddressSpace *as)
>   */
>  static void amdvi_update_addr_translation_mode(AMDVIState *s, uint16_t devid)
>  {
> -    uint8_t bus_num, devfn, dte_mode;
> +    uint8_t dte_mode;
>      AMDVIAddressSpace *as;
>      uint64_t dte[4] = { 0 };
>      int ret;
>  
> -    /*
> -     * Convert the devid encoded in the command to a bus and devfn in
> -     * order to retrieve the corresponding address space.
> -     */
> -    bus_num = PCI_BUS_NUM(devid);
> -    devfn = devid & 0xff;
> -
> -    /*
> -     * The main buffer of size (AMDVIAddressSpace *) * (PCI_BUS_MAX) has already
> -     * been allocated within AMDVIState, but must be careful to not access
> -     * unallocated devfn.
> -     */
> -    if (!s->address_spaces[bus_num] || !s->address_spaces[bus_num][devfn]) {
> +    as = amdvi_get_as_by_devid(s, devid);
> +    if (!as) {
>          return;
>      }
> -    as = s->address_spaces[bus_num][devfn];
>  
>      ret = amdvi_as_to_dte(as, dte);
>  
> @@ -1783,7 +1804,7 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr,
>                                 bool is_write, IOMMUTLBEntry *ret)
>  {
>      AMDVIState *s = as->iommu_state;
> -    uint16_t devid = PCI_BUILD_BDF(as->bus_num, as->devfn);
> +    uint16_t devid = PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
>      AMDVIIOTLBEntry *iotlb_entry = amdvi_iotlb_lookup(s, addr, devid);
>      uint64_t entry[4];
>      int dte_ret;
> @@ -1858,7 +1879,7 @@ static IOMMUTLBEntry amdvi_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
>      }
>  
>      amdvi_do_translate(as, addr, flag & IOMMU_WO, &ret);
> -    trace_amdvi_translation_result(as->bus_num, PCI_SLOT(as->devfn),
> +    trace_amdvi_translation_result(pci_bus_num(as->bus), PCI_SLOT(as->devfn),
>              PCI_FUNC(as->devfn), addr, ret.translated_addr);
>      return ret;
>  }
> @@ -2222,30 +2243,28 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>  {
>      char name[128];
>      AMDVIState *s = opaque;
> -    AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
> -    int bus_num = pci_bus_num(bus);
> +    AMDVIAddressSpace *amdvi_dev_as;
> +    amdvi_as_key *key;
>  
> -    iommu_as = s->address_spaces[bus_num];
> +    amdvi_dev_as = amdvi_as_lookup(s, bus, devfn);
>  
>      /* allocate memory during the first run */
> -    if (!iommu_as) {
> -        iommu_as = g_new0(AMDVIAddressSpace *, PCI_DEVFN_MAX);
> -        s->address_spaces[bus_num] = iommu_as;
> -    }
> -
> -    /* set up AMD-Vi region */
> -    if (!iommu_as[devfn]) {
> +    if (!amdvi_dev_as) {
>          snprintf(name, sizeof(name), "amd_iommu_devfn_%d", devfn);
>  
> -        iommu_as[devfn] = g_new0(AMDVIAddressSpace, 1);
> -        iommu_as[devfn]->bus_num = (uint8_t)bus_num;
> -        iommu_as[devfn]->devfn = (uint8_t)devfn;
> -        iommu_as[devfn]->iommu_state = s;
> -        iommu_as[devfn]->notifier_flags = IOMMU_NOTIFIER_NONE;
> -        iommu_as[devfn]->iova_tree = iova_tree_new();
> -        iommu_as[devfn]->addr_translation = false;
> +        amdvi_dev_as = g_new0(AMDVIAddressSpace, 1);
> +        key = g_new0(amdvi_as_key, 1);
>  
> -        amdvi_dev_as = iommu_as[devfn];
> +        amdvi_dev_as->bus = bus;
> +        amdvi_dev_as->devfn = (uint8_t)devfn;
> +        amdvi_dev_as->iommu_state = s;
> +        amdvi_dev_as->notifier_flags = IOMMU_NOTIFIER_NONE;
> +        amdvi_dev_as->iova_tree = iova_tree_new();
> +        amdvi_dev_as->addr_translation = false;
> +        key->bus = bus;
> +        key->devfn = devfn;
> +
> +        g_hash_table_insert(s->address_spaces, key, amdvi_dev_as);
>  
>          /*
>           * Memory region relationships looks like (Address range shows
> @@ -2288,7 +2307,7 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>  
>          amdvi_switch_address_space(amdvi_dev_as);
>      }
> -    return &iommu_as[devfn]->as;
> +    return &amdvi_dev_as->as;
>  }
>  
>  static const PCIIOMMUOps amdvi_iommu_ops = {
> @@ -2329,7 +2348,7 @@ static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>      if (!s->dma_remap && (new & IOMMU_NOTIFIER_MAP)) {
>          error_setg_errno(errp, ENOTSUP,
>              "device %02x.%02x.%x requires dma-remap=1",
> -            as->bus_num, PCI_SLOT(as->devfn), PCI_FUNC(as->devfn));
> +            pci_bus_num(as->bus), PCI_SLOT(as->devfn), PCI_FUNC(as->devfn));
>          return -ENOTSUP;
>      }
>  
> @@ -2510,6 +2529,9 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>      s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
>                                       amdvi_uint64_equal, g_free, g_free);
>  
> +    s->address_spaces = g_hash_table_new_full(amdvi_as_hash,
> +                                     amdvi_as_equal, g_free, g_free);
> +
>      /* set up MMIO */
>      memory_region_init_io(&s->mr_mmio, OBJECT(s), &mmio_mem_ops, s,
>                            "amdvi-mmio", AMDVI_MMIO_SIZE);
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index daf82fc85f96..38471b95d153 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -408,7 +408,7 @@ struct AMDVIState {
>      bool mmio_enabled;
>  
>      /* for each served device */
> -    AMDVIAddressSpace **address_spaces[PCI_BUS_MAX];
> +    GHashTable *address_spaces;
>  
>      /* list of address spaces with registered notifiers */
>      QLIST_HEAD(, AMDVIAddressSpace) amdvi_as_with_notifiers;
> -- 
> 2.34.1

On 10/13/2025 1:45 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 13, 2025 at 10:30:45AM +0530, Sairaj Kodilkar wrote:
>> The AMD IOMMU is set up at boot time and uses PCI bus numbers + devfn
>> for indexing into DTE. The problem is that before the guest started,
>> all PCI bus numbers are 0 as no PCI discovery happened yet (BIOS or/and
>> kernel will do that later) so relying on the bus number is wrong.
>> The immediate effect is emulated devices cannot do DMA when places on
>> a bus other that 0.
>>
>> Replace static array of address_space with hash table which uses devfn and
>> PCIBus* for key as it is not going to change after the guest is booted.
> I am curious whether this has any measureable impact on
> performance.

I dont think it should have much performance impact, as guest usually has
small number of devices attached to it and hash has O(1) average search cost
when hash key function is good.

>
>> Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>
> love the patch! yet something to improve:
>
>> ---
>>   hw/i386/amd_iommu.c | 134 ++++++++++++++++++++++++++------------------
>>   hw/i386/amd_iommu.h |   2 +-
>>   2 files changed, 79 insertions(+), 57 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 378e0cb55eab..b194e3294dd7 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -59,7 +59,7 @@ const char *amdvi_mmio_high[] = {
>>   };
>>   
>>   struct AMDVIAddressSpace {
>> -    uint8_t bus_num;            /* bus number                           */
>> +    PCIBus *bus;                /* PCIBus (for bus number)              */
>>       uint8_t devfn;              /* device function                      */
>>       AMDVIState *iommu_state;    /* AMDVI - one per machine              */
>>       MemoryRegion root;          /* AMDVI Root memory map region         */
>> @@ -101,6 +101,11 @@ typedef enum AMDVIFaultReason {
>>       AMDVI_FR_PT_ENTRY_INV,      /* Failure to read PTE from guest memory */
>>   } AMDVIFaultReason;
>>   
>> +typedef struct amdvi_as_key {
>> +    PCIBus *bus;
>> +    uint8_t devfn;
>> +} amdvi_as_key;
>> +
>>   uint64_t amdvi_extended_feature_register(AMDVIState *s)
>>   {
>>       uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
>
> Pls fix structure and typedef names according to the QEMU
> coding style. Thanks!
>

This is something I am struggling with, because the name
`AMDVIASKey` does not offer readability. Maybe we can come
up with an alternate style which is readable and does not
differ much from the current style.

@alejandro any suggestions ?

>> @@ -382,6 +387,44 @@ static guint amdvi_uint64_hash(gconstpointer v)
>>       return (guint)*(const uint64_t *)v;
>>   }
>>   
>> +static gboolean amdvi_as_equal(gconstpointer v1, gconstpointer v2)
>> +{
>> +    const struct amdvi_as_key *key1 = v1;
>> +    const struct amdvi_as_key *key2 = v2;
>> +
>> +    return key1->bus == key2->bus && key1->devfn == key2->devfn;
>> +}
>> +
>> +static guint amdvi_as_hash(gconstpointer v)
>> +{
>> +    const struct amdvi_as_key *key = v;
>> +    guint bus = (guint)(uintptr_t)key->bus;
>> +
>> +    return (guint)(bus << 8 | (uint)key->devfn);
>> +}
>> +
>> +static AMDVIAddressSpace *amdvi_as_lookup(AMDVIState *s, PCIBus *bus,
>> +                                          uint8_t devfn)
>> +{
>> +    amdvi_as_key key = { .bus = bus, .devfn = devfn };
>> +    return g_hash_table_lookup(s->address_spaces, &key);
>> +}
>> +
>> +gboolean amdvi_find_as_by_devid(gpointer key, gpointer value,
>> +                                  gpointer user_data)
>> +{
>> +    amdvi_as_key *as = (struct amdvi_as_key *)key;
> this assignment does not need a cast I think.
>
>> +    uint16_t devid = *((uint16_t *)user_data);
> would be better like this:
> 	    uint16_t * devidp = user_data;
> then just use *devidp instead of devid.

sure

Thanks
Sairaj

On Tue, Oct 14, 2025 at 11:13:51AM +0530, Sairaj Kodilkar wrote:
> 
> 
> On 10/13/2025 1:45 PM, Michael S. Tsirkin wrote:
> > On Mon, Oct 13, 2025 at 10:30:45AM +0530, Sairaj Kodilkar wrote:
> > > The AMD IOMMU is set up at boot time and uses PCI bus numbers + devfn
> > > for indexing into DTE. The problem is that before the guest started,
> > > all PCI bus numbers are 0 as no PCI discovery happened yet (BIOS or/and
> > > kernel will do that later) so relying on the bus number is wrong.
> > > The immediate effect is emulated devices cannot do DMA when places on
> > > a bus other that 0.
> > > 
> > > Replace static array of address_space with hash table which uses devfn and
> > > PCIBus* for key as it is not going to change after the guest is booted.
> > I am curious whether this has any measureable impact on
> > performance.
> 
> I dont think it should have much performance impact, as guest usually has
> small number of devices attached to it and hash has O(1) average search cost
> when hash key function is good.
> 
> > 
> > > Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
> > > Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> > > Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> > 
> > love the patch! yet something to improve:
> > 
> > > ---
> > >   hw/i386/amd_iommu.c | 134 ++++++++++++++++++++++++++------------------
> > >   hw/i386/amd_iommu.h |   2 +-
> > >   2 files changed, 79 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> > > index 378e0cb55eab..b194e3294dd7 100644
> > > --- a/hw/i386/amd_iommu.c
> > > +++ b/hw/i386/amd_iommu.c
> > > @@ -59,7 +59,7 @@ const char *amdvi_mmio_high[] = {
> > >   };
> > >   struct AMDVIAddressSpace {
> > > -    uint8_t bus_num;            /* bus number                           */
> > > +    PCIBus *bus;                /* PCIBus (for bus number)              */
> > >       uint8_t devfn;              /* device function                      */
> > >       AMDVIState *iommu_state;    /* AMDVI - one per machine              */
> > >       MemoryRegion root;          /* AMDVI Root memory map region         */
> > > @@ -101,6 +101,11 @@ typedef enum AMDVIFaultReason {
> > >       AMDVI_FR_PT_ENTRY_INV,      /* Failure to read PTE from guest memory */
> > >   } AMDVIFaultReason;
> > > +typedef struct amdvi_as_key {
> > > +    PCIBus *bus;
> > > +    uint8_t devfn;
> > > +} amdvi_as_key;
> > > +
> > >   uint64_t amdvi_extended_feature_register(AMDVIState *s)
> > >   {
> > >       uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
> > 
> > Pls fix structure and typedef names according to the QEMU
> > coding style. Thanks!
> > 
> 
> This is something I am struggling with, because the name
> `AMDVIASKey` does not offer readability.

AMDVIAsKey


Or you can update all code to use AmdVi and get AmdViAsKey if you prefer.


> Maybe we can come
> up with an alternate style which is readable and does not
> differ much from the current style.
> 
> @alejandro any suggestions ?
> 
> > > @@ -382,6 +387,44 @@ static guint amdvi_uint64_hash(gconstpointer v)
> > >       return (guint)*(const uint64_t *)v;
> > >   }
> > > +static gboolean amdvi_as_equal(gconstpointer v1, gconstpointer v2)
> > > +{
> > > +    const struct amdvi_as_key *key1 = v1;
> > > +    const struct amdvi_as_key *key2 = v2;
> > > +
> > > +    return key1->bus == key2->bus && key1->devfn == key2->devfn;
> > > +}
> > > +
> > > +static guint amdvi_as_hash(gconstpointer v)
> > > +{
> > > +    const struct amdvi_as_key *key = v;
> > > +    guint bus = (guint)(uintptr_t)key->bus;
> > > +
> > > +    return (guint)(bus << 8 | (uint)key->devfn);
> > > +}
> > > +
> > > +static AMDVIAddressSpace *amdvi_as_lookup(AMDVIState *s, PCIBus *bus,
> > > +                                          uint8_t devfn)
> > > +{
> > > +    amdvi_as_key key = { .bus = bus, .devfn = devfn };
> > > +    return g_hash_table_lookup(s->address_spaces, &key);
> > > +}
> > > +
> > > +gboolean amdvi_find_as_by_devid(gpointer key, gpointer value,
> > > +                                  gpointer user_data)
> > > +{
> > > +    amdvi_as_key *as = (struct amdvi_as_key *)key;
> > this assignment does not need a cast I think.
> > 
> > > +    uint16_t devid = *((uint16_t *)user_data);
> > would be better like this:
> > 	    uint16_t * devidp = user_data;
> > then just use *devidp instead of devid.
> 
> sure
> 
> Thanks
> Sairaj

On 10/14/25 5:02 AM, Michael S. Tsirkin wrote:
> On Tue, Oct 14, 2025 at 11:13:51AM +0530, Sairaj Kodilkar wrote:
>>
>>
>> On 10/13/2025 1:45 PM, Michael S. Tsirkin wrote:
>>> On Mon, Oct 13, 2025 at 10:30:45AM +0530, Sairaj Kodilkar wrote:
>>>> The AMD IOMMU is set up at boot time and uses PCI bus numbers + devfn
>>>> for indexing into DTE. The problem is that before the guest started,
>>>> all PCI bus numbers are 0 as no PCI discovery happened yet (BIOS or/and
>>>> kernel will do that later) so relying on the bus number is wrong.
>>>> The immediate effect is emulated devices cannot do DMA when places on
>>>> a bus other that 0.
>>>>
>>>> Replace static array of address_space with hash table which uses devfn and
>>>> PCIBus* for key as it is not going to change after the guest is booted.
>>> I am curious whether this has any measureable impact on
>>> performance.
>>
>> I dont think it should have much performance impact, as guest usually has
>> small number of devices attached to it and hash has O(1) average search cost
>> when hash key function is good.
>>
>>>
>>>> Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>>>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>>>
>>> love the patch! yet something to improve:
>>>
>>>> ---
>>>>    hw/i386/amd_iommu.c | 134 ++++++++++++++++++++++++++------------------
>>>>    hw/i386/amd_iommu.h |   2 +-
>>>>    2 files changed, 79 insertions(+), 57 deletions(-)
>>>>
>>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>>> index 378e0cb55eab..b194e3294dd7 100644
>>>> --- a/hw/i386/amd_iommu.c
>>>> +++ b/hw/i386/amd_iommu.c
>>>> @@ -59,7 +59,7 @@ const char *amdvi_mmio_high[] = {
>>>>    };
>>>>    struct AMDVIAddressSpace {
>>>> -    uint8_t bus_num;            /* bus number                           */
>>>> +    PCIBus *bus;                /* PCIBus (for bus number)              */
>>>>        uint8_t devfn;              /* device function                      */
>>>>        AMDVIState *iommu_state;    /* AMDVI - one per machine              */
>>>>        MemoryRegion root;          /* AMDVI Root memory map region         */
>>>> @@ -101,6 +101,11 @@ typedef enum AMDVIFaultReason {
>>>>        AMDVI_FR_PT_ENTRY_INV,      /* Failure to read PTE from guest memory */
>>>>    } AMDVIFaultReason;
>>>> +typedef struct amdvi_as_key {
>>>> +    PCIBus *bus;
>>>> +    uint8_t devfn;
>>>> +} amdvi_as_key;
>>>> +
>>>>    uint64_t amdvi_extended_feature_register(AMDVIState *s)
>>>>    {
>>>>        uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
>>>
>>> Pls fix structure and typedef names according to the QEMU
>>> coding style. Thanks!
>>>
>>
>> This is something I am struggling with, because the name
>> `AMDVIASKey` does not offer readability.
> 
> AMDVIAsKey
> 
> 
> Or you can update all code to use AmdVi and get AmdViAsKey if you prefer.
> 
> 
>> Maybe we can come
>> up with an alternate style which is readable and does not
>> differ much from the current style.
>>
>> @alejandro any suggestions ?
>>

I should have pointed out the CamelCase requirement for the typedef on 
v1. My initial reaction was: "do not use typedef" and go with the 
slightly longer 'struct amdvi_as_key' instead. The style guide has a 
warning about typedefs (which doesn't necessarily apply here), but IMO 
still better to avoid it in this case were we are not really gaining 
much from it.

If I were to use a typedef I would use 'AMDViAsKey'. After all, the 'i' 
in AMD-Vi and 'd' in VT-d are lowercase ;)

But my opinion is to avoid the typedef altogether.

>>>> @@ -382,6 +387,44 @@ static guint amdvi_uint64_hash(gconstpointer v)
>>>>        return (guint)*(const uint64_t *)v;
>>>>    }
>>>> +static gboolean amdvi_as_equal(gconstpointer v1, gconstpointer v2)
>>>> +{
>>>> +    const struct amdvi_as_key *key1 = v1;
>>>> +    const struct amdvi_as_key *key2 = v2;
>>>> +
>>>> +    return key1->bus == key2->bus && key1->devfn == key2->devfn;
>>>> +}
>>>> +
>>>> +static guint amdvi_as_hash(gconstpointer v)
>>>> +{
>>>> +    const struct amdvi_as_key *key = v;
>>>> +    guint bus = (guint)(uintptr_t)key->bus;
>>>> +
>>>> +    return (guint)(bus << 8 | (uint)key->devfn);
>>>> +}
>>>> +
>>>> +static AMDVIAddressSpace *amdvi_as_lookup(AMDVIState *s, PCIBus *bus,
>>>> +                                          uint8_t devfn)
>>>> +{
>>>> +    amdvi_as_key key = { .bus = bus, .devfn = devfn };
>>>> +    return g_hash_table_lookup(s->address_spaces, &key);
>>>> +}
>>>> +
>>>> +gboolean amdvi_find_as_by_devid(gpointer key, gpointer value,
>>>> +                                  gpointer user_data)
>>>> +{
>>>> +    amdvi_as_key *as = (struct amdvi_as_key *)key;
>>> this assignment does not need a cast I think.
>>>
Agree. And assuming you take my advice of not using a typedef, the line 
should be:

const struct amdvi_as_key *as = key;

to follow the style guide directive of using "const-correct" pointers. 
Putting back the static qualifier from my earlier reply:

static gboolean amdvi_find_as_by_devid(gpointer key, gpointer value, 

                                        gpointer user_data) 

{ 

     const struct amdvi_as_key *as = key; 

     const uint16_t *devidp = user_data; 

  

     return *devidp == PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn); 

} 


Thank you,
Alejandro

>>>> +    uint16_t devid = *((uint16_t *)user_data);
>>> would be better like this:
>>> 	    uint16_t * devidp = user_data;
>>> then just use *devidp instead of devid.
>>
>> sure
>>
>> Thanks
>> Sairaj
>

On 10/15/2025 3:16 AM, Alejandro Jimenez wrote:
>
>
> On 10/14/25 5:02 AM, Michael S. Tsirkin wrote:
>> On Tue, Oct 14, 2025 at 11:13:51AM +0530, Sairaj Kodilkar wrote:
>>>
>>>
>>> On 10/13/2025 1:45 PM, Michael S. Tsirkin wrote:
>>>> On Mon, Oct 13, 2025 at 10:30:45AM +0530, Sairaj Kodilkar wrote:
>>>>> The AMD IOMMU is set up at boot time and uses PCI bus numbers + devfn
>>>>> for indexing into DTE. The problem is that before the guest started,
>>>>> all PCI bus numbers are 0 as no PCI discovery happened yet (BIOS 
>>>>> or/and
>>>>> kernel will do that later) so relying on the bus number is wrong.
>>>>> The immediate effect is emulated devices cannot do DMA when places on
>>>>> a bus other that 0.
>>>>>
>>>>> Replace static array of address_space with hash table which uses 
>>>>> devfn and
>>>>> PCIBus* for key as it is not going to change after the guest is 
>>>>> booted.
>>>> I am curious whether this has any measureable impact on
>>>> performance.
>>>
>>> I dont think it should have much performance impact, as guest 
>>> usually has
>>> small number of devices attached to it and hash has O(1) average 
>>> search cost
>>> when hash key function is good.
>>>
>>>>
>>>>> Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>>>>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>>>>
>>>> love the patch! yet something to improve:
>>>>
>>>>> ---
>>>>>    hw/i386/amd_iommu.c | 134 
>>>>> ++++++++++++++++++++++++++------------------
>>>>>    hw/i386/amd_iommu.h |   2 +-
>>>>>    2 files changed, 79 insertions(+), 57 deletions(-)
>>>>>
>>>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>>>> index 378e0cb55eab..b194e3294dd7 100644
>>>>> --- a/hw/i386/amd_iommu.c
>>>>> +++ b/hw/i386/amd_iommu.c
>>>>> @@ -59,7 +59,7 @@ const char *amdvi_mmio_high[] = {
>>>>>    };
>>>>>    struct AMDVIAddressSpace {
>>>>> -    uint8_t bus_num;            /* bus 
>>>>> number                           */
>>>>> +    PCIBus *bus;                /* PCIBus (for bus 
>>>>> number)              */
>>>>>        uint8_t devfn;              /* device 
>>>>> function                      */
>>>>>        AMDVIState *iommu_state;    /* AMDVI - one per 
>>>>> machine              */
>>>>>        MemoryRegion root;          /* AMDVI Root memory map 
>>>>> region         */
>>>>> @@ -101,6 +101,11 @@ typedef enum AMDVIFaultReason {
>>>>>        AMDVI_FR_PT_ENTRY_INV,      /* Failure to read PTE from 
>>>>> guest memory */
>>>>>    } AMDVIFaultReason;
>>>>> +typedef struct amdvi_as_key {
>>>>> +    PCIBus *bus;
>>>>> +    uint8_t devfn;
>>>>> +} amdvi_as_key;
>>>>> +
>>>>>    uint64_t amdvi_extended_feature_register(AMDVIState *s)
>>>>>    {
>>>>>        uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
>>>>
>>>> Pls fix structure and typedef names according to the QEMU
>>>> coding style. Thanks!
>>>>
>>>
>>> This is something I am struggling with, because the name
>>> `AMDVIASKey` does not offer readability.
>>
>> AMDVIAsKey
>>
>>
>> Or you can update all code to use AmdVi and get AmdViAsKey if you 
>> prefer.
>>
>>
>>> Maybe we can come
>>> up with an alternate style which is readable and does not
>>> differ much from the current style.
>>>
>>> @alejandro any suggestions ?
>>>
>
> I should have pointed out the CamelCase requirement for the typedef on 
> v1. My initial reaction was: "do not use typedef" and go with the 
> slightly longer 'struct amdvi_as_key' instead. The style guide has a 
> warning about typedefs (which doesn't necessarily apply here), but IMO 
> still better to avoid it in this case were we are not really gaining 
> much from it.
>
> If I were to use a typedef I would use 'AMDViAsKey'. After all, the 
> 'i' in AMD-Vi and 'd' in VT-d are lowercase ;)
>
> But my opinion is to avoid the typedef altogether.
>
>>>>> @@ -382,6 +387,44 @@ static guint amdvi_uint64_hash(gconstpointer v)
>>>>>        return (guint)*(const uint64_t *)v;
>>>>>    }
>>>>> +static gboolean amdvi_as_equal(gconstpointer v1, gconstpointer v2)
>>>>> +{
>>>>> +    const struct amdvi_as_key *key1 = v1;
>>>>> +    const struct amdvi_as_key *key2 = v2;
>>>>> +
>>>>> +    return key1->bus == key2->bus && key1->devfn == key2->devfn;
>>>>> +}
>>>>> +
>>>>> +static guint amdvi_as_hash(gconstpointer v)
>>>>> +{
>>>>> +    const struct amdvi_as_key *key = v;
>>>>> +    guint bus = (guint)(uintptr_t)key->bus;
>>>>> +
>>>>> +    return (guint)(bus << 8 | (uint)key->devfn);
>>>>> +}
>>>>> +
>>>>> +static AMDVIAddressSpace *amdvi_as_lookup(AMDVIState *s, PCIBus 
>>>>> *bus,
>>>>> +                                          uint8_t devfn)
>>>>> +{
>>>>> +    amdvi_as_key key = { .bus = bus, .devfn = devfn };
>>>>> +    return g_hash_table_lookup(s->address_spaces, &key);
>>>>> +}
>>>>> +
>>>>> +gboolean amdvi_find_as_by_devid(gpointer key, gpointer value,
>>>>> +                                  gpointer user_data)
>>>>> +{
>>>>> +    amdvi_as_key *as = (struct amdvi_as_key *)key;
>>>> this assignment does not need a cast I think.
>>>>
> Agree. And assuming you take my advice of not using a typedef, the 
> line should be:
>
> const struct amdvi_as_key *as = key;
>
> to follow the style guide directive of using "const-correct" pointers. 
> Putting back the static qualifier from my earlier reply:
>
> static gboolean amdvi_find_as_by_devid(gpointer key, gpointer value,
>                                        gpointer user_data)
> {
>     const struct amdvi_as_key *as = key;
>     const uint16_t *devidp = user_data;
>
>
>     return *devidp == PCI_BUILD_BDF(pci_bus_num(as->bus), as->devfn);
> }
>

Sure. From now on we will avoid typedefs in the AMD-Vi and I liked your 
suggestion of
'struct AMDViAsKey'. Should I update entire code with this new naming 
style ?

Thanks
Sairaj

> Thank you,
> Alejandro
>
>>>>> +    uint16_t devid = *((uint16_t *)user_data);
>>>> would be better like this:
>>>>         uint16_t * devidp = user_data;
>>>> then just use *devidp instead of devid.
>>>
>>> sure
>>>
>>> Thanks
>>> Sairaj
>>
>

On Tue, Oct 14, 2025 at 05:46:40PM -0400, Alejandro Jimenez wrote:
> 
> 
> On 10/14/25 5:02 AM, Michael S. Tsirkin wrote:
> > On Tue, Oct 14, 2025 at 11:13:51AM +0530, Sairaj Kodilkar wrote:
> > > 
> > > 
> > > On 10/13/2025 1:45 PM, Michael S. Tsirkin wrote:
> > > > On Mon, Oct 13, 2025 at 10:30:45AM +0530, Sairaj Kodilkar wrote:
> > > > > The AMD IOMMU is set up at boot time and uses PCI bus numbers + devfn
> > > > > for indexing into DTE. The problem is that before the guest started,
> > > > > all PCI bus numbers are 0 as no PCI discovery happened yet (BIOS or/and
> > > > > kernel will do that later) so relying on the bus number is wrong.
> > > > > The immediate effect is emulated devices cannot do DMA when places on
> > > > > a bus other that 0.
> > > > > 
> > > > > Replace static array of address_space with hash table which uses devfn and
> > > > > PCIBus* for key as it is not going to change after the guest is booted.
> > > > I am curious whether this has any measureable impact on
> > > > performance.
> > > 
> > > I dont think it should have much performance impact, as guest usually has
> > > small number of devices attached to it and hash has O(1) average search cost
> > > when hash key function is good.
> > > 
> > > > 
> > > > > Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
> > > > > Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> > > > > Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> > > > 
> > > > love the patch! yet something to improve:
> > > > 
> > > > > ---
> > > > >    hw/i386/amd_iommu.c | 134 ++++++++++++++++++++++++++------------------
> > > > >    hw/i386/amd_iommu.h |   2 +-
> > > > >    2 files changed, 79 insertions(+), 57 deletions(-)
> > > > > 
> > > > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> > > > > index 378e0cb55eab..b194e3294dd7 100644
> > > > > --- a/hw/i386/amd_iommu.c
> > > > > +++ b/hw/i386/amd_iommu.c
> > > > > @@ -59,7 +59,7 @@ const char *amdvi_mmio_high[] = {
> > > > >    };
> > > > >    struct AMDVIAddressSpace {
> > > > > -    uint8_t bus_num;            /* bus number                           */
> > > > > +    PCIBus *bus;                /* PCIBus (for bus number)              */
> > > > >        uint8_t devfn;              /* device function                      */
> > > > >        AMDVIState *iommu_state;    /* AMDVI - one per machine              */
> > > > >        MemoryRegion root;          /* AMDVI Root memory map region         */
> > > > > @@ -101,6 +101,11 @@ typedef enum AMDVIFaultReason {
> > > > >        AMDVI_FR_PT_ENTRY_INV,      /* Failure to read PTE from guest memory */
> > > > >    } AMDVIFaultReason;
> > > > > +typedef struct amdvi_as_key {
> > > > > +    PCIBus *bus;
> > > > > +    uint8_t devfn;
> > > > > +} amdvi_as_key;
> > > > > +
> > > > >    uint64_t amdvi_extended_feature_register(AMDVIState *s)
> > > > >    {
> > > > >        uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
> > > > 
> > > > Pls fix structure and typedef names according to the QEMU
> > > > coding style. Thanks!
> > > > 
> > > 
> > > This is something I am struggling with, because the name
> > > `AMDVIASKey` does not offer readability.
> > 
> > AMDVIAsKey
> > 
> > 
> > Or you can update all code to use AmdVi and get AmdViAsKey if you prefer.
> > 
> > 
> > > Maybe we can come
> > > up with an alternate style which is readable and does not
> > > differ much from the current style.
> > > 
> > > @alejandro any suggestions ?
> > > 
> 
> I should have pointed out the CamelCase requirement for the typedef on v1.
> My initial reaction was: "do not use typedef" and go with the slightly
> longer 'struct amdvi_as_key' instead.

Sorry, that's a coding style violation too :)


	Typedefs
	--------

	Typedefs are used to eliminate the redundant 'struct' keyword, since type
	names have a different style than other identifiers ("CamelCase" versus
	"snake_case").  Each named struct type should have a CamelCase name and a
	corresponding typedef.

the only exceptions we make is when we import headers
from outside libraries to interface with them. 


> The style guide has a warning about
> typedefs (which doesn't necessarily apply here), but IMO still better to
> avoid it in this case were we are not really gaining much from it.

not sure which warning you mean, or why would not it apply.


> If I were to use a typedef I would use 'AMDViAsKey'. After all, the 'i' in
> AMD-Vi and 'd' in VT-d are lowercase ;)

Sounds good.

> But my opinion is to avoid the typedef altogether.

On 10/15/25 3:32 AM, Michael S. Tsirkin wrote:
> On Tue, Oct 14, 2025 at 05:46:40PM -0400, Alejandro Jimenez wrote:
>>
>>
>> On 10/14/25 5:02 AM, Michael S. Tsirkin wrote:
>>> On Tue, Oct 14, 2025 at 11:13:51AM +0530, Sairaj Kodilkar wrote:
>>>>
>>>>
>>>> On 10/13/2025 1:45 PM, Michael S. Tsirkin wrote:
>>>>> On Mon, Oct 13, 2025 at 10:30:45AM +0530, Sairaj Kodilkar wrote:
>>>>>> The AMD IOMMU is set up at boot time and uses PCI bus numbers + devfn
>>>>>> for indexing into DTE. The problem is that before the guest started,
>>>>>> all PCI bus numbers are 0 as no PCI discovery happened yet (BIOS or/and
>>>>>> kernel will do that later) so relying on the bus number is wrong.
>>>>>> The immediate effect is emulated devices cannot do DMA when places on
>>>>>> a bus other that 0.
>>>>>>
>>>>>> Replace static array of address_space with hash table which uses devfn and
>>>>>> PCIBus* for key as it is not going to change after the guest is booted.
>>>>> I am curious whether this has any measureable impact on
>>>>> performance.
>>>>
>>>> I dont think it should have much performance impact, as guest usually has
>>>> small number of devices attached to it and hash has O(1) average search cost
>>>> when hash key function is good.
>>>>
>>>>>
>>>>>> Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>>>>>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>>>>>
>>>>> love the patch! yet something to improve:
>>>>>
>>>>>> ---
>>>>>>     hw/i386/amd_iommu.c | 134 ++++++++++++++++++++++++++------------------
>>>>>>     hw/i386/amd_iommu.h |   2 +-
>>>>>>     2 files changed, 79 insertions(+), 57 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>>>>> index 378e0cb55eab..b194e3294dd7 100644
>>>>>> --- a/hw/i386/amd_iommu.c
>>>>>> +++ b/hw/i386/amd_iommu.c
>>>>>> @@ -59,7 +59,7 @@ const char *amdvi_mmio_high[] = {
>>>>>>     };
>>>>>>     struct AMDVIAddressSpace {
>>>>>> -    uint8_t bus_num;            /* bus number                           */
>>>>>> +    PCIBus *bus;                /* PCIBus (for bus number)              */
>>>>>>         uint8_t devfn;              /* device function                      */
>>>>>>         AMDVIState *iommu_state;    /* AMDVI - one per machine              */
>>>>>>         MemoryRegion root;          /* AMDVI Root memory map region         */
>>>>>> @@ -101,6 +101,11 @@ typedef enum AMDVIFaultReason {
>>>>>>         AMDVI_FR_PT_ENTRY_INV,      /* Failure to read PTE from guest memory */
>>>>>>     } AMDVIFaultReason;
>>>>>> +typedef struct amdvi_as_key {
>>>>>> +    PCIBus *bus;
>>>>>> +    uint8_t devfn;
>>>>>> +} amdvi_as_key;
>>>>>> +
>>>>>>     uint64_t amdvi_extended_feature_register(AMDVIState *s)
>>>>>>     {
>>>>>>         uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
>>>>>
>>>>> Pls fix structure and typedef names according to the QEMU
>>>>> coding style. Thanks!
>>>>>
>>>>
>>>> This is something I am struggling with, because the name
>>>> `AMDVIASKey` does not offer readability.
>>>
>>> AMDVIAsKey
>>>
>>>
>>> Or you can update all code to use AmdVi and get AmdViAsKey if you prefer.
>>>
>>>
>>>> Maybe we can come
>>>> up with an alternate style which is readable and does not
>>>> differ much from the current style.
>>>>
>>>> @alejandro any suggestions ?
>>>>
>>
>> I should have pointed out the CamelCase requirement for the typedef on v1.
>> My initial reaction was: "do not use typedef" and go with the slightly
>> longer 'struct amdvi_as_key' instead.
> 
> Sorry, that's a coding style violation too :)
> 

ACK

> 
> 	Typedefs
> 	--------
> 
> 	Typedefs are used to eliminate the redundant 'struct' keyword, since type
> 	names have a different style than other identifiers ("CamelCase" versus
> 	"snake_case").  Each named struct type should have a CamelCase name and a
> 	corresponding typedef.
> 

I hadn't parsed the mandatory nature of typedefs for named structs the 
last sentence is setting. Thank you for pointing it out.

> the only exceptions we make is when we import headers
> from outside libraries to interface with them.
> 
> 
>> The style guide has a warning about
>> typedefs (which doesn't necessarily apply here), but IMO still better to
>> avoid it in this case were we are not really gaining much from it.
> 
> not sure which warning you mean, or why would not it apply.
> 
I meant the duplicated typedefs portion, which is irrelevant for this 
discussion I believe.

> 
>> If I were to use a typedef I would use 'AMDViAsKey'. After all, the 'i' in
>> AMD-Vi and 'd' in VT-d are lowercase ;)
> 
> Sounds good.
> 

Looking back at the precedent in the current files, I think your initial 
proposal of AMDVIAsKey is the best choice for consistency at this point.

So if we have consensus on the typedef requirement and the name 
AMDVIAsKey, I'd say the other new struct type introduced in [PATCH 2/2] 
must use the same scheme i.e.

typedef struct AMDVIIOTLBKey {
     uint64_t gfn;
     uint16_t devid;
} AMDVIIOTLBKey

AMDVIIOTLBKey is a bit awkward, but closely matches the existing 
AMDVIIOTLBEntry type, so hopefully that is not controversial.

Thank you,
Alejandro


>> But my opinion is to avoid the typedef altogether.
>