[PATCH v7 00/12] hw/arm/virt: Add support for user creatable SMMUv3 device

[PATCH v7 00/12] hw/arm/virt: Add support for user creatable SMMUv3 device

[Shameer Kolothum via]

Hi All,

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

1. Fixed the warning case for DT support, reported by Eric(patch #8).
2. Picked up R-by's and T-by's. Thanks!

Please take a look and let me know. I think this is in a good shape now
for 10.1.

Thanks,
Shameer

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

1. Rebased to target-arm.next and resolved conflicts with the series 
   [PATCH-for-10.1 v6 0/9] hw/arm: GIC 'its=off'.
2. While at it, noticed an issue with RC id mappings creation
   and patch #1 is a fix for that.
3. Patches 3 and 4 have changes because of the conflict resolution with the
   above series. I have retained the R-by tags, but encourage all to take
   another look in case I missed anything.
4. Collected R-by and T-by tags. Thanks!.

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

Major changes from v4:

1. Added stricter validation for PCI buses associated with the SMMU.
   Only the default PCIe Root Complex (pcie.0) and additional root
   complexes created using pxb-pcie (see patch #1) are allowed.

2. While testing this series with a setup involving multiple PCIe root
   complexes using pxb-pcie, I encountered an issue related to IOMMU
   ops resolution. Consider the below configuration, where an
   arm-smmuv3 device is associated with the default root complex pcie.0,
   and an additional pxb-pcie-based root complex (pcie.1) is added
   without any associated SMMU:

   -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \
   ...
   -device pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \
   -device pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \
   -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \
   ...
   -device virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \
   -device virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1

   The guest boots fine, and virtionet.0(behind the SMMUV3) bring up
   is successful. However, attempting to bring up virtionet.1
   (behind pcie.1, which lacks a connected SMMU) results in a failure:

   root@ubuntu:/# dhclient enp9s0
   arm-smmu-v3 arm-smmu-v3.0.auto: event 0x02 received:
   arm-smmu-v3 arm-smmu-v3.0.auto: 0x0000090000000002
   arm-smmu-v3 arm-smmu-v3.0.auto: 0x0000000000000000
   arm-smmu-v3 arm-smmu-v3.0.auto: 0x0000000000000000
   arm-smmu-v3 arm-smmu-v3.0.auto: 0x0000000000000000
   arm-smmu-v3 arm-smmu-v3.0.auto: event: C_BAD_STREAMID client: (unassigned sid) sid: 0x900 ssid: 0x0
   virtio_net virtio1 enp9s0: NETDEV WATCHDOG: CPU: 2: transmit queue 0 timed out 5172 ms
   virtio_net virtio1 enp9s0: TX timeout on queue: 0, sq: output.0, vq: 0x1, name: output.0, 5172000 usecs ago
   ...

   Debug shows that QEMU currently registers IOMMU ops for bus using
   pci_setup_iommu(). However, when retrieving IOMMU ops for a device
   via pci_device_get_iommu_bus_devfn(), the function walks up to the 
   parent_dev and fetches the IOMMU ops from the parent, even if the
   current root bus has none configured.

   This works today because existing IOMMU models in QEMU are globally 
   scoped, and pxb-pcie based extra root complexes can use the
   bypass_iommu property to skip translation as needed.

   However, with this series introducing support for associating
   arm-smmuv3 devices with specific PCIe root complexes, this
   becomes problematic. In QEMU, pxb-pcie is implemented as a synthetic
   root complex whose parent_dev is pcie.0. As a result, even though
   pcie.1 has no SMMU attached, pci_device_get_iommu_bus_devfn()
   incorrectly returns the IOMMU ops associated with pcie.0 due to
   the fallback mechanism via parent_dev. This causes devices on
   pcie.1 to erroneously use the address space from pcie.0's SMMU,
   leading to failures like the one above.

   To address this, patch #6 in the series introduces a new helper 
   function pci_setup_iommu_per_bus(), which explicitly sets the 
   iommu_per_bus field in the PCIBus structure. This allows 
   pci_device_get_iommu_bus_devfn() to retrieve IOMMU ops based 
   on the specific bus.

   Not sure this is the correct approach or not. If there is a better
   way to handle this, please let me know .

3. Picked up few R-by tags where the patch content has not changed much.

4. Dropped T-by from Nathan for some patches as things have changed a bit.
   @Nathan, apprecaite if you have time to rerun the tests.

5. Added a bios table tests for both legacy SMMUv3 and new SMMMv3 devices.
   See last few patches.

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 (11):
  hw/arm/virt-acpi-build: Don't create ITS id mappings by default
  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/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops
    retrieval
  hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation
  qemu-options.hx: Document the arm-smmuv3 device
  bios-tables-test: Allow for smmuv3 test data.
  qtest/bios-tables-test: Add tests for legacy smmuv3 and smmuv3 device
  qtest/bios-tables-test: Update tables for smmuv3 tests

 hw/arm/smmu-common.c                          |  35 ++-
 hw/arm/smmuv3.c                               |   2 +
 hw/arm/virt-acpi-build.c                      | 207 +++++++++++++-----
 hw/arm/virt.c                                 | 111 +++++++---
 hw/core/sysbus-fdt.c                          |   3 +
 hw/pci-bridge/pci_expander_bridge.c           |   1 -
 hw/pci/pci.c                                  |  31 +++
 include/hw/arm/smmu-common.h                  |   1 +
 include/hw/arm/virt.h                         |   1 +
 include/hw/pci/pci.h                          |   2 +
 include/hw/pci/pci_bridge.h                   |   1 +
 include/hw/pci/pci_bus.h                      |   1 +
 qemu-options.hx                               |   7 +
 tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev  | Bin 0 -> 10162 bytes
 .../data/acpi/aarch64/virt/DSDT.smmuv3-legacy | Bin 0 -> 10162 bytes
 tests/data/acpi/aarch64/virt/IORT.smmuv3-dev  | Bin 0 -> 364 bytes
 .../data/acpi/aarch64/virt/IORT.smmuv3-legacy | Bin 0 -> 276 bytes
 tests/qtest/bios-tables-test.c                |  86 ++++++++
 18 files changed, 408 insertions(+), 81 deletions(-)
 create mode 100644 tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev
 create mode 100644 tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy
 create mode 100644 tests/data/acpi/aarch64/virt/IORT.smmuv3-dev
 create mode 100644 tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy

-- 
2.47.0

[PATCH v7 01/12] hw/arm/virt-acpi-build: Don't create ITS id mappings by default

[Shameer Kolothum via]

Commit d6afe18b7242 ("hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables
when its=off") moved ITS group node generation under the its=on condition.
However, it still creates rc_its_idmaps unconditionally, which results in
duplicate ID mappings in the IORT table.

Fixes:d6afe18b7242 ("hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off")
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Donald Dutile <ddutile@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt-acpi-build.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index cd90c47976..724fad5ffa 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -329,12 +329,6 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         /* Sort the smmu idmap by input_base */
         g_array_sort(rc_smmu_idmaps, iort_idmap_compare);
 
-        /*
-         * Knowing the ID ranges from the RC to the SMMU, it's possible to
-         * determine the ID ranges from RC that are directed to the ITS.
-         */
-        create_rc_its_idmaps(rc_its_idmaps, rc_smmu_idmaps);
-
         nb_nodes = 2; /* RC and SMMUv3 */
         rc_mapping_count = rc_smmu_idmaps->len;
 
-- 
2.47.0

[PATCH v7 02/12] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Shameer Kolothum via]

We only allow default PCIe Root Complex(pcie.0) or pxb-pcie based extra
root complexes to be associated with SMMU.

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

Note: Added a specific check to identify pxb-pcie to avoid matching
pxb-cxl host bridges, which are also of type PCI_HOST_BRIDGE. This
restriction can be relaxed once support for CXL devices on arm/virt
is added and validated with SMMUv3.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Nathan Chen <nathanc@nvidia.com>
Tested-by: Eric Auger <eric.auger@redhat.com> 
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/smmu-common.c                | 29 ++++++++++++++++++++++++++---
 hw/pci-bridge/pci_expander_bridge.c |  1 -
 include/hw/pci/pci_bridge.h         |  1 +
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index f39b99e526..b15e7fd0e4 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"
@@ -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,11 +939,32 @@ 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), TYPE_PXB_PCIE_BUS)) {
+                goto out_err;
+            }
+        }
+        pci_setup_iommu(pci_bus, &smmu_ops, s);
+        return;
     }
+out_err:
+    error_setg(errp, "SMMU should be attached to a default PCIe root complex"
+               "(pcie.0) or a pxb-pcie based root complex");
 }
 
 /*
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 3a29dfefc2..1bcceddbc4 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -34,7 +34,6 @@ typedef struct PXBBus PXBBus;
 DECLARE_INSTANCE_CHECKER(PXBBus, PXB_BUS,
                          TYPE_PXB_BUS)
 
-#define TYPE_PXB_PCIE_BUS "pxb-pcie-bus"
 DECLARE_INSTANCE_CHECKER(PXBBus, PXB_PCIE_BUS,
                          TYPE_PXB_PCIE_BUS)
 
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 8cdacbc4e1..a055fd8d32 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -104,6 +104,7 @@ typedef struct PXBPCIEDev {
     PXBDev parent_obj;
 } PXBPCIEDev;
 
+#define TYPE_PXB_PCIE_BUS "pxb-pcie-bus"
 #define TYPE_PXB_CXL_BUS "pxb-cxl-bus"
 #define TYPE_PXB_DEV "pxb"
 OBJECT_DECLARE_SIMPLE_TYPE(PXBDev, PXB_DEV)
-- 
2.47.0

[PATCH v7 03/12] hw/arm/virt-acpi-build: Re-arrange SMMUv3 IORT build

[Shameer Kolothum via]

Introduce 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>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt-acpi-build.c | 137 ++++++++++++++++++++++++++-------------
 hw/arm/virt.c            |   1 +
 include/hw/arm/virt.h    |   1 +
 3 files changed, 94 insertions(+), 45 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 724fad5ffa..cb11b316a2 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -266,29 +266,65 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
     return idmap_a->input_base - idmap_b->input_base;
 }
 
+typedef struct AcpiIortSMMUv3Dev {
+    int irq;
+    hwaddr base;
+    GArray *rc_smmu_idmaps;
+    /* Offset of the SMMUv3 IORT Node relative to the start of the IORT */
+    size_t offset;
+} AcpiIortSMMUv3Dev;
+
+/*
+ * Populate the struct AcpiIortSMMUv3Dev for the legacy SMMUv3 and
+ * return the total number of associated idmaps.
+ */
+static int populate_smmuv3_legacy_dev(GArray *sdev_blob)
+{
+    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
+    AcpiIortSMMUv3Dev sdev;
+
+    sdev.rc_smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
+    object_child_foreach_recursive(object_get_root(), iort_host_bridges,
+                                   sdev.rc_smmu_idmaps);
+    /*
+     * There can be only one legacy SMMUv3("iommu=smmuv3") as it is a machine
+     * wide one. Since it may cover multiple PCIe RCs(based on "bypass_iommu"
+     * property), may have multiple SMMUv3 idmaps. Sort it by input_base.
+     */
+    g_array_sort(sdev.rc_smmu_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);
+    return sdev.rc_smmu_idmaps->len;
+}
+
 /* Compute ID ranges (RIDs) from RC that are directed to the ITS Group node */
