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

[Eric Auger via <qemu-devel@nongnu.org>]

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