[PATCH v2 2/3] iommu/sva: Stop using ioasid_set for SVA

Jacob Pan posted 3 patches 2 years, 6 months ago
There is a newer version of this series
[PATCH v2 2/3] iommu/sva: Stop using ioasid_set for SVA
Posted by Jacob Pan 2 years, 6 months ago
From: Jason Gunthorpe <jgg@nvidia.com>

Instead SVA drivers can use a simple global IDA to allocate PASIDs for
each mm_struct.

Future work would be to allow drivers using the SVA APIs to reserve global
PASIDs from this IDA for their internal use, eg with the DMA API PASID
support.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
v2: minor fixes
	- let idxd driver use iommu.h for ioasid_t
	- return 0 after mm_pasid_set()
---
 drivers/dma/idxd/idxd.h   |  1 +
 drivers/iommu/iommu-sva.c | 53 +++++++++++----------------------------
 drivers/iommu/iommu-sva.h |  3 ---
 include/linux/sched/mm.h  |  8 +-----
 4 files changed, 17 insertions(+), 48 deletions(-)

diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 7ced8d283d98..417e602a46b6 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -13,6 +13,7 @@
 #include <linux/ioasid.h>
 #include <linux/bitmap.h>
 #include <linux/perf_event.h>
+#include <linux/iommu.h>
 #include <uapi/linux/idxd.h>
 #include "registers.h"
 
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 24bf9b2b58aa..8fb6f1867af3 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -9,24 +9,11 @@
 #include "iommu-sva.h"
 
 static DEFINE_MUTEX(iommu_sva_lock);
-static DECLARE_IOASID_SET(iommu_sva_pasid);
+static DEFINE_IDA(iommu_global_pasid_ida);
 
-/**
- * iommu_sva_alloc_pasid - Allocate a PASID for the mm
- * @mm: the mm
- * @min: minimum PASID value (inclusive)
- * @max: maximum PASID value (inclusive)
- *
- * Try to allocate a PASID for this mm, or take a reference to the existing one
- * provided it fits within the [@min, @max] range. On success the PASID is
- * available in mm->pasid and will be available for the lifetime of the mm.
- *
- * Returns 0 on success and < 0 on error.
- */
-int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
+static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
 {
-	int ret = 0;
-	ioasid_t pasid;
+	int ret;
 
 	if (min == INVALID_IOASID || max == INVALID_IOASID ||
 	    min == 0 || max < min)
@@ -37,39 +24,29 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
 	if (pasid_valid(mm->pasid)) {
 		if (mm->pasid < min || mm->pasid >= max)
 			ret = -EOVERFLOW;
+		else
+			ret = 0;
 		goto out;
 	}
 
-	pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
-	if (!pasid_valid(pasid))
-		ret = -ENOMEM;
-	else
-		mm_pasid_set(mm, pasid);
+	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
+	if (ret < min)
+		goto out;
+	mm_pasid_set(mm, ret);
+	ret = 0;
 out:
 	mutex_unlock(&iommu_sva_lock);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
 
-/* ioasid_find getter() requires a void * argument */
-static bool __mmget_not_zero(void *mm)
+void mm_pasid_drop(struct mm_struct *mm)
 {
-	return mmget_not_zero(mm);
-}
+	pr_alert("%s %d", __func__, mm->pasid);
+	if (likely(!pasid_valid(mm->pasid)))
+		return;
 
-/**
- * iommu_sva_find() - Find mm associated to the given PASID
- * @pasid: Process Address Space ID assigned to the mm
- *
- * On success a reference to the mm is taken, and must be released with mmput().
- *
- * Returns the mm corresponding to this PASID, or an error if not found.
- */
-struct mm_struct *iommu_sva_find(ioasid_t pasid)
-{
-	return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
+	ida_free(&iommu_global_pasid_ida, mm->pasid);
 }
-EXPORT_SYMBOL_GPL(iommu_sva_find);
 
 /**
  * iommu_sva_bind_device() - Bind a process address space to a device
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
index 7215a761b962..c22d0174ad61 100644
--- a/drivers/iommu/iommu-sva.h
+++ b/drivers/iommu/iommu-sva.h
@@ -8,9 +8,6 @@
 #include <linux/ioasid.h>
 #include <linux/mm_types.h>
 
-int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
-struct mm_struct *iommu_sva_find(ioasid_t pasid);
-
 /* I/O Page fault */
 struct device;
 struct iommu_fault;
diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 2a243616f222..cdc138086230 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -463,13 +463,7 @@ static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid)
 	mm->pasid = pasid;
 }
 
