[PATCH v2 0/6] Add support for user creatable SMMUv3 device

[PATCH v2 0/6] Add support for user creatable SMMUv3 device

[Shameer Kolothum via]

Hi All,

Changes from v1:
https://lore.kernel.org/qemu-devel/20250415081104.71708-1-shameerali.kolothum.thodi@huawei.com/

Addressed feedback on v1. Thanks to all.
1. Retained the same name as the legacy SMMUv3(arm-smmuv3) for new
   device type as well (instead of arm-smmuv3-dev type usage in v1).
2. Changes to ACPI IORT to use the same struct SMMUv3Device for both
   legacy SMMUv3 and the new SMMUV3 device
3. Removed the restriction of creating SMMUv3 dev if virt ver > 9.2.

Cover letter from v1:

This patch series introduces support for a user-creatable SMMUv3 device
(-device arm-smmuv3) in QEMU.

The implementation is based on feedback received from the RFCv2[0]:
"hw/arm/virt: Add support for user-creatable accelerated SMMUv3"

Currently, QEMU's SMMUv3 emulation (iommu=smmuv3) is tied to the machine
and does not support instantiating multiple SMMUv3 devices—each associated
with a separate PCIe root complex. In contrast, real-world ARM systems
often include multiple SMMUv3 instances, each bound to a different PCIe
root complex.

This also lays the groundwork for supporting accelerated SMMUv3, as
proposed in the aforementioned RFC. Please note, the accelerated SMMUv3
support is not part of this series and will be sent out as a separate
series later on top of this one.

Summary of changes:

 -Introduces support for multiple -device arm-smmuv3-dev,bus=pcie.x
  instances.

  Example usage:

  -device arm-smmuv3,bus=pcie.0
  -device virtio-net-pci,bus=pcie.0
  -device pxb-pcie,id=pcie.1,bus_nr=2
  -device arm-smmuv3,bus=pcie.1
  -device pcie-root-port,id=pcie.port1,bus=pcie.1
  -device virtio-net-pci,bus=pcie.port1

  -Supports either the legacy iommu=smmuv3 option or the new
  "-device arm-smmuv3" model.

  -Adds device tree bindings for the new SMMUv3 device on the arm/virt
   machine only, and only for the default pcie.0 root complex.
   (Note: pxb-pcie root complexes are currently not supported with the
    device tree due to known limitations[1].)

Please take a look and let me know your feedback.

Thanks,
Shameer
[0]:https://lore.kernel.org/qemu-devel/20250311141045.66620-1-shameerali.kolothum.thodi@huawei.com/
[1]:https://lore.kernel.org/qemu-devel/20230421165037.2506-1-Jonathan.Cameron@huawei.com/

Nicolin Chen (1):
  hw/arm/virt: Add an SMMU_IO_LEN macro

Shameer Kolothum (5):
  hw/arm/smmuv3: Add support to associate a PCIe RC
  hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices
  hw/arm/virt: Factor out common SMMUV3 dt bindings code
  hw/arm/virt: Add support for smmuv3 device
  hw/arm/smmuv3: Enable smmuv3 device creation

 hw/arm/smmuv3.c          |  27 +++++++
 hw/arm/virt-acpi-build.c | 162 +++++++++++++++++++++++++++++++--------
 hw/arm/virt.c            | 112 ++++++++++++++++++++-------
 hw/core/sysbus-fdt.c     |   3 +
 include/hw/arm/virt.h    |   1 +
 5 files changed, 244 insertions(+), 61 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameer Kolothum via]

Although this change does not affect functionality at present, it lays
the groundwork for enabling user-created SMMUv3 devices in
future patches

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
 hw/arm/virt.c   |  3 ++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index ab67972353..605de9b721 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -24,6 +24,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/qdev-core.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bridge.h"
 #include "cpu.h"
 #include "exec/target_page.h"
 #include "trace.h"
@@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj, ResetType type)
     smmuv3_init_regs(s);
 }
 
+static int smmuv3_pcie_bus(Object *obj, void *opaque)
+{
+    DeviceState *d = opaque;
+    PCIBus *bus;
+
+    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
+        return 0;
+    }
+
+    bus = PCI_HOST_BRIDGE(obj)->bus;
+    if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus->name)) {
+        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
+                                 &error_abort);
+        /* Return non-zero as we got the bus and don't need further iteration.*/
+        return 1;
+    }
+    return 0;
+}
+
 static void smmu_realize(DeviceState *d, Error **errp)
 {
     SMMUState *sys = ARM_SMMU(d);
@@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d, Error **errp)
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
     Error *local_err = NULL;
 
+    if (!object_property_get_link(OBJECT(d), "primary-bus", &error_abort)) {
+        object_child_foreach_recursive(object_get_root(), smmuv3_pcie_bus, d);
+    }
+
     c->parent_realize(d, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass *klass, const void *data)
     device_class_set_parent_realize(dc, smmu_realize,
                                     &c->parent_realize);
     device_class_set_props(dc, smmuv3_properties);
+    dc->hotpluggable = false;
+    dc->bus_type = TYPE_PCIE_BUS;
 }
 
 static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 177f3dd22c..3bae4e374f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -56,6 +56,7 @@
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
+#include "hw/pci/pci_bus.h"
 #include "hw/pci-host/gpex.h"
 #include "hw/virtio/virtio-pci.h"
 #include "hw/core/sysbus-fdt.h"
@@ -1442,7 +1443,7 @@ static void create_smmu(const VirtMachineState *vms,
     }
     object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
                              &error_abort);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
     for (i = 0; i < NUM_SMMU_IRQS; i++) {
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
-- 
2.34.1

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Nicolin Chen]

On Fri, May 02, 2025 at 11:27:02AM +0100, Shameer Kolothum wrote:
> Although this change does not affect functionality at present, it lays
> the groundwork for enabling user-created SMMUv3 devices in
> future patches
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

With some nits:

> ---
>  hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
>  hw/arm/virt.c   |  3 ++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index ab67972353..605de9b721 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -24,6 +24,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/qdev-core.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bridge.h"

Could probably replace the pci.h since pci_bridge.h includes it.

> +static int smmuv3_pcie_bus(Object *obj, void *opaque)
> +{
> +    DeviceState *d = opaque;
> +    PCIBus *bus;
> +
> +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> +        return 0;
> +    }
> +
> +    bus = PCI_HOST_BRIDGE(obj)->bus;
> +    if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus->name)) {
> +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
> +                                 &error_abort);
> +        /* Return non-zero as we got the bus and don't need further iteration.*/

Missing a space behind the '.'

> +        return 1;
> +    }
> +    return 0;
> +}

> @@ -1442,7 +1443,7 @@ static void create_smmu(const VirtMachineState *vms,
>      }
>      object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>                               &error_abort);
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);

Could add a line of note in the commit message for this change?

RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, May 2, 2025 6:23 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> On Fri, May 02, 2025 at 11:27:02AM +0100, Shameer Kolothum wrote:
> > Although this change does not affect functionality at present, it lays
> > the groundwork for enabling user-created SMMUv3 devices in future
> > patches
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> With some nits:
> 
> > ---
> >  hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
> >  hw/arm/virt.c   |  3 ++-
> >  2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index
> > ab67972353..605de9b721 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -24,6 +24,7 @@
> >  #include "hw/qdev-properties.h"
> >  #include "hw/qdev-core.h"
> >  #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bridge.h"
> 
> Could probably replace the pci.h since pci_bridge.h includes it.

I think it is best not to as it is indirect inclusion.

> 
> > +static int smmuv3_pcie_bus(Object *obj, void *opaque) {
> > +    DeviceState *d = opaque;
> > +    PCIBus *bus;
> > +
> > +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> > +        return 0;
> > +    }
> > +
> > +    bus = PCI_HOST_BRIDGE(obj)->bus;
> > +    if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus-
> >name)) {
> > +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
> > +                                 &error_abort);
> > +        /* Return non-zero as we got the bus and don't need further
> > + iteration.*/
> 
> Missing a space behind the '.'

Sure.

> 
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> 
> > @@ -1442,7 +1443,7 @@ static void create_smmu(const
> VirtMachineState *vms,
> >      }
> >      object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
> >                               &error_abort);
> > -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > +    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
> 
> Could add a line of note in the commit message for this change?

Will do. 

Thanks,
Shameer

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Donald Dutile]

On 5/2/25 6:27 AM, Shameer Kolothum wrote:
> Although this change does not affect functionality at present, it lays
> the groundwork for enabling user-created SMMUv3 devices in
> future patches
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
>   hw/arm/virt.c   |  3 ++-
>   2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index ab67972353..605de9b721 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -24,6 +24,7 @@
>   #include "hw/qdev-properties.h"
>   #include "hw/qdev-core.h"
>   #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bridge.h"
>   #include "cpu.h"
>   #include "exec/target_page.h"
>   #include "trace.h"
> @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj, ResetType type)
>       smmuv3_init_regs(s);
>   }
>   
> +static int smmuv3_pcie_bus(Object *obj, void *opaque)
> +{
> +    DeviceState *d = opaque;
> +    PCIBus *bus;
> +
> +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> +        return 0;
> +    }
> +
> +    bus = PCI_HOST_BRIDGE(obj)->bus;
> +    if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus->name)) {
> +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
> +                                 &error_abort);
> +        /* Return non-zero as we got the bus and don't need further iteration.*/
> +        return 1;
> +    }
> +    return 0;
> +}
> +
>   static void smmu_realize(DeviceState *d, Error **errp)
>   {
>       SMMUState *sys = ARM_SMMU(d);
> @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d, Error **errp)
>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>       Error *local_err = NULL;
>   
> +    if (!object_property_get_link(OBJECT(d), "primary-bus", &error_abort)) {
> +        object_child_foreach_recursive(object_get_root(), smmuv3_pcie_bus, d);
> +    }
> +
>       c->parent_realize(d, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass *klass, const void *data)
>       device_class_set_parent_realize(dc, smmu_realize,
>                                       &c->parent_realize);
>       device_class_set_props(dc, smmuv3_properties);
> +    dc->hotpluggable = false;
> +    dc->bus_type = TYPE_PCIE_BUS;
Does this force legacy SMMUv3 to be tied to a PCIe bus now?
if so, will that break some existing legacy smmuv3 configs?, i.e., virtio-scsi attached to a legacy smmuv3.

>   }
>   
>   static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 177f3dd22c..3bae4e374f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -56,6 +56,7 @@
>   #include "qemu/cutils.h"
>   #include "qemu/error-report.h"
>   #include "qemu/module.h"
> +#include "hw/pci/pci_bus.h"
>   #include "hw/pci-host/gpex.h"
>   #include "hw/virtio/virtio-pci.h"
>   #include "hw/core/sysbus-fdt.h"
> @@ -1442,7 +1443,7 @@ static void create_smmu(const VirtMachineState *vms,
>       }
>       object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>                                &error_abort);
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>       for (i = 0; i < NUM_SMMU_IRQS; i++) {
>           sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Eric Auger]

Hi,
On 5/2/25 8:16 PM, Donald Dutile wrote:
>
>
> On 5/2/25 6:27 AM, Shameer Kolothum wrote:
>> Although this change does not affect functionality at present, it lays
>> the groundwork for enabling user-created SMMUv3 devices in
>> future patches
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>>   hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
>>   hw/arm/virt.c   |  3 ++-
>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index ab67972353..605de9b721 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -24,6 +24,7 @@
>>   #include "hw/qdev-properties.h"
>>   #include "hw/qdev-core.h"
>>   #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bridge.h"
>>   #include "cpu.h"
>>   #include "exec/target_page.h"
>>   #include "trace.h"
>> @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj,
>> ResetType type)
>>       smmuv3_init_regs(s);
>>   }
>>   +static int smmuv3_pcie_bus(Object *obj, void *opaque)
>> +{
>> +    DeviceState *d = opaque;
>> +    PCIBus *bus;
>> +
>> +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
>> +        return 0;
>> +    }
>> +
>> +    bus = PCI_HOST_BRIDGE(obj)->bus;
>> +    if (d->parent_bus && !strcmp(bus->qbus.name,
>> d->parent_bus->name)) {
>> +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
>> +                                 &error_abort);
>> +        /* Return non-zero as we got the bus and don't need further
>> iteration.*/
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +
>>   static void smmu_realize(DeviceState *d, Error **errp)
>>   {
>>       SMMUState *sys = ARM_SMMU(d);
>> @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d, Error
>> **errp)
>>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>>       Error *local_err = NULL;
>>   +    if (!object_property_get_link(OBJECT(d), "primary-bus",
>> &error_abort)) {
>> +        object_child_foreach_recursive(object_get_root(),
>> smmuv3_pcie_bus, d);
>> +    }
>> +
>>       c->parent_realize(d, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>> @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass
>> *klass, const void *data)
>>       device_class_set_parent_realize(dc, smmu_realize,
>>                                       &c->parent_realize);
>>       device_class_set_props(dc, smmuv3_properties);
>> +    dc->hotpluggable = false;
>> +    dc->bus_type = TYPE_PCIE_BUS;
> Does this force legacy SMMUv3 to be tied to a PCIe bus now?
> if so, will that break some existing legacy smmuv3 configs?, i.e.,
> virtio-scsi attached to a legacy smmuv3.

Previously the SMMU was already always attached to a PCI primary-bus
(vms->bus ie. pci0). virtio-scsi-pci is the device being protected. The
SMMU is not able to protect platforms devices atm.

My only concern is we are highjacking the "bus" prop to record the bus
hierarchy the SMMU is protecting. While the SMMU is a platform device
and does not inherit the PCI device base class its bus type becomes
"TYPE_PCIE_BUS". So in terms of qom hierachy is is seen as a PCI device
now? I don't know if it is a problem. An alternative could be to keep
the bus pointer and type as it was before and introduce a primary-bus
property. Adding Markus, Peter, Daniel and Alex in to.

At some point it was envisionned to support protected platform devices
(I think this was need for CCA). My fear is that if we turn the bus type
to PCIE it may be difficult to extend the support to non PCIe protected
devices. The SMMU shall remain a platform device being able to protect
either PCI devices and, in the future, platform devices.

Thanks

Eric
>
>>   }
>>     static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 177f3dd22c..3bae4e374f 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -56,6 +56,7 @@
>>   #include "qemu/cutils.h"
>>   #include "qemu/error-report.h"
>>   #include "qemu/module.h"
>> +#include "hw/pci/pci_bus.h"
>>   #include "hw/pci-host/gpex.h"
>>   #include "hw/virtio/virtio-pci.h"
>>   #include "hw/core/sysbus-fdt.h"
>> @@ -1442,7 +1443,7 @@ static void create_smmu(const VirtMachineState
>> *vms,
>>       }
>>       object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>>                                &error_abort);
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
>>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>>       for (i = 0; i < NUM_SMMU_IRQS; i++) {
>>           sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
>

RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Monday, May 5, 2025 9:19 AM
> To: Donald Dutile <ddutile@redhat.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org; Markus Armbruster <armbru@redhat.com>;
> Peter Maydell <peter.maydell@linaro.org>; Daniel P. Berrange
> <berrange@redhat.com>; Alex Bennée <alex.bennee@linaro.org>
> Cc: jgg@nvidia.com; nicolinc@nvidia.com; berrange@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> Hi,
> On 5/2/25 8:16 PM, Donald Dutile wrote:
> >
> >
> > On 5/2/25 6:27 AM, Shameer Kolothum wrote:
> >> Although this change does not affect functionality at present, it lays
> >> the groundwork for enabling user-created SMMUv3 devices in
> >> future patches
> >>
> >> Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> >> ---
> >>   hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
> >>   hw/arm/virt.c   |  3 ++-
> >>   2 files changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> >> index ab67972353..605de9b721 100644
> >> --- a/hw/arm/smmuv3.c
> >> +++ b/hw/arm/smmuv3.c
> >> @@ -24,6 +24,7 @@
> >>   #include "hw/qdev-properties.h"
> >>   #include "hw/qdev-core.h"
> >>   #include "hw/pci/pci.h"
> >> +#include "hw/pci/pci_bridge.h"
> >>   #include "cpu.h"
> >>   #include "exec/target_page.h"
> >>   #include "trace.h"
> >> @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj,
> >> ResetType type)
> >>       smmuv3_init_regs(s);
> >>   }
> >>   +static int smmuv3_pcie_bus(Object *obj, void *opaque)
> >> +{
> >> +    DeviceState *d = opaque;
> >> +    PCIBus *bus;
> >> +
> >> +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    bus = PCI_HOST_BRIDGE(obj)->bus;
> >> +    if (d->parent_bus && !strcmp(bus->qbus.name,
> >> d->parent_bus->name)) {
> >> +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
> >> +                                 &error_abort);
> >> +        /* Return non-zero as we got the bus and don't need further
> >> iteration.*/
> >> +        return 1;
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >>   static void smmu_realize(DeviceState *d, Error **errp)
> >>   {
> >>       SMMUState *sys = ARM_SMMU(d);
> >> @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d,
> Error
> >> **errp)
> >>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
> >>       Error *local_err = NULL;
> >>   +    if (!object_property_get_link(OBJECT(d), "primary-bus",
> >> &error_abort)) {
> >> +        object_child_foreach_recursive(object_get_root(),
> >> smmuv3_pcie_bus, d);
> >> +    }
> >> +
> >>       c->parent_realize(d, &local_err);
> >>       if (local_err) {
> >>           error_propagate(errp, local_err);
> >> @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass
> >> *klass, const void *data)
> >>       device_class_set_parent_realize(dc, smmu_realize,
> >>                                       &c->parent_realize);
> >>       device_class_set_props(dc, smmuv3_properties);
> >> +    dc->hotpluggable = false;
> >> +    dc->bus_type = TYPE_PCIE_BUS;
> > Does this force legacy SMMUv3 to be tied to a PCIe bus now?
> > if so, will that break some existing legacy smmuv3 configs?, i.e.,
> > virtio-scsi attached to a legacy smmuv3.
> 
> Previously the SMMU was already always attached to a PCI primary-bus
> (vms->bus ie. pci0). virtio-scsi-pci is the device being protected. The
> SMMU is not able to protect platforms devices atm.
> 
> My only concern is we are highjacking the "bus" prop to record the bus
> hierarchy the SMMU is protecting. While the SMMU is a platform device
> and does not inherit the PCI device base class its bus type becomes
> "TYPE_PCIE_BUS". So in terms of qom hierachy is is seen as a PCI device
> now? I don't know if it is a problem. An alternative could be to keep
> the bus pointer and type as it was before and introduce a primary-bus
> property. Adding Markus, Peter, Daniel and Alex in to.

Yes. The SMMUV3 is a platform device and we are making the bus type
here as PCIE which is a bit odd. As replied elsewhere in this thread,
in the initial RFC we had a specific "pci-bus" property associated with 
SMMUv3 device, 
Eg:
-device arm-smmuv3,pci-bus=pcie.0,...

But then there were feedback that, it is more intuitive and easy for libvirt
if we can just use the default "bus" property associated with a  Qemu device.
Hence the change.

> 
> At some point it was envisionned to support protected platform devices
> (I think this was need for CCA). My fear is that if we turn the bus type
> to PCIE it may be difficult to extend the support to non PCIe protected
> devices. The SMMU shall remain a platform device being able to protect
> either PCI devices and, in the future, platform devices.

Ok. So in the case of platform device how do we envisage to specify the "bus"?

-device arm-smmv3, primary-bus=sysbus

Or something like having a SMMUv3 dev without any bus and each platform
device has to specify the SMMUv3?
Eg:

-device arm-smmv3,id=smmuv3.3
-device xxxx,smmuv3= smmuv3.3

If later, I think we can stick to current one. 

Thanks,
Shameer

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Eric Auger]

Hi Shameer,

On 5/6/25 11:07 AM, Shameerali Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: Monday, May 5, 2025 9:19 AM
>> To: Donald Dutile <ddutile@redhat.com>; Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
>> qemu-devel@nongnu.org; Markus Armbruster <armbru@redhat.com>;
>> Peter Maydell <peter.maydell@linaro.org>; Daniel P. Berrange
>> <berrange@redhat.com>; Alex Bennée <alex.bennee@linaro.org>
>> Cc: jgg@nvidia.com; nicolinc@nvidia.com; berrange@redhat.com;
>> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com; Linuxarm
>> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
>> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
>> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
>> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
>> PCIe RC
>>
>> Hi,
>> On 5/2/25 8:16 PM, Donald Dutile wrote:
>>>
>>> On 5/2/25 6:27 AM, Shameer Kolothum wrote:
>>>> Although this change does not affect functionality at present, it lays
>>>> the groundwork for enabling user-created SMMUv3 devices in
>>>> future patches
>>>>
>>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.thodi@huawei.com>
>>>> ---
>>>>   hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
>>>>   hw/arm/virt.c   |  3 ++-
>>>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>>> index ab67972353..605de9b721 100644
>>>> --- a/hw/arm/smmuv3.c
>>>> +++ b/hw/arm/smmuv3.c
>>>> @@ -24,6 +24,7 @@
>>>>   #include "hw/qdev-properties.h"
>>>>   #include "hw/qdev-core.h"
>>>>   #include "hw/pci/pci.h"
>>>> +#include "hw/pci/pci_bridge.h"
>>>>   #include "cpu.h"
>>>>   #include "exec/target_page.h"
>>>>   #include "trace.h"
>>>> @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj,
>>>> ResetType type)
>>>>       smmuv3_init_regs(s);
>>>>   }
>>>>   +static int smmuv3_pcie_bus(Object *obj, void *opaque)
>>>> +{
>>>> +    DeviceState *d = opaque;
>>>> +    PCIBus *bus;
>>>> +
>>>> +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    bus = PCI_HOST_BRIDGE(obj)->bus;
>>>> +    if (d->parent_bus && !strcmp(bus->qbus.name,
>>>> d->parent_bus->name)) {
>>>> +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
>>>> +                                 &error_abort);
>>>> +        /* Return non-zero as we got the bus and don't need further
>>>> iteration.*/
>>>> +        return 1;
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static void smmu_realize(DeviceState *d, Error **errp)
>>>>   {
>>>>       SMMUState *sys = ARM_SMMU(d);
>>>> @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d,
>> Error
>>>> **errp)
>>>>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>>>>       Error *local_err = NULL;
>>>>   +    if (!object_property_get_link(OBJECT(d), "primary-bus",
>>>> &error_abort)) {
>>>> +        object_child_foreach_recursive(object_get_root(),
>>>> smmuv3_pcie_bus, d);
>>>> +    }
>>>> +
>>>>       c->parent_realize(d, &local_err);
>>>>       if (local_err) {
>>>>           error_propagate(errp, local_err);
>>>> @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass
>>>> *klass, const void *data)
>>>>       device_class_set_parent_realize(dc, smmu_realize,
>>>>                                       &c->parent_realize);
>>>>       device_class_set_props(dc, smmuv3_properties);
>>>> +    dc->hotpluggable = false;
>>>> +    dc->bus_type = TYPE_PCIE_BUS;
>>> Does this force legacy SMMUv3 to be tied to a PCIe bus now?
>>> if so, will that break some existing legacy smmuv3 configs?, i.e.,
>>> virtio-scsi attached to a legacy smmuv3.
>> Previously the SMMU was already always attached to a PCI primary-bus
>> (vms->bus ie. pci0). virtio-scsi-pci is the device being protected. The
>> SMMU is not able to protect platforms devices atm.
>>
>> My only concern is we are highjacking the "bus" prop to record the bus
>> hierarchy the SMMU is protecting. While the SMMU is a platform device
>> and does not inherit the PCI device base class its bus type becomes
>> "TYPE_PCIE_BUS". So in terms of qom hierachy is is seen as a PCI device
>> now? I don't know if it is a problem. An alternative could be to keep
>> the bus pointer and type as it was before and introduce a primary-bus
>> property. Adding Markus, Peter, Daniel and Alex in to.
> Yes. The SMMUV3 is a platform device and we are making the bus type
> here as PCIE which is a bit odd. As replied elsewhere in this thread,
> in the initial RFC we had a specific "pci-bus" property associated with 
> SMMUv3 device, 
> Eg:
> -device arm-smmuv3,pci-bus=pcie.0,...
>
> But then there were feedback that, it is more intuitive and easy for libvirt
> if we can just use the default "bus" property associated with a  Qemu device.
> Hence the change.
I also think that using the bus property was natural. It is only when
you look at the actual implementation and potential implications on
hierarchy introspections that it may be frown upon. That's why I call
for specialists feedback ;-)
>
>> At some point it was envisionned to support protected platform devices
>> (I think this was need for CCA). My fear is that if we turn the bus type
>> to PCIE it may be difficult to extend the support to non PCIe protected
>> devices. The SMMU shall remain a platform device being able to protect
>> either PCI devices and, in the future, platform devices.
> Ok. So in the case of platform device how do we envisage to specify the "bus"?
>
> -device arm-smmv3, primary-bus=sysbus
The problem is on a SOC not all platform devices are protected by an
SMMU. One needs to be more precise I think

