[v5] hw/arm: GIC 'its=off' ACPI table fixes

[PATCH v5 4/9] hw/arm/virt-acpi-build: Improve comment in build_iort

Posted by Gustavo Romero 4 months, 3 weeks ago

When building the Root Complex table, the comment about the code that
maps the RC node to SMMU node is misleading because it reads
"RC -> SMMUv3 -> ITS", but the code is only mapping the RCs IDs to the
SMMUv3 node. The step of mapping from the SMMUv3 IDs to the ITS Group
node is actually defined in another table (in the SMMUv3 node). So
change the comment to read "RC -> SMMUv3" instead.

Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
---
 hw/arm/virt-acpi-build.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9eee284c80..e9cd3fb351 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -370,7 +370,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         /* DeviceID mapping index (ignored since interrupts are GSIV based) */
         build_append_int_noprefix(table_data, 0, 4);
 
-        /* output IORT node is the ITS group node (the first node) */
+        /* Output IORT node is the ITS Group node (the first node) */
         build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
     }
 
@@ -407,23 +407,36 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     if (vms->iommu == VIRT_IOMMU_SMMUV3) {
         AcpiIortIdMapping *range;
 
-        /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS */
+        /*
+         * 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.
+         */
         for (i = 0; i < smmu_idmaps->len; i++) {
             range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
-            /* output IORT node is the smmuv3 node */
+            /* Output IORT node is the SMMUv3 node. */
             build_iort_id_mapping(table_data, range->input_base,
                                   range->id_count, smmu_offset);
         }
 
-        /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
+        /*
+         * Map bypassed (don't go throught the SMMU) RIDs (input) to ITS Group
+         * node directly: RC -> ITS.
+         */
         for (i = 0; i < its_idmaps->len; i++) {
             range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
-            /* output IORT node is the ITS group node (the first node) */
+            /* Output IORT node is the ITS Group node (the first node). */
             build_iort_id_mapping(table_data, range->input_base,
                                   range->id_count, IORT_NODE_OFFSET);
         }
     } else {
-        /* output IORT node is the ITS group node (the first node) */
+        /*
+         * Map all RIDs (input) to ITS Group node directly, since there is no
+         * SMMU: RC -> ITS.
+         * Output IORT node is the ITS Group node (the first node).
+         */
         build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
     }
 
-- 
2.34.1

Re: [PATCH v5 4/9] hw/arm/virt-acpi-build: Improve comment in build_iort

Posted by Eric Auger 4 months, 3 weeks ago

Hi Gustavo,

On 6/23/25 3:57 PM, Gustavo Romero wrote:
> When building the Root Complex table, the comment about the code that
s/table/node? or do you refer to the IORT table?
> maps the RC node to SMMU node is misleading because it reads
> "RC -> SMMUv3 -> ITS", but the code is only mapping the RCs IDs to the
> SMMUv3 node. The step of mapping from the SMMUv3 IDs to the ITS Group
> node is actually defined in another table (in the SMMUv3 node). So
> change the comment to read "RC -> SMMUv3" instead.
>
> Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  hw/arm/virt-acpi-build.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9eee284c80..e9cd3fb351 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -370,7 +370,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          /* DeviceID mapping index (ignored since interrupts are GSIV based) */
>          build_append_int_noprefix(table_data, 0, 4);
>  
> -        /* output IORT node is the ITS group node (the first node) */
> +        /* Output IORT node is the ITS Group node (the first node) */
>          build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>      }
>  
> @@ -407,23 +407,36 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      if (vms->iommu == VIRT_IOMMU_SMMUV3) {
>          AcpiIortIdMapping *range;
>  
> -        /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS */
> +        /*
> +         * 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
s/table/node
> +         * ITS Group node.
> +         */
>          for (i = 0; i < smmu_idmaps->len; i++) {
>              range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> -            /* output IORT node is the smmuv3 node */
> +            /* Output IORT node is the SMMUv3 node. */
>              build_iort_id_mapping(table_data, range->input_base,
>                                    range->id_count, smmu_offset);
>          }
>  
> -        /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
> +        /*
> +         * Map bypassed (don't go throught the SMMU) RIDs (input) to ITS Group
> +         * node directly: RC -> ITS.
> +         */
>          for (i = 0; i < its_idmaps->len; i++) {
>              range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
> -            /* output IORT node is the ITS group node (the first node) */
> +            /* Output IORT node is the ITS Group node (the first node). */
>              build_iort_id_mapping(table_data, range->input_base,
>                                    range->id_count, IORT_NODE_OFFSET);
>          }
>      } else {
> -        /* output IORT node is the ITS group node (the first node) */
> +        /*
> +         * Map all RIDs (input) to ITS Group node directly, since there is no
> +         * SMMU: RC -> ITS.
> +         * Output IORT node is the ITS Group node (the first node).
> +         */
>          build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>      }
>  
Besides:
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric

