Re: [PATCH 4/5] iommu: Support mm PASID 1:n with sva domains

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Aug 08, 2023 at 03:49:43PM +0800, Tina Zhang wrote:
> Each mm bound to devices gets a PASID and corresponding sva domains
> allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> field of the mm. The PASID is released in __mmdrop(), while a sva domain
> is released when on one is using it (the reference count is decremented
> in iommu_sva_unbind_device()).
> 
> Since the required info of PASID and sva domains is kept in struct
> iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
> field in mm struct. The sva domain list is protected by iommu_sva_lock.
> 
> Besides, this patch removes mm_pasid_init(), as with the introduced
> iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  drivers/iommu/iommu-sva.c | 38 +++++++++++++++++++++++++-------------
>  include/linux/iommu.h     | 10 +++-------
>  kernel/fork.c             |  1 -
>  3 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 0a4a1ed40814..35366f51ad27 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -15,6 +15,7 @@ static DEFINE_IDA(iommu_global_pasid_ida);
>  /* Allocate a PASID for the mm within range (inclusive) */
>  static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
>  {
> +	struct iommu_mm_data *iommu_mm = NULL;
>  	int ret = 0;
>  
>  	if (min == IOMMU_PASID_INVALID ||
> @@ -33,11 +34,22 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
>  		goto out;
>  	}
>  
> +	iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL);
> +	if (!iommu_mm) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
>  	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		kfree(iommu_mm);
>  		goto out;
> +	}
> +
> +	iommu_mm->pasid = ret;
> +	mm->iommu_mm = iommu_mm;
> +	INIT_LIST_HEAD(&mm->iommu_mm->sva_domains);
>  
> -	mm->pasid = ret;
>  	ret = 0;
>  out:
>  	mutex_unlock(&iommu_sva_lock);
> @@ -82,16 +94,12 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm

Lets please rework this function into two parts

The first should be 'iommu_sva_alloc_domain()'

It should do the list searching and user management. The usual
'iommu_domain_free()' should be modified to clean it up.

Then you have the 'alloc global/enqcmd pasid' function

Then bind is just callling alloc sva domain, alloc global pasid,
set_dev_pasid in a sequence.

It will make this much more understandable what all the parts are
supposed to be doing.

Jason