[PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes

[PATCH v2 00/10] VT-d Native Shared virtual memory cleanup and fixes

[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

[PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag

[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

Re: [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag

[Auger Eric]

Hi Jacob,

On 11/18/19 8:42 PM, Jacob Pan wrote:
> Shared Virtual Memory(SVM) is based on a collective set of hardware
> features detected at runtime. There are requirements for matching CPU
> and IOMMU capabilities.
> 
> This patch introduces a flag which will be used to mark and test the
> capability of SVM.
> 
> Signed-off-by: Jacob Pan <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;
>  
> 

Re: [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag

[Jacob 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]

Re: [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag

[Lu Baolu]

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

Re: [PATCH v2 01/10] iommu/vt-d: Introduce native SVM capable flag

[Jacob 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]

[PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks

[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

Re: [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks

[Auger Eric]

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

Re: [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks

[Jacob 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]

Re: [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks

[Auger Eric]

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]
> 

Re: [PATCH v2 02/10] iommu/vt-d: Fix CPU and IOMMU SVM feature matching checks

[Jacob 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]

[PATCH v2 03/10] iommu/vt-d: Reject SVM bind for failed capability check

[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

Re: [PATCH v2 03/10] iommu/vt-d: Reject SVM bind for failed capability check

[Auger Eric]

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

[PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode

[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

Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode

[Auger Eric]

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);
> 

Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode

[Jacob 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]

Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode

[Lu Baolu]

Hi Eric and Jacob,

On 11/19/19 5:52 AM, Jacob Pan wrote:
> On Mon, 18 Nov 2019 21:55:03 +0100
> Auger Eric <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

Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode

[Auger Eric]

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
> 

Re: [PATCH v2 04/10] iommu/vt-d: Match CPU and IOMMU paging mode

[Jacob 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]

[PATCH v2 05/10] iommu/vt-d: Avoid duplicated code for PASID setup

[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

Re: [PATCH v2 05/10] iommu/vt-d: Avoid duplicated code for PASID setup

[Auger Eric]

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

[PATCH v2 06/10] iommu/vt-d: Fix off-by-one in PASID allocation

[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

Re: [PATCH v2 06/10] iommu/vt-d: Fix off-by-one in PASID allocation

[Auger Eric]

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

[PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID

[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

Re: [PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID

[Auger Eric]

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

Re: [PATCH v2 07/10] iommu/vt-d: Replace Intel specific PASID allocator with IOASID

[Jacob 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]

[PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush

[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

Re: [PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush

[Auger Eric]

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;
> 

Re: [PATCH v2 08/10] iommu/vt-d: Fix PASID cache flush

[Jacob 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]

[PATCH v2 09/10] iommu/vt-d: Avoid sending invalid page response

[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

Re: [PATCH v2 09/10] iommu/vt-d: Avoid sending invalid page response

[Auger Eric]

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

[PATCH v2 10/10] iommu/vt-d: Misc macro clean up for SVM

[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