Re: [PATCH] iommu/arm-smmu-v3: Allow stream table to have nodes with the same ID

[Nicolin Chen]

On Fri, Apr 11, 2025 at 05:07:03PM -0700, Nicolin Chen wrote:
> On Fri, Apr 11, 2025 at 08:44:19PM -0300, Jason Gunthorpe wrote:
> > On Fri, Apr 11, 2025 at 04:33:44PM -0700, Nicolin Chen wrote:
> > 
> > > > The bridge *does* claim its own RID, and per the aliasing rules the
> > > > devices behind it claim both their own RID and the alias to function
> > > > 00.0 on the bridge's secondary bus, like so in action:
> > > 
> > > Yea, I just found out that the bridge does have a different SID.
> > > It was actually the VGA controller itself having two fwspec->ids
> > > populated by the IORT code. Then, the SMMU driver allocated two
> > > separate streams with the same set of device pointer and SID:
> > >   pci 0008:06:00.0: arm_smmu_insert_master: fwspec index=0, sid=0x10600
> > >   pci 0008:06:00.0: Adding to iommu group 21
> > >   pci 0008:07:00.0: arm_smmu_insert_master: fwspec index=0, sid=0x10700
> > >   pci 0008:07:00.0: arm_smmu_insert_master: fwspec index=1, sid=0x10700
> > >   pci 0008:07:00.0: Adding to iommu group 21
> > > 
> > > Perhaps the duplicated fwspec->id should be avoided in the IORT
> > > code at the first place v.s. bypassing the fwspec->ids[1]?
> > 
> > It is a much easier fix if all you have to do is ignore hits in the RB
> > tree that match to the same master, just don't fail on duplicates and
> > don't add the duplicate rb node at all?
> >
> > Seem strange though. Where did the duplicate come from?
> 
> iort_iommu_configure_id
>  |=> pci_for_each_dma_alias
>       |=> iort_pci_iommu_init
>       |    |=> iort_iommu_xlate
>       |         |=> acpi_iommu_fwspec_init
>       |              |=> iommu_fwspec_add_id(sid=0x10700) //index=0
>       |=> iort_pci_iommu_init
>            |=> iort_iommu_xlate
>                 |=> acpi_iommu_fwspec_init
>                      |=> iommu_fwspec_add_id(sid=0x10700) //index=1
> 
> We could add a piece of code in iommu_fwspec_add_ids() rejecting
> a duplicated ID, which would be smaller than we do in SMMU driver
> that requires a device_group change too?

I figured that we could do something smaller too in SMMUv3 driver,
somewhat similar to what Robin did for STE in commit 563b5cbe334e
("iommu/arm-smmu-v3: Cope with duplicated Stream IDs"):
----------------------------------------------
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index b4c21aaed1266..4b66e8ebef539 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3393,6 +3393,10 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
                new_stream->id = sid;
                new_stream->master = master;

+               /* Bridged PCI devices may end up with duplicated IDs */
+               if (master == arm_smmu_find_master(smmu, sid))
+                       continue;
+
                ret = arm_smmu_init_sid_strtab(smmu, sid);
                if (ret)
                        break;
----------------------------------------------

This would bypass the duplicated ID. And here is the test result:
[    6.327800] pci 0008:06:00.0: Adding to iommu group 21
[    6.327847] pci 0008:07:00.0: Adding to iommu group 21

Thanks
Nicolin