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 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.