linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes
@ 2019-11-18 19:42 Jacob Pan
  2019-11-18 19:42 ` [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag Jacob Pan
                   ` (9 more replies)
  0 siblings, 10 replies; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  2019-11-18 20:33   ` Auger Eric
  2019-11-18 19:42 ` [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks Jacob Pan
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

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 <jacob.jun.pan@linux.intel.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
  2019-11-18 19:42 ` [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  2019-11-18 20:33   ` Auger Eric
  2019-11-18 19:42 ` [PATCH v2 03/10] iommu/vt-d: Reject SVM bind for failed capability check Jacob Pan
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

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 <jacob.jun.pan@linux.intel.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 03/10] iommu/vt-d: Reject SVM bind for failed capability check
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
  2019-11-18 19:42 ` [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag Jacob Pan
  2019-11-18 19:42 ` [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  2019-11-18 20:55   ` Auger Eric
  2019-11-18 19:42 ` [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode Jacob Pan
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

Add a check during SVM bind to ensure CPU and IOMMU hardware capabilities
are met.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-svm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 716c543488f6..74df10a39dfc 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -238,6 +238,9 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 	if (!iommu || dmar_disabled)
 		return -EINVAL;
 
+	if (!intel_svm_capable(iommu))
+		return -ENOTSUPP;
+
 	if (dev_is_pci(dev)) {
 		pasid_max = pci_max_pasids(to_pci_dev(dev));
 		if (pasid_max < 0)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
                   ` (2 preceding siblings ...)
  2019-11-18 19:42 ` [PATCH v2 03/10] iommu/vt-d: Reject SVM bind for failed capability check Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  2019-11-18 20:55   ` Auger Eric
  2019-11-18 19:42 ` [PATCH v2 05/10] iommu/vt-d: Avoid duplicated code for PASID setup Jacob Pan
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

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 <jacob.jun.pan@linux.intel.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 05/10] iommu/vt-d: Avoid duplicated code for PASID setup
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
                   ` (3 preceding siblings ...)
  2019-11-18 19:42 ` [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  2019-11-18 20:59   ` Auger Eric
  2019-11-18 19:42 ` [PATCH v2 06/10] iommu/vt-d: Fix off-by-one in PASID allocation Jacob Pan
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

After each setup for PASID entry, related translation caches must be
flushed. We can combine duplicated code into one function which is less
error prone.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-pasid.c | 48 +++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
index e7cb0b8a7332..732bfee228df 100644
--- a/drivers/iommu/intel-pasid.c
+++ b/drivers/iommu/intel-pasid.c
@@ -465,6 +465,21 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
 }
 
+static void pasid_flush_caches(struct intel_iommu *iommu,
+				struct pasid_entry *pte,
+				int pasid, u16 did)
+{
+	if (!ecap_coherent(iommu->ecap))
+		clflush_cache_range(pte, sizeof(*pte));
+
+	if (cap_caching_mode(iommu->cap)) {
+		pasid_cache_invalidation_with_pasid(iommu, did, pasid);
+		iotlb_invalidation_with_pasid(iommu, did, pasid);
+	} else {
+		iommu_flush_write_buffer(iommu);
+	}
+}
+
 /*
  * Set up the scalable mode pasid table entry for first only
  * translation type.
@@ -518,16 +533,7 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 	/* Setup Present and PASID Granular Transfer Type: */
 	pasid_set_translation_type(pte, 1);
 	pasid_set_present(pte);
-
-	if (!ecap_coherent(iommu->ecap))
-		clflush_cache_range(pte, sizeof(*pte));
-
-	if (cap_caching_mode(iommu->cap)) {
-		pasid_cache_invalidation_with_pasid(iommu, did, pasid);
-		iotlb_invalidation_with_pasid(iommu, did, pasid);
-	} else {
-		iommu_flush_write_buffer(iommu);
-	}
+	pasid_flush_caches(iommu, pte, pasid, did);
 
 	return 0;
 }
@@ -591,16 +597,7 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 	 */
 	pasid_set_sre(pte);
 	pasid_set_present(pte);
-
-	if (!ecap_coherent(iommu->ecap))
-		clflush_cache_range(pte, sizeof(*pte));
-
-	if (cap_caching_mode(iommu->cap)) {
-		pasid_cache_invalidation_with_pasid(iommu, did, pasid);
-		iotlb_invalidation_with_pasid(iommu, did, pasid);
-	} else {
-		iommu_flush_write_buffer(iommu);
-	}
+	pasid_flush_caches(iommu, pte, pasid, did);
 
 	return 0;
 }
@@ -634,16 +631,7 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 	 */
 	pasid_set_sre(pte);
 	pasid_set_present(pte);
-
-	if (!ecap_coherent(iommu->ecap))
-		clflush_cache_range(pte, sizeof(*pte));
-
-	if (cap_caching_mode(iommu->cap)) {
-		pasid_cache_invalidation_with_pasid(iommu, did, pasid);
-		iotlb_invalidation_with_pasid(iommu, did, pasid);
-	} else {
-		iommu_flush_write_buffer(iommu);
-	}
+	pasid_flush_caches(iommu, pte, pasid, did);
 
 	return 0;
 }
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 06/10] iommu/vt-d: Fix off-by-one in PASID allocation
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
                   ` (4 preceding siblings ...)
  2019-11-18 19:42 ` [PATCH v2 05/10] iommu/vt-d: Avoid duplicated code for PASID setup Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  2019-11-18 21:00   ` Auger Eric
  2019-11-18 19:42 ` [PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID Jacob Pan
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

PASID allocator uses IDR which is exclusive for the end of the
allocation range. There is no need to decrement pasid_max.

Fixes: af39507305fb ("iommu/vt-d: Apply global PASID in SVA")
Reported-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 74df10a39dfc..e90d0b914afe 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -338,7 +338,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 		/* 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);
+					   pasid_max, GFP_KERNEL);
 		if (ret < 0) {
 			kfree(svm);
 			kfree(sdev);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
                   ` (5 preceding siblings ...)
  2019-11-18 19:42 ` [PATCH v2 06/10] iommu/vt-d: Fix off-by-one in PASID allocation Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  2019-11-18 21:11   ` Auger Eric
  2019-11-18 19:42 ` [PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush Jacob Pan
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

Make use of generic IOASID code to manage PASID allocation,
free, and lookup. Replace Intel specific code.
IOASID allocator is inclusive for both start and end of the allocation
range. The current code is based on IDR, which is exclusive for
the end of the allocation range. This patch fixes the off-by-one
error in intel_svm_bind_mm, where pasid_max - 1 is used for the end of
allocation range.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/Kconfig       |  1 +
 drivers/iommu/intel-iommu.c | 13 +++++++------
 drivers/iommu/intel-pasid.c | 36 ------------------------------------
 drivers/iommu/intel-svm.c   | 41 +++++++++++++++++++++++++++--------------
 4 files changed, 35 insertions(+), 56 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index fd50ddffffbf..43ce450a40d3 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -212,6 +212,7 @@ config INTEL_IOMMU_SVM
 	depends on INTEL_IOMMU && X86
 	select PCI_PASID
 	select MMU_NOTIFIER
+	select IOASID
 	help
 	  Shared Virtual Memory (SVM) provides a facility for devices
 	  to access DMA resources through process address space by
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index d598168e410d..a84f0caa33a0 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5238,7 +5238,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,
@@ -5256,10 +5256,11 @@ 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;
 		}
@@ -5295,7 +5296,7 @@ static int aux_domain_add_dev(struct dmar_domain *domain,
 	spin_unlock(&iommu->lock);
 	spin_unlock_irqrestore(&device_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 732bfee228df..3cb569e76642 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(&pasid_lock);
-	ret = idr_alloc(&pasid_idr, ptr, min, max, GFP_ATOMIC);
-	spin_unlock(&pasid_lock);
-	idr_preload_end();
-
-	return ret;
-}
-
-void intel_pasid_free_id(int pasid)
-{
-	spin_lock(&pasid_lock);
-	idr_remove(&pasid_idr, pasid);
-	spin_unlock(&pasid_lock);
-}
-
-void *intel_pasid_lookup_id(int pasid)
-{
-	void *p;
-
-	spin_lock(&pasid_lock);
-	p = idr_find(&pasid_idr, pasid);
-	spin_unlock(&pasid_lock);
-
-	return p;
-}
 
 /*
  * Per device pasid table management:
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index e90d0b914afe..26a2f57763ec 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -17,6 +17,7 @@
 #include <linux/dmar.h>
 #include <linux/interrupt.h>
 #include <linux/mm_types.h>
+#include <linux/ioasid.h>
 #include <asm/page.h>
 
 #include "intel-pasid.h"
@@ -335,16 +336,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, 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 = &intel_mmuops;
 		svm->mm = mm;
 		svm->flags = flags;
@@ -354,7 +354,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 		if (mm) {
 			ret = mmu_notifier_register(&svm->notifier, mm);
 			if (ret) {
-				intel_pasid_free_id(svm->pasid);
+				ioasid_free(svm->pasid);
 				kfree(svm);
 				kfree(sdev);
 				goto out;
@@ -370,7 +370,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 		if (ret) {
 			if (mm)
 				mmu_notifier_unregister(&svm->notifier, mm);
-			intel_pasid_free_id(svm->pasid);
+			ioasid_free(svm->pasid);
 			kfree(svm);
 			kfree(sdev);
 			goto out;
@@ -418,7 +418,15 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
 	if (!iommu)
 		goto out;
 
-	svm = intel_pasid_lookup_id(pasid);
+	svm = ioasid_find(NULL, pasid, NULL);
+	if (!svm)
+		goto out;
+
+	if (IS_ERR(svm)) {
+		ret = PTR_ERR(svm);
+		goto out;
+	}
+
 	if (!svm)
 		goto out;
 
@@ -440,7 +448,9 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
 				kfree_rcu(sdev, rcu);
 
 				if (list_empty(&svm->devs)) {
-					intel_pasid_free_id(svm->pasid);
+					/* 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);
 
@@ -475,10 +485,14 @@ int intel_svm_is_pasid_valid(struct device *dev, int pasid)
 	if (!iommu)
 		goto out;
 
-	svm = intel_pasid_lookup_id(pasid);
+	svm = ioasid_find(NULL, pasid, NULL);
 	if (!svm)
 		goto out;
 
+	if (IS_ERR(svm)) {
+		ret = PTR_ERR(svm);
+		goto out;
+	}
 	/* init_mm is used in this case */
 	if (!svm->mm)
 		ret = 1;
@@ -585,13 +599,12 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 
 		if (!svm || svm->pasid != req->pasid) {
 			rcu_read_lock();
-			svm = intel_pasid_lookup_id(req->pasid);
+			svm = ioasid_find(NULL, req->pasid, NULL);
 			/* It *can't* go away, because the driver is not permitted
 			 * to unbind the mm while any page faults are outstanding.
 			 * So we only need RCU to protect the internal idr code. */
 			rcu_read_unlock();
-
-			if (!svm) {
+			if (IS_ERR_OR_NULL(svm)) {
 				pr_err("%s: Page request for invalid PASID %d: %08llx %08llx\n",
 				       iommu->name, req->pasid, ((unsigned long long *)req)[0],
 				       ((unsigned long long *)req)[1]);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
                   ` (6 preceding siblings ...)
  2019-11-18 19:42 ` [PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  2019-11-18 21:19   ` Auger Eric
  2019-11-18 19:42 ` [PATCH v2 09/10] iommu/vt-d: Avoid sending invalid page response Jacob Pan
  2019-11-18 19:42 ` [PATCH v2 10/10] iommu/vt-d: Misc macro clean up for SVM Jacob Pan
  9 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

Use the correct invalidation descriptor type and granularity.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-pasid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
index 3cb569e76642..ee6ea1bbd917 100644
--- a/drivers/iommu/intel-pasid.c
+++ b/drivers/iommu/intel-pasid.c
@@ -365,7 +365,8 @@ pasid_cache_invalidation_with_pasid(struct intel_iommu *iommu,
 {
 	struct qi_desc desc;
 
-	desc.qw0 = QI_PC_DID(did) | QI_PC_PASID_SEL | QI_PC_PASID(pasid);
+	desc.qw0 = QI_PC_DID(did) | QI_PC_GRAN(QI_PC_PASID_SEL) |
+		QI_PC_PASID(pasid) | QI_PC_TYPE;
 	desc.qw1 = 0;
 	desc.qw2 = 0;
 	desc.qw3 = 0;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 09/10] iommu/vt-d: Avoid sending invalid page response
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
                   ` (7 preceding siblings ...)
  2019-11-18 19:42 ` [PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  2019-11-18 21:26   ` Auger Eric
  2019-11-18 19:42 ` [PATCH v2 10/10] iommu/vt-d: Misc macro clean up for SVM Jacob Pan
  9 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

Page responses should only be sent when last page in group (LPIG) or
private data is present in the page request. This patch avoids sending
invalid descriptors.

Fixes: 5d308fc1ecf53 ("iommu/vt-d: Add 256-bit invalidation descriptor
support")
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-svm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 26a2f57763ec..098c89246593 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -688,11 +688,10 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			if (req->priv_data_present)
 				memcpy(&resp.qw2, req->priv_data,
 				       sizeof(req->priv_data));
+			resp.qw2 = 0;
+			resp.qw3 = 0;
+			qi_submit_sync(&resp, iommu);
 		}
-		resp.qw2 = 0;
-		resp.qw3 = 0;
-		qi_submit_sync(&resp, iommu);
-
 		head = (head + sizeof(*req)) & PRQ_RING_MASK;
 	}
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 10/10] iommu/vt-d: Misc macro clean up for SVM
  2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
                   ` (8 preceding siblings ...)
  2019-11-18 19:42 ` [PATCH v2 09/10] iommu/vt-d: Avoid sending invalid page response Jacob Pan
@ 2019-11-18 19:42 ` Jacob Pan
  9 siblings, 0 replies; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 19:42 UTC (permalink / raw)
  To: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu, Eric Auger, Jacob Pan

Use combined macros for_each_svm_dev() to simplify SVM device iteration
and error checking.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks
  2019-11-18 19:42 ` [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks Jacob Pan
@ 2019-11-18 20:33   ` Auger Eric
  2019-11-18 21:47     ` Jacob Pan
  0 siblings, 1 reply; 32+ messages in thread
From: Auger Eric @ 2019-11-18 20:33 UTC (permalink / raw)
  To: Jacob Pan, iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu

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 <jacob.jun.pan@linux.intel.com>
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  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 <eric.auger@redhat.com>

Thanks

Eric


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag
  2019-11-18 19:42 ` [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag Jacob Pan
@ 2019-11-18 20:33   ` Auger Eric
  2019-11-18 21:48     ` Jacob Pan
  0 siblings, 1 reply; 32+ messages in thread
From: Auger Eric @ 2019-11-18 20:33 UTC (permalink / raw)
  To: Jacob Pan, iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu

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 <jacob.jun.pan@linux.intel.com>
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  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;
>  
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode
  2019-11-18 19:42 ` [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode Jacob Pan
@ 2019-11-18 20:55   ` Auger Eric
  2019-11-18 21:52     ` Jacob Pan
  0 siblings, 1 reply; 32+ messages in thread
From: Auger Eric @ 2019-11-18 20:55 UTC (permalink / raw)
  To: Jacob Pan, iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu

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 <jacob.jun.pan@linux.intel.com>
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  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);
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 03/10] iommu/vt-d: Reject SVM bind for failed capability check
  2019-11-18 19:42 ` [PATCH v2 03/10] iommu/vt-d: Reject SVM bind for failed capability check Jacob Pan
@ 2019-11-18 20:55   ` Auger Eric
  0 siblings, 0 replies; 32+ messages in thread
From: Auger Eric @ 2019-11-18 20:55 UTC (permalink / raw)
  To: Jacob Pan, iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu

Hi Jacob,

On 11/18/19 8:42 PM, Jacob Pan wrote:
> Add a check during SVM bind to ensure CPU and IOMMU hardware capabilities
> are met.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel-svm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 716c543488f6..74df10a39dfc 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -238,6 +238,9 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
>  	if (!iommu || dmar_disabled)
>  		return -EINVAL;
>  
> +	if (!intel_svm_capable(iommu))
> +		return -ENOTSUPP;
> +
>  	if (dev_is_pci(dev)) {
>  		pasid_max = pci_max_pasids(to_pci_dev(dev));
>  		if (pasid_max < 0)
> 

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks

Eric


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 05/10] iommu/vt-d: Avoid duplicated code for PASID setup
  2019-11-18 19:42 ` [PATCH v2 05/10] iommu/vt-d: Avoid duplicated code for PASID setup Jacob Pan
@ 2019-11-18 20:59   ` Auger Eric
  0 siblings, 0 replies; 32+ messages in thread
From: Auger Eric @ 2019-11-18 20:59 UTC (permalink / raw)
  To: Jacob Pan, iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu

Hi Jacob,

On 11/18/19 8:42 PM, Jacob Pan wrote:
> After each setup for PASID entry, related translation caches must be
> flushed. We can combine duplicated code into one function which is less
> error prone.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel-pasid.c | 48 +++++++++++++++++----------------------------
>  1 file changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
> index e7cb0b8a7332..732bfee228df 100644
> --- a/drivers/iommu/intel-pasid.c
> +++ b/drivers/iommu/intel-pasid.c
> @@ -465,6 +465,21 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
>  		devtlb_invalidation_with_pasid(iommu, dev, pasid);
>  }
>  
> +static void pasid_flush_caches(struct intel_iommu *iommu,
> +				struct pasid_entry *pte,
> +				int pasid, u16 did)
> +{
> +	if (!ecap_coherent(iommu->ecap))
> +		clflush_cache_range(pte, sizeof(*pte));
> +
> +	if (cap_caching_mode(iommu->cap)) {
> +		pasid_cache_invalidation_with_pasid(iommu, did, pasid);
> +		iotlb_invalidation_with_pasid(iommu, did, pasid);
> +	} else {
> +		iommu_flush_write_buffer(iommu);
> +	}
> +}
> +
>  /*
>   * Set up the scalable mode pasid table entry for first only
>   * translation type.
> @@ -518,16 +533,7 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
>  	/* Setup Present and PASID Granular Transfer Type: */
>  	pasid_set_translation_type(pte, 1);
>  	pasid_set_present(pte);
> -
> -	if (!ecap_coherent(iommu->ecap))
> -		clflush_cache_range(pte, sizeof(*pte));
> -
> -	if (cap_caching_mode(iommu->cap)) {
> -		pasid_cache_invalidation_with_pasid(iommu, did, pasid);
> -		iotlb_invalidation_with_pasid(iommu, did, pasid);
> -	} else {
> -		iommu_flush_write_buffer(iommu);
> -	}
> +	pasid_flush_caches(iommu, pte, pasid, did);
>  
>  	return 0;
>  }
> @@ -591,16 +597,7 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
>  	 */
>  	pasid_set_sre(pte);
>  	pasid_set_present(pte);
> -
> -	if (!ecap_coherent(iommu->ecap))
> -		clflush_cache_range(pte, sizeof(*pte));
> -
> -	if (cap_caching_mode(iommu->cap)) {
> -		pasid_cache_invalidation_with_pasid(iommu, did, pasid);
> -		iotlb_invalidation_with_pasid(iommu, did, pasid);
> -	} else {
> -		iommu_flush_write_buffer(iommu);
> -	}
> +	pasid_flush_caches(iommu, pte, pasid, did);
>  
>  	return 0;
>  }
> @@ -634,16 +631,7 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
>  	 */
>  	pasid_set_sre(pte);
>  	pasid_set_present(pte);
> -
> -	if (!ecap_coherent(iommu->ecap))
> -		clflush_cache_range(pte, sizeof(*pte));
> -
> -	if (cap_caching_mode(iommu->cap)) {
> -		pasid_cache_invalidation_with_pasid(iommu, did, pasid);
> -		iotlb_invalidation_with_pasid(iommu, did, pasid);
> -	} else {
> -		iommu_flush_write_buffer(iommu);
> -	}
> +	pasid_flush_caches(iommu, pte, pasid, did);
>  
>  	return 0;
>  }
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 06/10] iommu/vt-d: Fix off-by-one in PASID allocation
  2019-11-18 19:42 ` [PATCH v2 06/10] iommu/vt-d: Fix off-by-one in PASID allocation Jacob Pan
@ 2019-11-18 21:00   ` Auger Eric
  0 siblings, 0 replies; 32+ messages in thread