-static void create_rc_its_idmaps(GArray *its_idmaps, GArray *smmu_idmaps)
+static void create_rc_its_idmaps(GArray *its_idmaps, GArray *smmuv3_devs)
 {
     AcpiIortIdMapping *idmap;
     AcpiIortIdMapping next_range = {0};
+    AcpiIortSMMUv3Dev *sdev;
 
-    /*
-     * Based on the RID ranges that are directed to the SMMU, determine the
-     * bypassed RID ranges, i.e., the ones that are directed to the ITS Group
-     * node and do not pass through the SMMU, by subtracting the SMMU-bound
-     * ranges from the full RID range (0x0000–0xFFFF).
-     */
-     for (int i = 0; i < smmu_idmaps->len; i++) {
-        idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
+    for (int i = 0; i < smmuv3_devs->len; i++) {
+        sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
+        /*
+         * Based on the RID ranges that are directed to the SMMU, determine the
+         * bypassed RID ranges, i.e., the ones that are directed to the ITS
+         * Group node and do not pass through the SMMU, by subtracting the
+         * SMMU-bound ranges from the full RID range (0x0000–0xFFFF).
+         */
+         for (int j = 0; j < sdev->rc_smmu_idmaps->len; j++) {
+            idmap = &g_array_index(sdev->rc_smmu_idmaps, AcpiIortIdMapping, j);
 
-        if (next_range.input_base < idmap->input_base) {
-            next_range.id_count = idmap->input_base - next_range.input_base;
-            g_array_append_val(its_idmaps, next_range);
-        }
+            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;
+            next_range.input_base = idmap->input_base + idmap->id_count;
+        }
     }
-
     /*
      * Append the last RC -> ITS ID mapping.
      *
@@ -302,7 +338,6 @@ static void create_rc_its_idmaps(GArray *its_idmaps, GArray *smmu_idmaps)
     }
 }
 
-
 /*
  * Input Output Remapping Table (IORT)
  * Conforms to "IO Remapping Table System Software on ARM Platforms",
@@ -312,9 +347,12 @@ 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;
+    AcpiIortSMMUv3Dev *sdev;
+    size_t node_size;
+    int num_smmus = 0;
     uint32_t id = 0;
-    GArray *rc_smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
+    int rc_smmu_idmaps_len = 0;
+    GArray *smmuv3_devs = g_array_new(false, true, sizeof(AcpiIortSMMUv3Dev));
     GArray *rc_its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
 
     AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
@@ -322,22 +360,21 @@ 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) {
-        object_child_foreach_recursive(object_get_root(),
-                                       iort_host_bridges, rc_smmu_idmaps);
-
-        /* Sort the smmu idmap by input_base */
-        g_array_sort(rc_smmu_idmaps, iort_idmap_compare);
+    if (vms->legacy_smmuv3_present) {
+        rc_smmu_idmaps_len = populate_smmuv3_legacy_dev(smmuv3_devs);
+    }
 
-        nb_nodes = 2; /* RC and SMMUv3 */
-        rc_mapping_count = rc_smmu_idmaps->len;
+    num_smmus = smmuv3_devs->len;
+    if (num_smmus) {
+        nb_nodes = num_smmus + 1; /* RC and SMMUv3 */
+        rc_mapping_count = rc_smmu_idmaps_len;
 
         if (vms->its) {
             /*
              * Knowing the ID ranges from the RC to the SMMU, it's possible to
              * determine the ID ranges from RC that go directly to ITS.
              */
-            create_rc_its_idmaps(rc_its_idmaps, rc_smmu_idmaps);
+            create_rc_its_idmaps(rc_its_idmaps, smmuv3_devs);
 
             nb_nodes++; /* ITS */
             rc_mapping_count += rc_its_idmaps->len;
@@ -372,9 +409,10 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         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 smmu_mapping_count, offset_to_id_array;
+        int irq = sdev->irq;
 
         if (vms->its) {
             smmu_mapping_count = 1; /* ITS Group node */
@@ -383,7 +421,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
             smmu_mapping_count = 0; /* No ID mappings */
             offset_to_id_array = 0; /* No ID mappings array */
         }
-        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 +
@@ -396,7 +434,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         /* Reference to ID Array */
         build_append_int_noprefix(table_data, offset_to_id_array, 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 */
@@ -447,21 +485,26 @@ 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;
 
-        /*
-         * Map RIDs (input) from RC to SMMUv3 nodes: RC -> SMMUv3.
-         *
-         * N.B.: The mapping from SMMUv3 to ITS Group node (SMMUv3 -> ITS) is
-         * defined in the SMMUv3 table, where all SMMUv3 IDs are mapped to the
-         * ITS Group node, if ITS is available.
-         */
-        for (i = 0; i < rc_smmu_idmaps->len; i++) {
-            range = &g_array_index(rc_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);
+
+            /*
+             * Map RIDs (input) from RC to SMMUv3 nodes: RC -> SMMUv3.
+             *
+             * N.B.: The mapping from SMMUv3 to ITS Group node (SMMUv3 -> ITS)
+             * is defined in the SMMUv3 table, where all SMMUv3 IDs are mapped
+             * to the ITS Group node, if ITS is available.
+             */
+             for (int j = 0; j < sdev->rc_smmu_idmaps->len; j++) {
+                range = &g_array_index(sdev->rc_smmu_idmaps,
+                                       AcpiIortIdMapping, j);
+                /* Output IORT node is the SMMUv3 node. */
+                build_iort_id_mapping(table_data, range->input_base,
+                                      range->id_count, sdev->offset);
+            }
         }
 
         if (vms->its) {
@@ -486,8 +529,12 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     }
 
     acpi_table_end(linker, &table);
-    g_array_free(rc_smmu_idmaps, true);
     g_array_free(rc_its_idmaps, true);
+    for (i = 0; i < num_smmus; i++) {
+        sdev = &g_array_index(smmuv3_devs, AcpiIortSMMUv3Dev, i);
+        g_array_free(sdev->rc_smmu_idmaps, true);
+    }
+    g_array_free(smmuv3_devs, true);
 }
 
 /*
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3bcdf92e2f..61a9a4fdc8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1616,6 +1616,7 @@ static void create_pcie(VirtMachineState *vms)
                 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 v7 04/12] 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>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt-acpi-build.c | 64 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index cb11b316a2..d2a3914d91 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"
@@ -299,6 +300,67 @@ static int populate_smmuv3_legacy_dev(GArray *sdev_blob)
     return sdev.rc_smmu_idmaps->len;
 }
 
+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->rc_smmu_idmaps,
+                                              AcpiIortIdMapping, 0);
+    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->rc_smmu_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.rc_smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
+    idmap.input_base = min_bus << 8,
+    idmap.id_count = (max_bus - min_bus + 1) << 8,
+    g_array_append_val(sdev.rc_smmu_idmaps, idmap);
+    g_array_append_val(sdev_blob, sdev);
+    return 0;
+}
+
+/*
+ * Populate the struct AcpiIortSMMUv3Dev for all SMMUv3 devices and
+ * return the total number of idmaps.
+ */
+static int 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);
+    /*
+     * Since each SMMUv3 dev is assocaited with specific host bridge,
+     * total number of idmaps equals to total number of smmuv3 devices.
+     */
+    return sdev_blob->len;
+}
+
 /* Compute ID ranges (RIDs) from RC that are directed to the ITS Group node */
 static void create_rc_its_idmaps(GArray *its_idmaps, GArray *smmuv3_devs)
 {
@@ -362,6 +424,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 
     if (vms->legacy_smmuv3_present) {
         rc_smmu_idmaps_len = populate_smmuv3_legacy_dev(smmuv3_devs);
+    } else {
+        rc_smmu_idmaps_len = populate_smmuv3_dev(smmuv3_devs);
     }
 
     num_smmus = smmuv3_devs->len;
-- 
2.47.0

[PATCH v7 05/12] 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>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Tested-by: Eric Auger <eric.auger@redhat.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 61a9a4fdc8..ee272b5b34 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1409,19 +1409,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;
@@ -1440,27 +1464,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 v7 06/12] 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>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ee272b5b34..05a14881cf 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},
-- 
2.47.0

[PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Shameer Kolothum via]

Currently, pci_setup_iommu() registers IOMMU ops for a given PCIBus.
However, when retrieving IOMMU ops for a device using
pci_device_get_iommu_bus_devfn(), the function checks the parent_dev
and fetches IOMMU ops from the parent device, even if the current
bus does not have any associated IOMMU ops.

This behavior works for now because QEMU's IOMMU implementations are
globally scoped, and host bridges rely on the bypass_iommu property
to skip IOMMU translation when needed.

However, this model will break with the soon to be introduced
arm-smmuv3 device, which allows users to associate the IOMMU
with a specific PCIe root complex (e.g., the default pcie.0
or a pxb-pcie root complex).

For example, consider the following setup with multiple root
complexes:

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

In Qemu, pxb-pcie acts as a special root complex whose parent is
effectively the default root complex(pcie.0). Hence, though pcie.1
has no associated SMMUv3 as per above, pci_device_get_iommu_bus_devfn()
will incorrectly return the IOMMU ops from pcie.0 due to the fallback
via parent_dev.

To fix this, introduce a new helper pci_setup_iommu_per_bus() that
explicitly sets the new iommu_per_bus field in the PCIBus structure.
This helper will be used in a subsequent patch that adds support for
the new arm-smmuv3 device.

Update pci_device_get_iommu_bus_devfn() to use iommu_per_bus when
determining the correct IOMMU ops, ensuring accurate behavior for
per-bus IOMMUs.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Nathan Chen <nathanc@nvidia.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/pci/pci.c             | 31 +++++++++++++++++++++++++++++++
 include/hw/pci/pci.h     |  2 ++
 include/hw/pci/pci_bus.h |  1 +
 3 files changed, 34 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c70b5ceeba..85d6a497da 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2909,6 +2909,19 @@ static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
             }
         }
 
+        /*
+         * When multiple PCI Express Root Buses are defined using pxb-pcie,
+         * the IOMMU configuration may be specific to each root bus. However,
+         * pxb-pcie acts as a special root complex whose parent is effectively
+         * the default root complex(pcie.0). Ensure that we retrieve the
+         * correct IOMMU ops(if any) in such cases.
+         */
+        if (pci_bus_is_express(iommu_bus) && pci_bus_is_root(iommu_bus)) {
+            if (!iommu_bus->iommu_per_bus && parent_bus->iommu_per_bus) {
+                break;
+            }
+        }
+
         iommu_bus = parent_bus;
     }
 
@@ -3169,6 +3182,24 @@ void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
     bus->iommu_opaque = opaque;
 }
 
+/*
+ * Similar to pci_setup_iommu(), but sets iommu_per_bus to true,
+ * indicating that the IOMMU is specific to this bus. This is used by
+ * IOMMU implementations that are tied to a specific PCIe root complex.
+ *
+ * In QEMU, pxb-pcie behaves as a special root complex whose parent is
+ * effectively the default root complex (pcie.0). The iommu_per_bus
+ * is checked in pci_device_get_iommu_bus_devfn() to ensure the correct
+ * IOMMU ops are returned, avoiding the use of the parent’s IOMMU when
+ * it's not appropriate.
+ */
+void pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops,
+                             void *opaque)
+{
+    pci_setup_iommu(bus, ops, opaque);
+    bus->iommu_per_bus = true;
+}
+
 static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
 {
     Range *range = opaque;
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index df3cc7b875..a3e0870a15 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -764,6 +764,8 @@ int pci_iommu_unregister_iotlb_notifier(PCIDevice *dev, uint32_t pasid,
  */
 void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque);
 
+void pci_setup_iommu_per_bus(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque);
+
 pcibus_t pci_bar_address(PCIDevice *d,
                          int reg, uint8_t type, pcibus_t size);
 
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 2261312546..c738446788 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -35,6 +35,7 @@ struct PCIBus {
     enum PCIBusFlags flags;
     const PCIIOMMUOps *iommu_ops;
     void *iommu_opaque;
+    bool iommu_per_bus;
     uint8_t devfn_min;
     uint32_t slot_reserved_mask;
     pci_set_irq_fn set_irq;
-- 
2.47.0

[PATCH v7 08/12] hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation

[Shameer Kolothum via]

Allow cold-plugging of an SMMUv3 device on the virt machine when no
global (legacy) SMMUv3 is present or when a virtio-iommu is specified.

This user-created SMMUv3 device is tied to a specific PCI bus provided
by the user, so ensure the IOMMU ops are configured accordingly.

Due to current limitations in QEMU’s device tree support, specifically
its inability to properly present pxb-pcie based root complexes and
their devices, the device tree support for the new SMMUv3 device is
limited to cases where it is attached to the default pcie.0 root complex.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Nathan Chen <nathanc@nvidia.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/smmu-common.c         |  8 +++++-
 hw/arm/smmuv3.c              |  2 ++
 hw/arm/virt.c                | 51 ++++++++++++++++++++++++++++++++++++
 hw/core/sysbus-fdt.c         |  3 +++
 include/hw/arm/smmu-common.h |  1 +
 5 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index b15e7fd0e4..2ee4691299 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -959,7 +959,12 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
                 goto out_err;
             }
         }
-        pci_setup_iommu(pci_bus, &smmu_ops, s);
+
+        if (s->smmu_per_bus) {
+            pci_setup_iommu_per_bus(pci_bus, &smmu_ops, s);
+        } else {
+            pci_setup_iommu(pci_bus, &smmu_ops, s);
+        }
         return;
     }
 out_err:
@@ -984,6 +989,7 @@ static void smmu_base_reset_exit(Object *obj, ResetType type)
 
 static const Property smmu_dev_properties[] = {
     DEFINE_PROP_UINT8("bus_num", SMMUState, bus_num, 0),
+    DEFINE_PROP_BOOL("smmu_per_bus", SMMUState, smmu_per_bus, false),
     DEFINE_PROP_LINK("primary-bus", SMMUState, primary_bus,
                      TYPE_PCI_BUS, PCIBus *),
 };
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 05a14881cf..8340ad33ba 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"
@@ -1440,6 +1441,29 @@ 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 (!(vms->bootinfo.firmware_loaded && virt_is_acpi_enabled(vms)) &&
+        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)
 {
@@ -2935,6 +2959,16 @@ 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");
+        } else if (vms->iommu == VIRT_IOMMU_NONE) {
+            /* The new SMMUv3 device is specific to the PCI bus */
+            object_property_set_bool(OBJECT(dev), "smmu_per_bus", true, NULL);
+        }
     }
 }
 
@@ -2958,6 +2992,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);
 
