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

[Will Deacon <will@kernel.org>]

On Tue, Jun 25, 2024 at 09:37:33AM -0300, Jason Gunthorpe wrote:
> Add arm_smmu_set_pasid()/arm_smmu_remove_pasid() which are to be used by
> callers that already constructed the arm_smmu_cd they wish to program.
> 
> These functions will encapsulate the shared logic to setup a CD entry that
> will be shared by SVA and S1 domain cases.
> 
> Prior fixes had already moved most of this logic up into
> __arm_smmu_sva_bind(), move it to it's final home.
> 
> Following patches will relieve some of the remaining SVA restrictions:
> 
>  - The RID domain is a S1 domain and has already setup the STE to point to
>    the CD table
>  - The programmed PASID is the mm_get_enqcmd_pasid()
>  - Nothing changes while SVA is running (sva_enable)
> 
> SVA invalidation will still iterate over the S1 domain's master list,
> later patches will resolve that.
> 
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 57 ++++++++++---------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 32 ++++++++++-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  9 ++-
>  3 files changed, 67 insertions(+), 31 deletions(-)

[...]

> @@ -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().

As an additional patch, perhaps we should consider documenting what each
lock in the driver protects and the lock ordering requirements they
have? 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.

Thanks,

Will