| Message ID | 20230301235646.2692846-4-jacob.jun.pan@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove VT-d virtual command interface and IOASID | expand |
> 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>
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, 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 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
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
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 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 > 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
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
> 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.
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
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;