[PATCH v4 0/7] hw/arm/virt: Add support for user creatable SMMUv3 device

[PATCH v4 0/7] hw/arm/virt: Add support for user creatable SMMUv3 device

[Shameer Kolothum via]

Hi All,

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

Addressed feedback on v3. Thanks to all.
Major changes:
1. Based on Igor's suggestion, moved the associated primary-bus is of type
   TYPE_PCI_HOST_BRIDGE check to smmu-common.
2. Addressed IORT related comments from Eric
3. Picked up T-by tag from Nathan. Thanks for giving it a spin.
4. Added a documents entry for arm-smmuv3.

Cover letter:

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 series allows to specify multiple SMMUv3 instances as below,

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

The multiple SMMUv3 instance support lays the groundwork for supporting
accelerated SMMUv3, as proposed in the aforementioned RFCv2[0]. The
proposed accelerated support will be an optional property like below,
-device arm-smmuv3,primary-bus=pcie.1,accel=on,..

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.

This series also,

-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].)

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

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 (6):
  hw/arm/smmu-common: Check SMMU has PCIe Root Complex association
  hw/arm/virt-acpi-build: Re-arrange SMMUv3 IORT build
  hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices
  hw/arm/virt: Factor out common SMMUV3 dt bindings code
  hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation
  qemu-options.hx: Document the arm-smmuv3 device

 hw/arm/smmu-common.c     |   4 +-
 hw/arm/smmuv3.c          |   2 +
 hw/arm/virt-acpi-build.c | 166 +++++++++++++++++++++++++++++++--------
 hw/arm/virt.c            | 108 ++++++++++++++++++-------
 hw/core/sysbus-fdt.c     |   3 +
 include/hw/arm/virt.h    |   1 +
 qemu-options.hx          |   6 ++
 7 files changed, 228 insertions(+), 62 deletions(-)

-- 
2.47.0

[PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Shameer Kolothum via]

Although this change does not affect functionality at present, it is
required when we add support for user-creatable SMMUv3 devices in
future patches.

Tested-by: Nathan Chen <nathanc@nvidia.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/smmu-common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index f39b99e526..7890aa12c1 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -20,6 +20,7 @@
 #include "trace.h"
 #include "exec/target_page.h"
 #include "hw/core/cpu.h"
+#include "hw/pci/pci_bridge.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu/jhash.h"
@@ -937,7 +938,8 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
                                      g_free, g_free);
     s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
 
-    if (s->primary_bus) {
+    if (s->primary_bus && object_dynamic_cast(OBJECT(s->primary_bus)->parent,
+                                              TYPE_PCI_HOST_BRIDGE)) {
         pci_setup_iommu(s->primary_bus, &smmu_ops, s);
     } else {
         error_setg(errp, "SMMU is not attached to any PCI bus!");
-- 
2.47.0

[PATCH v4 2/7] hw/arm/virt-acpi-build: Re-arrange SMMUv3 IORT build

[Shameer Kolothum via]

Introduces a new struct AcpiIortSMMUv3Dev to hold all the information
required for SMMUv3 IORT node and use that for populating the node.

The current machine wide SMMUv3 is named as legacy SMMUv3 as we will
soon add support for user-creatable SMMUv3 devices. These changes will
be useful to have common code paths when we add that support.

Tested-by: Nathan Chen <nathanc@nvidia.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt-acpi-build.c | 111 +++++++++++++++++++++++++++------------
 hw/arm/virt.c            |   1 +
 include/hw/arm/virt.h    |   1 +
 3 files changed, 79 insertions(+), 34 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 7e8e0f0298..d39506179a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -266,6 +266,36 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
     return idmap_a->input_base - idmap_b->input_base;
 }
 
+struct AcpiIortSMMUv3Dev {
+    int irq;
+    hwaddr base;
+    GArray *idmaps;
+    /* Offset of the SMMUv3 IORT Node relative to the start of the IORT. */
+    size_t offset;
+};
+typedef struct AcpiIortSMMUv3Dev AcpiIortSMMUv3Dev;
+
+static void
+populate_smmuv3_legacy_dev(GArray *sdev_blob)
+{
+    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
+    AcpiIortSMMUv3Dev sdev;
+
+    sdev.idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
+    object_child_foreach_recursive(object_get_root(),
+                                   iort_host_bridges, sdev.idmaps);
+
+    /*
+     * 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.
+     */
+    g_array_sort(sdev.idmaps, iort_idmap_compare);
+    sdev.base = vms->memmap[VIRT_SMMU].base;
+    sdev.irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
+    g_array_append_val(sdev_blob, sdev);
+}
+
 /*
  * Input Output Remapping Table (IORT)
  * Conforms to "IO Remapping Table System Software on ARM Platforms",
@@ -274,11 +304,12 @@ 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;
+    AcpiIortSMMUv3Dev *sdev;
+    size_t node_size;
+    int num_smmus = 0;
     uint32_t id = 0;
-    GArray *smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
+    GArray *smmuv3_devs = g_array_new(false, true, sizeof(AcpiIortSMMUv3Dev));
     GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
 
     AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
@@ -286,28 +317,32 @@ 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};
-
-        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);
+    nb_nodes = 2; /* RC, ITS */
+    if (vms->legacy_smmuv3_present) {
+        populate_smmuv3_legacy_dev(smmuv3_devs);
+    }
 
+    num_smmus = smmuv3_devs->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++) {
+            sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
+            for (j = 0; j < sdev->idmaps->len; j++) {
+                AcpiIortIdMapping *idmap = &g_array_index(sdev->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 +351,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 +375,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++) {
+        sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
+        int irq = sdev->irq;
 
-        smmu_offset = table_data->len - table.table_offset;
+        sdev->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 +390,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, sdev->base, 8);
         /* Flags */
         build_append_int_noprefix(table_data, 1 /* COHACC Override */, 4);
         build_append_int_noprefix(table_data, 0, 4); /* Reserved */
@@ -404,15 +439,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++) {
+            sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
+
+            for (j = 0; j < sdev->idmaps->len; j++) {
+                range = &g_array_index(sdev->idmaps, AcpiIortIdMapping, j);
+                /* output IORT node is the smmuv3 node */
+                build_iort_id_mapping(table_data, range->input_base,
+                                      range->id_count, sdev->offset);
+            }
         }
 
         /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
@@ -428,8 +467,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++) {
+        sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
+        g_array_free(sdev->idmaps, true);
+    }
+    g_array_free(smmuv3_devs, true);
 }
 
 /*
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9a6cd085a3..73bd2bd5f2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1614,6 +1614,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 9a1b0f53d2..8b1404b5f6 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -174,6 +174,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.47.0

[PATCH v4 3/7] 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.

An example IORT Id mappings for a Qemu virt machine with two
PCIe Root Complexes each assocaited with a SMMUv3 will
be something like below,

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

  +--------------------+           +--------------------+
  |   Root Complex 0   |           |   Root Complex 1   |
  |                    |           |                    |
  |  Requestor IDs     |           |  Requestor IDs     |
  |  0x0000 - 0x00FF   |           |  0x0100 - 0x01FF   |
  +---------+----------+           +---------+----------+
            |                               |
            |                               |
            |       Stream ID Mapping       |
            v                               v
  +--------------------+          +--------------------+
  |    SMMUv3 Node 0   |          |    SMMUv3 Node 1   |
  |                    |          |                    |
  | Stream IDs 0x0000- |          | Stream IDs 0x0100- |
  | 0x00FF mapped from |          | 0x01FF mapped from |
  | RC0 Requestor IDs  |          | RC1 Requestor IDs  |
  +--------------------+          +--------------------+
            |                                |
            |                                |
            +----------------+---------------+
                             |
                             |Device ID Mapping
                             v
              +----------------------------+
              |       ITS Node 0           |
              |                            |
              | Device IDs:                |
              | 0x0000 - 0x00FF (from RC0) |
              | 0x0100 - 0x01FF (from RC1) |
              | 0x0200 - 0xFFFF (No SMMU)  |
              +----------------------------+

Tested-by: Nathan Chen <nathanc@nvidia.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt-acpi-build.c | 55 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index d39506179a..72b79100ce 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"
@@ -296,6 +297,58 @@ populate_smmuv3_legacy_dev(GArray *sdev_blob)
     g_array_append_val(sdev_blob, sdev);
 }
 
+static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer b)
+{
+    AcpiIortSMMUv3Dev *sdev_a = (AcpiIortSMMUv3Dev *)a;
+    AcpiIortSMMUv3Dev *sdev_b = (AcpiIortSMMUv3Dev *)b;
+    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->idmaps,
+                                              AcpiIortIdMapping, 0);
+    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->idmaps,
+                                              AcpiIortIdMapping, 0);
+    return map_a->input_base - map_b->input_base;
+}
+
+static int iort_smmuv3_devices(Object *obj, void *opaque)
+{
+    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
+    GArray *sdev_blob = opaque;
+    AcpiIortIdMapping idmap;
+    PlatformBusDevice *pbus;
+    AcpiIortSMMUv3Dev sdev;
+    int min_bus, max_bus;
+    SysBusDevice *sbdev;
+    PCIBus *bus;
+
+    if (!object_dynamic_cast(obj, TYPE_ARM_SMMUV3)) {
+        return 0;
+    }
+
+    bus = PCI_BUS(object_property_get_link(obj, "primary-bus", &error_abort));
+    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.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.idmaps, idmap);
+    g_array_append_val(sdev_blob, sdev);
+    return 0;
+}
+
+static void populate_smmuv3_dev(GArray *sdev_blob)
+{
+    object_child_foreach_recursive(object_get_root(),
+                                   iort_smmuv3_devices, sdev_blob);
+    /* Sort the smmuv3 devices(if any) by smmu idmap input_base */
+    g_array_sort(sdev_blob, smmuv3_dev_idmap_compare);
+}
+
 /*
  * Input Output Remapping Table (IORT)
  * Conforms to "IO Remapping Table System Software on ARM Platforms",
@@ -320,6 +373,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     nb_nodes = 2; /* RC, ITS */
     if (vms->legacy_smmuv3_present) {
         populate_smmuv3_legacy_dev(smmuv3_devs);
+    } else {
+        populate_smmuv3_dev(smmuv3_devs);
     }
 
     num_smmus = smmuv3_devs->len;
-- 
2.47.0

[PATCH v4 4/7] 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.

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Nathan Chen <nathanc@nvidia.com>
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 73bd2bd5f2..71b923f786 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1412,19 +1412,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;
@@ -1443,27 +1467,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.47.0

