[PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus

[PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus

[Eric Auger]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3133 bytes --]

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

Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus

[Nathan Chen]

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

RE: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus

[Shameer Kolothum Thodi]

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

Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus

[Nicolin Chen]

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>

Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus

[Eric Auger]

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
>

Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus

[Philippe Mathieu-Daudé]

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);
>           }

Re: [PATCH] hw/pci/pci: Enforce pci_setup_iommu_per_bus() is called only once per bus

[Eric Auger]

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