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 - 2025 Red Hat, Inc.