From: Auger Eric @ 2019-11-18 21:00 UTC (permalink / raw)
  To: Jacob Pan, iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu

Hi Jacob,

On 11/18/19 8:42 PM, Jacob Pan wrote:
> PASID allocator uses IDR which is exclusive for the end of the
> allocation range. There is no need to decrement pasid_max.
> 
> Fixes: af39507305fb ("iommu/vt-d: Apply global PASID in SVA")
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel-svm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 74df10a39dfc..e90d0b914afe 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -338,7 +338,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
>  		/* 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);
> +					   pasid_max, GFP_KERNEL);
>  		if (ret < 0) {
>  			kfree(svm);
>  			kfree(sdev);
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID
  2019-11-18 19:42 ` [PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID Jacob Pan
@ 2019-11-18 21:11   ` Auger Eric
  2019-11-18 22:16     ` Jacob Pan
  0 siblings, 1 reply; 32+ messages in thread
From: Auger Eric @ 2019-11-18 21:11 UTC (permalink / raw)
  To: Jacob Pan, iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu

Hi Jacob,

On 11/18/19 8:42 PM, Jacob Pan wrote:
> Make use of generic IOASID code to manage PASID allocation,
> free, and lookup. Replace Intel specific code.
> IOASID allocator is inclusive for both start and end of the allocation
> range. The current code is based on IDR, which is exclusive for
> the end of the allocation range. This patch fixes the off-by-one
> error in intel_svm_bind_mm, where pasid_max - 1 is used for the end of
> allocation range.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/Kconfig       |  1 +
>  drivers/iommu/intel-iommu.c | 13 +++++++------
>  drivers/iommu/intel-pasid.c | 36 ------------------------------------
>  drivers/iommu/intel-svm.c   | 41 +++++++++++++++++++++++++++--------------
>  4 files changed, 35 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index fd50ddffffbf..43ce450a40d3 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -212,6 +212,7 @@ config INTEL_IOMMU_SVM
>  	depends on INTEL_IOMMU && X86
>  	select PCI_PASID
>  	select MMU_NOTIFIER
> +	select IOASID
>  	help
>  	  Shared Virtual Memory (SVM) provides a facility for devices
>  	  to access DMA resources through process address space by
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index d598168e410d..a84f0caa33a0 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -5238,7 +5238,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,
> @@ -5256,10 +5256,11 @@ 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;
>  		}
> @@ -5295,7 +5296,7 @@ static int aux_domain_add_dev(struct dmar_domain *domain,
>  	spin_unlock(&iommu->lock);
>  	spin_unlock_irqrestore(&device_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 732bfee228df..3cb569e76642 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(&pasid_lock);
> -	ret = idr_alloc(&pasid_idr, ptr, min, max, GFP_ATOMIC);
> -	spin_unlock(&pasid_lock);
> -	idr_preload_end();
> -
> -	return ret;
> -}
> -
> -void intel_pasid_free_id(int pasid)
> -{
> -	spin_lock(&pasid_lock);
> -	idr_remove(&pasid_idr, pasid);
> -	spin_unlock(&pasid_lock);
> -}
> -
> -void *intel_pasid_lookup_id(int pasid)
> -{
> -	void *p;
> -
> -	spin_lock(&pasid_lock);
> -	p = idr_find(&pasid_idr, pasid);
> -	spin_unlock(&pasid_lock);
> -
> -	return p;
> -}
>  
>  /*
>   * Per device pasid table management:
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index e90d0b914afe..26a2f57763ec 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -17,6 +17,7 @@
>  #include <linux/dmar.h>
>  #include <linux/interrupt.h>
>  #include <linux/mm_types.h>
> +#include <linux/ioasid.h>
>  #include <asm/page.h>
>  
>  #include "intel-pasid.h"
> @@ -335,16 +336,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, 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 = &intel_mmuops;
>  		svm->mm = mm;
>  		svm->flags = flags;
> @@ -354,7 +354,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
>  		if (mm) {
>  			ret = mmu_notifier_register(&svm->notifier, mm);
>  			if (ret) {
> -				intel_pasid_free_id(svm->pasid);
> +				ioasid_free(svm->pasid);
>  				kfree(svm);
>  				kfree(sdev);
>  				goto out;
> @@ -370,7 +370,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
>  		if (ret) {
>  			if (mm)
>  				mmu_notifier_unregister(&svm->notifier, mm);
> -			intel_pasid_free_id(svm->pasid);
> +			ioasid_free(svm->pasid);
>  			kfree(svm);
>  			kfree(sdev);
>  			goto out;
> @@ -418,7 +418,15 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
>  	if (!iommu)
>  		goto out;
>  
> -	svm = intel_pasid_lookup_id(pasid);
> +	svm = ioasid_find(NULL, pasid, NULL);
> +	if (!svm)
> +		goto out;
> +
> +	if (IS_ERR(svm)) {
> +		ret = PTR_ERR(svm);
> +		goto out;
> +	}
> +
>  	if (!svm)
>  		goto out;
>  
> @@ -440,7 +448,9 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
>  				kfree_rcu(sdev, rcu);
>  
>  				if (list_empty(&svm->devs)) {
> -					intel_pasid_free_id(svm->pasid);
> +					/* Clear private data so that free pass check */
> +					ioasid_set_data(svm->pasid, NULL);
This still looks weird to me. clearing the private data before freeing
is a requirement from the custom allocation, if I am not wrong, ie. not
a requirement from the generic allocator. If confirmed, this should not
be part of this patch but added later on?


