Re: [PATCH v2 1/2] amd_iommu: Fix handling device on buses != 0

["Michael S. Tsirkin" <mst@redhat.com>]

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