Re: [PATCH v6 02/10] iommu/vt-d: Add custom allocator for IOASID
Hi, On 10/24/19 7:01 AM, Jacob Pan wrote: On Wed, 23 Oct 2019 10:21:51 +0800 Lu Baolu wrote: +#ifdef CONFIG_INTEL_IOMMU_SVM Maybe move them to intel-svm.c instead? that's where the bulk of the svm support is? I think this is a generic PASID allocator for guest IOMMU although vSVA is currently the only consumer. Instead of making it SVM specific, I'd like to suggest moving it to intel-pasid.c and replace the @svm parameter with a void * one in intel_ioasid_free(). make sense to use void*, no need to tie that to svm bind data type. In terms of location, perhaps we can move if we have more consumers of custom allocator? Make sense to me. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 02/10] iommu/vt-d: Add custom allocator for IOASID
On Tue, 22 Oct 2019 21:04:00 -0700
Jacob Pan wrote:
> > > + if (cap_caching_mode(iommu->cap) &&
> > > sm_supported(iommu)) {
> >
> > do you need to check against cap_caching_mode() or ecap_vcmd?
> >
> I guess ecap_vcmd() will suffice. Kind of redundant.
Actually, we can check vcmd and vcmd_pasid here, then we dont need to
check it on every alloc/free calls.
Thanks,
Jacob
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 02/10] iommu/vt-d: Add custom allocator for IOASID
On Wed, 23 Oct 2019 10:21:51 +0800 Lu Baolu wrote: > >> +#ifdef CONFIG_INTEL_IOMMU_SVM > > > > Maybe move them to intel-svm.c instead? that's where the bulk > > of the svm support is? > > I think this is a generic PASID allocator for guest IOMMU although > vSVA is currently the only consumer. Instead of making it SVM > specific, I'd like to suggest moving it to intel-pasid.c and replace > the @svm parameter with a void * one in intel_ioasid_free(). make sense to use void*, no need to tie that to svm bind data type. In terms of location, perhaps we can move if we have more consumers of custom allocator? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 02/10] iommu/vt-d: Add custom allocator for IOASID
On Tue, 22 Oct 2019 17:51:29 -0700
"Raj, Ashok" wrote:
> On Tue, Oct 22, 2019 at 04:53:15PM -0700, Jacob Pan wrote:
> > When VT-d driver runs in the guest, PASID allocation must be
> > performed via virtual command interface. This patch registers a
> > custom IOASID allocator which takes precedence over the default
> > XArray based allocator. The resulting IOASID allocation will always
> > come from the host. This ensures that PASID namespace is system-
> > wide.
> >
> > Signed-off-by: Lu Baolu
> > Signed-off-by: Liu, Yi L
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/Kconfig | 1 +
> > drivers/iommu/intel-iommu.c | 67
> > +
> > include/linux/intel-iommu.h | 2 ++ 3 files changed, 70
> > insertions(+)
> >
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index fd50ddbf..961fe5795a90 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -211,6 +211,7 @@ config INTEL_IOMMU_SVM
> > bool "Support for Shared Virtual Memory with Intel IOMMU"
> > depends on INTEL_IOMMU && X86
> > select PCI_PASID
> > + select IOASID
> > select MMU_NOTIFIER
> > help
> > Shared Virtual Memory (SVM) provides a facility for
> > devices diff --git a/drivers/iommu/intel-iommu.c
> > b/drivers/iommu/intel-iommu.c index 3f974919d3bd..3aff0141c522
> > 100644 --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -1706,6 +1706,8 @@ static void free_dmar_iommu(struct
> > intel_iommu *iommu) if (ecap_prs(iommu->ecap))
> > intel_svm_finish_prq(iommu);
> > }
> > + ioasid_unregister_allocator(>pasid_allocator);
> > +
> > #endif
> > }
> >
> > @@ -4910,6 +4912,46 @@ static int __init
> > probe_acpi_namespace_devices(void) return 0;
> > }
> >
> > +#ifdef CONFIG_INTEL_IOMMU_SVM
>
> Maybe move them to intel-svm.c instead? that's where the bulk
> of the svm support is?
>
The reason I put them in intel-iommu.c is that pasid allocators need to
be registered during initialization of Intel iommu. No strong
preference.
> > +static ioasid_t intel_ioasid_alloc(ioasid_t min, ioasid_t max,
> > void *data) +{
> > + struct intel_iommu *iommu = data;
> > + ioasid_t ioasid;
> > +
> > + /*
> > +* VT-d virtual command interface always uses the full 20
> > bit
> > +* PASID range. Host can partition guest PASID range based
> > on
> > +* policies but it is out of guest's control.
> > +*/
> > + if (min < PASID_MIN || max > PASID_MAX)
> > + return INVALID_IOASID;
>
> What are these PASID_MIN/MAX? Do you check if these are within the
> limits supported by the iommu/vIOMMU as its enumerated?
>
PASID_MIN/MAX is the full range, 1-2M.
I do not check range because VCMD interface will always use the
full range. vIOMMU will always support full 20 bit guest PASID.
The host VFIO code calls IOASID allocator which should respect IOMMU
enumerated range.
>
> > +
> > + if (vcmd_alloc_pasid(iommu, ))
> > + return INVALID_IOASID;
> > +
> > + return ioasid;
> > +}
> > +
> > +static void intel_ioasid_free(ioasid_t ioasid, void *data)
> > +{
> > + struct iommu_pasid_alloc_info *svm;
> > + struct intel_iommu *iommu = data;
> > +
> > + if (!iommu)
> > + return;
> > + /*
> > +* Sanity check the ioasid owner is done at upper layer,
> > e.g. VFIO
> > +* We can only free the PASID when all the devices are
> > unbond.
> > +*/
> > + svm = ioasid_find(NULL, ioasid, NULL);
> > + if (!svm) {
> > + pr_warn("Freeing unbond IOASID %d\n", ioasid);
> > + return;
> > + }
> > + vcmd_free_pasid(iommu, ioasid);
> > +}
> > +#endif
> > +
> > int __init intel_iommu_init(void)
> > {
> > int ret = -ENODEV;
> > @@ -5020,6 +5062,31 @@ int __init intel_iommu_init(void)
> >"%s", iommu->name);
> > iommu_device_set_ops(>iommu,
> > _iommu_ops); iommu_device_register(>iommu);
> > +#ifdef CONFIG_INTEL_IOMMU_SVM
> > + if (cap_caching_mode(iommu->cap) &&
> > sm_supported(iommu)) {
>
> do you need to check against cap_caching_mode() or ecap_vcmd?
>
I guess ecap_vcmd() will suffice. Kind of redundant.
>
> > + /*
> > +* Register a custom ASID allocator if we
> > are running
> > +* in a guest, the purpose is to have a
> > system wide PASID
> > +* namespace among all PASID users.
> > +* There can be multiple vIOMMUs in each
> > guest but only
> > +* one allocator is active. All vIOMMU
> > allocators will
> > +* eventually be calling the same host
> > allocator.
> > +*/
> > + iommu->pasid_allocator.alloc =
> > intel_ioasid_alloc;
> > + iommu->pasid_allocator.free =
> > intel_ioasid_free;
> > + iommu->pasid_allocator.pdata = (void
> > *)iommu;
> >
Re: [PATCH v6 02/10] iommu/vt-d: Add custom allocator for IOASID
Hi,
On 10/23/19 8:51 AM, Raj, Ashok wrote:
On Tue, Oct 22, 2019 at 04:53:15PM -0700, Jacob Pan wrote:
When VT-d driver runs in the guest, PASID allocation must be
performed via virtual command interface. This patch registers a
custom IOASID allocator which takes precedence over the default
XArray based allocator. The resulting IOASID allocation will always
come from the host. This ensures that PASID namespace is system-
wide.
Signed-off-by: Lu Baolu
Signed-off-by: Liu, Yi L
Signed-off-by: Jacob Pan
---
drivers/iommu/Kconfig | 1 +
drivers/iommu/intel-iommu.c | 67 +
include/linux/intel-iommu.h | 2 ++
3 files changed, 70 insertions(+)
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index fd50ddbf..961fe5795a90 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -211,6 +211,7 @@ config INTEL_IOMMU_SVM
bool "Support for Shared Virtual Memory with Intel IOMMU"
depends on INTEL_IOMMU && X86
select PCI_PASID
+ select IOASID
select MMU_NOTIFIER
help
Shared Virtual Memory (SVM) provides a facility for devices
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3f974919d3bd..3aff0141c522 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1706,6 +1706,8 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
if (ecap_prs(iommu->ecap))
intel_svm_finish_prq(iommu);
}
+ ioasid_unregister_allocator(>pasid_allocator);
+
#endif
}
@@ -4910,6 +4912,46 @@ static int __init probe_acpi_namespace_devices(void)
return 0;
}
+#ifdef CONFIG_INTEL_IOMMU_SVM
Maybe move them to intel-svm.c instead? that's where the bulk
of the svm support is?
I think this is a generic PASID allocator for guest IOMMU although vSVA
is currently the only consumer. Instead of making it SVM specific, I'd
like to suggest moving it to intel-pasid.c and replace the @svm
parameter with a void * one in intel_ioasid_free().
+static ioasid_t intel_ioasid_alloc(ioasid_t min, ioasid_t max, void *data)
+{
+ struct intel_iommu *iommu = data;
+ ioasid_t ioasid;
+
+ /*
+* VT-d virtual command interface always uses the full 20 bit
+* PASID range. Host can partition guest PASID range based on
+* policies but it is out of guest's control.
+*/
+ if (min < PASID_MIN || max > PASID_MAX)
+ return INVALID_IOASID;
What are these PASID_MIN/MAX? Do you check if these are within the
limits supported by the iommu/vIOMMU as its enumerated?
Is it an invalid request when @max is greater than hardware capability?
Say, the consumer is asking for allocate a PASID within [0, 2^20], while
the PASID pool of the allocator is just, say, [4, 2^19] with others
reserved for other special usage or due to iommu capability. Instead
of returning error, why not just allocating a PASID within [2, 2^19]?
In another word, final allocation range should be Range[allocator
supported] & Range[customer specified]. Please correct me if I missed
anything.
+
+ if (vcmd_alloc_pasid(iommu, ))
+ return INVALID_IOASID;
+
+ return ioasid;
+}
+
+static void intel_ioasid_free(ioasid_t ioasid, void *data)
+{
+ struct iommu_pasid_alloc_info *svm;
+ struct intel_iommu *iommu = data;
+
+ if (!iommu)
+ return;
+ /*
+* Sanity check the ioasid owner is done at upper layer, e.g. VFIO
+* We can only free the PASID when all the devices are unbond.
+*/
+ svm = ioasid_find(NULL, ioasid, NULL);
+ if (!svm) {
+ pr_warn("Freeing unbond IOASID %d\n", ioasid);
+ return;
+ }
+ vcmd_free_pasid(iommu, ioasid);
+}
+#endif
+
int __init intel_iommu_init(void)
{
int ret = -ENODEV;
@@ -5020,6 +5062,31 @@ int __init intel_iommu_init(void)
"%s", iommu->name);
iommu_device_set_ops(>iommu, _iommu_ops);
iommu_device_register(>iommu);
+#ifdef CONFIG_INTEL_IOMMU_SVM
+ if (cap_caching_mode(iommu->cap) && sm_supported(iommu)) {
do you need to check against cap_caching_mode() or ecap_vcmd?
+ /*
+* Register a custom ASID allocator if we are running
+* in a guest, the purpose is to have a system wide
PASID
+* namespace among all PASID users.
+* There can be multiple vIOMMUs in each guest but only
+* one allocator is active. All vIOMMU allocators will
+* eventually be calling the same host allocator.
+*/
+ iommu->pasid_allocator.alloc = intel_ioasid_alloc;
+ iommu->pasid_allocator.free = intel_ioasid_free;
+
Re: [PATCH v6 02/10] iommu/vt-d: Add custom allocator for IOASID
On Tue, Oct 22, 2019 at 04:53:15PM -0700, Jacob Pan wrote:
> When VT-d driver runs in the guest, PASID allocation must be
> performed via virtual command interface. This patch registers a
> custom IOASID allocator which takes precedence over the default
> XArray based allocator. The resulting IOASID allocation will always
> come from the host. This ensures that PASID namespace is system-
> wide.
>
> Signed-off-by: Lu Baolu
> Signed-off-by: Liu, Yi L
> Signed-off-by: Jacob Pan
> ---
> drivers/iommu/Kconfig | 1 +
> drivers/iommu/intel-iommu.c | 67
> +
> include/linux/intel-iommu.h | 2 ++
> 3 files changed, 70 insertions(+)
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index fd50ddbf..961fe5795a90 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -211,6 +211,7 @@ config INTEL_IOMMU_SVM
> bool "Support for Shared Virtual Memory with Intel IOMMU"
> depends on INTEL_IOMMU && X86
> select PCI_PASID
> + select IOASID
> select MMU_NOTIFIER
> help
> Shared Virtual Memory (SVM) provides a facility for devices
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3f974919d3bd..3aff0141c522 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1706,6 +1706,8 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
> if (ecap_prs(iommu->ecap))
> intel_svm_finish_prq(iommu);
> }
> + ioasid_unregister_allocator(>pasid_allocator);
> +
> #endif
> }
>
> @@ -4910,6 +4912,46 @@ static int __init probe_acpi_namespace_devices(void)
> return 0;
> }
>
> +#ifdef CONFIG_INTEL_IOMMU_SVM
Maybe move them to intel-svm.c instead? that's where the bulk
of the svm support is?
> +static ioasid_t intel_ioasid_alloc(ioasid_t min, ioasid_t max, void *data)
> +{
> + struct intel_iommu *iommu = data;
> + ioasid_t ioasid;
> +
> + /*
> + * VT-d virtual command interface always uses the full 20 bit
> + * PASID range. Host can partition guest PASID range based on
> + * policies but it is out of guest's control.
> + */
> + if (min < PASID_MIN || max > PASID_MAX)
> + return INVALID_IOASID;
What are these PASID_MIN/MAX? Do you check if these are within the
limits supported by the iommu/vIOMMU as its enumerated?
> +
> + if (vcmd_alloc_pasid(iommu, ))
> + return INVALID_IOASID;
> +
> + return ioasid;
> +}
> +
> +static void intel_ioasid_free(ioasid_t ioasid, void *data)
> +{
> + struct iommu_pasid_alloc_info *svm;
> + struct intel_iommu *iommu = data;
> +
> + if (!iommu)
> + return;
> + /*
> + * Sanity check the ioasid owner is done at upper layer, e.g. VFIO
> + * We can only free the PASID when all the devices are unbond.
> + */
> + svm = ioasid_find(NULL, ioasid, NULL);
> + if (!svm) {
> + pr_warn("Freeing unbond IOASID %d\n", ioasid);
> + return;
> + }
> + vcmd_free_pasid(iommu, ioasid);
> +}
> +#endif
> +
> int __init intel_iommu_init(void)
> {
> int ret = -ENODEV;
> @@ -5020,6 +5062,31 @@ int __init intel_iommu_init(void)
> "%s", iommu->name);
> iommu_device_set_ops(>iommu, _iommu_ops);
> iommu_device_register(>iommu);
> +#ifdef CONFIG_INTEL_IOMMU_SVM
> + if (cap_caching_mode(iommu->cap) && sm_supported(iommu)) {
do you need to check against cap_caching_mode() or ecap_vcmd?
> + /*
> + * Register a custom ASID allocator if we are running
> + * in a guest, the purpose is to have a system wide
> PASID
> + * namespace among all PASID users.
> + * There can be multiple vIOMMUs in each guest but only
> + * one allocator is active. All vIOMMU allocators will
> + * eventually be calling the same host allocator.
> + */
> + iommu->pasid_allocator.alloc = intel_ioasid_alloc;
> + iommu->pasid_allocator.free = intel_ioasid_free;
> + iommu->pasid_allocator.pdata = (void *)iommu;
> + ret =
> ioasid_register_allocator(>pasid_allocator);
> + if (ret) {
> + pr_warn("Custom PASID allocator registeration
> failed\n");
> + /*
> + * Disable scalable mode on this IOMMU if there
> + * is no custom allocator. Mixing SM capable
> vIOMMU
> + * and non-SM vIOMMU are not supported.
> + */
> + intel_iommu_sm = 0;
> + }
> + }
> +#endif
> }
>
> bus_set_iommu(_bus_type,
[PATCH v6 02/10] iommu/vt-d: Add custom allocator for IOASID
When VT-d driver runs in the guest, PASID allocation must be
performed via virtual command interface. This patch registers a
custom IOASID allocator which takes precedence over the default
XArray based allocator. The resulting IOASID allocation will always
come from the host. This ensures that PASID namespace is system-
wide.
Signed-off-by: Lu Baolu
Signed-off-by: Liu, Yi L
Signed-off-by: Jacob Pan
---
drivers/iommu/Kconfig | 1 +
drivers/iommu/intel-iommu.c | 67 +
include/linux/intel-iommu.h | 2 ++
3 files changed, 70 insertions(+)
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index fd50ddbf..961fe5795a90 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -211,6 +211,7 @@ config INTEL_IOMMU_SVM
bool "Support for Shared Virtual Memory with Intel IOMMU"
depends on INTEL_IOMMU && X86
select PCI_PASID
+ select IOASID
select MMU_NOTIFIER
help
Shared Virtual Memory (SVM) provides a facility for devices
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3f974919d3bd..3aff0141c522 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1706,6 +1706,8 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
if (ecap_prs(iommu->ecap))
intel_svm_finish_prq(iommu);
}
+ ioasid_unregister_allocator(>pasid_allocator);
+
#endif
}
@@ -4910,6 +4912,46 @@ static int __init probe_acpi_namespace_devices(void)
return 0;
}
+#ifdef CONFIG_INTEL_IOMMU_SVM
+static ioasid_t intel_ioasid_alloc(ioasid_t min, ioasid_t max, void *data)
+{
+ struct intel_iommu *iommu = data;
+ ioasid_t ioasid;
+
+ /*
+* VT-d virtual command interface always uses the full 20 bit
+* PASID range. Host can partition guest PASID range based on
+* policies but it is out of guest's control.
+*/
+ if (min < PASID_MIN || max > PASID_MAX)
+ return INVALID_IOASID;
+
+ if (vcmd_alloc_pasid(iommu, ))
+ return INVALID_IOASID;
+
+ return ioasid;
+}
+
+static void intel_ioasid_free(ioasid_t ioasid, void *data)
+{
+ struct iommu_pasid_alloc_info *svm;
+ struct intel_iommu *iommu = data;
+
+ if (!iommu)
+ return;
+ /*
+* Sanity check the ioasid owner is done at upper layer, e.g. VFIO
+* We can only free the PASID when all the devices are unbond.
+*/
+ svm = ioasid_find(NULL, ioasid, NULL);
+ if (!svm) {
+ pr_warn("Freeing unbond IOASID %d\n", ioasid);
+ return;
+ }
+ vcmd_free_pasid(iommu, ioasid);
+}
+#endif
+
int __init intel_iommu_init(void)
{
int ret = -ENODEV;
@@ -5020,6 +5062,31 @@ int __init intel_iommu_init(void)
"%s", iommu->name);
iommu_device_set_ops(>iommu, _iommu_ops);
iommu_device_register(>iommu);
+#ifdef CONFIG_INTEL_IOMMU_SVM
+ if (cap_caching_mode(iommu->cap) && sm_supported(iommu)) {
+ /*
+* Register a custom ASID allocator if we are running
+* in a guest, the purpose is to have a system wide
PASID
+* namespace among all PASID users.
+* There can be multiple vIOMMUs in each guest but only
+* one allocator is active. All vIOMMU allocators will
+* eventually be calling the same host allocator.
+*/
+ iommu->pasid_allocator.alloc = intel_ioasid_alloc;
+ iommu->pasid_allocator.free = intel_ioasid_free;
+ iommu->pasid_allocator.pdata = (void *)iommu;
+ ret =
ioasid_register_allocator(>pasid_allocator);
+ if (ret) {
+ pr_warn("Custom PASID allocator registeration
failed\n");
+ /*
+* Disable scalable mode on this IOMMU if there
+* is no custom allocator. Mixing SM capable
vIOMMU
+* and non-SM vIOMMU are not supported.
+*/
+ intel_iommu_sm = 0;
+ }
+ }
+#endif
}
bus_set_iommu(_bus_type, _iommu_ops);
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index eea7468694a7..fb1973a761c1 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -542,6 +543,7 @@ struct intel_iommu {
#ifdef CONFIG_INTEL_IOMMU_SVM
struct page_req_dsc *prq;
unsigned char prq_name[16];/* Name for PRQ interrupt */
+
