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