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

[Shameerali Kolothum Thodi via <qemu-devel@nongnu.org>]

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: Wednesday, July 2, 2025 9:34 AM
> To: 'Gustavo Romero' <gustavo.romero@linaro.org>; qemu-
> devel@nongnu.org; eric.auger@redhat.com; philmd@linaro.org;
> mst@redhat.com
> Cc: qemu-arm@nongnu.org; alex.bennee@linaro.org; udo@hypervisor.org;
> ajones@ventanamicro.com; peter.maydell@linaro.org;
> imammedo@redhat.com; anisinha@redhat.com
> Subject: RE: [PATCH v6 8/9] hw/arm/virt-acpi-build: Fix ACPI IORT and MADT
> tables when its=off
> 
> Hi Gustavo,
> 
> > -----Original Message-----
> > From: qemu-devel-
> > bounces+shameerali.kolothum.thodi=huawei.com@nongnu.org <qemu-
> > devel-bounces+shameerali.kolothum.thodi=huawei.com@nongnu.org>
> On
> > Behalf Of Gustavo Romero
> > Sent: Saturday, June 28, 2025 8:57 PM
> > To: qemu-devel@nongnu.org; eric.auger@redhat.com;
> philmd@linaro.org;
> > mst@redhat.com
> > Cc: qemu-arm@nongnu.org; alex.bennee@linaro.org;
> > gustavo.romero@linaro.org; udo@hypervisor.org;
> > ajones@ventanamicro.com; peter.maydell@linaro.org;
> > imammedo@redhat.com; anisinha@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 <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                    | 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.romero@linaro.org/
> 
> Or am I missing something here?

I have included a fix for the above in my SMMUv3 dev series here,
https://lore.kernel.org/qemu-devel/20250703084643.85740-2-shameerali.kolothum.thodi@huawei.com/

Please take a look and let me know if it doesn't make sense.

Thanks,
Shameer