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

[Jason Gunthorpe <jgg@nvidia.com>]

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.

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?

Thanks,
Jason