The comment about the mapping from SMMU to ITS is incorrect and it reads
"RC -> ITS". The code in question actually maps SMMU -> ITS, so the
mapping in question is not direct. The direct mapping, i.e., RC -> ITS,
is handled a bit further down in the code, in the else block, and we
take the opportunity to update that as well, adding "RC -> ITS" to the
text so it's easier to see it's the direct map to the ITS Group node.
Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
---
hw/arm/virt-acpi-build.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9eee284c80..6990bce3bb 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -407,23 +407,27 @@ 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. */
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 DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> 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 bypassed RIDs (input) (don't go through the SMMU) to ITS Group
+ * nodes: 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.1Hi Gustavo,
On 6/16/25 3:18 PM, Gustavo Romero wrote:
> The comment about the mapping from SMMU to ITS is incorrect and it reads
> "RC -> ITS". The code in question actually maps SMMU -> ITS, so the
> mapping in question is not direct. The direct mapping, i.e., RC -> ITS,
> is handled a bit further down in the code, in the else block, and we
> take the opportunity to update that as well, adding "RC -> ITS" to the
> text so it's easier to see it's the direct map to the ITS Group node.
>
> Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
> ---
> hw/arm/virt-acpi-build.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9eee284c80..6990bce3bb 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -407,23 +407,27 @@ 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. */
yes this is what the code builds at this location.
> 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 DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */
But here I am confused. To me the its_idmaps matches the idmap between
RC and ITS. I understand this is built from holes left by bypassed
buses. See the build_iort() implementation. The comment at
/*
* 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;
}
is not crystal clear but it looks like filling the holes into its_idmap.
Besides there is another "Its group" instance to be replaced by ITS Group
Eric
> 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 bypassed RIDs (input) (don't go through the SMMU) to ITS Group
> + * nodes: RC -> ITS.
> + * Output IORT node is the ITS Group node (the first node).
> + */
> build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
> }
>
Hi Eric,
On 6/17/25 10:22, Eric Auger wrote:
> Hi Gustavo,
>
> On 6/16/25 3:18 PM, Gustavo Romero wrote:
>> The comment about the mapping from SMMU to ITS is incorrect and it reads
>> "RC -> ITS". The code in question actually maps SMMU -> ITS, so the
>> mapping in question is not direct. The direct mapping, i.e., RC -> ITS,
>> is handled a bit further down in the code, in the else block, and we
>> take the opportunity to update that as well, adding "RC -> ITS" to the
>> text so it's easier to see it's the direct map to the ITS Group node.
>>
>> Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>> hw/arm/virt-acpi-build.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 9eee284c80..6990bce3bb 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -407,23 +407,27 @@ 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. */
> yes this is what the code builds at this location.
>> 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 DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */
> But here I am confused. To me the its_idmaps matches the idmap between
> RC and ITS. I understand this is built from holes left by bypassed
> buses. See the build_iort() implementation. The comment at
ah, thanks! I see. Indeed, it's mapping the RC -> ITS, not the SMMU -> ITS.
I'll fix it in v5.
One thing that confused me, which I think is actually ok, is that the
output ID range from SMMU 0x000-0xFFFF) overlaps the output ID range
from the RC (e.g. 0x100-0xFFFF) because the SMMU output range is defined as
always taking the whole 16-bit range in:
[...]
366 build_append_int_noprefix(table_data, irq + 1, 4); /* PRI */
367 build_append_int_noprefix(table_data, irq + 3, 4); /* GERR */
368 build_append_int_noprefix(table_data, irq + 2, 4); /* Sync */
369 build_append_int_noprefix(table_data, 0, 4); /* Proximity domain */
370 /* DeviceID mapping index (ignored since interrupts are GSIV based) */
371 build_append_int_noprefix(table_data, 0, 4);
372
373 /* output IORT node is the ITS group node (the first node) */
374 build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET); <=========== HERE
375 }
376
377 /* Table 17 Root Complex Node */
I think that's ok since ITS can disambiguate the Device IDs from SMMU and from RC.
> /*
> * 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;
> }
>
> is not crystal clear but it looks like filling the holes into its_idmap.
k, I'll create a helper as you suggested in 7/8 to populate the its_idmaps.
> Besides there is another "Its group" instance to be replaced by ITS Group
Pardon, what do you mean? Another text to be replaced or it's just a comment
about the "else" block when smmu is off?
Thanks!
Cheers,
Gustavo
> Eric
>
>> 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 bypassed RIDs (input) (don't go through the SMMU) to ITS Group
>> + * nodes: RC -> ITS.
>> + * Output IORT node is the ITS Group node (the first node).
>> + */
>> build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>> }
>>
>
Hi Gustavo,
On 6/19/25 7:07 PM, Gustavo Romero wrote:
> Hi Eric,
>
> On 6/17/25 10:22, Eric Auger wrote:
>> Hi Gustavo,
>>
>> On 6/16/25 3:18 PM, Gustavo Romero wrote:
>>> The comment about the mapping from SMMU to ITS is incorrect and it
>>> reads
>>> "RC -> ITS". The code in question actually maps SMMU -> ITS, so the
>>> mapping in question is not direct. The direct mapping, i.e., RC -> ITS,
>>> is handled a bit further down in the code, in the else block, and we
>>> take the opportunity to update that as well, adding "RC -> ITS" to the
>>> text so it's easier to see it's the direct map to the ITS Group node.
>>>
>>> Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
>>> ---
>>> hw/arm/virt-acpi-build.c | 14 +++++++++-----
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 9eee284c80..6990bce3bb 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -407,23 +407,27 @@ 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. */
>> yes this is what the code builds at this location.
>>> 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 DeviceIDs (input) from SMMUv3 to ITS Group nodes:
>>> SMMU -> ITS. */
>> But here I am confused. To me the its_idmaps matches the idmap between
>> RC and ITS. I understand this is built from holes left by bypassed
>> buses. See the build_iort() implementation. The comment at
>
> ah, thanks! I see. Indeed, it's mapping the RC -> ITS, not the SMMU ->
> ITS.
> I'll fix it in v5.
>
> One thing that confused me, which I think is actually ok, is that the
> output ID range from SMMU 0x000-0xFFFF) overlaps the output ID range
> from the RC (e.g. 0x100-0xFFFF) because the SMMU output range is
> defined as
> always taking the whole 16-bit range in:
Agreed. However I think it is fine because those are ID mappings between
different input/output space pairs.
RC -> ITS
SMMU -> ITS
>
> [...]
> 366 build_append_int_noprefix(table_data, irq + 1, 4); /*
> PRI */
> 367 build_append_int_noprefix(table_data, irq + 3, 4); /*
> GERR */
> 368 build_append_int_noprefix(table_data, irq + 2, 4); /*
> Sync */
> 369 build_append_int_noprefix(table_data, 0, 4); /*
> Proximity domain */
> 370 /* DeviceID mapping index (ignored since interrupts are
> GSIV based) */
> 371 build_append_int_noprefix(table_data, 0, 4);
> 372
> 373 /* output IORT node is the ITS group node (the first
> node) */
> 374 build_iort_id_mapping(table_data, 0, 0x10000,
> IORT_NODE_OFFSET); <=========== HERE
> 375 }
> 376
> 377 /* Table 17 Root Complex Node */
>
>
> I think that's ok since ITS can disambiguate the Device IDs from SMMU
> and from RC.
>
>
>> /*
>> * 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;
>> }
>>
>> is not crystal clear but it looks like filling the holes into its_idmap.
>
> k, I'll create a helper as you suggested in 7/8 to populate the
> its_idmaps.
>
> > Besides there is another "Its group" instance to be replaced by ITS
> Group
>
> Pardon, what do you mean? Another text to be replaced or it's just a
> comment
> about the "else" block when smmu is off?
sorry, I meant another string to be replaced in the file
Cheers
Eric
>
> Thanks!
>
>
> Cheers,
> Gustavo
>
>> Eric
>>
>>> 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 bypassed RIDs (input) (don't go through the SMMU) to
>>> ITS Group
>>> + * nodes: RC -> ITS.
>>> + * Output IORT node is the ITS Group node (the first node).
>>> + */
>>> build_iort_id_mapping(table_data, 0, 0x10000,
>>> IORT_NODE_OFFSET);
>>> }
>>>
>>
>
© 2016 - 2025 Red Hat, Inc.