-static inline void mm_pasid_drop(struct mm_struct *mm)
-{
-	if (pasid_valid(mm->pasid)) {
-		ioasid_free(mm->pasid);
-		mm->pasid = INVALID_IOASID;
-	}
-}
+void mm_pasid_drop(struct mm_struct *mm);
 #else
 static inline void mm_pasid_init(struct mm_struct *mm) {}
 static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid) {}
-- 
2.25.1
RE: [PATCH v2 2/3] iommu/sva: Stop using ioasid_set for SVA
Posted by Tian, Kevin 2 years, 6 months ago
> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Tuesday, February 14, 2023 2:44 PM
> 
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Instead SVA drivers can use a simple global IDA to allocate PASIDs for
> each mm_struct.
> 
> Future work would be to allow drivers using the SVA APIs to reserve global
> PASIDs from this IDA for their internal use, eg with the DMA API PASID
> support.

I think DMA API PASID will need new API around this IDA. SVA APIs should
be just for SVA.

> +void mm_pasid_drop(struct mm_struct *mm)
>  {
> -	return mmget_not_zero(mm);
> -}
> +	pr_alert("%s %d", __func__, mm->pasid);
> +	if (likely(!pasid_valid(mm->pasid)))
> +		return;
> 
> -/**
> - * iommu_sva_find() - Find mm associated to the given PASID
> - * @pasid: Process Address Space ID assigned to the mm
> - *
> - * On success a reference to the mm is taken, and must be released with
> mmput().
> - *
> - * Returns the mm corresponding to this PASID, or an error if not found.
> - */
> -struct mm_struct *iommu_sva_find(ioasid_t pasid)
> -{
> -	return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
> +	ida_free(&iommu_global_pasid_ida, mm->pasid);
>  }

when moving this function following line is missed:

	mm->pasid = INVALID_IOASID;

btw the current placement looks asymmetric. We now have
mm_pasid_init() and mm_pasid_set() in mm.h while mm_pasid_drop()
in iommu-sva.c.

Is it cleaner to introduce a iommu_sva_free_pasid() helper and then
call it from mm_pasid_drop() in mm.h?
Re: [PATCH v2 2/3] iommu/sva: Stop using ioasid_set for SVA
Posted by Jacob Pan 2 years, 6 months ago
Hi Kevin,

On Wed, 15 Feb 2023 03:24:18 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Tuesday, February 14, 2023 2:44 PM
> > 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > 
> > Instead SVA drivers can use a simple global IDA to allocate PASIDs for
> > each mm_struct.
> > 
> > Future work would be to allow drivers using the SVA APIs to reserve
> > global PASIDs from this IDA for their internal use, eg with the DMA API
> > PASID support.  
> 
> I think DMA API PASID will need new API around this IDA. SVA APIs should
> be just for SVA.
> 
> > +void mm_pasid_drop(struct mm_struct *mm)
> >  {
> > -	return mmget_not_zero(mm);
> > -}
> > +	pr_alert("%s %d", __func__, mm->pasid);
> > +	if (likely(!pasid_valid(mm->pasid)))
> > +		return;
> > 
> > -/**
> > - * iommu_sva_find() - Find mm associated to the given PASID
> > - * @pasid: Process Address Space ID assigned to the mm
> > - *
> > - * On success a reference to the mm is taken, and must be released with
> > mmput().
> > - *
> > - * Returns the mm corresponding to this PASID, or an error if not
> > found.
> > - */
> > -struct mm_struct *iommu_sva_find(ioasid_t pasid)
> > -{
> > -	return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
> > +	ida_free(&iommu_global_pasid_ida, mm->pasid);
> >  }  
> 
> when moving this function following line is missed:
> 
> 	mm->pasid = INVALID_IOASID;
> 
not needed, mm is gone by now.

> btw the current placement looks asymmetric. We now have
> mm_pasid_init() and mm_pasid_set() in mm.h while mm_pasid_drop()
> in iommu-sva.c.
> Is it cleaner to introduce a iommu_sva_free_pasid() helper and then
> call it from mm_pasid_drop() in mm.h?
That is what we had before, it seems logical to me. Jason?

