Re: [PATCH v9 02/14] iommu/arm-smmu-v3: Start building a generic PASID layer

[Will Deacon <will@kernel.org>]

Hi Jason,

Sorry, it's taken me ages to get back to this after applying the series.

On Tue, Jul 09, 2024 at 04:39:05PM -0300, Jason Gunthorpe wrote:
> On Tue, Jul 02, 2024 at 03:57:05PM +0100, Will Deacon wrote:
> 
> > > @@ -611,10 +599,9 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
> > >  	struct arm_smmu_bond *bond = NULL, *t;
> > >  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> > >  
> > > +	arm_smmu_remove_pasid(master, to_smmu_domain(domain), id);
> > > +
> > >  	mutex_lock(&sva_lock);
> > > -
> > > -	arm_smmu_clear_cd(master, id);
> > 
> > This looks a bit alarming, as you're effectively moving the CD
> > modification outside of the critical section. I assume we're relying on
> > the iommu group mutex to serialise this in the caller? I can't see any
> > consistent locking in the driver for arm_smmu_clear_cd().
> 
> I see Nicolin got this - but yes, sva_lock has nothing to do with
> CD. CD/STE has always been protected by the group_mutex.
> 
> > As an additional patch, perhaps we should consider documenting what each
> > lock in the driver protects and the lock ordering requirements they
> > have?
> 
> There is still a bunch of rework to do here, it may be better to
> complete the rework than to try to document it, but let me know which
> ones you are interested in and I'll write some thing.

I think listing the locks we have in the driver and describing both
what they protect and the ordering between them would be really helpful.

> sva_lock is almost gone, it just locks the IOPF flow on the master
> because we are doing the IOPF flow in the wrong place. The IOPF
> enable/disbale should be done in attach under the group mutex.
> 
> init_mutex will be deleted once the iommu_domain_alloc_paging()
> conversion is done.
> 
> asid_lock.. Is a place holder for the nascent BTM support. It locks
> domain->asid only. The BTM patches make this per-instance instead of
> global. Unfortunately that locking scheme doesn't work 100%, but I
> have a notion how to fix it..
> 
> asid_lock is also going to need some reconsidering when we make the
> domain able to attach to multiple instances which is something iommufd
> wants.
> 
> devices_lock protects the device list only, excluding the special
> nr_ats_masters thing.
> 
> Then the hidden group mutex makes all the ops touching master single
> threaded, and the driver has always quietly relied on this. It
> protects the STE/CD and parts of the master.
> 
> So I think we end up with only the group mutex, asid_lock and
> devices_lock it is OK.
> 
> The order is group -> asid -> devices, which is layed out clearly in
> the attach functions.
> 
> > We've got a few global locks with generic names and after a few
> > rounds of refactoring it's really hard to know who's responsible for
> > what, especially now that we have stale comments referring to
> > arm_smmu_share_asid().
> 
> > We've also grown a number of places where we
> > drop a lock in the callee and immediately re-take it in the caller,
> > which tends to be a source of bugs.
> 
> Do we? Can you point to what you noticed?

As I recall, I just noticed that:

- We have a bunch of comments around 'arm_smmu_asid_lock' that refer
  to arm_smmu_share_asid(), which no longer exists.

- arm_smmu_remove_dev_pasid() drops the asid_lock only to have it retaken
  in the callee via ->attach_dev().

- arm_smmu_attach_dev() takes/drops/re-takes the devices_lock indirectly
  when it calls arm_smmu_attach_prepare() and arm_smmu_attach_commit().

- arm_smmu_attach_dev() takes/drops 'arm_smmu_asid_lock' via
  arm_smmu_domain_finalise()) and then re-takes it before the attach.

Please note, I'm not saying that there's a bug here, just that it would
be easier to work with if we had some documentation and lock ordering
assertions.

Will