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

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

> -----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 Eric Auger
> Sent: Monday, May 5, 2025 9:40 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@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 v2 2/6] hw/arm/virt-acpi-build: Update IORT for
> multiple smmuv3 devices
> 
> Hi Shameer,
> 
> On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> > With the soon to be introduced user-creatable SMMUv3 devices for
> > virt, it is possible to have multiple SMMUv3 devices associated
> > with different PCIe root complexes.
> >
> > Update IORT nodes accordingly.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/virt-acpi-build.c | 162 +++++++++++++++++++++++++++++++--------
> I would recommend to split that patch. I think you can first introduce
> the new struct and adapting the exicting code for the "legacy
> instantiation mode" and then have a saparet patch for handling user
> created instances. It will be easier to review.

Ok. I will split this patch.

> 
> At some point in the series you shall check the user is not attempting
> to use legacy mode and user creatable mode concurrently. I don't know if
> it is done yet.

Yes, it is done in patch #5. See changes to virt_machine_device_pre_plug_cb().


> >  hw/arm/virt.c            |   1 +
> >  include/hw/arm/virt.h    |   1 +
> >  3 files changed, 131 insertions(+), 33 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 3ac8f8e178..3ce70f8fa9 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -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;
> > +
> > +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)
> > +{
> > +    SMMUv3Device sdev;
> > +
> > +    sdev.smmu_idmaps = g_array_new(false, true,
> sizeof(AcpiIortIdMapping));
> > +    object_child_foreach_recursive(object_get_root(),
> > +                                   iort_host_bridges, sdev.smmu_idmaps);
> > +    sdev.base = vms->memmap[VIRT_SMMU].base;
> > +    sdev.irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> > +    g_array_append_val(smmuv3_devices, sdev);
> > +}
> > +
> > +static int get_smmuv3_devices(Object *obj, void *opaque)
> > +{
> > +    PCIBus *bus;
> > +    SMMUv3Device sdev;
> > +    SysBusDevice *sbdev;
> > +    int min_bus, max_bus;
> > +    AcpiIortIdMapping idmap;
> > +    PlatformBusDevice *pbus;
> > +    GArray *sdev_blob = opaque;
> > +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> > +
> > +    if (!object_dynamic_cast(obj, TYPE_ARM_SMMUV3)) {
> > +        return 0;
> > +    }
> > +
> > +    bus = PCI_BUS(object_property_get_link(obj, "primary-bus",
> &error_abort));
> > +    if (!bus || pci_bus_bypass_iommu(bus)) {
> can't we simply prevent a user creatable SMMU to be attached to
> 
> pci_bus_bypass_iommu(bus) ?

Ok. I think that can be done.

> 
> > +        return 0;
> > +    }
> > +
> > +    pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> > +    sbdev = SYS_BUS_DEVICE(obj);
> > +    sdev.base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> > +    sdev.base += vms->memmap[VIRT_PLATFORM_BUS].base;
> > +    sdev.irq = platform_bus_get_irqn(pbus, sbdev, 0);
> > +    sdev.irq += vms->irqmap[VIRT_PLATFORM_BUS];
> > +    sdev.irq += ARM_SPI_BASE;
> > +
> > +    pci_bus_range(bus, &min_bus, &max_bus);
> > +    sdev.smmu_idmaps = g_array_new(false, true,
> sizeof(AcpiIortIdMapping));
> > +    idmap.input_base = min_bus << 8,
> > +    idmap.id_count = (max_bus - min_bus + 1) << 8,
> > +    g_array_append_val(sdev.smmu_idmaps, idmap);
> > +    g_array_append_val(sdev_blob, sdev);
> > +    return 0;
> > +}
> > +
> >  /*
> >   * Input Output Remapping Table (IORT)
> >   * Conforms to "IO Remapping Table System Software on ARM
> Platforms",
> > @@ -274,11 +344,11 @@ static int iort_idmap_compare(gconstpointer a,
> gconstpointer b)
> >  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;
> > -    AcpiIortIdMapping *idmap;
> > +    int i, j, nb_nodes, rc_mapping_count;
> > +    size_t node_size;
> > +    int num_smmus = 0;
> >      uint32_t id = 0;
> > -    GArray *smmu_idmaps = g_array_new(false, true,
> sizeof(AcpiIortIdMapping));
> > +    GArray *smmuv3_devices = g_array_new(false, true,
> sizeof(SMMUv3Device));
> >      GArray *its_idmaps = g_array_new(false, true,
> sizeof(AcpiIortIdMapping));
> >
> >      AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
> > @@ -286,28 +356,46 @@ build_iort(GArray *table_data, BIOSLinker
> *linker, VirtMachineState *vms)
> >      /* 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->legacy_smmuv3_present) {
> > +        get_smmuv3_legacy_dev(vms, smmuv3_devices);
> > +        /*
> > +         * 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.
> > +         */
> > +        SMMUv3Device *s = &g_array_index(smmuv3_devices,
> SMMUv3Device, 0);
> > +        g_array_sort(s->smmu_idmaps, iort_idmap_compare);
> > +    } else {
> >          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);
> > +                                       get_smmuv3_devices, smmuv3_devices);
> > +        /* Sort the smmuv3 devices(if any) by smmu idmap input_base */
> > +        g_array_sort(smmuv3_devices, smmuv3_dev_idmap_compare);
> > +    }
> >
> > +    num_smmus = smmuv3_devices->len;
> > +    if (num_smmus) {
> > +        AcpiIortIdMapping next_range = {0};
> > +        int smmu_map_cnt = 0;
> >          /*
> >           * Split the whole RIDs by mapping from RC to SMMU,
> >           * build the ID mapping from RC to ITS directly.
> >           */
> > -        for (i = 0; i < smmu_idmaps->len; i++) {
> > -            idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> > -
> > -            if (next_range.input_base < idmap->input_base) {
> > -                next_range.id_count = idmap->input_base -
> next_range.input_base;
> > -                g_array_append_val(its_idmaps, next_range);
> > +        for (i = 0; i < num_smmus; i++) {
> > +            AcpiIortIdMapping *idmap;
> extra line needed

Sure. I wonder why the checkpatch --strict doesn't pick these things.

Thanks,
Shameer