RE: [PATCH 2/5] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

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

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, April 16, 2025 5:19 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH 2/5] hw/arm/virt-acpi-build: Update IORT for multiple
> smmuv3 devices
> 
> On Tue, Apr 15, 2025 at 09:11:01AM +0100, Shameer Kolothum wrote:
> > +static int get_smmuv3_device(Object *obj, void *opaque)
> > +{
> > +    GArray *sdev_blob = opaque;
> > +    int min_bus, max_bus;
> > +    VirtMachineState *vms;
> > +    PlatformBusDevice *pbus;
> > +    SysBusDevice *sbdev;
> > +    SMMUv3Device sdev;
> > +    hwaddr base;
> > +    int irq;
> > +    PCIBus *bus;
> 
> In a reverse christmas tree order? Or we could at least group
> those similar types together.

Yeah. Reverse will look better I guess.
> 
> > +    vms = VIRT_MACHINE(qdev_get_machine());
> > +    pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> > +    sbdev = SYS_BUS_DEVICE(obj);
> > +    base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> > +    irq = platform_bus_get_irqn(pbus, sbdev, 0);
> > +
> > +    base += vms->memmap[VIRT_PLATFORM_BUS].base;
> > +    irq += vms->irqmap[VIRT_PLATFORM_BUS];
> > +
> > +    pci_bus_range(bus, &min_bus, &max_bus);
> > +    sdev.smmu_idmap.input_base = min_bus << 8;
> > +    sdev.smmu_idmap.id_count = (max_bus - min_bus + 1) << 8;
> > +    sdev.base = base;
> > +    sdev.irq = irq + ARM_SPI_BASE;
> 
> Hmm, these base/irq local variables don't look necessary.
> 
> > +    g_array_append_val(sdev_blob, sdev);
> > +    return 0;
> > +}
> > +
> >  /*
> >   * Input Output Remapping Table (IORT)
> >   * Conforms to "IO Remapping Table System Software on ARM
> Platforms",
> > @@ -275,25 +330,42 @@ static void
> >  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState
> *vms)
> >  {
> >      int i, nb_nodes, rc_mapping_count;
> > -    size_t node_size, smmu_offset = 0;
> > +    size_t node_size, *smmu_offset = NULL;
> >      AcpiIortIdMapping *idmap;
> > +    int num_smmus = 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 *smmuv3_devices = g_array_new(false, true,
> sizeof(SMMUv3Device));
> >
> >      AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
> >                          .oem_table_id = vms->oem_table_id };
> >      /* Table 2 The IORT */
> >      acpi_table_begin(&table, table_data);
> >
> > -    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> > -        AcpiIortIdMapping next_range = {0};
> > -
> > +    nb_nodes = 2; /* RC, ITS */
> > +    if (vms->iommu == VIRT_IOMMU_SMMUV3_DEV) {
> > +        object_child_foreach_recursive(object_get_root(),
> > +                                       get_smmuv3_device, smmuv3_devices);
> > +         /* Sort the smmuv3-devices by smmu idmap input_base */
> > +        g_array_sort(smmuv3_devices, smmuv3_dev_idmap_compare);
> > +        /*  Fill smmu idmap from sorted smmuv3 devices array */
> > +        for (i = 0; i < smmuv3_devices->len; i++) {
> > +            SMMUv3Device *s = &g_array_index(smmuv3_devices,
> SMMUv3Device, i);
> > +            g_array_append_val(smmu_idmaps, s->smmu_idmap);
> > +        }
> > +        num_smmus = smmuv3_devices->len;
> > +    } else if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> >          object_child_foreach_recursive(object_get_root(),
> >                                         iort_host_bridges, smmu_idmaps);
> >
> >          /* Sort the smmu idmap by input_base */
> >          g_array_sort(smmu_idmaps, iort_idmap_compare);
> 
> VIRT_IOMMU_SMMUV3 seems to fit the struct SMMUv3Device very well,
> given that it has base, irq, and smmu_idmaps too?

One difference though is the legacy VIRT_IOMMU_SMMUV3 one is a global
Machine wide one nad can be associated with multiple PCIe root complexes
which will result in smmu_idmaps array. See the iort_host_bridges() fn.

> 
> Maybe we could generalize the struct SMMUv3Device to store both
> cases. Perhaps just SMMUv3AcpiInfo? And then ..

Could do. But then you have to have  a smmu_idmaps array and then check
the length of it to cover legacy SMMUv3 case I guess.

 > > @@ -341,10 +413,20 @@ build_iort(GArray *table_data, BIOSLinker
> *linker, VirtMachineState *vms)
> >      /* 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;
> > +    for (i = 0; i < num_smmus; i++) {
> > +        hwaddr base;
> > +        int irq;
> > +
> > +        if (vms->iommu == VIRT_IOMMU_SMMUV3_DEV) {
> > +            SMMUv3Device *s = &g_array_index(smmuv3_devices,
> SMMUv3Device, i);
> > +            base = s->base;
> > +            irq = s->irq;
> > +        } else {
> > +            base = vms->memmap[VIRT_SMMU].base;
> > +            irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> > +        }
> 
> .. we wouldn't need two paths here.
> 
> > @@ -404,15 +486,26 @@ build_iort(GArray *table_data, BIOSLinker
> *linker, VirtMachineState *vms)
> >      build_append_int_noprefix(table_data, 0, 3); /* Reserved */
> >
> >      /* Output Reference */
> > -    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> > +    if (num_smmus) {
> >          AcpiIortIdMapping *range;
> >
> >          /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS
> */
> >          for (i = 0; i < smmu_idmaps->len; i++) {
> > +            size_t offset;
> > +            if (vms->iommu == VIRT_IOMMU_SMMUV3_DEV) {
> > +                offset = smmu_offset[i];
> > +            } else {
> > +                /*
> > +                 * For legacy VIRT_IOMMU_SMMUV3 case, one machine wide
> > +                 * smmuv3 may have multiple smmu_idmaps.
> > +                 */
> > +                offset = smmu_offset[0];
> > +            }
> 
> And I think we can eliminate this too if we stuff an smmu_offset
> in struct AcpiIortIdMapping ..
> 
> >              range = &g_array_index(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);
> > +                                  range->id_count, offset);
> 
> ... and here it would be range->offset.

I will give it a try and if that simplifies things will include it in next respin.

Thanks,
Shameer