Mostly extracted from nested SVA/SVM series based on review comments of v7.
https://lkml.org/lkml/2019/10/24/852
This series also adds a few important fixes for native use of SVA. Nested
SVA new code will be submitted separately as a smaller set. Based on the
core branch of IOMMU tree staged for v5.5, where common APIs for vSVA were
applied.
git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git core
Changelog:
V2
- Coding style fixes based on Baolu's input, no functional change
- Added Acked-by tags.
Thanks,
Jacob
Jacob Pan (10):
iommu/vt-d: Introduce native SVM capable flag
iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks
iommu/vt-d: Reject SVM bind for failed capability check
iommu/vt-d: Match CPU and IOMMU paging mode
iommu/vt-d: Avoid duplicated code for PASID setup
iommu/vt-d: Fix off-by-one in PASID allocation
iommu/vt-d: Replace Intel specific PASID allocator with IOASID
iommu/vt-d: Fix PASID cache flush
iommu/vt-d: Avoid sending invalid page response
iommu/vt-d: Misc macro clean up for SVM
drivers/iommu/Kconfig | 1 +
drivers/iommu/intel-iommu.c | 23 +++----
drivers/iommu/intel-pasid.c | 99 ++++++++------------------
drivers/iommu/intel-svm.c | 164 +++++++++++++++++++++++++-------------------
include/linux/intel-iommu.h | 5 +-
5 files changed, 138 insertions(+), 154 deletions(-)
--
2.7.4
Use combined macros for_each_svm_dev() to simplify SVM device iteration
and error checking.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
Reviewed-by: Eric Auger <[email protected]>
Acked-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel-svm.c | 85 ++++++++++++++++++++++-------------------------
1 file changed, 40 insertions(+), 45 deletions(-)
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 098c89246593..9fe183376a20 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -226,6 +226,10 @@ static const struct mmu_notifier_ops intel_mmuops = {
static DEFINE_MUTEX(pasid_mutex);
static LIST_HEAD(global_svm_list);
+#define for_each_svm_dev(sdev, svm, d) \
+ list_for_each_entry((sdev), &(svm)->devs, list) \
+ if ((d) != (sdev)->dev) {} else
+
int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_ops *ops)
{
struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
@@ -274,15 +278,13 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
goto out;
}
- list_for_each_entry(sdev, &svm->devs, list) {
- if (dev == sdev->dev) {
- if (sdev->ops != ops) {
- ret = -EBUSY;
- goto out;
- }
- sdev->users++;
- goto success;
+ for_each_svm_dev(sdev, svm, dev) {
+ if (sdev->ops != ops) {
+ ret = -EBUSY;
+ goto out;
}
+ sdev->users++;
+ goto success;
}
break;
@@ -427,45 +429,38 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
goto out;
}
- if (!svm)
- goto out;
-
- list_for_each_entry(sdev, &svm->devs, list) {
- if (dev == sdev->dev) {
- ret = 0;
- sdev->users--;
- if (!sdev->users) {
- list_del_rcu(&sdev->list);
- /* Flush the PASID cache and IOTLB for this device.
- * Note that we do depend on the hardware *not* using
- * the PASID any more. Just as we depend on other
- * devices never using PASIDs that they have no right
- * to use. We have a *shared* PASID table, because it's
- * large and has to be physically contiguous. So it's
- * hard to be as defensive as we might like. */
- intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
- intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
- kfree_rcu(sdev, rcu);
-
- if (list_empty(&svm->devs)) {
- /* Clear private data so that free pass check */
- ioasid_set_data(svm->pasid, NULL);
- ioasid_free(svm->pasid);
- if (svm->mm)
- mmu_notifier_unregister(&svm->notifier, svm->mm);
-
- list_del(&svm->list);
-
- /* We mandate that no page faults may be outstanding
- * for the PASID when intel_svm_unbind_mm() is called.
- * If that is not obeyed, subtle errors will happen.
- * Let's make them less subtle... */
- memset(svm, 0x6b, sizeof(*svm));
- kfree(svm);
- }
+ for_each_svm_dev(sdev, svm, dev) {
+ ret = 0;
+ sdev->users--;
+ if (!sdev->users) {
+ list_del_rcu(&sdev->list);
+ /* Flush the PASID cache and IOTLB for this device.
+ * Note that we do depend on the hardware *not* using
+ * the PASID any more. Just as we depend on other
+ * devices never using PASIDs that they have no right
+ * to use. We have a *shared* PASID table, because it's
+ * large and has to be physically contiguous. So it's
+ * hard to be as defensive as we might like. */
+ intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
+ intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
+ kfree_rcu(sdev, rcu);
+
+ if (list_empty(&svm->devs)) {
+ /* Clear private data so that free pass check */
+ ioasid_set_data(svm->pasid, NULL);
+ ioasid_free(svm->pasid);
+ if (svm->mm)
+ mmu_notifier_unregister(&svm->notifier, svm->mm);
+ list_del(&svm->list);
+ /* We mandate that no page faults may be outstanding
+ * for the PASID when intel_svm_unbind_mm() is called.
+ * If that is not obeyed, subtle errors will happen.
+ * Let's make them less subtle... */
+ memset(svm, 0x6b, sizeof(*svm));
+ kfree(svm);
}
- break;
}
+ break;
}
out:
mutex_unlock(&pasid_mutex);
--
2.7.4
The current code checks CPU and IOMMU feature set for SVM support but
the result is never stored nor used. Therefore, SVM can still be used
even when these checks failed.
This patch consolidates code for checking PASID, CPU vs. IOMMU paging
mode compatibility, as well as provides specific error messages for
each failed checks.
Signed-off-by: Jacob Pan <[email protected]>
Acked-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel-iommu.c | 10 ++--------
drivers/iommu/intel-svm.c | 40 +++++++++++++++++++++++++++-------------
include/linux/intel-iommu.h | 4 +++-
3 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3f974919d3bd..d598168e410d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3289,10 +3289,7 @@ static int __init init_dmars(void)
if (!ecap_pass_through(iommu->ecap))
hw_pass_through = 0;
-#ifdef CONFIG_INTEL_IOMMU_SVM
- if (pasid_supported(iommu))
- intel_svm_init(iommu);
-#endif
+ intel_svm_check(iommu);
}
/*
@@ -4471,10 +4468,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
if (ret)
goto out;
-#ifdef CONFIG_INTEL_IOMMU_SVM
- if (pasid_supported(iommu))
- intel_svm_init(iommu);
-#endif
+ intel_svm_check(iommu);
if (dmaru->ignored) {
/*
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 9b159132405d..716c543488f6 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -23,19 +23,6 @@
static irqreturn_t prq_event_thread(int irq, void *d);
-int intel_svm_init(struct intel_iommu *iommu)
-{
- if (cpu_feature_enabled(X86_FEATURE_GBPAGES) &&
- !cap_fl1gp_support(iommu->cap))
- return -EINVAL;
-
- if (cpu_feature_enabled(X86_FEATURE_LA57) &&
- !cap_5lp_support(iommu->cap))
- return -EINVAL;
-
- return 0;
-}
-
#define PRQ_ORDER 0
int intel_svm_enable_prq(struct intel_iommu *iommu)
@@ -99,6 +86,33 @@ int intel_svm_finish_prq(struct intel_iommu *iommu)
return 0;
}
+static inline bool intel_svm_capable(struct intel_iommu *iommu)
+{
+ return iommu->flags & VTD_FLAG_SVM_CAPABLE;
+}
+
+void intel_svm_check(struct intel_iommu *iommu)
+{
+ if (!pasid_supported(iommu))
+ return;
+
+ if (cpu_feature_enabled(X86_FEATURE_GBPAGES) &&
+ !cap_fl1gp_support(iommu->cap)) {
+ pr_err("%s SVM disabled, incompatible 1GB page capability\n",
+ iommu->name);
+ return;
+ }
+
+ if (cpu_feature_enabled(X86_FEATURE_LA57) &&
+ !cap_5lp_support(iommu->cap)) {
+ pr_err("%s SVM disabled, incompatible paging mode\n",
+ iommu->name);
+ return;
+ }
+
+ iommu->flags |= VTD_FLAG_SVM_CAPABLE;
+}
+
static void intel_flush_svm_range_dev (struct intel_svm *svm, struct intel_svm_dev *sdev,
unsigned long address, unsigned long pages, int ih)
{
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 63118991824c..7dcfa1c4a844 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -657,7 +657,7 @@ void iommu_flush_write_buffer(struct intel_iommu *iommu);
int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev);
#ifdef CONFIG_INTEL_IOMMU_SVM
-int intel_svm_init(struct intel_iommu *iommu);
+extern void intel_svm_check(struct intel_iommu *iommu);
extern int intel_svm_enable_prq(struct intel_iommu *iommu);
extern int intel_svm_finish_prq(struct intel_iommu *iommu);
@@ -685,6 +685,8 @@ struct intel_svm {
};
extern struct intel_iommu *intel_svm_device_to_iommu(struct device *dev);
+#else
+static inline void intel_svm_check(struct intel_iommu *iommu) {}
#endif
#ifdef CONFIG_INTEL_IOMMU_DEBUGFS
--
2.7.4
When setting up first level page tables for sharing with CPU, we need
to ensure IOMMU can support no less than the levels supported by the
CPU.
It is not adequate, as in the current code, to set up 5-level paging
in PASID entry First Level Paging Mode(FLPM) solely based on CPU.
Fixes: 437f35e1cd4c8 ("iommu/vt-d: Add first level page table
interface")
Signed-off-by: Jacob Pan <[email protected]>
Acked-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel-pasid.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
index 040a445be300..e7cb0b8a7332 100644
--- a/drivers/iommu/intel-pasid.c
+++ b/drivers/iommu/intel-pasid.c
@@ -499,8 +499,16 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
}
#ifdef CONFIG_X86
- if (cpu_feature_enabled(X86_FEATURE_LA57))
- pasid_set_flpm(pte, 1);
+ /* Both CPU and IOMMU paging mode need to match */
+ if (cpu_feature_enabled(X86_FEATURE_LA57)) {
+ if (cap_5lp_support(iommu->cap)) {
+ pasid_set_flpm(pte, 1);
+ } else {
+ pr_err("VT-d has no 5-level paging support for CPU\n");
+ pasid_clear_entry(pte);
+ return -EINVAL;
+ }
+ }
#endif /* CONFIG_X86 */
pasid_set_domain_id(pte, did);
--
2.7.4
Shared Virtual Memory(SVM) is based on a collective set of hardware
features detected at runtime. There are requirements for matching CPU
and IOMMU capabilities.
This patch introduces a flag which will be used to mark and test the
capability of SVM.
Signed-off-by: Jacob Pan <[email protected]>
Acked-by: Lu Baolu <[email protected]>
---
include/linux/intel-iommu.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index ed11ef594378..63118991824c 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -433,6 +433,7 @@ enum {
#define VTD_FLAG_TRANS_PRE_ENABLED (1 << 0)
#define VTD_FLAG_IRQ_REMAP_PRE_ENABLED (1 << 1)
+#define VTD_FLAG_SVM_CAPABLE (1 << 2)
extern int intel_iommu_sm;
--
2.7.4
Hi Jacob,
On 11/18/19 8:42 PM, Jacob Pan wrote:
> The current code checks CPU and IOMMU feature set for SVM support but
> the result is never stored nor used. Therefore, SVM can still be used
> even when these checks failed.
"SVM can still be used even when these checks failed". What were the
consequences if it happened? Does it fix this cleanly now.
>
> This patch consolidates code for checking PASID, CPU vs. IOMMU paging
> mode compatibility, as well as provides specific error messages for
> each failed checks.>
> Signed-off-by: Jacob Pan <[email protected]>
> Acked-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/intel-iommu.c | 10 ++--------
> drivers/iommu/intel-svm.c | 40 +++++++++++++++++++++++++++-------------
> include/linux/intel-iommu.h | 4 +++-
> 3 files changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3f974919d3bd..d598168e410d 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3289,10 +3289,7 @@ static int __init init_dmars(void)
>
> if (!ecap_pass_through(iommu->ecap))
> hw_pass_through = 0;
> -#ifdef CONFIG_INTEL_IOMMU_SVM
> - if (pasid_supported(iommu))
> - intel_svm_init(iommu);
> -#endif
> + intel_svm_check(iommu);
> }
>
> /*
> @@ -4471,10 +4468,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
> if (ret)
> goto out;
>
> -#ifdef CONFIG_INTEL_IOMMU_SVM
> - if (pasid_supported(iommu))
> - intel_svm_init(iommu);
> -#endif
> + intel_svm_check(iommu);
>
> if (dmaru->ignored) {
> /*
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 9b159132405d..716c543488f6 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -23,19 +23,6 @@
>
> static irqreturn_t prq_event_thread(int irq, void *d);
>
> -int intel_svm_init(struct intel_iommu *iommu)
> -{
> - if (cpu_feature_enabled(X86_FEATURE_GBPAGES) &&
> - !cap_fl1gp_support(iommu->cap))
> - return -EINVAL;
> -
> - if (cpu_feature_enabled(X86_FEATURE_LA57) &&
> - !cap_5lp_support(iommu->cap))
> - return -EINVAL;
> -
> - return 0;
> -}
> -
> #define PRQ_ORDER 0
>
> int intel_svm_enable_prq(struct intel_iommu *iommu)
> @@ -99,6 +86,33 @@ int intel_svm_finish_prq(struct intel_iommu *iommu)
> return 0;
> }
>
> +static inline bool intel_svm_capable(struct intel_iommu *iommu)
> +{
> + return iommu->flags & VTD_FLAG_SVM_CAPABLE;
> +}
> +
> +void intel_svm_check(struct intel_iommu *iommu)
> +{
> + if (!pasid_supported(iommu))
> + return;
> +
> + if (cpu_feature_enabled(X86_FEATURE_GBPAGES) &&
> + !cap_fl1gp_support(iommu->cap)) {
> + pr_err("%s SVM disabled, incompatible 1GB page capability\n",
> + iommu->name);
nit: is it really an error or just a warning?
> + return;
> + }
> +
> + if (cpu_feature_enabled(X86_FEATURE_LA57) &&
> + !cap_5lp_support(iommu->cap)) {
> + pr_err("%s SVM disabled, incompatible paging mode\n",
> + iommu->name);
> + return;
> + }
> +
> + iommu->flags |= VTD_FLAG_SVM_CAPABLE;
> +}
> +
> static void intel_flush_svm_range_dev (struct intel_svm *svm, struct intel_svm_dev *sdev,
> unsigned long address, unsigned long pages, int ih)
> {
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 63118991824c..7dcfa1c4a844 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -657,7 +657,7 @@ void iommu_flush_write_buffer(struct intel_iommu *iommu);
> int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev);
>
> #ifdef CONFIG_INTEL_IOMMU_SVM
> -int intel_svm_init(struct intel_iommu *iommu);
> +extern void intel_svm_check(struct intel_iommu *iommu);
> extern int intel_svm_enable_prq(struct intel_iommu *iommu);
> extern int intel_svm_finish_prq(struct intel_iommu *iommu);
>
> @@ -685,6 +685,8 @@ struct intel_svm {
> };
>
> extern struct intel_iommu *intel_svm_device_to_iommu(struct device *dev);
> +#else
> +static inline void intel_svm_check(struct intel_iommu *iommu) {}
> #endif
>
> #ifdef CONFIG_INTEL_IOMMU_DEBUGFS
>
Besides,
Reviewed-by: Eric Auger <[email protected]>
Thanks
Eric
Hi Jacob,
On 11/18/19 8:42 PM, Jacob Pan wrote:
> Shared Virtual Memory(SVM) is based on a collective set of hardware
> features detected at runtime. There are requirements for matching CPU
> and IOMMU capabilities.
>
> This patch introduces a flag which will be used to mark and test the
> capability of SVM.
>
> Signed-off-by: Jacob Pan <[email protected]>
> Acked-by: Lu Baolu <[email protected]>
> ---
> include/linux/intel-iommu.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index ed11ef594378..63118991824c 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -433,6 +433,7 @@ enum {
>
> #define VTD_FLAG_TRANS_PRE_ENABLED (1 << 0)
> #define VTD_FLAG_IRQ_REMAP_PRE_ENABLED (1 << 1)
> +#define VTD_FLAG_SVM_CAPABLE (1 << 2)
I think I would rather squash this into the next patch as there is no
user here.
Thanks
Eric
>
> extern int intel_iommu_sm;
>
>
Hi Jacob,
On 11/18/19 8:42 PM, Jacob Pan wrote:
> When setting up first level page tables for sharing with CPU, we need
> to ensure IOMMU can support no less than the levels supported by the
> CPU.
> It is not adequate, as in the current code, to set up 5-level paging
> in PASID entry First Level Paging Mode(FLPM) solely based on CPU.
>
> Fixes: 437f35e1cd4c8 ("iommu/vt-d: Add first level page table
> interface")
> Signed-off-by: Jacob Pan <[email protected]>
> Acked-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/intel-pasid.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
> index 040a445be300..e7cb0b8a7332 100644
> --- a/drivers/iommu/intel-pasid.c
> +++ b/drivers/iommu/intel-pasid.c
> @@ -499,8 +499,16 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
> }
>
> #ifdef CONFIG_X86
> - if (cpu_feature_enabled(X86_FEATURE_LA57))
> - pasid_set_flpm(pte, 1);
> + /* Both CPU and IOMMU paging mode need to match */
> + if (cpu_feature_enabled(X86_FEATURE_LA57)) {
> + if (cap_5lp_support(iommu->cap)) {
> + pasid_set_flpm(pte, 1);
> + } else {
> + pr_err("VT-d has no 5-level paging support for CPU\n");
> + pasid_clear_entry(pte);
> + return -EINVAL;
Can it happen? If I am not wrong intel_pasid_setup_first_level() only
seems to be called from intel_svm_bind_mm which now checks the
SVM_CAPABLE flag.
Thanks
Eric
> + }
> + }
> #endif /* CONFIG_X86 */
>
> pasid_set_domain_id(pte, did);
>
On Mon, 18 Nov 2019 21:33:34 +0100
Auger Eric <[email protected]> wrote:
> Hi Jacob,
>
> On 11/18/19 8:42 PM, Jacob Pan wrote:
> > The current code checks CPU and IOMMU feature set for SVM support
> > but the result is never stored nor used. Therefore, SVM can still
> > be used even when these checks failed.
> "SVM can still be used even when these checks failed". What were the
> consequences if it happened? Does it fix this cleanly now.
> >
The consequence is DMA cannot reach above 48-bit virtual address range
when CPU does 5-level and IOMMU can only do 4-level. With is fix,
svm_bind_mm will fail in the first place to prevent SVM use by DMA.
> > This patch consolidates code for checking PASID, CPU vs. IOMMU
> > paging mode compatibility, as well as provides specific error
> > messages for each failed checks.>
> > Signed-off-by: Jacob Pan <[email protected]>
> > Acked-by: Lu Baolu <[email protected]>
> > ---
> > drivers/iommu/intel-iommu.c | 10 ++--------
> > drivers/iommu/intel-svm.c | 40
> > +++++++++++++++++++++++++++-------------
> > include/linux/intel-iommu.h | 4 +++- 3 files changed, 32
> > insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/iommu/intel-iommu.c
> > b/drivers/iommu/intel-iommu.c index 3f974919d3bd..d598168e410d
> > 100644 --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -3289,10 +3289,7 @@ static int __init init_dmars(void)
> >
> > if (!ecap_pass_through(iommu->ecap))
> > hw_pass_through = 0;
> > -#ifdef CONFIG_INTEL_IOMMU_SVM
> > - if (pasid_supported(iommu))
> > - intel_svm_init(iommu);
> > -#endif
> > + intel_svm_check(iommu);
> > }
> >
> > /*
> > @@ -4471,10 +4468,7 @@ static int intel_iommu_add(struct
> > dmar_drhd_unit *dmaru) if (ret)
> > goto out;
> >
> > -#ifdef CONFIG_INTEL_IOMMU_SVM
> > - if (pasid_supported(iommu))
> > - intel_svm_init(iommu);
> > -#endif
> > + intel_svm_check(iommu);
> >
> > if (dmaru->ignored) {
> > /*
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index 9b159132405d..716c543488f6 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -23,19 +23,6 @@
> >
> > static irqreturn_t prq_event_thread(int irq, void *d);
> >
> > -int intel_svm_init(struct intel_iommu *iommu)
> > -{
> > - if (cpu_feature_enabled(X86_FEATURE_GBPAGES) &&
> > - !cap_fl1gp_support(iommu->cap))
> > - return -EINVAL;
> > -
> > - if (cpu_feature_enabled(X86_FEATURE_LA57) &&
> > - !cap_5lp_support(iommu->cap))
> > - return -EINVAL;
> > -
> > - return 0;
> > -}
> > -
> > #define PRQ_ORDER 0
> >
> > int intel_svm_enable_prq(struct intel_iommu *iommu)
> > @@ -99,6 +86,33 @@ int intel_svm_finish_prq(struct intel_iommu
> > *iommu) return 0;
> > }
> >
> > +static inline bool intel_svm_capable(struct intel_iommu *iommu)
> > +{
> > + return iommu->flags & VTD_FLAG_SVM_CAPABLE;
> > +}
> > +
> > +void intel_svm_check(struct intel_iommu *iommu)
> > +{
> > + if (!pasid_supported(iommu))
> > + return;
> > +
> > + if (cpu_feature_enabled(X86_FEATURE_GBPAGES) &&
> > + !cap_fl1gp_support(iommu->cap)) {
> > + pr_err("%s SVM disabled, incompatible 1GB page
> > capability\n",
> > + iommu->name);
> nit: is it really an error or just a warning?
I think it is an error in that there is an illegal configuration. It is
mostly for vIOMMU, we expect native HW should have these features
matched.
> > + return;
> > + }
> > +
> > + if (cpu_feature_enabled(X86_FEATURE_LA57) &&
> > + !cap_5lp_support(iommu->cap)) {
> > + pr_err("%s SVM disabled, incompatible paging
> > mode\n",
> > + iommu->name);
> > + return;
> > + }
> > +
> > + iommu->flags |= VTD_FLAG_SVM_CAPABLE;
> > +}
> > +
> > static void intel_flush_svm_range_dev (struct intel_svm *svm,
> > struct intel_svm_dev *sdev, unsigned long address, unsigned long
> > pages, int ih) {
> > diff --git a/include/linux/intel-iommu.h
> > b/include/linux/intel-iommu.h index 63118991824c..7dcfa1c4a844
> > 100644 --- a/include/linux/intel-iommu.h
> > +++ b/include/linux/intel-iommu.h
> > @@ -657,7 +657,7 @@ void iommu_flush_write_buffer(struct
> > intel_iommu *iommu); int intel_iommu_enable_pasid(struct
> > intel_iommu *iommu, struct device *dev);
> > #ifdef CONFIG_INTEL_IOMMU_SVM
> > -int intel_svm_init(struct intel_iommu *iommu);
> > +extern void intel_svm_check(struct intel_iommu *iommu);
> > extern int intel_svm_enable_prq(struct intel_iommu *iommu);
> > extern int intel_svm_finish_prq(struct intel_iommu *iommu);
> >
> > @@ -685,6 +685,8 @@ struct intel_svm {
> > };
> >
> > extern struct intel_iommu *intel_svm_device_to_iommu(struct device
> > *dev); +#else
> > +static inline void intel_svm_check(struct intel_iommu *iommu) {}
> > #endif
> >
> > #ifdef CONFIG_INTEL_IOMMU_DEBUGFS
> >
> Besides,
> Reviewed-by: Eric Auger <[email protected]>
>
> Thanks
>
> Eric
>
[Jacob Pan]
On Mon, 18 Nov 2019 21:33:53 +0100
Auger Eric <[email protected]> wrote:
> Hi Jacob,
>
> On 11/18/19 8:42 PM, Jacob Pan wrote:
> > Shared Virtual Memory(SVM) is based on a collective set of hardware
> > features detected at runtime. There are requirements for matching
> > CPU and IOMMU capabilities.
> >
> > This patch introduces a flag which will be used to mark and test the
> > capability of SVM.
> >
> > Signed-off-by: Jacob Pan <[email protected]>
> > Acked-by: Lu Baolu <[email protected]>
> > ---
> > include/linux/intel-iommu.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/intel-iommu.h
> > b/include/linux/intel-iommu.h index ed11ef594378..63118991824c
> > 100644 --- a/include/linux/intel-iommu.h
> > +++ b/include/linux/intel-iommu.h
> > @@ -433,6 +433,7 @@ enum {
> >
> > #define VTD_FLAG_TRANS_PRE_ENABLED (1 << 0)
> > #define VTD_FLAG_IRQ_REMAP_PRE_ENABLED (1 << 1)
> > +#define VTD_FLAG_SVM_CAPABLE (1 << 2)
>
> I think I would rather squash this into the next patch as there is no
> user here.
>
Sure, I don't have strong preference. Baolu, what is your call?
> Thanks
>
> Eric
> >
> > extern int intel_iommu_sm;
> >
> >
>
[Jacob Pan]
On Mon, 18 Nov 2019 21:55:03 +0100
Auger Eric <[email protected]> wrote:
> Hi Jacob,
>
> On 11/18/19 8:42 PM, Jacob Pan wrote:
> > When setting up first level page tables for sharing with CPU, we
> > need to ensure IOMMU can support no less than the levels supported
> > by the CPU.
> > It is not adequate, as in the current code, to set up 5-level paging
> > in PASID entry First Level Paging Mode(FLPM) solely based on CPU.
> >
> > Fixes: 437f35e1cd4c8 ("iommu/vt-d: Add first level page table
> > interface")
> > Signed-off-by: Jacob Pan <[email protected]>
> > Acked-by: Lu Baolu <[email protected]>
> > ---
> > drivers/iommu/intel-pasid.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/intel-pasid.c
> > b/drivers/iommu/intel-pasid.c index 040a445be300..e7cb0b8a7332
> > 100644 --- a/drivers/iommu/intel-pasid.c
> > +++ b/drivers/iommu/intel-pasid.c
> > @@ -499,8 +499,16 @@ int intel_pasid_setup_first_level(struct
> > intel_iommu *iommu, }
> >
> > #ifdef CONFIG_X86
> > - if (cpu_feature_enabled(X86_FEATURE_LA57))
> > - pasid_set_flpm(pte, 1);
> > + /* Both CPU and IOMMU paging mode need to match */
> > + if (cpu_feature_enabled(X86_FEATURE_LA57)) {
> > + if (cap_5lp_support(iommu->cap)) {
> > + pasid_set_flpm(pte, 1);
> > + } else {
> > + pr_err("VT-d has no 5-level paging support
> > for CPU\n");
> > + pasid_clear_entry(pte);
> > + return -EINVAL;
> Can it happen? If I am not wrong intel_pasid_setup_first_level() only
> seems to be called from intel_svm_bind_mm which now checks the
> SVM_CAPABLE flag.
>
You are right, this check is not needed any more. I will drop the patch.
> Thanks
>
> Eric
> > + }
> > + }
> > #endif /* CONFIG_X86 */
> >
> > pasid_set_domain_id(pte, did);
> >
>
[Jacob Pan]
Hi,
On 11/19/19 5:48 AM, Jacob Pan wrote:
> On Mon, 18 Nov 2019 21:33:53 +0100
> Auger Eric <[email protected]> wrote:
>
>> Hi Jacob,
>>
>> On 11/18/19 8:42 PM, Jacob Pan wrote:
>>> Shared Virtual Memory(SVM) is based on a collective set of hardware
>>> features detected at runtime. There are requirements for matching
>>> CPU and IOMMU capabilities.
>>>
>>> This patch introduces a flag which will be used to mark and test the
>>> capability of SVM.
>>>
>>> Signed-off-by: Jacob Pan <[email protected]>
>>> Acked-by: Lu Baolu <[email protected]>
>>> ---
>>> include/linux/intel-iommu.h | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/linux/intel-iommu.h
>>> b/include/linux/intel-iommu.h index ed11ef594378..63118991824c
>>> 100644 --- a/include/linux/intel-iommu.h
>>> +++ b/include/linux/intel-iommu.h
>>> @@ -433,6 +433,7 @@ enum {
>>>
>>> #define VTD_FLAG_TRANS_PRE_ENABLED (1 << 0)
>>> #define VTD_FLAG_IRQ_REMAP_PRE_ENABLED (1 << 1)
>>> +#define VTD_FLAG_SVM_CAPABLE (1 << 2)
>>
>> I think I would rather squash this into the next patch as there is no
>> user here.
>>
> Sure, I don't have strong preference. Baolu, what is your call?
It's okay for me.
Best regards,
baolu
Hi Eric and Jacob,
On 11/19/19 5:52 AM, Jacob Pan wrote:
> On Mon, 18 Nov 2019 21:55:03 +0100
> Auger Eric <[email protected]> wrote:
>
>> Hi Jacob,
>>
>> On 11/18/19 8:42 PM, Jacob Pan wrote:
>>> When setting up first level page tables for sharing with CPU, we
>>> need to ensure IOMMU can support no less than the levels supported
>>> by the CPU.
>>> It is not adequate, as in the current code, to set up 5-level paging
>>> in PASID entry First Level Paging Mode(FLPM) solely based on CPU.
>>>
>>> Fixes: 437f35e1cd4c8 ("iommu/vt-d: Add first level page table
>>> interface")
>>> Signed-off-by: Jacob Pan <[email protected]>
>>> Acked-by: Lu Baolu <[email protected]>
>>> ---
>>> drivers/iommu/intel-pasid.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel-pasid.c
>>> b/drivers/iommu/intel-pasid.c index 040a445be300..e7cb0b8a7332
>>> 100644 --- a/drivers/iommu/intel-pasid.c
>>> +++ b/drivers/iommu/intel-pasid.c
>>> @@ -499,8 +499,16 @@ int intel_pasid_setup_first_level(struct
>>> intel_iommu *iommu, }
>>>
>>> #ifdef CONFIG_X86
>>> - if (cpu_feature_enabled(X86_FEATURE_LA57))
>>> - pasid_set_flpm(pte, 1);
>>> + /* Both CPU and IOMMU paging mode need to match */
>>> + if (cpu_feature_enabled(X86_FEATURE_LA57)) {
>>> + if (cap_5lp_support(iommu->cap)) {
>>> + pasid_set_flpm(pte, 1);
>>> + } else {
>>> + pr_err("VT-d has no 5-level paging support
>>> for CPU\n");
>>> + pasid_clear_entry(pte);
>>> + return -EINVAL;
>> Can it happen? If I am not wrong intel_pasid_setup_first_level() only
>> seems to be called from intel_svm_bind_mm which now checks the
>> SVM_CAPABLE flag.
>>
> You are right, this check is not needed any more. I will drop the patch.
>> Thanks
I'd suggest to keep this. This helper is not only for svm, although
currently svm is the only caller. For first level pasid setup, let's
set an assumption that hardware should never report mismatching paging
modes, this is helpful especially when running vIOMMU in VM guests.
Best regards,
baolu
Hi Jacob,
On 11/18/19 10:47 PM, Jacob Pan wrote:
> On Mon, 18 Nov 2019 21:33:34 +0100
> Auger Eric <[email protected]> wrote:
>
>> Hi Jacob,
>>
>> On 11/18/19 8:42 PM, Jacob Pan wrote:
>>> The current code checks CPU and IOMMU feature set for SVM support
>>> but the result is never stored nor used. Therefore, SVM can still
>>> be used even when these checks failed.
>> "SVM can still be used even when these checks failed". What were the
>> consequences if it happened? Does it fix this cleanly now.
>>>
> The consequence is DMA cannot reach above 48-bit virtual address range
> when CPU does 5-level and IOMMU can only do 4-level. With is fix,
> svm_bind_mm will fail in the first place to prevent SVM use by DMA.
OK thank you for the clarification. Maybe this latter can be added in
the commit message
>
>>> This patch consolidates code for checking PASID, CPU vs. IOMMU
>>> paging mode compatibility, as well as provides specific error
>>> messages for each failed checks.>
>>> Signed-off-by: Jacob Pan <[email protected]>
>>> Acked-by: Lu Baolu <[email protected]>
>>> ---
>>> drivers/iommu/intel-iommu.c | 10 ++--------
>>> drivers/iommu/intel-svm.c | 40
>>> +++++++++++++++++++++++++++-------------
>>> include/linux/intel-iommu.h | 4 +++- 3 files changed, 32
>>> insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel-iommu.c
>>> b/drivers/iommu/intel-iommu.c index 3f974919d3bd..d598168e410d
>>> 100644 --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -3289,10 +3289,7 @@ static int __init init_dmars(void)
>>>
>>> if (!ecap_pass_through(iommu->ecap))
>>> hw_pass_through = 0;
>>> -#ifdef CONFIG_INTEL_IOMMU_SVM
>>> - if (pasid_supported(iommu))
>>> - intel_svm_init(iommu);
>>> -#endif
>>> + intel_svm_check(iommu);
>>> }
>>>
>>> /*
>>> @@ -4471,10 +4468,7 @@ static int intel_iommu_add(struct
>>> dmar_drhd_unit *dmaru) if (ret)
>>> goto out;
>>>
>>> -#ifdef CONFIG_INTEL_IOMMU_SVM
>>> - if (pasid_supported(iommu))
>>> - intel_svm_init(iommu);
>>> -#endif
>>> + intel_svm_check(iommu);
>>>
>>> if (dmaru->ignored) {
>>> /*
>>> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
>>> index 9b159132405d..716c543488f6 100644
>>> --- a/drivers/iommu/intel-svm.c
>>> +++ b/drivers/iommu/intel-svm.c
>>> @@ -23,19 +23,6 @@
>>>
>>> static irqreturn_t prq_event_thread(int irq, void *d);
>>>
>>> -int intel_svm_init(struct intel_iommu *iommu)
>>> -{
>>> - if (cpu_feature_enabled(X86_FEATURE_GBPAGES) &&
>>> - !cap_fl1gp_support(iommu->cap))
>>> - return -EINVAL;
>>> -
>>> - if (cpu_feature_enabled(X86_FEATURE_LA57) &&
>>> - !cap_5lp_support(iommu->cap))
>>> - return -EINVAL;
>>> -
>>> - return 0;
>>> -}
>>> -
>>> #define PRQ_ORDER 0
>>>
>>> int intel_svm_enable_prq(struct intel_iommu *iommu)
>>> @@ -99,6 +86,33 @@ int intel_svm_finish_prq(struct intel_iommu
>>> *iommu) return 0;
>>> }
>>>
>>> +static inline bool intel_svm_capable(struct intel_iommu *iommu)
>>> +{
>>> + return iommu->flags & VTD_FLAG_SVM_CAPABLE;
>>> +}
>>> +
>>> +void intel_svm_check(struct intel_iommu *iommu)
>>> +{
>>> + if (!pasid_supported(iommu))
>>> + return;
>>> +
>>> + if (cpu_feature_enabled(X86_FEATURE_GBPAGES) &&
>>> + !cap_fl1gp_support(iommu->cap)) {
>>> + pr_err("%s SVM disabled, incompatible 1GB page
>>> capability\n",
>>> + iommu->name);
>> nit: is it really an error or just a warning?
> I think it is an error in that there is an illegal configuration. It is
> mostly for vIOMMU, we expect native HW should have these features
> matched.
OK
Thanks
Eric
>
>>> + return;
>>> + }
>>> +
>>> + if (cpu_feature_enabled(X86_FEATURE_LA57) &&
>>> + !cap_5lp_support(iommu->cap)) {
>>> + pr_err("%s SVM disabled, incompatible paging
>>> mode\n",
>>> + iommu->name);
>>> + return;
>>> + }
>>> +
>>> + iommu->flags |= VTD_FLAG_SVM_CAPABLE;
>>> +}
>>> +
>>> static void intel_flush_svm_range_dev (struct intel_svm *svm,
>>> struct intel_svm_dev *sdev, unsigned long address, unsigned long
>>> pages, int ih) {
>>> diff --git a/include/linux/intel-iommu.h
>>> b/include/linux/intel-iommu.h index 63118991824c..7dcfa1c4a844
>>> 100644 --- a/include/linux/intel-iommu.h
>>> +++ b/include/linux/intel-iommu.h
>>> @@ -657,7 +657,7 @@ void iommu_flush_write_buffer(struct
>>> intel_iommu *iommu); int intel_iommu_enable_pasid(struct
>>> intel_iommu *iommu, struct device *dev);
>>> #ifdef CONFIG_INTEL_IOMMU_SVM
>>> -int intel_svm_init(struct intel_iommu *iommu);
>>> +extern void intel_svm_check(struct intel_iommu *iommu);
>>> extern int intel_svm_enable_prq(struct intel_iommu *iommu);
>>> extern int intel_svm_finish_prq(struct intel_iommu *iommu);
>>>
>>> @@ -685,6 +685,8 @@ struct intel_svm {
>>> };
>>>
>>> extern struct intel_iommu *intel_svm_device_to_iommu(struct device
>>> *dev); +#else
>>> +static inline void intel_svm_check(struct intel_iommu *iommu) {}
>>> #endif
>>>
>>> #ifdef CONFIG_INTEL_IOMMU_DEBUGFS
>>>
>> Besides,
>> Reviewed-by: Eric Auger <[email protected]>
>>
>> Thanks
>>
>> Eric
>>
>
> [Jacob Pan]
>
Hi Lu, Jacob,
On 11/19/19 4:06 AM, Lu Baolu wrote:
> Hi Eric and Jacob,
>
> On 11/19/19 5:52 AM, Jacob Pan wrote:
>> On Mon, 18 Nov 2019 21:55:03 +0100
>> Auger Eric <[email protected]> wrote:
>>
>>> Hi Jacob,
>>>
>>> On 11/18/19 8:42 PM, Jacob Pan wrote:
>>>> When setting up first level page tables for sharing with CPU, we
>>>> need to ensure IOMMU can support no less than the levels supported
>>>> by the CPU.
>>>> It is not adequate, as in the current code, to set up 5-level paging
>>>> in PASID entry First Level Paging Mode(FLPM) solely based on CPU.
>>>>
>>>> Fixes: 437f35e1cd4c8 ("iommu/vt-d: Add first level page table
>>>> interface")
>>>> Signed-off-by: Jacob Pan <[email protected]>
>>>> Acked-by: Lu Baolu <[email protected]>
>>>> ---
>>>> drivers/iommu/intel-pasid.c | 12 ++++++++++--
>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/intel-pasid.c
>>>> b/drivers/iommu/intel-pasid.c index 040a445be300..e7cb0b8a7332
>>>> 100644 --- a/drivers/iommu/intel-pasid.c
>>>> +++ b/drivers/iommu/intel-pasid.c
>>>> @@ -499,8 +499,16 @@ int intel_pasid_setup_first_level(struct
>>>> intel_iommu *iommu, }
>>>> #ifdef CONFIG_X86
>>>> - if (cpu_feature_enabled(X86_FEATURE_LA57))
>>>> - pasid_set_flpm(pte, 1);
>>>> + /* Both CPU and IOMMU paging mode need to match */
>>>> + if (cpu_feature_enabled(X86_FEATURE_LA57)) {
>>>> + if (cap_5lp_support(iommu->cap)) {
>>>> + pasid_set_flpm(pte, 1);
>>>> + } else {
>>>> + pr_err("VT-d has no 5-level paging support
>>>> for CPU\n");
>>>> + pasid_clear_entry(pte);
>>>> + return -EINVAL;
>>> Can it happen? If I am not wrong intel_pasid_setup_first_level() only
>>> seems to be called from intel_svm_bind_mm which now checks the
>>> SVM_CAPABLE flag.
>>>
>> You are right, this check is not needed any more. I will drop the patch.
>>> Thanks
>
> I'd suggest to keep this. This helper is not only for svm, although
> currently svm is the only caller. For first level pasid setup, let's
> set an assumption that hardware should never report mismatching paging
> modes, this is helpful especially when running vIOMMU in VM guests.
OK. So maybe just add the rationale in the commit message?
Thanks
Eric
>
> Best regards,
> baolu
>
On Tue, 19 Nov 2019 10:55:29 +0800
Lu Baolu <[email protected]> wrote:
> Hi,
>
> On 11/19/19 5:48 AM, Jacob Pan wrote:
> > On Mon, 18 Nov 2019 21:33:53 +0100
> > Auger Eric <[email protected]> wrote:
> >
> >> Hi Jacob,
> >>
> >> On 11/18/19 8:42 PM, Jacob Pan wrote:
> >>> Shared Virtual Memory(SVM) is based on a collective set of
> >>> hardware features detected at runtime. There are requirements for
> >>> matching CPU and IOMMU capabilities.
> >>>
> >>> This patch introduces a flag which will be used to mark and test
> >>> the capability of SVM.
> >>>
> >>> Signed-off-by: Jacob Pan <[email protected]>
> >>> Acked-by: Lu Baolu <[email protected]>
> >>> ---
> >>> include/linux/intel-iommu.h | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/include/linux/intel-iommu.h
> >>> b/include/linux/intel-iommu.h index ed11ef594378..63118991824c
> >>> 100644 --- a/include/linux/intel-iommu.h
> >>> +++ b/include/linux/intel-iommu.h
> >>> @@ -433,6 +433,7 @@ enum {
> >>>
> >>> #define VTD_FLAG_TRANS_PRE_ENABLED (1 << 0)
> >>> #define VTD_FLAG_IRQ_REMAP_PRE_ENABLED (1 << 1)
> >>> +#define VTD_FLAG_SVM_CAPABLE (1 << 2)
> >>
> >> I think I would rather squash this into the next patch as there is
> >> no user here.
> >>
> > Sure, I don't have strong preference. Baolu, what is your call?
>
> It's okay for me.
>
will squash then.
Thanks!
> Best regards,
> baolu
[Jacob Pan]
On Tue, 19 Nov 2019 09:04:10 +0100
Auger Eric <[email protected]> wrote:
> Hi Lu, Jacob,
>
> On 11/19/19 4:06 AM, Lu Baolu wrote:
> > Hi Eric and Jacob,
> >
> > On 11/19/19 5:52 AM, Jacob Pan wrote:
> >> On Mon, 18 Nov 2019 21:55:03 +0100
> >> Auger Eric <[email protected]> wrote:
> >>
> >>> Hi Jacob,
> >>>
> >>> On 11/18/19 8:42 PM, Jacob Pan wrote:
> >>>> When setting up first level page tables for sharing with CPU, we
> >>>> need to ensure IOMMU can support no less than the levels
> >>>> supported by the CPU.
> >>>> It is not adequate, as in the current code, to set up 5-level
> >>>> paging in PASID entry First Level Paging Mode(FLPM) solely based
> >>>> on CPU.
> >>>>
> >>>> Fixes: 437f35e1cd4c8 ("iommu/vt-d: Add first level page table
> >>>> interface")
> >>>> Signed-off-by: Jacob Pan <[email protected]>
> >>>> Acked-by: Lu Baolu <[email protected]>
> >>>> ---
> >>>> drivers/iommu/intel-pasid.c | 12 ++++++++++--
> >>>> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iommu/intel-pasid.c
> >>>> b/drivers/iommu/intel-pasid.c index 040a445be300..e7cb0b8a7332
> >>>> 100644 --- a/drivers/iommu/intel-pasid.c
> >>>> +++ b/drivers/iommu/intel-pasid.c
> >>>> @@ -499,8 +499,16 @@ int intel_pasid_setup_first_level(struct
> >>>> intel_iommu *iommu, }
> >>>> #ifdef CONFIG_X86
> >>>> - if (cpu_feature_enabled(X86_FEATURE_LA57))
> >>>> - pasid_set_flpm(pte, 1);
> >>>> + /* Both CPU and IOMMU paging mode need to match */
> >>>> + if (cpu_feature_enabled(X86_FEATURE_LA57)) {
> >>>> + if (cap_5lp_support(iommu->cap)) {
> >>>> + pasid_set_flpm(pte, 1);
> >>>> + } else {
> >>>> + pr_err("VT-d has no 5-level paging support
> >>>> for CPU\n");
> >>>> + pasid_clear_entry(pte);
> >>>> + return -EINVAL;
> >>> Can it happen? If I am not wrong intel_pasid_setup_first_level()
> >>> only seems to be called from intel_svm_bind_mm which now checks
> >>> the SVM_CAPABLE flag.
> >>>
> >> You are right, this check is not needed any more. I will drop the
> >> patch.
> >>> Thanks
> >
> > I'd suggest to keep this. This helper is not only for svm, although
> > currently svm is the only caller. For first level pasid setup, let's
> > set an assumption that hardware should never report mismatching
> > paging modes, this is helpful especially when running vIOMMU in VM
> > guests.
>
> OK. So maybe just add the rationale in the commit message?
>
OK. will do. I thought about Baolu's point as well then I thought the
other use of first level map for replacing today's VFIO second level
map in the guest, but that will have a different helper. Anyway, I will
keep this one then.
Thanks you both!
> Thanks
>
> Eric
> >
> > Best regards,
> > baolu
> >
>
[Jacob Pan]
On Tue, 19 Nov 2019 09:02:26 +0100
Auger Eric <[email protected]> wrote:
> Hi Jacob,
>
> On 11/18/19 10:47 PM, Jacob Pan wrote:
> > On Mon, 18 Nov 2019 21:33:34 +0100
> > Auger Eric <[email protected]> wrote:
> >
> >> Hi Jacob,
> >>
> >> On 11/18/19 8:42 PM, Jacob Pan wrote:
> >>> The current code checks CPU and IOMMU feature set for SVM support
> >>> but the result is never stored nor used. Therefore, SVM can still
> >>> be used even when these checks failed.
> >> "SVM can still be used even when these checks failed". What were
> >> the consequences if it happened? Does it fix this cleanly now.
> >>>
> > The consequence is DMA cannot reach above 48-bit virtual address
> > range when CPU does 5-level and IOMMU can only do 4-level. With is
> > fix, svm_bind_mm will fail in the first place to prevent SVM use by
> > DMA.
> OK thank you for the clarification. Maybe this latter can be added in
> the commit message
> >
Yes, will do.
> [...]
> >> nit: is it really an error or just a warning?
> > I think it is an error in that there is an illegal configuration.
> > It is mostly for vIOMMU, we expect native HW should have these
> > features matched.
>
> OK
>
> Thanks
>
> Eric
> >
> [...]
> >> Besides,
> >> Reviewed-by: Eric Auger <[email protected]>
> >>
> >> Thanks
> >>
> >> Eric
> >>
> >
> > [Jacob Pan]
> >
>
[Jacob Pan]