qemu-arm
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Donald Dutile
Subject: Re: [PATCH 2/5] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices
Date: Fri, 18 Apr 2025 16:34:26 -0400
User-agent: Mozilla Thunderbird 


On 4/16/25 1:38 AM, Shameerali Kolothum Thodi wrote:




-----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.


Didn't quite follow this logic; shouldn't the iort be built for devices
tagged for a virt-iommu or dependent on a pcie rp (bus num/bus-num range of an 
RP)?
Thus, it's just an IORT with one or more IORT nodes?
I could see how the current iort_host_bridge() may need a revision to work with 
-device arm-smmu configs.

Which would bring me back to my earlier question, and it's likely tied to this 
IORT construction:
 -- can you have both types defined in a machine model?




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



I didn't follow 'SMMUv3AcpiInfo' since I don't see that in current qemu tree;
or is the statement trying to say its a 'mere matter of the proper acpi info 
generation'?


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.


Aren't they going to be different idmaps for different IORT nodes?


  > > @@ -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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]