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

[Gustavo Romero <gustavo.romero@linaro.org>]

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
>