>
> Or something like having a SMMUv3 dev without any bus and each platform
> device has to specify the SMMUv3?
> Eg:
>
> -device arm-smmv3,id=smmuv3.3
> -device xxxx,smmuv3= smmuv3.3
Yeah I think this would look that way instead.

The last attempt was
https://lore.kernel.org/all/20210902081429.140293-1-chunming_li1234@163.com/
if I remember correctly but it did not go further. It defined a
peri-sid-map[%d] prop array to define SIDs but I don't really get how it
defines which device is protected.



> If later, I think we can stick to current one. 
That's also my pov.

Cheers

Eric

>
> Thanks,
> Shameer

RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Donald Dutile <ddutile@redhat.com>
> Sent: Friday, May 2, 2025 7:17 PM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Cc: eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> nicolinc@nvidia.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> 
> 
> On 5/2/25 6:27 AM, Shameer Kolothum wrote:
> > Although this change does not affect functionality at present, it lays
> > the groundwork for enabling user-created SMMUv3 devices in
> > future patches
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
> >   hw/arm/virt.c   |  3 ++-
> >   2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index ab67972353..605de9b721 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -24,6 +24,7 @@
> >   #include "hw/qdev-properties.h"
> >   #include "hw/qdev-core.h"
> >   #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bridge.h"
> >   #include "cpu.h"
> >   #include "exec/target_page.h"
> >   #include "trace.h"
> > @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj,
> ResetType type)
> >       smmuv3_init_regs(s);
> >   }
> >
> > +static int smmuv3_pcie_bus(Object *obj, void *opaque)
> > +{
> > +    DeviceState *d = opaque;
> > +    PCIBus *bus;
> > +
> > +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> > +        return 0;
> > +    }
> > +
> > +    bus = PCI_HOST_BRIDGE(obj)->bus;
> > +    if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus-
> >name)) {
> > +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
> > +                                 &error_abort);
> > +        /* Return non-zero as we got the bus and don't need further
> iteration.*/
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > +
> >   static void smmu_realize(DeviceState *d, Error **errp)
> >   {
> >       SMMUState *sys = ARM_SMMU(d);
> > @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d, Error
> **errp)
> >       SysBusDevice *dev = SYS_BUS_DEVICE(d);
> >       Error *local_err = NULL;
> >
> > +    if (!object_property_get_link(OBJECT(d), "primary-bus", &error_abort))
> {
> > +        object_child_foreach_recursive(object_get_root(),
> smmuv3_pcie_bus, d);
> > +    }
> > +
> >       c->parent_realize(d, &local_err);
> >       if (local_err) {
> >           error_propagate(errp, local_err);
> > @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass
> *klass, const void *data)
> >       device_class_set_parent_realize(dc, smmu_realize,
> >                                       &c->parent_realize);
> >       device_class_set_props(dc, smmuv3_properties);
> > +    dc->hotpluggable = false;
> > +    dc->bus_type = TYPE_PCIE_BUS;
> Does this force legacy SMMUv3 to be tied to a PCIe bus now?
> if so, will that break some existing legacy smmuv3 configs?, i.e., virtio-scsi
> attached to a legacy smmuv3.

No. It won't break the existing config as Eric has already replied. The only difference is
as Eric rightly pointed out, SMMUv3 is a platform device and we are specifying
the device bus type as PCIE, which is a bit odd. The initial RFC was with a specific
"pci-bus" as command line option , ie,

-device arm-smmuv3, pci-bus=pcie0,...

and then there were feedbacks that it is more intuitive and makes life easy with 
libvirt if we can use the Qemu device "bus" option and hence the change.

Thanks,
Shameer

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Markus Armbruster]

Shameer Kolothum via <qemu-devel@nongnu.org> writes:

> Although this change does not affect functionality at present, it lays
> the groundwork for enabling user-created SMMUv3 devices in
> future patches
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
>  hw/arm/virt.c   |  3 ++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index ab67972353..605de9b721 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -24,6 +24,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/qdev-core.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bridge.h"
>  #include "cpu.h"
>  #include "exec/target_page.h"
>  #include "trace.h"
> @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj, ResetType type)
>      smmuv3_init_regs(s);
>  }
>  
> +static int smmuv3_pcie_bus(Object *obj, void *opaque)
> +{
> +    DeviceState *d = opaque;
> +    PCIBus *bus;
> +
> +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> +        return 0;
> +    }
> +
> +    bus = PCI_HOST_BRIDGE(obj)->bus;
> +    if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus->name)) {
> +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
> +                                 &error_abort);
> +        /* Return non-zero as we got the bus and don't need further iteration.*/
> +        return 1;
> +    }
> +    return 0;
> +}
> +
>  static void smmu_realize(DeviceState *d, Error **errp)
>  {
>      SMMUState *sys = ARM_SMMU(d);
> @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d, Error **errp)
>      SysBusDevice *dev = SYS_BUS_DEVICE(d);
>      Error *local_err = NULL;
>  
> +    if (!object_property_get_link(OBJECT(d), "primary-bus", &error_abort)) {
> +        object_child_foreach_recursive(object_get_root(), smmuv3_pcie_bus, d);
> +    }
> +
>      c->parent_realize(d, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass *klass, const void *data)
>      device_class_set_parent_realize(dc, smmu_realize,
>                                      &c->parent_realize);
>      device_class_set_props(dc, smmuv3_properties);
> +    dc->hotpluggable = false;
> +    dc->bus_type = TYPE_PCIE_BUS;

This is very, very wrong.

The function serves as .class_init() for QOM type "arm-smmuv3", defined
as:

   static const TypeInfo smmuv3_type_info = {
       .name          = TYPE_ARM_SMMUV3,
       .parent        = TYPE_ARM_SMMU,

Subtype of "arm-smmuv3".

       .instance_size = sizeof(SMMUv3State),
       .instance_init = smmuv3_instance_init,
       .class_size    = sizeof(SMMUv3Class),
       .class_init    = smmuv3_class_init,
   };


    static const TypeInfo smmu_base_info = {
        .name          = TYPE_ARM_SMMU,
        .parent        = TYPE_SYS_BUS_DEVICE,

Subtype of "sys-bus-device".

        .instance_size = sizeof(SMMUState),
        .class_data    = NULL,
        .class_size    = sizeof(SMMUBaseClass),
        .class_init    = smmu_base_class_init,
        .abstract      = true,
    };

Have a look at the instance struct:

   struct SMMUv3State {
       SMMUState     smmu_state;

Starts with the supertype's instance struct, as is proper.

       uint32_t features;
       [more ...]
   };

Here's the supertype's instance struct:

   struct SMMUState {
       /* <private> */
       SysBusDevice  dev;

Again, starts with the supertype's instance struct.

       const char *mrtypename;
       [more...]
   };

This is a sysbus device, not a PCI device.  Monkey-patching dc->bus_type
from TYPE_SYSTEM_BUS to TYPE_PCIE_BUS won't change that.  All it
accomplishes is making the qdev core believe it plugs into a PCIE bus.
This can only end in tears.

In fact, when I build with the entire series applied (so the device can
actually be used with -device), the result dies within seconds of my ad
hoc testing:

    $ qemu-system-aarch64 -nodefaults -S -display none -monitor stdio -M virt -device pxb-pcie,id=pcie.1,bus_nr=2 -device arm-smmuv3,bus=pcie.1
    QEMU 10.0.50 monitor - type 'help' for more information
    qemu-system-aarch64: -device arm-smmuv3,bus=pcie.1: warning: SMMUv3 device only supported with pcie.0 for DT
    (qemu) info qtree
    bus: main-system-bus
      type System
      dev: pxb-host, id ""
        x-config-reg-migration-enabled = true
        bypass-iommu = false
        bus: pcie.1
          type pxb-pcie-bus
          dev: arm-smmuv3, id ""
            gpio-out "sysbus-irq" 4
            stage = "nested"
            bus_num = 0 (0x0)
    Segmentation fault (core dumped)

Backtrace:

    #0  0x00005555557d8521 in pcibus_dev_print
        (mon=0x55555826d0e0, dev=0x5555590ad360, indent=8)
        at ../hw/pci/pci-hmp-cmds.c:140
    #1  0x0000555555eac0a0 in bus_print_dev
        (bus=<optimized out>, mon=<optimized out>, dev=0x5555590ad360, indent=8)
        at ../system/qdev-monitor.c:773
    #2  qdev_print (mon=<optimized out>, dev=<optimized out>, indent=8)
        at ../system/qdev-monitor.c:805
    #3  qbus_print
        (mon=mon@entry=0x55555826d0e0, bus=bus@entry=0x5555590ac4a0, indent=6, 
        indent@entry=4, details=details@entry=true) at ../system/qdev-monitor.c:821
    #4  0x0000555555eabd92 in qbus_print
        (mon=0x55555826d0e0, bus=<optimized out>, indent=2, details=true)
        at ../system/qdev-monitor.c:824
    #5  0x0000555555979789 in handle_hmp_command_exec
        (cmd=<optimized out>, mon=0x55555826d0e0, qdict=0x55555907d8e0)
        at ../monitor/hmp.c:1106
    #6  handle_hmp_command_exec
        (mon=0x55555826d0e0, cmd=0x55555769d220 <hmp_info_cmds+2560>, qdict=0x55555907d8e0) at ../monitor/hmp.c:1098
    #7  handle_hmp_command (mon=mon@entry=0x55555826d0e0, cmdline=<optimized out>, 
        cmdline@entry=0x555558657490 "info qtree") at ../monitor/hmp.c:1158
    #8  0x000055555597983c in monitor_command_cb

Crash line is

    int class = pci_get_word(d->config + PCI_CLASS_DEVICE);

Debugger shows that d->config is invalid.  This is hardly surprising!

The qdev core is trying to print information on the "arm_smmuv3" device
here.  It's working with a DeviceState.  Your monkey-patching convinced
it it's a PCIDevice, so it duly calls pcibus_dev_print() to print PCI
device information.  pcibus_dev_print() casts the DeviceState * to
PCIDevice *.  This assumes the device's instance actually starts with
PCIDevice.  It actually starts with SysBusDevice.

Unsurprisingly, and "make check" also fails:

    >>> MESON_TEST_ITERATION=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 RUST_BACKTRACE=1 QTEST_QEMU_BINARY=./qemu-system-aarch64 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/work/armbru/qemu/tests/dbus-vmstate-daemon.sh ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 PYTHON=/work/armbru/qemu/bld-arm/pyvenv/bin/python3 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MALLOC_PERTURB_=22 /work/armbru/qemu/bld-arm/tests/qtest/test-hmp --tap -k
    ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
    stderr:
    qemu-system-aarch64: ../hw/core/qdev.c:113: qdev_set_parent_bus: Assertion `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)' failed.
    Broken pipe
    ../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)

and

    >>> MESON_TEST_ITERATION=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 RUST_BACKTRACE=1 QTEST_QEMU_BINARY=./qemu-system-aarch64 MALLOC_PERTURB_=86 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/work/armbru/qemu/tests/dbus-vmstate-daemon.sh ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 PYTHON=/work/armbru/qemu/bld-arm/pyvenv/bin/python3 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 /work/armbru/qemu/bld-arm/tests/qtest/qom-test --tap -k
    ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
    stderr:
    qemu-system-aarch64: ../hw/core/qdev.c:113: qdev_set_parent_bus: Assertion `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)' failed.
    Broken pipe
    ../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)

Please make sure "make check" passes before posting patches for review.
If you need help getting there, post them marked RFC and with the bad
test results right in the cover letter.

>  }
>  
>  static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 177f3dd22c..3bae4e374f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -56,6 +56,7 @@
>  #include "qemu/cutils.h"
>  #include "qemu/error-report.h"
>  #include "qemu/module.h"
> +#include "hw/pci/pci_bus.h"
>  #include "hw/pci-host/gpex.h"
>  #include "hw/virtio/virtio-pci.h"
>  #include "hw/core/sysbus-fdt.h"
> @@ -1442,7 +1443,7 @@ static void create_smmu(const VirtMachineState *vms,
>      }
>      object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>                               &error_abort);
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>      for (i = 0; i < NUM_SMMU_IRQS; i++) {
>          sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,

What are you trying to accomplish?

I *guess* you're trying to change the "arm-smmuv3" device to be a PCI
device.  Correct?

The only way to do that is making it a subtype of PCIDevice, i.e. change
the parent chain from

   arm-smmuv3 -> arm-smmu -> sys-bus-device -> device -> object

to something like

   arm-smmuv3 ->    ...   -> pci-device -> device -> object

Note you cannot have different subtypes of the same supertype (say
"arm-smmu") plug into different buses.  If you need a common device core
to plug into different buses, things get more complicated.  Here's how
the "serial-FOO" devices do it:

* "serial-mm", a subtype of "sys-bus-device", thus plugs into system bus
* "serial-isa", a subtype of "isa-device", thus plugs into ISA bus
* "serial-pci", a subtype of "pci-device", thus plugs into PCI bus

They all *contain* the core "serial" device, which is a subtype of
"device", and thus does not plug into any bus.  "Contain" is "has a",
not "subtype of".

[...]

RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

Hi Markus,

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Tuesday, May 6, 2025 12:47 PM
> To: Shameer Kolothum via <qemu-devel@nongnu.org>
> Cc: qemu-arm@nongnu.org; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; eric.auger@redhat.com;
> peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> Shameer Kolothum via <qemu-devel@nongnu.org> writes:
> 
> > Although this change does not affect functionality at present, it lays
> > the groundwork for enabling user-created SMMUv3 devices in
> > future patches
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
> >  hw/arm/virt.c   |  3 ++-
> >  2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index ab67972353..605de9b721 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -24,6 +24,7 @@
> >  #include "hw/qdev-properties.h"
> >  #include "hw/qdev-core.h"
> >  #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bridge.h"
> >  #include "cpu.h"
> >  #include "exec/target_page.h"
> >  #include "trace.h"
> > @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj,
> ResetType type)
> >      smmuv3_init_regs(s);
> >  }
> >
> > +static int smmuv3_pcie_bus(Object *obj, void *opaque)
> > +{
> > +    DeviceState *d = opaque;
> > +    PCIBus *bus;
> > +
> > +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> > +        return 0;
> > +    }
> > +
> > +    bus = PCI_HOST_BRIDGE(obj)->bus;
> > +    if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus-
> >name)) {
> > +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
> > +                                 &error_abort);
> > +        /* Return non-zero as we got the bus and don't need further
> iteration.*/
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > +
> >  static void smmu_realize(DeviceState *d, Error **errp)
> >  {
> >      SMMUState *sys = ARM_SMMU(d);
> > @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d, Error
> **errp)
> >      SysBusDevice *dev = SYS_BUS_DEVICE(d);
> >      Error *local_err = NULL;
> >
> > +    if (!object_property_get_link(OBJECT(d), "primary-bus", &error_abort))
> {
> > +        object_child_foreach_recursive(object_get_root(),
> smmuv3_pcie_bus, d);
> > +    }
> > +
> >      c->parent_realize(d, &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> > @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass
> *klass, const void *data)
> >      device_class_set_parent_realize(dc, smmu_realize,
> >                                      &c->parent_realize);
> >      device_class_set_props(dc, smmuv3_properties);
> > +    dc->hotpluggable = false;
> > +    dc->bus_type = TYPE_PCIE_BUS;
> 
> This is very, very wrong.
> 
> The function serves as .class_init() for QOM type "arm-smmuv3", defined
> as:
> 
>    static const TypeInfo smmuv3_type_info = {
>        .name          = TYPE_ARM_SMMUV3,
>        .parent        = TYPE_ARM_SMMU,
> 
> Subtype of "arm-smmuv3".
> 
>        .instance_size = sizeof(SMMUv3State),
>        .instance_init = smmuv3_instance_init,
>        .class_size    = sizeof(SMMUv3Class),
>        .class_init    = smmuv3_class_init,
>    };
> 
> 
>     static const TypeInfo smmu_base_info = {
>         .name          = TYPE_ARM_SMMU,
>         .parent        = TYPE_SYS_BUS_DEVICE,
> 
> Subtype of "sys-bus-device".
> 
>         .instance_size = sizeof(SMMUState),
>         .class_data    = NULL,
>         .class_size    = sizeof(SMMUBaseClass),
>         .class_init    = smmu_base_class_init,
>         .abstract      = true,
>     };
> 
> Have a look at the instance struct:
> 
>    struct SMMUv3State {
>        SMMUState     smmu_state;
> 
> Starts with the supertype's instance struct, as is proper.
> 
>        uint32_t features;
>        [more ...]
>    };
> 
> Here's the supertype's instance struct:
> 
>    struct SMMUState {
>        /* <private> */
>        SysBusDevice  dev;
> 
> Again, starts with the supertype's instance struct.
> 
>        const char *mrtypename;
>        [more...]
>    };
> 
> This is a sysbus device, not a PCI device.  Monkey-patching dc->bus_type
> from TYPE_SYSTEM_BUS to TYPE_PCIE_BUS won't change that.  All it
> accomplishes is making the qdev core believe it plugs into a PCIE bus.
> This can only end in tears.
> 
> In fact, when I build with the entire series applied (so the device can
> actually be used with -device), the result dies within seconds of my ad
> hoc testing:
> 
>     $ qemu-system-aarch64 -nodefaults -S -display none -monitor stdio -M
> virt -device pxb-pcie,id=pcie.1,bus_nr=2 -device arm-smmuv3,bus=pcie.1
>     QEMU 10.0.50 monitor - type 'help' for more information
>     qemu-system-aarch64: -device arm-smmuv3,bus=pcie.1: warning:
> SMMUv3 device only supported with pcie.0 for DT
>     (qemu) info qtree
>     bus: main-system-bus
>       type System
>       dev: pxb-host, id ""
>         x-config-reg-migration-enabled = true
>         bypass-iommu = false
>         bus: pcie.1
>           type pxb-pcie-bus
>           dev: arm-smmuv3, id ""
>             gpio-out "sysbus-irq" 4
>             stage = "nested"
>             bus_num = 0 (0x0)
>     Segmentation fault (core dumped)

Ah..My bad. I didn't test this. Thanks for taking time to test this.

> 
> Backtrace:
> 
>     #0  0x00005555557d8521 in pcibus_dev_print
>         (mon=0x55555826d0e0, dev=0x5555590ad360, indent=8)
>         at ../hw/pci/pci-hmp-cmds.c:140
>     #1  0x0000555555eac0a0 in bus_print_dev
>         (bus=<optimized out>, mon=<optimized out>, dev=0x5555590ad360,
> indent=8)
>         at ../system/qdev-monitor.c:773
>     #2  qdev_print (mon=<optimized out>, dev=<optimized out>, indent=8)
>         at ../system/qdev-monitor.c:805
>     #3  qbus_print
>         (mon=mon@entry=0x55555826d0e0,
> bus=bus@entry=0x5555590ac4a0, indent=6,
>         indent@entry=4, details=details@entry=true) at ../system/qdev-
> monitor.c:821
>     #4  0x0000555555eabd92 in qbus_print
>         (mon=0x55555826d0e0, bus=<optimized out>, indent=2, details=true)
>         at ../system/qdev-monitor.c:824
>     #5  0x0000555555979789 in handle_hmp_command_exec
>         (cmd=<optimized out>, mon=0x55555826d0e0, qdict=0x55555907d8e0)
>         at ../monitor/hmp.c:1106
>     #6  handle_hmp_command_exec
>         (mon=0x55555826d0e0, cmd=0x55555769d220
> <hmp_info_cmds+2560>, qdict=0x55555907d8e0) at ../monitor/hmp.c:1098
>     #7  handle_hmp_command (mon=mon@entry=0x55555826d0e0,
> cmdline=<optimized out>,
>         cmdline@entry=0x555558657490 "info qtree") at
> ../monitor/hmp.c:1158
>     #8  0x000055555597983c in monitor_command_cb
> 
> Crash line is
> 
>     int class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> 
> Debugger shows that d->config is invalid.  This is hardly surprising!
> 
> The qdev core is trying to print information on the "arm_smmuv3" device
> here.  It's working with a DeviceState.  Your monkey-patching convinced
> it it's a PCIDevice, so it duly calls pcibus_dev_print() to print PCI
> device information.  pcibus_dev_print() casts the DeviceState * to
> PCIDevice *.  This assumes the device's instance actually starts with
> PCIDevice.  It actually starts with SysBusDevice.
> 
> Unsurprisingly, and "make check" also fails:
> 
>     >>> MESON_TEST_ITERATION=1
> MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print
> _stacktrace=1 RUST_BACKTRACE=1 QTEST_QEMU_BINARY=./qemu-system-
> aarch64 QTEST_QEMU_IMG=./qemu-img
> G_TEST_DBUS_DAEMON=/work/armbru/qemu/tests/dbus-vmstate-
> daemon.sh
> ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1
> PYTHON=/work/armbru/qemu/bld-arm/pyvenv/bin/python3
> UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:prin
> t_stacktrace=1 MALLOC_PERTURB_=22 /work/armbru/qemu/bld-
> arm/tests/qtest/test-hmp --tap -k
> 
> ―――――――――――――――――――――――――――――――――――
> ―― ✀
> ―――――――――――――――――――――――――――――――――――
> ――
>     stderr:
>     qemu-system-aarch64: ../hw/core/qdev.c:113: qdev_set_parent_bus:
> Assertion `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc-
> >bus_type)' failed.
>     Broken pipe
>     ../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from
> signal 6 (Aborted) (core dumped)
> 
> and
> 
>     >>> MESON_TEST_ITERATION=1
> MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print
> _stacktrace=1 RUST_BACKTRACE=1 QTEST_QEMU_BINARY=./qemu-system-
> aarch64 MALLOC_PERTURB_=86 QTEST_QEMU_IMG=./qemu-img
> G_TEST_DBUS_DAEMON=/work/armbru/qemu/tests/dbus-vmstate-
> daemon.sh
> ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1
> PYTHON=/work/armbru/qemu/bld-arm/pyvenv/bin/python3
> UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:prin
> t_stacktrace=1 /work/armbru/qemu/bld-arm/tests/qtest/qom-test --tap -k
> 
> ―――――――――――――――――――――――――――――――――――
> ―― ✀
> ―――――――――――――――――――――――――――――――――――
> ――
>     stderr:
>     qemu-system-aarch64: ../hw/core/qdev.c:113: qdev_set_parent_bus:
> Assertion `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc-
> >bus_type)' failed.
>     Broken pipe
>     ../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from
> signal 6 (Aborted) (core dumped)
> 
> Please make sure "make check" passes before posting patches for review.
> If you need help getting there, post them marked RFC and with the bad
> test results right in the cover letter.

Sure. Will run "make check" in future.

