> -----Original Message-----
> From: Nicolin Chen <nicol...@nvidia.com>
> Sent: Friday, May 2, 2025 6:14 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org;
> eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
> ddut...@redhat.com; berra...@redhat.com; nath...@nvidia.com;
> mo...@nvidia.com; smost...@google.com; Linuxarm
> <linux...@huawei.com>; Wangzhou (B) <wangzh...@hisilicon.com>;
> jiangkunkun <jiangkun...@huawei.com>; Jonathan Cameron
> <jonathan.came...@huawei.com>; zhangfei....@linaro.org
> Subject: Re: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for
> multiple smmuv3 devices
>
> On Fri, May 02, 2025 at 11:27:03AM +0100, Shameer Kolothum wrote:
> > @@ -43,6 +43,7 @@
> > #include "hw/acpi/generic_event_device.h"
> > #include "hw/acpi/tpm.h"
> > #include "hw/acpi/hmat.h"
> > +#include "hw/arm/smmuv3.h"
> > #include "hw/pci/pcie_host.h"
> > #include "hw/pci/pci.h"
> > #include "hw/pci/pci_bus.h"
> > @@ -266,6 +267,75 @@ static int iort_idmap_compare(gconstpointer a,
> gconstpointer b)
> > return idmap_a->input_base - idmap_b->input_base;
> > }
> >
> > +struct SMMUv3Device {
> > + int irq;
> > + hwaddr base;
> > + GArray *smmu_idmaps;
> > + size_t offset;
> > +};
> > +typedef struct SMMUv3Device SMMUv3Device;
>
> "SMMUv3Device" sounds too general, like coming from smmuv3.h :-/
>
> Given this describes SMMUv3's IORT node, I still think we should
> name it something like "IortSMMUv3Node" or so.
The way I thought about it is, it is mostly a structure to hold the SMMUv3
device
information, that will be used in IORT node creation. We are indeed going
through the
SMMUv3 devices created by virt and populating this. IORT is nothing but ACPI
way for
describing various devices like PCIE RC, SMMUv3 etc. I thought it is very clear
from code
that it is used in IORT and an explicit IORT in the name makes any difference
here 😊
Having said that I don’t have a strong bias towards this. I will consider it
when I respin.
>
> > +static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer
> b)
> > +{
> > + SMMUv3Device *sdev_a = (SMMUv3Device *)a;
> > + SMMUv3Device *sdev_b = (SMMUv3Device *)b;
> > + AcpiIortIdMapping *map_a = &g_array_index(sdev_a->smmu_idmaps,
> > + AcpiIortIdMapping, 0);
> > + AcpiIortIdMapping *map_b = &g_array_index(sdev_b->smmu_idmaps,
> > + AcpiIortIdMapping, 0);
> > + return map_a->input_base - map_b->input_base;
> > +}
> > +
> > +static void
> > +get_smmuv3_legacy_dev(VirtMachineState *vms, GArray *
> smmuv3_devices)
>
> GArray *smmuv3_devices
>
> Or maybe align with the non-legacy path, i.e. "sdev_blob"? Or the
> other way around.
Sure. I think void *sdev_blob for both is good.
> Otherwise, lgtm
> Reviewed-by: Nicolin Chen <nicol...@nvidia.com>
Thanks,
Shameer
