| Message ID | 20250616131824.425315-8-gustavo.romero@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm: GIC 'its=off' ACPI table fixes | expand |
Hi Gustavo, On 6/16/25 3:18 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> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/arm/virt-acpi-build.c | 152 ++++++++++++-------- > tests/qtest/bios-tables-test-allowed-diff.h | 2 + > 2 files changed, 93 insertions(+), 61 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 6990bce3bb..2240421980 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -295,32 +295,42 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > /* 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); > + nb_nodes = 2; /* RC and SMMUv3 */ > + rc_mapping_count = smmu_idmaps->len; > + > + if (vms->its) { > + /* > + * 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; > + } I think I would create a helper that populates the its_idmap from the smmu_idmaps. To me to would make the code more readable. Also we could add a doc comment bout the helper explaining what it does. > > - if (next_range.input_base < idmap->input_base) { > - next_range.id_count = idmap->input_base - next_range.input_base; > + /* 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); > } > > - next_range.input_base = idmap->input_base + idmap->id_count; > + nb_nodes++; /* ITS */ > + rc_mapping_count += its_idmaps->len; > } > - > - /* 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); > - } > - > - nb_nodes = 3; /* RC, ITS, SMMUv3 */ > - rc_mapping_count = smmu_idmaps->len + 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); > @@ -329,31 +339,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 */ > + 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 */ > @@ -370,8 +392,10 @@ 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) */ > - build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET); > + if (vms->its) { I would use 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 */ > @@ -415,20 +439,24 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > range->id_count, smmu_offset); > } > > - /* Map DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */ see my previous comment > - 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 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). */ > + build_iort_id_mapping(table_data, range->input_base, > + range->id_count, IORT_NODE_OFFSET); > + } > } > } else { > - /* > - * 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); > + if (vms->its) { > + /* > + * 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); > + } > } > > acpi_table_end(linker, &table); > @@ -741,18 +769,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", Thanks Eric
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 6990bce3bb..2240421980 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -295,32 +295,42 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) /* 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); + nb_nodes = 2; /* RC and SMMUv3 */ + rc_mapping_count = smmu_idmaps->len; + + if (vms->its) { + /* + * 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; + } - if (next_range.input_base < idmap->input_base) { - next_range.id_count = idmap->input_base - next_range.input_base; + /* 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); } - next_range.input_base = idmap->input_base + idmap->id_count; + nb_nodes++; /* ITS */ + rc_mapping_count += its_idmaps->len; } - - /* 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); - } - - nb_nodes = 3; /* RC, ITS, SMMUv3 */ - rc_mapping_count = smmu_idmaps->len + 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); @@ -329,31 +339,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 */ + 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 */ @@ -370,8 +392,10 @@ 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) */ - build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET); + if (vms->its) { + /* 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 */ @@ -415,20 +439,24 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) range->id_count, smmu_offset); } - /* 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). */ - build_iort_id_mapping(table_data, range->input_base, - range->id_count, IORT_NODE_OFFSET); + if (vms->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). */ + build_iort_id_mapping(table_data, range->input_base, + range->id_count, IORT_NODE_OFFSET); + } } } else { - /* - * 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); + if (vms->its) { + /* + * 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); + } } acpi_table_end(linker, &table); @@ -741,18 +769,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",