[PATCH v4 5/7] 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>
Reviewed-by: Donald Dutile <ddutile@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Nathan Chen <nathanc@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 71b923f786..eeace4754d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -146,6 +146,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.
@@ -177,7 +180,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},
@@ -1447,7 +1450,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) {
@@ -1467,7 +1469,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.47.0

[PATCH v4 6/7] hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation

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

Tested-by: Nathan Chen <nathanc@nvidia.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/smmuv3.c      |  2 ++
 hw/arm/virt.c        | 47 ++++++++++++++++++++++++++++++++++++++++++++
 hw/core/sysbus-fdt.c |  3 +++
 3 files changed, 52 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index ab67972353..bcf8af8dc7 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1996,6 +1996,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->user_creatable = true;
 }
 
 static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index eeace4754d..3be007d87c 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"
@@ -1443,6 +1444,28 @@ static void create_smmuv3_dt_bindings(const VirtMachineState *vms, hwaddr base,
     g_free(node);
 }
 
+static void create_smmuv3_dev_dtb(VirtMachineState *vms,
+                                  DeviceState *dev, PCIBus *bus)
+{
+    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);
+
+    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)
 {
@@ -2931,6 +2954,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");
+        }
     }
 }
 
@@ -2954,6 +2984,22 @@ 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) {
+            PCIBus *bus;
+
+            bus = PCI_BUS(object_property_get_link(OBJECT(dev), "primary-bus",
+                                                   &error_abort));
+            if (pci_bus_bypass_iommu(bus)) {
+                error_setg(errp, "Bypass option cannot be set for SMMUv3 "
+                           "associated PCIe RC");
+                return;
+            }
+
+            create_smmuv3_dev_dtb(vms, dev, bus);
+        }
+    }
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
         PCIDevice *pdev = PCI_DEVICE(dev);
 
@@ -3156,6 +3202,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..e80776080b 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"
@@ -518,6 +519,8 @@ static const BindingEntry bindings[] = {
 #ifdef CONFIG_TPM
     TYPE_BINDING(TYPE_TPM_TIS_SYSBUS, add_tpm_tis_fdt_node),
 #endif
+    /* No generic DT support for smmuv3 dev. Support added for arm virt only */
+    TYPE_BINDING(TYPE_ARM_SMMUV3, no_fdt_node),
     TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
     TYPE_BINDING(TYPE_UEFI_VARS_SYSBUS, add_uefi_vars_node),
     TYPE_BINDING("", NULL), /* last element */
-- 
2.47.0

[PATCH v4 7/7] qemu-options.hx: Document the arm-smmuv3 device

[Shameer Kolothum via]

Now that arm,virt can have user-creatable smmuv3 devices, document it.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 qemu-options.hx | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 7eb8e02b4b..3edbde45bb 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1226,6 +1226,12 @@ SRST
     ``aw-bits=val`` (val between 32 and 64, default depends on machine)
         This decides the address width of the IOVA address space.
 
+``-device arm-smmuv3,primary-bus=id``
+    This is only supported by ``-machine virt`` (ARM).
+
+    ``primary-bus=id``
+        The PCIe Root Complex to be associated with.
+
 ERST
 
 DEF("name", HAS_ARG, QEMU_OPTION_name,
-- 
2.47.0

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Nicolin Chen]

On Fri, Jun 13, 2025 at 03:44:43PM +0100, Shameer Kolothum wrote:
> Although this change does not affect functionality at present, it is
> required when we add support for user-creatable SMMUv3 devices in
> future patches.
> 
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

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

Re: [PATCH v4 2/7] hw/arm/virt-acpi-build: Re-arrange SMMUv3 IORT build

[Nicolin Chen]

On Fri, Jun 13, 2025 at 03:44:44PM +0100, Shameer Kolothum wrote:
> Introduces a new struct AcpiIortSMMUv3Dev to hold all the information
> required for SMMUv3 IORT node and use that for populating the node.
> 
> The current machine wide SMMUv3 is named as legacy SMMUv3 as we will
> soon add support for user-creatable SMMUv3 devices. These changes will
> be useful to have common code paths when we add that support.
> 
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

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

> +static void
> +populate_smmuv3_legacy_dev(GArray *sdev_blob)

nit: could fit into one line?

Re: [PATCH v4 3/7] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Nicolin Chen]

On Fri, Jun 13, 2025 at 03:44:45PM +0100, 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.
> 
> An example IORT Id mappings for a Qemu virt machine with two
> PCIe Root Complexes each assocaited with a SMMUv3 will
> be something like below,
> 
>   -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.0
>   -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1
>   ...
> 
>   +--------------------+           +--------------------+
>   |   Root Complex 0   |           |   Root Complex 1   |
>   |                    |           |                    |
>   |  Requestor IDs     |           |  Requestor IDs     |
>   |  0x0000 - 0x00FF   |           |  0x0100 - 0x01FF   |
>   +---------+----------+           +---------+----------+
>             |                               |
>             |                               |
>             |       Stream ID Mapping       |
>             v                               v
>   +--------------------+          +--------------------+
>   |    SMMUv3 Node 0   |          |    SMMUv3 Node 1   |
>   |                    |          |                    |
>   | Stream IDs 0x0000- |          | Stream IDs 0x0100- |
>   | 0x00FF mapped from |          | 0x01FF mapped from |
>   | RC0 Requestor IDs  |          | RC1 Requestor IDs  |
>   +--------------------+          +--------------------+
>             |                                |
>             |                                |
>             +----------------+---------------+
>                              |
>                              |Device ID Mapping
>                              v
>               +----------------------------+
>               |       ITS Node 0           |
>               |                            |
>               | Device IDs:                |
>               | 0x0000 - 0x00FF (from RC0) |
>               | 0x0100 - 0x01FF (from RC1) |
>               | 0x0200 - 0xFFFF (No SMMU)  |
>               +----------------------------+
> 
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

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

Re: [PATCH v4 6/7] hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation

[Nicolin Chen]

On Fri, Jun 13, 2025 at 03:44:48PM +0100, Shameer Kolothum wrote:
> 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.

I forgot the detail for this limitation. Maybe spare a few more
words briefly describing why?

> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

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

Re: [PATCH v4 7/7] qemu-options.hx: Document the arm-smmuv3 device

[Nicolin Chen]

On Fri, Jun 13, 2025 at 03:44:49PM +0100, Shameer Kolothum wrote:
> Now that arm,virt can have user-creatable smmuv3 devices, document it.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

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

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Jonathan Cameron via]

On Fri, 13 Jun 2025 15:44:43 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> Although this change does not affect functionality at present, it is

Patch title says PCIe.  This check is vs PCI host bridge.

No idea which one you wanted, but if it is PCIe needs to be
TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
I think.

> required when we add support for user-creatable SMMUv3 devices in
> future patches.
> 
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/smmu-common.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index f39b99e526..7890aa12c1 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -20,6 +20,7 @@
>  #include "trace.h"
>  #include "exec/target_page.h"
>  #include "hw/core/cpu.h"
> +#include "hw/pci/pci_bridge.h"
>  #include "hw/qdev-properties.h"
>  #include "qapi/error.h"
>  #include "qemu/jhash.h"
> @@ -937,7 +938,8 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
>                                       g_free, g_free);
>      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
>  
> -    if (s->primary_bus) {
> +    if (s->primary_bus && object_dynamic_cast(OBJECT(s->primary_bus)->parent,
> +                                              TYPE_PCI_HOST_BRIDGE)) {
>          pci_setup_iommu(s->primary_bus, &smmu_ops, s);
>      } else {
>          error_setg(errp, "SMMU is not attached to any PCI bus!");

Re: [PATCH v4 2/7] hw/arm/virt-acpi-build: Re-arrange SMMUv3 IORT build

[Jonathan Cameron via]

On Fri, 13 Jun 2025 15:44:44 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> Introduces a new struct AcpiIortSMMUv3Dev to hold all the information
> required for SMMUv3 IORT node and use that for populating the node.
> 
> The current machine wide SMMUv3 is named as legacy SMMUv3 as we will
> soon add support for user-creatable SMMUv3 devices. These changes will
> be useful to have common code paths when we add that support.
> 
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Some trivial stuff inline. Otherwise only comment in passing is that
some of the loop nests are deep enough that it might be worth
considering factoring some of those out as helper functions.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 111 +++++++++++++++++++++++++++------------
>  hw/arm/virt.c            |   1 +
>  include/hw/arm/virt.h    |   1 +
>  3 files changed, 79 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 7e8e0f0298..d39506179a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -266,6 +266,36 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
>      return idmap_a->input_base - idmap_b->input_base;
>  }
>  
> +struct AcpiIortSMMUv3Dev {
> +    int irq;
> +    hwaddr base;
> +    GArray *idmaps;
> +    /* Offset of the SMMUv3 IORT Node relative to the start of the IORT. */
> +    size_t offset;
> +};
> +typedef struct AcpiIortSMMUv3Dev AcpiIortSMMUv3Dev;

Hmm. This file is a bit inconsistent on style but there are instances of the more
compact

typedef struct AcpiIortSMMUv3Dev {
    int irq;
    hwaddr base;
    GArray *idmaps;
    /* Offset of the SMMUv3 IORT Node relative to the start of the IORT. */
    size_t offset;
} AcpiIortSMMUv3Dev;

> +
> +static void
> +populate_smmuv3_legacy_dev(GArray *sdev_blob)
What Nicolin said here.


> +{
> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> +    AcpiIortSMMUv3Dev sdev;
> +
> +    sdev.idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> +    object_child_foreach_recursive(object_get_root(),
> +                                   iort_host_bridges, sdev.idmaps);

Not sure why this wrap. I'd move iort_host_bridges up a line probably.

> +
> +    /*
> +     * 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.
> +     */
> +    g_array_sort(sdev.idmaps, iort_idmap_compare);

I'd add a blank line here to make it more clear the comment only (I think)
applies to the one line of code and not this whole block.

> +    sdev.base = vms->memmap[VIRT_SMMU].base;
> +    sdev.irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> +    g_array_append_val(sdev_blob, sdev);
> +}

Re: [PATCH v4 3/7] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Jonathan Cameron via]