@@ -3160,6 +3210,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 */
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index e5e2d09294..80d0fecfde 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -161,6 +161,7 @@ struct SMMUState {
     QLIST_HEAD(, SMMUDevice) devices_with_notifiers;
     uint8_t bus_num;
     PCIBus *primary_bus;
+    bool smmu_per_bus; /* SMMU is specific to the primary_bus */
 };
 
 struct SMMUBaseClass {
-- 
2.47.0

[PATCH v7 09/12] qemu-options.hx: Document the arm-smmuv3 device

[Shameer Kolothum via]

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

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 qemu-options.hx | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 1f862b19a6..17d51714d7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1226,6 +1226,13 @@ 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``
+        Accepts either the default root complex (pcie.0) or a
+        pxb-pcie based root complex.
+
 ERST
 
 DEF("name", HAS_ARG, QEMU_OPTION_name,
-- 
2.47.0

[PATCH v7 10/12] bios-tables-test: Allow for smmuv3 test data.

[Shameer Kolothum via]

The tests to be added exercise both legacy(iommu=smmuv3) and new
-device arm-smmuv3,.. cases.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev    | 0
 tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy | 0
 tests/data/acpi/aarch64/virt/IORT.smmuv3-dev    | 0
 tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy | 0
 tests/qtest/bios-tables-test-allowed-diff.h     | 4 ++++
 5 files changed, 4 insertions(+)
 create mode 100644 tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev
 create mode 100644 tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy
 create mode 100644 tests/data/acpi/aarch64/virt/IORT.smmuv3-dev
 create mode 100644 tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy

diff --git a/tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev b/tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy b/tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/data/acpi/aarch64/virt/IORT.smmuv3-dev b/tests/data/acpi/aarch64/virt/IORT.smmuv3-dev
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy b/tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..2e3e3ccdce 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,5 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy",
+"tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev",
+"tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy",
+"tests/data/acpi/aarch64/virt/IORT.smmuv3-dev",
-- 
2.47.0

[PATCH v7 11/12] qtest/bios-tables-test: Add tests for legacy smmuv3 and smmuv3 device

[Shameer Kolothum via]

For the legacy SMMUv3 test, the setup includes three PCIe Root Complexes,
one of which has bypass_iommu enabled. The generated IORT table contains
a single SMMUv3 node, a Root Complex(RC) node and 1 ITS node.
RC node features 4 ID mappings, of which 2 points to SMMU node and the
remaining ones points to ITS.

       pcie.0 -> {SMMU0} -> {ITS}
{RC}   pcie.1 -> {SMMU0} -> {ITS}
       pcie.2            -> {ITS}
       [all other ids]   -> {ITS}

For the -device arm-smmuv3,... test, the configuration also includes three
Root Complexes, with two connected to separate SMMUv3 devices.
The resulting IORT table contains 1 RC node, 2 SMMU nodes and 1 ITS node.
RC node features 4 ID mappings. 2 of them target the 2 SMMU nodes while
the others targets the ITS.

        pcie.0 -> {SMMU0} -> {ITS}
{RC}    pcie.1 -> {SMMU1} -> {ITS}
        pcie.2            -> {ITS}
        [all other ids]   -> {ITS}

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 tests/qtest/bios-tables-test.c | 86 ++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 4dbc07ec5e..79f8ca6559 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -2250,6 +2250,86 @@ static void test_acpi_aarch64_virt_viot(void)
     free_test_data(&data);
 }
 
+static void test_acpi_aarch64_virt_smmuv3_legacy(void)
+{
+    test_data data = {
+        .machine = "virt",
+        .arch = "aarch64",
+        .tcg_only = true,
+        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
+        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
+        .ram_start = 0x40000000ULL,
+        .scan_len = 128ULL * MiB,
+    };
+
+    /*
+     * cdrom is plugged into scsi controller to avoid conflict
+     * with pxb-pcie. See comments in test_acpi_aarch64_virt_tcg_pxb() for
+     * details.
+     *
+     * The setup includes three PCIe root complexes, one of which has
+     * bypass_iommu enabled. The generated IORT table contains a single
+     * SMMUv3 node and a Root Complex node with three ID mappings. Two
+     * of the ID mappings have output references pointing to the SMMUv3
+     * node and the remaining one points to ITS.
+     */
+    data.variant = ".smmuv3-legacy";
+    test_acpi_one(" -device pcie-root-port,chassis=1,id=pci.1"
+                  " -device virtio-scsi-pci,id=scsi0,bus=pci.1"
+                  " -drive file="
+                  "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2,"
+                  "if=none,media=cdrom,id=drive-scsi0-0-0-1,readonly=on"
+                  " -device scsi-cd,bus=scsi0.0,scsi-id=0,"
+                  "drive=drive-scsi0-0-0-1,id=scsi0-0-0-1,bootindex=1"
+                  " -cpu cortex-a57"
+                  " -M iommu=smmuv3"
+                  " -device pxb-pcie,id=pcie.1,bus=pcie.0,bus_nr=0x10"
+                  " -device pxb-pcie,id=pcie.2,bus=pcie.0,bus_nr=0x20,bypass_iommu=on",
+                  &data);
+    free_test_data(&data);
+}
+
+static void test_acpi_aarch64_virt_smmuv3_dev(void)
+{
+    test_data data = {
+        .machine = "virt",
+        .arch = "aarch64",
+        .tcg_only = true,
+        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
+        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
+        .ram_start = 0x40000000ULL,
+        .scan_len = 128ULL * MiB,
+    };
+
+    /*
+     * cdrom is plugged into scsi controller to avoid conflict
+     * with pxb-pcie. See comments in test_acpi_aarch64_virt_tcg_pxb()
+     * for details.
+     *
+     * The setup includes three PCie root complexes, two of which are
+     * connected to separate SMMUv3 devices. The resulting IORT table
+     * contains two SMMUv3 nodes and a Root Complex node with ID mappings
+     * of which two of the ID mappings have output references pointing
+     * to two different SMMUv3 nodes and the remaining ones pointing to
+     * ITS.
+     */
+    data.variant = ".smmuv3-dev";
+    test_acpi_one(" -device pcie-root-port,chassis=1,id=pci.1"
+                  " -device virtio-scsi-pci,id=scsi0,bus=pci.1"
+                  " -drive file="
+                  "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2,"
+                  "if=none,media=cdrom,id=drive-scsi0-0-0-1,readonly=on"
+                  " -device scsi-cd,bus=scsi0.0,scsi-id=0,"
+                  "drive=drive-scsi0-0-0-1,id=scsi0-0-0-1,bootindex=1"
+                  " -cpu cortex-a57"
+                  " -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.0"
+                  " -device pxb-pcie,id=pcie.1,bus=pcie.0,bus_nr=0x10"
+                  " -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1"
+                  " -device pxb-pcie,id=pcie.2,bus=pcie.0,bus_nr=0x20",
+                  &data);
+    free_test_data(&data);
+}
+
 #ifndef _WIN32
 # define DEV_NULL "/dev/null"
 #else
@@ -2607,6 +2687,12 @@ int main(int argc, char *argv[])
             if (qtest_has_device("virtio-iommu-pci")) {
                 qtest_add_func("acpi/virt/viot", test_acpi_aarch64_virt_viot);
             }
+            qtest_add_func("acpi/virt/smmuv3-legacy",
+                           test_acpi_aarch64_virt_smmuv3_legacy);
+            if (qtest_has_device("arm-smmuv3")) {
+                qtest_add_func("acpi/virt/smmuv3-dev",
+                               test_acpi_aarch64_virt_smmuv3_dev);
+            }
         }
     } else if (strcmp(arch, "riscv64") == 0) {
         if (has_tcg && qtest_has_device("virtio-blk-pci")) {
-- 
2.47.0

[PATCH v7 12/12] qtest/bios-tables-test: Update tables for smmuv3 tests

[Shameer Kolothum via]

For the legacy smmuv3 test case, generated IORT has a single SMMUv3 node,
a Root Complex(RC) node and 1 ITS node.
RC node features 4 ID mappings, of which 2 points to SMMU node and the
remaining ones points to ITS.

       pcie.0 -> {SMMU0} -> {ITS}
{RC}   pcie.1 -> {SMMU0} -> {ITS}
       pcie.2            -> {ITS}
       [all other ids]   -> {ITS}

...
[030h 0048   1]                         Type : 00
[031h 0049   2]                       Length : 0018
[033h 0051   1]                     Revision : 01
[034h 0052   4]                   Identifier : 00000000
[038h 0056   4]                Mapping Count : 00000000
[03Ch 0060   4]               Mapping Offset : 00000000

[040h 0064   4]                     ItsCount : 00000001
[044h 0068   4]                  Identifiers : 00000000

[048h 0072   1]                         Type : 04
[049h 0073   2]                       Length : 0058
[04Bh 0075   1]                     Revision : 04
[04Ch 0076   4]                   Identifier : 00000001
[050h 0080   4]                Mapping Count : 00000001
[054h 0084   4]               Mapping Offset : 00000044

[058h 0088   8]                 Base Address : 0000000009050000
[060h 0096   4]        Flags (decoded below) : 00000001
                             COHACC Override : 1
                               HTTU Override : 0
                      Proximity Domain Valid : 0
[064h 0100   4]                     Reserved : 00000000
[068h 0104   8]                VATOS Address : 0000000000000000
[070h 0112   4]                        Model : 00000000
[074h 0116   4]                   Event GSIV : 0000006A
[078h 0120   4]                     PRI GSIV : 0000006B
[07Ch 0124   4]                    GERR GSIV : 0000006D
[080h 0128   4]                    Sync GSIV : 0000006C
[084h 0132   4]             Proximity Domain : 00000000
[088h 0136   4]      Device ID Mapping Index : 00000000

[08Ch 0140   4]                   Input base : 00000000
[090h 0144   4]                     ID Count : 0000FFFF
[094h 0148   4]                  Output Base : 00000000
[098h 0152   4]             Output Reference : 00000030
[09Ch 0156   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

[0A0h 0160   1]                         Type : 02
[0A1h 0161   2]                       Length : 0074
[0A3h 0163   1]                     Revision : 03
[0A4h 0164   4]                   Identifier : 00000002
[0A8h 0168   4]                Mapping Count : 00000004
[0ACh 0172   4]               Mapping Offset : 00000024

[0B0h 0176   8]            Memory Properties : [IORT Memory Access Properties]
[0B0h 0176   4]              Cache Coherency : 00000001
[0B4h 0180   1]        Hints (decoded below) : 00
                                   Transient : 0
                              Write Allocate : 0
                               Read Allocate : 0
                                    Override : 0
[0B5h 0181   2]                     Reserved : 0000
[0B7h 0183   1] Memory Flags (decoded below) : 03
                                   Coherency : 1
                            Device Attribute : 1
[0B8h 0184   4]                ATS Attribute : 00000000
[0BCh 0188   4]           PCI Segment Number : 00000000
[0C0h 0192   1]            Memory Size Limit : 40
[0C1h 0193   2]           PASID Capabilities : 0000
[0C3h 0195   1]                     Reserved : 00

[0C4h 0196   4]                   Input base : 00000000
[0C8h 0200   4]                     ID Count : 000001FF
[0CCh 0204   4]                  Output Base : 00000000
[0D0h 0208   4]             Output Reference : 00000048
[0D4h 0212   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

[0D8h 0216   4]                   Input base : 00001000
[0DCh 0220   4]                     ID Count : 000000FF
[0E0h 0224   4]                  Output Base : 00001000
[0E4h 0228   4]             Output Reference : 00000048
[0E8h 0232   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

[0ECh 0236   4]                   Input base : 00000200
[0F0h 0240   4]                     ID Count : 00000DFF
[0F4h 0244   4]                  Output Base : 00000200
[0F8h 0248   4]             Output Reference : 00000030
[0FCh 0252   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

[100h 0256   4]                   Input base : 00001100
[104h 0260   4]                     ID Count : 0000EEFF
[108h 0264   4]                  Output Base : 00001100
[10Ch 0268   4]             Output Reference : 00000030
[110h 0272   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

For the smmuv3-dev test case, IORT has 2 SMMUV3 nodes,
1 RC node and 1 ITS node.
RC node features 4 ID mappings. 2 of them target the 2
SMMU nodes while the others targets the ITS.

        pcie.0 -> {SMMU0} -> {ITS}
{RC}    pcie.1 -> {SMMU1} -> {ITS}
        pcie.2            -> {ITS}
        [all other ids]   -> {ITS}
...
[030h 0048   1]                         Type : 00
[031h 0049   2]                       Length : 0018
[033h 0051   1]                     Revision : 01
[034h 0052   4]                   Identifier : 00000000
[038h 0056   4]                Mapping Count : 00000000
[03Ch 0060   4]               Mapping Offset : 00000000

[040h 0064   4]                     ItsCount : 00000001
[044h 0068   4]                  Identifiers : 00000000

[048h 0072   1]                         Type : 04
[049h 0073   2]                       Length : 0058
[04Bh 0075   1]                     Revision : 04
[04Ch 0076   4]                   Identifier : 00000001
[050h 0080   4]                Mapping Count : 00000001
[054h 0084   4]               Mapping Offset : 00000044

[058h 0088   8]                 Base Address : 000000000C000000
[060h 0096   4]        Flags (decoded below) : 00000001
                             COHACC Override : 1
                               HTTU Override : 0
                      Proximity Domain Valid : 0
[064h 0100   4]                     Reserved : 00000000
[068h 0104   8]                VATOS Address : 0000000000000000
[070h 0112   4]                        Model : 00000000
[074h 0116   4]                   Event GSIV : 00000090
[078h 0120   4]                     PRI GSIV : 00000091
[07Ch 0124   4]                    GERR GSIV : 00000093
[080h 0128   4]                    Sync GSIV : 00000092
[084h 0132   4]             Proximity Domain : 00000000
[088h 0136   4]      Device ID Mapping Index : 00000000

[08Ch 0140   4]                   Input base : 00000000
[090h 0144   4]                     ID Count : 0000FFFF
[094h 0148   4]                  Output Base : 00000000
[098h 0152   4]             Output Reference : 00000030
[09Ch 0156   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

[0A0h 0160   1]                         Type : 04
[0A1h 0161   2]                       Length : 0058
[0A3h 0163   1]                     Revision : 04
[0A4h 0164   4]                   Identifier : 00000002
[0A8h 0168   4]                Mapping Count : 00000001
[0ACh 0172   4]               Mapping Offset : 00000044

[0B0h 0176   8]                 Base Address : 000000000C020000
[0B8h 0184   4]        Flags (decoded below) : 00000001
                             COHACC Override : 1
                               HTTU Override : 0
                      Proximity Domain Valid : 0
[0BCh 0188   4]                     Reserved : 00000000
[0C0h 0192   8]                VATOS Address : 0000000000000000
[0C8h 0200   4]                        Model : 00000000
[0CCh 0204   4]                   Event GSIV : 00000094
[0D0h 0208   4]                     PRI GSIV : 00000095
[0D4h 0212   4]                    GERR GSIV : 00000097
[0D8h 0216   4]                    Sync GSIV : 00000096
[0DCh 0220   4]             Proximity Domain : 00000000
[0E0h 0224   4]      Device ID Mapping Index : 00000000

[0E4h 0228   4]                   Input base : 00000000
[0E8h 0232   4]                     ID Count : 0000FFFF
[0ECh 0236   4]                  Output Base : 00000000
[0F0h 0240   4]             Output Reference : 00000030
[0F4h 0244   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

[0F8h 0248   1]                         Type : 02
[0F9h 0249   2]                       Length : 0074
[0FBh 0251   1]                     Revision : 03
[0FCh 0252   4]                   Identifier : 00000003
[100h 0256   4]                Mapping Count : 00000004
[104h 0260   4]               Mapping Offset : 00000024

[108h 0264   8]            Memory Properties : [IORT Memory Access Properties]
[108h 0264   4]              Cache Coherency : 00000001
[10Ch 0268   1]        Hints (decoded below) : 00
                                   Transient : 0
                              Write Allocate : 0
                               Read Allocate : 0
                                    Override : 0
[10Dh 0269   2]                     Reserved : 0000
[10Fh 0271   1] Memory Flags (decoded below) : 03
                                   Coherency : 1
                            Device Attribute : 1
[110h 0272   4]                ATS Attribute : 00000000
[114h 0276   4]           PCI Segment Number : 00000000
[118h 0280   1]            Memory Size Limit : 40
[119h 0281   2]           PASID Capabilities : 0000
[11Bh 0283   1]                     Reserved : 00

[11Ch 0284   4]                   Input base : 00000000
[120h 0288   4]                     ID Count : 000001FF
[124h 0292   4]                  Output Base : 00000000
[128h 0296   4]             Output Reference : 00000048
[12Ch 0300   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

[130h 0304   4]                   Input base : 00001000
[134h 0308   4]                     ID Count : 000000FF
[138h 0312   4]                  Output Base : 00001000
[13Ch 0316   4]             Output Reference : 000000A0
[140h 0320   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

[144h 0324   4]                   Input base : 00000200
[148h 0328   4]                     ID Count : 00000DFF
[14Ch 0332   4]                  Output Base : 00000200
[150h 0336   4]             Output Reference : 00000030
[154h 0340   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

[158h 0344   4]                   Input base : 00001100
[15Ch 0348   4]                     ID Count : 0000EEFF
[160h 0352   4]                  Output Base : 00001100
[164h 0356   4]             Output Reference : 00000030
[168h 0360   4]        Flags (decoded below) : 00000000
                              Single Mapping : 0

Note: DSDT changes are not described here as it is not impacted by the
way the SMMUv3 is instantiated.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev    | Bin 0 -> 10162 bytes
 tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy | Bin 0 -> 10162 bytes
 tests/data/acpi/aarch64/virt/IORT.smmuv3-dev    | Bin 0 -> 364 bytes
 tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy | Bin 0 -> 276 bytes
 tests/qtest/bios-tables-test-allowed-diff.h     |   4 ----
 5 files changed, 4 deletions(-)

diff --git a/tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev b/tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..4b166b836e75f0c049c245b0c1dc7955f0dbcd04 100644
GIT binary patch
literal 10162
zcmds-OK)4(6@?Ee+M=!=rrwq;SxTFxZ-bQlP~0SybR}6zC7UJXxPSsMXwjmnCk7f6
zX@df;2We`cgGOk_D&SwxUl5?9&I)wULC2Zo4|G;_uXTB~_JL1}z|bfJOmdU^ti9Id
zq51JrTivb0zkD@>>OVH`<-6VG%^$S$WxG^D2)K{i#_!*+zTN9?_Ku#+=gnrb-rL>V
z8r{g-dsf^E_1^uxt#I_@c7E@me6Dk_+ibR4{paevywe?@&t^Xy3J@F$jSzl!F;prq
zS@la>ySDQn-zjvzzkP7ubXRO`_i!T{w#E5IrH~lU#tMn*(B=NhjoMqC($fCj&OKXt
zLu1cYYIqi^{oze7(JH3L5*YiOL^VjF)tA_Wi<Oi_R!wQtoW_dRSWRjSq%<x%ja9F4
zJ*iPkY1Ey@b+2(Fsc|8tkvWYUUgKs`<6=r<#A)308n==fgDH(Mr*X?`tR*$-DUF8H
zSo0dMCN+jq8WT?ARj1MFzd7P9mSri8DW`GUYb3X6{3a>4`i#@~j@L-<(~*>q%TD7p
zuaVxTqbZGfr}4ViNbl3Jl*WS7c*AR?_vxjS##N{BU9XYer;U`xveQ`i8tHvHp3?Y|
z)7bDD>3uqp(s<cvY<i9KKAlWyyy7%+r?HaWr&B47uQ`n^uaVxT(<zN_IE`(uk=~~>
zDUEMAjUBI%-lwxEjcZQhj@L-<)5|H1rqkH<8tHvHm(o~q8hc(Ny-(*;8rPl1n_eTm
zPp_mjZaR(cd5!cwT}WxHIgNd<k>00^DUI7s<G^XGruXU9l*VgL<F408@6)A}#v4xK
zUSDIcw!c!+NBeHOnKyUN;G=EKGnNgLbH2=eQ+@8Ssf)dCr&~FCLaDx;NXj6@OTC<s
zDjqwNdK0et@195sFygholF%w1i?#aiqFBqI#B05h&?+7~)5<?;=jHz{(AsFO+0n`k
zCW~s-s?E*2*)ow5H{HX|_*^#gD7+tj7?#6U_;L6}csKmWZUdGQ&f1;B$^D;ak0*v}
z!>6YKuSh8FSL}W(OpM_1tcrtD76VqShKbSI$myveY0=lOu*w?8is!h_TFVkEO`p9+
zl0|R7!pgGMFcA~2l`E~pN^`^B0j?|^3<Rt!TL+4j*2<MuVx>9a${JUe4r*Lkwi;IU
zQ*x!1KIerf=LN1Tb6zM`mgcSWIWI;z4cyI7*=iW-b6V+h4n{c#qnv|rPAh%RdX%#s
z<uq*dIj!_LhoYSJm7_R%JS0PLPAh%REXtWhISq4tPAh%R;V9>Dlyf-FX{FCO66G9;
zavB!<oL2grqfyS$DCcOL(@LLnEXp|+<ur`;Ij!_LFGV>oML93UIj!_L?JHlg$GM9e
zQBK2dpVLa8b3Dp99_1X5b6V+hPDD8;qMU~5KBtvF=VX*~GRip_=d{x2oQiTzML7-Y
zeNHQV&gm%Ubd+;C&S|C3ITPiaiE`T40H4!JpK~_KIUD7ijdNP*bJ`bh_j+EAa@yAh
zpVLa8b1up`7v-Fbb6V+h&PO@tqn!3N!{@Zp=e!c-yb|TS66dti=Uj+#E<`!)Yl+Wk
zrO&w-<y?$%F2*^n^f|9aIj=@J?Q4wBX{FD(6y;osaxTR=tt@gDSTdbWd^8%W_TQ0x
z&@S6`#m>ysyZ77eP|puL`(bKlsCTcM*PcEM`PPHh$?o9sPsb;-#?isQR{y=Uu>Pk9
z?`?eY`agg2^kG)Bh84T5+wJNj6wtw)RY-k+t859^CykGf->v-Go@ueWbu#nuaoJZq
z+`yBM(2U2QJ^b@n@i>OGo;{rUc5kbDP&;|4aP#+DfBUq1<5vfNnEmWv?yvd5$<Xo7
z!n5&#N(EPTs>wL3b$-a!YMvXiwayXQTFrArw(`Wq|3fzWwrpnsGrM^qWV2V~4B70E
zmXubtm&zfVrE<u|K7N=~Y@4%@AMq7zbGDY{0A6v2Z0zOADp!^$Lu6wsR}$GY1;{1`
zKsL1!+1SdJL^e$UvdICEO|3*WnJ2Po3Xn|>fNW|dvMDE#O;dnuasXsgE0Im+iENqz
zWRn9Rn_7u%GEZdF6d;=%0NK<^WRrO!o2CHS<N(N~RwA3s6WKHc$R-CsHnkGjWS+>T
zDL^(k0J5o-$R_hdHcbJt$pMf}twc7NC$eb@kWCJNY-%O4$vlxwQ-Ex80Ay1ukxk}_
zY?=aOlLH`|T8V5jPh`^+Ae$Tj+0;s8lX)VWrU2RG0LZ3RBAd(;*)#>nCI>(^wG!E6
zp2(&tKsGr5vZ<BGCi6r#O#!mW0gz3tL^hcxvS|vCO%8x;Y9+GCJdsUPfNXLAWK%1V
zP3DPgngV2#10b7PiEJ`YWYZKNn;Zby)JkNNc_N#p0NLaK$fi~zo6Hm0GzG{e2S7Hp
z64_*)$fhYkHaP&Ysg=kk^F%gH0kX*fkWH;bHkl`~X$p`{4uEWGC9=spkxf&8Y;pi(
zQ!9~8=80^Y0%Vf|Ae&l=Y%)(|!<6Tct%0A^zQD+K_OoMj4$6*$KLz#q&a&ALlf{oY
zk<<&FWwW0t6H+gBmaTbS$hQ8+cD~m?H~PPjt-F24j-8!rvupf%TVUH#w$KiF*t4^1
z%b{uiKHvE^du`6(%?_^M4Zq^{PreQ8TRsPG>^p-uHkPzjEn3SGD?Pli_YB^sTGAR;
zT5DNirMY1*;7twyZ)zpHv6U+cZ<+#llLNq;S_yA5Pk7T5z?&QZ-qcEXlX=3MrU2gL
z0Pv<(!kf$!-ZTa9CI^5wwG!TBp75qAfHye+ys4G&Ci8?hO#!^g0pLxogg2QdylD#H
zO%4EWY9+kMJmF1K0B>>tcvCCkP38%2ngV!}1HhYF32!n_c+(WXn;Zb%)Jk}hdBU5f
z0N&&P@TOM6o6Hm5GzIV`2Y@%V65eE<@TMt%H#q>jsg>|1^Mp4|0ldip;7zTBH<>5A
zX$s&?4ghazCA`Tz;Z0KjZ*l;5Q!C+3<_T|_0(g@Hz?)hLZ!%AK(-gp)901<bN_dlb
z!keZ5-sAxArdGn6%oE-;1@I;ZfH$=g-ejKerYV3oIRLz=mGCC>gf~qAyvYIJO|67C
znJ2ty3gAr+0B>p~yvaP_O;Z4GasYT!E8$J%32&MLc#{Lbn_3BPGEaEZ6u_Gt0N&I}
zc$0a;o2CHX<N)xdR>GUi6W%lh@FoX<H?<PpWS;PbDN~JJzPS;e+5ZZmv2Ird&Fv0m
z@0DlZLa;Z!A>D6nzl>DuE-sDJ)BoU)AMA2^&7P@@*r!@?v7f!{a%#_3!qacMHGKLp
zp2LNeuJHI5@*Lkmt+U;A2$Y{8^J><7?)dOEu%olIu0w-+AZq?~+UDTf*YNjmceb~B
z1@9Gq{MNz`mWEHG_C)_hEPn7EsC>Ox`gprzq-7sD^`WeG^qXOn`F3}Ee|c)LxBb@P
j&?6M@oHUL<cqhZ@rw`i?I-A+!qbGB?<KZ$dW`+L(PyH`>

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy b/tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..4b166b836e75f0c049c245b0c1dc7955f0dbcd04 100644
GIT binary patch
literal 10162
zcmds-OK)4(6@?Ee+M=!=rrwq;SxTFxZ-bQlP~0SybR}6zC7UJXxPSsMXwjmnCk7f6
zX@df;2We`cgGOk_D&SwxUl5?9&I)wULC2Zo4|G;_uXTB~_JL1}z|bfJOmdU^ti9Id
zq51JrTivb0zkD@>>OVH`<-6VG%^$S$WxG^D2)K{i#_!*+zTN9?_Ku#+=gnrb-rL>V
z8r{g-dsf^E_1^uxt#I_@c7E@me6Dk_+ibR4{paevywe?@&t^Xy3J@F$jSzl!F;prq
zS@la>ySDQn-zjvzzkP7ubXRO`_i!T{w#E5IrH~lU#tMn*(B=NhjoMqC($fCj&OKXt
zLu1cYYIqi^{oze7(JH3L5*YiOL^VjF)tA_Wi<Oi_R!wQtoW_dRSWRjSq%<x%ja9F4
zJ*iPkY1Ey@b+2(Fsc|8tkvWYUUgKs`<6=r<#A)308n==fgDH(Mr*X?`tR*$-DUF8H
zSo0dMCN+jq8WT?ARj1MFzd7P9mSri8DW`GUYb3X6{3a>4`i#@~j@L-<(~*>q%TD7p
zuaVxTqbZGfr}4ViNbl3Jl*WS7c*AR?_vxjS##N{BU9XYer;U`xveQ`i8tHvHp3?Y|
z)7bDD>3uqp(s<cvY<i9KKAlWyyy7%+r?HaWr&B47uQ`n^uaVxT(<zN_IE`(uk=~~>
zDUEMAjUBI%-lwxEjcZQhj@L-<)5|H1rqkH<8tHvHm(o~q8hc(Ny-(*;8rPl1n_eTm
zPp_mjZaR(cd5!cwT}WxHIgNd<k>00^DUI7s<G^XGruXU9l*VgL<F408@6)A}#v4xK
zUSDIcw!c!+NBeHOnKyUN;G=EKGnNgLbH2=eQ+@8Ssf)dCr&~FCLaDx;NXj6@OTC<s
zDjqwNdK0et@195sFygholF%w1i?#aiqFBqI#B05h&?+7~)5<?;=jHz{(AsFO+0n`k
zCW~s-s?E*2*)ow5H{HX|_*^#gD7+tj7?#6U_;L6}csKmWZUdGQ&f1;B$^D;ak0*v}
z!>6YKuSh8FSL}W(OpM_1tcrtD76VqShKbSI$myveY0=lOu*w?8is!h_TFVkEO`p9+
zl0|R7!pgGMFcA~2l`E~pN^`^B0j?|^3<Rt!TL+4j*2<MuVx>9a${JUe4r*Lkwi;IU
zQ*x!1KIerf=LN1Tb6zM`mgcSWIWI;z4cyI7*=iW-b6V+h4n{c#qnv|rPAh%RdX%#s
z<uq*dIj!_LhoYSJm7_R%JS0PLPAh%REXtWhISq4tPAh%R;V9>Dlyf-FX{FCO66G9;
zavB!<oL2grqfyS$DCcOL(@LLnEXp|+<ur`;Ij!_LFGV>oML93UIj!_L?JHlg$GM9e
zQBK2dpVLa8b3Dp99_1X5b6V+hPDD8;qMU~5KBtvF=VX*~GRip_=d{x2oQiTzML7-Y
zeNHQV&gm%Ubd+;C&S|C3ITPiaiE`T40H4!JpK~_KIUD7ijdNP*bJ`bh_j+EAa@yAh
zpVLa8b1up`7v-Fbb6V+h&PO@tqn!3N!{@Zp=e!c-yb|TS66dti=Uj+#E<`!)Yl+Wk
zrO&w-<y?$%F2*^n^f|9aIj=@J?Q4wBX{FD(6y;osaxTR=tt@gDSTdbWd^8%W_TQ0x
z&@S6`#m>ysyZ77eP|puL`(bKlsCTcM*PcEM`PPHh$?o9sPsb;-#?isQR{y=Uu>Pk9
z?`?eY`agg2^kG)Bh84T5+wJNj6wtw)RY-k+t859^CykGf->v-Go@ueWbu#nuaoJZq
z+`yBM(2U2QJ^b@n@i>OGo;{rUc5kbDP&;|4aP#+DfBUq1<5vfNnEmWv?yvd5$<Xo7
z!n5&#N(EPTs>wL3b$-a!YMvXiwayXQTFrArw(`Wq|3fzWwrpnsGrM^qWV2V~4B70E
zmXubtm&zfVrE<u|K7N=~Y@4%@AMq7zbGDY{0A6v2Z0zOADp!^$Lu6wsR}$GY1;{1`
zKsL1!+1SdJL^e$UvdICEO|3*WnJ2Po3Xn|>fNW|dvMDE#O;dnuasXsgE0Im+iENqz
zWRn9Rn_7u%GEZdF6d;=%0NK<^WRrO!o2CHS<N(N~RwA3s6WKHc$R-CsHnkGjWS+>T
zDL^(k0J5o-$R_hdHcbJt$pMf}twc7NC$eb@kWCJNY-%O4$vlxwQ-Ex80Ay1ukxk}_
zY?=aOlLH`|T8V5jPh`^+Ae$Tj+0;s8lX)VWrU2RG0LZ3RBAd(;*)#>nCI>(^wG!E6
zp2(&tKsGr5vZ<BGCi6r#O#!mW0gz3tL^hcxvS|vCO%8x;Y9+GCJdsUPfNXLAWK%1V
zP3DPgngV2#10b7PiEJ`YWYZKNn;Zby)JkNNc_N#p0NLaK$fi~zo6Hm0GzG{e2S7Hp
z64_*)$fhYkHaP&Ysg=kk^F%gH0kX*fkWH;bHkl`~X$p`{4uEWGC9=spkxf&8Y;pi(
zQ!9~8=80^Y0%Vf|Ae&l=Y%)(|!<6Tct%0A^zQD+K_OoMj4$6*$KLz#q&a&ALlf{oY
zk<<&FWwW0t6H+gBmaTbS$hQ8+cD~m?H~PPjt-F24j-8!rvupf%TVUH#w$KiF*t4^1
z%b{uiKHvE^du`6(%?_^M4Zq^{PreQ8TRsPG>^p-uHkPzjEn3SGD?Pli_YB^sTGAR;
zT5DNirMY1*;7twyZ)zpHv6U+cZ<+#llLNq;S_yA5Pk7T5z?&QZ-qcEXlX=3MrU2gL
z0Pv<(!kf$!-ZTa9CI^5wwG!TBp75qAfHye+ys4G&Ci8?hO#!^g0pLxogg2QdylD#H
zO%4EWY9+kMJmF1K0B>>tcvCCkP38%2ngV!}1HhYF32!n_c+(WXn;Zb%)Jk}hdBU5f
z0N&&P@TOM6o6Hm5GzIV`2Y@%V65eE<@TMt%H#q>jsg>|1^Mp4|0ldip;7zTBH<>5A
zX$s&?4ghazCA`Tz;Z0KjZ*l;5Q!C+3<_T|_0(g@Hz?)hLZ!%AK(-gp)901<bN_dlb
z!keZ5-sAxArdGn6%oE-;1@I;ZfH$=g-ejKerYV3oIRLz=mGCC>gf~qAyvYIJO|67C
znJ2ty3gAr+0B>p~yvaP_O;Z4GasYT!E8$J%32&MLc#{Lbn_3BPGEaEZ6u_Gt0N&I}
zc$0a;o2CHX<N)xdR>GUi6W%lh@FoX<H?<PpWS;PbDN~JJzPS;e+5ZZmv2Ird&Fv0m
z@0DlZLa;Z!A>D6nzl>DuE-sDJ)BoU)AMA2^&7P@@*r!@?v7f!{a%#_3!qacMHGKLp
zp2LNeuJHI5@*Lkmt+U;A2$Y{8^J><7?)dOEu%olIu0w-+AZq?~+UDTf*YNjmceb~B
z1@9Gq{MNz`mWEHG_C)_hEPn7EsC>Ox`gprzq-7sD^`WeG^qXOn`F3}Ee|c)LxBb@P
j&?6M@oHUL<cqhZ@rw`i?I-A+!qbGB?<KZ$dW`+L(PyH`>

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/aarch64/virt/IORT.smmuv3-dev b/tests/data/acpi/aarch64/virt/IORT.smmuv3-dev
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..67be268f62afbf2d9459540984da5e9340afdaaa 100644
GIT binary patch
literal 364
zcmebD4+_a)WME)E<>c?|5v<@85#X!<1VAAM5F13Z0I>lOgMkDCNC*yK9F_<M77!bR
zT_CbNAPOcA5rU8tfYd}Fo(#m3AVP5R|9=P*W*^90CZG_)Tqd06P64W$3dGZacp4BR
z19WqlN*I`#feJu=QvqVAJ3&HV-~grnLnS<*d<Fpq2Cx%>^a7X|(1HJXfgB(Wb2oz^
MQ0yI03`oPo01?F*0RR91

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy b/tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..41981a449fc306b80cccd87ddec3c593a8d72c07 100644
GIT binary patch
literal 276
zcmX|*K@NgI3`M`puy8?wi3^u3IDkhWzycE!jI!Vis5{Ti6^7s1@{>QmeVugXHa@5G
z0SNbY?1op>&X2C5h#<9Ops%#*0ztdHi8G?q;$EluQNrhn>{ys@`b&R|d8G8O{Jrdl
mkP$_?rfr{mN!3^;8w}Q?1auX1XIzvDUSRruoXA!(rn3!nx)K2Z

literal 0
HcmV?d00001

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 2e3e3ccdce..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,5 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/aarch64/virt/DSDT.smmuv3-legacy",
-"tests/data/acpi/aarch64/virt/DSDT.smmuv3-dev",
-"tests/data/acpi/aarch64/virt/IORT.smmuv3-legacy",
-"tests/data/acpi/aarch64/virt/IORT.smmuv3-dev",
-- 
2.47.0

Re: [PATCH v7 02/12] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Nicolin Chen]

On Tue, Jul 08, 2025 at 04:40:45PM +0100, Shameer Kolothum wrote:
> @@ -937,11 +939,32 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
>                                       g_free, g_free);
>      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);

Although this is not introduced by this patch, is there a
g_hash_table_remove() somewhere in the code?

> +    /*
> +     * 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), TYPE_PXB_PCIE_BUS)) {

The pci_bus_is_express(pci_bus) at the top is equivalent to:
	object_dynamic_cast(OBJECT(pci_bus), TYPE_PCIE_BUS)
Then here it is doing:
	object_dynamic_cast(OBJECT(pci_bus), TYPE_PXB_PCIE_BUS)

So, this checks the same pci_bus but expects two different types?

I don't see the code check "PCIe Root Complex" explicitly, which
should be TYPE_GPEX_HOST?

Thanks
Nicolin

Re: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Nicolin Chen]

On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote:
> @@ -2909,6 +2909,19 @@ static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
>              }
>          }
>  
> +        /*
> +         * When multiple PCI Express Root Buses are defined using pxb-pcie,
> +         * the IOMMU configuration may be specific to each root bus. However,
> +         * pxb-pcie acts as a special root complex whose parent is effectively
> +         * the default root complex(pcie.0). Ensure that we retrieve the
> +         * correct IOMMU ops(if any) in such cases.
> +         */
> +        if (pci_bus_is_express(iommu_bus) && pci_bus_is_root(iommu_bus)) {
> +            if (!iommu_bus->iommu_per_bus && parent_bus->iommu_per_bus) {
> +                break;
 
Mind elaborating why the current bus must unset iommu_per_bus while
its parent sets iommu_per_bus?

My understanding is that for a pxb-pcie we should set iommu_per_bus
but for its parent (the default root complex) we should unset its
iommu_per_bus?

Thanks
Nicolin

Re: [PATCH v7 08/12] hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation

[Nicolin Chen]

On Tue, Jul 08, 2025 at 04:40:51PM +0100, Shameer Kolothum wrote:
> Allow cold-plugging of an SMMUv3 device on the virt machine when no
> global (legacy) SMMUv3 is present or when a virtio-iommu is specified.
> 
> This user-created SMMUv3 device is tied to a specific PCI bus provided
> by the user, so ensure the IOMMU ops are configured accordingly.
> 
> Due to current limitations in QEMU’s device tree support, specifically
> its inability to properly present pxb-pcie based root complexes and
> their devices, the device tree support for the new SMMUv3 device is
> limited to cases where it is attached to the default pcie.0 root complex.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

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

RE: [PATCH v7 02/12] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, July 8, 2025 9:57 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; imammedo@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com;
> gustavo.romero@linaro.org; mst@redhat.com;
> marcel.apfelbaum@gmail.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 v7 02/12] hw/arm/smmu-common: Check SMMU has
> PCIe Root Complex association
> 
> On Tue, Jul 08, 2025 at 04:40:45PM +0100, Shameer Kolothum wrote:
> > @@ -937,11 +939,32 @@ static void smmu_base_realize(DeviceState
> *dev, Error **errp)
> >                                       g_free, g_free);
> >      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
> 
> Although this is not introduced by this patch, is there a
> g_hash_table_remove() somewhere in the code?

g_hash_table_remove()  is to remove a key/value pair, isn't it? Or you meant
a corresponding free in case of failure here? It's a realize() fn and errp is set
if something goes wrong and QEMU will exit. Not sure we need an explicit
free here.
 
> > +    /*
> > +     * 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), TYPE_PXB_PCIE_BUS)) {
> 
> The pci_bus_is_express(pci_bus) at the top is equivalent to:
> 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PCIE_BUS)
> Then here it is doing:
> 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PXB_PCIE_BUS)

Yes.

> So, this checks the same pci_bus but expects two different types?

In QEMU,  we can have three types of PCIe root complexes to be specified for
virt machine. 

1. default pcie.0 (TYPE_GPEX_HOST --> TYPE_PCIE_HOST_BRIDGE --> TYPE_PCI_HOST_BRIDGE)
2. pxb-pcie (TYPE_PXB_HOST  -->TYPE_PCI_HOST_BRIDGE)
2. pxb-cxl (TYPE_PXB_CXL_HOST  --> TYPE_PCI_HOST_BRIDGE)

The above first check is to see whether the bus is  PCIE && root bus && parent 
of type TYPE_PCI_HOST_BRIDGE. This will identify all the above three cases.

Both pxb-pcie and pxb-cxl are special extra root complexes based on PCI
expansion bridges and has a parent_dev set(both has pcie.0 has parent bus).

Hence we check to see parent_dev is set and make sure it is indeed 
TYPE_PXB_PCIE_BUS to avoid attaching to pxb-cxl. 

As mentioned in the commit log above, cxl support for virt is currently in
progress and once it has verified the functionality with SMMUv3
we can relax that check.

> I don't see the code check "PCIe Root Complex" explicitly, which
> should be TYPE_GPEX_HOST?

Hope it is clear now.

Thanks,
Shameer

RE: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, July 8, 2025 10:26 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; imammedo@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com;
> gustavo.romero@linaro.org; mst@redhat.com;
> marcel.apfelbaum@gmail.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 v7 07/12] hw/pci: Introduce
> pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval
> 
> On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote:
> > @@ -2909,6 +2909,19 @@ static void
> pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> >              }
> >          }
> >
> > +        /*
> > +         * When multiple PCI Express Root Buses are defined using pxb-
> pcie,
> > +         * the IOMMU configuration may be specific to each root bus.
> However,
> > +         * pxb-pcie acts as a special root complex whose parent is
> effectively
> > +         * the default root complex(pcie.0). Ensure that we retrieve the
> > +         * correct IOMMU ops(if any) in such cases.
> > +         */
> > +        if (pci_bus_is_express(iommu_bus) &&
> pci_bus_is_root(iommu_bus)) {
> > +            if (!iommu_bus->iommu_per_bus && parent_bus-
> >iommu_per_bus) {
> > +                break;
> 
> Mind elaborating why the current bus must unset iommu_per_bus while
> its parent sets iommu_per_bus?
> 
> My understanding is that for a pxb-pcie we should set iommu_per_bus
> but for its parent (the default root complex) we should unset its
> iommu_per_bus?

Well, for new arm-smmuv3 dev you need an associated pcie root
complex. Either the default pcie.0 or a pxb-pcie one. And as I
mentioned in my reply to the other thread(patch #2) and commit log here,
the pxb-pcie is special extra root complex in Qemu which has pcie.0 has
parent bus.

The above pci_device_get_iommu_bus_devfn() at present, iterate over the
parent_dev if it is set and returns the parent_bus IOMMU ops even if the
associated pxb-pcie bus doesn't have any IOMMU. This creates problem
for a case that is described here in the cover letter here,
https://lore.kernel.org/qemu-devel/20250708154055.101012-1-shameerali.kolothum.thodi@huawei.com/

(Please see "Major changes from v4:" section)

To address that issue, this patch introduces an new helper function to specify that
the IOMMU ops are specific to the associated root complex(iommu_per_bus) and
use that to return the correct IOMMU ops.

Hope with that context it is clear now.

Thanks,
Shameer

Re: [PATCH v7 02/12] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Nicolin Chen]

On Wed, Jul 09, 2025 at 08:08:49AM +0000, Shameerali Kolothum Thodi wrote:
> > On Tue, Jul 08, 2025 at 04:40:45PM +0100, Shameer Kolothum wrote:
> > > @@ -937,11 +939,32 @@ static void smmu_base_realize(DeviceState
> > *dev, Error **errp)
> > >                                       g_free, g_free);
> > >      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
> > 
> > Although this is not introduced by this patch, is there a
> > g_hash_table_remove() somewhere in the code?
> 
> g_hash_table_remove()  is to remove a key/value pair, isn't it?

Yes.

> Or you meant
> a corresponding free in case of failure here?

Yes. But I saw the other two g_hash_table_new_full calls were not
reverted in the exit path either. Then I saw smmu_base_reset_exit
does the clean up of those two but not this smmu_pcibus_by_busptr.

> It's a realize() fn and errp is set
> if something goes wrong and QEMU will exit. Not sure we need an explicit
> free here.
>  
> > > +    /*
> > > +     * 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), TYPE_PXB_PCIE_BUS)) {
> > 
> > The pci_bus_is_express(pci_bus) at the top is equivalent to:
> > 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PCIE_BUS)
> > Then here it is doing:
> > 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PXB_PCIE_BUS)
> 
> Yes.

Hmm?

We have these two types defined as two different strings, right?

#define TYPE_PCIE_BUS "PCIE"
#define TYPE_PXB_PCIE_BUS "pxb-pcie-bus"

So the first test is to make sure pci_bus string is "PCIE",
then the second one testing the same pci_bus string will
never be true?

> > So, this checks the same pci_bus but expects two different types?
> 
> In QEMU,  we can have three types of PCIe root complexes to be specified for
> virt machine. 
> 
> 1. default pcie.0 (TYPE_GPEX_HOST --> TYPE_PCIE_HOST_BRIDGE --> TYPE_PCI_HOST_BRIDGE)
> 2. pxb-pcie (TYPE_PXB_HOST  -->TYPE_PCI_HOST_BRIDGE)
> 2. pxb-cxl (TYPE_PXB_CXL_HOST  --> TYPE_PCI_HOST_BRIDGE)
> 
> The above first check is to see whether the bus is  PCIE && root bus && parent 
> of type TYPE_PCI_HOST_BRIDGE. This will identify all the above three cases.
> 
> Both pxb-pcie and pxb-cxl are special extra root complexes based on PCI
> expansion bridges and has a parent_dev set(both has pcie.0 has parent bus).
> 
> Hence we check to see parent_dev is set and make sure it is indeed 
> TYPE_PXB_PCIE_BUS to avoid attaching to pxb-cxl. 

I see. That's clear now. I think it'd help by writing:
		/*
		 * While pcie.0 doesn't set the parent_dev, either pxb-pcie or
		 * pxb-cxl does. Re-test the type to make sure it is pxb-pcie.
		 */

Thanks
Nicolin

Re: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Nicolin Chen]

On Wed, Jul 09, 2025 at 08:20:35AM +0000, Shameerali Kolothum Thodi wrote:
> > On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote:
> > > @@ -2909,6 +2909,19 @@ static void
> > pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> > >              }
> > >          }
> > >
> > > +        /*
> > > +         * When multiple PCI Express Root Buses are defined using pxb-
> > pcie,
> > > +         * the IOMMU configuration may be specific to each root bus.
> > However,
> > > +         * pxb-pcie acts as a special root complex whose parent is
> > effectively
> > > +         * the default root complex(pcie.0). Ensure that we retrieve the
> > > +         * correct IOMMU ops(if any) in such cases.
> > > +         */
> > > +        if (pci_bus_is_express(iommu_bus) &&
> > pci_bus_is_root(iommu_bus)) {
> > > +            if (!iommu_bus->iommu_per_bus && parent_bus-
> > >iommu_per_bus) {
> > > +                break;
> > 
> > Mind elaborating why the current bus must unset iommu_per_bus while
> > its parent sets iommu_per_bus?
> > 
> > My understanding is that for a pxb-pcie we should set iommu_per_bus
> > but for its parent (the default root complex) we should unset its
> > iommu_per_bus?
> 
> Well, for new arm-smmuv3 dev you need an associated pcie root
> complex. Either the default pcie.0 or a pxb-pcie one. And as I
> mentioned in my reply to the other thread(patch #2) and commit log here,
> the pxb-pcie is special extra root complex in Qemu which has pcie.0 has
> parent bus.
> 
> The above pci_device_get_iommu_bus_devfn() at present, iterate over the
> parent_dev if it is set and returns the parent_bus IOMMU ops even if the
> associated pxb-pcie bus doesn't have any IOMMU. This creates problem
> for a case that is described here in the cover letter here,
> https://lore.kernel.org/qemu-devel/20250708154055.101012-1-shameerali.kolothum.thodi@huawei.com/
> 
> (Please see "Major changes from v4:" section)
> 
> To address that issue, this patch introduces an new helper function to specify that
> the IOMMU ops are specific to the associated root complex(iommu_per_bus) and
> use that to return the correct IOMMU ops.
> 
> Hope with that context it is clear now.

Hmm, I was not questioning the context, I get what the patch is
supposed to do.

I was asking the logic that is unclear to me why it breaks when:
    !pxb-pcie->iommu_per_bus && pcie.0->iommu_per_bus

Or in which case pcie.0 would be set to iommu_per_bus=true?

Thanks
Nicolin

RE: [PATCH v7 02/12] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, July 10, 2025 12:54 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;
> gustavo.romero@linaro.org; mst@redhat.com;
> marcel.apfelbaum@gmail.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 v7 02/12] hw/arm/smmu-common: Check SMMU has
> PCIe Root Complex association
> 
> On Wed, Jul 09, 2025 at 08:08:49AM +0000, Shameerali Kolothum Thodi
> wrote:
> > > On Tue, Jul 08, 2025 at 04:40:45PM +0100, Shameer Kolothum wrote:
> > > > @@ -937,11 +939,32 @@ static void smmu_base_realize(DeviceState
> > > *dev, Error **errp)
> > > >                                       g_free, g_free);
> > > >      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
> > >
> > > Although this is not introduced by this patch, is there a
> > > g_hash_table_remove() somewhere in the code?
> >
> > g_hash_table_remove()  is to remove a key/value pair, isn't it?
> 
> Yes.
> 
> > Or you meant
> > a corresponding free in case of failure here?
> 
> Yes. But I saw the other two g_hash_table_new_full calls were not
> reverted in the exit path either. Then I saw smmu_base_reset_exit
> does the clean up of those two but not this smmu_pcibus_by_busptr.

Ok. I think that is by design. The insert for busptr cache happens during
early stages of Qemu through get_address_space() callback and
smmu_base_reset_exit() is called after that, just before the Guest boot.
So if you clean that cache at that time , you need to handle it differently
at a later stage. Also I don't think it makes much sense to clear busptr
before the Guest boot as it is not going to become stale unlike configs
and iotlb.

> > It's a realize() fn and errp is set
> > if something goes wrong and QEMU will exit. Not sure we need an explicit
> > free here.
> >
> > > > +    /*
> > > > +     * 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),
> TYPE_PXB_PCIE_BUS)) {
> > >
> > > The pci_bus_is_express(pci_bus) at the top is equivalent to:
> > > 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PCIE_BUS)
> > > Then here it is doing:
> > > 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PXB_PCIE_BUS)
> >
> > Yes.
> 
> Hmm?
> 
> We have these two types defined as two different strings, right?
> 
> #define TYPE_PCIE_BUS "PCIE"
> #define TYPE_PXB_PCIE_BUS "pxb-pcie-bus"
> 
> So the first test is to make sure pci_bus string is "PCIE",
> then the second one testing the same pci_bus string will
> never be true?
>

It will be true.

static const TypeInfo pxb_pcie_bus_info = {
    .name          = TYPE_PXB_PCIE_BUS,
    .parent        = TYPE_PCIE_BUS,
    .instance_size = sizeof(PXBBus),
    .class_init    = pxb_bus_class_init,
};

TYPE_PXB_PCIE_BUS has a parent TYPE_PCIE_BUS. And the function
object_dynamic_cast() does the magic. It will return non-null for an
exact object type and also for its parents in the QOM hierarchy.

> > > So, this checks the same pci_bus but expects two different types?
>
> > In QEMU,  we can have three types of PCIe root complexes to be specified
> for
> > virt machine.
> >
> > 1. default pcie.0 (TYPE_GPEX_HOST --> TYPE_PCIE_HOST_BRIDGE -->
> TYPE_PCI_HOST_BRIDGE)
> > 2. pxb-pcie (TYPE_PXB_HOST  -->TYPE_PCI_HOST_BRIDGE)
> > 2. pxb-cxl (TYPE_PXB_CXL_HOST  --> TYPE_PCI_HOST_BRIDGE)
> >
> > The above first check is to see whether the bus is  PCIE && root bus &&
> parent
> > of type TYPE_PCI_HOST_BRIDGE. This will identify all the above three
> cases.
> >
> > Both pxb-pcie and pxb-cxl are special extra root complexes based on PCI
> > expansion bridges and has a parent_dev set(both has pcie.0 has parent
> bus).
> >
> > Hence we check to see parent_dev is set and make sure it is indeed
> > TYPE_PXB_PCIE_BUS to avoid attaching to pxb-cxl.
> 
> I see. That's clear now. I think it'd help by writing:
> 		/*
> 		 * While pcie.0 doesn't set the parent_dev, either pxb-pcie
> or
> 		 * pxb-cxl does. Re-test the type to make sure it is pxb-pcie.
> 		 */

I think it is already captured in the comments in this patch.

Thanks,
Shameer

RE: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, July 10, 2025 1:07 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;
> gustavo.romero@linaro.org; mst@redhat.com;
> marcel.apfelbaum@gmail.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 v7 07/12] hw/pci: Introduce
> pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval
> 
> On Wed, Jul 09, 2025 at 08:20:35AM +0000, Shameerali Kolothum Thodi
> wrote:
> > > On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote:
> > > > @@ -2909,6 +2909,19 @@ static void
> > > pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> > > >              }
> > > >          }
> > > >
> > > > +        /*
> > > > +         * When multiple PCI Express Root Buses are defined using pxb-
> > > pcie,
> > > > +         * the IOMMU configuration may be specific to each root bus.
> > > However,
> > > > +         * pxb-pcie acts as a special root complex whose parent is
> > > effectively
> > > > +         * the default root complex(pcie.0). Ensure that we retrieve the
> > > > +         * correct IOMMU ops(if any) in such cases.
> > > > +         */
> > > > +        if (pci_bus_is_express(iommu_bus) &&
> > > pci_bus_is_root(iommu_bus)) {
> > > > +            if (!iommu_bus->iommu_per_bus && parent_bus-
> > > >iommu_per_bus) {
> > > > +                break;
> > >
> > > Mind elaborating why the current bus must unset iommu_per_bus
> while
> > > its parent sets iommu_per_bus?
> > >
> > > My understanding is that for a pxb-pcie we should set iommu_per_bus
> > > but for its parent (the default root complex) we should unset its
> > > iommu_per_bus?
> >
> > Well, for new arm-smmuv3 dev you need an associated pcie root
> > complex. Either the default pcie.0 or a pxb-pcie one. And as I
> > mentioned in my reply to the other thread(patch #2) and commit log
> here,
> > the pxb-pcie is special extra root complex in Qemu which has pcie.0 has
> > parent bus.
> >
> > The above pci_device_get_iommu_bus_devfn() at present, iterate over
> the
> > parent_dev if it is set and returns the parent_bus IOMMU ops even if the
> > associated pxb-pcie bus doesn't have any IOMMU. This creates problem
> > for a case that is described here in the cover letter here,
> > https://lore.kernel.org/qemu-devel/20250708154055.101012-1-
> shameerali.kolothum.thodi@huawei.com/
> >
> > (Please see "Major changes from v4:" section)
> >
> > To address that issue, this patch introduces an new helper function to
> specify that
> > the IOMMU ops are specific to the associated root
> complex(iommu_per_bus) and
> > use that to return the correct IOMMU ops.
> >
> > Hope with that context it is clear now.
> 
> Hmm, I was not questioning the context, I get what the patch is
> supposed to do.
> 
> I was asking the logic that is unclear to me why it breaks when:
>     !pxb-pcie->iommu_per_bus && pcie.0->iommu_per_bus
> 
> Or in which case pcie.0 would be set to iommu_per_bus=true?

Yes. Consider the example I gave in cover  letter,

-device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \
-device virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \
-device pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \
-device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \
-device pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \
-device virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1

pcie.0 is behind new SMMUv3 dev(smmuv3.1) and has iommu_per_bus set.
pcie.1 has no SMMv3U and iommu_per_bus is not set for it.

And we don't want pci_device_get_iommu_bus_devfn() to return pcie.0's
IOMMU ops for virtionet.1. Hence the break.

Thanks,
Shameer

RE: [PATCH v7 00/12] hw/arm/virt: Add support for user creatable SMMUv3 device

[Shameerali Kolothum Thodi via]

Hi Peter,

> -----Original Message-----
> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Sent: Tuesday, July 8, 2025 4:41 PM
> To: qemu-arm@nongnu.org; qemu-devel@nongnu.org
> Cc: 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; gustavo.romero@linaro.org; mst@redhat.com;
> marcel.apfelbaum@gmail.com; linuxarm@huawei.com;
> wangzhou1@hisilicon.com; jiangkunkun@huawei.com;
> jonathan.cameron@huawei.com; zhangfei.gao@linaro.org
> Subject: [PATCH v7 00/12] hw/arm/virt: Add support for user creatable
> SMMUv3 device
> 
> Hi All,
> 
> Changes from v6:
> https://lore.kernel.org/qemu-devel/20250703084643.85740-1-
> shameerali.kolothum.thodi@huawei.com/
> 
> 1. Fixed the warning case for DT support, reported by Eric(patch #8).
> 2. Picked up R-by's and T-by's. Thanks!
> 
> Please take a look and let me know. I think this is in a good shape now
> for 10.1.

I understand the soft-freeze for 10.1 is next week. Any chance this series
can be picked for 10.1? Please let me know.

Thanks,
Shameer
 

Re: [PATCH v7 00/12] hw/arm/virt: Add support for user creatable SMMUv3 device

[Peter Maydell]

On Thu, 10 Jul 2025 at 11:10, Shameerali Kolothum Thodi
<shameerali.kolothum.thodi@huawei.com> wrote:
>
>
> Hi Peter,
>
> > -----Original Message-----
> > From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Sent: Tuesday, July 8, 2025 4:41 PM
> > To: qemu-arm@nongnu.org; qemu-devel@nongnu.org
> > Cc: 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; gustavo.romero@linaro.org; mst@redhat.com;
> > marcel.apfelbaum@gmail.com; linuxarm@huawei.com;
> > wangzhou1@hisilicon.com; jiangkunkun@huawei.com;
> > jonathan.cameron@huawei.com; zhangfei.gao@linaro.org
> > Subject: [PATCH v7 00/12] hw/arm/virt: Add support for user creatable
> > SMMUv3 device
> >
> > Hi All,
> >
> > Changes from v6:
> > https://lore.kernel.org/qemu-devel/20250703084643.85740-1-
> > shameerali.kolothum.thodi@huawei.com/
> >
> > 1. Fixed the warning case for DT support, reported by Eric(patch #8).
> > 2. Picked up R-by's and T-by's. Thanks!
> >
> > Please take a look and let me know. I think this is in a good shape now
> > for 10.1.
>
> I understand the soft-freeze for 10.1 is next week. Any chance this series
> can be picked for 10.1? Please let me know.

I'm afraid it's already pretty late, and you seem to still have
at least one person with comments/questions about this v7
series which has only just hit the list in the last few days.
So I think we should leave this until 10.2.

-- PMM

Re: [PATCH v7 01/12] hw/arm/virt-acpi-build: Don't create ITS id mappings by default

[Eric Auger]

Hi Peter,

On 7/8/25 5:40 PM, Shameer Kolothum wrote:
> Commit d6afe18b7242 ("hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables
> when its=off") moved ITS group node generation under the its=on condition.
> However, it still creates rc_its_idmaps unconditionally, which results in
> duplicate ID mappings in the IORT table.
>
> Fixes:d6afe18b7242 ("hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off")

At least please could you take this fix?

Thanks

Eric
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Donald Dutile <ddutile@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index cd90c47976..724fad5ffa 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -329,12 +329,6 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          /* Sort the smmu idmap by input_base */
>          g_array_sort(rc_smmu_idmaps, iort_idmap_compare);
>  
> -        /*
> -         * Knowing the ID ranges from the RC to the SMMU, it's possible to
> -         * determine the ID ranges from RC that are directed to the ITS.
> -         */
> -        create_rc_its_idmaps(rc_its_idmaps, rc_smmu_idmaps);
> -
>          nb_nodes = 2; /* RC and SMMUv3 */
>          rc_mapping_count = rc_smmu_idmaps->len;
>  

Re: [PATCH v7 01/12] hw/arm/virt-acpi-build: Don't create ITS id mappings by default

[Peter Maydell]

On Thu, 10 Jul 2025 at 15:00, Eric Auger <eric.auger@redhat.com> wrote:
>
> Hi Peter,
>
> On 7/8/25 5:40 PM, Shameer Kolothum wrote:
> > Commit d6afe18b7242 ("hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables
> > when its=off") moved ITS group node generation under the its=on condition.
> > However, it still creates rc_its_idmaps unconditionally, which results in
> > duplicate ID mappings in the IORT table.
> >
> > Fixes:d6afe18b7242 ("hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off")
>
> At least please could you take this fix?

Sure; I've taken this patch into target-arm.next.

-- PMM

Re: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Donald Dutile]

On 7/10/25 3:37 AM, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Nicolin Chen <nicolinc@nvidia.com>
>> Sent: Thursday, July 10, 2025 1:07 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;
>> gustavo.romero@linaro.org; mst@redhat.com;
>> marcel.apfelbaum@gmail.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 v7 07/12] hw/pci: Introduce
>> pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval
>>
>> On Wed, Jul 09, 2025 at 08:20:35AM +0000, Shameerali Kolothum Thodi
>> wrote:
>>>> On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote:
>>>>> @@ -2909,6 +2909,19 @@ static void
>>>> pci_device_get_iommu_bus_devfn(PCIDevice *dev,
>>>>>               }
>>>>>           }
>>>>>
>>>>> +        /*
>>>>> +         * When multiple PCI Express Root Buses are defined using pxb-
>>>> pcie,
>>>>> +         * the IOMMU configuration may be specific to each root bus.
>>>> However,
>>>>> +         * pxb-pcie acts as a special root complex whose parent is
>>>> effectively
>>>>> +         * the default root complex(pcie.0). Ensure that we retrieve the
>>>>> +         * correct IOMMU ops(if any) in such cases.
>>>>> +         */
>>>>> +        if (pci_bus_is_express(iommu_bus) &&
>>>> pci_bus_is_root(iommu_bus)) {
>>>>> +            if (!iommu_bus->iommu_per_bus && parent_bus-
>>>>> iommu_per_bus) {
>>>>> +                break;
>>>>
>>>> Mind elaborating why the current bus must unset iommu_per_bus
>> while
>>>> its parent sets iommu_per_bus?
>>>>
>>>> My understanding is that for a pxb-pcie we should set iommu_per_bus
>>>> but for its parent (the default root complex) we should unset its
>>>> iommu_per_bus?
>>>
>>> Well, for new arm-smmuv3 dev you need an associated pcie root
>>> complex. Either the default pcie.0 or a pxb-pcie one. And as I
>>> mentioned in my reply to the other thread(patch #2) and commit log
>> here,
>>> the pxb-pcie is special extra root complex in Qemu which has pcie.0 has
>>> parent bus.
>>>
>>> The above pci_device_get_iommu_bus_devfn() at present, iterate over
>> the
>>> parent_dev if it is set and returns the parent_bus IOMMU ops even if the
>>> associated pxb-pcie bus doesn't have any IOMMU. This creates problem
>>> for a case that is described here in the cover letter here,
>>> https://lore.kernel.org/qemu-devel/20250708154055.101012-1-
>> shameerali.kolothum.thodi@huawei.com/
>>>
>>> (Please see "Major changes from v4:" section)
>>>
>>> To address that issue, this patch introduces an new helper function to
>> specify that
>>> the IOMMU ops are specific to the associated root
>> complex(iommu_per_bus) and
>>> use that to return the correct IOMMU ops.
>>>
>>> Hope with that context it is clear now.
>>
>> Hmm, I was not questioning the context, I get what the patch is
>> supposed to do.
>>
>> I was asking the logic that is unclear to me why it breaks when:
>>      !pxb-pcie->iommu_per_bus && pcie.0->iommu_per_bus
>>
>> Or in which case pcie.0 would be set to iommu_per_bus=true?
> 
> Yes. Consider the example I gave in cover  letter,
> 
> -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \
> -device virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \
> -device pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \
> -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \
> -device pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \
> -device virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1
> 
> pcie.0 is behind new SMMUv3 dev(smmuv3.1) and has iommu_per_bus set.
> pcie.1 has no SMMv3U and iommu_per_bus is not set for it.
pcie.1 doesn't?   then what is this line saying/meaning?:
  -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \

I read that as an smmuv3 attached to pcie.1, with an id of smmuv3.2;
just as I read this config:
  -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \
as an smmuv3 attached to pcie.0 iwth id smmuv3.1

> 
> And we don't want pci_device_get_iommu_bus_devfn() to return pcie.0's
> IOMMU ops for virtionet.1. Hence the break.
> 
> Thanks,
> Shameer
> 

RE: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Donald Dutile <ddutile@redhat.com>
> Sent: Thursday, July 10, 2025 5:00 PM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Nicolin Chen
> <nicolinc@nvidia.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> berrange@redhat.com; imammedo@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; gustavo.romero@linaro.org;
> mst@redhat.com; marcel.apfelbaum@gmail.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 v7 07/12] hw/pci: Introduce
> pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval
> 
> 
> 
> On 7/10/25 3:37 AM, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Nicolin Chen <nicolinc@nvidia.com>
> >> Sent: Thursday, July 10, 2025 1:07 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;
> >> gustavo.romero@linaro.org; mst@redhat.com;
> >> marcel.apfelbaum@gmail.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 v7 07/12] hw/pci: Introduce
> >> pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval
> >>
> >> On Wed, Jul 09, 2025 at 08:20:35AM +0000, Shameerali Kolothum Thodi
> >> wrote:
> >>>> On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote:
> >>>>> @@ -2909,6 +2909,19 @@ static void
> >>>> pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> >>>>>               }
> >>>>>           }
> >>>>>
> >>>>> +        /*
> >>>>> +         * When multiple PCI Express Root Buses are defined using
> >>>>> + pxb-
> >>>> pcie,
> >>>>> +         * the IOMMU configuration may be specific to each root bus.
> >>>> However,
> >>>>> +         * pxb-pcie acts as a special root complex whose parent
> >>>>> + is
> >>>> effectively
> >>>>> +         * the default root complex(pcie.0). Ensure that we retrieve the
> >>>>> +         * correct IOMMU ops(if any) in such cases.
> >>>>> +         */
> >>>>> +        if (pci_bus_is_express(iommu_bus) &&
> >>>> pci_bus_is_root(iommu_bus)) {
> >>>>> +            if (!iommu_bus->iommu_per_bus && parent_bus-
> >>>>> iommu_per_bus) {
> >>>>> +                break;
> >>>>
> >>>> Mind elaborating why the current bus must unset iommu_per_bus
> >> while
> >>>> its parent sets iommu_per_bus?
> >>>>
> >>>> My understanding is that for a pxb-pcie we should set
> iommu_per_bus
> >>>> but for its parent (the default root complex) we should unset its
> >>>> iommu_per_bus?
> >>>
> >>> Well, for new arm-smmuv3 dev you need an associated pcie root
> >>> complex. Either the default pcie.0 or a pxb-pcie one. And as I
> >>> mentioned in my reply to the other thread(patch #2) and commit log
> >> here,
> >>> the pxb-pcie is special extra root complex in Qemu which has pcie.0
> >>> has parent bus.
> >>>
> >>> The above pci_device_get_iommu_bus_devfn() at present, iterate over
> >> the
> >>> parent_dev if it is set and returns the parent_bus IOMMU ops even if
> >>> the associated pxb-pcie bus doesn't have any IOMMU. This creates
> >>> problem for a case that is described here in the cover letter here,
> >>> https://lore.kernel.org/qemu-devel/20250708154055.101012-1-
> >> shameerali.kolothum.thodi@huawei.com/
> >>>
> >>> (Please see "Major changes from v4:" section)
> >>>
> >>> To address that issue, this patch introduces an new helper function
> >>> to
> >> specify that
> >>> the IOMMU ops are specific to the associated root
> >> complex(iommu_per_bus) and
> >>> use that to return the correct IOMMU ops.
> >>>
> >>> Hope with that context it is clear now.
> >>
> >> Hmm, I was not questioning the context, I get what the patch is
> >> supposed to do.
> >>
> >> I was asking the logic that is unclear to me why it breaks when:
> >>      !pxb-pcie->iommu_per_bus && pcie.0->iommu_per_bus
> >>
> >> Or in which case pcie.0 would be set to iommu_per_bus=true?
> >
> > Yes. Consider the example I gave in cover  letter,
> >
> > -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ -device
> > virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \ -device
> > pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \ -device
> > arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \ -device
> > pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \ -device
> > virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1
> >
> > pcie.0 is behind new SMMUv3 dev(smmuv3.1) and has iommu_per_bus
> set.
> > pcie.1 has no SMMv3U and iommu_per_bus is not set for it.
> pcie.1 doesn't?   then what is this line saying/meaning?:
>   -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \
> 
> I read that as an smmuv3 attached to pcie.1, with an id of smmuv3.2; just
> as I read this config:
>   -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ as an smmuv3
> attached to pcie.0 iwth id smmuv3.1

Oops..I forgot to delete that from the config:
This is what I meant,

-device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \
-device virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \
-device pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \
-device pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \
-device virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1 \

Thanks,
Shameer

> 
> >
> > And we don't want pci_device_get_iommu_bus_devfn() to return pcie.0's
> > IOMMU ops for virtionet.1. Hence the break.
> >
> > Thanks,
> > Shameer
> >
> 

Re: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Nicolin Chen]

On Thu, Jul 10, 2025 at 04:21:41PM +0000, Shameerali Kolothum Thodi wrote:
> > >> On Wed, Jul 09, 2025 at 08:20:35AM +0000, Shameerali Kolothum Thodi
> > >> wrote:
> > >>>> On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote:
> > >>>>> @@ -2909,6 +2909,19 @@ static void
> > >>>> pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> > >>>>>               }
> > >>>>>           }
> > >>>>>
> > >>>>> +        /*
> > >>>>> +         * When multiple PCI Express Root Buses are defined using
> > >>>>> + pxb-
> > >>>> pcie,
> > >>>>> +         * the IOMMU configuration may be specific to each root bus.
> > >>>> However,
> > >>>>> +         * pxb-pcie acts as a special root complex whose parent
> > >>>>> + is
> > >>>> effectively
> > >>>>> +         * the default root complex(pcie.0). Ensure that we retrieve the
> > >>>>> +         * correct IOMMU ops(if any) in such cases.
> > >>>>> +         */
> > >>>>> +        if (pci_bus_is_express(iommu_bus) &&
> > >>>> pci_bus_is_root(iommu_bus)) {
> > >>>>> +            if (!iommu_bus->iommu_per_bus && parent_bus-
> > >>>>> iommu_per_bus) {
> > >>>>> +                break;
> > >>>>
> > >>>> Mind elaborating why the current bus must unset iommu_per_bus
> > >> while
> > >>>> its parent sets iommu_per_bus?
> > >>>>
> > >>>> My understanding is that for a pxb-pcie we should set
> > iommu_per_bus
> > >>>> but for its parent (the default root complex) we should unset its
> > >>>> iommu_per_bus?
> > >>>
> > >>> Well, for new arm-smmuv3 dev you need an associated pcie root
> > >>> complex. Either the default pcie.0 or a pxb-pcie one. And as I
> > >>> mentioned in my reply to the other thread(patch #2) and commit log
> > >> here,
> > >>> the pxb-pcie is special extra root complex in Qemu which has pcie.0
> > >>> has parent bus.
> > >>>
> > >>> The above pci_device_get_iommu_bus_devfn() at present, iterate over
> > >> the
> > >>> parent_dev if it is set and returns the parent_bus IOMMU ops even if
> > >>> the associated pxb-pcie bus doesn't have any IOMMU. This creates
> > >>> problem for a case that is described here in the cover letter here,
> > >>> https://lore.kernel.org/qemu-devel/20250708154055.101012-1-
> > >> shameerali.kolothum.thodi@huawei.com/
> > >>>
> > >>> (Please see "Major changes from v4:" section)
> > >>>
> > >>> To address that issue, this patch introduces an new helper function
> > >>> to
> > >> specify that
> > >>> the IOMMU ops are specific to the associated root
> > >> complex(iommu_per_bus) and
> > >>> use that to return the correct IOMMU ops.
> > >>>
> > >>> Hope with that context it is clear now.
> > >>
> > >> Hmm, I was not questioning the context, I get what the patch is
> > >> supposed to do.
> > >>
> > >> I was asking the logic that is unclear to me why it breaks when:
> > >>      !pxb-pcie->iommu_per_bus && pcie.0->iommu_per_bus
> > >>
> > >> Or in which case pcie.0 would be set to iommu_per_bus=true?
> > >
> > > Yes. Consider the example I gave in cover  letter,
> > >
> > > -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ -device
> > > virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \ -device
> > > pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \ -device
> > > arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \ -device
> > > pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \ -device
> > > virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1
> > >
> > > pcie.0 is behind new SMMUv3 dev(smmuv3.1) and has iommu_per_bus
> > set.
> > > pcie.1 has no SMMv3U and iommu_per_bus is not set for it.
> > pcie.1 doesn't?   then what is this line saying/meaning?:
> >   -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \
> > 
> > I read that as an smmuv3 attached to pcie.1, with an id of smmuv3.2; just
> > as I read this config:
> >   -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ as an smmuv3
> > attached to pcie.0 iwth id smmuv3.1
> 
> Oops..I forgot to delete that from the config:
> This is what I meant,
> 
> -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \
> -device virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \
> -device pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \
> -device pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \
> -device virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1 \

So, the logic is trying to avoid:
        "iommu_bus = parent_bus;"
for a case that parent_bus (pcie.0) having its own IOMMU.

But shouldn't it be just "if (parent_bus->iommu_per_bus)"?

Why does the current iommu_bus->iommu_per_bus has to be unset?

I think "iommu_bus = parent_bus" should be avoided too even if
the current iommu_bus has its own IOMMU, i.e. iommu_per_bus is
set?

Thanks
Nicolin

Re: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Donald Dutile]

On 7/10/25 12:21 PM, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Donald Dutile <ddutile@redhat.com>
>> Sent: Thursday, July 10, 2025 5:00 PM
>> To: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; Nicolin Chen
>> <nicolinc@nvidia.com>
>> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
>> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
>> berrange@redhat.com; imammedo@redhat.com; nathanc@nvidia.com;
>> mochs@nvidia.com; smostafa@google.com; gustavo.romero@linaro.org;
>> mst@redhat.com; marcel.apfelbaum@gmail.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 v7 07/12] hw/pci: Introduce
>> pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval
>>
>>
>>
>> On 7/10/25 3:37 AM, Shameerali Kolothum Thodi wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Nicolin Chen <nicolinc@nvidia.com>
>>>> Sent: Thursday, July 10, 2025 1:07 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;
>>>> gustavo.romero@linaro.org; mst@redhat.com;
>>>> marcel.apfelbaum@gmail.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 v7 07/12] hw/pci: Introduce
>>>> pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval
>>>>
>>>> On Wed, Jul 09, 2025 at 08:20:35AM +0000, Shameerali Kolothum Thodi
>>>> wrote:
>>>>>> On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote:
>>>>>>> @@ -2909,6 +2909,19 @@ static void
>>>>>> pci_device_get_iommu_bus_devfn(PCIDevice *dev,
>>>>>>>                }
>>>>>>>            }
>>>>>>>
>>>>>>> +        /*
>>>>>>> +         * When multiple PCI Express Root Buses are defined using
>>>>>>> + pxb-
>>>>>> pcie,
>>>>>>> +         * the IOMMU configuration may be specific to each root bus.
>>>>>> However,
>>>>>>> +         * pxb-pcie acts as a special root complex whose parent
>>>>>>> + is
>>>>>> effectively
>>>>>>> +         * the default root complex(pcie.0). Ensure that we retrieve the
>>>>>>> +         * correct IOMMU ops(if any) in such cases.
>>>>>>> +         */
>>>>>>> +        if (pci_bus_is_express(iommu_bus) &&
>>>>>> pci_bus_is_root(iommu_bus)) {
>>>>>>> +            if (!iommu_bus->iommu_per_bus && parent_bus-
>>>>>>> iommu_per_bus) {
>>>>>>> +                break;
>>>>>>
>>>>>> Mind elaborating why the current bus must unset iommu_per_bus
>>>> while
>>>>>> its parent sets iommu_per_bus?
>>>>>>
>>>>>> My understanding is that for a pxb-pcie we should set
>> iommu_per_bus
>>>>>> but for its parent (the default root complex) we should unset its
>>>>>> iommu_per_bus?
>>>>>
>>>>> Well, for new arm-smmuv3 dev you need an associated pcie root
>>>>> complex. Either the default pcie.0 or a pxb-pcie one. And as I
>>>>> mentioned in my reply to the other thread(patch #2) and commit log
>>>> here,
>>>>> the pxb-pcie is special extra root complex in Qemu which has pcie.0
>>>>> has parent bus.
>>>>>
>>>>> The above pci_device_get_iommu_bus_devfn() at present, iterate over
>>>> the
>>>>> parent_dev if it is set and returns the parent_bus IOMMU ops even if
>>>>> the associated pxb-pcie bus doesn't have any IOMMU. This creates
>>>>> problem for a case that is described here in the cover letter here,
>>>>> https://lore.kernel.org/qemu-devel/20250708154055.101012-1-
>>>> shameerali.kolothum.thodi@huawei.com/
>>>>>
>>>>> (Please see "Major changes from v4:" section)
>>>>>
>>>>> To address that issue, this patch introduces an new helper function
>>>>> to
>>>> specify that
>>>>> the IOMMU ops are specific to the associated root
>>>> complex(iommu_per_bus) and
>>>>> use that to return the correct IOMMU ops.
>>>>>
>>>>> Hope with that context it is clear now.
>>>>
>>>> Hmm, I was not questioning the context, I get what the patch is
>>>> supposed to do.
>>>>
>>>> I was asking the logic that is unclear to me why it breaks when:
>>>>       !pxb-pcie->iommu_per_bus && pcie.0->iommu_per_bus
>>>>
>>>> Or in which case pcie.0 would be set to iommu_per_bus=true?
>>>
>>> Yes. Consider the example I gave in cover  letter,
>>>
>>> -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ -device
>>> virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \ -device
>>> pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \ -device
>>> arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \ -device
>>> pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \ -device
>>> virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1
>>>
>>> pcie.0 is behind new SMMUv3 dev(smmuv3.1) and has iommu_per_bus
>> set.
>>> pcie.1 has no SMMv3U and iommu_per_bus is not set for it.
>> pcie.1 doesn't?   then what is this line saying/meaning?:
>>    -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \
>>
>> I read that as an smmuv3 attached to pcie.1, with an id of smmuv3.2; just
>> as I read this config:
>>    -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ as an smmuv3
>> attached to pcie.0 iwth id smmuv3.1
> 
> Oops..I forgot to delete that from the config:
> This is what I meant,
> 
> -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \
> -device virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \
> -device pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \
> -device pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \
> -device virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1 \
> 
> Thanks,
> Shameer
> 
the dirt is in the details... thanks for the clarification.
- Don

>>
>>>
>>> And we don't want pci_device_get_iommu_bus_devfn() to return pcie.0's
>>> IOMMU ops for virtionet.1. Hence the break.
>>>
>>> Thanks,
>>> Shameer
>>>
>>
> 

Re: [PATCH v7 02/12] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Nicolin Chen]

On Thu, Jul 10, 2025 at 07:27:10AM +0000, Shameerali Kolothum Thodi wrote:
> > On Wed, Jul 09, 2025 at 08:08:49AM +0000, Shameerali Kolothum Thodi
> > wrote:
> > > > On Tue, Jul 08, 2025 at 04:40:45PM +0100, Shameer Kolothum wrote:
> > > > > @@ -937,11 +939,32 @@ static void smmu_base_realize(DeviceState
> > > > *dev, Error **errp)
> > > > >                                       g_free, g_free);
> > > > >      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
> > > >
> > > > Although this is not introduced by this patch, is there a
> > > > g_hash_table_remove() somewhere in the code?
> > >
> > > g_hash_table_remove()  is to remove a key/value pair, isn't it?
> > 
> > Yes.
> > 
> > > Or you meant
> > > a corresponding free in case of failure here?
> > 
> > Yes. But I saw the other two g_hash_table_new_full calls were not
> > reverted in the exit path either. Then I saw smmu_base_reset_exit
> > does the clean up of those two but not this smmu_pcibus_by_busptr.
> 
> Ok. I think that is by design. The insert for busptr cache happens during
> early stages of Qemu through get_address_space() callback and
> smmu_base_reset_exit() is called after that, just before the Guest boot.
> So if you clean that cache at that time , you need to handle it differently
> at a later stage. Also I don't think it makes much sense to clear busptr
> before the Guest boot as it is not going to become stale unlike configs
> and iotlb.

Hmm, my main point was there is seemingly no "g_hash_table_remove"
for s->smmu_pcibus_by_busptr throughout the vIOMMU code.

> > > It's a realize() fn and errp is set
> > > if something goes wrong and QEMU will exit. Not sure we need an explicit
> > > free here.
> > >
> > > > > +    /*
> > > > > +     * 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),
> > TYPE_PXB_PCIE_BUS)) {
> > > >
> > > > The pci_bus_is_express(pci_bus) at the top is equivalent to:
> > > > 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PCIE_BUS)
> > > > Then here it is doing:
> > > > 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PXB_PCIE_BUS)
> > >
> > > Yes.
> > 
> > Hmm?
> > 
> > We have these two types defined as two different strings, right?
> > 
> > #define TYPE_PCIE_BUS "PCIE"
> > #define TYPE_PXB_PCIE_BUS "pxb-pcie-bus"
> > 
> > So the first test is to make sure pci_bus string is "PCIE",
> > then the second one testing the same pci_bus string will
> > never be true?
> >
> 
> It will be true.
> 
> static const TypeInfo pxb_pcie_bus_info = {
>     .name          = TYPE_PXB_PCIE_BUS,
>     .parent        = TYPE_PCIE_BUS,
>     .instance_size = sizeof(PXBBus),
>     .class_init    = pxb_bus_class_init,
> };
> 
> TYPE_PXB_PCIE_BUS has a parent TYPE_PCIE_BUS. And the function
> object_dynamic_cast() does the magic. It will return non-null for an
> exact object type and also for its parents in the QOM hierarchy.

I see. Thanks for the explain.

> > > > So, this checks the same pci_bus but expects two different types?
> >
> > > In QEMU,  we can have three types of PCIe root complexes to be specified
> > for
> > > virt machine.
> > >
> > > 1. default pcie.0 (TYPE_GPEX_HOST --> TYPE_PCIE_HOST_BRIDGE -->
> > TYPE_PCI_HOST_BRIDGE)
> > > 2. pxb-pcie (TYPE_PXB_HOST  -->TYPE_PCI_HOST_BRIDGE)
> > > 2. pxb-cxl (TYPE_PXB_CXL_HOST  --> TYPE_PCI_HOST_BRIDGE)
> > >
> > > The above first check is to see whether the bus is  PCIE && root bus &&
> > parent
> > > of type TYPE_PCI_HOST_BRIDGE. This will identify all the above three
> > cases.
> > >
> > > Both pxb-pcie and pxb-cxl are special extra root complexes based on PCI
> > > expansion bridges and has a parent_dev set(both has pcie.0 has parent
> > bus).
> > >
> > > Hence we check to see parent_dev is set and make sure it is indeed
> > > TYPE_PXB_PCIE_BUS to avoid attaching to pxb-cxl.
> > 
> > I see. That's clear now. I think it'd help by writing:
> > 		/*
> > 		 * While pcie.0 doesn't set the parent_dev, either pxb-pcie
> > or
> > 		 * pxb-cxl does. Re-test the type to make sure it is pxb-pcie.
> > 		 */
> 
> I think it is already captured in the comments in this patch.

But I couldn't understand until your further clarification in the
mail :(

Thanks
Nicolin

Re: [PATCH v7 02/12] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Nicolin Chen]

On Tue, Jul 08, 2025 at 04:40:45PM +0100, Shameer Kolothum wrote:
> We only allow default PCIe Root Complex(pcie.0) or pxb-pcie based extra
> root complexes to be associated with SMMU.
> 
> Although this change does not affect functionality at present, it is
> required when we add support for user-creatable SMMUv3 devices in
> future patches.
> 
> Note: Added a specific check to identify pxb-pcie to avoid matching
> pxb-cxl host bridges, which are also of type PCI_HOST_BRIDGE. This
> restriction can be relaxed once support for CXL devices on arm/virt
> is added and validated with SMMUv3.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Tested-by: Eric Auger <eric.auger@redhat.com> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

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

With a small suggestion for clarification.

> +    /*
> +     * 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.
> +         */

        /*
         * While pcie.0 doesn't set the parent_dev, either pxb-pcie or pxb-cxl
         * does. Re-test the type to make sure it is pxb-pcie indeed.
         */

> +        if (pci_bus->parent_dev) {
> +            if (!object_dynamic_cast(OBJECT(pci_bus), TYPE_PXB_PCIE_BUS)) {
> +                goto out_err;

RE: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, July 10, 2025 5:56 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Donald Dutile <ddutile@redhat.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org; eric.auger@redhat.com; peter.maydell@linaro.org;
> jgg@nvidia.com; berrange@redhat.com; imammedo@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com;
> gustavo.romero@linaro.org; mst@redhat.com;
> marcel.apfelbaum@gmail.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 v7 07/12] hw/pci: Introduce
> pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval
> 
> On Thu, Jul 10, 2025 at 04:21:41PM +0000, Shameerali Kolothum Thodi
> wrote:
> > > >> On Wed, Jul 09, 2025 at 08:20:35AM +0000, Shameerali Kolothum
> Thodi
> > > >> wrote:
> > > >>>> On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum
> wrote:
> > > >>>>> @@ -2909,6 +2909,19 @@ static void
> > > >>>> pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> > > >>>>>               }
> > > >>>>>           }
> > > >>>>>
> > > >>>>> +        /*
> > > >>>>> +         * When multiple PCI Express Root Buses are defined using
> > > >>>>> + pxb-
> > > >>>> pcie,
> > > >>>>> +         * the IOMMU configuration may be specific to each root
> bus.
> > > >>>> However,
> > > >>>>> +         * pxb-pcie acts as a special root complex whose parent
> > > >>>>> + is
> > > >>>> effectively
> > > >>>>> +         * the default root complex(pcie.0). Ensure that we retrieve
> the
> > > >>>>> +         * correct IOMMU ops(if any) in such cases.
> > > >>>>> +         */
> > > >>>>> +        if (pci_bus_is_express(iommu_bus) &&
> > > >>>> pci_bus_is_root(iommu_bus)) {
> > > >>>>> +            if (!iommu_bus->iommu_per_bus && parent_bus-
> > > >>>>> iommu_per_bus) {
> > > >>>>> +                break;
> > > >>>>
> > > >>>> Mind elaborating why the current bus must unset iommu_per_bus
> > > >> while
> > > >>>> its parent sets iommu_per_bus?
> > > >>>>
> > > >>>> My understanding is that for a pxb-pcie we should set
> > > iommu_per_bus
> > > >>>> but for its parent (the default root complex) we should unset its
> > > >>>> iommu_per_bus?
> > > >>>
> > > >>> Well, for new arm-smmuv3 dev you need an associated pcie root
> > > >>> complex. Either the default pcie.0 or a pxb-pcie one. And as I
> > > >>> mentioned in my reply to the other thread(patch #2) and commit
> log
> > > >> here,
> > > >>> the pxb-pcie is special extra root complex in Qemu which has pcie.0
> > > >>> has parent bus.
> > > >>>
> > > >>> The above pci_device_get_iommu_bus_devfn() at present, iterate
> over
> > > >> the
> > > >>> parent_dev if it is set and returns the parent_bus IOMMU ops even
> if
> > > >>> the associated pxb-pcie bus doesn't have any IOMMU. This creates
> > > >>> problem for a case that is described here in the cover letter here,
> > > >>> https://lore.kernel.org/qemu-devel/20250708154055.101012-1-
> > > >> shameerali.kolothum.thodi@huawei.com/
> > > >>>
> > > >>> (Please see "Major changes from v4:" section)
> > > >>>
> > > >>> To address that issue, this patch introduces an new helper function
> > > >>> to
> > > >> specify that
> > > >>> the IOMMU ops are specific to the associated root
> > > >> complex(iommu_per_bus) and
> > > >>> use that to return the correct IOMMU ops.
> > > >>>
> > > >>> Hope with that context it is clear now.
> > > >>
> > > >> Hmm, I was not questioning the context, I get what the patch is
> > > >> supposed to do.
> > > >>
> > > >> I was asking the logic that is unclear to me why it breaks when:
> > > >>      !pxb-pcie->iommu_per_bus && pcie.0->iommu_per_bus
> > > >>
> > > >> Or in which case pcie.0 would be set to iommu_per_bus=true?
> > > >
> > > > Yes. Consider the example I gave in cover  letter,
> > > >
> > > > -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ -device
> > > > virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \ -device
> > > > pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \ -device
> > > > arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \ -device
> > > > pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \ -device
> > > > virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1
> > > >
> > > > pcie.0 is behind new SMMUv3 dev(smmuv3.1) and has
> iommu_per_bus
> > > set.
> > > > pcie.1 has no SMMv3U and iommu_per_bus is not set for it.
> > > pcie.1 doesn't?   then what is this line saying/meaning?:
> > >   -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \
> > >
> > > I read that as an smmuv3 attached to pcie.1, with an id of smmuv3.2;
> just
> > > as I read this config:
> > >   -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ as an smmuv3
> > > attached to pcie.0 iwth id smmuv3.1
> >
> > Oops..I forgot to delete that from the config:
> > This is what I meant,
> >
> > -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \
> > -device virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \
> > -device pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \
> > -device pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \
> > -device virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1 \
> 
> So, the logic is trying to avoid:
>         "iommu_bus = parent_bus;"
> for a case that parent_bus (pcie.0) having its own IOMMU.
> 
> But shouldn't it be just "if (parent_bus->iommu_per_bus)"?
> 
> Why does the current iommu_bus->iommu_per_bus has to be unset?

I think that !iommu_bus->iommu_per_bus check will be always true as 
it enters the while loop only if !iommu_bus->iommu_ops case,

while (iommu_bus && !iommu_bus->iommu_ops && iommu_bus->parent_dev) {

}

So yes, I think that can be removed. I will double check though as I cant
recollect why I added that now.

> 
> I think "iommu_bus = parent_bus" should be avoided too even if
> the current iommu_bus has its own IOMMU, i.e. iommu_per_bus is
> set?

Why? Not clear to me. It only enters the loop if the current iommu_bus
doesn't have Iommu_ops set which in turn means iommu_per_bus is not set. 
Isn't it? . Do you have a particular configuration in mind where it will fail
otherwise?

Thanks,
Shameer

Re: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Nicolin Chen]

On Thu, Jul 10, 2025 at 09:40:28PM +0000, Shameerali Kolothum Thodi wrote:
> > So, the logic is trying to avoid:
> >         "iommu_bus = parent_bus;"
> > for a case that parent_bus (pcie.0) having its own IOMMU.
> > 
> > But shouldn't it be just "if (parent_bus->iommu_per_bus)"?
> > 
> > Why does the current iommu_bus->iommu_per_bus has to be unset?
> 
> I think that !iommu_bus->iommu_per_bus check will be always true as 
> it enters the while loop only if !iommu_bus->iommu_ops case,
> 
> while (iommu_bus && !iommu_bus->iommu_ops && iommu_bus->parent_dev) {
> 
> }

Yea, that makses sense. "!iommu_bus->iommu_ops" should imply that
"iommu_bus->iommu_per_bus" is unset.

> > I think "iommu_bus = parent_bus" should be avoided too even if
> > the current iommu_bus has its own IOMMU, i.e. iommu_per_bus is
> > set?
> 
> Why? Not clear to me. It only enters the loop if the current iommu_bus
> doesn't have Iommu_ops set which in turn means iommu_per_bus is not set. 
> Isn't it?

Yea. I missed that condition. But what I said still stays correct:
if iommu_bus (pcie.1) has its own IOMMU, it shouldn't take the ops
from the parent_bus. The thing is that this logic is inside a while
condition that has already excluded such a case.

Thanks
Nicolin

Re: [PATCH v7 07/12] hw/pci: Introduce pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval

[Nicolin Chen]

On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote:
> Currently, pci_setup_iommu() registers IOMMU ops for a given PCIBus.
> However, when retrieving IOMMU ops for a device using
> pci_device_get_iommu_bus_devfn(), the function checks the parent_dev
> and fetches IOMMU ops from the parent device, even if the current
> bus does not have any associated IOMMU ops.
> 
> This behavior works for now because QEMU's IOMMU implementations are
> globally scoped, and host bridges rely on the bypass_iommu property
> to skip IOMMU translation when needed.
> 
> However, this model will break with the soon to be introduced
> arm-smmuv3 device, which allows users to associate the IOMMU
> with a specific PCIe root complex (e.g., the default pcie.0
> or a pxb-pcie root complex).
> 
> For example, consider the following setup with multiple root
> complexes:
> 
> -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.0 \
> ...
> -device pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \
> -device pcie-root-port,id=pcie.port1,bus=pcie.1 \
> -device virtio-net-pci,bus=pcie.port1
> 
> In Qemu, pxb-pcie acts as a special root complex whose parent is
> effectively the default root complex(pcie.0). Hence, though pcie.1
> has no associated SMMUv3 as per above, pci_device_get_iommu_bus_devfn()
> will incorrectly return the IOMMU ops from pcie.0 due to the fallback
> via parent_dev.
> 
> To fix this, introduce a new helper pci_setup_iommu_per_bus() that
> explicitly sets the new iommu_per_bus field in the PCIBus structure.
> This helper will be used in a subsequent patch that adds support for
> the new arm-smmuv3 device.
> 
> Update pci_device_get_iommu_bus_devfn() to use iommu_per_bus when
> determining the correct IOMMU ops, ensuring accurate behavior for
> per-bus IOMMUs.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

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

With a nit:

> +        /*
> +         * When multiple PCI Express Root Buses are defined using pxb-pcie,
> +         * the IOMMU configuration may be specific to each root bus. However,
> +         * pxb-pcie acts as a special root complex whose parent is effectively
> +         * the default root complex(pcie.0). Ensure that we retrieve the
> +         * correct IOMMU ops(if any) in such cases.
> +         */
> +        if (pci_bus_is_express(iommu_bus) && pci_bus_is_root(iommu_bus)) {
> +            if (!iommu_bus->iommu_per_bus && parent_bus->iommu_per_bus) {
> +                break;
> +            }

I think this should just check "if (parent_bus->iommu_per_bus)",
which means that the parent's iommu bus is private so not shared
with any other PCI buses.

Re: [PATCH v7 00/12] hw/arm/virt: Add support for user creatable SMMUv3 device

[Nicolin Chen]

Hi Peter,

On Thu, Jul 10, 2025 at 12:48:20PM +0100, Peter Maydell wrote:
> On Thu, 10 Jul 2025 at 11:10, Shameerali Kolothum Thodi
> > > Changes from v6:
> > > https://lore.kernel.org/qemu-devel/20250703084643.85740-1-
> > > shameerali.kolothum.thodi@huawei.com/
> > >
> > > 1. Fixed the warning case for DT support, reported by Eric(patch #8).
> > > 2. Picked up R-by's and T-by's. Thanks!
> > >
> > > Please take a look and let me know. I think this is in a good shape now
> > > for 10.1.
> >
> > I understand the soft-freeze for 10.1 is next week. Any chance this series
> > can be picked for 10.1? Please let me know.
> 
> I'm afraid it's already pretty late, and you seem to still have
> at least one person with comments/questions about this v7
> series which has only just hit the list in the last few days.
> So I think we should leave this until 10.2.

Sorry for hitting this series late.

All my questions were addressed. And I have given my "Reviewed-by".
Once Shameer confirms (maybe with a v8), everything would be fine.

That being said, it's still up to you to take it or not :)

Thanks
Nicolin