> -----Original Message----- > From: Eric Auger <[email protected]> > Sent: 09 March 2026 18:23 > To: Shameer Kolothum Thodi <[email protected]>; qemu- > [email protected]; [email protected] > Cc: [email protected]; [email protected]; [email protected]; Nicolin > Chen <[email protected]>; Nathan Chen <[email protected]>; Matt > Ochs <[email protected]>; Jiandi An <[email protected]>; Jason Gunthorpe > <[email protected]>; [email protected]; > [email protected]; [email protected]; Krishnakant Jaju > <[email protected]>; [email protected] > Subject: Re: [PATCH v3 28/32] hw/arm/smmuv3: Add per-device identifier > property > > External email: Use caution opening links or attachments > > > On 2/26/26 11:50 AM, Shameer Kolothum wrote: > > Add an "identifier" property to the SMMUv3 device and use it when > > building the ACPI IORT SMMUv3 node Identifier field. > > > > This avoids relying on device enumeration order and provides a stable > > per-device identifier. A subsequent patch will use the same identifier > > when generating the DSDT description for Tegra241 CMDQV, ensuring that > > the IORT and DSDT entries refer to the same SMMUv3 instance. > > > > No functional change intended. > > > > Reviewed-by: Nicolin Chen <[email protected]> > > Signed-off-by: Shameer Kolothum <[email protected]> > > --- > > include/hw/arm/smmuv3.h | 1 + > > hw/arm/smmuv3.c | 2 ++ > > hw/arm/virt-acpi-build.c | 4 +++- > > hw/arm/virt.c | 3 +++ > > 4 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h > > index 648412cafc..73b8f39aaa 100644 > > --- a/include/hw/arm/smmuv3.h > > +++ b/include/hw/arm/smmuv3.h > > @@ -63,6 +63,7 @@ struct SMMUv3State { > > qemu_irq irq[4]; > > QemuMutex mutex; > > char *stage; > > + uint8_t identifier; > > > > /* SMMU has HW accelerator support for nested S1 + s2 */ > > bool accel; > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > > index 468135bb24..c1f84bedd4 100644 > > --- a/hw/arm/smmuv3.c > > +++ b/hw/arm/smmuv3.c > > @@ -2109,6 +2109,8 @@ static const Property smmuv3_properties[] = { > > * Defaults to stage 1 > > */ > > DEFINE_PROP_STRING("stage", SMMUv3State, stage), > > + /* Identifier used for ACPI IORT SMMUv3 (and DSDT for CMDQV) > generation */ > > + DEFINE_PROP_UINT8("identifier", SMMUv3State, identifier, 0), > > DEFINE_PROP_BOOL("accel", SMMUv3State, accel, false), > > /* GPA of MSI doorbell, for SMMUv3 accel use. */ > > DEFINE_PROP_UINT64("msi-gpa", SMMUv3State, msi_gpa, 0), > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > index ae78e9b9e0..20605185c5 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -342,6 +342,7 @@ static int iort_idmap_compare(gconstpointer a, > gconstpointer b) > > typedef struct AcpiIortSMMUv3Dev { > > int irq; > > hwaddr base; > > + uint8_t id; > > GArray *rc_smmu_idmaps; > > /* Offset of the SMMUv3 IORT Node relative to the start of the IORT */ > > size_t offset; > > @@ -404,6 +405,7 @@ static int populate_smmuv3_dev(GArray > *sdev_blob, VirtMachineState *vms) > > &error_abort)); > > sdev.accel = object_property_get_bool(obj, "accel", &error_abort); > > sdev.ats = object_property_get_bool(obj, "ats", &error_abort); > > + sdev.id = object_property_get_uint(obj, "identifier", > > &error_abort); > > pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev); > > sbdev = SYS_BUS_DEVICE(obj); > > sdev.base = platform_bus_get_mmio_addr(pbus, sbdev, 0); > > @@ -630,7 +632,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > > (ID_MAPPING_ENTRY_SIZE * smmu_mapping_count); > > build_append_int_noprefix(table_data, node_size, 2); /* Length */ > > build_append_int_noprefix(table_data, 4, 1); /* Revision */ > > - build_append_int_noprefix(table_data, id++, 4); /* Identifier */ > > + build_append_int_noprefix(table_data, sdev->id, 4); /* Identifier > > */ > Hum actually I see that previously the id scope was shared by all nodes, > including ITS, SMMU, RC. > So now can have its and smmu node having both id=0. And this is why you > have a change in the IORT table for its=off test.
Hmm... I had that doubt as well. But I read it as a unique identifier for each type of IORT node. > Is it legal? > > IORT spec says: > > Unique identifier for this node that can be used to locate it in the > parent table. This identifier enables other ACPI tables and DSDT objects > to locate this node. This field serves in the same capacity as the _UID > object associated with ACPI device objects. Note IMPLEMENTATION NOTE: In > the simplest scheme, the Identifier might be set to the node’s index in > the array of IORT nodes in the parent IORT table. Other schemes are also > possible and permitted. > > So I don't think this is valid I think we need another way to sort out the identifier then. Thanks, Shameer