On Fri, 13 Jun 2025 15:44:45 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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.
> 
> An example IORT Id mappings for a Qemu virt machine with two
> PCIe Root Complexes each assocaited with a SMMUv3 will
> be something like below,
> 
>   -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.0
>   -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1
>   ...
> 
>   +--------------------+           +--------------------+
>   |   Root Complex 0   |           |   Root Complex 1   |
>   |                    |           |                    |
>   |  Requestor IDs     |           |  Requestor IDs     |
>   |  0x0000 - 0x00FF   |           |  0x0100 - 0x01FF   |
>   +---------+----------+           +---------+----------+
>             |                               |
>             |                               |
>             |       Stream ID Mapping       |
>             v                               v
>   +--------------------+          +--------------------+
>   |    SMMUv3 Node 0   |          |    SMMUv3 Node 1   |
>   |                    |          |                    |
>   | Stream IDs 0x0000- |          | Stream IDs 0x0100- |
>   | 0x00FF mapped from |          | 0x01FF mapped from |
>   | RC0 Requestor IDs  |          | RC1 Requestor IDs  |
>   +--------------------+          +--------------------+
>             |                                |
>             |                                |
>             +----------------+---------------+
>                              |
>                              |Device ID Mapping
>                              v
>               +----------------------------+
>               |       ITS Node 0           |
>               |                            |
>               | Device IDs:                |
>               | 0x0000 - 0x00FF (from RC0) |
>               | 0x0100 - 0x01FF (from RC1) |
>               | 0x0200 - 0xFFFF (No SMMU)  |
>               +----------------------------+
> 
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Seems fine to me.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Other than needs a bios table test :)

> ---
>  hw/arm/virt-acpi-build.c | 55 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index d39506179a..72b79100ce 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"
> @@ -296,6 +297,58 @@ populate_smmuv3_legacy_dev(GArray *sdev_blob)
>      g_array_append_val(sdev_blob, sdev);
>  }
>  
> +static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer b)
> +{
> +    AcpiIortSMMUv3Dev *sdev_a = (AcpiIortSMMUv3Dev *)a;
> +    AcpiIortSMMUv3Dev *sdev_b = (AcpiIortSMMUv3Dev *)b;
> +    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->idmaps,
> +                                              AcpiIortIdMapping, 0);
> +    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->idmaps,
> +                                              AcpiIortIdMapping, 0);
> +    return map_a->input_base - map_b->input_base;
> +}
> +
> +static int iort_smmuv3_devices(Object *obj, void *opaque)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> +    GArray *sdev_blob = opaque;
> +    AcpiIortIdMapping idmap;
> +    PlatformBusDevice *pbus;
> +    AcpiIortSMMUv3Dev sdev;
> +    int min_bus, max_bus;
> +    SysBusDevice *sbdev;
> +    PCIBus *bus;
> +
> +    if (!object_dynamic_cast(obj, TYPE_ARM_SMMUV3)) {
> +        return 0;
> +    }
> +
> +    bus = PCI_BUS(object_property_get_link(obj, "primary-bus", &error_abort));
> +    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.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.idmaps, idmap);
> +    g_array_append_val(sdev_blob, sdev);
> +    return 0;
> +}

Re: [PATCH v4 4/7] hw/arm/virt: Factor out common SMMUV3 dt bindings code

[Jonathan Cameron via]

On Fri, 13 Jun 2025 15:44:46 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> No functional changes intended. This will be useful when we
> add support for user-creatable smmuv3 device.
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH v4 5/7] hw/arm/virt: Add an SMMU_IO_LEN macro

[Jonathan Cameron via]

On Fri, 13 Jun 2025 15:44:47 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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>
> Reviewed-by: Donald Dutile <ddutile@redhat.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Nathan Chen <nathanc@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 71b923f786..eeace4754d 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -146,6 +146,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.
> @@ -177,7 +180,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 },

This bit is fine.

>      [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},
> @@ -1447,7 +1450,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) {
> @@ -1467,7 +1469,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);

This seems a little odd.  I guess it may make sense later, but for all other
cases we only use the macros to define the base_memmap.

Perhaps delay this change until it is more obvious why?


>  }
>  
>  static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)

Re: [PATCH v4 6/7] hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation

[Jonathan Cameron via]

On Fri, 13 Jun 2025 15:44:48 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> 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.
> 
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Looks good to me. 
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  hw/arm/smmuv3.c      |  2 ++
>  hw/arm/virt.c        | 47 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/core/sysbus-fdt.c |  3 +++
>  3 files changed, 52 insertions(+)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index ab67972353..bcf8af8dc7 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1996,6 +1996,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->user_creatable = true;
>  }
>  
>  static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index eeace4754d..3be007d87c 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"
> @@ -1443,6 +1444,28 @@ static void create_smmuv3_dt_bindings(const VirtMachineState *vms, hwaddr base,
>      g_free(node);
>  }
>  
> +static void create_smmuv3_dev_dtb(VirtMachineState *vms,
> +                                  DeviceState *dev, PCIBus *bus)
> +{
> +    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);
> +
> +    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)
>  {
> @@ -2931,6 +2954,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");
> +        }
>      }
>  }
>  
> @@ -2954,6 +2984,22 @@ 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) {
> +            PCIBus *bus;
> +
> +            bus = PCI_BUS(object_property_get_link(OBJECT(dev), "primary-bus",
> +                                                   &error_abort));
> +            if (pci_bus_bypass_iommu(bus)) {
> +                error_setg(errp, "Bypass option cannot be set for SMMUv3 "
> +                           "associated PCIe RC");
> +                return;
> +            }
> +
> +            create_smmuv3_dev_dtb(vms, dev, bus);
> +        }
> +    }
> +

Re: [PATCH v4 7/7] qemu-options.hx: Document the arm-smmuv3 device

[Jonathan Cameron via]

On Fri, 13 Jun 2025 15:44:49 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> Now that arm,virt can have user-creatable smmuv3 devices, document it.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  qemu-options.hx | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 7eb8e02b4b..3edbde45bb 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1226,6 +1226,12 @@ SRST
>      ``aw-bits=val`` (val between 32 and 64, default depends on machine)
>          This decides the address width of the IOVA address space.
>  
> +``-device arm-smmuv3,primary-bus=id``
> +    This is only supported by ``-machine virt`` (ARM).
> +
> +    ``primary-bus=id``
> +        The PCIe Root Complex to be associated with.

Hmm.  Root complex or host bridge?
I think an RC is allowed to have multiple heirarchy and hence multiple
host bridges. Figure 1.2 in the PCI spec. So my gut feeling is this
should be host bridge. 


> +
>  ERST
>  
>  DEF("name", HAS_ARG, QEMU_OPTION_name,

RE: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: Monday, June 16, 2025 11:20 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> <linuxarm@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
> Root Complex association
> 
> On Fri, 13 Jun 2025 15:44:43 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > Although this change does not affect functionality at present, it is
> 
> Patch title says PCIe.  This check is vs PCI host bridge.
> 
> No idea which one you wanted, but if it is PCIe needs to be
> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> I think.

Looking at the code in virt.c and virt-acpi-build.c , this should be 
TYPE_PCI_HOST_BRIDGE. I will change the documentation to 
refer to PCI host bridge.

Thanks,
Shameer

> 
> > required when we add support for user-creatable SMMUv3 devices in
> > future patches.
> >
> > Tested-by: Nathan Chen <nathanc@nvidia.com>
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/smmu-common.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> > index f39b99e526..7890aa12c1 100644
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -20,6 +20,7 @@
> >  #include "trace.h"
> >  #include "exec/target_page.h"
> >  #include "hw/core/cpu.h"
> > +#include "hw/pci/pci_bridge.h"
> >  #include "hw/qdev-properties.h"
> >  #include "qapi/error.h"
> >  #include "qemu/jhash.h"
> > @@ -937,7 +938,8 @@ static void smmu_base_realize(DeviceState *dev,
> Error **errp)
> >                                       g_free, g_free);
> >      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
> >
> > -    if (s->primary_bus) {
> > +    if (s->primary_bus && object_dynamic_cast(OBJECT(s->primary_bus)-
> >parent,
> > +                                              TYPE_PCI_HOST_BRIDGE)) {
> >          pci_setup_iommu(s->primary_bus, &smmu_ops, s);
> >      } else {
> >          error_setg(errp, "SMMU is not attached to any PCI bus!");

RE: [PATCH v4 3/7] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: Monday, June 16, 2025 11:55 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> <linuxarm@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v4 3/7] hw/arm/virt-acpi-build: Update IORT for
> multiple smmuv3 devices
> 
> On Fri, 13 Jun 2025 15:44:45 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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.
> >
> > An example IORT Id mappings for a Qemu virt machine with two
> > PCIe Root Complexes each assocaited with a SMMUv3 will
> > be something like below,
> >
> >   -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.0
> >   -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1
> >   ...
> >
> >   +--------------------+           +--------------------+
> >   |   Root Complex 0   |           |   Root Complex 1   |
> >   |                    |           |                    |
> >   |  Requestor IDs     |           |  Requestor IDs     |
> >   |  0x0000 - 0x00FF   |           |  0x0100 - 0x01FF   |
> >   +---------+----------+           +---------+----------+
> >             |                               |
> >             |                               |
> >             |       Stream ID Mapping       |
> >             v                               v
> >   +--------------------+          +--------------------+
> >   |    SMMUv3 Node 0   |          |    SMMUv3 Node 1   |
> >   |                    |          |                    |
> >   | Stream IDs 0x0000- |          | Stream IDs 0x0100- |
> >   | 0x00FF mapped from |          | 0x01FF mapped from |
> >   | RC0 Requestor IDs  |          | RC1 Requestor IDs  |
> >   +--------------------+          +--------------------+
> >             |                                |
> >             |                                |
> >             +----------------+---------------+
> >                              |
> >                              |Device ID Mapping
> >                              v
> >               +----------------------------+
> >               |       ITS Node 0           |
> >               |                            |
> >               | Device IDs:                |
> >               | 0x0000 - 0x00FF (from RC0) |
> >               | 0x0100 - 0x01FF (from RC1) |
> >               | 0x0200 - 0xFFFF (No SMMU)  |
> >               +----------------------------+
> >
> > Tested-by: Nathan Chen <nathanc@nvidia.com>
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> Seems fine to me.
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> 
> Other than needs a bios table test :)

Sure, Will add one.

Thanks,
Shameer

RE: [PATCH v4 7/7] qemu-options.hx: Document the arm-smmuv3 device

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: Monday, June 16, 2025 12:13 PM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> <linuxarm@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v4 7/7] qemu-options.hx: Document the arm-smmuv3
> device
> 
> On Fri, 13 Jun 2025 15:44:49 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > Now that arm,virt can have user-creatable smmuv3 devices, document it.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  qemu-options.hx | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 7eb8e02b4b..3edbde45bb 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1226,6 +1226,12 @@ SRST
> >      ``aw-bits=val`` (val between 32 and 64, default depends on machine)
> >          This decides the address width of the IOVA address space.
> >
> > +``-device arm-smmuv3,primary-bus=id``
> > +    This is only supported by ``-machine virt`` (ARM).
> > +
> > +    ``primary-bus=id``
> > +        The PCIe Root Complex to be associated with.
> 
> Hmm.  Root complex or host bridge?
> I think an RC is allowed to have multiple heirarchy and hence multiple
> host bridges. Figure 1.2 in the PCI spec. So my gut feeling is this
> should be host bridge.

