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

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?

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