Thanks,

Jacob
Re: [PATCH v2 2/3] iommu/sva: Stop using ioasid_set for SVA
Posted by Jason Gunthorpe 2 years, 6 months ago
On Wed, Feb 15, 2023 at 10:38:22AM -0800, Jacob Pan wrote:
> Hi Kevin,
> 
> On Wed, 15 Feb 2023 03:24:18 +0000, "Tian, Kevin" <kevin.tian@intel.com>
> wrote:
> 
> > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > Sent: Tuesday, February 14, 2023 2:44 PM
> > > 
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > 
> > > Instead SVA drivers can use a simple global IDA to allocate PASIDs for
> > > each mm_struct.
> > > 
> > > Future work would be to allow drivers using the SVA APIs to reserve
> > > global PASIDs from this IDA for their internal use, eg with the DMA API
> > > PASID support.  
> > 
> > I think DMA API PASID will need new API around this IDA. SVA APIs should
> > be just for SVA.
> > 
> > > +void mm_pasid_drop(struct mm_struct *mm)
> > >  {
> > > -	return mmget_not_zero(mm);
> > > -}
> > > +	pr_alert("%s %d", __func__, mm->pasid);
> > > +	if (likely(!pasid_valid(mm->pasid)))
> > > +		return;
> > > 
> > > -/**
> > > - * iommu_sva_find() - Find mm associated to the given PASID
> > > - * @pasid: Process Address Space ID assigned to the mm
> > > - *
> > > - * On success a reference to the mm is taken, and must be released with
> > > mmput().
> > > - *
> > > - * Returns the mm corresponding to this PASID, or an error if not
> > > found.
> > > - */
> > > -struct mm_struct *iommu_sva_find(ioasid_t pasid)
> > > -{
> > > -	return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
> > > +	ida_free(&iommu_global_pasid_ida, mm->pasid);
> > >  }  
> > 
> > when moving this function following line is missed:
> > 
> > 	mm->pasid = INVALID_IOASID;
> > 
> not needed, mm is gone by now.
> 
> > btw the current placement looks asymmetric. We now have
> > mm_pasid_init() and mm_pasid_set() in mm.h while mm_pasid_drop()
> > in iommu-sva.c.
> > Is it cleaner to introduce a iommu_sva_free_pasid() helper and then
> > call it from mm_pasid_drop() in mm.h?
> That is what we had before, it seems logical to me. Jason?

I don't much like a maze of functions calling each other.

Renaming the helper might help, especially if it comes from iommu.h

Jason
Re: [PATCH v2 2/3] iommu/sva: Stop using ioasid_set for SVA
Posted by Jean-Philippe Brucker 2 years, 6 months ago
On Mon, Feb 13, 2023 at 10:44:13PM -0800, Jacob Pan wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Instead SVA drivers can use a simple global IDA to allocate PASIDs for
> each mm_struct.
> 
> Future work would be to allow drivers using the SVA APIs to reserve global
> PASIDs from this IDA for their internal use, eg with the DMA API PASID
> support.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
> v2: minor fixes
> 	- let idxd driver use iommu.h for ioasid_t
> 	- return 0 after mm_pasid_set()
> ---
>  drivers/dma/idxd/idxd.h   |  1 +
>  drivers/iommu/iommu-sva.c | 53 +++++++++++----------------------------
>  drivers/iommu/iommu-sva.h |  3 ---
>  include/linux/sched/mm.h  |  8 +-----
>  4 files changed, 17 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> index 7ced8d283d98..417e602a46b6 100644
> --- a/drivers/dma/idxd/idxd.h
> +++ b/drivers/dma/idxd/idxd.h
> @@ -13,6 +13,7 @@
>  #include <linux/ioasid.h>
>  #include <linux/bitmap.h>
>  #include <linux/perf_event.h>
> +#include <linux/iommu.h>

Unrelated 

