Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids

[Jason Gunthorpe]

On Fri, Aug 04, 2023 at 12:32:08AM +0800, Michael Shavit wrote:
> On Thu, Aug 3, 2023 at 11:42 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> >
> > On Thu, Aug 03, 2023 at 06:12:22PM +0800, Michael Shavit wrote:
> > > The arm-smmu-v3 driver keeps track of all masters that a domain is
> > > attached to so that it can re-write their STEs when the domain's ASID is
> > > upated by SVA.
> >
> > Wah?
> >
> > A domain's ASID shouldn't change, why does it change for SVA? Doesn't
> > SVA use CDTE's only? Why would it ever change a STE? I'm confused what
> > you are trying to explain here.
> 
> Urh right, I mixed up CD entry and STE here. Before this patch, SVA
> keeps tracks of all the masters attached to a CD domain, and updates
> the CD entry 0 in their CD table. 

Because it assumes that if a domain is returned from the ASID lookup
it is a RID domain.

> Now that a CD domain can be attached on non-zero SSIDs, SVA can't
> simply update slot 0 in the table; it must know which slot(s) this
> domain is attached to.

Yes, so why are you passing in 0 as the ssid argument to
arm_smmu_write_ctx_desc_devices() for the ASID change?

I think your commit message is trying to say:

The SMMUv3 driver keeps track of all the iommu_domains that are
assigned to an ASID in an xarray. The SVA code needs to re-use the
same ASID as the CPU's ASID (presumably only for BTM mode?) so it has
a mechanism to reclaim an already used ASID from an existing domain.

This is currently hardwired with the assumption that a domain using an
ASID is only on SSID 0.

Add a list to the struct arm_smmu_domain recording each master and
SSID that the domain is attached to and update it when any domain is
attached/detached.

Make arm_smmu_write_ctx_desc_devices() follow this list when storing
the CD table entries for the domain.

Remove 'ssid' as an argument to arm_smmu_write_ctx_desc_devices()
since it is always found in the attached_ssids.

> > What is a "primary domain"? Why can't we fix SVA first so it doesn't
> > have this weird "piggybacks" or:
> >
> ...
> >
> > This patch is not making sense to me, the goal in the commit message
> > seems logical, but why is tracking CD entries introducing this concept
> > of a primary domain and doing special stuff for SSID=0?
> 
> I'd argue this patch isn't introducing anything the driver isn't
> already doing.

So this I don't understand:

+               if (ssid && attached_domain->ssid == 0) {
+                       ret = arm_smmu_write_ctx_desc(master, ssid, cd);
+               } else {
+                       ret = arm_smmu_write_ctx_desc(
+                               master, attached_domain->ssid, cd);
+               }

Fix this patch so attached_domain->ssid is never wrong?

Remove ssid as an input to the function.

(I'd ultimately expect to remove CD too)

> it. I do have a patch series in the works to properly fix SVA, but
> it's growing quite large and I was trying to get this feature
> out first. At a high level, the subsequent series:
> 1. Nests the list of attached masters in a list of SMMUs the domain is
> installed in. Updates SMMU-level operations (such as invalidations) to
> iterate over said list.
> 2. Checks the compatibility of a domain when attaching to a new SMMU
> instead of outright rejecting, to allow attaching a domain to multiple
> SMMUs.
> 3. Thus allowing SVA to alloc a single domain for the MM struct (which
> the series maps from multiple SVA domains internally, pending support
> at the iommu core layer)

This should not be hard for the core code

> 4. And allowing SVA to use the same set_dev_pasid implementation used
> here on that domain.

This list all makes alot of sense to me

> Now having said that, it might be possible to get rid of piggybacking
> sooner if we create an MM per master instead of per "primary-domain",
> but I'm not too sure about performance implications. AFAICT, the only
> downside might be for invalidate_range commands that could no longer
> be sent as a batched command to the SMMU (since each mmu notifier
> would be called independently).

I'm not sure this series leaves things in a better state than before,
now we have two parallel domain attachment paths for PASID :(

Jason