> >  }
> >
> >  static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 177f3dd22c..3bae4e374f 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -56,6 +56,7 @@
> >  #include "qemu/cutils.h"
> >  #include "qemu/error-report.h"
> >  #include "qemu/module.h"
> > +#include "hw/pci/pci_bus.h"
> >  #include "hw/pci-host/gpex.h"
> >  #include "hw/virtio/virtio-pci.h"
> >  #include "hw/core/sysbus-fdt.h"
> > @@ -1442,7 +1443,7 @@ static void create_smmu(const
> VirtMachineState *vms,
> >      }
> >      object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
> >                               &error_abort);
> > -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > +    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
> >      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> >      for (i = 0; i < NUM_SMMU_IRQS; i++) {
> >          sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
> 
> What are you trying to accomplish?
> 
> I *guess* you're trying to change the "arm-smmuv3" device to be a PCI
> device.  Correct?

Not really. SMMUv3 is a platform device and it is normally associated with a PCIe RC.
Currently Qemu has a machine wide SMMUv3(-machine virt, iommu=smmuv3) and 
is associated by default with the Pcie.0 bus. This series as explained in cover letter adds
support to user-creatable SMMUv3 devices and requires a way to associate a PCIe RC.

This was initially done in the RFC as below,

-device arm-smmuv3,pci-bus=pcie.0

But there were feedback received that it is more intuitive and easy for libvirt to use
the Qemu device default "bus" property to achieve this,

-device arm-smmuv3,bus=pcie.0

And hence the change to,

  dc->bus_type = TYPE_PCIE_BUS;

Please see the discussion on this we had in this thread,
https://lore.kernel.org/qemu-devel/f1b91034-be2a-493e-9229-c164e0895825@redhat.com/

So from your reply, it is obvious that we can't use the default "bus" as used  this patch and
probably the only way is to use  a different property name like,

-device arm-smmuv3,primary-bus=pcie.0

Please let me know if there is another way to achieve the same with default "bus".

Thanks again for taking the time to run the above tests and the explanation.

Thanks,
Shameer


> The only way to do that is making it a subtype of PCIDevice, i.e. change
> the parent chain from
> 
>    arm-smmuv3 -> arm-smmu -> sys-bus-device -> device -> object
> 
> to something like
> 
>    arm-smmuv3 ->    ...   -> pci-device -> device -> object
> 
> Note you cannot have different subtypes of the same supertype (say
> "arm-smmu") plug into different buses.  If you need a common device core
> to plug into different buses, things get more complicated.  Here's how
> the "serial-FOO" devices do it:
> 
> * "serial-mm", a subtype of "sys-bus-device", thus plugs into system bus
> * "serial-isa", a subtype of "isa-device", thus plugs into ISA bus
> * "serial-pci", a subtype of "pci-device", thus plugs into PCI bus
> 
> They all *contain* the core "serial" device, which is a subtype of
> "device", and thus does not plug into any bus.  "Contain" is "has a",
> not "subtype of".
> 
> [...]
> 

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Donald Dutile]

On 5/6/25 7:47 AM, Markus Armbruster wrote:
> Shameer Kolothum via <qemu-devel@nongnu.org> writes:
> 
>> Although this change does not affect functionality at present, it lays
>> the groundwork for enabling user-created SMMUv3 devices in
>> future patches
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>>   hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
>>   hw/arm/virt.c   |  3 ++-
>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index ab67972353..605de9b721 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -24,6 +24,7 @@
>>   #include "hw/qdev-properties.h"
>>   #include "hw/qdev-core.h"
>>   #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bridge.h"
>>   #include "cpu.h"
>>   #include "exec/target_page.h"
>>   #include "trace.h"
>> @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj, ResetType type)
>>       smmuv3_init_regs(s);
>>   }
>>   
>> +static int smmuv3_pcie_bus(Object *obj, void *opaque)
>> +{
>> +    DeviceState *d = opaque;
>> +    PCIBus *bus;
>> +
>> +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
>> +        return 0;
>> +    }
>> +
>> +    bus = PCI_HOST_BRIDGE(obj)->bus;
>> +    if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus->name)) {
>> +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
>> +                                 &error_abort);
>> +        /* Return non-zero as we got the bus and don't need further iteration.*/
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +
>>   static void smmu_realize(DeviceState *d, Error **errp)
>>   {
>>       SMMUState *sys = ARM_SMMU(d);
>> @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d, Error **errp)
>>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>>       Error *local_err = NULL;
>>   
>> +    if (!object_property_get_link(OBJECT(d), "primary-bus", &error_abort)) {
>> +        object_child_foreach_recursive(object_get_root(), smmuv3_pcie_bus, d);
>> +    }
>> +
>>       c->parent_realize(d, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>> @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass *klass, const void *data)
>>       device_class_set_parent_realize(dc, smmu_realize,
>>                                       &c->parent_realize);
>>       device_class_set_props(dc, smmuv3_properties);
>> +    dc->hotpluggable = false;
>> +    dc->bus_type = TYPE_PCIE_BUS;
> 
> This is very, very wrong.
> 
> The function serves as .class_init() for QOM type "arm-smmuv3", defined
> as:
> 
>     static const TypeInfo smmuv3_type_info = {
>         .name          = TYPE_ARM_SMMUV3,
>         .parent        = TYPE_ARM_SMMU,
> 
> Subtype of "arm-smmuv3".
> 
>         .instance_size = sizeof(SMMUv3State),
>         .instance_init = smmuv3_instance_init,
>         .class_size    = sizeof(SMMUv3Class),
>         .class_init    = smmuv3_class_init,
>     };
> 
> 
>      static const TypeInfo smmu_base_info = {
>          .name          = TYPE_ARM_SMMU,
>          .parent        = TYPE_SYS_BUS_DEVICE,
> 
> Subtype of "sys-bus-device".
> 
>          .instance_size = sizeof(SMMUState),
>          .class_data    = NULL,
>          .class_size    = sizeof(SMMUBaseClass),
>          .class_init    = smmu_base_class_init,
>          .abstract      = true,
>      };
> 
> Have a look at the instance struct:
> 
>     struct SMMUv3State {
>         SMMUState     smmu_state;
> 
> Starts with the supertype's instance struct, as is proper.
> 
>         uint32_t features;
>         [more ...]
>     };
> 
> Here's the supertype's instance struct:
> 
>     struct SMMUState {
>         /* <private> */
>         SysBusDevice  dev;
> 
> Again, starts with the supertype's instance struct.
> 
>         const char *mrtypename;
>         [more...]
>     };
> 
> This is a sysbus device, not a PCI device.  Monkey-patching dc->bus_type
> from TYPE_SYSTEM_BUS to TYPE_PCIE_BUS won't change that.  All it
> accomplishes is making the qdev core believe it plugs into a PCIE bus.
> This can only end in tears.
> 
> In fact, when I build with the entire series applied (so the device can
> actually be used with -device), the result dies within seconds of my ad
> hoc testing:
> 
>      $ qemu-system-aarch64 -nodefaults -S -display none -monitor stdio -M virt -device pxb-pcie,id=pcie.1,bus_nr=2 -device arm-smmuv3,bus=pcie.1
>      QEMU 10.0.50 monitor - type 'help' for more information
>      qemu-system-aarch64: -device arm-smmuv3,bus=pcie.1: warning: SMMUv3 device only supported with pcie.0 for DT
>      (qemu) info qtree
>      bus: main-system-bus
>        type System
>        dev: pxb-host, id ""
>          x-config-reg-migration-enabled = true
>          bypass-iommu = false
>          bus: pcie.1
>            type pxb-pcie-bus
>            dev: arm-smmuv3, id ""
>              gpio-out "sysbus-irq" 4
>              stage = "nested"
>              bus_num = 0 (0x0)
>      Segmentation fault (core dumped)
> 
> Backtrace:
> 
>      #0  0x00005555557d8521 in pcibus_dev_print
>          (mon=0x55555826d0e0, dev=0x5555590ad360, indent=8)
>          at ../hw/pci/pci-hmp-cmds.c:140
>      #1  0x0000555555eac0a0 in bus_print_dev
>          (bus=<optimized out>, mon=<optimized out>, dev=0x5555590ad360, indent=8)
>          at ../system/qdev-monitor.c:773
>      #2  qdev_print (mon=<optimized out>, dev=<optimized out>, indent=8)
>          at ../system/qdev-monitor.c:805
>      #3  qbus_print
>          (mon=mon@entry=0x55555826d0e0, bus=bus@entry=0x5555590ac4a0, indent=6,
>          indent@entry=4, details=details@entry=true) at ../system/qdev-monitor.c:821
>      #4  0x0000555555eabd92 in qbus_print
>          (mon=0x55555826d0e0, bus=<optimized out>, indent=2, details=true)
>          at ../system/qdev-monitor.c:824
>      #5  0x0000555555979789 in handle_hmp_command_exec
>          (cmd=<optimized out>, mon=0x55555826d0e0, qdict=0x55555907d8e0)
>          at ../monitor/hmp.c:1106
>      #6  handle_hmp_command_exec
>          (mon=0x55555826d0e0, cmd=0x55555769d220 <hmp_info_cmds+2560>, qdict=0x55555907d8e0) at ../monitor/hmp.c:1098
>      #7  handle_hmp_command (mon=mon@entry=0x55555826d0e0, cmdline=<optimized out>,
>          cmdline@entry=0x555558657490 "info qtree") at ../monitor/hmp.c:1158
>      #8  0x000055555597983c in monitor_command_cb
> 
> Crash line is
> 
>      int class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> 
> Debugger shows that d->config is invalid.  This is hardly surprising!
> 
> The qdev core is trying to print information on the "arm_smmuv3" device
> here.  It's working with a DeviceState.  Your monkey-patching convinced
> it it's a PCIDevice, so it duly calls pcibus_dev_print() to print PCI
> device information.  pcibus_dev_print() casts the DeviceState * to
> PCIDevice *.  This assumes the device's instance actually starts with
> PCIDevice.  It actually starts with SysBusDevice.
> 
> Unsurprisingly, and "make check" also fails:
> 
>      >>> MESON_TEST_ITERATION=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 RUST_BACKTRACE=1 QTEST_QEMU_BINARY=./qemu-system-aarch64 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/work/armbru/qemu/tests/dbus-vmstate-daemon.sh ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 PYTHON=/work/armbru/qemu/bld-arm/pyvenv/bin/python3 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MALLOC_PERTURB_=22 /work/armbru/qemu/bld-arm/tests/qtest/test-hmp --tap -k
>      ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>      stderr:
>      qemu-system-aarch64: ../hw/core/qdev.c:113: qdev_set_parent_bus: Assertion `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)' failed.
>      Broken pipe
>      ../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
> 
> and
> 
>      >>> MESON_TEST_ITERATION=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 RUST_BACKTRACE=1 QTEST_QEMU_BINARY=./qemu-system-aarch64 MALLOC_PERTURB_=86 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/work/armbru/qemu/tests/dbus-vmstate-daemon.sh ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 PYTHON=/work/armbru/qemu/bld-arm/pyvenv/bin/python3 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 /work/armbru/qemu/bld-arm/tests/qtest/qom-test --tap -k
>      ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>      stderr:
>      qemu-system-aarch64: ../hw/core/qdev.c:113: qdev_set_parent_bus: Assertion `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)' failed.
>      Broken pipe
>      ../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
> 
> Please make sure "make check" passes before posting patches for review.
> If you need help getting there, post them marked RFC and with the bad
> test results right in the cover letter.
> 
>>   }
>>   
>>   static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 177f3dd22c..3bae4e374f 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -56,6 +56,7 @@
>>   #include "qemu/cutils.h"
>>   #include "qemu/error-report.h"
>>   #include "qemu/module.h"
>> +#include "hw/pci/pci_bus.h"
>>   #include "hw/pci-host/gpex.h"
>>   #include "hw/virtio/virtio-pci.h"
>>   #include "hw/core/sysbus-fdt.h"
>> @@ -1442,7 +1443,7 @@ static void create_smmu(const VirtMachineState *vms,
>>       }
>>       object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>>                                &error_abort);
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
>>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>>       for (i = 0; i < NUM_SMMU_IRQS; i++) {
>>           sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
> 
> What are you trying to accomplish?
> 
> I *guess* you're trying to change the "arm-smmuv3" device to be a PCI
> device.  Correct?
> 
> The only way to do that is making it a subtype of PCIDevice, i.e. change
> the parent chain from
> 
>     arm-smmuv3 -> arm-smmu -> sys-bus-device -> device -> object
> 
> to something like
> 
>     arm-smmuv3 ->    ...   -> pci-device -> device -> object
> 
> Note you cannot have different subtypes of the same supertype (say
> "arm-smmu") plug into different buses.  If you need a common device core
> to plug into different buses, things get more complicated.  Here's how
> the "serial-FOO" devices do it:
> 
> * "serial-mm", a subtype of "sys-bus-device", thus plugs into system bus
> * "serial-isa", a subtype of "isa-device", thus plugs into ISA bus
> * "serial-pci", a subtype of "pci-device", thus plugs into PCI bus
> 
> They all *contain* the core "serial" device, which is a subtype of
> "device", and thus does not plug into any bus.  "Contain" is "has a",
> not "subtype of".
> 
> [...]
> 
The serial-FOO analogy doesn't quite work for iommu/smmu's, because one is plugging a 'serial device' INTO a FOO-bus.

In this series, an iommu/smmu needs to be placed -BETWEEN- a sysbus and a PCIe-tree,
or step-wise, plug an smmuv3 into a sysbus, and a pcie tree/domain/RC into an SMMUv3.

So, an smmu needs to be associated with a bus (tree), i.e., pcie.0, pcie.1...
One could model it as a PCIe device, attached at the pcie-RC ... but that's not how it's modelled in ARM hw.
SMMU's are discovered via ACPI tables.

That leaves us back to the 'how to associate an SMMUv3 to a PCIe tree(RC)',
and that leads me to the other discussion & format I saw btwn Eric & Shameer:
  -device arm-smmv3,id=smmuv3.3
  -device xxxx,smmuv3= smmuv3.3
where one tags a (PCIe) device to an smmuv3(id), which is needed to build the (proper) IORT for (pcie-)device<->SMMUv3 associativity in a multi-SMMUv3 configuration.

We could keep the bus=pcie.X option for the -device arm-smmuv3 to indicate that all PCIe devices connected to the pcie.0 tree go through that smmuv3;
qdev would model/config as the smmuv3 is 'attached to pcie.0'... which it sorta is...  and I think the IORT build could associate all devices on pcie.0 to be associated
with the proper smmuv3.



  

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Markus Armbruster]

Donald Dutile <ddutile@redhat.com> writes:

[...]

> In this series, an iommu/smmu needs to be placed -BETWEEN- a sysbus and a PCIe-tree,
> or step-wise, plug an smmuv3 into a sysbus, and a pcie tree/domain/RC into an SMMUv3.

RC = root complex?

> So, an smmu needs to be associated with a bus (tree), i.e., pcie.0, pcie.1...
> One could model it as a PCIe device, attached at the pcie-RC ... but that's not how it's modelled in ARM hw.

Physical ARM hardware?

Assuming the virtual devices and buses we're discussing model physical
devices and buses:

* What are the physical devices of interest?

* How are they wired together?  Which of the wires are buses, in
  particular PCI buses?

> SMMU's are discovered via ACPI tables.
>
> That leaves us back to the 'how to associate an SMMUv3 to a PCIe tree(RC)',
> and that leads me to the other discussion & format I saw btwn Eric & Shameer:
>  -device arm-smmv3,id=smmuv3.3
>  -device xxxx,smmuv3= smmuv3.3
> where one tags a (PCIe) device to an smmuv3(id), which is needed to build the (proper) IORT for (pcie-)device<->SMMUv3 associativity in a multi-SMMUv3 configuration.
>
> We could keep the bus=pcie.X option for the -device arm-smmuv3 to indicate that all PCIe devices connected to the pcie.0 tree go through that smmuv3;
> qdev would model/config as the smmuv3 is 'attached to pcie.0'... which it sorta is...  and I think the IORT build could associate all devices on pcie.0 to be associated
> with the proper smmuv3.

Device property "bus" is strictly for specifying into which the bus the
device is to be plugged.  The device's type must match the bus: only a
PCI device can plug into a PCI bus, and so forth.

A PCI device has a PCI address (dev.fn) on the bus it's plugged into.
If that's not the case for a physical smmuv3, we should not make the
virtual smmuv3 a PCI device.

Is there any prior art in QEMU, or is this the first device of this
kind?

RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Wednesday, May 7, 2025 8:17 AM
> To: Donald Dutile <ddutile@redhat.com>
> Cc: Shameer Kolothum via <qemu-devel@nongnu.org>; qemu-
> arm@nongnu.org; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; eric.auger@redhat.com;
> peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> berrange@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B)
> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>;
> zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> Donald Dutile <ddutile@redhat.com> writes:
> 
> [...]
> 
> > In this series, an iommu/smmu needs to be placed -BETWEEN- a sysbus
> and a PCIe-tree,
> > or step-wise, plug an smmuv3 into a sysbus, and a pcie tree/domain/RC
> into an SMMUv3.
> 
> RC = root complex?

Yes.

> 
> > So, an smmu needs to be associated with a bus (tree), i.e., pcie.0, pcie.1...
> > One could model it as a PCIe device, attached at the pcie-RC ... but that's
> not how it's modelled in ARM hw.
> 
> Physical ARM hardware?
> 
> Assuming the virtual devices and buses we're discussing model physical
> devices and buses:
> 
> * What are the physical devices of interest?
> 
> * How are they wired together?  Which of the wires are buses, in
>   particular PCI buses?

SMMUv3 is a platform device and its placement in a system is typically as below
for PCI devices,

+------------------+
|   PCIe Devices   |
+------------------+
         |
         v
  +-------------+      +---------------+
  |  PCIe RC A  |<---->| Interconnect  |
  +-------------+      +---------------+
                               |
                               |
                        +------v---+
                        | SMMUv3.A |
                        | (IOMMU)  |
                        +----+-----+
                             |
                             v
                     +-------+--------+
                     |   System RAM   |
                     +----------------+
 
This patch is attempting to establish that association between the PCIe RC and 
the SMMUv3 device so that Qemu can build the ACPI tables/DT iommu mappings  
for the SMMUv3 device.

> > SMMU's are discovered via ACPI tables.
> >
> > That leaves us back to the 'how to associate an SMMUv3 to a PCIe
> tree(RC)',
> > and that leads me to the other discussion & format I saw btwn Eric &
> Shameer:
> >  -device arm-smmv3,id=smmuv3.3
> >  -device xxxx,smmuv3= smmuv3.3
> > where one tags a (PCIe) device to an smmuv3(id), which is needed to build
> the (proper) IORT for (pcie-)device<->SMMUv3 associativity in a multi-
> SMMUv3 configuration.
> >
> > We could keep the bus=pcie.X option for the -device arm-smmuv3 to
> indicate that all PCIe devices connected to the pcie.0 tree go through that
> smmuv3;
> > qdev would model/config as the smmuv3 is 'attached to pcie.0'... which it
> sorta is...  and I think the IORT build could associate all devices on pcie.0 to
> be associated
> > with the proper smmuv3.
> 
> Device property "bus" is strictly for specifying into which the bus the
> device is to be plugged.  The device's type must match the bus: only a
> PCI device can plug into a PCI bus, and so forth.

The whole idea of reusing the "bus" property for SMMUv3 device was to make
it easier for libvirt. As I mentioned earlier we could go back and use a different
property name like "primary-bus" or "pci-bus" for SMMUv3 dev here.

Thanks,
Shameer

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Donald Dutile]

On 5/7/25 4:50 AM, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Wednesday, May 7, 2025 8:17 AM
>> To: Donald Dutile <ddutile@redhat.com>
>> Cc: Shameer Kolothum via <qemu-devel@nongnu.org>; qemu-
>> arm@nongnu.org; Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; eric.auger@redhat.com;
>> peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
>> berrange@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
>> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B)
>> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
>> Jonathan Cameron <jonathan.cameron@huawei.com>;
>> zhangfei.gao@linaro.org
>> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
>> PCIe RC
>>
>> Donald Dutile <ddutile@redhat.com> writes:
>>
>> [...]
>>
>>> In this series, an iommu/smmu needs to be placed -BETWEEN- a sysbus
>> and a PCIe-tree,
>>> or step-wise, plug an smmuv3 into a sysbus, and a pcie tree/domain/RC
>> into an SMMUv3.
>>
>> RC = root complex?
> 
> Yes.
> 
+1.

>>
>>> So, an smmu needs to be associated with a bus (tree), i.e., pcie.0, pcie.1...
>>> One could model it as a PCIe device, attached at the pcie-RC ... but that's
>> not how it's modelled in ARM hw.
>>
>> Physical ARM hardware?
>>
yes, physical hw.

>> Assuming the virtual devices and buses we're discussing model physical
>> devices and buses:
>>
>> * What are the physical devices of interest?
>>
>> * How are they wired together?  Which of the wires are buses, in
>>    particular PCI buses?
> 
> SMMUv3 is a platform device and its placement in a system is typically as below
> for PCI devices,
> 
> +------------------+
> |   PCIe Devices   |
> +------------------+
>           |
>           v
>    +-------------+      +---------------+
>    |  PCIe RC A  |<---->| Interconnect  |
>    +-------------+      +---------------+
>                                 |
>                                 |
>                          +------v---+
>                          | SMMUv3.A |
>                          | (IOMMU)  |
>                          +----+-----+
>                               |
>                               v
>                       +-------+--------+
>                       |   System RAM   |
>                       +----------------+
>   
> This patch is attempting to establish that association between the PCIe RC and
> the SMMUv3 device so that Qemu can build the ACPI tables/DT iommu mappings
> for the SMMUv3 device.
> 
I would refer to the ARM SMMU spec, Figure 2.3 in the G.a version, where
it's slightly different; more like:

  +------------------+
  |   PCIe Devices   | (one device, unless a PCIe switch is btwn the RC & 'Devices';
  +------------------+   or, see more typical expansion below)
            |
     +-------------+
     |  PCIe RC A  |
     +-------------+
            |
     +------v---+    +-----------------------------------+
     | SMMUv3.A |    | Wide assortment of other platform |
     | (IOMMU)  |    |   devices not using SMMU          |
     +----+-----+    +-----------------------------------+
          |                      |   |   |
   +------+----------------------+---+---+-+
   |         System Interconnect           |
   +---------------------------------------+
                                |
   +-------+--------+     +-----+-------------+
   |   System RAM   |<--->| CPU (NUMA socket) |
   +----------------+     +-------------------+