Ok. I will change the documentation and other comments where it matters
in this series to host bridge.

Thanks,
Shameer

RE: [PATCH v4 5/7] hw/arm/virt: Add an SMMU_IO_LEN macro

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: Monday, June 16, 2025 12:02 PM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> <linuxarm@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v4 5/7] hw/arm/virt: Add an SMMU_IO_LEN macro
> 
> On Fri, 13 Jun 2025 15:44:47 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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>
> > Reviewed-by: Donald Dutile <ddutile@redhat.com>
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > Tested-by: Nathan Chen <nathanc@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 71b923f786..eeace4754d 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -146,6 +146,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.
> > @@ -177,7 +180,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 },
> 
> This bit is fine.
> 
> >      [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},
> > @@ -1447,7 +1450,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) {
> > @@ -1467,7 +1469,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);
> 
> This seems a little odd.  I guess it may make sense later, but for all other
> cases we only use the macros to define the base_memmap.
> 
> Perhaps delay this change until it is more obvious why?

Right, looking at this patch now, I think it is not necessary at the moment
to introduce this macro now. I will take another look and discard this
if it doesn't make sense.

Thanks,
Shameer

RE: [PATCH v4 6/7] hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Monday, June 16, 2025 6:31 AM
> 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; imammedo@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 v4 6/7] hw/arm/virt: Allow user-creatable SMMUv3 dev
> instantiation
> 
> On Fri, Jun 13, 2025 at 03:44:48PM +0100, Shameer Kolothum wrote:
> > 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.
> 
> I forgot the detail for this limitation. Maybe spare a few more
> words briefly describing why?

Qemu lacks support for presenting PCI device tree bindings for the primary
PCIe bus and PXB bus instances completely.

Please see the details here:
https://lore.kernel.org/qemu-devel/20230421165037.2506-1-Jonathan.Cameron@huawei.com/

I will add bit more description here to make it more clear.

> 
> > Tested-by: Nathan Chen <nathanc@nvidia.com>
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Thanks,
Shameer

Re: [PATCH v4 7/7] qemu-options.hx: Document the arm-smmuv3 device

[Donald Dutile]

On 6/16/25 7:12 AM, Jonathan Cameron wrote:
> On Fri, 13 Jun 2025 15:44:49 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
>> Now that arm,virt can have user-creatable smmuv3 devices, document it.
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>>   qemu-options.hx | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 7eb8e02b4b..3edbde45bb 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -1226,6 +1226,12 @@ SRST
>>       ``aw-bits=val`` (val between 32 and 64, default depends on machine)
>>           This decides the address width of the IOVA address space.
>>   
>> +``-device arm-smmuv3,primary-bus=id``
>> +    This is only supported by ``-machine virt`` (ARM).
>> +
>> +    ``primary-bus=id``
>> +        The PCIe Root Complex to be associated with.
> 
> Hmm.  Root complex or host bridge?
> I think an RC is allowed to have multiple heirarchy and hence multiple
> host bridges. Figure 1.2 in the PCI spec. So my gut feeling is this
> should be host bridge.
> 
+1.
the key word-hint: 'complex' -- a RP (a Root *Port*) can only host a single PCI(e) (sub-)tree,
but a RC can have multiple PCI domains, not to mention a bunch of platform-level,
acpi-defined PCI(e) devices.


> 
>> +
>>   ERST
>>   
>>   DEF("name", HAS_ARG, QEMU_OPTION_name,
> 

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Eric Auger]

On 6/16/25 12:20 PM, Jonathan Cameron wrote:
> On Fri, 13 Jun 2025 15:44:43 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
>> Although this change does not affect functionality at present, it is
> Patch title says PCIe.  This check is vs PCI host bridge.
>
> No idea which one you wanted, but if it is PCIe needs to be
> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> I think.
I think we need TYPE_PCI_HOST_BRIDGE as we want to check against pxb

pci-bridge/pci_expander_bridge.c:    .parent        = TYPE_PCI_HOST_BRIDGE,

Besides the commit title/desc suggested by Jonathan you may also refine
the error msg which can stem now from 2 different causes

Thanks

Eric
>
>> required when we add support for user-creatable SMMUv3 devices in
>> future patches.
>>
>> Tested-by: Nathan Chen <nathanc@nvidia.com>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>>  hw/arm/smmu-common.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>> index f39b99e526..7890aa12c1 100644
>> --- a/hw/arm/smmu-common.c
>> +++ b/hw/arm/smmu-common.c
>> @@ -20,6 +20,7 @@
>>  #include "trace.h"
>>  #include "exec/target_page.h"
>>  #include "hw/core/cpu.h"
>> +#include "hw/pci/pci_bridge.h"
>>  #include "hw/qdev-properties.h"
>>  #include "qapi/error.h"
>>  #include "qemu/jhash.h"
>> @@ -937,7 +938,8 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
>>                                       g_free, g_free);
>>      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
>>  
>> -    if (s->primary_bus) {
>> +    if (s->primary_bus && object_dynamic_cast(OBJECT(s->primary_bus)->parent,
>> +                                              TYPE_PCI_HOST_BRIDGE)) {
>>          pci_setup_iommu(s->primary_bus, &smmu_ops, s);
>>      } else {
>>          error_setg(errp, "SMMU is not attached to any PCI bus!");

Re: [PATCH v4 2/7] hw/arm/virt-acpi-build: Re-arrange SMMUv3 IORT build

[Eric Auger]

Hi Shameer,

Some cosmetic comments below.

On 6/13/25 4:44 PM, Shameer Kolothum wrote:
> Introduces a new struct AcpiIortSMMUv3Dev to hold all the information
Introduce
> required for SMMUv3 IORT node and use that for populating the node.
>
> The current machine wide SMMUv3 is named as legacy SMMUv3 as we will
> soon add support for user-creatable SMMUv3 devices. These changes will
> be useful to have common code paths when we add that support.
>
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 111 +++++++++++++++++++++++++++------------
>  hw/arm/virt.c            |   1 +
>  include/hw/arm/virt.h    |   1 +
>  3 files changed, 79 insertions(+), 34 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 7e8e0f0298..d39506179a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -266,6 +266,36 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
>      return idmap_a->input_base - idmap_b->input_base;
>  }
>  
> +struct AcpiIortSMMUv3Dev {
> +    int irq;
> +    hwaddr base;
> +    GArray *idmaps;
> +    /* Offset of the SMMUv3 IORT Node relative to the start of the IORT. */
remove "."?
> +    size_t offset;
> +};
> +typedef struct AcpiIortSMMUv3Dev AcpiIortSMMUv3Dev;
> +
> +static void
> +populate_smmuv3_legacy_dev(GArray *sdev_blob)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> +    AcpiIortSMMUv3Dev sdev;
> +
> +    sdev.idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> +    object_child_foreach_recursive(object_get_root(),
> +                                   iort_host_bridges, sdev.idmaps);
> +
> +    /*
> +     * There will be only one legacy SMMUv3 as it is a machine wide one.
there can be only one "legacy" machine wide SMMUv3?
> +     * And since it covers all the PCIe RCs in the machine, may have
nit: Remove And. I would slighly rephrase into
"Since it may cover several PCIe RCs ..."
Some may be bypassed.
> +     * multiple SMMUv3 idmaps. Sort it by input_base.
> +     */
> +    g_array_sort(sdev.idmaps, iort_idmap_compare);
> +    sdev.base = vms->memmap[VIRT_SMMU].base;
> +    sdev.irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> +    g_array_append_val(sdev_blob, sdev);
> +}
> +
>  /*
>   * Input Output Remapping Table (IORT)
>   * Conforms to "IO Remapping Table System Software on ARM Platforms",
> @@ -274,11 +304,12 @@ 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;
> +    AcpiIortSMMUv3Dev *sdev;
> +    size_t node_size;
> +    int num_smmus = 0;
>      uint32_t id = 0;
> -    GArray *smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> +    GArray *smmuv3_devs = g_array_new(false, true, sizeof(AcpiIortSMMUv3Dev));
>      GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
>  
>      AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
> @@ -286,28 +317,32 @@ 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};
> -
> -        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);
> +    nb_nodes = 2; /* RC, ITS */
> +    if (vms->legacy_smmuv3_present) {
> +        populate_smmuv3_legacy_dev(smmuv3_devs);
> +    }
>  
> +    num_smmus = smmuv3_devs->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++) {
> +            sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
> +            for (j = 0; j < sdev->idmaps->len; j++) {
> +                AcpiIortIdMapping *idmap = &g_array_index(sdev->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 +351,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 +375,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++) {
> +        sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
> +        int irq = sdev->irq;
>  
> -        smmu_offset = table_data->len - table.table_offset;
> +        sdev->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 +390,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, sdev->base, 8);
>          /* Flags */
>          build_append_int_noprefix(table_data, 1 /* COHACC Override */, 4);
>          build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> @@ -404,15 +439,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++) {
> +            sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
> +
> +            for (j = 0; j < sdev->idmaps->len; j++) {
> +                range = &g_array_index(sdev->idmaps, AcpiIortIdMapping, j);
> +                /* output IORT node is the smmuv3 node */
> +                build_iort_id_mapping(table_data, range->input_base,
> +                                      range->id_count, sdev->offset);
> +            }
>          }
>  
>          /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
> @@ -428,8 +467,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++) {
> +        sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
> +        g_array_free(sdev->idmaps, true);
> +    }
> +    g_array_free(smmuv3_devs, true);
>  }
>  
>  /*
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9a6cd085a3..73bd2bd5f2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1614,6 +1614,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 9a1b0f53d2..8b1404b5f6 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -174,6 +174,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)
Thanks

Eric

Re: [PATCH v4 3/7] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Eric Auger]

Hi Shameer,

On 6/13/25 4:44 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.
>
> An example IORT Id mappings for a Qemu virt machine with two
> PCIe Root Complexes each assocaited with a SMMUv3 will
> be something like below,
>
>   -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.0
>   -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1
>   ...
>
>   +--------------------+           +--------------------+
>   |   Root Complex 0   |           |   Root Complex 1   |
>   |                    |           |                    |
>   |  Requestor IDs     |           |  Requestor IDs     |
>   |  0x0000 - 0x00FF   |           |  0x0100 - 0x01FF   |
>   +---------+----------+           +---------+----------+
>             |                               |
>             |                               |
>             |       Stream ID Mapping       |
>             v                               v
>   +--------------------+          +--------------------+
>   |    SMMUv3 Node 0   |          |    SMMUv3 Node 1   |
>   |                    |          |                    |
>   | Stream IDs 0x0000- |          | Stream IDs 0x0100- |
>   | 0x00FF mapped from |          | 0x01FF mapped from |
>   | RC0 Requestor IDs  |          | RC1 Requestor IDs  |
>   +--------------------+          +--------------------+
>             |                                |
>             |                                |
>             +----------------+---------------+
>                              |
>                              |Device ID Mapping
>                              v
>               +----------------------------+
>               |       ITS Node 0           |
>               |                            |
>               | Device IDs:                |
>               | 0x0000 - 0x00FF (from RC0) |
>               | 0x0100 - 0x01FF (from RC1) |
>               | 0x0200 - 0xFFFF (No SMMU)  |
>               +----------------------------+
>
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 55 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index d39506179a..72b79100ce 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"
> @@ -296,6 +297,58 @@ populate_smmuv3_legacy_dev(GArray *sdev_blob)
>      g_array_append_val(sdev_blob, sdev);
>  }
>  
> +static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer b)
> +{
> +    AcpiIortSMMUv3Dev *sdev_a = (AcpiIortSMMUv3Dev *)a;
> +    AcpiIortSMMUv3Dev *sdev_b = (AcpiIortSMMUv3Dev *)b;
> +    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->idmaps,
> +                                              AcpiIortIdMapping, 0);
> +    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->idmaps,
> +                                              AcpiIortIdMapping, 0);
> +    return map_a->input_base - map_b->input_base;
> +}
> +
> +static int iort_smmuv3_devices(Object *obj, void *opaque)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> +    GArray *sdev_blob = opaque;
> +    AcpiIortIdMapping idmap;
> +    PlatformBusDevice *pbus;
> +    AcpiIortSMMUv3Dev sdev;
> +    int min_bus, max_bus;
> +    SysBusDevice *sbdev;
> +    PCIBus *bus;
> +
> +    if (!object_dynamic_cast(obj, TYPE_ARM_SMMUV3)) {
> +        return 0;
> +    }
> +
> +    bus = PCI_BUS(object_property_get_link(obj, "primary-bus", &error_abort));
> +    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.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.idmaps, idmap);
> +    g_array_append_val(sdev_blob, sdev);
> +    return 0;
> +}
> +
> +static void populate_smmuv3_dev(GArray *sdev_blob)
> +{
> +    object_child_foreach_recursive(object_get_root(),
> +                                   iort_smmuv3_devices, sdev_blob);
> +    /* Sort the smmuv3 devices(if any) by smmu idmap input_base */
> +    g_array_sort(sdev_blob, smmuv3_dev_idmap_compare);
> +}
> +
>  /*
>   * Input Output Remapping Table (IORT)
>   * Conforms to "IO Remapping Table System Software on ARM Platforms",
> @@ -320,6 +373,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      nb_nodes = 2; /* RC, ITS */
>      if (vms->legacy_smmuv3_present) {
>          populate_smmuv3_legacy_dev(smmuv3_devs);
> +    } else {
> +        populate_smmuv3_dev(smmuv3_devs);
>      }
>  
>      num_smmus = smmuv3_devs->len;
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

