On 4/16/25 1:38 AM, Shameerali Kolothum Thodi wrote:
-----Original Message-----
From: Nicolin Chen <nicol...@nvidia.com>
Sent: Wednesday, April 16, 2025 5:19 AM
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 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