>  #include <uapi/linux/idxd.h>
>  #include "registers.h"
>  
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 24bf9b2b58aa..8fb6f1867af3 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -9,24 +9,11 @@
>  #include "iommu-sva.h"
>  
>  static DEFINE_MUTEX(iommu_sva_lock);
> -static DECLARE_IOASID_SET(iommu_sva_pasid);
> +static DEFINE_IDA(iommu_global_pasid_ida);
>  
> -/**
> - * iommu_sva_alloc_pasid - Allocate a PASID for the mm
> - * @mm: the mm
> - * @min: minimum PASID value (inclusive)
> - * @max: maximum PASID value (inclusive)
> - *
> - * Try to allocate a PASID for this mm, or take a reference to the existing one
> - * provided it fits within the [@min, @max] range. On success the PASID is
> - * available in mm->pasid and will be available for the lifetime of the mm.
> - *
> - * Returns 0 on success and < 0 on error.
> - */
> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> +static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
>  {
> -	int ret = 0;
> -	ioasid_t pasid;
> +	int ret;
>  
>  	if (min == INVALID_IOASID || max == INVALID_IOASID ||
>  	    min == 0 || max < min)
> @@ -37,39 +24,29 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
>  	if (pasid_valid(mm->pasid)) {
>  		if (mm->pasid < min || mm->pasid >= max)
>  			ret = -EOVERFLOW;
> +		else
> +			ret = 0;
>  		goto out;
>  	}
>  
> -	pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
> -	if (!pasid_valid(pasid))
> -		ret = -ENOMEM;
> -	else
> -		mm_pasid_set(mm, pasid);
> +	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
> +	if (ret < min)

Just check ret < 0

> +		goto out;
> +	mm_pasid_set(mm, ret);
> +	ret = 0;
>  out:
>  	mutex_unlock(&iommu_sva_lock);
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
>  
> -/* ioasid_find getter() requires a void * argument */
> -static bool __mmget_not_zero(void *mm)
> +void mm_pasid_drop(struct mm_struct *mm)
>  {
> -	return mmget_not_zero(mm);
> -}
> +	pr_alert("%s %d", __func__, mm->pasid);

Debug print

Thanks,
Jean

> +	if (likely(!pasid_valid(mm->pasid)))
> +		return;
>  
> -/**
> - * iommu_sva_find() - Find mm associated to the given PASID
> - * @pasid: Process Address Space ID assigned to the mm
> - *
> - * On success a reference to the mm is taken, and must be released with mmput().
> - *
> - * Returns the mm corresponding to this PASID, or an error if not found.
> - */
> -struct mm_struct *iommu_sva_find(ioasid_t pasid)
> -{
> -	return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
> +	ida_free(&iommu_global_pasid_ida, mm->pasid);
>  }
> -EXPORT_SYMBOL_GPL(iommu_sva_find);
>  
>  /**
>   * iommu_sva_bind_device() - Bind a process address space to a device
> diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
> index 7215a761b962..c22d0174ad61 100644
> --- a/drivers/iommu/iommu-sva.h
> +++ b/drivers/iommu/iommu-sva.h
> @@ -8,9 +8,6 @@
>  #include <linux/ioasid.h>
>  #include <linux/mm_types.h>
>  
> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
> -struct mm_struct *iommu_sva_find(ioasid_t pasid);
> -
>  /* I/O Page fault */
>  struct device;
>  struct iommu_fault;
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index 2a243616f222..cdc138086230 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -463,13 +463,7 @@ static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid)
>  	mm->pasid = pasid;
>  }
>  
> -static inline void mm_pasid_drop(struct mm_struct *mm)
> -{
> -	if (pasid_valid(mm->pasid)) {
> -		ioasid_free(mm->pasid);
> -		mm->pasid = INVALID_IOASID;
> -	}
> -}
> +void mm_pasid_drop(struct mm_struct *mm);
>  #else
>  static inline void mm_pasid_init(struct mm_struct *mm) {}
>  static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid) {}
> -- 
> 2.25.1
>
Re: [PATCH v2 2/3] iommu/sva: Stop using ioasid_set for SVA
Posted by Jacob Pan 2 years, 6 months ago
Hi Jean-Philippe,

On Tue, 14 Feb 2023 09:39:42 +0000, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:

