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>
---
v4:
	- Keep GFP_ATOMIC flag for PASID allocation, will changed to
	GFP_KERNEL in a separate patch.
---
 drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
 drivers/iommu/iommu-sva.h |  3 --
 2 files changed, 15 insertions(+), 50 deletions(-)

diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 376b2a9e2543..297852ae5e7c 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -9,26 +9,13 @@
 #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 ||
+	if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
 	    min == 0 || max < min)
 		return -EINVAL;
 
@@ -37,39 +24,20 @@ 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 = ret;
+	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_ATOMIC);
+	if (ret < min)
+		goto out;
+	mm->pasid = 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)
-{
-	return mmget_not_zero(mm);
-}
-
-/**
- * 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);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_find);
 
 /**
  * iommu_sva_bind_device() - Bind a process address space to a device
@@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
 
 void mm_pasid_drop(struct mm_struct *mm)
 {
-	if (pasid_valid(mm->pasid)) {
-		ioasid_free(mm->pasid);
-		mm->pasid = INVALID_IOASID;
-	}
+	if (likely(!pasid_valid(mm->pasid)))
+		return;
+
+	ida_free(&iommu_global_pasid_ida, mm->pasid);
 }
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;
-- 
2.25.1

Hi,

On 3/2/23 07:56, 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>
> ---
> v4:
> 	- Keep GFP_ATOMIC flag for PASID allocation, will changed to
> 	GFP_KERNEL in a separate patch.
> ---
>   drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
>   drivers/iommu/iommu-sva.h |  3 --
>   2 files changed, 15 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 376b2a9e2543..297852ae5e7c 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -9,26 +9,13 @@
>   #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 ||
> +	if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
>   	    min == 0 || max < min)
>   		return -EINVAL;
>   
> @@ -37,39 +24,20 @@ 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)
Here seems not right, since the valid range is defined [min, max]. 
Shouldn't the invalid range be:
		if (mm->pasid < min || mm->pasid > max)

Regards,
-Tina

>   			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 = ret;
> +	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_ATOMIC);
> +	if (ret < min)
> +		goto out;
> +	mm->pasid = 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)
> -{
> -	return mmget_not_zero(mm);
> -}
> -
> -/**
> - * 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);
> -}
> -EXPORT_SYMBOL_GPL(iommu_sva_find);
>   
>   /**
>    * iommu_sva_bind_device() - Bind a process address space to a device
> @@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
>   
>   void mm_pasid_drop(struct mm_struct *mm)
>   {
> -	if (pasid_valid(mm->pasid)) {
> -		ioasid_free(mm->pasid);
> -		mm->pasid = INVALID_IOASID;
> -	}
> +	if (likely(!pasid_valid(mm->pasid)))
> +		return;
> +
> +	ida_free(&iommu_global_pasid_ida, mm->pasid);
>   }
> 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;

Hi Tina,

On Thu, 2 Mar 2023 21:52:42 +0800, Tina Zhang <tina.zhang@intel.com> wrote:

> >   		if (mm->pasid < min || mm->pasid >= max)  
> Here seems not right, since the valid range is defined [min, max]. 
> Shouldn't the invalid range be:
> 		if (mm->pasid < min || mm->pasid > max)
yes it is better to be consistent even if we removed the inclusive
requirements in the previous comments.

Thanks,

Jacob

On 2023/3/2 7:56, 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>
> ---
> v4:
> 	- Keep GFP_ATOMIC flag for PASID allocation, will changed to
> 	GFP_KERNEL in a separate patch.
> ---
>   drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
>   drivers/iommu/iommu-sva.h |  3 --
>   2 files changed, 15 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 376b2a9e2543..297852ae5e7c 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -9,26 +9,13 @@
>   #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 ||
> +	if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
>   	    min == 0 || max < min)

It's irrelevant to this patch. Just out of curiosity, why do we need to
exclude PASID 0 here? I just had a quick look at PCI spec section 6.20.
The spec does not state that PASID 0 is invalid.

>   		return -EINVAL;
>   
> @@ -37,39 +24,20 @@ 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;

Nit:

If you didn't change "int ret = 0" to "int ret", we don't need above two
lines. Did I miss anything?

>   		goto out;
>   	}
>   
> -	pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
> -	if (!pasid_valid(pasid))
> -		ret = -ENOMEM;
> -	else
> -		mm->pasid = ret;
> +	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_ATOMIC);
> +	if (ret < min)

Nit:
	    ret < 0?

ida_alloc_range() returns negative error number on failure.

> +		goto out;
> +	mm->pasid = 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)
> -{
> -	return mmget_not_zero(mm);
> -}
> -
> -/**
> - * 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);
> -}
> -EXPORT_SYMBOL_GPL(iommu_sva_find);

Removing iommu_sva_find() has nothing to do with the intention of this
patch. Perhaps make it in a separated patch?

>   
>   /**
>    * iommu_sva_bind_device() - Bind a process address space to a device
> @@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
>   
>   void mm_pasid_drop(struct mm_struct *mm)
>   {
> -	if (pasid_valid(mm->pasid)) {
> -		ioasid_free(mm->pasid);
> -		mm->pasid = INVALID_IOASID;
> -	}
> +	if (likely(!pasid_valid(mm->pasid)))

Why is this a likely?

> +		return;
> +
> +	ida_free(&iommu_global_pasid_ida, mm->pasid);
>   }
> 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;

Best regards,
baolu

Hi Baolu,

On Thu, 2 Mar 2023 21:01:42 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> On 2023/3/2 7:56, 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>
> > ---
> > v4:
> > 	- Keep GFP_ATOMIC flag for PASID allocation, will changed to
> > 	GFP_KERNEL in a separate patch.
> > ---
> >   drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
> >   drivers/iommu/iommu-sva.h |  3 --
> >   2 files changed, 15 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index 376b2a9e2543..297852ae5e7c 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -9,26 +9,13 @@
> >   #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 ||
> > +	if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
> >   	    min == 0 || max < min)  
> 
> It's irrelevant to this patch. Just out of curiosity, why do we need to
> exclude PASID 0 here? I just had a quick look at PCI spec section 6.20.
> The spec does not state that PASID 0 is invalid.
> 
my understanding is that ARM reserves PASID0, unlike VT-d where RID_PASID
is programmable.

> >   		return -EINVAL;
> >   
> > @@ -37,39 +24,20 @@ 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;  
> 
> Nit:
> 
> If you didn't change "int ret = 0" to "int ret", we don't need above two
> lines. Did I miss anything?
> 
you are right

> >   		goto out;
> >   	}
> >   
> > -	pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
> > -	if (!pasid_valid(pasid))
> > -		ret = -ENOMEM;
> > -	else
> > -		mm->pasid = ret;
> > +	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,
> > GFP_ATOMIC);
> > +	if (ret < min)  
> 
> Nit:
> 	    ret < 0?
will do

> ida_alloc_range() returns negative error number on failure.
> 
> > +		goto out;
> > +	mm->pasid = 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)
> > -{
> > -	return mmget_not_zero(mm);
> > -}
> > -
> > -/**
> > - * 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);
> > -}
> > -EXPORT_SYMBOL_GPL(iommu_sva_find);  
> 
> Removing iommu_sva_find() has nothing to do with the intention of this
> patch. Perhaps make it in a separated patch?
will do

> >   
> >   /**
> >    * iommu_sva_bind_device() - Bind a process address space to a device
> > @@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault,
> > void *data) 
> >   void mm_pasid_drop(struct mm_struct *mm)
> >   {
> > -	if (pasid_valid(mm->pasid)) {
> > -		ioasid_free(mm->pasid);
> > -		mm->pasid = INVALID_IOASID;
> > -	}
> > +	if (likely(!pasid_valid(mm->pasid)))  
> 
> Why is this a likely?
most mm does not have a PASID, thus initialized with invalid ioasid during
fork. This function is called for every mm.

> > +		return;
> > +
> > +	ida_free(&iommu_global_pasid_ida, mm->pasid);
> >   }
> > 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;  
> 
> Best regards,
> baolu


Thanks,

Jacob

On 3/3/23 1:17 AM, Jacob Pan wrote:
> Hi Baolu,
> 
> On Thu, 2 Mar 2023 21:01:42 +0800, Baolu Lu <baolu.lu@linux.intel.com>
> wrote:
> 
>> On 2023/3/2 7:56, 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>
>>> ---
>>> v4:
>>> 	- Keep GFP_ATOMIC flag for PASID allocation, will changed to
>>> 	GFP_KERNEL in a separate patch.
>>> ---
>>>    drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
>>>    drivers/iommu/iommu-sva.h |  3 --
>>>    2 files changed, 15 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
>>> index 376b2a9e2543..297852ae5e7c 100644
>>> --- a/drivers/iommu/iommu-sva.c
>>> +++ b/drivers/iommu/iommu-sva.c
>>> @@ -9,26 +9,13 @@
>>>    #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 ||
>>> +	if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
>>>    	    min == 0 || max < min)
>>
>> It's irrelevant to this patch. Just out of curiosity, why do we need to
>> exclude PASID 0 here? I just had a quick look at PCI spec section 6.20.
>> The spec does not state that PASID 0 is invalid.
>>
> my understanding is that ARM reserves PASID0, unlike VT-d where RID_PASID
> is programmable.

I suppose the common thing is reserving some kind of special PASIDs.

> 
>>>    		return -EINVAL;
>>>    
>>> @@ -37,39 +24,20 @@ 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;
>>
>> Nit:
>>
>> If you didn't change "int ret = 0" to "int ret", we don't need above two
>> lines. Did I miss anything?
>>
> you are right
> 
>>>    		goto out;
>>>    	}
>>>    
>>> -	pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
>>> -	if (!pasid_valid(pasid))
>>> -		ret = -ENOMEM;
>>> -	else
>>> -		mm->pasid = ret;
>>> +	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,
>>> GFP_ATOMIC);
>>> +	if (ret < min)
>>
>> Nit:
>> 	    ret < 0?
> will do
> 
>> ida_alloc_range() returns negative error number on failure.
>>
>>> +		goto out;
>>> +	mm->pasid = 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)
>>> -{
>>> -	return mmget_not_zero(mm);
>>> -}
>>> -
>>> -/**
>>> - * 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);
>>> -}
>>> -EXPORT_SYMBOL_GPL(iommu_sva_find);
>>
>> Removing iommu_sva_find() has nothing to do with the intention of this
>> patch. Perhaps make it in a separated patch?
> will do
> 
>>>    
>>>    /**
>>>     * iommu_sva_bind_device() - Bind a process address space to a device
>>> @@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault,
>>> void *data)
>>>    void mm_pasid_drop(struct mm_struct *mm)
>>>    {
>>> -	if (pasid_valid(mm->pasid)) {
>>> -		ioasid_free(mm->pasid);
>>> -		mm->pasid = INVALID_IOASID;
>>> -	}
>>> +	if (likely(!pasid_valid(mm->pasid)))
>>
>> Why is this a likely?
> most mm does not have a PASID, thus initialized with invalid ioasid during
> fork. This function is called for every mm.

Make sense.

> 
>>> +		return;
>>> +
>>> +	ida_free(&iommu_global_pasid_ida, mm->pasid);
>>>    }
>>> 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;
>>
>> Best regards,
>> baolu
> 
> 
> Thanks,
> 
> Jacob

Best regards,
baolu

On Fri, Mar 03, 2023 at 10:24:42AM +0800, Baolu Lu wrote:
> > > > -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 ||
> > > > +	if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
> > > >    	    min == 0 || max < min)
> > > 
> > > It's irrelevant to this patch. Just out of curiosity, why do we need to
> > > exclude PASID 0 here? I just had a quick look at PCI spec section 6.20.
> > > The spec does not state that PASID 0 is invalid.
> > > 
> > my understanding is that ARM reserves PASID0, unlike VT-d where RID_PASID
> > is programmable.

It does, but that's specific to the IOMMU driver so we shouldn't check it
here.

> I suppose the common thing is reserving some kind of special PASIDs.

Are you planning to use RID_PASID != 0 in VT-d?  Otherwise we could just
communicate min_pasid from the IOMMU driver the same way we do max_pasid.

Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
driver calls to reserve PASID0/RID_PASID.

Thanks,
Jean

On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
>> I suppose the common thing is reserving some kind of special PASIDs.
> Are you planning to use RID_PASID != 0 in VT-d?  Otherwise we could just
> communicate min_pasid from the IOMMU driver the same way we do max_pasid.
> 
> Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> driver calls to reserve PASID0/RID_PASID.

Yes. We probably will use a non-zero RID_PASID in the future. An
interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
should work then.

Best regards,
baolu

On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
> > > I suppose the common thing is reserving some kind of special PASIDs.
> > Are you planning to use RID_PASID != 0 in VT-d?  Otherwise we could just
> > communicate min_pasid from the IOMMU driver the same way we do max_pasid.
> > 
> > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > driver calls to reserve PASID0/RID_PASID.
> 
> Yes. We probably will use a non-zero RID_PASID in the future. An
> interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> should work then.

Just allowing the driver to store XA_ZERO_ENTRY would be fine

Jason

Hi Jason,

On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> > On 2023/3/3 17:32, Jean-Philippe Brucker wrote:  
> > > > I suppose the common thing is reserving some kind of special
> > > > PASIDs.  
> > > Are you planning to use RID_PASID != 0 in VT-d?  Otherwise we could
> > > just communicate min_pasid from the IOMMU driver the same way we do
> > > max_pasid.
> > > 
> > > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > > driver calls to reserve PASID0/RID_PASID.  
> > 
> > Yes. We probably will use a non-zero RID_PASID in the future. An
> > interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> > should work then.  
> 
> Just allowing the driver to store XA_ZERO_ENTRY would be fine
> 
It looks like there are incoming users of iommu_sva_find()
https://lore.kernel.org/lkml/20230306163138.587484-1-fenghua.yu@intel.com/T/#m1fc97725a0e56ea269c8bdabacee447070d51846
Should we keep the xa here instead of the global ida?

 
Thanks,

Jacob

On Tue, Mar 07, 2023 at 02:32:09PM -0800, Jacob Pan wrote:
> Hi Jason,
> 
> On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> > > On 2023/3/3 17:32, Jean-Philippe Brucker wrote:  
> > > > > I suppose the common thing is reserving some kind of special
> > > > > PASIDs.  
> > > > Are you planning to use RID_PASID != 0 in VT-d?  Otherwise we could
> > > > just communicate min_pasid from the IOMMU driver the same way we do
> > > > max_pasid.
> > > > 
> > > > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > > > driver calls to reserve PASID0/RID_PASID.  
> > > 
> > > Yes. We probably will use a non-zero RID_PASID in the future. An
> > > interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> > > should work then.  
> > 
> > Just allowing the driver to store XA_ZERO_ENTRY would be fine
> > 
> It looks like there are incoming users of iommu_sva_find()
> https://lore.kernel.org/lkml/20230306163138.587484-1-fenghua.yu@intel.com/T/#m1fc97725a0e56ea269c8bdabacee447070d51846
> Should we keep the xa here instead of the global ida?

I'm not sure this should be in the iommu core, it is really gross.

I would expect IDXD to keep track of the PASID's and mms it is using
and do this kind of stuff itself.

And why is this using access_remote_vm anyhow? If you know you are in
a kthread then kthread_use_mm() is probably better anyhow.

In any event we don't need a iommu_sva_find() function to wrapper
xa_load for another function inside the same .c file.

Jason

Hi, Jason and Jacob,

On 3/8/23 10:23, Jason Gunthorpe wrote:
> On Tue, Mar 07, 2023 at 02:32:09PM -0800, Jacob Pan wrote:
>> Hi Jason,
>>
>> On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>
>>> On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
>>>> On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
>>>>>> I suppose the common thing is reserving some kind of special
>>>>>> PASIDs.
>>>>> Are you planning to use RID_PASID != 0 in VT-d?  Otherwise we could
>>>>> just communicate min_pasid from the IOMMU driver the same way we do
>>>>> max_pasid.
>>>>>
>>>>> Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
>>>>> driver calls to reserve PASID0/RID_PASID.
>>>>
>>>> Yes. We probably will use a non-zero RID_PASID in the future. An
>>>> interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
>>>> should work then.
>>>
>>> Just allowing the driver to store XA_ZERO_ENTRY would be fine
>>>
>> It looks like there are incoming users of iommu_sva_find()
>> https://lore.kernel.org/lkml/20230306163138.587484-1-fenghua.yu@intel.com/T/#m1fc97725a0e56ea269c8bdabacee447070d51846
>> Should we keep the xa here instead of the global ida?
> 
> I'm not sure this should be in the iommu core, it is really gross.
> 
> I would expect IDXD to keep track of the PASID's and mms it is using
> and do this kind of stuff itself.
> 
> And why is this using access_remote_vm anyhow? If you know you are in
> a kthread then kthread_use_mm() is probably better anyhow.
> 
> In any event we don't need a iommu_sva_find() function to wrapper
> xa_load for another function inside the same .c file.

Ok. I will maintain mm and find mm from PASID inside IDXD driver. And 
will implement accessing the remote mm inside IDXD driver although the 
implementation will have duplicate code as access_remote_vm().

Thanks.

-Fenghua

On Sat, Mar 11, 2023 at 09:18:30AM -0800, Fenghua Yu wrote:

> Ok. I will maintain mm and find mm from PASID inside IDXD driver. And will
> implement accessing the remote mm inside IDXD driver although the
> implementation will have duplicate code as access_remote_vm().

If you really need it and it really makes sense, then export it 

Jason

Hi Jason,

On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> > On 2023/3/3 17:32, Jean-Philippe Brucker wrote:  
> > > > I suppose the common thing is reserving some kind of special
> > > > PASIDs.  
> > > Are you planning to use RID_PASID != 0 in VT-d?  Otherwise we could
> > > just communicate min_pasid from the IOMMU driver the same way we do
> > > max_pasid.
> > > 
> > > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > > driver calls to reserve PASID0/RID_PASID.  
> > 
> > Yes. We probably will use a non-zero RID_PASID in the future. An
> > interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> > should work then.  
> 
> Just allowing the driver to store XA_ZERO_ENTRY would be fine
> 
So we provide APIs for both?
1. alloc a global PASID, returned by this API
2. try to reserve a global PASID given by the driver, i.e.
	xa_cmpxchg(&iommu_global_pasid_ida.xa, 2, NULL, XA_ZERO_ENTRY,
			 GFP_KERNEL);
seems #1 is sufficient.

Thanks,

Jacob

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Wednesday, March 8, 2023 1:42 AM
> 
> Hi Jason,
> 
> On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <jgg@nvidia.com>
> wrote:
> 
> > On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> > > On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
> > > > > I suppose the common thing is reserving some kind of special
> > > > > PASIDs.
> > > > Are you planning to use RID_PASID != 0 in VT-d?  Otherwise we could
> > > > just communicate min_pasid from the IOMMU driver the same way we
> do
> > > > max_pasid.
> > > >
> > > > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > > > driver calls to reserve PASID0/RID_PASID.
> > >
> > > Yes. We probably will use a non-zero RID_PASID in the future. An
> > > interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> > > should work then.
> >
> > Just allowing the driver to store XA_ZERO_ENTRY would be fine
> >
> So we provide APIs for both?
> 1. alloc a global PASID, returned by this API
> 2. try to reserve a global PASID given by the driver, i.e.
> 	xa_cmpxchg(&iommu_global_pasid_ida.xa, 2, NULL,
> XA_ZERO_ENTRY,
> 			 GFP_KERNEL);
> seems #1 is sufficient.
> 

No need for both. There will be on-demand allocation on this global
space so a reservation interface doesn't make sense.

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Thursday, March 2, 2023 7:57 AM
> 
> 
> -	if (min == INVALID_IOASID || max == INVALID_IOASID ||
> +	if (min == IOMMU_PASID_INVALID || max ==
> IOMMU_PASID_INVALID ||
>  	    min == 0 || max < min)
>  		return -EINVAL;
> 

if (!pasid_valid(min) || !pasid_valid(max) || ...)

with that,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Hi Kevin,

On Thu, 2 Mar 2023 08:58:21 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Thursday, March 2, 2023 7:57 AM
> > 
> > 
> > -	if (min == INVALID_IOASID || max == INVALID_IOASID ||
> > +	if (min == IOMMU_PASID_INVALID || max ==
> > IOMMU_PASID_INVALID ||
> >  	    min == 0 || max < min)
> >  		return -EINVAL;
> >   
> 
> if (!pasid_valid(min) || !pasid_valid(max) || ...)
> 
will do
> with that,
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 


Thanks,

Jacob