In fact, the PCIe can be quite complex with PCIe bridges, and multiple Root Ports (RP's),
and multiple SMMU's:

     +--------------+   +--------------+   +--------------+
     | PCIe Device  |   | PCIe Device  |   | PCIe Device  |
     +--------------+   +--------------+   +--------------+
           |                  |                  |        <--- PCIe bus
      +----------+       +----------+      +----------+
      | PCIe RP  |       | PCIe RP  |      | PCIe RP  |  <- may be PCI Bridge, may not
      +----------+       +----------+      +----------+
          |                  |                  |
      +----------+       +----------+       +----------+
      |  SMMU    |       |  SMMU    |       |  SMMU    |
      +----------+       +----------+       +----------+
          |                  |                  |   <- may be a bus, may not(hidden from OS)
          +------------------+------------------+
                             |
             +--------------------------+
             |          PCI RC A        |
             +--------------------------+
  
where PCIe RP's could be represented (even virtually) in -hw-
as a PCIe bridge, each downstream being a different PCIe bus under
a single PCIe RC (A, in above pic) -domain-.
... or the RPs don't have to have a PCIe bridge, and look like
'just an RP' that provides a PCIe (pt-to-pt, serial) bus, provided
by a PCIe RC. ... the PCIe architecture allows both, and I've seen
both implementations in hw (at least from an lspci perspective).

You can see the above hw implementation by doing an lspci on most
PCIe systems (definitely common on x86), where the RP's are represented
by 'PCIe bridge' elements -- and lots of them.
In real hw, these RP's effectively become (multiple) up & downstream transaction queues
(which implement PCI ordering, and deadlock avoidance).
SMMUs are effectively 'inserted' in the (upstream) queue path(s).

The important take away above: the SMMU can be RP &/or device-specific -- they
do not have to be bound to an entire PCIe domain ... the *fourth* part of
an lspci output for a PCIe device: Domain:Bus:Device.Function.
This is the case for INTEL & AMD IOMMUs ... and why the ACPI tables have
to describe which devices (busses often) are associated with which SMMU(in IORT)
or IOMMU(DMAR/IVRS's for INTEL/AMD IOMMU).

The final take away: the (QEMU) SMMU/IOMMU must be associated with a PCIe bus
OR, the format has to be something like:
   -device smmuv3, id=smmuv3.1
   -device <blah>, smmu=smmuv3.1
where the device <-> SMMU (or if extended to x86, iommu) associativity is set w/o bus associativity.
It'd be far easier to tag an entire bus with an SMMU/IOMMU, than a per-device format, esp. if
one has lots of PCIe devices in their model; actually, even if they only have one bus and 8 devices
(common), it'd be nice if a single iommu/smmu<->bus-num associativity could be set.

Oh, one final note: it is possible, although I haven't seen it done this way yet,
that an SMMU could be -in- a PCIe switch (further distributing SMMU functionality
across a large PCIe subsystem) and it -could- be a PCIe device in the switch,
btwn the upstream and downstream bridges -- actually doing the SMMU xlations
at that layer..... for QEMU & IORT, it's associated with a PCIe bus.
But, if done correctly, that shouldn't matter -- in the example you gave wrt serial,
it would be a new device, using common smmu core: smmuv3-pcie.
[Note: AMD actually identifies it's IOMMU as a PCIe device in an RC ... but still uses
        the ACPI tables to configure it to the OS.. so the PCIe-device is basically a
        device w/o a PCIe driver. AMD just went through hoops dealing with MS
        and AMD-IOMMU-identification via PCIe.]

So, stepping back, and looking at a broad(er) SMMU -or- IOMMU QEMU perspective,
I would think this type of format would be best:

- bus pcie, id=pcie.<num>
- device iommu=[intel_iommu|smmuv3|amd_iommu], bus=[sysbus | pcie.<num>], id=iommu.<num>
[Yes, I'm sticking with 'iommu' as the generic naming... everyone thinks of device SMMUs as IOMMUs,
  and QEMU should have a more arch-agnostic naming of these system functions. ]

and then the bus that devices are attached to in the system will define the IOMMU/SMMU
devices that they manage/translate (for simpler IORT/DMAR/IVRS generation.]
Options for iommu=none could be applied to any device on any bus (pcie or sysbus) to
logically exclude them from an IOMMU (effectively creating a virtual RP not managed by
an IOMMU; and simple IORT/DMAR/IVRS exclusion).

If/when intel_iommu (& eventual amd_iommu) get muti-instance support, the above
formatting would work for them.
... and I would expect someone from libvirt-land to chime in on even a better
format that makes it more common/generic, but allows for more robust, per-arch or
per-IOMMU/SMMU-arch variants/parametization.


If any of the above seems mirky, please ask for clarification(s).
Hopefully I haven't mis-typed any of the above, causing conflict or confusion,
as the concepts above are shared to show the array of hw architectures,
yet, try to dissolve them into common IOMMU config formats for QEMU
(for multi-instance-iommu and multi-bus).

- Don

>>> SMMU's are discovered via ACPI tables.
>>>
>>> That leaves us back to the 'how to associate an SMMUv3 to a PCIe
>> tree(RC)',
>>> and that leads me to the other discussion & format I saw btwn Eric &
>> Shameer:
>>>   -device arm-smmv3,id=smmuv3.3
>>>   -device xxxx,smmuv3= smmuv3.3
>>> where one tags a (PCIe) device to an smmuv3(id), which is needed to build
>> the (proper) IORT for (pcie-)device<->SMMUv3 associativity in a multi-
>> SMMUv3 configuration.
>>>
>>> We could keep the bus=pcie.X option for the -device arm-smmuv3 to
>> indicate that all PCIe devices connected to the pcie.0 tree go through that
>> smmuv3;
>>> qdev would model/config as the smmuv3 is 'attached to pcie.0'... which it
>> sorta is...  and I think the IORT build could associate all devices on pcie.0 to
>> be associated
>>> with the proper smmuv3.
>>
>> Device property "bus" is strictly for specifying into which the bus the
>> device is to be plugged.  The device's type must match the bus: only a
>> PCI device can plug into a PCI bus, and so forth.
> 
> The whole idea of reusing the "bus" property for SMMUv3 device was to make
> it easier for libvirt. As I mentioned earlier we could go back and use a different
> property name like "primary-bus" or "pci-bus" for SMMUv3 dev here.
> 
> Thanks,
> Shameer
> 

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Peter Maydell]

On Thu, 8 May 2025 at 14:46, Donald Dutile <ddutile@redhat.com> wrote:

> I would refer to the ARM SMMU spec, Figure 2.3 in the G.a version, where
> it's slightly different; more like:
>
>   +------------------+
>   |   PCIe Devices   | (one device, unless a PCIe switch is btwn the RC & 'Devices';
>   +------------------+   or, see more typical expansion below)
>             |
>      +-------------+
>      |  PCIe RC A  |
>      +-------------+
>             |
>      +------v---+    +-----------------------------------+
>      | SMMUv3.A |    | Wide assortment of other platform |
>      | (IOMMU)  |    |   devices not using SMMU          |
>      +----+-----+    +-----------------------------------+
>           |                      |   |   |
>    +------+----------------------+---+---+-+
>    |         System Interconnect           |
>    +---------------------------------------+
>                                 |
>    +-------+--------+     +-----+-------------+
>    |   System RAM   |<--->| CPU (NUMA socket) |
>    +----------------+     +-------------------+
>
> In fact, the PCIe can be quite complex with PCIe bridges, and multiple Root Ports (RP's),
> and multiple SMMU's:
>
>      +--------------+   +--------------+   +--------------+
>      | PCIe Device  |   | PCIe Device  |   | PCIe Device  |
>      +--------------+   +--------------+   +--------------+
>            |                  |                  |        <--- PCIe bus
>       +----------+       +----------+      +----------+
>       | PCIe RP  |       | PCIe RP  |      | PCIe RP  |  <- may be PCI Bridge, may not
>       +----------+       +----------+      +----------+
>           |                  |                  |
>       +----------+       +----------+       +----------+
>       |  SMMU    |       |  SMMU    |       |  SMMU    |
>       +----------+       +----------+       +----------+
>           |                  |                  |   <- may be a bus, may not(hidden from OS)
>           +------------------+------------------+
>                              |
>              +--------------------------+
>              |          PCI RC A        |
>              +--------------------------+
>


> The final take away: the (QEMU) SMMU/IOMMU must be associated with a PCIe bus
> OR, the format has to be something like:
>    -device smmuv3, id=smmuv3.1
>    -device <blah>, smmu=smmuv3.1
> where the device <-> SMMU (or if extended to x86, iommu) associativity is set w/o bus associativity.

The problem here seems to me to be that in the hardware we're
modelling the SMMU always exists, because it's in the SoC,
but you're trying to arrange for it to be created on the
command line, via -device.

We don't have any of these problems with the current 'virt'
board code, because we have the board code create the iommu
(if the user asks for it via the iommu machine property),
and it can wire it up to the PCI root complex as needed.

thanks
-- PMM

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> The problem here seems to me to be that in the hardware we're
> modelling the SMMU always exists, because it's in the SoC,
> but you're trying to arrange for it to be created on the
> command line, via -device.
>
> We don't have any of these problems with the current 'virt'
> board code, because we have the board code create the iommu
> (if the user asks for it via the iommu machine property),
> and it can wire it up to the PCI root complex as needed.

So what is the motivation for creating it with -device?

RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Thursday, May 8, 2025 2:58 PM
> To: Donald Dutile <ddutile@redhat.com>
> Cc: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Markus Armbruster
> <armbru@redhat.com>; Shameer Kolothum via <qemu-
> devel@nongnu.org>; qemu-arm@nongnu.org; eric.auger@redhat.com;
> jgg@nvidia.com; nicolinc@nvidia.com; berrange@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> On Thu, 8 May 2025 at 14:46, Donald Dutile <ddutile@redhat.com> wrote:
> 
> > I would refer to the ARM SMMU spec, Figure 2.3 in the G.a version, where
> > it's slightly different; more like:
> >
> >   +------------------+
> >   |   PCIe Devices   | (one device, unless a PCIe switch is btwn the RC &
> 'Devices';
> >   +------------------+   or, see more typical expansion below)
> >             |
> >      +-------------+
> >      |  PCIe RC A  |
> >      +-------------+
> >             |
> >      +------v---+    +-----------------------------------+
> >      | SMMUv3.A |    | Wide assortment of other platform |
> >      | (IOMMU)  |    |   devices not using SMMU          |
> >      +----+-----+    +-----------------------------------+
> >           |                      |   |   |
> >    +------+----------------------+---+---+-+
> >    |         System Interconnect           |
> >    +---------------------------------------+
> >                                 |
> >    +-------+--------+     +-----+-------------+
> >    |   System RAM   |<--->| CPU (NUMA socket) |
> >    +----------------+     +-------------------+
> >
> > In fact, the PCIe can be quite complex with PCIe bridges, and multiple
> Root Ports (RP's),
> > and multiple SMMU's:
> >
> >      +--------------+   +--------------+   +--------------+
> >      | PCIe Device  |   | PCIe Device  |   | PCIe Device  |
> >      +--------------+   +--------------+   +--------------+
> >            |                  |                  |        <--- PCIe bus
> >       +----------+       +----------+      +----------+
> >       | PCIe RP  |       | PCIe RP  |      | PCIe RP  |  <- may be PCI Bridge, may
> not
> >       +----------+       +----------+      +----------+
> >           |                  |                  |
> >       +----------+       +----------+       +----------+
> >       |  SMMU    |       |  SMMU    |       |  SMMU    |
> >       +----------+       +----------+       +----------+
> >           |                  |                  |   <- may be a bus, may not(hidden from
> OS)
> >           +------------------+------------------+
> >                              |
> >              +--------------------------+
> >              |          PCI RC A        |
> >              +--------------------------+
> >
> 
> 
> > The final take away: the (QEMU) SMMU/IOMMU must be associated with
> a PCIe bus
> > OR, the format has to be something like:
> >    -device smmuv3, id=smmuv3.1
> >    -device <blah>, smmu=smmuv3.1
> > where the device <-> SMMU (or if extended to x86, iommu) associativity is
> set w/o bus associativity.
> 
> The problem here seems to me to be that in the hardware we're
> modelling the SMMU always exists, because it's in the SoC,
> but you're trying to arrange for it to be created on the
> command line, via -device.
> 
> We don't have any of these problems with the current 'virt'
> board code, because we have the board code create the iommu
> (if the user asks for it via the iommu machine property),
> and it can wire it up to the PCI root complex as needed.

Yes, currently virt creates a SMMUv3 instance and associates it with
pcie.0 by default. However, this setup isn't ideal once we introduce
support for accelerated SMMUv3, where we need to configure the
host SMMUv3 for nested stage translation. This is essential for VFIO-PCI
passthrough scenarios, where the guest manages the stage-1 page tables
and the host manages stage-2.

In such cases, devices may be associated with different host SMMUv3s, 
and having a single vSMMUv3 in the guest isn't ideal as-

-We would need to broadcast TLBIs or perform lookups to identify the
 corresponding host SMMUv3.
-The physical SMMUv3s might differ in functionality.
-Some SMMUv3 implementations offer additional virtualization features (e.g., vCMDQ),
  where the CMDQ is is directly gets mapped to the Guest and invalidations
  aren't  even trapped.

Please refer to the earlier RFC [1], which proposed a new arm-smmuv3-accel
device. The main feedback on that RFC was to instead convert the existing
arm-smmuv3 into a user-creatable device, and introduce acceleration support
via an optional property:

-device arm-smmuv3,accel=on,..

This series is the first step toward that goal—making the current arm-smmuv3 device
user-creatable.

Please let me know if there's a better approach to integrating accelerated
SMMUv3 support here.

Thanks,
Shameer
[1] https://lore.kernel.org/qemu-devel/20250311141045.66620-1-shameerali.kolothum.thodi@huawei.com/

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Peter Maydell]

On Fri, 9 May 2025 at 09:00, Shameerali Kolothum Thodi
<shameerali.kolothum.thodi@huawei.com> wrote:
> From: Peter Maydell <peter.maydell@linaro.org>
> > The problem here seems to me to be that in the hardware we're
> > modelling the SMMU always exists, because it's in the SoC,
> > but you're trying to arrange for it to be created on the
> > command line, via -device.
> >
> > We don't have any of these problems with the current 'virt'
> > board code, because we have the board code create the iommu
> > (if the user asks for it via the iommu machine property),
> > and it can wire it up to the PCI root complex as needed.
>
> Yes, currently virt creates a SMMUv3 instance and associates it with
> pcie.0 by default. However, this setup isn't ideal once we introduce
> support for accelerated SMMUv3, where we need to configure the
> host SMMUv3 for nested stage translation. This is essential for VFIO-PCI
> passthrough scenarios, where the guest manages the stage-1 page tables
> and the host manages stage-2.
>
> In such cases, devices may be associated with different host SMMUv3s,
> and having a single vSMMUv3 in the guest isn't ideal as-
>
> -We would need to broadcast TLBIs or perform lookups to identify the
>  corresponding host SMMUv3.
> -The physical SMMUv3s might differ in functionality.
> -Some SMMUv3 implementations offer additional virtualization features (e.g., vCMDQ),
>   where the CMDQ is is directly gets mapped to the Guest and invalidations
>   aren't  even trapped.
>
> Please refer to the earlier RFC [1], which proposed a new arm-smmuv3-accel
> device. The main feedback on that RFC was to instead convert the existing
> arm-smmuv3 into a user-creatable device, and introduce acceleration support
> via an optional property:
>
> -device arm-smmuv3,accel=on,..
>
> This series is the first step toward that goal—making the current arm-smmuv3 device
> user-creatable.

(I want to start here by saying that I appreciate that I'm
coming in without having read the previous discussion, so
this is kind of going back over ground you've already
been through.)

I agree that rather than having an entirely separate "SMMU with
acceleration" it would be better to have it be a property on
the SMMU device. But why do we need it to be user created?
Making it user-created leads into all kinds of tricky areas
mostly surrounding the fact that QEMU right now simply doesn't
support having user-created sysbus devices and other kinds
of device with complex wiring-up. -device is really intended
for "this is a model of a device that in real hardware is
pluggable and has basically one connection, like a PCI card
has a PCI-slot".

-- PMM

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Daniel P. Berrangé]

On Fri, May 09, 2025 at 11:37:14AM +0100, Peter Maydell wrote:
> On Fri, 9 May 2025 at 09:00, Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com> wrote:
> > From: Peter Maydell <peter.maydell@linaro.org>
> > > The problem here seems to me to be that in the hardware we're
> > > modelling the SMMU always exists, because it's in the SoC,
> > > but you're trying to arrange for it to be created on the
> > > command line, via -device.
> > >
> > > We don't have any of these problems with the current 'virt'
> > > board code, because we have the board code create the iommu
> > > (if the user asks for it via the iommu machine property),
> > > and it can wire it up to the PCI root complex as needed.
> >
> > Yes, currently virt creates a SMMUv3 instance and associates it with
> > pcie.0 by default. However, this setup isn't ideal once we introduce
> > support for accelerated SMMUv3, where we need to configure the
> > host SMMUv3 for nested stage translation. This is essential for VFIO-PCI
> > passthrough scenarios, where the guest manages the stage-1 page tables
> > and the host manages stage-2.
> >
> > In such cases, devices may be associated with different host SMMUv3s,
> > and having a single vSMMUv3 in the guest isn't ideal as-
> >
> > -We would need to broadcast TLBIs or perform lookups to identify the
> >  corresponding host SMMUv3.
> > -The physical SMMUv3s might differ in functionality.
> > -Some SMMUv3 implementations offer additional virtualization features (e.g., vCMDQ),
> >   where the CMDQ is is directly gets mapped to the Guest and invalidations
> >   aren't  even trapped.
> >
> > Please refer to the earlier RFC [1], which proposed a new arm-smmuv3-accel
> > device. The main feedback on that RFC was to instead convert the existing
> > arm-smmuv3 into a user-creatable device, and introduce acceleration support
> > via an optional property:
> >
> > -device arm-smmuv3,accel=on,..
> >
> > This series is the first step toward that goal—making the current arm-smmuv3 device
> > user-creatable.
> 
> (I want to start here by saying that I appreciate that I'm
> coming in without having read the previous discussion, so
> this is kind of going back over ground you've already
> been through.)
> 
> I agree that rather than having an entirely separate "SMMU with
> acceleration" it would be better to have it be a property on
> the SMMU device. But why do we need it to be user created?
> Making it user-created leads into all kinds of tricky areas
> mostly surrounding the fact that QEMU right now simply doesn't
> support having user-created sysbus devices and other kinds
> of device with complex wiring-up. -device is really intended
> for "this is a model of a device that in real hardware is
> pluggable and has basically one connection, like a PCI card
> has a PCI-slot".

In terms of "why does it need to be user created" - the goal was to expose
multiple SMMUs to the guest, each associated with a separate physical SMMU.
IIUC, each physical NUMA node would have its own SMMU.

So configuring a guest VM will require creating multiple PXBs, one for
each virtual NUMA node, and then creating SMMUs for each PXB.

Since there was a need for the user to create SMMUs for the PXBs, the
question was then raised, why shouldn't the default SMMU also be
user creatable in the same way, so that mgmt apps like libvirt have
a single way to configure the SMMUs with -device.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Peter Maydell]

On Fri, 9 May 2025 at 11:46, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Fri, May 09, 2025 at 11:37:14AM +0100, Peter Maydell wrote:
> > (I want to start here by saying that I appreciate that I'm
> > coming in without having read the previous discussion, so
> > this is kind of going back over ground you've already
> > been through.)
> >
> > I agree that rather than having an entirely separate "SMMU with
> > acceleration" it would be better to have it be a property on
> > the SMMU device. But why do we need it to be user created?
> > Making it user-created leads into all kinds of tricky areas
> > mostly surrounding the fact that QEMU right now simply doesn't
> > support having user-created sysbus devices and other kinds
> > of device with complex wiring-up. -device is really intended
> > for "this is a model of a device that in real hardware is
> > pluggable and has basically one connection, like a PCI card
> > has a PCI-slot".
>
> In terms of "why does it need to be user created" - the goal was to expose
> multiple SMMUs to the guest, each associated with a separate physical SMMU.
> IIUC, each physical NUMA node would have its own SMMU.
>
> So configuring a guest VM will require creating multiple PXBs, one for
> each virtual NUMA node, and then creating SMMUs for each PXB.
>
> Since there was a need for the user to create SMMUs for the PXBs, the
> question was then raised, why shouldn't the default SMMU also be
> user creatable in the same way, so that mgmt apps like libvirt have
> a single way to configure the SMMUs with -device.

Sure, the default "there's just one pci bridge and either
no SMMU or one SMMU" isn't that special. But we don't
have good infrastructure for creating sysbus devices on
the command line, whether it's the default SMMU or the
extra SMMUs or a UART or anything else. I guess the
dynamic_sysbus stuff works, but I've never really liked it
(it's basically "the board will magically do the right thing",
and to some extent it's working around the way we have
very patchy support for "I want to configure a board the
device created rather than configuring a device I am
creating").

-- PMM

RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Friday, May 9, 2025 12:44 PM
> To: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Donald Dutile
> <ddutile@redhat.com>; Markus Armbruster <armbru@redhat.com>;
> Shameer Kolothum via <qemu-devel@nongnu.org>; qemu-
> arm@nongnu.org; eric.auger@redhat.com; jgg@nvidia.com;
> nicolinc@nvidia.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B)
> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>;
> zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> On Fri, 9 May 2025 at 11:46, Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> >
> > On Fri, May 09, 2025 at 11:37:14AM +0100, Peter Maydell wrote:
> > > (I want to start here by saying that I appreciate that I'm
> > > coming in without having read the previous discussion, so
> > > this is kind of going back over ground you've already
> > > been through.)
> > >
> > > I agree that rather than having an entirely separate "SMMU with
> > > acceleration" it would be better to have it be a property on
> > > the SMMU device. But why do we need it to be user created?
> > > Making it user-created leads into all kinds of tricky areas
> > > mostly surrounding the fact that QEMU right now simply doesn't
> > > support having user-created sysbus devices and other kinds
> > > of device with complex wiring-up. -device is really intended
> > > for "this is a model of a device that in real hardware is
> > > pluggable and has basically one connection, like a PCI card
> > > has a PCI-slot".
> >
> > In terms of "why does it need to be user created" - the goal was to expose
> > multiple SMMUs to the guest, each associated with a separate physical
> SMMU.
> > IIUC, each physical NUMA node would have its own SMMU.
> >
> > So configuring a guest VM will require creating multiple PXBs, one for
> > each virtual NUMA node, and then creating SMMUs for each PXB.
> >
> > Since there was a need for the user to create SMMUs for the PXBs, the
> > question was then raised, why shouldn't the default SMMU also be
> > user creatable in the same way, so that mgmt apps like libvirt have
> > a single way to configure the SMMUs with -device.
> 
> Sure, the default "there's just one pci bridge and either
> no SMMU or one SMMU" isn't that special. But we don't
> have good infrastructure for creating sysbus devices on
> the command line, whether it's the default SMMU or the
> extra SMMUs or a UART or anything else. I guess the
> dynamic_sysbus stuff works, but I've never really liked it
> (it's basically "the board will magically do the right thing",
> and to some extent it's working around the way we have
> very patchy support for "I want to configure a board the
> device created rather than configuring a device I am
> creating").

I agree that having users create sysbus devices for a machine isn't
ideal.  However, in this case, the association between SMMUv3 and
PCIe makes the topology difficult to represent cleanly in any other way.

There was a previous attempt by Nicolin [0], where the virt machine
would probe all host physical SMMUv3 instances and automatically
create the corresponding arm-smmuv3 devices along with associated
pxb-pcie bridges. The main concern raised with that approach was that
QEMU shouldn't implicitly construct PCIe topology behind the scenes
especially when it can conflict with how libvirt expects to define the
PCIe hierarchy [1].

Another suggestion was to add an iommu=<id> option to the PCIe bridge:
-device pxb-pcie,iommu=<id>,...

With this, the virt machine would create a new SMMUv3 device
whenever a new iommu ID is encountered.

But this also has limitations, as we want to support additional options
like accel or vcmdq on the created SMMUv3 devices. The most flexible
way to express such configurations is directly via the device model, e.g.:

-device arm-smmuv3,accel=on,vcmdq=on,

I think this is also consistent with how the intel-iommu device works
today.

Please let me know suggestions if there's a better solution here. But if
the current approach — while not ideal — is acceptable, I can rework 
the series with the review comments and post a v3 soon.

Please let me know.

Thanks,
Shameer
[0] https://lore.kernel.org/qemu-devel/cover.1719361174.git.nicolinc@nvidia.com/
[1] https://lore.kernel.org/qemu-devel/CABJz62PVt9h9776DjXkPYq_Mf+AUJ-0YhnDi-OsaqCqrsrGSQQ@mail.gmail.com/

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Donald Dutile]

