Hi Gustavo, > -----Original Message----- > From: qemu-devel- > bounces+shameerali.kolothum.thodi=huawei....@nongnu.org <qemu- > devel-bounces+shameerali.kolothum.thodi=huawei....@nongnu.org> On > Behalf Of Gustavo Romero > Sent: Saturday, June 28, 2025 8:57 PM > To: qemu-devel@nongnu.org; eric.au...@redhat.com; phi...@linaro.org; > m...@redhat.com > Cc: qemu-...@nongnu.org; alex.ben...@linaro.org; > gustavo.rom...@linaro.org; u...@hypervisor.org; > ajo...@ventanamicro.com; peter.mayd...@linaro.org; > imamm...@redhat.com; anisi...@redhat.com > Subject: [PATCH v6 8/9] hw/arm/virt-acpi-build: Fix ACPI IORT and MADT > tables when its=off > > 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. > > This commit also renames the smmu_idmaps and its_idmaps variables in > build_iort() to rc_smmu_idmaps and rc_its_idmaps, respectively, to make > it clearer which nodes are involved in the mappings associated with > these variables. > > Reported-by: Udo Steinberg <u...@hypervisor.org> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2886 > Signed-off-by: Gustavo Romero <gustavo.rom...@linaro.org> > Co-authored-by: Philippe Mathieu-Daudé <phi...@linaro.org> > --- > hw/arm/virt-acpi-build.c | 142 ++++++++++++-------- > tests/qtest/bios-tables-test-allowed-diff.h | 2 + > 2 files changed, 90 insertions(+), 54 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 068383f982..eff0d698df 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -267,7 +267,7 @@ static int iort_idmap_compare(gconstpointer a, > gconstpointer b) > } > > /* Compute ID ranges (RIDs) from RC that are directed to the ITS Group > node */ > -static void create_its_idmaps(GArray *its_idmaps, GArray *smmu_idmaps) > +static void create_rc_its_idmaps(GArray *its_idmaps, GArray > *smmu_idmaps) > { > AcpiIortIdMapping *idmap; > AcpiIortIdMapping next_range = {0}; > @@ -314,8 +314,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > int i, nb_nodes, rc_mapping_count; > size_t node_size, smmu_offset = 0; > uint32_t id = 0; > - GArray *smmu_idmaps = g_array_new(false, true, > sizeof(AcpiIortIdMapping)); > - GArray *its_idmaps = g_array_new(false, true, > sizeof(AcpiIortIdMapping)); > + GArray *rc_smmu_idmaps = g_array_new(false, true, > sizeof(AcpiIortIdMapping)); > + GArray *rc_its_idmaps = g_array_new(false, true, > sizeof(AcpiIortIdMapping)); > > AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id, > .oem_table_id = vms->oem_table_id }; > @@ -324,22 +324,38 @@ build_iort(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > > if (vms->iommu == VIRT_IOMMU_SMMUV3) { > object_child_foreach_recursive(object_get_root(), > - iort_host_bridges, smmu_idmaps); > + iort_host_bridges, rc_smmu_idmaps); > > /* Sort the smmu idmap by input_base */ > - g_array_sort(smmu_idmaps, iort_idmap_compare); > + g_array_sort(rc_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 are directed to the ITS. > */ > - create_its_idmaps(its_idmaps, smmu_idmaps); > + create_rc_its_idmaps(rc_its_idmaps, rc_smmu_idmaps);
Hmm...not sure why we still need the above now as this is being moved down for vms->its is set case. I had a look at v5, which seems to be doing the right thing. https://lore.kernel.org/qemu-devel/20250623135749.691137-9-gustavo.rom...@linaro.org/ Or am I missing something here? Please let me know. Thanks, Shameer > > - nb_nodes = 3; /* RC, ITS, SMMUv3 */ > - rc_mapping_count = smmu_idmaps->len + its_idmaps->len; > + nb_nodes = 2; /* RC and SMMUv3 */ > + rc_mapping_count = rc_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_rc_its_idmaps(rc_its_idmaps, rc_smmu_idmaps); > + > + nb_nodes++; /* ITS */ > + rc_mapping_count += rc_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); > @@ -348,31 +364,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 smmu_mapping_count, offset_to_id_array; > + > + if (vms->its) { > + smmu_mapping_count = 1; /* ITS Group node */ > + offset_to_id_array = SMMU_V3_ENTRY_SIZE; /* Just after the > header */ > + } else { > + smmu_mapping_count = 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 * smmu_mapping_count); > 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, smmu_mapping_count, 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 */ > @@ -388,9 +416,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 (smmu_mapping_count) { > + /* 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 */ > @@ -431,24 +461,26 @@ 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); > + for (i = 0; i < rc_smmu_idmaps->len; i++) { > + range = &g_array_index(rc_smmu_idmaps, AcpiIortIdMapping, i); > /* Output IORT node is the SMMUv3 node. */ > build_iort_id_mapping(table_data, range->input_base, > 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 < rc_its_idmaps->len; i++) { > + range = &g_array_index(rc_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 { > /* > @@ -460,8 +492,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > } > > acpi_table_end(linker, &table); > - g_array_free(smmu_idmaps, true); > - g_array_free(its_idmaps, true); > + g_array_free(rc_smmu_idmaps, true); > + g_array_free(rc_its_idmaps, true); > } > > /* > @@ -769,18 +801,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", > -- > 2.34.1 >