Re: [PATCH v4 6/7] hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation

[Eric Auger]

Hi Shameer,
On 6/13/25 4:44 PM, Shameer Kolothum wrote:
> 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.
>
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/arm/smmuv3.c      |  2 ++
>  hw/arm/virt.c        | 47 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/core/sysbus-fdt.c |  3 +++
>  3 files changed, 52 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index ab67972353..bcf8af8dc7 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1996,6 +1996,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->user_creatable = true;
>  }
>  
>  static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index eeace4754d..3be007d87c 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"
> @@ -1443,6 +1444,28 @@ static void create_smmuv3_dt_bindings(const VirtMachineState *vms, hwaddr base,
>      g_free(node);
>  }
>  
> +static void create_smmuv3_dev_dtb(VirtMachineState *vms,
> +                                  DeviceState *dev, PCIBus *bus)
> +{
> +    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);
> +
> +    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)
>  {
> @@ -2931,6 +2954,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");
> +        }
>      }
>  }
>  
> @@ -2954,6 +2984,22 @@ 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) {
> +            PCIBus *bus;
> +
> +            bus = PCI_BUS(object_property_get_link(OBJECT(dev), "primary-bus",
> +                                                   &error_abort));
> +            if (pci_bus_bypass_iommu(bus)) {
> +                error_setg(errp, "Bypass option cannot be set for SMMUv3 "
> +                           "associated PCIe RC");
> +                return;
> +            }
> +
> +            create_smmuv3_dev_dtb(vms, dev, bus);
> +        }
> +    }
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>          PCIDevice *pdev = PCI_DEVICE(dev);
>  
> @@ -3156,6 +3202,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..e80776080b 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"
> @@ -518,6 +519,8 @@ static const BindingEntry bindings[] = {
>  #ifdef CONFIG_TPM
>      TYPE_BINDING(TYPE_TPM_TIS_SYSBUS, add_tpm_tis_fdt_node),
>  #endif
> +    /* No generic DT support for smmuv3 dev. Support added for arm virt only */
> +    TYPE_BINDING(TYPE_ARM_SMMUV3, no_fdt_node),
>      TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
>      TYPE_BINDING(TYPE_UEFI_VARS_SYSBUS, add_uefi_vars_node),
>      TYPE_BINDING("", NULL), /* last element */

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Jonathan Cameron via]

On Tue, 17 Jun 2025 09:49:54 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> On 6/16/25 12:20 PM, Jonathan Cameron wrote:
> > On Fri, 13 Jun 2025 15:44:43 +0100
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >  
> >> Although this change does not affect functionality at present, it is  
> > Patch title says PCIe.  This check is vs PCI host bridge.
> >
> > No idea which one you wanted, but if it is PCIe needs to be
> > TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> > I think.  
> I think we need TYPE_PCI_HOST_BRIDGE as we want to check against pxb
> 
> pci-bridge/pci_expander_bridge.c:    .parent        = TYPE_PCI_HOST_BRIDGE,

Hmm. That's awkward and I'd forgotten that wrinkle.
Need a stronger test but which one?  The PXB root bus has a parent of
TYPE_PCIE_BUS.  Maybe we can check that?


> 
> Besides the commit title/desc suggested by Jonathan you may also refine
> the error msg which can stem now from 2 different causes
> 
> Thanks
> 
> Eric
> >  
> >> required when we add support for user-creatable SMMUv3 devices in
> >> future patches.
> >>
> >> Tested-by: Nathan Chen <nathanc@nvidia.com>
> >> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> >> ---
> >>  hw/arm/smmu-common.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> >> index f39b99e526..7890aa12c1 100644
> >> --- a/hw/arm/smmu-common.c
> >> +++ b/hw/arm/smmu-common.c
> >> @@ -20,6 +20,7 @@
> >>  #include "trace.h"
> >>  #include "exec/target_page.h"
> >>  #include "hw/core/cpu.h"
> >> +#include "hw/pci/pci_bridge.h"
> >>  #include "hw/qdev-properties.h"
> >>  #include "qapi/error.h"
> >>  #include "qemu/jhash.h"
> >> @@ -937,7 +938,8 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
> >>                                       g_free, g_free);
> >>      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
> >>  
> >> -    if (s->primary_bus) {
> >> +    if (s->primary_bus && object_dynamic_cast(OBJECT(s->primary_bus)->parent,
> >> +                                              TYPE_PCI_HOST_BRIDGE)) {
> >>          pci_setup_iommu(s->primary_bus, &smmu_ops, s);
> >>      } else {
> >>          error_setg(errp, "SMMU is not attached to any PCI bus!");  
> 
> 

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Donald Dutile]

On 6/17/25 12:52 PM, Jonathan Cameron wrote:
> On Tue, 17 Jun 2025 09:49:54 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> On 6/16/25 12:20 PM, Jonathan Cameron wrote:
>>> On Fri, 13 Jun 2025 15:44:43 +0100
>>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>>>   
>>>> Although this change does not affect functionality at present, it is
>>> Patch title says PCIe.  This check is vs PCI host bridge.
>>>
>>> No idea which one you wanted, but if it is PCIe needs to be
>>> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
>>> I think.
>> I think we need TYPE_PCI_HOST_BRIDGE as we want to check against pxb
>>
>> pci-bridge/pci_expander_bridge.c:    .parent        = TYPE_PCI_HOST_BRIDGE,
> 
> Hmm. That's awkward and I'd forgotten that wrinkle.
> Need a stronger test but which one?  The PXB root bus has a parent of
> TYPE_PCIE_BUS.  Maybe we can check that?
> 
Yeah, rooted in the problem that we should have pci-switches or pci-rp's,
not just 'pxb'.


> 
>>
>> Besides the commit title/desc suggested by Jonathan you may also refine
>> the error msg which can stem now from 2 different causes
>>
>> Thanks
>>
>> Eric
>>>   
>>>> required when we add support for user-creatable SMMUv3 devices in
>>>> future patches.
>>>>
>>>> Tested-by: Nathan Chen <nathanc@nvidia.com>
>>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>>> ---
>>>>   hw/arm/smmu-common.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>>>> index f39b99e526..7890aa12c1 100644
>>>> --- a/hw/arm/smmu-common.c
>>>> +++ b/hw/arm/smmu-common.c
>>>> @@ -20,6 +20,7 @@
>>>>   #include "trace.h"
>>>>   #include "exec/target_page.h"
>>>>   #include "hw/core/cpu.h"
>>>> +#include "hw/pci/pci_bridge.h"
>>>>   #include "hw/qdev-properties.h"
>>>>   #include "qapi/error.h"
>>>>   #include "qemu/jhash.h"
>>>> @@ -937,7 +938,8 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
>>>>                                        g_free, g_free);
>>>>       s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
>>>>   
>>>> -    if (s->primary_bus) {
>>>> +    if (s->primary_bus && object_dynamic_cast(OBJECT(s->primary_bus)->parent,
>>>> +                                              TYPE_PCI_HOST_BRIDGE)) {
>>>>           pci_setup_iommu(s->primary_bus, &smmu_ops, s);
>>>>       } else {
>>>>           error_setg(errp, "SMMU is not attached to any PCI bus!");
>>
>>
> 

RE: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: Tuesday, June 17, 2025 5:53 PM
> To: Eric Auger <eric.auger@redhat.com>
> Cc: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
> Root Complex association
> 
> On Tue, 17 Jun 2025 09:49:54 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
> > On 6/16/25 12:20 PM, Jonathan Cameron wrote:
> > > On Fri, 13 Jun 2025 15:44:43 +0100
> > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> > >
> > >> Although this change does not affect functionality at present, it is
> > > Patch title says PCIe.  This check is vs PCI host bridge.
> > >
> > > No idea which one you wanted, but if it is PCIe needs to be
> > > TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> > > I think.
> > I think we need TYPE_PCI_HOST_BRIDGE as we want to check against pxb
> >
> > pci-bridge/pci_expander_bridge.c:    .parent        =
> TYPE_PCI_HOST_BRIDGE,
> 
> Hmm. That's awkward and I'd forgotten that wrinkle.
> Need a stronger test but which one?  The PXB root bus has a parent of
> TYPE_PCIE_BUS.  Maybe we can check that?

Ok. How about we do something like below?


@@ -925,6 +926,7 @@ static void smmu_base_realize(DeviceState *dev,
Error **errp)
 {
     SMMUState *s = ARM_SMMU(dev);
     SMMUBaseClass *sbc = ARM_SMMU_GET_CLASS(dev);
+    PCIBus *pci_bus = s->primary_bus;
     Error *local_err = NULL;

     sbc->parent_realize(dev, &local_err);
@@ -937,10 +939,31 @@ static void smmu_base_realize(DeviceState *dev,
Error **errp)
                                      g_free, g_free);
     s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);

-    if (s->primary_bus) {
-        pci_setup_iommu(s->primary_bus, &smmu_ops, s);
-    } else {
+    if (!pci_bus) {
         error_setg(errp, "SMMU is not attached to any PCI bus!");
+        return;
+    }
+
+    /*
+     * We only allow default PCIe Root Complex(pcie.0) or pxb-pcie based extra
+     * root complexes to be associated with SMMU.
+     */
+    if (pci_bus_is_express(pci_bus) && pci_bus_is_root(pci_bus) &&
+        object_dynamic_cast(OBJECT(pci_bus)->parent, TYPE_PCI_HOST_BRIDGE)) {
+        /*
+         * For pxb-pcie, parent_dev will be set. Make sure it is
+         * pxb-pcie indeed.
+         */
+        if (pci_bus->parent_dev) {
+            if (!object_dynamic_cast(OBJECT(pci_bus), "pxb-pcie-bus")) {
+                error_setg(errp, "SMMU is not attached to pxb-pcie bus!");
+                return;
+            }
+        }
+        pci_setup_iommu(pci_bus, &smmu_ops, s);
+    } else {
+       error_setg(errp, "SMMU should be attached to a default PCIe
root complex"
+                  "(pcie.0) or a pxb-pcie based root complex");
     }
 }

Please let me know if this is good enough or not.

Thanks,
Shameer

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Jonathan Cameron via]