On 5/8/25 9:57 AM, Peter Maydell wrote:
> On Thu, 8 May 2025 at 14:46, Donald Dutile <ddutile@redhat.com> wrote:
> 
>> I would refer to the ARM SMMU spec, Figure 2.3 in the G.a version, where
>> it's slightly different; more like:
>>
>>    +------------------+
>>    |   PCIe Devices   | (one device, unless a PCIe switch is btwn the RC & 'Devices';
>>    +------------------+   or, see more typical expansion below)
>>              |
>>       +-------------+
>>       |  PCIe RC A  |
>>       +-------------+
>>              |
>>       +------v---+    +-----------------------------------+
>>       | SMMUv3.A |    | Wide assortment of other platform |
>>       | (IOMMU)  |    |   devices not using SMMU          |
>>       +----+-----+    +-----------------------------------+
>>            |                      |   |   |
>>     +------+----------------------+---+---+-+
>>     |         System Interconnect           |
>>     +---------------------------------------+
>>                                  |
>>     +-------+--------+     +-----+-------------+
>>     |   System RAM   |<--->| CPU (NUMA socket) |
>>     +----------------+     +-------------------+
>>
>> In fact, the PCIe can be quite complex with PCIe bridges, and multiple Root Ports (RP's),
>> and multiple SMMU's:
>>
>>       +--------------+   +--------------+   +--------------+
>>       | PCIe Device  |   | PCIe Device  |   | PCIe Device  |
>>       +--------------+   +--------------+   +--------------+
>>             |                  |                  |        <--- PCIe bus
>>        +----------+       +----------+      +----------+
>>        | PCIe RP  |       | PCIe RP  |      | PCIe RP  |  <- may be PCI Bridge, may not
>>        +----------+       +----------+      +----------+
>>            |                  |                  |
>>        +----------+       +----------+       +----------+
>>        |  SMMU    |       |  SMMU    |       |  SMMU    |
>>        +----------+       +----------+       +----------+
>>            |                  |                  |   <- may be a bus, may not(hidden from OS)
>>            +------------------+------------------+
>>                               |
>>               +--------------------------+
>>               |          PCI RC A        |
>>               +--------------------------+
>>
> 
> 
>> The final take away: the (QEMU) SMMU/IOMMU must be associated with a PCIe bus
>> OR, the format has to be something like:
>>     -device smmuv3, id=smmuv3.1
>>     -device <blah>, smmu=smmuv3.1
>> where the device <-> SMMU (or if extended to x86, iommu) associativity is set w/o bus associativity.
> 
> The problem here seems to me to be that in the hardware we're
> modelling the SMMU always exists, because it's in the SoC,
> but you're trying to arrange for it to be created on the
> command line, via -device.
> 
> We don't have any of these problems with the current 'virt'
> board code, because we have the board code create the iommu
> (if the user asks for it via the iommu machine property),
> and it can wire it up to the PCI root complex as needed.
> 
> thanks
> -- PMM
> 
Peter,
Hey!
Learning more about the use-of/focus/expectation of '-device', I see your point.
The issue is that there isn't just one SMMU in a system (as the diagram above illustrates), they have associativity to
(pcie) devices (possibly more accurately, a pcie-(sub) tree) (for the focus of this discussion; could be non-pcie devices),
and we need an option format to associate smmu to pcie(domain/subtree(RPs), devices), and that typically references
busses (pxbs?) &/or pcie devices (-device). ... or do we only need to add iommu associativity to devices,
  ie, the second option line above, -device <blah>, iommu=smmuv3.1 ... which should enable the right IORT(s) to be
made for multiple smmus/iommus.

- Don

RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Donald Dutile <ddutile@redhat.com>
> Sent: Thursday, May 8, 2025 2:45 PM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Markus Armbruster
> <armbru@redhat.com>
> Cc: Shameer Kolothum via <qemu-devel@nongnu.org>; qemu-
> arm@nongnu.org; eric.auger@redhat.com; peter.maydell@linaro.org;
> jgg@nvidia.com; nicolinc@nvidia.com; berrange@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> 
> 
> On 5/7/25 4:50 AM, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Wednesday, May 7, 2025 8:17 AM
> >> To: Donald Dutile <ddutile@redhat.com>
> >> Cc: Shameer Kolothum via <qemu-devel@nongnu.org>; qemu-
> >> arm@nongnu.org; Shameerali Kolothum Thodi
> >> <shameerali.kolothum.thodi@huawei.com>; eric.auger@redhat.com;
> >> peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> >> berrange@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> >> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou
> (B)
> >> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> >> Jonathan Cameron <jonathan.cameron@huawei.com>;
> >> zhangfei.gao@linaro.org
> >> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> >> PCIe RC
> >>
> >> Donald Dutile <ddutile@redhat.com> writes:
> >>
> >> [...]
> >>
> >>> In this series, an iommu/smmu needs to be placed -BETWEEN- a sysbus
> >> and a PCIe-tree,
> >>> or step-wise, plug an smmuv3 into a sysbus, and a pcie tree/domain/RC
> >> into an SMMUv3.
> >>
> >> RC = root complex?
> >
> > Yes.
> >
> +1.
> 
> >>
> >>> So, an smmu needs to be associated with a bus (tree), i.e., pcie.0,
> pcie.1...
> >>> One could model it as a PCIe device, attached at the pcie-RC ... but
> that's
> >> not how it's modelled in ARM hw.
> >>
> >> Physical ARM hardware?
> >>
> yes, physical hw.
> 
> >> Assuming the virtual devices and buses we're discussing model physical
> >> devices and buses:
> >>
> >> * What are the physical devices of interest?
> >>
> >> * How are they wired together?  Which of the wires are buses, in
> >>    particular PCI buses?
> >
> > SMMUv3 is a platform device and its placement in a system is typically as
> below
> > for PCI devices,
> >
> > +------------------+
> > |   PCIe Devices   |
> > +------------------+
> >           |
> >           v
> >    +-------------+      +---------------+
> >    |  PCIe RC A  |<---->| Interconnect  |
> >    +-------------+      +---------------+
> >                                 |
> >                                 |
> >                          +------v---+
> >                          | SMMUv3.A |
> >                          | (IOMMU)  |
> >                          +----+-----+
> >                               |
> >                               v
> >                       +-------+--------+
> >                       |   System RAM   |
> >                       +----------------+
> >
> > This patch is attempting to establish that association between the PCIe
> RC and
> > the SMMUv3 device so that Qemu can build the ACPI tables/DT iommu
> mappings
> > for the SMMUv3 device.
> >
> I would refer to the ARM SMMU spec, Figure 2.3 in the G.a version, where
> it's slightly different; more like:

That's right. The spec does indeed cover all possible scenarios, whereas my earlier
comments were focused more specifically on the common case of systems using
SMMUv3 with PCIe devices.

Currently, QEMU doesn't support non-PCI devices with SMMUv3, neither the
more complex distributed SMMU cases you have described below. And this series
doesn't aim to add those supports either. If needed, we can treat those as a separate
efforts—similar to what was attempted in [1]. That said, agree that the design
choices we make now should not hinder adding such support in the future.

And as far as I can see, nothing in this series would prevent that and if anything,
the new device type SMMUv3 model actually makes it easier to support those.

>   +------------------+
>   |   PCIe Devices   | (one device, unless a PCIe switch is btwn the RC &
> 'Devices';
>   +------------------+   or, see more typical expansion below)
>             |
>      +-------------+
>      |  PCIe RC A  |
>      +-------------+
>             |
>      +------v---+    +-----------------------------------+
>      | SMMUv3.A |    | Wide assortment of other platform |
>      | (IOMMU)  |    |   devices not using SMMU          |
>      +----+-----+    +-----------------------------------+
>           |                      |   |   |
>    +------+----------------------+---+---+-+
>    |         System Interconnect           |
>    +---------------------------------------+
>                                 |
>    +-------+--------+     +-----+-------------+
>    |   System RAM   |<--->| CPU (NUMA socket) |
>    +----------------+     +-------------------+
> 
> In fact, the PCIe can be quite complex with PCIe bridges, and multiple Root
> Ports (RP's),
> and multiple SMMU's:
> 
>      +--------------+   +--------------+   +--------------+
>      | PCIe Device  |   | PCIe Device  |   | PCIe Device  |
>      +--------------+   +--------------+   +--------------+
>            |                  |                  |        <--- PCIe bus
>       +----------+       +----------+      +----------+
>       | PCIe RP  |       | PCIe RP  |      | PCIe RP  |  <- may be PCI Bridge, may
> not
>       +----------+       +----------+      +----------+
>           |                  |                  |
>       +----------+       +----------+       +----------+
>       |  SMMU    |       |  SMMU    |       |  SMMU    |
>       +----------+       +----------+       +----------+
>           |                  |                  |   <- may be a bus, may not(hidden from OS)
>           +------------------+------------------+
>                              |
>              +--------------------------+
>              |          PCI RC A        |
>              +--------------------------+
> 
> where PCIe RP's could be represented (even virtually) in -hw-
> as a PCIe bridge, each downstream being a different PCIe bus under
> a single PCIe RC (A, in above pic) -domain-.
> ... or the RPs don't have to have a PCIe bridge, and look like
> 'just an RP' that provides a PCIe (pt-to-pt, serial) bus, provided
> by a PCIe RC. ... the PCIe architecture allows both, and I've seen
> both implementations in hw (at least from an lspci perspective).
> 
> You can see the above hw implementation by doing an lspci on most
> PCIe systems (definitely common on x86), where the RP's are represented
> by 'PCIe bridge' elements -- and lots of them.
> In real hw, these RP's effectively become (multiple) up & downstream
> transaction queues
> (which implement PCI ordering, and deadlock avoidance).
> SMMUs are effectively 'inserted' in the (upstream) queue path(s).
> 
> The important take away above: the SMMU can be RP &/or device-specific -
> - they
> do not have to be bound to an entire PCIe domain ... the *fourth* part of
> an lspci output for a PCIe device: Domain:Bus:Device.Function.
> This is the case for INTEL & AMD IOMMUs ... and why the ACPI tables have
> to describe which devices (busses often) are associated with which
> SMMU(in IORT)
> or IOMMU(DMAR/IVRS's for INTEL/AMD IOMMU).
> 
> The final take away: the (QEMU) SMMU/IOMMU must be associated with a
> PCIe bus
> OR, the format has to be something like:
>    -device smmuv3, id=smmuv3.1
>    -device <blah>, smmu=smmuv3.1

Agree. For PCie devices with SMMUv3 we need to associate it with a PCIe bus
and for non-pci cases probably needs a per device association.

> where the device <-> SMMU (or if extended to x86, iommu) associativity is
> set w/o bus associativity.
> It'd be far easier to tag an entire bus with an SMMU/IOMMU, than a per-
> device format, esp. if
> one has lots of PCIe devices in their model; actually, even if they only have
> one bus and 8 devices
> (common), it'd be nice if a single iommu/smmu<->bus-num associativity
> could be set.
> 
> Oh, one final note: it is possible, although I haven't seen it done this way
> yet,
> that an SMMU could be -in- a PCIe switch (further distributing SMMU
> functionality
> across a large PCIe subsystem) and it -could- be a PCIe device in the switch,
> btwn the upstream and downstream bridges -- actually doing the SMMU
> xlations
> at that layer..... for QEMU & IORT, it's associated with a PCIe bus.
> But, if done correctly, that shouldn't matter -- in the example you gave wrt
> serial,
> it would be a new device, using common smmu core: smmuv3-pcie.
> [Note: AMD actually identifies it's IOMMU as a PCIe device in an RC ... but
> still uses
>         the ACPI tables to configure it to the OS.. so the PCIe-device is basically
> a
>         device w/o a PCIe driver. AMD just went through hoops dealing with
> MS
>         and AMD-IOMMU-identification via PCIe.]
> 
> So, stepping back, and looking at a broad(er) SMMU -or- IOMMU QEMU
> perspective,
> I would think this type of format would be best:
> 
> - bus pcie, id=pcie.<num>
> - device iommu=[intel_iommu|smmuv3|amd_iommu], bus=[sysbus |
> pcie.<num>], id=iommu.<num>
> [Yes, I'm sticking with 'iommu' as the generic naming... everyone thinks of
> device SMMUs as IOMMUs,
>   and QEMU should have a more arch-agnostic naming of these system
> functions. ]

Ok. But to circle back to what originally started this discussion—how important
is it to rely on the default "bus" in this case? As Markus pointed out, SMMUv3
is a platform device on the sysbus, so its default bus type can’t point to something
like PCIe. QEMU doesn’t currently support that.

The main motivation for using the default "bus" so far has been to have better
compatibility with libvirt. Would libvirt be flexible enough if we switched to using
something like a "primary-bus" property instead?

-device arm-smmuv3,primary-bus=pcie.0
-device virtio-net-pci,bus=pcie.0
-device pxb-pcie,id=pcie.1,bus_nr=2
-device arm-smmuv3,primary-bus=pcie.1
...

Please let me know.

Thanks,
Shameer
[1] https://lore.kernel.org/all/20210902081429.140293-1-chunming_li1234@163.com/

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Daniel P. Berrangé]

On Fri, May 09, 2025 at 07:29:28AM +0000, Shameerali Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Donald Dutile <ddutile@redhat.com>
> > Sent: Thursday, May 8, 2025 2:45 PM
> > To: Shameerali Kolothum Thodi
> > <shameerali.kolothum.thodi@huawei.com>; Markus Armbruster
> > <armbru@redhat.com>
> > Cc: Shameer Kolothum via <qemu-devel@nongnu.org>; qemu-
> > arm@nongnu.org; eric.auger@redhat.com; peter.maydell@linaro.org;
> > jgg@nvidia.com; nicolinc@nvidia.com; berrange@redhat.com;
> > nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com; Linuxarm
> > <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> > Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> > PCIe RC
> > 
> > 
> > 
> > On 5/7/25 4:50 AM, Shameerali Kolothum Thodi wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Markus Armbruster <armbru@redhat.com>
> > >> Sent: Wednesday, May 7, 2025 8:17 AM
> > >> To: Donald Dutile <ddutile@redhat.com>
> > >> Cc: Shameer Kolothum via <qemu-devel@nongnu.org>; qemu-
> > >> arm@nongnu.org; Shameerali Kolothum Thodi
> > >> <shameerali.kolothum.thodi@huawei.com>; eric.auger@redhat.com;
> > >> peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> > >> berrange@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > >> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou
> > (B)
> > >> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> > >> Jonathan Cameron <jonathan.cameron@huawei.com>;
> > >> zhangfei.gao@linaro.org
> > >> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> > >> PCIe RC
> > >>
> > >> Donald Dutile <ddutile@redhat.com> writes:
> > >>
> > >> [...]
> > >>
> > >>> In this series, an iommu/smmu needs to be placed -BETWEEN- a sysbus
> > >> and a PCIe-tree,
> > >>> or step-wise, plug an smmuv3 into a sysbus, and a pcie tree/domain/RC
> > >> into an SMMUv3.
> > >>
> > >> RC = root complex?
> > >
> > > Yes.
> > >
> > +1.
> > 
> > >>
> > >>> So, an smmu needs to be associated with a bus (tree), i.e., pcie.0,
> > pcie.1...
> > >>> One could model it as a PCIe device, attached at the pcie-RC ... but
> > that's
> > >> not how it's modelled in ARM hw.
> > >>
> > >> Physical ARM hardware?
> > >>
> > yes, physical hw.
> > 
> > >> Assuming the virtual devices and buses we're discussing model physical
> > >> devices and buses:
> > >>
> > >> * What are the physical devices of interest?
> > >>
> > >> * How are they wired together?  Which of the wires are buses, in
> > >>    particular PCI buses?
> > >
> > > SMMUv3 is a platform device and its placement in a system is typically as
> > below
> > > for PCI devices,
> > >
> > > +------------------+
> > > |   PCIe Devices   |
> > > +------------------+
> > >           |
> > >           v
> > >    +-------------+      +---------------+
> > >    |  PCIe RC A  |<---->| Interconnect  |
> > >    +-------------+      +---------------+
> > >                                 |
> > >                                 |
> > >                          +------v---+
> > >                          | SMMUv3.A |
> > >                          | (IOMMU)  |
> > >                          +----+-----+
> > >                               |
> > >                               v
> > >                       +-------+--------+
> > >                       |   System RAM   |
> > >                       +----------------+
> > >
> > > This patch is attempting to establish that association between the PCIe
> > RC and
> > > the SMMUv3 device so that Qemu can build the ACPI tables/DT iommu
> > mappings
> > > for the SMMUv3 device.
> > >
> > I would refer to the ARM SMMU spec, Figure 2.3 in the G.a version, where
> > it's slightly different; more like:
> 
> That's right. The spec does indeed cover all possible scenarios, whereas my earlier
> comments were focused more specifically on the common case of systems using
> SMMUv3 with PCIe devices.
> 
> Currently, QEMU doesn't support non-PCI devices with SMMUv3, neither the
> more complex distributed SMMU cases you have described below. And this series
> doesn't aim to add those supports either. If needed, we can treat those as a separate
> efforts—similar to what was attempted in [1]. That said, agree that the design
> choices we make now should not hinder adding such support in the future.
> 
> And as far as I can see, nothing in this series would prevent that and if anything,
> the new device type SMMUv3 model actually makes it easier to support those.
> 
> >   +------------------+
> >   |   PCIe Devices   | (one device, unless a PCIe switch is btwn the RC &
> > 'Devices';
> >   +------------------+   or, see more typical expansion below)
> >             |
> >      +-------------+
> >      |  PCIe RC A  |
> >      +-------------+
> >             |
> >      +------v---+    +-----------------------------------+
> >      | SMMUv3.A |    | Wide assortment of other platform |
> >      | (IOMMU)  |    |   devices not using SMMU          |
> >      +----+-----+    +-----------------------------------+
> >           |                      |   |   |
> >    +------+----------------------+---+---+-+
> >    |         System Interconnect           |
> >    +---------------------------------------+
> >                                 |
> >    +-------+--------+     +-----+-------------+
> >    |   System RAM   |<--->| CPU (NUMA socket) |
> >    +----------------+     +-------------------+
> > 
> > In fact, the PCIe can be quite complex with PCIe bridges, and multiple Root
> > Ports (RP's),
> > and multiple SMMU's:
> > 
> >      +--------------+   +--------------+   +--------------+
> >      | PCIe Device  |   | PCIe Device  |   | PCIe Device  |
> >      +--------------+   +--------------+   +--------------+
> >            |                  |                  |        <--- PCIe bus
> >       +----------+       +----------+      +----------+
> >       | PCIe RP  |       | PCIe RP  |      | PCIe RP  |  <- may be PCI Bridge, may
> > not
> >       +----------+       +----------+      +----------+
> >           |                  |                  |
> >       +----------+       +----------+       +----------+
> >       |  SMMU    |       |  SMMU    |       |  SMMU    |
> >       +----------+       +----------+       +----------+
> >           |                  |                  |   <- may be a bus, may not(hidden from OS)
> >           +------------------+------------------+
> >                              |
> >              +--------------------------+
> >              |          PCI RC A        |
> >              +--------------------------+
> > 
> > where PCIe RP's could be represented (even virtually) in -hw-
> > as a PCIe bridge, each downstream being a different PCIe bus under
> > a single PCIe RC (A, in above pic) -domain-.
> > ... or the RPs don't have to have a PCIe bridge, and look like
> > 'just an RP' that provides a PCIe (pt-to-pt, serial) bus, provided
> > by a PCIe RC. ... the PCIe architecture allows both, and I've seen
> > both implementations in hw (at least from an lspci perspective).
> > 
> > You can see the above hw implementation by doing an lspci on most
> > PCIe systems (definitely common on x86), where the RP's are represented
> > by 'PCIe bridge' elements -- and lots of them.
> > In real hw, these RP's effectively become (multiple) up & downstream
> > transaction queues
> > (which implement PCI ordering, and deadlock avoidance).
> > SMMUs are effectively 'inserted' in the (upstream) queue path(s).
> > 
> > The important take away above: the SMMU can be RP &/or device-specific -
> > - they
> > do not have to be bound to an entire PCIe domain ... the *fourth* part of
> > an lspci output for a PCIe device: Domain:Bus:Device.Function.
> > This is the case for INTEL & AMD IOMMUs ... and why the ACPI tables have
> > to describe which devices (busses often) are associated with which
> > SMMU(in IORT)
> > or IOMMU(DMAR/IVRS's for INTEL/AMD IOMMU).
> > 
> > The final take away: the (QEMU) SMMU/IOMMU must be associated with a
> > PCIe bus
> > OR, the format has to be something like:
> >    -device smmuv3, id=smmuv3.1
> >    -device <blah>, smmu=smmuv3.1
> 
> Agree. For PCie devices with SMMUv3 we need to associate it with a PCIe bus
> and for non-pci cases probably needs a per device association.
> 
> > where the device <-> SMMU (or if extended to x86, iommu) associativity is
> > set w/o bus associativity.
> > It'd be far easier to tag an entire bus with an SMMU/IOMMU, than a per-
> > device format, esp. if
> > one has lots of PCIe devices in their model; actually, even if they only have
> > one bus and 8 devices
> > (common), it'd be nice if a single iommu/smmu<->bus-num associativity
> > could be set.
> > 
> > Oh, one final note: it is possible, although I haven't seen it done this way
> > yet,
> > that an SMMU could be -in- a PCIe switch (further distributing SMMU
> > functionality
> > across a large PCIe subsystem) and it -could- be a PCIe device in the switch,
> > btwn the upstream and downstream bridges -- actually doing the SMMU
> > xlations
> > at that layer..... for QEMU & IORT, it's associated with a PCIe bus.
> > But, if done correctly, that shouldn't matter -- in the example you gave wrt
> > serial,
> > it would be a new device, using common smmu core: smmuv3-pcie.
> > [Note: AMD actually identifies it's IOMMU as a PCIe device in an RC ... but
> > still uses
> >         the ACPI tables to configure it to the OS.. so the PCIe-device is basically
> > a
> >         device w/o a PCIe driver. AMD just went through hoops dealing with
> > MS
> >         and AMD-IOMMU-identification via PCIe.]
> > 
> > So, stepping back, and looking at a broad(er) SMMU -or- IOMMU QEMU
> > perspective,
> > I would think this type of format would be best:
> > 
> > - bus pcie, id=pcie.<num>
> > - device iommu=[intel_iommu|smmuv3|amd_iommu], bus=[sysbus |
> > pcie.<num>], id=iommu.<num>
> > [Yes, I'm sticking with 'iommu' as the generic naming... everyone thinks of
> > device SMMUs as IOMMUs,
> >   and QEMU should have a more arch-agnostic naming of these system
> > functions. ]
> 
> Ok. But to circle back to what originally started this discussion—how important
> is it to rely on the default "bus" in this case? As Markus pointed out, SMMUv3
> is a platform device on the sysbus, so its default bus type can’t point to something
> like PCIe. QEMU doesn’t currently support that.
> 
> The main motivation for using the default "bus" so far has been to have better
> compatibility with libvirt. Would libvirt be flexible enough if we switched to using
> something like a "primary-bus" property instead?

Sorry if my previous comments misled you, when I previously talked about
linking via a "bus" property I was not considering the fact that "bus"
is a special property inside QEMU. From a libvirt POV we don't care what
the property is call - it was just intended to be a general illustration
of cross-referencing the iommu with the PCI bus it needed to be associated
with.

> -device arm-smmuv3,primary-bus=pcie.0
> -device virtio-net-pci,bus=pcie.0
> -device pxb-pcie,id=pcie.1,bus_nr=2
> -device arm-smmuv3,primary-bus=pcie.1
> ...


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Daniel P. Berrangé <berrange@redhat.com>
> Sent: Friday, May 9, 2025 9:14 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Donald Dutile <ddutile@redhat.com>; Markus Armbruster
> <armbru@redhat.com>; Shameer Kolothum via <qemu-
> devel@nongnu.org>; qemu-arm@nongnu.org; eric.auger@redhat.com;
> peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 

[...]

> > > - bus pcie, id=pcie.<num>
> > > - device iommu=[intel_iommu|smmuv3|amd_iommu], bus=[sysbus |
> > > pcie.<num>], id=iommu.<num>
> > > [Yes, I'm sticking with 'iommu' as the generic naming... everyone thinks
> of
> > > device SMMUs as IOMMUs,
> > >   and QEMU should have a more arch-agnostic naming of these system
> > > functions. ]
> >
> > Ok. But to circle back to what originally started this discussion—how
> important
> > is it to rely on the default "bus" in this case? As Markus pointed out,
> SMMUv3
> > is a platform device on the sysbus, so its default bus type can’t point to
> something
> > like PCIe. QEMU doesn’t currently support that.
> >
> > The main motivation for using the default "bus" so far has been to have
> better
> > compatibility with libvirt. Would libvirt be flexible enough if we switched
> to using
> > something like a "primary-bus" property instead?
> 
> Sorry if my previous comments misled you, when I previously talked about
> linking via a "bus" property I was not considering the fact that "bus"
> is a special property inside QEMU. From a libvirt POV we don't care what
> the property is call - it was just intended to be a general illustration
> of cross-referencing the iommu with the PCI bus it needed to be associated
> with.

Cool. That makes life easier 😊

Thanks,
Shameer

Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Eric Auger]

On 5/9/25 10:18 AM, Shameerali Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Daniel P. Berrangé <berrange@redhat.com>
>> Sent: Friday, May 9, 2025 9:14 AM
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> Cc: Donald Dutile <ddutile@redhat.com>; Markus Armbruster
>> <armbru@redhat.com>; Shameer Kolothum via <qemu-
>> devel@nongnu.org>; qemu-arm@nongnu.org; eric.auger@redhat.com;
>> peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
>> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com; Linuxarm
>> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
>> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
>> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
>> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
>> PCIe RC
>>
> [...]
>
>>>> - bus pcie, id=pcie.<num>
>>>> - device iommu=[intel_iommu|smmuv3|amd_iommu], bus=[sysbus |
>>>> pcie.<num>], id=iommu.<num>
>>>> [Yes, I'm sticking with 'iommu' as the generic naming... everyone thinks
>> of
>>>> device SMMUs as IOMMUs,
>>>>   and QEMU should have a more arch-agnostic naming of these system
>>>> functions. ]
>>> Ok. But to circle back to what originally started this discussion—how
>> important
>>> is it to rely on the default "bus" in this case? As Markus pointed out,
>> SMMUv3
>>> is a platform device on the sysbus, so its default bus type can’t point to
>> something
>>> like PCIe. QEMU doesn’t currently support that.
>>>
>>> The main motivation for using the default "bus" so far has been to have
>> better
>>> compatibility with libvirt. Would libvirt be flexible enough if we switched
>> to using
>>> something like a "primary-bus" property instead?
>> Sorry if my previous comments misled you, when I previously talked about
>> linking via a "bus" property I was not considering the fact that "bus"
>> is a special property inside QEMU. From a libvirt POV we don't care what
>> the property is call - it was just intended to be a general illustration
>> of cross-referencing the iommu with the PCI bus it needed to be associated
>> with.
> Cool. That makes life easier 😊
Agreed ;-)

Eric


>
> Thanks,
> Shameer

[PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Shameer Kolothum via]

With the soon to be introduced user-creatable SMMUv3 devices for
virt, it is possible to have multiple SMMUv3 devices associated
with different PCIe root complexes.

Update IORT nodes accordingly.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt-acpi-build.c | 162 +++++++++++++++++++++++++++++++--------
 hw/arm/virt.c            |   1 +
 include/hw/arm/virt.h    |   1 +
 3 files changed, 131 insertions(+), 33 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3ac8f8e178..3ce70f8fa9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -43,6 +43,7 @@
 #include "hw/acpi/generic_event_device.h"
 #include "hw/acpi/tpm.h"
 #include "hw/acpi/hmat.h"
+#include "hw/arm/smmuv3.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
@@ -266,6 +267,75 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
     return idmap_a->input_base - idmap_b->input_base;
 }
 
+struct SMMUv3Device {
+    int irq;
+    hwaddr base;
+    GArray *smmu_idmaps;
+    size_t offset;
+};
+typedef struct SMMUv3Device SMMUv3Device;
+
+static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer b)
+{
+    SMMUv3Device *sdev_a = (SMMUv3Device *)a;
+    SMMUv3Device *sdev_b = (SMMUv3Device *)b;
+    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->smmu_idmaps,
+                                              AcpiIortIdMapping, 0);
+    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->smmu_idmaps,
+                                              AcpiIortIdMapping, 0);
+    return map_a->input_base - map_b->input_base;
+}
+
+static void
+get_smmuv3_legacy_dev(VirtMachineState *vms, GArray * smmuv3_devices)
+{
+    SMMUv3Device sdev;
+
+    sdev.smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
+    object_child_foreach_recursive(object_get_root(),
+                                   iort_host_bridges, sdev.smmu_idmaps);
+    sdev.base = vms->memmap[VIRT_SMMU].base;
+    sdev.irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
+    g_array_append_val(smmuv3_devices, sdev);
+}
+
+static int get_smmuv3_devices(Object *obj, void *opaque)
+{
+    PCIBus *bus;
+    SMMUv3Device sdev;
+    SysBusDevice *sbdev;
+    int min_bus, max_bus;
+    AcpiIortIdMapping idmap;
+    PlatformBusDevice *pbus;
+    GArray *sdev_blob = opaque;
+    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
+
+    if (!object_dynamic_cast(obj, TYPE_ARM_SMMUV3)) {
+        return 0;
+    }
+
+    bus = PCI_BUS(object_property_get_link(obj, "primary-bus", &error_abort));
+    if (!bus || pci_bus_bypass_iommu(bus)) {
+        return 0;
+    }
+
+    pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
+    sbdev = SYS_BUS_DEVICE(obj);
+    sdev.base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
+    sdev.base += vms->memmap[VIRT_PLATFORM_BUS].base;
+    sdev.irq = platform_bus_get_irqn(pbus, sbdev, 0);
+    sdev.irq += vms->irqmap[VIRT_PLATFORM_BUS];
+    sdev.irq += ARM_SPI_BASE;
+
+    pci_bus_range(bus, &min_bus, &max_bus);
+    sdev.smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
+    idmap.input_base = min_bus << 8,
+    idmap.id_count = (max_bus - min_bus + 1) << 8,
+    g_array_append_val(sdev.smmu_idmaps, idmap);
+    g_array_append_val(sdev_blob, sdev);
+    return 0;
+}
+
 /*
  * Input Output Remapping Table (IORT)
  * Conforms to "IO Remapping Table System Software on ARM Platforms",
@@ -274,11 +344,11 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
-    int i, nb_nodes, rc_mapping_count;
-    size_t node_size, smmu_offset = 0;
-    AcpiIortIdMapping *idmap;
+    int i, j, nb_nodes, rc_mapping_count;
+    size_t node_size;
+    int num_smmus = 0;
     uint32_t id = 0;
-    GArray *smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
+    GArray *smmuv3_devices = g_array_new(false, true, sizeof(SMMUv3Device));
     GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
 
     AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
@@ -286,28 +356,46 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     /* Table 2 The IORT */
     acpi_table_begin(&table, table_data);
 
