Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Michael Shavit]

On Mon, Aug 21, 2023 at 10:26 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
>
> On Mon, Aug 21, 2023 at 10:16:54PM +0800, Michael Shavit wrote:
> > On Mon, Aug 21, 2023 at 9:50 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Aug 21, 2023 at 09:38:40PM +0800, Michael Shavit wrote:
> > > > On Mon, Aug 21, 2023 at 7:54 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, Aug 21, 2023 at 05:31:23PM +0800, Michael Shavit wrote:
> > > > > > On Fri, Aug 18, 2023 at 2:38 AM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Fri, Aug 18, 2023 at 02:16:25AM +0800, Michael Shavit wrote:
> > > > > > > > Pick an ASID that is within the supported range of all SMMUs that the
> > > > > > > > domain is installed to.
> > > > > > > >
> > > > > > > > Signed-off-by: Michael Shavit <mshavit@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > >
> > > > > > > This seems like a pretty niche scenario, maybe we should just keep a
> > > > > > > global for the max ASID?
> > > > > > >
> > > > > > > Otherwise we need a code to change the ASID, even for non-SVA domains,
> > > > > > > when the domain is installed in different devices if the current ASID
> > > > > > > is over the instance max..
> > > > > >
> > > > > > This RFC took the other easy way out for this problem by rejecting
> > > > > > attaching a domain if its currently assigned ASID/VMID
> > > > > > is out of range when attaching to a new SMMU. But I'm not sure
> > > > > > which of the two options is the right trade-off.
> > > > > > Especially if we move VMID to a global allocator (which I plan to add
> > > > > > for v2), setting a global maximum for VMID of 256 sounds small.
> > > > >
> > > > > IMHO the simplest and best thing is to make both vmid and asid as
> > > > > local allocators. Then alot of these problems disappear
> > > >
> > > > Well that does sound like the most flexible, but IMO quite a lot more
> > > > complicated.
> > > >
> > > > I'll post a v2 RFC that removes the `iommu/arm-smmu-v3: Add list of
> > > > installed_smmus` patch and uses a flat master list in smmu_domain as
> > > > suggested by Robin, for comparison with the v1. But at a glance using a
> > > > local allocator would require:
> > >
> > > > 1. Keeping that patch so we can track the asid/vmid for a domain on a
> > > > per smmu instance
> > >
> > > You'd have to store the cache tag in the per-master struct on that
> > > list and take it out of the domain struct.
> > >
> > > Ie the list of attached masters contains the per-master cache tag
> > > instead of a global cache tag.
> > >
> > > The only place you need the cache tag is when iterating over the list
> > > of masters, so it is OK.
> > >
> > > If the list of masters is sorted by smmu then the first master of each
> > > smmu can be used to perform the cache tag invalidation, then the rest
> > > of the list is the ATC invalidation.
> > >
> > > The looping code will be a bit ugly.
> >
> > I suppose that could work.... but I'm worried it's gonna be messy,
> > especially if we think about how the PASID feature would interact.
> > With PASID, there could be multiple domains attached to a master. So
> > we won't be able to store a single cache tag/asid for the currently
> > attached domain on the arm_smmu_master.
>
> I wasn't suggesting to store it in the arm_smmu_master, I was
> suggesting to store it in the same place you store the per-master
> PASID.
>
> eg I expect that on attach the domain will allocate new memory to
> store the pasid/cache tag/master/domain and thread that memory on a
> list of attached masters.

Gotcha.

> > > > (on a loop over every smmu the domain in arm_smmu_mmu_notifier_get is
> > > > attached to, which just at a glance looks headache inducing because of
> > > > sva's piggybacking on the rid domain.)
> > >
> > > Not every smmu, just the one you are *currently* attaching to. We
> > > don't care if the *other* smmu's have different ASIDs, maybe they are
> > > not using BTM, or won't use SVA.
> >
> > I mean because the domain in arm_smmu_mmu_notifier_get is the RID
> > domain (not the SVA domain, same issue we discussed in previous
> > thread) , which can be attached to multiple SMMUs.
>
> Oh that is totally nonsensical. I expect you will need to fix that
> sooner than later. Once the CD table is moved and there is a proper
> way to track the PASID it should not be needed. It shouldn't fall into
> the decision making about where to put the ASID xarray.

Right I got a bit of a chicken and egg problem with all these series.

Can we keep the simpler solutions where ASID/VMID across SMMUs has
non-optimal constraints and re-consider this after all the other
changes land (this series, set_dev_pasid series, fixing sva)?