> -----Original Message-----
> From: qemu-devel-
> bounces+shameerali.kolothum.thodi=huawei....@nongnu.org <qemu-
> devel-bounces+shameerali.kolothum.thodi=huawei....@nongnu.org> On
> Behalf Of Eric Auger
> Sent: Monday, May 5, 2025 9:40 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.th...@huawei.com>; qemu-...@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.mayd...@linaro.org; j...@nvidia.com; nicol...@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
>
> 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.th...@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
