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

Jason Gunthorpe
2023-08-08T17:34:22Z

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