On Wed, 18 Jun 2025 09:35:35 +0100
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Sent: Tuesday, June 17, 2025 5:53 PM
> > To: Eric Auger <eric.auger@redhat.com>
> > Cc: Shameerali Kolothum Thodi
> > <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> > <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
> > Root Complex association
> > 
> > On Tue, 17 Jun 2025 09:49:54 +0200
> > Eric Auger <eric.auger@redhat.com> wrote:
> >   
> > > On 6/16/25 12:20 PM, Jonathan Cameron wrote:  
> > > > On Fri, 13 Jun 2025 15:44:43 +0100
> > > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> > > >  
> > > >> Although this change does not affect functionality at present, it is  
> > > > Patch title says PCIe.  This check is vs PCI host bridge.
> > > >
> > > > No idea which one you wanted, but if it is PCIe needs to be
> > > > TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> > > > I think.  
> > > I think we need TYPE_PCI_HOST_BRIDGE as we want to check against pxb
> > >
> > > pci-bridge/pci_expander_bridge.c:    .parent        =  
> > TYPE_PCI_HOST_BRIDGE,
> > 
> > Hmm. That's awkward and I'd forgotten that wrinkle.
> > Need a stronger test but which one?  The PXB root bus has a parent of
> > TYPE_PCIE_BUS.  Maybe we can check that?  
> 
> Ok. How about we do something like below?
> 
> 
> @@ -925,6 +926,7 @@ static void smmu_base_realize(DeviceState *dev,
> Error **errp)
>  {
>      SMMUState *s = ARM_SMMU(dev);
>      SMMUBaseClass *sbc = ARM_SMMU_GET_CLASS(dev);
> +    PCIBus *pci_bus = s->primary_bus;
>      Error *local_err = NULL;
> 
>      sbc->parent_realize(dev, &local_err);
> @@ -937,10 +939,31 @@ static void smmu_base_realize(DeviceState *dev,
> Error **errp)
>                                       g_free, g_free);
>      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
> 
> -    if (s->primary_bus) {
> -        pci_setup_iommu(s->primary_bus, &smmu_ops, s);
> -    } else {
> +    if (!pci_bus) {
>          error_setg(errp, "SMMU is not attached to any PCI bus!");
> +        return;
> +    }
> +
> +    /*
> +     * We only allow default PCIe Root Complex(pcie.0) or pxb-pcie based extra
> +     * root complexes to be associated with SMMU.
> +     */
> +    if (pci_bus_is_express(pci_bus) && pci_bus_is_root(pci_bus) &&
> +        object_dynamic_cast(OBJECT(pci_bus)->parent, TYPE_PCI_HOST_BRIDGE)) {
> +        /*
> +         * For pxb-pcie, parent_dev will be set. Make sure it is
> +         * pxb-pcie indeed.
> +         */
> +        if (pci_bus->parent_dev) {
> +            if (!object_dynamic_cast(OBJECT(pci_bus), "pxb-pcie-bus")) {
> +                error_setg(errp, "SMMU is not attached to pxb-pcie bus!");
> +                return;
> +            }
> +        }
> +        pci_setup_iommu(pci_bus, &smmu_ops, s);
> +    } else {
> +       error_setg(errp, "SMMU should be attached to a default PCIe
> root complex"
> +                  "(pcie.0) or a pxb-pcie based root complex");
>      }
>  }
> 
> Please let me know if this is good enough or not.
LGTM

> 
> Thanks,
> Shameer

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Eric Auger]

Hi Shameer, Jonathan,

On 6/18/25 10:35 AM, Shameerali Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Sent: Tuesday, June 17, 2025 5:53 PM
>> To: Eric Auger <eric.auger@redhat.com>
>> Cc: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
>> <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
>> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
>> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
>> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
>> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
>> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
>> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
>> Root Complex association
>>
>> On Tue, 17 Jun 2025 09:49:54 +0200
>> Eric Auger <eric.auger@redhat.com> wrote:
>>
>>> On 6/16/25 12:20 PM, Jonathan Cameron wrote:
>>>> On Fri, 13 Jun 2025 15:44:43 +0100
>>>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>>>>
>>>>> Although this change does not affect functionality at present, it is
>>>> Patch title says PCIe.  This check is vs PCI host bridge.
>>>>
>>>> No idea which one you wanted, but if it is PCIe needs to be
>>>> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
>>>> I think.
>>> I think we need TYPE_PCI_HOST_BRIDGE as we want to check against pxb
>>>
>>> pci-bridge/pci_expander_bridge.c:    .parent        =
>> TYPE_PCI_HOST_BRIDGE,
sorry but I still fail to understand why we can't just check against

TYPE_PCI_HOST_BRIDGE for making sure the SMMU is attached to PXB or GPEX. What does it fail to check? Why shall we care about PCI vs PCIe?

Thanks

Eric

>>
>> Hmm. That's awkward and I'd forgotten that wrinkle.
>> Need a stronger test but which one?  The PXB root bus has a parent of
>> TYPE_PCIE_BUS.  Maybe we can check that?
> Ok. How about we do something like below?
>
>
> @@ -925,6 +926,7 @@ static void smmu_base_realize(DeviceState *dev,
> Error **errp)
>  {
>      SMMUState *s = ARM_SMMU(dev);
>      SMMUBaseClass *sbc = ARM_SMMU_GET_CLASS(dev);
> +    PCIBus *pci_bus = s->primary_bus;
>      Error *local_err = NULL;
>
>      sbc->parent_realize(dev, &local_err);
> @@ -937,10 +939,31 @@ static void smmu_base_realize(DeviceState *dev,
> Error **errp)
>                                       g_free, g_free);
>      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
>
> -    if (s->primary_bus) {
> -        pci_setup_iommu(s->primary_bus, &smmu_ops, s);
> -    } else {
> +    if (!pci_bus) {
>          error_setg(errp, "SMMU is not attached to any PCI bus!");
> +        return;
> +    }
> +
> +    /*
> +     * We only allow default PCIe Root Complex(pcie.0) or pxb-pcie based extra
> +     * root complexes to be associated with SMMU.
> +     */
> +    if (pci_bus_is_express(pci_bus) && pci_bus_is_root(pci_bus) &&
> +        object_dynamic_cast(OBJECT(pci_bus)->parent, TYPE_PCI_HOST_BRIDGE)) {
> +        /*
> +         * For pxb-pcie, parent_dev will be set. Make sure it is
> +         * pxb-pcie indeed.
> +         */
> +        if (pci_bus->parent_dev) {
> +            if (!object_dynamic_cast(OBJECT(pci_bus), "pxb-pcie-bus")) {
> +                error_setg(errp, "SMMU is not attached to pxb-pcie bus!");
> +                return;
> +            }
> +        }
> +        pci_setup_iommu(pci_bus, &smmu_ops, s);
> +    } else {
> +       error_setg(errp, "SMMU should be attached to a default PCIe
> root complex"
> +                  "(pcie.0) or a pxb-pcie based root complex");
>      }
>  }
>
> Please let me know if this is good enough or not.
>
> Thanks,
> Shameer
>

RE: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Shameerali Kolothum Thodi via]

Hi Eric,

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Wednesday, June 18, 2025 6:00 PM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>
> Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
> Root Complex association
> 
> Hi Shameer, Jonathan,
> 
> On 6/18/25 10:35 AM, Shameerali Kolothum Thodi wrote:
> >
> >> -----Original Message-----
> >> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> >> Sent: Tuesday, June 17, 2025 5:53 PM
> >> To: Eric Auger <eric.auger@redhat.com>
> >> Cc: Shameerali Kolothum Thodi
> >> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> >> <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> >> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> >> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> >> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> >> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> >> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> >> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has
> PCIe
> >> Root Complex association
> >>
> >> On Tue, 17 Jun 2025 09:49:54 +0200
> >> Eric Auger <eric.auger@redhat.com> wrote:
> >>
> >>> On 6/16/25 12:20 PM, Jonathan Cameron wrote:
> >>>> On Fri, 13 Jun 2025 15:44:43 +0100
> >>>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >>>>
> >>>>> Although this change does not affect functionality at present, it is
> >>>> Patch title says PCIe.  This check is vs PCI host bridge.
> >>>>
> >>>> No idea which one you wanted, but if it is PCIe needs to be
> >>>> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> >>>> I think.
> >>> I think we need TYPE_PCI_HOST_BRIDGE as we want to check against
> pxb
> >>>
> >>> pci-bridge/pci_expander_bridge.c:    .parent        =
> >> TYPE_PCI_HOST_BRIDGE,
> sorry but I still fail to understand why we can't just check against
> 
> TYPE_PCI_HOST_BRIDGE for making sure the SMMU is attached to PXB or
> GPEX. What does it fail to check? Why shall we care about PCI vs PCIe?

