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

Gustavo Romero posted 9 patches 4 months, 3 weeks ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Shannon Zhao <shannon.zhaosl@gmail.com>
There is a newer version of this series
[PATCH v5 5/9] hw/arm/virt-acpi-build: Factor out create_its_idmaps
Posted by Gustavo Romero 4 months, 3 weeks ago
Factor out a new function, create_its_idmaps(), from the current
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.auger@redhat.com>
Signed-off-by: Gustavo Romero <gustavo.romero@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 */
+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
+     * 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.
+     */
+     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.
+	 */
+        create_its_idmaps(its_idmaps, smmu_idmaps);
 
         nb_nodes = 3; /* RC, ITS, SMMUv3 */
         rc_mapping_count = smmu_idmaps->len + its_idmaps->len;
-- 
2.34.1


Re: [PATCH v5 5/9] hw/arm/virt-acpi-build: Factor out create_its_idmaps
Posted by Eric Auger 4 months, 2 weeks ago
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.auger@redhat.com>
> Signed-off-by: Gustavo Romero <gustavo.romero@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
> +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
> +     */
> +     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


Re: [PATCH v5 5/9] hw/arm/virt-acpi-build: Factor out create_its_idmaps
Posted by Gustavo Romero 4 months, 2 weeks ago
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.auger@redhat.com>
>> Signed-off-by: Gustavo Romero <gustavo.romero@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
>