-    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
-        AcpiIortIdMapping next_range = {0};
-
+    nb_nodes = 2; /* RC, ITS */
+    if (vms->legacy_smmuv3_present) {
+        get_smmuv3_legacy_dev(vms, smmuv3_devices);
+        /*
+         * There will be only one legacy SMMUv3 as it is a machine wide one.
+         * And since it covers all the PCIe RCs in the machine, may have
+         * multiple SMMUv3 idmaps. Sort it by input_base.
+         */
+        SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, 0);
+        g_array_sort(s->smmu_idmaps, iort_idmap_compare);
+    } else {
         object_child_foreach_recursive(object_get_root(),
-                                       iort_host_bridges, smmu_idmaps);
-
-        /* Sort the smmu idmap by input_base */
-        g_array_sort(smmu_idmaps, iort_idmap_compare);
+                                       get_smmuv3_devices, smmuv3_devices);
+        /* Sort the smmuv3 devices(if any) by smmu idmap input_base */
+        g_array_sort(smmuv3_devices, smmuv3_dev_idmap_compare);
+    }
 
+    num_smmus = smmuv3_devices->len;
+    if (num_smmus) {
+        AcpiIortIdMapping next_range = {0};
+        int smmu_map_cnt = 0;
         /*
          * Split the whole RIDs by mapping from RC to SMMU,
          * build the ID mapping from RC to ITS directly.
          */
-        for (i = 0; i < smmu_idmaps->len; i++) {
-            idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
-
-            if (next_range.input_base < idmap->input_base) {
-                next_range.id_count = idmap->input_base - next_range.input_base;
-                g_array_append_val(its_idmaps, next_range);
+        for (i = 0; i < num_smmus; i++) {
+            AcpiIortIdMapping *idmap;
+            SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, i);
+
+            for (j = 0; j < s->smmu_idmaps->len; j++) {
+                idmap = &g_array_index(s->smmu_idmaps, AcpiIortIdMapping, j);
+
+                if (next_range.input_base < idmap->input_base) {
+                    next_range.id_count = idmap->input_base -
+                                          next_range.input_base;
+                    g_array_append_val(its_idmaps, next_range);
+                }
+                next_range.input_base = idmap->input_base + idmap->id_count;
+                smmu_map_cnt++;
             }
-
-            next_range.input_base = idmap->input_base + idmap->id_count;
         }
 
         /* Append the last RC -> ITS ID mapping */
@@ -316,10 +404,9 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
             g_array_append_val(its_idmaps, next_range);
         }
 
-        nb_nodes = 3; /* RC, ITS, SMMUv3 */
-        rc_mapping_count = smmu_idmaps->len + its_idmaps->len;
+        nb_nodes += num_smmus;
+        rc_mapping_count = smmu_map_cnt + its_idmaps->len;
     } else {
-        nb_nodes = 2; /* RC, ITS */
         rc_mapping_count = 1;
     }
     /* Number of IORT Nodes */
@@ -341,10 +428,11 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     /* GIC ITS Identifier Array */
     build_append_int_noprefix(table_data, 0 /* MADT translation_id */, 4);
 
-    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
-        int irq =  vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
+    for (i = 0; i < num_smmus; i++) {
+        SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, i);
+        int irq = s->irq;
 
-        smmu_offset = table_data->len - table.table_offset;
+        s->offset = table_data->len - table.table_offset;
         /* Table 9 SMMUv3 Format */
         build_append_int_noprefix(table_data, 4 /* SMMUv3 */, 1); /* Type */
         node_size =  SMMU_V3_ENTRY_SIZE + ID_MAPPING_ENTRY_SIZE;
@@ -355,7 +443,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         /* Reference to ID Array */
         build_append_int_noprefix(table_data, SMMU_V3_ENTRY_SIZE, 4);
         /* Base address */
-        build_append_int_noprefix(table_data, vms->memmap[VIRT_SMMU].base, 8);
+        build_append_int_noprefix(table_data, s->base, 8);
         /* Flags */
         build_append_int_noprefix(table_data, 1 /* COHACC Override */, 4);
         build_append_int_noprefix(table_data, 0, 4); /* Reserved */
@@ -404,15 +492,19 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     build_append_int_noprefix(table_data, 0, 3); /* Reserved */
 
     /* Output Reference */
-    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
+    if (num_smmus) {
         AcpiIortIdMapping *range;
 
         /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS */
-        for (i = 0; i < smmu_idmaps->len; i++) {
-            range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
-            /* output IORT node is the smmuv3 node */
-            build_iort_id_mapping(table_data, range->input_base,
-                                  range->id_count, smmu_offset);
+        for (i = 0; i < num_smmus; i++) {
+            SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, i);
+
+            for (j = 0; j < s->smmu_idmaps->len; j++) {
+                range = &g_array_index(s->smmu_idmaps, AcpiIortIdMapping, j);
+                /* output IORT node is the smmuv3 node */
+                build_iort_id_mapping(table_data, range->input_base,
+                                      range->id_count, s->offset);
+            }
         }
 
         /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
@@ -428,8 +520,12 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     }
 
     acpi_table_end(linker, &table);
-    g_array_free(smmu_idmaps, true);
     g_array_free(its_idmaps, true);
+    for (i = 0; i < num_smmus; i++) {
+        SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, i);
+        g_array_free(s->smmu_idmaps, true);
+    }
+    g_array_free(smmuv3_devices, true);
 }
 
 /*
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3bae4e374f..dd355f4454 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1620,6 +1620,7 @@ static void create_pcie(VirtMachineState *vms)
             create_smmu(vms, vms->bus);
             qemu_fdt_setprop_cells(ms->fdt, nodename, "iommu-map",
                                    0x0, vms->iommu_phandle, 0x0, 0x10000);
+            vms->legacy_smmuv3_present = true;
             break;
         default:
             g_assert_not_reached();
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index c8e94e6aed..2e2867c906 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -180,6 +180,7 @@ struct VirtMachineState {
     char *oem_id;
     char *oem_table_id;
     bool ns_el2_virt_timer_irq;
+    bool legacy_smmuv3_present;
 };
 
 #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
-- 
2.34.1

Re: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Nicolin Chen]

On Fri, May 02, 2025 at 11:27:03AM +0100, Shameer Kolothum wrote:
> @@ -43,6 +43,7 @@
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/acpi/tpm.h"
>  #include "hw/acpi/hmat.h"
> +#include "hw/arm/smmuv3.h"
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bus.h"
> @@ -266,6 +267,75 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
>      return idmap_a->input_base - idmap_b->input_base;
>  }
>  
> +struct SMMUv3Device {
> +    int irq;
> +    hwaddr base;
> +    GArray *smmu_idmaps;
> +    size_t offset;
> +};
> +typedef struct SMMUv3Device SMMUv3Device;

"SMMUv3Device" sounds too general, like coming from smmuv3.h :-/

Given this describes SMMUv3's IORT node, I still think we should
name it something like "IortSMMUv3Node" or so.

> +static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer b)
> +{
> +    SMMUv3Device *sdev_a = (SMMUv3Device *)a;
> +    SMMUv3Device *sdev_b = (SMMUv3Device *)b;
> +    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->smmu_idmaps,
> +                                              AcpiIortIdMapping, 0);
> +    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->smmu_idmaps,
> +                                              AcpiIortIdMapping, 0);
> +    return map_a->input_base - map_b->input_base;
> +}
> +
> +static void
> +get_smmuv3_legacy_dev(VirtMachineState *vms, GArray * smmuv3_devices)

GArray *smmuv3_devices

Or maybe align with the non-legacy path, i.e. "sdev_blob"? Or the
other way around.

Otherwise, lgtm
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Re: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Donald Dutile]

On 5/2/25 1:13 PM, Nicolin Chen wrote:
> On Fri, May 02, 2025 at 11:27:03AM +0100, Shameer Kolothum wrote:
>> @@ -43,6 +43,7 @@
>>   #include "hw/acpi/generic_event_device.h"
>>   #include "hw/acpi/tpm.h"
>>   #include "hw/acpi/hmat.h"
>> +#include "hw/arm/smmuv3.h"
>>   #include "hw/pci/pcie_host.h"
>>   #include "hw/pci/pci.h"
>>   #include "hw/pci/pci_bus.h"
>> @@ -266,6 +267,75 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
>>       return idmap_a->input_base - idmap_b->input_base;
>>   }
>>   
>> +struct SMMUv3Device {
>> +    int irq;
>> +    hwaddr base;
>> +    GArray *smmu_idmaps;
>> +    size_t offset;
>> +};
>> +typedef struct SMMUv3Device SMMUv3Device;
> 
> "SMMUv3Device" sounds too general, like coming from smmuv3.h :-/
> 
> Given this describes SMMUv3's IORT node, I still think we should
> name it something like "IortSMMUv3Node" or so.
> 
+1.. the more generic name had me thinking it was broader than IORT..
the IORT-related naming is an improvement.

>> +static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer b)
>> +{
>> +    SMMUv3Device *sdev_a = (SMMUv3Device *)a;
>> +    SMMUv3Device *sdev_b = (SMMUv3Device *)b;
>> +    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->smmu_idmaps,
>> +                                              AcpiIortIdMapping, 0);
>> +    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->smmu_idmaps,
>> +                                              AcpiIortIdMapping, 0);
>> +    return map_a->input_base - map_b->input_base;
>> +}
>> +
>> +static void
>> +get_smmuv3_legacy_dev(VirtMachineState *vms, GArray * smmuv3_devices)
> 
> GArray *smmuv3_devices
> 
> Or maybe align with the non-legacy path, i.e. "sdev_blob"? Or the
> other way around.
> 
> Otherwise, lgtm
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 

RE: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Donald Dutile <ddutile@redhat.com>
> Sent: Friday, May 2, 2025 7:18 PM
> To: Nicolin Chen <nicolinc@nvidia.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> berrange@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B)
> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>;
> zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for
> multiple smmuv3 devices
> 
> 
> 
> On 5/2/25 1:13 PM, Nicolin Chen wrote:
> > On Fri, May 02, 2025 at 11:27:03AM +0100, Shameer Kolothum wrote:
> >> @@ -43,6 +43,7 @@
> >>   #include "hw/acpi/generic_event_device.h"
> >>   #include "hw/acpi/tpm.h"
> >>   #include "hw/acpi/hmat.h"
> >> +#include "hw/arm/smmuv3.h"
> >>   #include "hw/pci/pcie_host.h"
> >>   #include "hw/pci/pci.h"
> >>   #include "hw/pci/pci_bus.h"
> >> @@ -266,6 +267,75 @@ static int iort_idmap_compare(gconstpointer a,
> gconstpointer b)
> >>       return idmap_a->input_base - idmap_b->input_base;
> >>   }
> >>
> >> +struct SMMUv3Device {
> >> +    int irq;
> >> +    hwaddr base;
> >> +    GArray *smmu_idmaps;
> >> +    size_t offset;
> >> +};
> >> +typedef struct SMMUv3Device SMMUv3Device;
> >
> > "SMMUv3Device" sounds too general, like coming from smmuv3.h :-/
> >
> > Given this describes SMMUv3's IORT node, I still think we should name
> > it something like "IortSMMUv3Node" or so.
> >
> +1.. the more generic name had me thinking it was broader than IORT..
> the IORT-related naming is an improvement.

I thought it is obvious as I already replied to Nicolin 😊. I will consider this.

Thanks,
Shameer

RE: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, May 2, 2025 6:14 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for
> multiple smmuv3 devices
> 
> On Fri, May 02, 2025 at 11:27:03AM +0100, Shameer Kolothum wrote:
> > @@ -43,6 +43,7 @@
> >  #include "hw/acpi/generic_event_device.h"
> >  #include "hw/acpi/tpm.h"
> >  #include "hw/acpi/hmat.h"
> > +#include "hw/arm/smmuv3.h"
> >  #include "hw/pci/pcie_host.h"
> >  #include "hw/pci/pci.h"
> >  #include "hw/pci/pci_bus.h"
> > @@ -266,6 +267,75 @@ static int iort_idmap_compare(gconstpointer a,
> gconstpointer b)
> >      return idmap_a->input_base - idmap_b->input_base;
> >  }
> >
> > +struct SMMUv3Device {
> > +    int irq;
> > +    hwaddr base;
> > +    GArray *smmu_idmaps;
> > +    size_t offset;
> > +};
> > +typedef struct SMMUv3Device SMMUv3Device;
> 
> "SMMUv3Device" sounds too general, like coming from smmuv3.h :-/
> 
> Given this describes SMMUv3's IORT node, I still think we should
> name it something like "IortSMMUv3Node" or so.

The way I thought about it is, it is mostly a structure to hold the SMMUv3 device
information, that will be used in IORT node creation. We are indeed going through the 
SMMUv3 devices created by virt and populating this. IORT is nothing but ACPI way for
describing various devices like PCIE RC, SMMUv3 etc. I thought it is very clear from code
that it is used in IORT and an explicit IORT in the name makes any difference here 😊

Having said that I don’t have a strong bias towards this. I will consider it when I respin.

> 
> > +static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer
> b)
> > +{
> > +    SMMUv3Device *sdev_a = (SMMUv3Device *)a;
> > +    SMMUv3Device *sdev_b = (SMMUv3Device *)b;
> > +    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->smmu_idmaps,
> > +                                              AcpiIortIdMapping, 0);
> > +    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->smmu_idmaps,
> > +                                              AcpiIortIdMapping, 0);
> > +    return map_a->input_base - map_b->input_base;
> > +}
> > +
> > +static void
> > +get_smmuv3_legacy_dev(VirtMachineState *vms, GArray *
> smmuv3_devices)
> 
> GArray *smmuv3_devices
> 
> Or maybe align with the non-legacy path, i.e. "sdev_blob"? Or the
> other way around.

Sure. I think void *sdev_blob for both is good.
 
> Otherwise, lgtm
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Thanks,
Shameer

Re: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Eric Auger]

Hi Shameer,

On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> With the soon to be introduced user-creatable SMMUv3 devices for
> virt, it is possible to have multiple SMMUv3 devices associated
> with different PCIe root complexes.
>
> Update IORT nodes accordingly.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 162 +++++++++++++++++++++++++++++++--------
I would recommend to split that patch. I think you can first introduce
the new struct and adapting the exicting code for the "legacy
instantiation mode" and then have a saparet patch for handling user
created instances. It will be easier to review.

At some point in the series you shall check the user is not attempting
to use legacy mode and user creatable mode concurrently. I don't know if
it is done yet.
>  hw/arm/virt.c            |   1 +
>  include/hw/arm/virt.h    |   1 +
>  3 files changed, 131 insertions(+), 33 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 3ac8f8e178..3ce70f8fa9 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -43,6 +43,7 @@
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/acpi/tpm.h"
>  #include "hw/acpi/hmat.h"
> +#include "hw/arm/smmuv3.h"
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bus.h"
> @@ -266,6 +267,75 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
>      return idmap_a->input_base - idmap_b->input_base;
>  }
>  
> +struct SMMUv3Device {
> +    int irq;
> +    hwaddr base;
> +    GArray *smmu_idmaps;
> +    size_t offset;
> +};
> +typedef struct SMMUv3Device SMMUv3Device;
> +
> +static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer b)
> +{
> +    SMMUv3Device *sdev_a = (SMMUv3Device *)a;
> +    SMMUv3Device *sdev_b = (SMMUv3Device *)b;
> +    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->smmu_idmaps,
> +                                              AcpiIortIdMapping, 0);
> +    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->smmu_idmaps,
> +                                              AcpiIortIdMapping, 0);
> +    return map_a->input_base - map_b->input_base;
> +}
> +
> +static void
> +get_smmuv3_legacy_dev(VirtMachineState *vms, GArray * smmuv3_devices)
> +{
> +    SMMUv3Device sdev;
> +
> +    sdev.smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> +    object_child_foreach_recursive(object_get_root(),
> +                                   iort_host_bridges, sdev.smmu_idmaps);
> +    sdev.base = vms->memmap[VIRT_SMMU].base;
> +    sdev.irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> +    g_array_append_val(smmuv3_devices, sdev);
> +}
> +
> +static int get_smmuv3_devices(Object *obj, void *opaque)
> +{
> +    PCIBus *bus;
> +    SMMUv3Device sdev;
> +    SysBusDevice *sbdev;
> +    int min_bus, max_bus;
> +    AcpiIortIdMapping idmap;
> +    PlatformBusDevice *pbus;
> +    GArray *sdev_blob = opaque;
> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> +
> +    if (!object_dynamic_cast(obj, TYPE_ARM_SMMUV3)) {
> +        return 0;
> +    }
> +
> +    bus = PCI_BUS(object_property_get_link(obj, "primary-bus", &error_abort));
> +    if (!bus || pci_bus_bypass_iommu(bus)) {
can't we simply prevent a user creatable SMMU to be attached to

pci_bus_bypass_iommu(bus) ?

> +        return 0;
> +    }
> +
> +    pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> +    sbdev = SYS_BUS_DEVICE(obj);
> +    sdev.base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> +    sdev.base += vms->memmap[VIRT_PLATFORM_BUS].base;
> +    sdev.irq = platform_bus_get_irqn(pbus, sbdev, 0);
> +    sdev.irq += vms->irqmap[VIRT_PLATFORM_BUS];
> +    sdev.irq += ARM_SPI_BASE;
> +
> +    pci_bus_range(bus, &min_bus, &max_bus);
> +    sdev.smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> +    idmap.input_base = min_bus << 8,
> +    idmap.id_count = (max_bus - min_bus + 1) << 8,
> +    g_array_append_val(sdev.smmu_idmaps, idmap);
> +    g_array_append_val(sdev_blob, sdev);
> +    return 0;
> +}
> +
>  /*
>   * Input Output Remapping Table (IORT)
>   * Conforms to "IO Remapping Table System Software on ARM Platforms",
> @@ -274,11 +344,11 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
>  static void
>  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> -    int i, nb_nodes, rc_mapping_count;
> -    size_t node_size, smmu_offset = 0;
> -    AcpiIortIdMapping *idmap;
> +    int i, j, nb_nodes, rc_mapping_count;
> +    size_t node_size;
> +    int num_smmus = 0;
>      uint32_t id = 0;
> -    GArray *smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> +    GArray *smmuv3_devices = g_array_new(false, true, sizeof(SMMUv3Device));
>      GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
>  
>      AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
> @@ -286,28 +356,46 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      /* Table 2 The IORT */
>      acpi_table_begin(&table, table_data);
>  
> -    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> -        AcpiIortIdMapping next_range = {0};
> -
> +    nb_nodes = 2; /* RC, ITS */
> +    if (vms->legacy_smmuv3_present) {
> +        get_smmuv3_legacy_dev(vms, smmuv3_devices);
> +        /*
> +         * There will be only one legacy SMMUv3 as it is a machine wide one.
> +         * And since it covers all the PCIe RCs in the machine, may have
> +         * multiple SMMUv3 idmaps. Sort it by input_base.
> +         */
> +        SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, 0);
> +        g_array_sort(s->smmu_idmaps, iort_idmap_compare);
> +    } else {
>          object_child_foreach_recursive(object_get_root(),
> -                                       iort_host_bridges, smmu_idmaps);
> -
> -        /* Sort the smmu idmap by input_base */
> -        g_array_sort(smmu_idmaps, iort_idmap_compare);
> +                                       get_smmuv3_devices, smmuv3_devices);
> +        /* Sort the smmuv3 devices(if any) by smmu idmap input_base */
> +        g_array_sort(smmuv3_devices, smmuv3_dev_idmap_compare);
> +    }
>  
> +    num_smmus = smmuv3_devices->len;
> +    if (num_smmus) {
> +        AcpiIortIdMapping next_range = {0};
> +        int smmu_map_cnt = 0;
>          /*
>           * Split the whole RIDs by mapping from RC to SMMU,
>           * build the ID mapping from RC to ITS directly.
>           */
> -        for (i = 0; i < smmu_idmaps->len; i++) {
> -            idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> -
> -            if (next_range.input_base < idmap->input_base) {
> -                next_range.id_count = idmap->input_base - next_range.input_base;
> -                g_array_append_val(its_idmaps, next_range);
> +        for (i = 0; i < num_smmus; i++) {
> +            AcpiIortIdMapping *idmap;
extra line needed
> +            SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, i);
> +
> +            for (j = 0; j < s->smmu_idmaps->len; j++) {
> +                idmap = &g_array_index(s->smmu_idmaps, AcpiIortIdMapping, j);
> +
> +                if (next_range.input_base < idmap->input_base) {
> +                    next_range.id_count = idmap->input_base -
> +                                          next_range.input_base;
> +                    g_array_append_val(its_idmaps, next_range);
> +                }
> +                next_range.input_base = idmap->input_base + idmap->id_count;
> +                smmu_map_cnt++;
>              }
> -
> -            next_range.input_base = idmap->input_base + idmap->id_count;
>          }
>  
>          /* Append the last RC -> ITS ID mapping */
> @@ -316,10 +404,9 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>              g_array_append_val(its_idmaps, next_range);
>          }
>  
> -        nb_nodes = 3; /* RC, ITS, SMMUv3 */
> -        rc_mapping_count = smmu_idmaps->len + its_idmaps->len;
> +        nb_nodes += num_smmus;
> +        rc_mapping_count = smmu_map_cnt + its_idmaps->len;
>      } else {
> -        nb_nodes = 2; /* RC, ITS */
>          rc_mapping_count = 1;
>      }
>      /* Number of IORT Nodes */
> @@ -341,10 +428,11 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      /* GIC ITS Identifier Array */
>      build_append_int_noprefix(table_data, 0 /* MADT translation_id */, 4);
>  
> -    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> -        int irq =  vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> +    for (i = 0; i < num_smmus; i++) {
> +        SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, i);
> +        int irq = s->irq;
>  
> -        smmu_offset = table_data->len - table.table_offset;
> +        s->offset = table_data->len - table.table_offset;
>          /* Table 9 SMMUv3 Format */
>          build_append_int_noprefix(table_data, 4 /* SMMUv3 */, 1); /* Type */
>          node_size =  SMMU_V3_ENTRY_SIZE + ID_MAPPING_ENTRY_SIZE;
> @@ -355,7 +443,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          /* Reference to ID Array */
>          build_append_int_noprefix(table_data, SMMU_V3_ENTRY_SIZE, 4);
>          /* Base address */
> -        build_append_int_noprefix(table_data, vms->memmap[VIRT_SMMU].base, 8);
> +        build_append_int_noprefix(table_data, s->base, 8);
>          /* Flags */
>          build_append_int_noprefix(table_data, 1 /* COHACC Override */, 4);
>          build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> @@ -404,15 +492,19 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      build_append_int_noprefix(table_data, 0, 3); /* Reserved */
>  
>      /* Output Reference */
> -    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> +    if (num_smmus) {
>          AcpiIortIdMapping *range;
>  
>          /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS */
> -        for (i = 0; i < smmu_idmaps->len; i++) {
> -            range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> -            /* output IORT node is the smmuv3 node */
> -            build_iort_id_mapping(table_data, range->input_base,
> -                                  range->id_count, smmu_offset);
> +        for (i = 0; i < num_smmus; i++) {
> +            SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, i);
> +
> +            for (j = 0; j < s->smmu_idmaps->len; j++) {
> +                range = &g_array_index(s->smmu_idmaps, AcpiIortIdMapping, j);
> +                /* output IORT node is the smmuv3 node */
> +                build_iort_id_mapping(table_data, range->input_base,
> +                                      range->id_count, s->offset);
> +            }
>          }
>  
>          /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
> @@ -428,8 +520,12 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      }
>  
>      acpi_table_end(linker, &table);
> -    g_array_free(smmu_idmaps, true);
>      g_array_free(its_idmaps, true);
> +    for (i = 0; i < num_smmus; i++) {
> +        SMMUv3Device *s = &g_array_index(smmuv3_devices, SMMUv3Device, i);
> +        g_array_free(s->smmu_idmaps, true);
> +    }
> +    g_array_free(smmuv3_devices, true);
>  }
>  
>  /*
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 3bae4e374f..dd355f4454 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1620,6 +1620,7 @@ static void create_pcie(VirtMachineState *vms)
>              create_smmu(vms, vms->bus);
>              qemu_fdt_setprop_cells(ms->fdt, nodename, "iommu-map",
>                                     0x0, vms->iommu_phandle, 0x0, 0x10000);
> +            vms->legacy_smmuv3_present = true;
>              break;
>          default:
>              g_assert_not_reached();
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index c8e94e6aed..2e2867c906 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -180,6 +180,7 @@ struct VirtMachineState {
>      char *oem_id;
>      char *oem_table_id;
>      bool ns_el2_virt_timer_irq;
> +    bool legacy_smmuv3_present;
>  };
>  
>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
Eric

RE: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: qemu-devel-
> bounces+shameerali.kolothum.thodi=huawei.com@nongnu.org <qemu-
> devel-bounces+shameerali.kolothum.thodi=huawei.com@nongnu.org> On
> Behalf Of Eric Auger
> Sent: Monday, May 5, 2025 9:40 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for
> multiple smmuv3 devices
> 
> Hi Shameer,
> 
> On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> > With the soon to be introduced user-creatable SMMUv3 devices for
> > virt, it is possible to have multiple SMMUv3 devices associated
> > with different PCIe root complexes.
> >
> > Update IORT nodes accordingly.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/virt-acpi-build.c | 162 +++++++++++++++++++++++++++++++--------
> I would recommend to split that patch. I think you can first introduce
> the new struct and adapting the exicting code for the "legacy
> instantiation mode" and then have a saparet patch for handling user
> created instances. It will be easier to review.

Ok. I will split this patch.

> 
> At some point in the series you shall check the user is not attempting
> to use legacy mode and user creatable mode concurrently. I don't know if
> it is done yet.

Yes, it is done in patch #5. See changes to virt_machine_device_pre_plug_cb().


> >  hw/arm/virt.c            |   1 +
> >  include/hw/arm/virt.h    |   1 +
> >  3 files changed, 131 insertions(+), 33 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 3ac8f8e178..3ce70f8fa9 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -43,6 +43,7 @@
> >  #include "hw/acpi/generic_event_device.h"
> >  #include "hw/acpi/tpm.h"
> >  #include "hw/acpi/hmat.h"
> > +#include "hw/arm/smmuv3.h"
> >  #include "hw/pci/pcie_host.h"
> >  #include "hw/pci/pci.h"
> >  #include "hw/pci/pci_bus.h"
> > @@ -266,6 +267,75 @@ static int iort_idmap_compare(gconstpointer a,
> gconstpointer b)
> >      return idmap_a->input_base - idmap_b->input_base;
> >  }
> >
> > +struct SMMUv3Device {
> > +    int irq;
> > +    hwaddr base;
> > +    GArray *smmu_idmaps;
> > +    size_t offset;
> > +};
> > +typedef struct SMMUv3Device SMMUv3Device;
> > +
> > +static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer
> b)
> > +{
> > +    SMMUv3Device *sdev_a = (SMMUv3Device *)a;
> > +    SMMUv3Device *sdev_b = (SMMUv3Device *)b;
> > +    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->smmu_idmaps,
> > +                                              AcpiIortIdMapping, 0);
> > +    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->smmu_idmaps,
> > +                                              AcpiIortIdMapping, 0);
> > +    return map_a->input_base - map_b->input_base;
> > +}
> > +
> > +static void
> > +get_smmuv3_legacy_dev(VirtMachineState *vms, GArray *
> smmuv3_devices)
> > +{
> > +    SMMUv3Device sdev;
> > +
> > +    sdev.smmu_idmaps = g_array_new(false, true,
> sizeof(AcpiIortIdMapping));
> > +    object_child_foreach_recursive(object_get_root(),
> > +                                   iort_host_bridges, sdev.smmu_idmaps);
> > +    sdev.base = vms->memmap[VIRT_SMMU].base;
> > +    sdev.irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> > +    g_array_append_val(smmuv3_devices, sdev);
> > +}
> > +
> > +static int get_smmuv3_devices(Object *obj, void *opaque)
> > +{
> > +    PCIBus *bus;
> > +    SMMUv3Device sdev;
> > +    SysBusDevice *sbdev;
> > +    int min_bus, max_bus;
> > +    AcpiIortIdMapping idmap;
> > +    PlatformBusDevice *pbus;
> > +    GArray *sdev_blob = opaque;
> > +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> > +
> > +    if (!object_dynamic_cast(obj, TYPE_ARM_SMMUV3)) {
> > +        return 0;
> > +    }
> > +
> > +    bus = PCI_BUS(object_property_get_link(obj, "primary-bus",
> &error_abort));
> > +    if (!bus || pci_bus_bypass_iommu(bus)) {
> can't we simply prevent a user creatable SMMU to be attached to
> 
> pci_bus_bypass_iommu(bus) ?

Ok. I think that can be done.

> 
> > +        return 0;
> > +    }
> > +
> > +    pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> > +    sbdev = SYS_BUS_DEVICE(obj);
> > +    sdev.base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> > +    sdev.base += vms->memmap[VIRT_PLATFORM_BUS].base;
> > +    sdev.irq = platform_bus_get_irqn(pbus, sbdev, 0);
> > +    sdev.irq += vms->irqmap[VIRT_PLATFORM_BUS];
> > +    sdev.irq += ARM_SPI_BASE;
> > +
> > +    pci_bus_range(bus, &min_bus, &max_bus);
> > +    sdev.smmu_idmaps = g_array_new(false, true,
> sizeof(AcpiIortIdMapping));
> > +    idmap.input_base = min_bus << 8,
> > +    idmap.id_count = (max_bus - min_bus + 1) << 8,
> > +    g_array_append_val(sdev.smmu_idmaps, idmap);
> > +    g_array_append_val(sdev_blob, sdev);
> > +    return 0;
> > +}
> > +
> >  /*
> >   * Input Output Remapping Table (IORT)
> >   * Conforms to "IO Remapping Table System Software on ARM
> Platforms",
> > @@ -274,11 +344,11 @@ static int iort_idmap_compare(gconstpointer a,
> gconstpointer b)
> >  static void
> >  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState
> *vms)
> >  {
> > -    int i, nb_nodes, rc_mapping_count;
> > -    size_t node_size, smmu_offset = 0;
> > -    AcpiIortIdMapping *idmap;
> > +    int i, j, nb_nodes, rc_mapping_count;
> > +    size_t node_size;
> > +    int num_smmus = 0;
> >      uint32_t id = 0;
> > -    GArray *smmu_idmaps = g_array_new(false, true,
> sizeof(AcpiIortIdMapping));
> > +    GArray *smmuv3_devices = g_array_new(false, true,
> sizeof(SMMUv3Device));
> >      GArray *its_idmaps = g_array_new(false, true,
> sizeof(AcpiIortIdMapping));
> >
> >      AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
> > @@ -286,28 +356,46 @@ build_iort(GArray *table_data, BIOSLinker
> *linker, VirtMachineState *vms)
> >      /* Table 2 The IORT */
> >      acpi_table_begin(&table, table_data);
> >
> > -    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> > -        AcpiIortIdMapping next_range = {0};
> > -
> > +    nb_nodes = 2; /* RC, ITS */
> > +    if (vms->legacy_smmuv3_present) {
> > +        get_smmuv3_legacy_dev(vms, smmuv3_devices);
> > +        /*
> > +         * There will be only one legacy SMMUv3 as it is a machine wide
> one.
> > +         * And since it covers all the PCIe RCs in the machine, may have
> > +         * multiple SMMUv3 idmaps. Sort it by input_base.
> > +         */
> > +        SMMUv3Device *s = &g_array_index(smmuv3_devices,
> SMMUv3Device, 0);
> > +        g_array_sort(s->smmu_idmaps, iort_idmap_compare);
> > +    } else {
> >          object_child_foreach_recursive(object_get_root(),
> > -                                       iort_host_bridges, smmu_idmaps);
> > -
> > -        /* Sort the smmu idmap by input_base */
> > -        g_array_sort(smmu_idmaps, iort_idmap_compare);
> > +                                       get_smmuv3_devices, smmuv3_devices);
> > +        /* Sort the smmuv3 devices(if any) by smmu idmap input_base */
> > +        g_array_sort(smmuv3_devices, smmuv3_dev_idmap_compare);
> > +    }
> >
> > +    num_smmus = smmuv3_devices->len;
> > +    if (num_smmus) {
> > +        AcpiIortIdMapping next_range = {0};
> > +        int smmu_map_cnt = 0;
> >          /*
> >           * Split the whole RIDs by mapping from RC to SMMU,
> >           * build the ID mapping from RC to ITS directly.
> >           */
> > -        for (i = 0; i < smmu_idmaps->len; i++) {
> > -            idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> > -
> > -            if (next_range.input_base < idmap->input_base) {
> > -                next_range.id_count = idmap->input_base -
> next_range.input_base;
> > -                g_array_append_val(its_idmaps, next_range);
> > +        for (i = 0; i < num_smmus; i++) {
> > +            AcpiIortIdMapping *idmap;
> extra line needed

Sure. I wonder why the checkpatch --strict doesn't pick these things.

Thanks,
Shameer

[PATCH v2 3/6] hw/arm/virt: Factor out common SMMUV3 dt bindings code

[Shameer Kolothum via]

No functional changes intended. This will be useful when we
add support for user-creatable smmuv3 device.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt.c | 54 +++++++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index dd355f4454..464e84ae67 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1418,19 +1418,43 @@ static void create_pcie_irq_map(const MachineState *ms,
                            0x7           /* PCI irq */);
 }
 
+static void create_smmuv3_dt_bindings(const VirtMachineState *vms, hwaddr base,
+                                      hwaddr size, int irq)
+{
+    char *node;
+    const char compat[] = "arm,smmu-v3";
+    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
+    MachineState *ms = MACHINE(vms);
+
+    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
+    qemu_fdt_add_subnode(ms->fdt, node);
+    qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
+
+    qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
+            GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
+            GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
+            GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
+            GIC_FDT_IRQ_TYPE_SPI, irq + 3, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
+
+    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
+                     sizeof(irq_names));
+
+    qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
+    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
+    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms->iommu_phandle);
+    g_free(node);
+}
+
 static void create_smmu(const VirtMachineState *vms,
                         PCIBus *bus)
 {
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
-    char *node;
-    const char compat[] = "arm,smmu-v3";
     int irq =  vms->irqmap[VIRT_SMMU];
     int i;
     hwaddr base = vms->memmap[VIRT_SMMU].base;
     hwaddr size = vms->memmap[VIRT_SMMU].size;
-    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
     DeviceState *dev;
-    MachineState *ms = MACHINE(vms);
 
     if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) {
         return;
@@ -1449,27 +1473,7 @@ static void create_smmu(const VirtMachineState *vms,
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
                            qdev_get_gpio_in(vms->gic, irq + i));
     }
-
-    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
-    qemu_fdt_add_subnode(ms->fdt, node);
-    qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
-    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
-
-    qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
-            GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
-            GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
-            GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
-            GIC_FDT_IRQ_TYPE_SPI, irq + 3, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
-
-    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
-                     sizeof(irq_names));
-
-    qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
-
-    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
-
-    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms->iommu_phandle);
-    g_free(node);
+    create_smmuv3_dt_bindings(vms, base, size, irq);
 }
 
 static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
