[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