Re: [PATCH 4/8] acpi_build: Use IOMMU pci device to build IOMMU device ID

On Fri, Jul 31, 2026 at 05:47:18PM -0400, Alejandro Jimenez wrote:
> 
> On 7/31/26 8:32 AM, Sairaj Kodilkar wrote:
> > On 7/31/2026 3: 20 PM, Michael S. Tsirkin wrote: > On Mon, May 11, 2026 at
> > 06: 09: 33PM +0530, Sairaj Kodilkar wrote: >> Currently, build_amd_iommu()
> > uses "addr" property to build the device ID for >> IOMMU device and
> > advertise it
> > 
> > 
> > On 7/31/2026 3:20 PM, Michael S. Tsirkin wrote:
> >> On Mon, May 11, 2026 at 06:09:33PM +0530, Sairaj Kodilkar wrote:
> >>> Currently, build_amd_iommu() uses "addr" property to build the device ID for
> >>> IOMMU device and advertise it throught IVRS. But this property does not encode
> >>> IOMMU bus. This will be a problem if IOMMU is attached to different bus.
> >>> Hence use iommu pci device which provides bus, to build the IOMMU device ID.
> >>>
> >>> Signed-off-by: Sairaj Kodilkar <[email protected]>
> >>> Reviewed-by: Vasant Hegde <[email protected]>
> >> 
> >> But is this called after firmware has enumerated the pci bus?
> >> And I guess OS better not change that bus number eh?
> 
> The above line sounds fairly threatening :) so I dug a bit more into the
> details.
> Short story: I tested placing the IOMMU behind a PCI bridge, which the
> implementation currently allows, and the guest can (easily) change the bus
> number. That changes the IOMMU BDF, so the DeviceID encoded in IVRS becomes
> incorrect.
> 
> The spec doesn't forbid the above scenario, but "strongly recommends":
> 
> • An IOMMU should be a root-complex device (i.e., appear directly on the
> bus at the top of the PCI tree hierarchy).
> • Some system software may prohibit an IOMMU from appearing under a
> PCI-to-PCI bridge.
> 
> (from Section 4.5 Software and Platform Firmware Implementation Issues)
> 
> While I don't know if that is the case in all HW implementations, my
> Genoa/Zen4 system does follow the recommended topology i.e. it exposes all
> of its 8 IOMMU functions directly on a PCI root bus, with no bridges.
> 
> So I think a reasonable choice is to enforce that an AMDVI-PCI device must
> always be directly attached to a PCI root bus. There is precedent for this
> in the virtio-iommu implementation already, the code change would be
> basically the same in amdvi_pci_realize(), see:
> 
> e72cfabf4ef2 ("hw/virtio/virtio-iommu-pci: Enforce the device is plugged on
> the root bus")
> 
> MST: does this address your concern?

Indeed, it is.

> Sairaj: Am I missing anything, perhaps from your unpublished patches, that
> makes this approach non-viable?
> 
> Thank you,
> Alejandro
> 
> > 
> > Hi Michael
> > 
> > Yes, the ACPI function is called two times -- during qemu initialization
> > and firmware writes. During first call, bus numbers are 0 and during
> > second call, IVRS is created with bus number. This second IVRS
> > overwrites the first one.
> > 
> > Thanks
> > Sairaj
> > 
> >>> ---
> >>>  hw/i386/acpi-build.c | 12 ++++++------
> >>>  1 file changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>> index e4ad01eec037..718e3f546b18 100644
> >>> --- a/hw/i386/acpi-build.c
> >>> +++ b/hw/i386/acpi-build.c
> >>> @@ -1752,10 +1752,13 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> >>>                  const char *oem_table_id)
> >>>  {
> >>>      AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
> >>> +    PCIDevice *iommu_dev = &(s->pci->dev);
> >>>      GArray *ivhd_blob = g_array_new(false, true, 1);
> >>>      AcpiTable table = { .sig = "IVRS", .rev = 1, .oem_id = oem_id,
> >>>                          .oem_table_id = oem_table_id };
> >>>      uint64_t feature_report;
> >>> +    int iommu_bus = pci_bus_num(pci_get_bus(iommu_dev));
> >>> +    uint16_t iommu_devid = PCI_BUILD_BDF(iommu_bus, iommu_dev->devfn);
> >>>  
> >>>      acpi_table_begin(&table, table_data);
> >>>      /* IVinfo - IO virtualization information common to all
> >>> @@ -1816,9 +1819,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> >>>      /* IVHD length */
> >>>      build_append_int_noprefix(table_data, ivhd_blob->len + 24, 2);
> >>>      /* DeviceID */
> >>> -    build_append_int_noprefix(table_data,
> >>> -                              object_property_get_int(OBJECT(s->pci), "addr",
> >>> -                                                      &error_abort), 2);
> >>> +    build_append_int_noprefix(table_data, iommu_devid, 2);
> >>>      /* Capability offset */
> >>>      build_append_int_noprefix(table_data, s->pci->capab_offset, 2);
> >>>      /* IOMMU base address */
> >>> @@ -1850,10 +1851,9 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> >>>  
> >>>      /* IVHD length */
> >>>      build_append_int_noprefix(table_data, ivhd_blob->len + 40, 2);
> >>> +
> >>>      /* DeviceID */
> >>> -    build_append_int_noprefix(table_data,
> >>> -                              object_property_get_int(OBJECT(s->pci), "addr",
> >>> -                                                      &error_abort), 2);
> >>> +    build_append_int_noprefix(table_data, iommu_devid, 2);
> >>>      /* Capability offset */
> >>>      build_append_int_noprefix(table_data, s->pci->capab_offset, 2);
> >>>      /* IOMMU base address */
> >>> -- 
> >>> 2.34.1
> >> 
> >