-- 
2.34.1

Re: [PATCH v2 3/6] hw/arm/virt: Factor out common SMMUV3 dt bindings code

[Nicolin Chen]

On Fri, May 02, 2025 at 11:27:04AM +0100, Shameer Kolothum wrote:
> No functional changes intended. This will be useful when we
> add support for user-creatable smmuv3 device.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Re: [PATCH v2 3/6] hw/arm/virt: Factor out common SMMUV3 dt bindings code

[Eric Auger]

Hi Shameer,

On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> No functional changes intended. This will be useful when we
> add support for user-creatable smmuv3 device.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/virt.c | 54 +++++++++++++++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index dd355f4454..464e84ae67 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1418,19 +1418,43 @@ static void create_pcie_irq_map(const MachineState *ms,
>                             0x7           /* PCI irq */);
>  }
>  
> +static void create_smmuv3_dt_bindings(const VirtMachineState *vms, hwaddr base,
> +                                      hwaddr size, int irq)
> +{
> +    char *node;
> +    const char compat[] = "arm,smmu-v3";
> +    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
> +    MachineState *ms = MACHINE(vms);
> +
> +    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
> +    qemu_fdt_add_subnode(ms->fdt, node);
> +    qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
> +    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
> +
> +    qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
> +            GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> +            GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> +            GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> +            GIC_FDT_IRQ_TYPE_SPI, irq + 3, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> +
> +    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
> +                     sizeof(irq_names));
> +
> +    qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
> +    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
> +    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms->iommu_phandle);
> +    g_free(node);
> +}
> +
>  static void create_smmu(const VirtMachineState *vms,
>                          PCIBus *bus)
>  {
>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> -    char *node;
> -    const char compat[] = "arm,smmu-v3";
>      int irq =  vms->irqmap[VIRT_SMMU];
>      int i;
>      hwaddr base = vms->memmap[VIRT_SMMU].base;
>      hwaddr size = vms->memmap[VIRT_SMMU].size;
> -    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
>      DeviceState *dev;
> -    MachineState *ms = MACHINE(vms);
>  
>      if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) {
>          return;
> @@ -1449,27 +1473,7 @@ static void create_smmu(const VirtMachineState *vms,
>          sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
>                             qdev_get_gpio_in(vms->gic, irq + i));
>      }
> -
> -    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
> -    qemu_fdt_add_subnode(ms->fdt, node);
> -    qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
> -    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
> -
> -    qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
> -            GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> -            GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> -            GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> -            GIC_FDT_IRQ_TYPE_SPI, irq + 3, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> -
> -    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
> -                     sizeof(irq_names));
> -
> -    qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
> -
> -    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
> -
> -    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms->iommu_phandle);
> -    g_free(node);
> +    create_smmuv3_dt_bindings(vms, base, size, irq);
>  }
>  
>  static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
Reviewed-by: Eric Auger <eric.auger@redhat.com>

nothing to do with that patch but I just noticed we omitted to support the

bypass_iommu=true along with DT mode. I don't see the iommu-map property set accordingly.

Something to further consolidate?

Eric


Eric

RE: [PATCH v2 3/6] hw/arm/virt: Factor out common SMMUV3 dt bindings code

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Monday, May 5, 2025 10:02 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 3/6] hw/arm/virt: Factor out common SMMUV3 dt
> bindings code
> 
> Hi Shameer,
> 
> On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> > No functional changes intended. This will be useful when we
> > add support for user-creatable smmuv3 device.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/virt.c | 54 +++++++++++++++++++++++++++------------------------
> >  1 file changed, 29 insertions(+), 25 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index dd355f4454..464e84ae67 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1418,19 +1418,43 @@ static void create_pcie_irq_map(const
> MachineState *ms,
> >                             0x7           /* PCI irq */);
> >  }
> >
> > +static void create_smmuv3_dt_bindings(const VirtMachineState *vms,
> hwaddr base,
> > +                                      hwaddr size, int irq)
> > +{
> > +    char *node;
> > +    const char compat[] = "arm,smmu-v3";
> > +    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
> > +    MachineState *ms = MACHINE(vms);
> > +
> > +    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
> > +    qemu_fdt_add_subnode(ms->fdt, node);
> > +    qemu_fdt_setprop(ms->fdt, node, "compatible", compat,
> sizeof(compat));
> > +    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
> > +
> > +    qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
> > +            GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > +            GIC_FDT_IRQ_TYPE_SPI, irq + 1,
> GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > +            GIC_FDT_IRQ_TYPE_SPI, irq + 2,
> GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > +            GIC_FDT_IRQ_TYPE_SPI, irq + 3,
> GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> > +
> > +    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
> > +                     sizeof(irq_names));
> > +
> > +    qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
> > +    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
> > +    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms-
> >iommu_phandle);
> > +    g_free(node);
> > +}
> > +
> >  static void create_smmu(const VirtMachineState *vms,
> >                          PCIBus *bus)
> >  {
> >      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > -    char *node;
> > -    const char compat[] = "arm,smmu-v3";
> >      int irq =  vms->irqmap[VIRT_SMMU];
> >      int i;
> >      hwaddr base = vms->memmap[VIRT_SMMU].base;
> >      hwaddr size = vms->memmap[VIRT_SMMU].size;
> > -    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
> >      DeviceState *dev;
> > -    MachineState *ms = MACHINE(vms);
> >
> >      if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) {
> >          return;
> > @@ -1449,27 +1473,7 @@ static void create_smmu(const
> VirtMachineState *vms,
> >          sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
> >                             qdev_get_gpio_in(vms->gic, irq + i));
> >      }
> > -
> > -    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
> > -    qemu_fdt_add_subnode(ms->fdt, node);
> > -    qemu_fdt_setprop(ms->fdt, node, "compatible", compat,
> sizeof(compat));
> > -    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
> > -
> > -    qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
> > -            GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > -            GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > -            GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
> > -            GIC_FDT_IRQ_TYPE_SPI, irq + 3,
> GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> > -
> > -    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
> > -                     sizeof(irq_names));
> > -
> > -    qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
> > -
> > -    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
> > -
> > -    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms-
> >iommu_phandle);
> > -    g_free(node);
> > +    create_smmuv3_dt_bindings(vms, base, size, irq);
> >  }
> >
> >  static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> nothing to do with that patch but I just noticed we omitted to support the
> 
> bypass_iommu=true along with DT mode. I don't see the iommu-map
> property set accordingly.
> 
> Something to further consolidate?

Yes. It looks like currently the virt SMMUv3 DT code doesn't take care of
bypass_iommu=true case. I will add that check.

Thanks,
Shameer

[PATCH v2 4/6] hw/arm/virt: Add an SMMU_IO_LEN macro

[Shameer Kolothum via]

From: Nicolin Chen <nicolinc@nvidia.com>

This is useful as the subsequent support for new SMMUv3 dev will also
use the same.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 464e84ae67..e178282d71 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -147,6 +147,9 @@ static void arm_virt_compat_set(MachineClass *mc)
 #define LEGACY_RAMLIMIT_GB 255
 #define LEGACY_RAMLIMIT_BYTES (LEGACY_RAMLIMIT_GB * GiB)
 
