Re: [PATCH v5 5/9] hw/arm/virt-acpi-build: Factor out create_its_idmaps

[Gustavo Romero]

Hi Eric,

Thanks a lot for another round of reviews :)

On 6/27/25 12:28, Eric Auger wrote:

Hi Gustavo,

On 6/23/25 3:57 PM, Gustavo Romero wrote:

Factor out a new function, create_its_idmaps(), from the current

I would call it build_rc_its_idmap() to be clearer on what relationship
we build.

build_iort code. Add proper comments to it clarifying how the ID ranges
that go directly to the ITS Group node are computed based on the ones
that go to the SMMU node.

Suggested-by: Eric Auger <eric.au...@redhat.com>
Signed-off-by: Gustavo Romero <gustavo.rom...@linaro.org>
---
  hw/arm/virt-acpi-build.c | 64 +++++++++++++++++++++++++---------------
  1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index e9cd3fb351..40a782a498 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -266,6 +266,42 @@ static int iort_idmap_compare(gconstpointer a, 
gconstpointer b)
      return idmap_a->input_base - idmap_b->input_base;
  }

+/* Compute ID ranges (RIDs) from RC that do directly to the ITS Group node */

s/do/go
Ior use the spec terminology: that are directed to the ITS Group node

Thanks, I stuck to "directed to" in v6.

+static void create_its_idmaps(GArray *its_idmaps, GArray *smmu_idmaps)
+{
+    AcpiIortIdMapping *idmap;
+    AcpiIortIdMapping next_range = {0};
+
+    /*
+     * Based on the RID ranges that go to the SMMU, determine the bypassed RID

same here

+     * ranges, i.e., the ones that go directly to the ITS Group node, by
+     * subtracting the SMMU-bound ranges from the full RID range, 
0x0000–0xFFFF.

substracting

I believe that "subtracting" is the correct form (but I'm not a native English 
speaker, of course).
It seems the "substract" is sometimes used in ancient texts and I do see them 
occurring in the
commit messages, but they occur much less frequently than "subtract":

gromero@gromero0:/mnt/git/qemu_/build$ git log | grep substract | wc -l
13
gromero@gromero0:/mnt/git/qemu_/build$ git log | grep subtract | wc -l
188


Cheers,
Gustavo

+     */
+     for (int i = 0; i < smmu_idmaps->len; i++) {
+        idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
+
+        if (next_range.input_base < idmap->input_base) {
+            next_range.id_count = idmap->input_base - next_range.input_base;
+            g_array_append_val(its_idmaps, next_range);
+        }
+
+        next_range.input_base = idmap->input_base + idmap->id_count;
+    }
+
+    /*
+     * Append the last RC -> ITS ID mapping.
+     *
+     * RIDs are 16-bit, according to the PCI Express 2.0 Base Specification, 
rev
+     * 0.9, section 2.2.6.2, "Transaction Descriptor - Transaction ID Field",
+     * hence, the end of the range is 0x10000.
+     */
+    if (next_range.input_base < 0x10000) {
+        next_range.id_count = 0x10000 - next_range.input_base;
+        g_array_append_val(its_idmaps, next_range);
+    }
+}
+
+
  /*
   * Input Output Remapping Table (IORT)
   * Conforms to "IO Remapping Table System Software on ARM Platforms",
@@ -276,7 +312,6 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
VirtMachineState *vms)
  {
      int i, nb_nodes, rc_mapping_count;
      size_t node_size, smmu_offset = 0;
-    AcpiIortIdMapping *idmap;
      uint32_t id = 0;
      GArray *smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
      GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
@@ -287,34 +322,17 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
VirtMachineState *vms)
      acpi_table_begin(&table, table_data);

if (vms->iommu == VIRT_IOMMU_SMMUV3) {

-        AcpiIortIdMapping next_range = {0};
-
          object_child_foreach_recursive(object_get_root(),
                                         iort_host_bridges, smmu_idmaps);

/* Sort the smmu idmap by input_base */

          g_array_sort(smmu_idmaps, iort_idmap_compare);

- /*

-         * Split the whole RIDs by mapping from RC to SMMU,
-         * build the ID mapping from RC to ITS directly.
-         */
-        for (i = 0; i < smmu_idmaps->len; i++) {
-            idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
-
-            if (next_range.input_base < idmap->input_base) {
-                next_range.id_count = idmap->input_base - 
next_range.input_base;
-                g_array_append_val(its_idmaps, next_range);
-            }
-
-            next_range.input_base = idmap->input_base + idmap->id_count;
-        }
-
-        /* Append the last RC -> ITS ID mapping */
-        if (next_range.input_base < 0x10000) {
-            next_range.id_count = 0x10000 - next_range.input_base;
-            g_array_append_val(its_idmaps, next_range);
-        }
+       /*
+        * 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.

are directed to

+        */
+        create_its_idmaps(its_idmaps, smmu_idmaps);

nb_nodes = 3; /* RC, ITS, SMMUv3 */

          rc_mapping_count = smmu_idmaps->len + its_idmaps->len;

Thanks

Eric