I think the concern is  getting any other TYPE_PCI_HOST_BRIDGE types attached
to SMMUv3 other than pxb-pcie or GPEX. For example you could do,

-device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
-device arm-smmuv3,primary-bus=cxl.1,id=smmuv3.1 \

as pxb-cxl is of type TYPE_PCI_HOST_BRIDGE. I don't know if there are any other
ones similar to this out there.

So the aim is to make the checking more specific to PXB.

Thanks,
Shameer

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Eric Auger]

Hi Shameer,

On 6/19/25 9:24 AM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: Wednesday, June 18, 2025 6:00 PM
>> To: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
>> <jonathan.cameron@huawei.com>
>> Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
>> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
>> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
>> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
>> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
>> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
>> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
>> Root Complex association
>>
>> Hi Shameer, Jonathan,
>>
>> On 6/18/25 10:35 AM, Shameerali Kolothum Thodi wrote:
>>>> -----Original Message-----
>>>> From: Jonathan Cameron <jonathan.cameron@huawei.com>
>>>> Sent: Tuesday, June 17, 2025 5:53 PM
>>>> To: Eric Auger <eric.auger@redhat.com>
>>>> Cc: Shameerali Kolothum Thodi
>>>> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
>>>> <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
>>>> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
>>>> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
>>>> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
>>>> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
>>>> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
>>>> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has
>> PCIe
>>>> Root Complex association
>>>>
>>>> On Tue, 17 Jun 2025 09:49:54 +0200
>>>> Eric Auger <eric.auger@redhat.com> wrote:
>>>>
>>>>> On 6/16/25 12:20 PM, Jonathan Cameron wrote:
>>>>>> On Fri, 13 Jun 2025 15:44:43 +0100
>>>>>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>>>>>>
>>>>>>> Although this change does not affect functionality at present, it is
>>>>>> Patch title says PCIe.  This check is vs PCI host bridge.
>>>>>>
>>>>>> No idea which one you wanted, but if it is PCIe needs to be
>>>>>> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
>>>>>> I think.
>>>>> I think we need TYPE_PCI_HOST_BRIDGE as we want to check against
>> pxb
>>>>> pci-bridge/pci_expander_bridge.c:    .parent        =
>>>> TYPE_PCI_HOST_BRIDGE,
>> sorry but I still fail to understand why we can't just check against
>>
>> TYPE_PCI_HOST_BRIDGE for making sure the SMMU is attached to PXB or
>> GPEX. What does it fail to check? Why shall we care about PCI vs PCIe?
> I think the concern is  getting any other TYPE_PCI_HOST_BRIDGE types attached
> to SMMUv3 other than pxb-pcie or GPEX. For example you could do,
>
> -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
> -device arm-smmuv3,primary-bus=cxl.1,id=smmuv3.1 \
>
> as pxb-cxl is of type TYPE_PCI_HOST_BRIDGE. I don't know if there are any other
> ones similar to this out there.
>
> So the aim is to make the checking more specific to PXB.

thank you for the clarification. Is it invalid to have the SMMU
protecting RIDs comming from the pxb-cxl hierarchy?

Eric
>
> Thanks,
> Shameer

RE: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Thursday, June 19, 2025 8:41 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>
> Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
> Root Complex association
> 
> Hi Shameer,
> 
> On 6/19/25 9:24 AM, Shameerali Kolothum Thodi wrote:
> > Hi Eric,
> >
> >> -----Original Message-----
> >> From: Eric Auger <eric.auger@redhat.com>
> >> Sent: Wednesday, June 18, 2025 6:00 PM
> >> To: Shameerali Kolothum Thodi
> >> <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> >> <jonathan.cameron@huawei.com>
> >> Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> >> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> >> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> >> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> >> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> >> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> >> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has
> PCIe
> >> Root Complex association
> >>
> >> Hi Shameer, Jonathan,
> >>
> >> On 6/18/25 10:35 AM, Shameerali Kolothum Thodi wrote:
> >>>> -----Original Message-----
> >>>> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> >>>> Sent: Tuesday, June 17, 2025 5:53 PM
> >>>> To: Eric Auger <eric.auger@redhat.com>
> >>>> Cc: Shameerali Kolothum Thodi
> >>>> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> >>>> <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> >>>> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> >>>> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> >>>> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> >>>> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> >>>> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> >>>> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has
> >> PCIe
> >>>> Root Complex association
> >>>>
> >>>> On Tue, 17 Jun 2025 09:49:54 +0200
> >>>> Eric Auger <eric.auger@redhat.com> wrote:
> >>>>
> >>>>> On 6/16/25 12:20 PM, Jonathan Cameron wrote:
> >>>>>> On Fri, 13 Jun 2025 15:44:43 +0100
> >>>>>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> wrote:
> >>>>>>
> >>>>>>> Although this change does not affect functionality at present, it is
> >>>>>> Patch title says PCIe.  This check is vs PCI host bridge.
> >>>>>>
> >>>>>> No idea which one you wanted, but if it is PCIe needs to be
> >>>>>> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> >>>>>> I think.
> >>>>> I think we need TYPE_PCI_HOST_BRIDGE as we want to check against
> >> pxb
> >>>>> pci-bridge/pci_expander_bridge.c:    .parent        =
> >>>> TYPE_PCI_HOST_BRIDGE,
> >> sorry but I still fail to understand why we can't just check against
> >>
> >> TYPE_PCI_HOST_BRIDGE for making sure the SMMU is attached to PXB or
> >> GPEX. What does it fail to check? Why shall we care about PCI vs PCIe?
> > I think the concern is  getting any other TYPE_PCI_HOST_BRIDGE types
> attached
> > to SMMUv3 other than pxb-pcie or GPEX. For example you could do,
> >
> > -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
> > -device arm-smmuv3,primary-bus=cxl.1,id=smmuv3.1 \
> >
> > as pxb-cxl is of type TYPE_PCI_HOST_BRIDGE. I don't know if there are any
> other
> > ones similar to this out there.
> >
> > So the aim is to make the checking more specific to PXB.
> 
> thank you for the clarification. Is it invalid to have the SMMU
> protecting RIDs comming from the pxb-cxl hierarchy?

That’s a good question. I don't know that for sure.
Anyway currently the full support for CXL on virt is in progress here,
https://lore.kernel.org/qemu-devel/20250612134338.1871023-1-Jonathan.Cameron@huawei.com/

Jonathan?

Thanks,
Shameer

> 

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Jonathan Cameron via]

On Thu, 19 Jun 2025 09:05:07 +0100
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> > -----Original Message-----
> > From: Eric Auger <eric.auger@redhat.com>
> > Sent: Thursday, June 19, 2025 8:41 AM
> > To: Shameerali Kolothum Thodi
> > <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>
> > Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
> > Root Complex association
> > 
> > Hi Shameer,
> > 
> > On 6/19/25 9:24 AM, Shameerali Kolothum Thodi wrote:  
> > > Hi Eric,
> > >  
> > >> -----Original Message-----
> > >> From: Eric Auger <eric.auger@redhat.com>
> > >> Sent: Wednesday, June 18, 2025 6:00 PM
> > >> To: Shameerali Kolothum Thodi
> > >> <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> > >> <jonathan.cameron@huawei.com>
> > >> Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > >> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > >> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > >> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > >> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > >> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > >> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has  
> > PCIe  
> > >> Root Complex association
> > >>
> > >> Hi Shameer, Jonathan,
> > >>
> > >> On 6/18/25 10:35 AM, Shameerali Kolothum Thodi wrote:  
> > >>>> -----Original Message-----
> > >>>> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> > >>>> Sent: Tuesday, June 17, 2025 5:53 PM
> > >>>> To: Eric Auger <eric.auger@redhat.com>
> > >>>> Cc: Shameerali Kolothum Thodi
> > >>>> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> > >>>> <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > >>>> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > >>>> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > >>>> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > >>>> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > >>>> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > >>>> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has  
> > >> PCIe  
> > >>>> Root Complex association
> > >>>>
> > >>>> On Tue, 17 Jun 2025 09:49:54 +0200
> > >>>> Eric Auger <eric.auger@redhat.com> wrote:
> > >>>>  
> > >>>>> On 6/16/25 12:20 PM, Jonathan Cameron wrote:  
> > >>>>>> On Fri, 13 Jun 2025 15:44:43 +0100
> > >>>>>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>  
> > wrote:  
> > >>>>>>  
> > >>>>>>> Although this change does not affect functionality at present, it is  
> > >>>>>> Patch title says PCIe.  This check is vs PCI host bridge.
> > >>>>>>
> > >>>>>> No idea which one you wanted, but if it is PCIe needs to be
> > >>>>>> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> > >>>>>> I think.  
> > >>>>> I think we need TYPE_PCI_HOST_BRIDGE as we want to check against  
> > >> pxb  
> > >>>>> pci-bridge/pci_expander_bridge.c:    .parent        =  
> > >>>> TYPE_PCI_HOST_BRIDGE,  
> > >> sorry but I still fail to understand why we can't just check against
> > >>
> > >> TYPE_PCI_HOST_BRIDGE for making sure the SMMU is attached to PXB or
> > >> GPEX. What does it fail to check? Why shall we care about PCI vs PCIe?  
> > > I think the concern is  getting any other TYPE_PCI_HOST_BRIDGE types  
> > attached  
> > > to SMMUv3 other than pxb-pcie or GPEX. For example you could do,
> > >
> > > -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
> > > -device arm-smmuv3,primary-bus=cxl.1,id=smmuv3.1 \
> > >
> > > as pxb-cxl is of type TYPE_PCI_HOST_BRIDGE. I don't know if there are any  
> > other  
> > > ones similar to this out there.
> > >
> > > So the aim is to make the checking more specific to PXB.  
> > 
> > thank you for the clarification. Is it invalid to have the SMMU
> > protecting RIDs comming from the pxb-cxl hierarchy?  
> 
> That’s a good question. I don't know that for sure.

It should be fine to support CXL for this but we can work that out later.

For now limited use cases as there is no CXL VFIO support and the only thing
emulated devices do that the SMMU might influence is MSIX.

