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

On 8/1/2026 3:17 AM, 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?
> 
> Sairaj: Am I missing anything, perhaps from your unpublished patches, that
> makes this approach non-viable?

Hi Alejandro,

The Aim of this cleanup is to allow devices and IOMMU isolation under
PCI-PXB and PCI-ROOT-PORT in future patches. Please see patches at [1]
for more information.

Examples
--------
Example1, For below PCI configuration AMD IOMMU 1 will serve all the
devices under pxb-pci1 and AMD IOMMU 2 will serve all the devices under
pxb-pci2.

pcie.0 -+- pxb-pci1 -+- pcie-root-port1.1 --- AMD IOMMU 1
        |            +- pcie-root-port1.2 --- Device1
        |            \- pcie-root-port1.3 --- Device2
        \- pxb-pci2 -+- pcie-root-port2.1 --- AMD IOMMU 2
                     +- pcie-root-port2.2 --- Device3
                     \- pcie-root-port2.3 --- Device4

Example2, For below PCI configuration, AMD IOMMU 3 will only serve
Device5 and Device6 and devices under pxb-pci buses will be served by
IOMMUs of respective buses.

pcie.0 -+- pxb-pci1 -+- pcie-root-port1.1 --- AMD IOMMU 1
        |            +- pcie-root-port1.2 --- Device1
        |            \- pcie-root-port1.3 --- Device2
        +- pxb-pci2 -+- pcie-root-port2.1 --- AMD IOMMU 2
        |            +- pcie-root-port2.2 --- Device3
        |            \- pcie-root-port2.3 --- Device4
        +--- AMD IOMMU 3
        +--- Device5
        \--- Device6

Example3, The below PCI configuration is invalid since both IOMMUs share
same root bus i.e. pxb-pci1.

pcie.0 -+- pxb-pci1 -+- pcie-root-port1 --- AMD IOMMU 1
                     +- pcie-root-port2 --- AMD IOMMU 2
                     +- pcie-root-port3 --- Device2
                     \- pcie-root-port4 --- Device3
----

Hardware vIOMMU requires this isolation since it does not support
emulated devices (due to accelerated MMIO region). Enforcing IOMMU on
pcie.0 will not work for vIOMMU.

Furthurmore, we need multiple hw-vIOMMUs when vfio devices do not share
host IOMMUs (one hw-vIOMMU device for each host IOMMU)

Considering above facts, we cannot enforce pcie.0 for the IOMMU, but
future patches ensure that IOMMU is attached to a PCI root port.

Also, Outline for vIOMMU developement will be as follow
1. IVRS cleanup
2. IOMMU PCI bus isolation support
3. Multiple vIOMMU support
4. New vIOMMU model (-device amd-viommu)

@Alejandro, does this answer your question ? or let me know if you need
an overview of upcoming changes to evaluate this

[1] https://github.com/AMDESE/qemu-iommu/tree/wip/amd-viommu_20260728

Thanks
Sairaj

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