Message-ID: <CADe9J7Er0wnP5ZZbWM1CkUx7ry0znKr05Lvx7PEMdHuwKr_RZQ@mail.gmail.com>
Date:   Wed, 13 Jan 2021 01:59:37 +0530
From:   Ajay Kumar <ajaykumar.rs1989@...il.com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Will Deacon <will@...nel.org>,
        Ajay Kumar <ajaykumar.rs@...sung.com>, mark.rutland@....com,
        Linux IOMMU <iommu@...ts.linux-foundation.org>,
        robh+dt@...nel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] iommu/arm-smmu-v3: Handle duplicated Stream IDs from
 other masters

On Tue, Jan 12, 2021 at 12:57 AM Robin Murphy <robin.murphy@....com> wrote:
>
> On 2021-01-07 13:03, Will Deacon wrote:
> > On Thu, Jan 07, 2021 at 03:03:40PM +0530, Ajay Kumar wrote:
> >> When PCI function drivers(ex:pci-endpoint-test) are probed for already
> >> initialized PCIe-RC(Root Complex), and PCIe-RC is already bound to SMMU,
> >> then we encounter a situation where the function driver tries to attach
> >> itself to the smmu with the same stream-id as PCIe-RC and re-initialize
> >> an already initialized STE. This causes ste_live BUG_ON() in the driver.
>
> Note that this is actually expected behaviour, since Stream ID aliasing
> has remained officially not supported until a sufficiently compelling
> reason to do so appears. I always thought the most likely scenario would
> be a legacy PCI bridge with multiple devices behind it, but even that
> seems increasingly improbable for a modern SMMUv3-based system to ever see.
Thanks to Will and Robin for reviewing this. I am pretty new to PCI,
sorry about that.
I assumed that the support for stream-id alias is already handled as
part of this patch:
https://www.spinics.net/lists/arm-kernel/msg626087.html
which prevents STE re-initialization. But, what I do not understand is
why the path
taken by the arm-smmu-v3 driver misses the aforementioned check for my usecase.

> > I don't understand why the endpoint is using the same stream ID as the root
> > complex in this case. Why is that? Is the grouping logic not working
> > properly?
>
> It's not so much that it isn't working properly, it's more that it needs
> to be implemented at all ;)
The pci_endpoint_test picks up the same of_ DMA config node as the PCI RC
because they sit on the same PCI bus [via pci_dma_configure( )]
While in the arm-smmu-v3 driver, I can see that the pci_device_group( ) hands
over the same iommu group as the Root Complex to the newly added master
device (pci_endpoint_test in our case) because they share the same stream ID.
Shouldn't they?

> >> There is an already existing check in the driver to manage duplicated ids
> >> if duplicated ids are added in same master device, but there can be
> >> scenarios like above where we need to extend the check for other masters
> >> using the same stream-id.
> >>
> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@...sung.com>
> >> ---
> >>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 33 +++++++++++++++++++++
> >>   1 file changed, 33 insertions(+)
> >
> > It doesn't feel like the driver is the right place to fix this, as the same
> > issue could surely occur for other IOMMUs too, right? In which case, I think
> > we should avoid getting into the situation where different groups have
> > overlapping stream IDs.
>
> Yes, this patch does not represent the correct thing to do either way.
> The main reason that Stream ID aliasing hasn't been supported so far is
> that the required Stream ID to group lookup is rather awkward, and
> adding all of that complexity just for the sake of a rather unlikely
> possibility seemed dubious. However, PRI support has always had a more
> pressing need to implement almost the same thing (Stream ID to device),
> so once that lands we can finally get round to adding the rest of proper
> group support relatively easily.
I hope the support will be added soon. Also, can you point me to few drivers
which already handle this type of stream-ID aliasing?

Thanks,
Ajay Kumar
> ______________________________________________
> iommu mailing list
> iommu@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Powered by blists - more mailing lists