The one that concerned me is pxb-pci if we only care about pcie.
I'm not sure if we need to make that distinction or not.  

Jonathan

> Anyway currently the full support for CXL on virt is in progress here,
> https://lore.kernel.org/qemu-devel/20250612134338.1871023-1-Jonathan.Cameron@huawei.com/
> 
> Jonathan?
> 
> Thanks,
> Shameer
> 
> >   
> 

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Jonathan Cameron via]

On Thu, 19 Jun 2025 10:30:28 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Thu, 19 Jun 2025 09:05:07 +0100
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > > -----Original Message-----
> > > From: Eric Auger <eric.auger@redhat.com>
> > > Sent: Thursday, June 19, 2025 8:41 AM
> > > To: Shameerali Kolothum Thodi
> > > <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> > > <jonathan.cameron@huawei.com>
> > > Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > > devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > > nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > > imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > > smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > > jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > > Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
> > > Root Complex association
> > > 
> > > Hi Shameer,
> > > 
> > > On 6/19/25 9:24 AM, Shameerali Kolothum Thodi wrote:    
> > > > Hi Eric,
> > > >    
> > > >> -----Original Message-----
> > > >> From: Eric Auger <eric.auger@redhat.com>
> > > >> Sent: Wednesday, June 18, 2025 6:00 PM
> > > >> To: Shameerali Kolothum Thodi
> > > >> <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> > > >> <jonathan.cameron@huawei.com>
> > > >> Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > > >> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > > >> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > > >> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > > >> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > > >> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > > >> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has    
> > > PCIe    
> > > >> Root Complex association
> > > >>
> > > >> Hi Shameer, Jonathan,
> > > >>
> > > >> On 6/18/25 10:35 AM, Shameerali Kolothum Thodi wrote:    
> > > >>>> -----Original Message-----
> > > >>>> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> > > >>>> Sent: Tuesday, June 17, 2025 5:53 PM
> > > >>>> To: Eric Auger <eric.auger@redhat.com>
> > > >>>> Cc: Shameerali Kolothum Thodi
> > > >>>> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> > > >>>> <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > > >>>> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > > >>>> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > > >>>> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > > >>>> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > > >>>> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > > >>>> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has    
> > > >> PCIe    
> > > >>>> Root Complex association
> > > >>>>
> > > >>>> On Tue, 17 Jun 2025 09:49:54 +0200
> > > >>>> Eric Auger <eric.auger@redhat.com> wrote:
> > > >>>>    
> > > >>>>> On 6/16/25 12:20 PM, Jonathan Cameron wrote:    
> > > >>>>>> On Fri, 13 Jun 2025 15:44:43 +0100
> > > >>>>>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>    
> > > wrote:    
> > > >>>>>>    
> > > >>>>>>> Although this change does not affect functionality at present, it is    
> > > >>>>>> Patch title says PCIe.  This check is vs PCI host bridge.
> > > >>>>>>
> > > >>>>>> No idea which one you wanted, but if it is PCIe needs to be
> > > >>>>>> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> > > >>>>>> I think.    
> > > >>>>> I think we need TYPE_PCI_HOST_BRIDGE as we want to check against    
> > > >> pxb    
> > > >>>>> pci-bridge/pci_expander_bridge.c:    .parent        =    
> > > >>>> TYPE_PCI_HOST_BRIDGE,    
> > > >> sorry but I still fail to understand why we can't just check against
> > > >>
> > > >> TYPE_PCI_HOST_BRIDGE for making sure the SMMU is attached to PXB or
> > > >> GPEX. What does it fail to check? Why shall we care about PCI vs PCIe?    
> > > > I think the concern is  getting any other TYPE_PCI_HOST_BRIDGE types    
> > > attached    
> > > > to SMMUv3 other than pxb-pcie or GPEX. For example you could do,
> > > >
> > > > -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
> > > > -device arm-smmuv3,primary-bus=cxl.1,id=smmuv3.1 \
> > > >
> > > > as pxb-cxl is of type TYPE_PCI_HOST_BRIDGE. I don't know if there are any    
> > > other    
> > > > ones similar to this out there.
> > > >
> > > > So the aim is to make the checking more specific to PXB.    
> > > 
> > > thank you for the clarification. Is it invalid to have the SMMU
> > > protecting RIDs comming from the pxb-cxl hierarchy?    
> > 
> > That’s a good question. I don't know that for sure.  
> 
> It should be fine to support CXL for this but we can work that out later.
> 
> For now limited use cases as there is no CXL VFIO support and the only thing
> emulated devices do that the SMMU might influence is MSIX.
> 
> The one that concerned me is pxb-pci if we only care about pcie.
> I'm not sure if we need to make that distinction or not.  
> 
Hmm. Shameer pointed out I hallucinated the existence of a PCI only expander bridge.
So ignore that.

> Jonathan
> 
> > Anyway currently the full support for CXL on virt is in progress here,
> > https://lore.kernel.org/qemu-devel/20250612134338.1871023-1-Jonathan.Cameron@huawei.com/
> > 
> > Jonathan?
> > 
> > Thanks,
> > Shameer
> >   
> > >     
> >   
> 

Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Jonathan Cameron via]

On Thu, 19 Jun 2025 10:38:02 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Thu, 19 Jun 2025 10:30:28 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Thu, 19 Jun 2025 09:05:07 +0100
> > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Eric Auger <eric.auger@redhat.com>
> > > > Sent: Thursday, June 19, 2025 8:41 AM
> > > > To: Shameerali Kolothum Thodi
> > > > <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> > > > <jonathan.cameron@huawei.com>
> > > > Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > > > devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > > > nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > > > imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > > > smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > > > jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > > > Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
> > > > Root Complex association
> > > > 
> > > > Hi Shameer,
> > > > 
> > > > On 6/19/25 9:24 AM, Shameerali Kolothum Thodi wrote:      
> > > > > Hi Eric,
> > > > >      
> > > > >> -----Original Message-----
> > > > >> From: Eric Auger <eric.auger@redhat.com>
> > > > >> Sent: Wednesday, June 18, 2025 6:00 PM
> > > > >> To: Shameerali Kolothum Thodi
> > > > >> <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> > > > >> <jonathan.cameron@huawei.com>
> > > > >> Cc: Linuxarm <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > > > >> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > > > >> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > > > >> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > > > >> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > > > >> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > > > >> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has      
> > > > PCIe      
> > > > >> Root Complex association
> > > > >>
> > > > >> Hi Shameer, Jonathan,
> > > > >>
> > > > >> On 6/18/25 10:35 AM, Shameerali Kolothum Thodi wrote:      
> > > > >>>> -----Original Message-----
> > > > >>>> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> > > > >>>> Sent: Tuesday, June 17, 2025 5:53 PM
> > > > >>>> To: Eric Auger <eric.auger@redhat.com>
> > > > >>>> Cc: Shameerali Kolothum Thodi
> > > > >>>> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> > > > >>>> <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> > > > >>>> devel@nongnu.org; peter.maydell@linaro.org; jgg@nvidia.com;
> > > > >>>> nicolinc@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> > > > >>>> imammedo@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> > > > >>>> smostafa@google.com; Wangzhou (B) <wangzhou1@hisilicon.com>;
> > > > >>>> jiangkunkun <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org
> > > > >>>> Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has      
> > > > >> PCIe      
> > > > >>>> Root Complex association
> > > > >>>>
> > > > >>>> On Tue, 17 Jun 2025 09:49:54 +0200
> > > > >>>> Eric Auger <eric.auger@redhat.com> wrote:
> > > > >>>>      
> > > > >>>>> On 6/16/25 12:20 PM, Jonathan Cameron wrote:      
> > > > >>>>>> On Fri, 13 Jun 2025 15:44:43 +0100
> > > > >>>>>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>      
> > > > wrote:      
> > > > >>>>>>      
> > > > >>>>>>> Although this change does not affect functionality at present, it is      
> > > > >>>>>> Patch title says PCIe.  This check is vs PCI host bridge.
> > > > >>>>>>
> > > > >>>>>> No idea which one you wanted, but if it is PCIe needs to be
> > > > >>>>>> TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> > > > >>>>>> I think.      
> > > > >>>>> I think we need TYPE_PCI_HOST_BRIDGE as we want to check against      
> > > > >> pxb      
> > > > >>>>> pci-bridge/pci_expander_bridge.c:    .parent        =      
> > > > >>>> TYPE_PCI_HOST_BRIDGE,      
> > > > >> sorry but I still fail to understand why we can't just check against
> > > > >>
> > > > >> TYPE_PCI_HOST_BRIDGE for making sure the SMMU is attached to PXB or
> > > > >> GPEX. What does it fail to check? Why shall we care about PCI vs PCIe?      
> > > > > I think the concern is  getting any other TYPE_PCI_HOST_BRIDGE types      
> > > > attached      
> > > > > to SMMUv3 other than pxb-pcie or GPEX. For example you could do,
> > > > >
> > > > > -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
> > > > > -device arm-smmuv3,primary-bus=cxl.1,id=smmuv3.1 \
> > > > >
> > > > > as pxb-cxl is of type TYPE_PCI_HOST_BRIDGE. I don't know if there are any      
> > > > other      
> > > > > ones similar to this out there.
> > > > >
> > > > > So the aim is to make the checking more specific to PXB.      
> > > > 
> > > > thank you for the clarification. Is it invalid to have the SMMU
> > > > protecting RIDs comming from the pxb-cxl hierarchy?      
> > > 
> > > That’s a good question. I don't know that for sure.    
> > 
> > It should be fine to support CXL for this but we can work that out later.
> > 
> > For now limited use cases as there is no CXL VFIO support and the only thing
> > emulated devices do that the SMMU might influence is MSIX.
> > 
> > The one that concerned me is pxb-pci if we only care about pcie.
> > I'm not sure if we need to make that distinction or not.  
> >   
> Hmm. Shameer pointed out I hallucinated the existence of a PCI only expander bridge.
> So ignore that.
After some more discussions... 

Ah but it does - just called "pxb" - but it's irrelevant anyway as you can only add
them to a PCI root bridge (Shameer tested with a PCIe to PCI bridge with a pxb below
it and it rejects that config).  So can't plug them into virt anyway.

That's just for the record if I forget the whole discussion in future ;)

Jonathan

> 
> > Jonathan
> >   
> > > Anyway currently the full support for CXL on virt is in progress here,
> > > https://lore.kernel.org/qemu-devel/20250612134338.1871023-1-Jonathan.Cameron@huawei.com/
> > > 
> > > Jonathan?
> > > 
> > > Thanks,
> > > Shameer
> > >     
> > > >       
> > >     
> >   
>