Re: [PATCH v4 2/7] hw/arm/virt-acpi-build: Re-arrange SMMUv3 IORT build

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Fri, 13 Jun 2025 15:44:44 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> Introduces a new struct AcpiIortSMMUv3Dev to hold all the information
> required for SMMUv3 IORT node and use that for populating the node.
> 
> The current machine wide SMMUv3 is named as legacy SMMUv3 as we will
> soon add support for user-creatable SMMUv3 devices. These changes will
> be useful to have common code paths when we add that support.
> 
> Tested-by: Nathan Chen <nathanc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Some trivial stuff inline. Otherwise only comment in passing is that
some of the loop nests are deep enough that it might be worth
considering factoring some of those out as helper functions.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 111 +++++++++++++++++++++++++++------------
>  hw/arm/virt.c            |   1 +
>  include/hw/arm/virt.h    |   1 +
>  3 files changed, 79 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 7e8e0f0298..d39506179a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -266,6 +266,36 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
>      return idmap_a->input_base - idmap_b->input_base;
>  }
>  
> +struct AcpiIortSMMUv3Dev {
> +    int irq;
> +    hwaddr base;
> +    GArray *idmaps;
> +    /* Offset of the SMMUv3 IORT Node relative to the start of the IORT. */
> +    size_t offset;
> +};
> +typedef struct AcpiIortSMMUv3Dev AcpiIortSMMUv3Dev;

Hmm. This file is a bit inconsistent on style but there are instances of the more
compact

typedef struct AcpiIortSMMUv3Dev {
    int irq;
    hwaddr base;
    GArray *idmaps;
    /* Offset of the SMMUv3 IORT Node relative to the start of the IORT. */
    size_t offset;
} AcpiIortSMMUv3Dev;

> +
> +static void
> +populate_smmuv3_legacy_dev(GArray *sdev_blob)
What Nicolin said here.


> +{
> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> +    AcpiIortSMMUv3Dev sdev;
> +
> +    sdev.idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> +    object_child_foreach_recursive(object_get_root(),
> +                                   iort_host_bridges, sdev.idmaps);

Not sure why this wrap. I'd move iort_host_bridges up a line probably.

> +
> +    /*
> +     * There will be only one legacy SMMUv3 as it is a machine wide one.
> +     * And since it covers all the PCIe RCs in the machine, may have
> +     * multiple SMMUv3 idmaps. Sort it by input_base.
> +     */
> +    g_array_sort(sdev.idmaps, iort_idmap_compare);

I'd add a blank line here to make it more clear the comment only (I think)
applies to the one line of code and not this whole block.

> +    sdev.base = vms->memmap[VIRT_SMMU].base;
> +    sdev.irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> +    g_array_append_val(sdev_blob, sdev);
> +}