> On Mon, Feb 13, 2023 at 10:44:13PM -0800, Jacob Pan wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > 
> > Instead SVA drivers can use a simple global IDA to allocate PASIDs for
> > each mm_struct.
> > 
> > Future work would be to allow drivers using the SVA APIs to reserve
> > global PASIDs from this IDA for their internal use, eg with the DMA API
> > PASID support.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> > v2: minor fixes
> > 	- let idxd driver use iommu.h for ioasid_t
> > 	- return 0 after mm_pasid_set()
> > ---
> >  drivers/dma/idxd/idxd.h   |  1 +
> >  drivers/iommu/iommu-sva.c | 53 +++++++++++----------------------------
> >  drivers/iommu/iommu-sva.h |  3 ---
> >  include/linux/sched/mm.h  |  8 +-----
> >  4 files changed, 17 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> > index 7ced8d283d98..417e602a46b6 100644
> > --- a/drivers/dma/idxd/idxd.h
> > +++ b/drivers/dma/idxd/idxd.h
> > @@ -13,6 +13,7 @@
> >  #include <linux/ioasid.h>
> >  #include <linux/bitmap.h>
> >  #include <linux/perf_event.h>
> > +#include <linux/iommu.h>  
> 
> Unrelated 
this is for ioasid_t, won't compile without it 

> >  #include <uapi/linux/idxd.h>
> >  #include "registers.h"
> >  
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index 24bf9b2b58aa..8fb6f1867af3 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -9,24 +9,11 @@
> >  #include "iommu-sva.h"
> >  
> >  static DEFINE_MUTEX(iommu_sva_lock);
> > -static DECLARE_IOASID_SET(iommu_sva_pasid);
> > +static DEFINE_IDA(iommu_global_pasid_ida);
> >  
> > -/**
> > - * iommu_sva_alloc_pasid - Allocate a PASID for the mm
> > - * @mm: the mm
> > - * @min: minimum PASID value (inclusive)
> > - * @max: maximum PASID value (inclusive)
> > - *
> > - * Try to allocate a PASID for this mm, or take a reference to the
> > existing one
> > - * provided it fits within the [@min, @max] range. On success the
> > PASID is
> > - * available in mm->pasid and will be available for the lifetime of
> > the mm.
> > - *
> > - * Returns 0 on success and < 0 on error.
> > - */
> > -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t
> > max) +static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t
> > min, ioasid_t max) {
> > -	int ret = 0;
> > -	ioasid_t pasid;
> > +	int ret;
> >  
> >  	if (min == INVALID_IOASID || max == INVALID_IOASID ||
> >  	    min == 0 || max < min)
> > @@ -37,39 +24,29 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm,
> > ioasid_t min, ioasid_t max) if (pasid_valid(mm->pasid)) {
> >  		if (mm->pasid < min || mm->pasid >= max)
> >  			ret = -EOVERFLOW;
> > +		else
> > +			ret = 0;
> >  		goto out;
> >  	}
> >  
> > -	pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
> > -	if (!pasid_valid(pasid))
> > -		ret = -ENOMEM;
> > -	else
> > -		mm_pasid_set(mm, pasid);
> > +	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,
> > GFP_KERNEL);
> > +	if (ret < min)  
> 
> Just check ret < 0
yes, but i thought < min is safe too. 

> > +		goto out;
> > +	mm_pasid_set(mm, ret);
> > +	ret = 0;
> >  out:
> >  	mutex_unlock(&iommu_sva_lock);
> >  	return ret;
> >  }
> > -EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
> >  
> > -/* ioasid_find getter() requires a void * argument */
> > -static bool __mmget_not_zero(void *mm)
> > +void mm_pasid_drop(struct mm_struct *mm)
> >  {
> > -	return mmget_not_zero(mm);
> > -}
> > +	pr_alert("%s %d", __func__, mm->pasid);  
> 
> Debug print
will remove,
Thanks a lot!


Jacob
Re: [PATCH v2 2/3] iommu/sva: Stop using ioasid_set for SVA
Posted by Jean-Philippe Brucker 2 years, 6 months ago
On Wed, Feb 15, 2023 at 01:36:51PM -0800, Jacob Pan wrote:
> > > -	pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
> > > -	if (!pasid_valid(pasid))
> > > -		ret = -ENOMEM;
> > > -	else
> > > -		mm_pasid_set(mm, pasid);
> > > +	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,
> > > GFP_KERNEL);
> > > +	if (ret < min)  
> > 
> > Just check ret < 0
> yes, but i thought < min is safe too. 

It is, but 'ret < 0' is the standard used everywhere else. So someone
reading this code will stop and spend time trying to figure out why this
isn't 'ret < 0', wonder what's special here, inspect the ida_alloc_range()
doc, etc. No big deal, but I think it's unnecessary.

Thanks,
Jean