> +					ioasid_free(svm->pasid);
>  					if (svm->mm)
>  						mmu_notifier_unregister(&svm->notifier, svm->mm);
>  
> @@ -475,10 +485,14 @@ int intel_svm_is_pasid_valid(struct device *dev, int pasid)
>  	if (!iommu)
>  		goto out;
>  
> -	svm = intel_pasid_lookup_id(pasid);
> +	svm = ioasid_find(NULL, pasid, NULL);
>  	if (!svm)
>  		goto out;
>  
> +	if (IS_ERR(svm)) {
> +		ret = PTR_ERR(svm);
> +		goto out;
> +	}
>  	/* init_mm is used in this case */
>  	if (!svm->mm)
>  		ret = 1;
not related to this patch but in prq_event_thread
resp.qw0 and qw1 are not initialized in case (req->lpig ||
req->priv_data_present) is not true

> @@ -585,13 +599,12 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>  
>  		if (!svm || svm->pasid != req->pasid) {
>  			rcu_read_lock();
> -			svm = intel_pasid_lookup_id(req->pasid);
> +			svm = ioasid_find(NULL, req->pasid, NULL);
>  			/* It *can't* go away, because the driver is not permitted
>  			 * to unbind the mm while any page faults are outstanding.
>  			 * So we only need RCU to protect the internal idr code. */
>  			rcu_read_unlock();
> -
> -			if (!svm) {
> +			if (IS_ERR_OR_NULL(svm)) {
>  				pr_err("%s: Page request for invalid PASID %d: %08llx %08llx\n",
>  				       iommu->name, req->pasid, ((unsigned long long *)req)[0],
>  				       ((unsigned long long *)req)[1]);
> 

Thanks

Eric


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush
  2019-11-18 19:42 ` [PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush Jacob Pan
@ 2019-11-18 21:19   ` Auger Eric
  2019-11-18 23:38     ` Jacob Pan
  0 siblings, 1 reply; 32+ messages in thread
From: Auger Eric @ 2019-11-18 21:19 UTC (permalink / raw)
  To: Jacob Pan, iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu

Hi Jacob,
On 11/18/19 8:42 PM, Jacob Pan wrote:
> Use the correct invalidation descriptor type and granularity.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel-pasid.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
> index 3cb569e76642..ee6ea1bbd917 100644
> --- a/drivers/iommu/intel-pasid.c
> +++ b/drivers/iommu/intel-pasid.c
> @@ -365,7 +365,8 @@ pasid_cache_invalidation_with_pasid(struct intel_iommu *iommu,
>  {
>  	struct qi_desc desc;
>  
> -	desc.qw0 = QI_PC_DID(did) | QI_PC_PASID_SEL | QI_PC_PASID(pasid);
> +	desc.qw0 = QI_PC_DID(did) | QI_PC_GRAN(QI_PC_PASID_SEL) |
> +		QI_PC_PASID(pasid) | QI_PC_TYPE;
Hum I am confused

#define QI_PC_PASID_SEL         (QI_PC_TYPE | QI_PC_GRAN(1))

So the original looks correct to me?

Thanks

Eric



>  	desc.qw1 = 0;
>  	desc.qw2 = 0;
>  	desc.qw3 = 0;
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 09/10] iommu/vt-d: Avoid sending invalid page response
  2019-11-18 19:42 ` [PATCH v2 09/10] iommu/vt-d: Avoid sending invalid page response Jacob Pan
@ 2019-11-18 21:26   ` Auger Eric
  0 siblings, 0 replies; 32+ messages in thread
From: Auger Eric @ 2019-11-18 21:26 UTC (permalink / raw)
  To: Jacob Pan, iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse
  Cc: Tian, Kevin, Raj Ashok, Yi Liu

Hi Jacob,

On 11/18/19 8:42 PM, Jacob Pan wrote:
> Page responses should only be sent when last page in group (LPIG) or
> private data is present in the page request. This patch avoids sending
> invalid descriptors.
> 
> Fixes: 5d308fc1ecf53 ("iommu/vt-d: Add 256-bit invalidation descriptor
> support")
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel-svm.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 26a2f57763ec..098c89246593 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -688,11 +688,10 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>  			if (req->priv_data_present)
>  				memcpy(&resp.qw2, req->priv_data,
>  				       sizeof(req->priv_data));
> +			resp.qw2 = 0;
> +			resp.qw3 = 0;
> +			qi_submit_sync(&resp, iommu);
OK so now all the qwords are initialized ;-)
>  		}
> -		resp.qw2 = 0;
> -		resp.qw3 = 0;
> -		qi_submit_sync(&resp, iommu);
> -
>  		head = (head + sizeof(*req)) & PRQ_RING_MASK;
>  	}
>  
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks
  2019-11-18 20:33   ` Auger Eric
@ 2019-11-18 21:47     ` Jacob Pan
  2019-11-19  8:02       ` Auger Eric
  0 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 21:47 UTC (permalink / raw)
  To: Auger Eric
  Cc: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu, jacob.jun.pan

On Mon, 18 Nov 2019 21:33:34 +0100
Auger Eric <eric.auger@redhat.com> 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 <jacob.jun.pan@linux.intel.com>
> > Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> > ---
> >  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 <eric.auger@redhat.com>
> 
> Thanks
> 
> Eric
> 

[Jacob Pan]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag
  2019-11-18 20:33   ` Auger Eric
@ 2019-11-18 21:48     ` Jacob Pan
  2019-11-19  2:55       ` Lu Baolu
  0 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 21:48 UTC (permalink / raw)
  To: Auger Eric
  Cc: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu, jacob.jun.pan

On Mon, 18 Nov 2019 21:33:53 +0100
Auger Eric <eric.auger@redhat.com> 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 <jacob.jun.pan@linux.intel.com>
> > Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> > ---
> >  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]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode
  2019-11-18 20:55   ` Auger Eric
@ 2019-11-18 21:52     ` Jacob Pan
  2019-11-19  3:06       ` Lu Baolu
  0 siblings, 1 reply; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 21:52 UTC (permalink / raw)
  To: Auger Eric
  Cc: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu, jacob.jun.pan

On Mon, 18 Nov 2019 21:55:03 +0100
Auger Eric <eric.auger@redhat.com> 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 <jacob.jun.pan@linux.intel.com>
> > Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> > ---
> >  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]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID
  2019-11-18 21:11   ` Auger Eric
@ 2019-11-18 22:16     ` Jacob Pan
  0 siblings, 0 replies; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 22:16 UTC (permalink / raw)
  To: Auger Eric
  Cc: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu, jacob.jun.pan

On Mon, 18 Nov 2019 22:11:52 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Jacob,
> 
> On 11/18/19 8:42 PM, Jacob Pan wrote:
> > Make use of generic IOASID code to manage PASID allocation,
> > free, and lookup. Replace Intel specific code.
> > IOASID allocator is inclusive for both start and end of the
> > allocation range. The current code is based on IDR, which is
> > exclusive for the end of the allocation range. This patch fixes the
> > off-by-one error in intel_svm_bind_mm, where pasid_max - 1 is used
> > for the end of allocation range.
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> > Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> > ---
> >  drivers/iommu/Kconfig       |  1 +
> >  drivers/iommu/intel-iommu.c | 13 +++++++------
> >  drivers/iommu/intel-pasid.c | 36
> > ------------------------------------ drivers/iommu/intel-svm.c   |
> > 41 +++++++++++++++++++++++++++-------------- 4 files changed, 35
> > insertions(+), 56 deletions(-)
> > 
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index fd50ddffffbf..43ce450a40d3 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -212,6 +212,7 @@ config INTEL_IOMMU_SVM
> >  	depends on INTEL_IOMMU && X86
> >  	select PCI_PASID
> >  	select MMU_NOTIFIER
> > +	select IOASID
> >  	help
> >  	  Shared Virtual Memory (SVM) provides a facility for
> > devices to access DMA resources through process address space by
> > diff --git a/drivers/iommu/intel-iommu.c
> > b/drivers/iommu/intel-iommu.c index d598168e410d..a84f0caa33a0
> > 100644 --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -5238,7 +5238,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,
> > @@ -5256,10 +5256,11 @@ 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;
> >  		}
> > @@ -5295,7 +5296,7 @@ static int aux_domain_add_dev(struct
> > dmar_domain *domain, spin_unlock(&iommu->lock);
> >  	spin_unlock_irqrestore(&device_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 732bfee228df..3cb569e76642
> > 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(&pasid_lock);
> > -	ret = idr_alloc(&pasid_idr, ptr, min, max, GFP_ATOMIC);
> > -	spin_unlock(&pasid_lock);
> > -	idr_preload_end();
> > -
> > -	return ret;
> > -}
> > -
> > -void intel_pasid_free_id(int pasid)
> > -{
> > -	spin_lock(&pasid_lock);
> > -	idr_remove(&pasid_idr, pasid);
> > -	spin_unlock(&pasid_lock);
> > -}
> > -
> > -void *intel_pasid_lookup_id(int pasid)
> > -{
> > -	void *p;
> > -
> > -	spin_lock(&pasid_lock);
> > -	p = idr_find(&pasid_idr, pasid);
> > -	spin_unlock(&pasid_lock);
> > -
> > -	return p;
> > -}
> >  
> >  /*
> >   * Per device pasid table management:
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index e90d0b914afe..26a2f57763ec 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/dmar.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/mm_types.h>
> > +#include <linux/ioasid.h>
> >  #include <asm/page.h>
> >  
> >  #include "intel-pasid.h"
> > @@ -335,16 +336,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, 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 = &intel_mmuops;
> >  		svm->mm = mm;
> >  		svm->flags = flags;
> > @@ -354,7 +354,7 @@ int intel_svm_bind_mm(struct device *dev, int
> > *pasid, int flags, struct svm_dev_ if (mm) {
> >  			ret =
> > mmu_notifier_register(&svm->notifier, mm); if (ret) {
> > -				intel_pasid_free_id(svm->pasid);
> > +				ioasid_free(svm->pasid);
> >  				kfree(svm);
> >  				kfree(sdev);
> >  				goto out;
> > @@ -370,7 +370,7 @@ int intel_svm_bind_mm(struct device *dev, int
> > *pasid, int flags, struct svm_dev_ if (ret) {
> >  			if (mm)
> >  				mmu_notifier_unregister(&svm->notifier,
> > mm);
> > -			intel_pasid_free_id(svm->pasid);
> > +			ioasid_free(svm->pasid);
> >  			kfree(svm);
> >  			kfree(sdev);
> >  			goto out;
> > @@ -418,7 +418,15 @@ int intel_svm_unbind_mm(struct device *dev,
> > int pasid) if (!iommu)
> >  		goto out;
> >  
> > -	svm = intel_pasid_lookup_id(pasid);
> > +	svm = ioasid_find(NULL, pasid, NULL);
> > +	if (!svm)
> > +		goto out;
> > +
> > +	if (IS_ERR(svm)) {
> > +		ret = PTR_ERR(svm);
> > +		goto out;
> > +	}
> > +
> >  	if (!svm)
> >  		goto out;
> >  
> > @@ -440,7 +448,9 @@ int intel_svm_unbind_mm(struct device *dev, int
> > pasid) kfree_rcu(sdev, rcu);
> >  
> >  				if (list_empty(&svm->devs)) {
> > -
> > intel_pasid_free_id(svm->pasid);
> > +					/* Clear private data so
> > that free pass check */
> > +
> > ioasid_set_data(svm->pasid, NULL);  
> This still looks weird to me. clearing the private data before freeing
> is a requirement from the custom allocation, if I am not wrong, ie.
> not a requirement from the generic allocator. If confirmed, this
> should not be part of this patch but added later on?
> 
You are right, we don't need to clear private data for native bind_mm. I
got confused with the guest PASID bind.
The presence of the private data is used to indicate whether there is
an active guest bind.

One fundamental difference between native and guest bind is that native
bind includes IOASID allocation but guest bind has IOASID preallocated.
So guest bind/unbind is a two step process which I thought would be
done in order. But as Alex pointed out, we should not hold IOASID and
fail the free() call if caller didn't do an unbind.
https://lore.kernel.org/linux-iommu/20191113082940.1b415d00@x1.home/

I will remove this line.
> 
> > +					ioasid_free(svm->pasid);
> >  					if (svm->mm)
> >  						mmu_notifier_unregister(&svm->notifier,
> > svm->mm); 
> > @@ -475,10 +485,14 @@ int intel_svm_is_pasid_valid(struct device
> > *dev, int pasid) if (!iommu)
> >  		goto out;
> >  
> > -	svm = intel_pasid_lookup_id(pasid);
> > +	svm = ioasid_find(NULL, pasid, NULL);
> >  	if (!svm)
> >  		goto out;
> >  
> > +	if (IS_ERR(svm)) {
> > +		ret = PTR_ERR(svm);
> > +		goto out;
> > +	}
> >  	/* init_mm is used in this case */
> >  	if (!svm->mm)
> >  		ret = 1;  
> not related to this patch but in prq_event_thread
> resp.qw0 and qw1 are not initialized in case (req->lpig ||
> req->priv_data_present) is not true
> 
resp is not used either in this case.

> > @@ -585,13 +599,12 @@ static irqreturn_t prq_event_thread(int irq,
> > void *d) 
> >  		if (!svm || svm->pasid != req->pasid) {
> >  			rcu_read_lock();
> > -			svm = intel_pasid_lookup_id(req->pasid);
> > +			svm = ioasid_find(NULL, req->pasid, NULL);
> >  			/* It *can't* go away, because the driver
> > is not permitted
> >  			 * to unbind the mm while any page faults
> > are outstanding.
> >  			 * So we only need RCU to protect the
> > internal idr code. */ rcu_read_unlock();
> > -
> > -			if (!svm) {
> > +			if (IS_ERR_OR_NULL(svm)) {
> >  				pr_err("%s: Page request for
> > invalid PASID %d: %08llx %08llx\n", iommu->name, req->pasid,
> > ((unsigned long long *)req)[0], ((unsigned long long *)req)[1]);
> >   
> 
> Thanks
> 
> Eric
> 

[Jacob Pan]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush
  2019-11-18 21:19   ` Auger Eric
@ 2019-11-18 23:38     ` Jacob Pan
  0 siblings, 0 replies; 32+ messages in thread
From: Jacob Pan @ 2019-11-18 23:38 UTC (permalink / raw)
  To: Auger Eric
  Cc: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu, jacob.jun.pan

On Mon, 18 Nov 2019 22:19:03 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Jacob,
> On 11/18/19 8:42 PM, Jacob Pan wrote:
> > Use the correct invalidation descriptor type and granularity.
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> > ---
> >  drivers/iommu/intel-pasid.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/intel-pasid.c
> > b/drivers/iommu/intel-pasid.c index 3cb569e76642..ee6ea1bbd917
> > 100644 --- a/drivers/iommu/intel-pasid.c
> > +++ b/drivers/iommu/intel-pasid.c
> > @@ -365,7 +365,8 @@ pasid_cache_invalidation_with_pasid(struct
> > intel_iommu *iommu, {
> >  	struct qi_desc desc;
> >  
> > -	desc.qw0 = QI_PC_DID(did) | QI_PC_PASID_SEL |
> > QI_PC_PASID(pasid);
> > +	desc.qw0 = QI_PC_DID(did) | QI_PC_GRAN(QI_PC_PASID_SEL) |
> > +		QI_PC_PASID(pasid) | QI_PC_TYPE;  
> Hum I am confused
> 
> #define QI_PC_PASID_SEL         (QI_PC_TYPE | QI_PC_GRAN(1))
> 
My mistake. I redefined QI_PC_PASID_SEL and QI_PC_ALL_PASIDS without
QI_PCI_TYPE such that they can be used in the granularity conversion
table. But did not include in this patch. I will drop this patch and
keep it with the next series.

> So the original looks correct to me?
> 
> Thanks
> 
> Eric
> 
> 
> 
> >  	desc.qw1 = 0;
> >  	desc.qw2 = 0;
> >  	desc.qw3 = 0;
> >   
> 

[Jacob Pan]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag
  2019-11-18 21:48     ` Jacob Pan
@ 2019-11-19  2:55       ` Lu Baolu
  2019-11-19 17:06         ` Jacob Pan
  0 siblings, 1 reply; 32+ messages in thread
From: Lu Baolu @ 2019-11-19  2:55 UTC (permalink / raw)
  To: Jacob Pan, Auger Eric
  Cc: baolu.lu, iommu, LKML, Joerg Roedel, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu

Hi,

On 11/19/19 5:48 AM, Jacob Pan wrote:
> On Mon, 18 Nov 2019 21:33:53 +0100
> Auger Eric <eric.auger@redhat.com> 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 <jacob.jun.pan@linux.intel.com>
>>> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> ---
>>>   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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode
  2019-11-18 21:52     ` Jacob Pan
@ 2019-11-19  3:06       ` Lu Baolu
  2019-11-19  8:04         ` Auger Eric
  0 siblings, 1 reply; 32+ messages in thread
From: Lu Baolu @ 2019-11-19  3:06 UTC (permalink / raw)
  To: Jacob Pan, Auger Eric
  Cc: baolu.lu, iommu, LKML, Joerg Roedel, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu

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 <eric.auger@redhat.com> 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 <jacob.jun.pan@linux.intel.com>
>>> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> ---
>>>   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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks
  2019-11-18 21:47     ` Jacob Pan
@ 2019-11-19  8:02       ` Auger Eric
  2019-11-19 17:32         ` Jacob Pan
  0 siblings, 1 reply; 32+ messages in thread
From: Auger Eric @ 2019-11-19  8:02 UTC (permalink / raw)
  To: Jacob Pan
  Cc: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu

Hi Jacob,

On 11/18/19 10:47 PM, Jacob Pan wrote:
> On Mon, 18 Nov 2019 21:33:34 +0100
> Auger Eric <eric.auger@redhat.com> 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 <jacob.jun.pan@linux.intel.com>
>>> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> ---
>>>  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 <eric.auger@redhat.com>
>>
>> Thanks
>>
>> Eric
>>
> 
> [Jacob Pan]
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode
  2019-11-19  3:06       ` Lu Baolu
@ 2019-11-19  8:04         ` Auger Eric
  2019-11-19 17:12           ` Jacob Pan
  0 siblings, 1 reply; 32+ messages in thread
From: Auger Eric @ 2019-11-19  8:04 UTC (permalink / raw)
  To: Lu Baolu, Jacob Pan
  Cc: iommu, LKML, Joerg Roedel, David Woodhouse, Tian, Kevin,
	Raj Ashok, Yi Liu

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 <eric.auger@redhat.com> 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 <jacob.jun.pan@linux.intel.com>
>>>> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
>>>> ---
>>>>   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
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag
  2019-11-19  2:55       ` Lu Baolu
@ 2019-11-19 17:06         ` Jacob Pan
  0 siblings, 0 replies; 32+ messages in thread
From: Jacob Pan @ 2019-11-19 17:06 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Auger Eric, iommu, LKML, Joerg Roedel, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu, jacob.jun.pan

On Tue, 19 Nov 2019 10:55:29 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi,
> 
> On 11/19/19 5:48 AM, Jacob Pan wrote:
> > On Mon, 18 Nov 2019 21:33:53 +0100
> > Auger Eric <eric.auger@redhat.com> 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 <jacob.jun.pan@linux.intel.com>
> >>> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> >>> ---
> >>>   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]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode
  2019-11-19  8:04         ` Auger Eric
@ 2019-11-19 17:12           ` Jacob Pan
  0 siblings, 0 replies; 32+ messages in thread
From: Jacob Pan @ 2019-11-19 17:12 UTC (permalink / raw)
  To: Auger Eric
  Cc: Lu Baolu, iommu, LKML, Joerg Roedel, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu, jacob.jun.pan

On Tue, 19 Nov 2019 09:04:10 +0100
Auger Eric <eric.auger@redhat.com> 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 <eric.auger@redhat.com> 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 <jacob.jun.pan@linux.intel.com>
> >>>> Acked-by: Lu Baolu <baolu.lu@linux.intel.com>
> >>>> ---
> >>>>   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]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks
  2019-11-19  8:02       ` Auger Eric
@ 2019-11-19 17:32         ` Jacob Pan
  0 siblings, 0 replies; 32+ messages in thread
From: Jacob Pan @ 2019-11-19 17:32 UTC (permalink / raw)
  To: Auger Eric
  Cc: iommu, LKML, Joerg Roedel, Lu Baolu, David Woodhouse, Tian,
	Kevin, Raj Ashok, Yi Liu, jacob.jun.pan

On Tue, 19 Nov 2019 09:02:26 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Jacob,
> 
> On 11/18/19 10:47 PM, Jacob Pan wrote:
> > On Mon, 18 Nov 2019 21:33:34 +0100
> > Auger Eric <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
> >>
> >> Thanks
> >>
> >> Eric
> >>  
> > 
> > [Jacob Pan]
> >   
> 

[Jacob Pan]

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2019-11-19 17:27 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-18 19:42 [PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes Jacob Pan
2019-11-18 19:42 ` [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag Jacob Pan
2019-11-18 20:33   ` Auger Eric
2019-11-18 21:48     ` Jacob Pan
2019-11-19  2:55       ` Lu Baolu
2019-11-19 17:06         ` Jacob Pan
2019-11-18 19:42 ` [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks Jacob Pan
2019-11-18 20:33   ` Auger Eric
2019-11-18 21:47     ` Jacob Pan
2019-11-19  8:02       ` Auger Eric
2019-11-19 17:32         ` Jacob Pan
2019-11-18 19:42 ` [PATCH v2 03/10] iommu/vt-d: Reject SVM bind for failed capability check Jacob Pan
2019-11-18 20:55   ` Auger Eric
2019-11-18 19:42 ` [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode Jacob Pan
2019-11-18 20:55   ` Auger Eric
2019-11-18 21:52     ` Jacob Pan
2019-11-19  3:06       ` Lu Baolu
2019-11-19  8:04         ` Auger Eric
2019-11-19 17:12           ` Jacob Pan
2019-11-18 19:42 ` [PATCH v2 05/10] iommu/vt-d: Avoid duplicated code for PASID setup Jacob Pan
2019-11-18 20:59   ` Auger Eric
2019-11-18 19:42 ` [PATCH v2 06/10] iommu/vt-d: Fix off-by-one in PASID allocation Jacob Pan
2019-11-18 21:00   ` Auger Eric
2019-11-18 19:42 ` [PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID Jacob Pan
2019-11-18 21:11   ` Auger Eric
2019-11-18 22:16     ` Jacob Pan
2019-11-18 19:42 ` [PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush Jacob Pan
2019-11-18 21:19   ` Auger Eric
2019-11-18 23:38     ` Jacob Pan
2019-11-18 19:42 ` [PATCH v2 09/10] iommu/vt-d: Avoid sending invalid page response Jacob Pan
2019-11-18 21:26   ` Auger Eric
2019-11-18 19:42 ` [PATCH v2 10/10] iommu/vt-d: Misc macro clean up for SVM Jacob Pan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).