Re: [PATCH v5 8/9] hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off

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

Hi Eric,

On 6/27/25 12:44, Eric Auger wrote:
> Hi Gustavo,
> 
> On 6/23/25 3:57 PM, Gustavo Romero wrote:
>> Currently, the ITS Group nodes in the IORT table and the GIC ITS Struct
>> in the MADT table are always generated, even if GIC ITS is not available
>> on the machine.
>>
>> This commit fixes it by not generating the ITS Group nodes, not mapping
>> any other node to them, and not advertising the GIC ITS in the MADT
>> table, when GIC ITS is not available on the machine.
>>
>> Since the fix changes the MADT and IORT tables, add the blobs for the
>> "its=off" test to the allow list and update them in the next commit.
>>
>> Reported-by: Udo Steinberg <udo@hypervisor.org>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2886
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> Co-authored-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/virt-acpi-build.c                    | 128 ++++++++++++--------
>>   tests/qtest/bios-tables-test-allowed-diff.h |   2 +
>>   2 files changed, 80 insertions(+), 50 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 40a782a498..17ae46804a 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -328,17 +328,27 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>           /* Sort the smmu idmap by input_base */
>>           g_array_sort(smmu_idmaps, iort_idmap_compare);
>>   
>> -	/*
>> -	 * 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;
>> +        nb_nodes = 2; /* RC and SMMUv3 */
>> +        rc_mapping_count = smmu_idmaps->len;
>> +
>> +        if (vms->its) {
>> +            /*
>> +             * 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++; /* ITS */
>> +            rc_mapping_count += its_idmaps->len;
>> +        }
>>       } else {
>> -        nb_nodes = 2; /* RC, ITS */
>> -        rc_mapping_count = 1;
>> +        if (vms->its) {
>> +            nb_nodes = 2; /* RC and ITS */
>> +            rc_mapping_count = 1; /* Direct map to ITS */
>> +        } else {
>> +            nb_nodes = 1; /* RC only */
>> +            rc_mapping_count = 0; /* No output mapping */
>> +        }
>>       }
>>       /* Number of IORT Nodes */
>>       build_append_int_noprefix(table_data, nb_nodes, 4);
>> @@ -347,31 +357,43 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>       build_append_int_noprefix(table_data, IORT_NODE_OFFSET, 4);
>>       build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>>   
>> -    /* Table 12 ITS Group Format */
>> -    build_append_int_noprefix(table_data, 0 /* ITS Group */, 1); /* Type */
>> -    node_size =  20 /* fixed header size */ + 4 /* 1 GIC ITS Identifier */;
>> -    build_append_int_noprefix(table_data, node_size, 2); /* Length */
>> -    build_append_int_noprefix(table_data, 1, 1); /* Revision */
>> -    build_append_int_noprefix(table_data, id++, 4); /* Identifier */
>> -    build_append_int_noprefix(table_data, 0, 4); /* Number of ID mappings */
>> -    build_append_int_noprefix(table_data, 0, 4); /* Reference to ID Array */
>> -    build_append_int_noprefix(table_data, 1, 4); /* Number of ITSs */
>> -    /* GIC ITS Identifier Array */
>> -    build_append_int_noprefix(table_data, 0 /* MADT translation_id */, 4);
>> +    if (vms->its) {
>> +        /* Table 12 ITS Group Format */
>> +        build_append_int_noprefix(table_data, 0 /* ITS Group */, 1); /* Type */
>> +        node_size =  20 /* fixed header size */ + 4 /* 1 GIC ITS Identifier */;
>> +        build_append_int_noprefix(table_data, node_size, 2); /* Length */
>> +        build_append_int_noprefix(table_data, 1, 1); /* Revision */
>> +        build_append_int_noprefix(table_data, id++, 4); /* Identifier */
>> +        build_append_int_noprefix(table_data, 0, 4); /* Number of ID mappings */
>> +        build_append_int_noprefix(table_data, 0, 4); /* Reference to ID Array */
>> +        build_append_int_noprefix(table_data, 1, 4); /* Number of ITSs */
>> +        /* GIC ITS Identifier Array */
>> +        build_append_int_noprefix(table_data, 0 /* MADT translation_id */, 4);
>> +    }
>>   
>>       if (vms->iommu == VIRT_IOMMU_SMMUV3) {
>>           int irq =  vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
>> -
>> +        int num_id_mappings, offset_to_id_array;
>> +
>> +        if (vms->its) {
>> +            num_id_mappings = 1; /* ITS Group node */
> While at it, I would suggest to rename num_id_mappings into
> smmu_mapping_count for consistency with
> 
> rc_mapping_count.
> 
> Also I would recommend to rename smmu_idmaps and its_idmaps into rc_smmu_idmaps and rc_its_idmaps to make things clearer. But that be done outside of thise series if you prefer.

Thanks, that's indeed a nice change for clarification. I've done it in v6.


> Otherwise this looks good.

I was not sure if I had to add your R-b in v6 after I addressed your comments so
I left v6 without a R-b.


Cheers,
Gustavo

> Thanks
> 
> Eric
> 
>> +            offset_to_id_array = SMMU_V3_ENTRY_SIZE; /* Just after the header */
>> +        } else {
>> +            num_id_mappings = 0; /* No ID mappings */
>> +            offset_to_id_array = 0; /* No ID mappings array */
>> +        }
>>           smmu_offset = table_data->len - table.table_offset;
>>           /* Table 9 SMMUv3 Format */
>>           build_append_int_noprefix(table_data, 4 /* SMMUv3 */, 1); /* Type */
>> -        node_size =  SMMU_V3_ENTRY_SIZE + ID_MAPPING_ENTRY_SIZE;
>> +        node_size =  SMMU_V3_ENTRY_SIZE +
>> +                     (ID_MAPPING_ENTRY_SIZE * num_id_mappings);
>>           build_append_int_noprefix(table_data, node_size, 2); /* Length */
>>           build_append_int_noprefix(table_data, 4, 1); /* Revision */
>>           build_append_int_noprefix(table_data, id++, 4); /* Identifier */
>> -        build_append_int_noprefix(table_data, 1, 4); /* Number of ID mappings */
>> +        /* Number of ID mappings */
>> +        build_append_int_noprefix(table_data, num_id_mappings, 4);
>>           /* Reference to ID Array */
>> -        build_append_int_noprefix(table_data, SMMU_V3_ENTRY_SIZE, 4);
>> +        build_append_int_noprefix(table_data, offset_to_id_array, 4);
>>           /* Base address */
>>           build_append_int_noprefix(table_data, vms->memmap[VIRT_SMMU].base, 8);
>>           /* Flags */
>> @@ -387,9 +409,11 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>           build_append_int_noprefix(table_data, 0, 4); /* Proximity domain */
>>           /* 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) */
>> -        build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>> +        /* Array of ID mappings */
>> +        if (num_id_mappings) {
>> +            /* Output IORT node is the ITS Group node (the first node). */
>> +            build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>> +        }
>>       }
>>   
>>       /* Table 17 Root Complex Node */
>> @@ -430,7 +454,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>            *
>>            * 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.
>> +         * ITS Group node, if ITS is available.
>>            */
>>           for (i = 0; i < smmu_idmaps->len; i++) {
>>               range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
>> @@ -439,15 +463,17 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>                                     range->id_count, smmu_offset);
>>           }
>>   
>> -        /*
>> -         * 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). */
>> -            build_iort_id_mapping(table_data, range->input_base,
>> -                                  range->id_count, IORT_NODE_OFFSET);
>> +        if (vms->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). */
>> +                build_iort_id_mapping(table_data, range->input_base,
>> +                                      range->id_count, IORT_NODE_OFFSET);
>> +            }
>>           }
>>       } else {
>>           /*
>> @@ -768,18 +794,20 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>                                             memmap[VIRT_HIGH_GIC_REDIST2].size);
>>           }
>>   
>> -        /*
>> -         * ACPI spec, Revision 6.0 Errata A
>> -         * (original 6.0 definition has invalid Length)
>> -         * 5.2.12.18 GIC ITS Structure
>> -         */
>> -        build_append_int_noprefix(table_data, 0xF, 1);  /* Type */
>> -        build_append_int_noprefix(table_data, 20, 1);   /* Length */
>> -        build_append_int_noprefix(table_data, 0, 2);    /* Reserved */
>> -        build_append_int_noprefix(table_data, 0, 4);    /* GIC ITS ID */
>> -        /* Physical Base Address */
>> -        build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
>> -        build_append_int_noprefix(table_data, 0, 4);    /* Reserved */
>> +        if (vms->its) {
>> +            /*
>> +             * ACPI spec, Revision 6.0 Errata A
>> +             * (original 6.0 definition has invalid Length)
>> +             * 5.2.12.18 GIC ITS Structure
>> +             */
>> +            build_append_int_noprefix(table_data, 0xF, 1);  /* Type */
>> +            build_append_int_noprefix(table_data, 20, 1);   /* Length */
>> +            build_append_int_noprefix(table_data, 0, 2);    /* Reserved */
>> +            build_append_int_noprefix(table_data, 0, 4);    /* GIC ITS ID */
>> +            /* Physical Base Address */
>> +            build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
>> +            build_append_int_noprefix(table_data, 0, 4);    /* Reserved */
>> +        }
>>       } else {
>>           const uint16_t spi_base = vms->irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE;
>>   
>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
>> index dfb8523c8b..a88198d5c2 100644
>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>> @@ -1 +1,3 @@
>>   /* List of comma-separated changed AML files to ignore */
>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
>> +"tests/data/acpi/aarch64/virt/IORT.its_off",
>