+/* MMIO region size for SMMUv3 */
+#define SMMU_IO_LEN 0x20000
+
 /* Addresses and sizes of our components.
  * 0..128MB is space for a flash device so we can run bootrom code such as UEFI.
  * 128MB..256MB is used for miscellaneous device I/O.
@@ -178,7 +181,7 @@ static const MemMapEntry base_memmap[] = {
     [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_UART1] =              { 0x09040000, 0x00001000 },
-    [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
+    [VIRT_SMMU] =               { 0x09050000, SMMU_IO_LEN },
     [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
     [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
     [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
@@ -1453,7 +1456,6 @@ static void create_smmu(const VirtMachineState *vms,
     int irq =  vms->irqmap[VIRT_SMMU];
     int i;
     hwaddr base = vms->memmap[VIRT_SMMU].base;
-    hwaddr size = vms->memmap[VIRT_SMMU].size;
     DeviceState *dev;
 
     if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) {
@@ -1473,7 +1475,7 @@ static void create_smmu(const VirtMachineState *vms,
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
                            qdev_get_gpio_in(vms->gic, irq + i));
     }
-    create_smmuv3_dt_bindings(vms, base, size, irq);
+    create_smmuv3_dt_bindings(vms, base, SMMU_IO_LEN, irq);
 }
 
 static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
-- 
2.34.1

Re: [PATCH v2 4/6] hw/arm/virt: Add an SMMU_IO_LEN macro

[Donald Dutile]

On 5/2/25 6:27 AM, Shameer Kolothum wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
> 
> This is useful as the subsequent support for new SMMUv3 dev will also
> use the same.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   hw/arm/virt.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 464e84ae67..e178282d71 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -147,6 +147,9 @@ static void arm_virt_compat_set(MachineClass *mc)
>   #define LEGACY_RAMLIMIT_GB 255
>   #define LEGACY_RAMLIMIT_BYTES (LEGACY_RAMLIMIT_GB * GiB)
>   
> +/* MMIO region size for SMMUv3 */
> +#define SMMU_IO_LEN 0x20000
> +
>   /* Addresses and sizes of our components.
>    * 0..128MB is space for a flash device so we can run bootrom code such as UEFI.
>    * 128MB..256MB is used for miscellaneous device I/O.
> @@ -178,7 +181,7 @@ static const MemMapEntry base_memmap[] = {
>       [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
>       [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>       [VIRT_UART1] =              { 0x09040000, 0x00001000 },
> -    [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
> +    [VIRT_SMMU] =               { 0x09050000, SMMU_IO_LEN },
>       [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
>       [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
>       [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
> @@ -1453,7 +1456,6 @@ static void create_smmu(const VirtMachineState *vms,
>       int irq =  vms->irqmap[VIRT_SMMU];
>       int i;
>       hwaddr base = vms->memmap[VIRT_SMMU].base;
> -    hwaddr size = vms->memmap[VIRT_SMMU].size;
>       DeviceState *dev;
>   
>       if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) {
> @@ -1473,7 +1475,7 @@ static void create_smmu(const VirtMachineState *vms,
>           sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
>                              qdev_get_gpio_in(vms->gic, irq + i));
>       }
> -    create_smmuv3_dt_bindings(vms, base, size, irq);
> +    create_smmuv3_dt_bindings(vms, base, SMMU_IO_LEN, irq);
>   }
>   
>   static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)

Reviewed-by: Donald Dutile <ddutile@redhat.com>

Re: [PATCH v2 4/6] hw/arm/virt: Add an SMMU_IO_LEN macro

[Eric Auger]

On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
>
> This is useful as the subsequent support for new SMMUv3 dev will also
> use the same.
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/arm/virt.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 464e84ae67..e178282d71 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -147,6 +147,9 @@ static void arm_virt_compat_set(MachineClass *mc)
>  #define LEGACY_RAMLIMIT_GB 255
>  #define LEGACY_RAMLIMIT_BYTES (LEGACY_RAMLIMIT_GB * GiB)
>  
> +/* MMIO region size for SMMUv3 */
> +#define SMMU_IO_LEN 0x20000
> +
>  /* Addresses and sizes of our components.
>   * 0..128MB is space for a flash device so we can run bootrom code such as UEFI.
>   * 128MB..256MB is used for miscellaneous device I/O.
> @@ -178,7 +181,7 @@ static const MemMapEntry base_memmap[] = {
>      [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>      [VIRT_UART1] =              { 0x09040000, 0x00001000 },
> -    [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
> +    [VIRT_SMMU] =               { 0x09050000, SMMU_IO_LEN },
>      [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
>      [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
>      [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
> @@ -1453,7 +1456,6 @@ static void create_smmu(const VirtMachineState *vms,
>      int irq =  vms->irqmap[VIRT_SMMU];
>      int i;
>      hwaddr base = vms->memmap[VIRT_SMMU].base;
> -    hwaddr size = vms->memmap[VIRT_SMMU].size;
>      DeviceState *dev;
>  
>      if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) {
> @@ -1473,7 +1475,7 @@ static void create_smmu(const VirtMachineState *vms,
>          sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
>                             qdev_get_gpio_in(vms->gic, irq + i));
>      }
> -    create_smmuv3_dt_bindings(vms, base, size, irq);
> +    create_smmuv3_dt_bindings(vms, base, SMMU_IO_LEN, irq);
>  }
>  
>  static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)

[PATCH v2 5/6] hw/arm/virt: Add support for smmuv3 device

[Shameer Kolothum via]

Allow cold-plug of smmuv3 device to virt if there is no machine
wide legacy smmuv3 or a virtio-iommu is specified.

Device tree support for new smmuv3 dev is limited to the case where
it is associated with the default pcie.0 RC.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt.c        | 48 ++++++++++++++++++++++++++++++++++++++++++++
 hw/core/sysbus-fdt.c |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e178282d71..f6ff584bac 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1449,6 +1449,31 @@ static void create_smmuv3_dt_bindings(const VirtMachineState *vms, hwaddr base,
     g_free(node);
 }
 
+static void create_smmuv3_dev_dtb(VirtMachineState *vms,
+                                  DeviceState *dev)
+{
+    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
+    SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
+    int irq = platform_bus_get_irqn(pbus, sbdev, 0);
+    hwaddr base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
+    MachineState *ms = MACHINE(vms);
+    PCIBus *bus;
+
+    bus = PCI_BUS(object_property_get_link(OBJECT(dev), "primary-bus",
+                                           &error_abort));
+    if (strcmp("pcie.0", bus->qbus.name)) {
+        warn_report("SMMUv3 device only supported with pcie.0 for DT");
+        return;
+    }
+    base += vms->memmap[VIRT_PLATFORM_BUS].base;
+    irq += vms->irqmap[VIRT_PLATFORM_BUS];
+
+    vms->iommu_phandle = qemu_fdt_alloc_phandle(ms->fdt);
+    create_smmuv3_dt_bindings(vms, base, SMMU_IO_LEN, irq);
+    qemu_fdt_setprop_cells(ms->fdt, vms->pciehb_nodename, "iommu-map",
+                           0x0, vms->iommu_phandle, 0x0, 0x10000);
+}
+
 static void create_smmu(const VirtMachineState *vms,
                         PCIBus *bus)
 {
@@ -2949,6 +2974,13 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
         qlist_append_str(reserved_regions, resv_prop_str);
         qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
         g_free(resv_prop_str);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
+        if (vms->legacy_smmuv3_present || vms->iommu == VIRT_IOMMU_VIRTIO) {
+            error_setg(errp, "virt machine already has %s set. "
+                       "Doesn't support incompatible iommus",
+                       (vms->legacy_smmuv3_present) ?
+                       "iommu=smmuv3" : "virtio-iommu");
+        }
     }
 }
 
@@ -2972,6 +3004,21 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
         virtio_md_pci_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
     }
 
+    if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
+        if (!vms->legacy_smmuv3_present && vms->platform_bus_dev) {
+            VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
+
+            create_smmuv3_dev_dtb(vms, dev);
+            if (vms->iommu != VIRT_IOMMU_SMMUV3) {
+                vms->iommu = VIRT_IOMMU_SMMUV3;
+            }
+            if (!vmc->no_nested_smmu) {
+                object_property_set_str(OBJECT(dev), "stage", "nested",
+                                        &error_fatal);
+            }
+        }
+    }
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
         PCIDevice *pdev = PCI_DEVICE(dev);
 
@@ -3174,6 +3221,7 @@ static void virt_machine_class_init(ObjectClass *oc, const void *data)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_UEFI_VARS_SYSBUS);
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ARM_SMMUV3);
 #ifdef CONFIG_TPM
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
 #endif
diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
index c339a27875..d778c0f559 100644
--- a/hw/core/sysbus-fdt.c
+++ b/hw/core/sysbus-fdt.c
@@ -31,6 +31,7 @@
 #include "qemu/error-report.h"
 #include "system/device_tree.h"
 #include "system/tpm.h"
+#include "hw/arm/smmuv3.h"
 #include "hw/platform-bus.h"
 #include "hw/vfio/vfio-platform.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
@@ -513,6 +514,8 @@ static const BindingEntry bindings[] = {
 #ifdef CONFIG_LINUX
     TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node),
     TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
+    /* No generic DT support for smmuv3 dev. Support added for arm virt only */
+    TYPE_BINDING(TYPE_ARM_SMMUV3, no_fdt_node),
     VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
 #endif
 #ifdef CONFIG_TPM
-- 
2.34.1

Re: [PATCH v2 5/6] hw/arm/virt: Add support for smmuv3 device

[Nicolin Chen]

On Fri, May 02, 2025 at 11:27:06AM +0100, Shameer Kolothum wrote:
> @@ -2972,6 +3004,21 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>          virtio_md_pci_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
>      }
>  
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {

This seems to be on the path of a hotplug function? Mind elaborating
why, while PATCH-1 sets hotpluggable to false?

> +        if (!vms->legacy_smmuv3_present && vms->platform_bus_dev) {
> +            VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> +
> +            create_smmuv3_dev_dtb(vms, dev);
> +            if (vms->iommu != VIRT_IOMMU_SMMUV3) {

Should this be VIRT_IOMMU_NONE only as the other cases are rejected?

> +                vms->iommu = VIRT_IOMMU_SMMUV3;

Thanks
nicolin

RE: [PATCH v2 5/6] hw/arm/virt: Add support for smmuv3 device

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, May 2, 2025 6:55 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 5/6] hw/arm/virt: Add support for smmuv3 device
> 
> On Fri, May 02, 2025 at 11:27:06AM +0100, Shameer Kolothum wrote:
> > @@ -2972,6 +3004,21 @@ static void
> virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> >          virtio_md_pci_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev),
> errp);
> >      }
> >
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
> 
> This seems to be on the path of a hotplug function? Mind elaborating
> why, while PATCH-1 sets hotpluggable to false?

Not really. Qemu has this path for both cold plug case(which is before the Guest
is booted, ie, -device arm-smmuv3, case) and for hot plug case (where we try to
add the device after Guest is booted, ie,  device_add arm-smmuv3, case).

We are preventing only the hot plug case.

> 
> > +        if (!vms->legacy_smmuv3_present && vms->platform_bus_dev) {
> > +            VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > +
> > +            create_smmuv3_dev_dtb(vms, dev);
> > +            if (vms->iommu != VIRT_IOMMU_SMMUV3) {
> 
> Should this be VIRT_IOMMU_NONE only as the other cases are rejected?

The check is basically to handle cases where we will have multiple SMMUv3 devices
and to set this only once.

Thanks,
Shameer

Re: [PATCH v2 5/6] hw/arm/virt: Add support for smmuv3 device

[Eric Auger]

On 5/2/25 12:27 PM, Shameer Kolothum wrote:
I would change the title into something like "Allow -device arm-smmuv3
instantiation"
> Allow cold-plug of smmuv3 device to virt if there is no machine
> wide legacy smmuv3 or a virtio-iommu is specified.
>
> Device tree support for new smmuv3 dev is limited to the case where
> it is associated with the default pcie.0 RC.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/virt.c        | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/core/sysbus-fdt.c |  3 +++
>  2 files changed, 51 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e178282d71..f6ff584bac 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1449,6 +1449,31 @@ static void create_smmuv3_dt_bindings(const VirtMachineState *vms, hwaddr base,
>      g_free(node);
>  }
>  
> +static void create_smmuv3_dev_dtb(VirtMachineState *vms,
> +                                  DeviceState *dev)
> +{
> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> +    SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
> +    int irq = platform_bus_get_irqn(pbus, sbdev, 0);
> +    hwaddr base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> +    MachineState *ms = MACHINE(vms);
> +    PCIBus *bus;
> +
> +    bus = PCI_BUS(object_property_get_link(OBJECT(dev), "primary-bus",
> +                                           &error_abort));
> +    if (strcmp("pcie.0", bus->qbus.name)) {
> +        warn_report("SMMUv3 device only supported with pcie.0 for DT");
> +        return;
> +    }
> +    base += vms->memmap[VIRT_PLATFORM_BUS].base;
> +    irq += vms->irqmap[VIRT_PLATFORM_BUS];
> +
> +    vms->iommu_phandle = qemu_fdt_alloc_phandle(ms->fdt);
> +    create_smmuv3_dt_bindings(vms, base, SMMU_IO_LEN, irq);
> +    qemu_fdt_setprop_cells(ms->fdt, vms->pciehb_nodename, "iommu-map",
> +                           0x0, vms->iommu_phandle, 0x0, 0x10000);
> +}
> +
>  static void create_smmu(const VirtMachineState *vms,
>                          PCIBus *bus)
>  {
> @@ -2949,6 +2974,13 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>          qlist_append_str(reserved_regions, resv_prop_str);
>          qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
>          g_free(resv_prop_str);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
> +        if (vms->legacy_smmuv3_present || vms->iommu == VIRT_IOMMU_VIRTIO) {
> +            error_setg(errp, "virt machine already has %s set. "
> +                       "Doesn't support incompatible iommus",
> +                       (vms->legacy_smmuv3_present) ?
> +                       "iommu=smmuv3" : "virtio-iommu");
what about bypass mode?
> +        }
>      }
>  }
>  
> @@ -2972,6 +3004,21 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>          virtio_md_pci_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
>      }
>  
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
> +        if (!vms->legacy_smmuv3_present && vms->platform_bus_dev) {
this answers my previous comment ;-)
> +            VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> +
> +            create_smmuv3_dev_dtb(vms, dev);
> +            if (vms->iommu != VIRT_IOMMU_SMMUV3) {
> +                vms->iommu = VIRT_IOMMU_SMMUV3;
> +            }
> +            if (!vmc->no_nested_smmu) {
> +                object_property_set_str(OBJECT(dev), "stage", "nested",
> +                                        &error_fatal);
> +            }
> +        }
> +    }
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>          PCIDevice *pdev = PCI_DEVICE(dev);
>  
> @@ -3174,6 +3221,7 @@ static void virt_machine_class_init(ObjectClass *oc, const void *data)
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_UEFI_VARS_SYSBUS);
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ARM_SMMUV3);
>  #ifdef CONFIG_TPM
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
>  #endif
> diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
> index c339a27875..d778c0f559 100644
> --- a/hw/core/sysbus-fdt.c
> +++ b/hw/core/sysbus-fdt.c
> @@ -31,6 +31,7 @@
>  #include "qemu/error-report.h"
>  #include "system/device_tree.h"
>  #include "system/tpm.h"
> +#include "hw/arm/smmuv3.h"
>  #include "hw/platform-bus.h"
>  #include "hw/vfio/vfio-platform.h"
>  #include "hw/vfio/vfio-calxeda-xgmac.h"
> @@ -513,6 +514,8 @@ static const BindingEntry bindings[] = {
>  #ifdef CONFIG_LINUX
>      TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node),
>      TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
> +    /* No generic DT support for smmuv3 dev. Support added for arm virt only */
> +    TYPE_BINDING(TYPE_ARM_SMMUV3, no_fdt_node),
Can't it live outside the CONFIG_LINUX?


>      VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
>  #endif
>  #ifdef CONFIG_TPM
Eric

RE: [PATCH v2 5/6] hw/arm/virt: Add support for smmuv3 device

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Monday, May 5, 2025 11:12 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v2 5/6] hw/arm/virt: Add support for smmuv3 device
> 
> 
> 
> On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> I would change the title into something like "Allow -device arm-smmuv3
> instantiation"

Ok.

> > Allow cold-plug of smmuv3 device to virt if there is no machine
> > wide legacy smmuv3 or a virtio-iommu is specified.
> >
> > Device tree support for new smmuv3 dev is limited to the case where
> > it is associated with the default pcie.0 RC.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/virt.c        | 48
> ++++++++++++++++++++++++++++++++++++++++++++
> >  hw/core/sysbus-fdt.c |  3 +++
> >  2 files changed, 51 insertions(+)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index e178282d71..f6ff584bac 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1449,6 +1449,31 @@ static void create_smmuv3_dt_bindings(const
> VirtMachineState *vms, hwaddr base,
> >      g_free(node);
> >  }
> >
> > +static void create_smmuv3_dev_dtb(VirtMachineState *vms,
> > +                                  DeviceState *dev)
> > +{
> > +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms-
> >platform_bus_dev);
> > +    SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
> > +    int irq = platform_bus_get_irqn(pbus, sbdev, 0);
> > +    hwaddr base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> > +    MachineState *ms = MACHINE(vms);
> > +    PCIBus *bus;
> > +
> > +    bus = PCI_BUS(object_property_get_link(OBJECT(dev), "primary-bus",
> > +                                           &error_abort));
> > +    if (strcmp("pcie.0", bus->qbus.name)) {
> > +        warn_report("SMMUv3 device only supported with pcie.0 for DT");
> > +        return;
> > +    }
> > +    base += vms->memmap[VIRT_PLATFORM_BUS].base;
> > +    irq += vms->irqmap[VIRT_PLATFORM_BUS];
> > +
> > +    vms->iommu_phandle = qemu_fdt_alloc_phandle(ms->fdt);
> > +    create_smmuv3_dt_bindings(vms, base, SMMU_IO_LEN, irq);
> > +    qemu_fdt_setprop_cells(ms->fdt, vms->pciehb_nodename, "iommu-
> map",
> > +                           0x0, vms->iommu_phandle, 0x0, 0x10000);
> > +}
> > +
> >  static void create_smmu(const VirtMachineState *vms,
> >                          PCIBus *bus)
> >  {
> > @@ -2949,6 +2974,13 @@ static void
> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> >          qlist_append_str(reserved_regions, resv_prop_str);
> >          qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
> >          g_free(resv_prop_str);
> > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
> > +        if (vms->legacy_smmuv3_present || vms->iommu ==
> VIRT_IOMMU_VIRTIO) {
> > +            error_setg(errp, "virt machine already has %s set. "
> > +                       "Doesn't support incompatible iommus",
> > +                       (vms->legacy_smmuv3_present) ?
> > +                       "iommu=smmuv3" : "virtio-iommu");
> what about bypass mode?

Yeah. Bypass is handled only in IORT ACPI code(that too silently!). I will add
a check to explicitly block specifying a SMMUv3 dev with RC bypass mode.

> > +        }
> >      }
> >  }
> >
> > @@ -2972,6 +3004,21 @@ static void
> virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> >          virtio_md_pci_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev),
> errp);
> >      }
> >
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
> > +        if (!vms->legacy_smmuv3_present && vms->platform_bus_dev) {
> this answers my previous comment ;-)

Yes.

> > +            VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > +
> > +            create_smmuv3_dev_dtb(vms, dev);
> > +            if (vms->iommu != VIRT_IOMMU_SMMUV3) {
> > +                vms->iommu = VIRT_IOMMU_SMMUV3;
> > +            }
> > +            if (!vmc->no_nested_smmu) {
> > +                object_property_set_str(OBJECT(dev), "stage", "nested",
> > +                                        &error_fatal);
> > +            }
> > +        }
> > +    }
> > +
> >      if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> >          PCIDevice *pdev = PCI_DEVICE(dev);
> >
> > @@ -3174,6 +3221,7 @@ static void virt_machine_class_init(ObjectClass
> *oc, const void *data)
> >      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
> >      machine_class_allow_dynamic_sysbus_dev(mc,
> TYPE_VFIO_PLATFORM);
> >      machine_class_allow_dynamic_sysbus_dev(mc,
> TYPE_UEFI_VARS_SYSBUS);
> > +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ARM_SMMUV3);
> >  #ifdef CONFIG_TPM
> >      machine_class_allow_dynamic_sysbus_dev(mc,
> TYPE_TPM_TIS_SYSBUS);
> >  #endif
> > diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
> > index c339a27875..d778c0f559 100644
> > --- a/hw/core/sysbus-fdt.c
> > +++ b/hw/core/sysbus-fdt.c
> > @@ -31,6 +31,7 @@
> >  #include "qemu/error-report.h"
> >  #include "system/device_tree.h"
> >  #include "system/tpm.h"
> > +#include "hw/arm/smmuv3.h"
> >  #include "hw/platform-bus.h"
> >  #include "hw/vfio/vfio-platform.h"
> >  #include "hw/vfio/vfio-calxeda-xgmac.h"
> > @@ -513,6 +514,8 @@ static const BindingEntry bindings[] = {
> >  #ifdef CONFIG_LINUX
> >      TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC,
> add_calxeda_midway_xgmac_fdt_node),
> >      TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
> > +    /* No generic DT support for smmuv3 dev. Support added for arm virt
> only */
> > +    TYPE_BINDING(TYPE_ARM_SMMUV3, no_fdt_node),
> Can't it live outside the CONFIG_LINUX?

Yes, It could. Will change.

Thanks,
Shameer

[PATCH v2 6/6] hw/arm/smmuv3: Enable smmuv3 device creation

[Shameer Kolothum via]

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/smmuv3.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 605de9b721..e13950b7c5 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -2022,6 +2022,7 @@ static void smmuv3_class_init(ObjectClass *klass, const void *data)
     device_class_set_props(dc, smmuv3_properties);
     dc->hotpluggable = false;
     dc->bus_type = TYPE_PCIE_BUS;
+    dc->user_creatable = true;
 }
 
 static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
-- 
2.34.1

Re: [PATCH v2 6/6] hw/arm/smmuv3: Enable smmuv3 device creation

[Nicolin Chen]

On Fri, May 02, 2025 at 11:27:07AM +0100, Shameer Kolothum wrote:
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Though I think the commit log shouldn't be empty,

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Re: [PATCH v2 6/6] hw/arm/smmuv3: Enable smmuv3 device creation

[Eric Auger]

On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/smmuv3.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 605de9b721..e13950b7c5 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -2022,6 +2022,7 @@ static void smmuv3_class_init(ObjectClass *klass, const void *data)
>      device_class_set_props(dc, smmuv3_properties);
>      dc->hotpluggable = false;
>      dc->bus_type = TYPE_PCIE_BUS;
> +    dc->user_creatable = true;
>  }
>  
>  static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
I would squash this into the previous patch

Eric

Re: [PATCH v2 0/6] Add support for user creatable SMMUv3 device

[Donald Dutile]

On 5/2/25 6:27 AM, Shameer Kolothum wrote:
> Hi All,
> 
> Changes from v1:
> https://lore.kernel.org/qemu-devel/20250415081104.71708-1-shameerali.kolothum.thodi@huawei.com/
> 
> Addressed feedback on v1. Thanks to all.
> 1. Retained the same name as the legacy SMMUv3(arm-smmuv3) for new
>     device type as well (instead of arm-smmuv3-dev type usage in v1).
> 2. Changes to ACPI IORT to use the same struct SMMUv3Device for both
>     legacy SMMUv3 and the new SMMUV3 device
> 3. Removed the restriction of creating SMMUv3 dev if virt ver > 9.2.
> 
> Cover letter from v1:
> 
> This patch series introduces support for a user-creatable SMMUv3 device
> (-device arm-smmuv3) in QEMU.
> 
> The implementation is based on feedback received from the RFCv2[0]:
> "hw/arm/virt: Add support for user-creatable accelerated SMMUv3"
> 
> Currently, QEMU's SMMUv3 emulation (iommu=smmuv3) is tied to the machine
should it be clarified as 'to the machine's sysbus' ?
> and does not support instantiating multiple SMMUv3 devices—each associated
> with a separate PCIe root complex. In contrast, real-world ARM systems
> often include multiple SMMUv3 instances, each bound to a different PCIe
> root complex.
> 
> This also lays the groundwork for supporting accelerated SMMUv3, as
> proposed in the aforementioned RFC. Please note, the accelerated SMMUv3
> support is not part of this series and will be sent out as a separate
> series later on top of this one.
> 
> Summary of changes:
> 
>   -Introduces support for multiple -device arm-smmuv3-dev,bus=pcie.x
Drop, as you've done elsewhere.........................^^^^

>    instances.
> 
>    Example usage:
> 
>    -device arm-smmuv3,bus=pcie.0
>    -device virtio-net-pci,bus=pcie.0
>    -device pxb-pcie,id=pcie.1,bus_nr=2
>    -device arm-smmuv3,bus=pcie.1
>    -device pcie-root-port,id=pcie.port1,bus=pcie.1
>    -device virtio-net-pci,bus=pcie.port1
> 
>    -Supports either the legacy iommu=smmuv3 option or the new
>    "-device arm-smmuv3" model.> 
>    -Adds device tree bindings for the new SMMUv3 device on the arm/virt
>     machine only, and only for the default pcie.0 root complex.
>     (Note: pxb-pcie root complexes are currently not supported with the
>      device tree due to known limitations[1].)
> 
> Please take a look and let me know your feedback.
> 
> Thanks,
> Shameer
> [0]:https://lore.kernel.org/qemu-devel/20250311141045.66620-1-shameerali.kolothum.thodi@huawei.com/
> [1]:https://lore.kernel.org/qemu-devel/20230421165037.2506-1-Jonathan.Cameron@huawei.com/
> 
> Nicolin Chen (1):
>    hw/arm/virt: Add an SMMU_IO_LEN macro
> 
> Shameer Kolothum (5):
>    hw/arm/smmuv3: Add support to associate a PCIe RC
>    hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices
>    hw/arm/virt: Factor out common SMMUV3 dt bindings code
>    hw/arm/virt: Add support for smmuv3 device
>    hw/arm/smmuv3: Enable smmuv3 device creation
> 
>   hw/arm/smmuv3.c          |  27 +++++++
>   hw/arm/virt-acpi-build.c | 162 +++++++++++++++++++++++++++++++--------
>   hw/arm/virt.c            | 112 ++++++++++++++++++++-------
>   hw/core/sysbus-fdt.c     |   3 +
>   include/hw/arm/virt.h    |   1 +
>   5 files changed, 244 insertions(+), 61 deletions(-)
>