* Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus
2026-03-23 18:30 [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus Eric Auger
@ 2026-03-24 17:25 ` Nathan Chen
2026-03-30 8:59 ` Shameer Kolothum Thodi
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Nathan Chen @ 2026-03-24 17:25 UTC (permalink / raw)
To: eric.auger
Cc: eric.auger.pro, mst, nathanc, nicolinc, peter.maydell, qemu-arm,
qemu-devel, skolothumtho
Hi Eric,
> Currently it is possible to attach several arm-smmuv3 devices to the
> same bus although it is a wrong setup.
>
> Change the prototype of pci_setup_iommu_per_bus to pass an error
> handle. This latter is set when iommu_per_bus is already set and
> used by the single caller (smmu_base_realize) to report a useful
> error to the end-user.
>
> While at it document pci_setup_iommu_per_bus callback in the header.
>
> Fixes: 66d2f665e163 ("hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
> include/hw/pci/pci.h | 16 +++++++++++++++-
> hw/arm/smmu-common.c | 2 +-
> hw/pci/pci.c | 9 +++++++--
> 3 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 5b179091dee..f2448e941a0 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -863,7 +863,21 @@ int pci_iommu_unregister_iotlb_notifier(PCIDevice *dev, uint32_t pasid,
> */
> void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque);
>
> -void pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque);
> +/**
> + * pci_setup_iommu_per_bus: Initialize specific IOMMU handlers for a PCIBus
> + *
> + * Similar to pci_setup_iommu but enforces that the iommu only protects
> + * @bus downstream end points and no other bus hierarchy
> + *
> + * @bus: the #PCIBus being updated.
> + * @ops: the #PCIIOMMUOps
> + * @opaque: passed to callbacks of the @ops structure.
> + * @errp: error handle
> + *
> + * Returns false on failure with @errp set, true on success
> + */
> +bool pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
> + void *opaque, Error **errp);
>
> pcibus_t pci_bar_address(PCIDevice *d,
> int reg, uint8_t type, pcibus_t size);
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 58c4452b1f5..a3aeb5a8796 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -981,7 +981,7 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
> }
>
> if (s->smmu_per_bus) {
> - pci_setup_iommu_per_bus(pci_bus, s->iommu_ops, s);
> + pci_setup_iommu_per_bus(pci_bus, s->iommu_ops, s, errp);
> } else {
> pci_setup_iommu(pci_bus, s->iommu_ops, s);
> }
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2c3657d00de..5610193783d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -3307,11 +3307,16 @@ void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
> * IOMMU ops are returned, avoiding the use of the parent’s IOMMU when
> * it's not appropriate.
> */
> -void pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
> - void *opaque)
> +bool pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
> + void *opaque, Error **errp)
> {
> + if (bus->iommu_per_bus) {
> + error_setg(errp, "An iommu is already attached to this bus");
> + return false;
> + }
> pci_setup_iommu(bus, ops, opaque);
> bus->iommu_per_bus = true;
> + return true;
> }
>
> static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
I verified that assigning multiple SMMUv3 to the same bus is prevented
by this patch, and confirmed that basic sanity testing is unaffected in
the normal case with one SMMUv3 per bus (GPU passthrough with CUDA test
apps).
Tested-by: Nathan Chen <nathanc@nvidia.com>
Thanks,
Nathan
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus
2026-03-23 18:30 [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus Eric Auger
2026-03-24 17:25 ` Nathan Chen
@ 2026-03-30 8:59 ` Shameer Kolothum Thodi
2026-03-30 9:06 ` Nicolin Chen
2026-03-31 17:59 ` Philippe Mathieu-Daudé
3 siblings, 0 replies; 7+ messages in thread
From: Shameer Kolothum Thodi @ 2026-03-30 8:59 UTC (permalink / raw)
To: Eric Auger, eric.auger.pro@gmail.com, qemu-devel@nongnu.org,
qemu-arm@nongnu.org, peter.maydell@linaro.org, mst@redhat.com,
Nicolin Chen, Nathan Chen
> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: 23 March 2026 18:30
> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu-
> devel@nongnu.org; qemu-arm@nongnu.org; peter.maydell@linaro.org;
> mst@redhat.com; Shameer Kolothum Thodi <skolothumtho@nvidia.com>;
> Nicolin Chen <nicolinc@nvidia.com>; Nathan Chen <nathanc@nvidia.com>
> Subject: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called
> only once per bus
>
> External email: Use caution opening links or attachments
>
>
> Currently it is possible to attach several arm-smmuv3 devices to the
> same bus although it is a wrong setup.
>
> Change the prototype of pci_setup_iommu_per_bus to pass an error
> handle. This latter is set when iommu_per_bus is already set and
> used by the single caller (smmu_base_realize) to report a useful
> error to the end-user.
>
> While at it document pci_setup_iommu_per_bus callback in the header.
>
> Fixes: 66d2f665e163 ("hw/arm/virt: Allow user-creatable SMMUv3 dev
> instantiation")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
LGTM,
Reviewed-by: Shameer Kolothum <skolothumtho@nvidia.com>
Thanks,
Shameer
> ---
> include/hw/pci/pci.h | 16 +++++++++++++++-
> hw/arm/smmu-common.c | 2 +-
> hw/pci/pci.c | 9 +++++++--
> 3 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 5b179091dee..f2448e941a0 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -863,7 +863,21 @@ int pci_iommu_unregister_iotlb_notifier(PCIDevice
> *dev, uint32_t pasid,
> */
> void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void
> *opaque);
>
> -void pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
> void *opaque);
> +/**
> + * pci_setup_iommu_per_bus: Initialize specific IOMMU handlers for a
> PCIBus
> + *
> + * Similar to pci_setup_iommu but enforces that the iommu only protects
> + * @bus downstream end points and no other bus hierarchy
> + *
> + * @bus: the #PCIBus being updated.
> + * @ops: the #PCIIOMMUOps
> + * @opaque: passed to callbacks of the @ops structure.
> + * @errp: error handle
> + *
> + * Returns false on failure with @errp set, true on success
> + */
> +bool pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
> + void *opaque, Error **errp);
>
> pcibus_t pci_bar_address(PCIDevice *d,
> int reg, uint8_t type, pcibus_t size);
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 58c4452b1f5..a3aeb5a8796 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -981,7 +981,7 @@ static void smmu_base_realize(DeviceState *dev,
> Error **errp)
> }
>
> if (s->smmu_per_bus) {
> - pci_setup_iommu_per_bus(pci_bus, s->iommu_ops, s);
> + pci_setup_iommu_per_bus(pci_bus, s->iommu_ops, s, errp);
> } else {
> pci_setup_iommu(pci_bus, s->iommu_ops, s);
> }
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2c3657d00de..5610193783d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -3307,11 +3307,16 @@ void pci_setup_iommu(PCIBus *bus, const
> PCIIOMMUOps *ops, void *opaque)
> * IOMMU ops are returned, avoiding the use of the parentâEUR(tm)s
> IOMMU when
> * it's not appropriate.
> */
> -void pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
> - void *opaque)
> +bool pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
> + void *opaque, Error **errp)
> {
> + if (bus->iommu_per_bus) {
> + error_setg(errp, "An iommu is already attached to this bus");
> + return false;
> + }
> pci_setup_iommu(bus, ops, opaque);
> bus->iommu_per_bus = true;
> + return true;
> }
>
> static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> --
> 2.53.0
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus
2026-03-23 18:30 [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus Eric Auger
2026-03-24 17:25 ` Nathan Chen
2026-03-30 8:59 ` Shameer Kolothum Thodi
@ 2026-03-30 9:06 ` Nicolin Chen
2026-03-31 15:06 ` Eric Auger
2026-03-31 17:59 ` Philippe Mathieu-Daudé
3 siblings, 1 reply; 7+ messages in thread
From: Nicolin Chen @ 2026-03-30 9:06 UTC (permalink / raw)
To: Eric Auger
Cc: eric.auger.pro, qemu-devel, qemu-arm, peter.maydell, mst,
skolothumtho, nathanc
On Mon, Mar 23, 2026 at 07:30:20PM +0100, Eric Auger wrote:
> @@ -3307,11 +3307,16 @@ void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
> * IOMMU ops are returned, avoiding the use of the parent’s IOMMU when
> * it's not appropriate.
> */
> -void pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
> - void *opaque)
> +bool pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
> + void *opaque, Error **errp)
> {
> + if (bus->iommu_per_bus) {
> + error_setg(errp, "An iommu is already attached to this bus");
> + return false;
> + }
> pci_setup_iommu(bus, ops, opaque);
> bus->iommu_per_bus = true;
> + return true;
Do we need the same check in pci_setup_iommu()?
Apart from this question,
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus
2026-03-30 9:06 ` Nicolin Chen
@ 2026-03-31 15:06 ` Eric Auger
2026-06-03 12:40 ` Eric Auger
0 siblings, 1 reply; 7+ messages in thread
From: Eric Auger @ 2026-03-31 15:06 UTC (permalink / raw)
To: Nicolin Chen
Cc: eric.auger.pro, qemu-devel, qemu-arm, peter.maydell, mst,
skolothumtho, nathanc
On 3/30/26 11:06 AM, Nicolin Chen wrote:
> On Mon, Mar 23, 2026 at 07:30:20PM +0100, Eric Auger wrote:
>> @@ -3307,11 +3307,16 @@ void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
>> * IOMMU ops are returned, avoiding the use of the parent’s IOMMU when
>> * it's not appropriate.
>> */
>> -void pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
>> - void *opaque)
>> +bool pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
>> + void *opaque, Error **errp)
>> {
>> + if (bus->iommu_per_bus) {
>> + error_setg(errp, "An iommu is already attached to this bus");
>> + return false;
>> + }
>> pci_setup_iommu(bus, ops, opaque);
>> bus->iommu_per_bus = true;
>> + return true;
> Do we need the same check in pci_setup_iommu()?
For machine wide IOMMU the check is handled in machine code (at least in
arm virt).
Nevertheless a more generic solution could be to check in
pci_setup_iommu() that bus->iommu_ops is not already set. If set, return
an error, cascaded up through pci_setup_iommu() and
pci_setup_iommu_per_bus().
However the change is much more invasive and impacts several archs. What
do you think?
I don't think this can be a valid use case to call pci_setup_iommu()
several times on the same bus?
>
> Apart from this question,
>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Thanks
Eric
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus
2026-03-31 15:06 ` Eric Auger
@ 2026-06-03 12:40 ` Eric Auger
0 siblings, 0 replies; 7+ messages in thread
From: Eric Auger @ 2026-06-03 12:40 UTC (permalink / raw)
To: eric.auger, Nicolin Chen
Cc: eric.auger.pro, qemu-devel, qemu-arm, peter.maydell, mst,
skolothumtho, nathanc
Hi Nicolin,
On 3/31/26 5:06 PM, Eric Auger wrote:
>
>
> On 3/30/26 11:06 AM, Nicolin Chen wrote:
>> On Mon, Mar 23, 2026 at 07:30:20PM +0100, Eric Auger wrote:
>>> @@ -3307,11 +3307,16 @@ void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
>>> * IOMMU ops are returned, avoiding the use of the parent’s IOMMU when
>>> * it's not appropriate.
>>> */
>>> -void pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
>>> - void *opaque)
>>> +bool pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
>>> + void *opaque, Error **errp)
>>> {
>>> + if (bus->iommu_per_bus) {
>>> + error_setg(errp, "An iommu is already attached to this bus");
>>> + return false;
>>> + }
>>> pci_setup_iommu(bus, ops, opaque);
>>> bus->iommu_per_bus = true;
>>> + return true;
>> Do we need the same check in pci_setup_iommu()?
I further looked at it. pci_setup_iommu registers a machine wide iommu.
Only ARM uses pci_setup_iommu_per_bus for now and usage of both
pci_setup_iommu and pci_setup_iommu_per_bus are exclusive and check is
done at machine level.
So I think it is reasonable for now to keep that change limited to
pci_setup_iommu_per_bus.
Needs a rebase though, including Philippe's suggestion.
Thanks
Eric
>
> For machine wide IOMMU the check is handled in machine code (at least in
> arm virt).
>
> Nevertheless a more generic solution could be to check in
> pci_setup_iommu() that bus->iommu_ops is not already set. If set, return
> an error, cascaded up through pci_setup_iommu() and
> pci_setup_iommu_per_bus().
> However the change is much more invasive and impacts several archs. What
> do you think?
>
> I don't think this can be a valid use case to call pci_setup_iommu()
> several times on the same bus?
>>
>> Apart from this question,
>>
>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>
> Thanks
>
> Eric
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus
2026-03-23 18:30 [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus Eric Auger
` (2 preceding siblings ...)
2026-03-30 9:06 ` Nicolin Chen
@ 2026-03-31 17:59 ` Philippe Mathieu-Daudé
3 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-03-31 17:59 UTC (permalink / raw)
To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell,
mst, skolothumtho, nicolinc, nathanc
On 23/3/26 19:30, Eric Auger wrote:
> Currently it is possible to attach several arm-smmuv3 devices to the
> same bus although it is a wrong setup.
>
> Change the prototype of pci_setup_iommu_per_bus to pass an error
> handle. This latter is set when iommu_per_bus is already set and
> used by the single caller (smmu_base_realize) to report a useful
> error to the end-user.
>
> While at it document pci_setup_iommu_per_bus callback in the header.
>
> Fixes: 66d2f665e163 ("hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
> include/hw/pci/pci.h | 16 +++++++++++++++-
> hw/arm/smmu-common.c | 2 +-
> hw/pci/pci.c | 9 +++++++--
> 3 files changed, 23 insertions(+), 4 deletions(-)
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 58c4452b1f5..a3aeb5a8796 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -981,7 +981,7 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
> }
>
> if (s->smmu_per_bus) {
> - pci_setup_iommu_per_bus(pci_bus, s->iommu_ops, s);
> + pci_setup_iommu_per_bus(pci_bus, s->iommu_ops, s, errp);
Should we return if the caller returned false? More code could be added
at the end of this function.
> } else {
> pci_setup_iommu(pci_bus, s->iommu_ops, s);
> }
^ permalink raw reply [flat|nested] 7+ messages in thread