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> 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?
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
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
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
>
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
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
© 2016 - 2026 Red Hat, Inc.