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 <[email protected]>
> > > Signed-off-by: Alexey Kardashevskiy <[email protected]>
> > > Signed-off-by: Sairaj Kodilkar <[email protected]>
> >
> > 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
