Re: [PATCH v6 03/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID
On Tue, 22 Oct 2019 17:58:59 -0700
"Raj, Ashok" wrote:
> On Tue, Oct 22, 2019 at 04:53:16PM -0700, Jacob Pan wrote:
> > Make use of generic IOASID code to manage PASID allocation,
> > free, and lookup. Replace Intel specific code.
> >
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/intel-iommu.c | 12 ++--
> > drivers/iommu/intel-pasid.c | 36
> > drivers/iommu/intel-svm.c |
> > 37 + 3 files changed, 27
> > insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/iommu/intel-iommu.c
> > b/drivers/iommu/intel-iommu.c index 3aff0141c522..72febcf2c48f
> > 100644 --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -5311,7 +5311,7 @@ static void auxiliary_unlink_device(struct
> > dmar_domain *domain, domain->auxd_refcnt--;
> >
> > if (!domain->auxd_refcnt && domain->default_pasid > 0)
> > - intel_pasid_free_id(domain->default_pasid);
> > + ioasid_free(domain->default_pasid);
>
> if the domain is gauranteed to be torn down, its ok.. but otherwise
> do you need to clear domain->default_pasid to avoid accidental
> reference in some other code path?
>
Good point, but i think it is out of the scope of this patch. Perhaps
another patch to fix that.
> > }
> >
> > static int aux_domain_add_dev(struct dmar_domain *domain,
> > @@ -5329,10 +5329,10 @@ static int aux_domain_add_dev(struct
> > dmar_domain *domain, if (domain->default_pasid <= 0) {
> > int pasid;
> >
> > - pasid = intel_pasid_alloc_id(domain, PASID_MIN,
> > -
> > pci_max_pasids(to_pci_dev(dev)),
> > -GFP_KERNEL);
> > - if (pasid <= 0) {
> > + /* No private data needed for the default pasid */
> > + pasid = ioasid_alloc(NULL, PASID_MIN,
> > pci_max_pasids(to_pci_dev(dev)) - 1,
>
> If we ensure IOMMU max-pasid is full 20bit width, its good. In a
> vIOMMU if iommu max pasid is restricted for some kind of partitioning
> in future you want to consider limiting to no more than what's
> provisioned in the vIOMMU right?
>
Yes, I agree we should double check IOMMU ecap PSS.
But it is outside the scope of this patch, which is merely replacing the
current logic with a different API.
Also, unless we do nested mdev, this code should run in host only,
no vIOMMU, right?
>
> > + NULL);
> > + if (pasid == INVALID_IOASID) {
> > pr_err("Can't allocate default pasid\n");
> > return -ENODEV;
> > }
> > @@ -5368,7 +5368,7 @@ static int aux_domain_add_dev(struct
> > dmar_domain *domain, spin_unlock(>lock);
> > spin_unlock_irqrestore(_domain_lock, flags);
> > if (!domain->auxd_refcnt && domain->default_pasid > 0)
> > - intel_pasid_free_id(domain->default_pasid);
> > + ioasid_free(domain->default_pasid);
> >
> > return ret;
> > }
> > diff --git a/drivers/iommu/intel-pasid.c
> > b/drivers/iommu/intel-pasid.c index 76bcbb21e112..c0d1f28d3412
> > 100644 --- a/drivers/iommu/intel-pasid.c
> > +++ b/drivers/iommu/intel-pasid.c
> > @@ -26,42 +26,6 @@
> > */
> > static DEFINE_SPINLOCK(pasid_lock);
> > u32 intel_pasid_max_id = PASID_MAX;
> > -static DEFINE_IDR(pasid_idr);
> > -
> > -int intel_pasid_alloc_id(void *ptr, int start, int end, gfp_t gfp)
> > -{
> > - int ret, min, max;
> > -
> > - min = max_t(int, start, PASID_MIN);
> > - max = min_t(int, end, intel_pasid_max_id);
> > -
> > - WARN_ON(in_interrupt());
> > - idr_preload(gfp);
> > - spin_lock(_lock);
> > - ret = idr_alloc(_idr, ptr, min, max, GFP_ATOMIC);
> > - spin_unlock(_lock);
> > - idr_preload_end();
> > -
> > - return ret;
> > -}
> > -
> > -void intel_pasid_free_id(int pasid)
> > -{
> > - spin_lock(_lock);
> > - idr_remove(_idr, pasid);
> > - spin_unlock(_lock);
> > -}
> > -
> > -void *intel_pasid_lookup_id(int pasid)
> > -{
> > - void *p;
> > -
> > - spin_lock(_lock);
> > - p = idr_find(_idr, pasid);
> > - spin_unlock(_lock);
> > -
> > - return p;
> > -}
> >
> > static int check_vcmd_pasid(struct intel_iommu *iommu)
> > {
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index 9b159132405d..5aef5b7bf561 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "intel-pasid.h"
> > @@ -318,16 +319,15 @@ int intel_svm_bind_mm(struct device *dev, int
> > *pasid, int flags, struct svm_dev_ if (pasid_max >
> > intel_pasid_max_id) pasid_max = intel_pasid_max_id;
> >
> > - /* Do not use PASID 0 in caching mode (virtualised
> > IOMMU) */
> > - ret = intel_pasid_alloc_id(svm,
> > - !!cap_caching_mode(iommu->cap),
> > - pasid_max - 1,
> > GFP_KERNEL);
> > -
Re: [PATCH v6 03/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID
On Tue, Oct 22, 2019 at 04:53:16PM -0700, Jacob Pan wrote:
> Make use of generic IOASID code to manage PASID allocation,
> free, and lookup. Replace Intel specific code.
>
> Signed-off-by: Jacob Pan
> ---
> drivers/iommu/intel-iommu.c | 12 ++--
> drivers/iommu/intel-pasid.c | 36
> drivers/iommu/intel-svm.c | 37 +
> 3 files changed, 27 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3aff0141c522..72febcf2c48f 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -5311,7 +5311,7 @@ static void auxiliary_unlink_device(struct dmar_domain
> *domain,
> domain->auxd_refcnt--;
>
> if (!domain->auxd_refcnt && domain->default_pasid > 0)
> - intel_pasid_free_id(domain->default_pasid);
> + ioasid_free(domain->default_pasid);
if the domain is gauranteed to be torn down, its ok.. but otherwise
do you need to clear domain->default_pasid to avoid accidental reference
in some other code path?
> }
>
> static int aux_domain_add_dev(struct dmar_domain *domain,
> @@ -5329,10 +5329,10 @@ static int aux_domain_add_dev(struct dmar_domain
> *domain,
> if (domain->default_pasid <= 0) {
> int pasid;
>
> - pasid = intel_pasid_alloc_id(domain, PASID_MIN,
> - pci_max_pasids(to_pci_dev(dev)),
> - GFP_KERNEL);
> - if (pasid <= 0) {
> + /* No private data needed for the default pasid */
> + pasid = ioasid_alloc(NULL, PASID_MIN,
> pci_max_pasids(to_pci_dev(dev)) - 1,
If we ensure IOMMU max-pasid is full 20bit width, its good. In a vIOMMU
if iommu max pasid is restricted for some kind of partitioning in future
you want to consider limiting to no more than what's provisioned in the vIOMMU
right?
> + NULL);
> + if (pasid == INVALID_IOASID) {
> pr_err("Can't allocate default pasid\n");
> return -ENODEV;
> }
> @@ -5368,7 +5368,7 @@ static int aux_domain_add_dev(struct dmar_domain
> *domain,
> spin_unlock(>lock);
> spin_unlock_irqrestore(_domain_lock, flags);
> if (!domain->auxd_refcnt && domain->default_pasid > 0)
> - intel_pasid_free_id(domain->default_pasid);
> + ioasid_free(domain->default_pasid);
>
> return ret;
> }
> diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
> index 76bcbb21e112..c0d1f28d3412 100644
> --- a/drivers/iommu/intel-pasid.c
> +++ b/drivers/iommu/intel-pasid.c
> @@ -26,42 +26,6 @@
> */
> static DEFINE_SPINLOCK(pasid_lock);
> u32 intel_pasid_max_id = PASID_MAX;
> -static DEFINE_IDR(pasid_idr);
> -
> -int intel_pasid_alloc_id(void *ptr, int start, int end, gfp_t gfp)
> -{
> - int ret, min, max;
> -
> - min = max_t(int, start, PASID_MIN);
> - max = min_t(int, end, intel_pasid_max_id);
> -
> - WARN_ON(in_interrupt());
> - idr_preload(gfp);
> - spin_lock(_lock);
> - ret = idr_alloc(_idr, ptr, min, max, GFP_ATOMIC);
> - spin_unlock(_lock);
> - idr_preload_end();
> -
> - return ret;
> -}
> -
> -void intel_pasid_free_id(int pasid)
> -{
> - spin_lock(_lock);
> - idr_remove(_idr, pasid);
> - spin_unlock(_lock);
> -}
> -
> -void *intel_pasid_lookup_id(int pasid)
> -{
> - void *p;
> -
> - spin_lock(_lock);
> - p = idr_find(_idr, pasid);
> - spin_unlock(_lock);
> -
> - return p;
> -}
>
> static int check_vcmd_pasid(struct intel_iommu *iommu)
> {
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 9b159132405d..5aef5b7bf561 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -17,6 +17,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "intel-pasid.h"
> @@ -318,16 +319,15 @@ int intel_svm_bind_mm(struct device *dev, int *pasid,
> int flags, struct svm_dev_
> if (pasid_max > intel_pasid_max_id)
> pasid_max = intel_pasid_max_id;
>
> - /* Do not use PASID 0 in caching mode (virtualised IOMMU) */
> - ret = intel_pasid_alloc_id(svm,
> -!!cap_caching_mode(iommu->cap),
> -pasid_max - 1, GFP_KERNEL);
> - if (ret < 0) {
> + /* Do not use PASID 0, reserved for RID to PASID */
> + svm->pasid = ioasid_alloc(NULL, PASID_MIN,
> + pasid_max - 1, svm);
> + if (svm->pasid == INVALID_IOASID) {
> kfree(svm);
> kfree(sdev);
> + ret = ENOSPC;
> goto out;
> }
> - svm->pasid = ret;
>
[PATCH v6 03/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID
Make use of generic IOASID code to manage PASID allocation,
free, and lookup. Replace Intel specific code.
Signed-off-by: Jacob Pan
---
drivers/iommu/intel-iommu.c | 12 ++--
drivers/iommu/intel-pasid.c | 36
drivers/iommu/intel-svm.c | 37 +
3 files changed, 27 insertions(+), 58 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3aff0141c522..72febcf2c48f 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5311,7 +5311,7 @@ static void auxiliary_unlink_device(struct dmar_domain
*domain,
domain->auxd_refcnt--;
if (!domain->auxd_refcnt && domain->default_pasid > 0)
- intel_pasid_free_id(domain->default_pasid);
+ ioasid_free(domain->default_pasid);
}
static int aux_domain_add_dev(struct dmar_domain *domain,
@@ -5329,10 +5329,10 @@ static int aux_domain_add_dev(struct dmar_domain
*domain,
if (domain->default_pasid <= 0) {
int pasid;
- pasid = intel_pasid_alloc_id(domain, PASID_MIN,
-pci_max_pasids(to_pci_dev(dev)),
-GFP_KERNEL);
- if (pasid <= 0) {
+ /* No private data needed for the default pasid */
+ pasid = ioasid_alloc(NULL, PASID_MIN,
pci_max_pasids(to_pci_dev(dev)) - 1,
+ NULL);
+ if (pasid == INVALID_IOASID) {
pr_err("Can't allocate default pasid\n");
return -ENODEV;
}
@@ -5368,7 +5368,7 @@ static int aux_domain_add_dev(struct dmar_domain *domain,
spin_unlock(>lock);
spin_unlock_irqrestore(_domain_lock, flags);
if (!domain->auxd_refcnt && domain->default_pasid > 0)
- intel_pasid_free_id(domain->default_pasid);
+ ioasid_free(domain->default_pasid);
return ret;
}
diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
index 76bcbb21e112..c0d1f28d3412 100644
--- a/drivers/iommu/intel-pasid.c
+++ b/drivers/iommu/intel-pasid.c
@@ -26,42 +26,6 @@
*/
static DEFINE_SPINLOCK(pasid_lock);
u32 intel_pasid_max_id = PASID_MAX;
-static DEFINE_IDR(pasid_idr);
-
-int intel_pasid_alloc_id(void *ptr, int start, int end, gfp_t gfp)
-{
- int ret, min, max;
-
- min = max_t(int, start, PASID_MIN);
- max = min_t(int, end, intel_pasid_max_id);
-
- WARN_ON(in_interrupt());
- idr_preload(gfp);
- spin_lock(_lock);
- ret = idr_alloc(_idr, ptr, min, max, GFP_ATOMIC);
- spin_unlock(_lock);
- idr_preload_end();
-
- return ret;
-}
-
-void intel_pasid_free_id(int pasid)
-{
- spin_lock(_lock);
- idr_remove(_idr, pasid);
- spin_unlock(_lock);
-}
-
-void *intel_pasid_lookup_id(int pasid)
-{
- void *p;
-
- spin_lock(_lock);
- p = idr_find(_idr, pasid);
- spin_unlock(_lock);
-
- return p;
-}
static int check_vcmd_pasid(struct intel_iommu *iommu)
{
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 9b159132405d..5aef5b7bf561 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include "intel-pasid.h"
@@ -318,16 +319,15 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int
flags, struct svm_dev_
if (pasid_max > intel_pasid_max_id)
pasid_max = intel_pasid_max_id;
- /* Do not use PASID 0 in caching mode (virtualised IOMMU) */
- ret = intel_pasid_alloc_id(svm,
- !!cap_caching_mode(iommu->cap),
- pasid_max - 1, GFP_KERNEL);
- if (ret < 0) {
+ /* Do not use PASID 0, reserved for RID to PASID */
+ svm->pasid = ioasid_alloc(NULL, PASID_MIN,
+ pasid_max - 1, svm);
+ if (svm->pasid == INVALID_IOASID) {
kfree(svm);
kfree(sdev);
+ ret = ENOSPC;
goto out;
}
- svm->pasid = ret;
svm->notifier.ops = _mmuops;
svm->mm = mm;
svm->flags = flags;
@@ -337,7 +337,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int
flags, struct svm_dev_
if (mm) {
ret = mmu_notifier_register(>notifier, mm);
if (ret) {
- intel_pasid_free_id(svm->pasid);
+ ioasid_free(svm->pasid);
kfree(svm);
kfree(sdev);
goto out;
@@ -353,7 +353,7 @@ int