Re: [PATCH v5 4/9] hw/arm/virt-acpi-build: Improve comment in build_iort

Posted by Eric Auger via 4 months, 3 weeks ago


On 6/27/25 3:57 PM, Eric Auger wrote:
> Hi Gustavo,
> 
> On 6/23/25 3:57 PM, Gustavo Romero wrote:
>> When building the Root Complex table, the comment about the code that
> s/table/node? or do you refer to the IORT table?
Reading the IORT spec again, both terminologies are used, ie. node / table

So please ignore my comment

Thanks

Eric
>> maps the RC node to SMMU node is misleading because it reads
>> "RC -> SMMUv3 -> ITS", but the code is only mapping the RCs IDs to the
>> SMMUv3 node. The step of mapping from the SMMUv3 IDs to the ITS Group
>> node is actually defined in another table (in the SMMUv3 node). So
>> change the comment to read "RC -> SMMUv3" instead.
>>
>> Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>  hw/arm/virt-acpi-build.c | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 9eee284c80..e9cd3fb351 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -370,7 +370,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>          /* DeviceID mapping index (ignored since interrupts are GSIV based) */
>>          build_append_int_noprefix(table_data, 0, 4);
>>  
>> -        /* output IORT node is the ITS group node (the first node) */
>> +        /* Output IORT node is the ITS Group node (the first node) */
>>          build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>>      }
>>  
>> @@ -407,23 +407,36 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>      if (vms->iommu == VIRT_IOMMU_SMMUV3) {
>>          AcpiIortIdMapping *range;
>>  
>> -        /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS */
>> +        /*
>> +         * 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
> s/table/node
>> +         * ITS Group node.
>> +         */
>>          for (i = 0; i < smmu_idmaps->len; i++) {
>>              range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
>> -            /* output IORT node is the smmuv3 node */
>> +            /* Output IORT node is the SMMUv3 node. */
>>              build_iort_id_mapping(table_data, range->input_base,
>>                                    range->id_count, smmu_offset);
>>          }
>>  
>> -        /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
>> +        /*
>> +         * Map bypassed (don't go throught the SMMU) RIDs (input) to ITS Group
>> +         * node directly: RC -> ITS.
>> +         */
>>          for (i = 0; i < its_idmaps->len; i++) {
>>              range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
>> -            /* output IORT node is the ITS group node (the first node) */
>> +            /* Output IORT node is the ITS Group node (the first node). */
>>              build_iort_id_mapping(table_data, range->input_base,
>>                                    range->id_count, IORT_NODE_OFFSET);
>>          }
>>      } else {
>> -        /* output IORT node is the ITS group node (the first node) */
>> +        /*
>> +         * Map all RIDs (input) to ITS Group node directly, since there is no
>> +         * SMMU: RC -> ITS.
>> +         * Output IORT node is the ITS Group node (the first node).
>> +         */
>>          build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>>      }
>>  
> Besides:
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Eric