[PATCH 0/4] Misc vSVA fixes for VT-d

[PATCH 0/4] Misc vSVA fixes for VT-d

[Jacob Pan]

Hi Baolu et al,

This is a collection of SVA-related fixes.

Thanks,
Jacob


Jacob Pan (4):
  iommu/vt-d: Enable write protect for supervisor SVM
  iommu/vt-d: Enable write protect propagation from guest
  iommu/vt-d: Reject unsupported page request modes
  iommu/vt-d: Calculate and set flags for handle_mm_fault

 drivers/iommu/intel/pasid.c | 31 +++++++++++++++++++++++++++++++
 drivers/iommu/intel/svm.c   | 21 +++++++++++++++++----
 include/uapi/linux/iommu.h  |  3 ++-
 3 files changed, 50 insertions(+), 5 deletions(-)

-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/4] iommu/vt-d: Enable write protect for supervisor SVM

[Jacob Pan]

Write protect bit, when set, inhibits supervisor writes to the read-only
pages. In supervisor shared virtual addressing (SVA), where page tables
are shared between CPU and DMA, IOMMU PASID entry WPE bit should match
CR0.WP bit in the CPU.
This patch sets WPE bit for supervisor PASIDs if CR0.WP is set.

Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/pasid.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 0cceaabc3ce6..0b7e0e726ade 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -410,6 +410,15 @@ static inline void pasid_set_sre(struct pasid_entry *pe)
 	pasid_set_bits(&pe->val[2], 1 << 0, 1);
 }
 
+/*
+ * Setup the WPE(Write Protect Enable) field (Bit 132) of a
+ * scalable mode PASID entry.
+ */
+static inline void pasid_set_wpe(struct pasid_entry *pe)
+{
+	pasid_set_bits(&pe->val[2], 1 << 4, 1 << 4);
+}
+
 /*
  * Setup the P(Present) field (Bit 0) of a scalable mode PASID
  * entry.
@@ -553,6 +562,20 @@ static void pasid_flush_caches(struct intel_iommu *iommu,
 	}
 }
 
+static inline int pasid_enable_wpe(struct pasid_entry *pte)
+{
+	unsigned long cr0 = read_cr0();
+
+	/* CR0.WP is normally set but just to be sure */
+	if (unlikely(!(cr0 & X86_CR0_WP))) {
+		pr_err_ratelimited("No CPU write protect!\n");
+		return -EINVAL;
+	}
+	pasid_set_wpe(pte);
+
+	return 0;
+};
+
 /*
  * Set up the scalable mode pasid table entry for first only
  * translation type.
@@ -584,6 +607,9 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 			return -EINVAL;
 		}
 		pasid_set_sre(pte);
+		if (pasid_enable_wpe(pte))
+			return -EINVAL;
+
 	}
 
 	if (flags & PASID_FLAG_FL5LP) {
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/4] iommu/vt-d: Enable write protect for supervisor SVM

[Lu Baolu]

Hi Jacob and Sanjay,

On 2/19/21 5:31 AM, Jacob Pan wrote:
> Write protect bit, when set, inhibits supervisor writes to the read-only
> pages. In supervisor shared virtual addressing (SVA), where page tables
> are shared between CPU and DMA, IOMMU PASID entry WPE bit should match
> CR0.WP bit in the CPU.
> This patch sets WPE bit for supervisor PASIDs if CR0.WP is set.

 From reading the commit message, the intention of this patch is to match
PASID entry WPE bith with CPU CR0.WP if 1) SRE is set (supervisor
pasid); 2) page table is shared between CPU and IOMMU. Do I understand
it right?

But what the real code doing is failing pasid entry setup for first
level translation if CPU CR0.WP is not set. It's not consistent with
what described above.

What I am thinking is that, as long as SRE is set, we should always set
WPE in intel_pasid_setup_first_level(). For supervisor SVA case, we
should check CPU CR0.WP in intel_svm_bind_mm() and abort binding if
CR0.WP is not set.

Thought?

Best regards,
baolu

> 
> Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>   drivers/iommu/intel/pasid.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 0cceaabc3ce6..0b7e0e726ade 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -410,6 +410,15 @@ static inline void pasid_set_sre(struct pasid_entry *pe)
>   	pasid_set_bits(&pe->val[2], 1 << 0, 1);
>   }
>   
> +/*
> + * Setup the WPE(Write Protect Enable) field (Bit 132) of a
> + * scalable mode PASID entry.
> + */
> +static inline void pasid_set_wpe(struct pasid_entry *pe)
> +{
> +	pasid_set_bits(&pe->val[2], 1 << 4, 1 << 4);
> +}
> +
>   /*
>    * Setup the P(Present) field (Bit 0) of a scalable mode PASID
>    * entry.
> @@ -553,6 +562,20 @@ static void pasid_flush_caches(struct intel_iommu *iommu,
>   	}
>   }
>   
> +static inline int pasid_enable_wpe(struct pasid_entry *pte)
> +{
> +	unsigned long cr0 = read_cr0();
> +
> +	/* CR0.WP is normally set but just to be sure */
> +	if (unlikely(!(cr0 & X86_CR0_WP))) {
> +		pr_err_ratelimited("No CPU write protect!\n");
> +		return -EINVAL;
> +	}
> +	pasid_set_wpe(pte);
> +
> +	return 0;
> +};
> +
>   /*
>    * Set up the scalable mode pasid table entry for first only
>    * translation type.
> @@ -584,6 +607,9 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
>   			return -EINVAL;
>   		}
>   		pasid_set_sre(pte);
> +		if (pasid_enable_wpe(pte))
> +			return -EINVAL;
> +
>   	}
>   
>   	if (flags & PASID_FLAG_FL5LP) {
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/4] iommu/vt-d: Enable write protect for supervisor SVM

[Jacob Pan]

Hi Lu,

On Sat, 20 Feb 2021 09:56:26 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> Hi Jacob and Sanjay,
> 
> On 2/19/21 5:31 AM, Jacob Pan wrote:
> > Write protect bit, when set, inhibits supervisor writes to the read-only
> > pages. In supervisor shared virtual addressing (SVA), where page tables
> > are shared between CPU and DMA, IOMMU PASID entry WPE bit should match
> > CR0.WP bit in the CPU.
> > This patch sets WPE bit for supervisor PASIDs if CR0.WP is set.  
> 
>  From reading the commit message, the intention of this patch is to match
> PASID entry WPE bith with CPU CR0.WP if 1) SRE is set (supervisor
> pasid); 2) page table is shared between CPU and IOMMU. Do I understand
> it right?
> 
yes. that is my intention.

> But what the real code doing is failing pasid entry setup for first
> level translation if CPU CR0.WP is not set. It's not consistent with
> what described above.
> 
> What I am thinking is that, as long as SRE is set, we should always set
> WPE in intel_pasid_setup_first_level(). For supervisor SVA case, we
> should check CPU CR0.WP in intel_svm_bind_mm() and abort binding if
> CR0.WP is not set.
> 
> Thought?
> 
This code only affects supervisor SVA, since PASID_FLAG_SUPERVISOR_MODE
flag is not set for FL IOVA.

> Best regards,
> baolu
> 
> > 
> > Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >   drivers/iommu/intel/pasid.c | 26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> > index 0cceaabc3ce6..0b7e0e726ade 100644
> > --- a/drivers/iommu/intel/pasid.c
> > +++ b/drivers/iommu/intel/pasid.c
> > @@ -410,6 +410,15 @@ static inline void pasid_set_sre(struct
> > pasid_entry *pe) pasid_set_bits(&pe->val[2], 1 << 0, 1);
> >   }
> >   
> > +/*
> > + * Setup the WPE(Write Protect Enable) field (Bit 132) of a
> > + * scalable mode PASID entry.
> > + */
> > +static inline void pasid_set_wpe(struct pasid_entry *pe)
> > +{
> > +	pasid_set_bits(&pe->val[2], 1 << 4, 1 << 4);
> > +}
> > +
> >   /*
> >    * Setup the P(Present) field (Bit 0) of a scalable mode PASID
> >    * entry.
> > @@ -553,6 +562,20 @@ static void pasid_flush_caches(struct intel_iommu
> > *iommu, }
> >   }
> >   
> > +static inline int pasid_enable_wpe(struct pasid_entry *pte)
> > +{
> > +	unsigned long cr0 = read_cr0();
> > +
> > +	/* CR0.WP is normally set but just to be sure */
> > +	if (unlikely(!(cr0 & X86_CR0_WP))) {
> > +		pr_err_ratelimited("No CPU write protect!\n");
> > +		return -EINVAL;
> > +	}
> > +	pasid_set_wpe(pte);
> > +
> > +	return 0;
> > +};
> > +
> >   /*
> >    * Set up the scalable mode pasid table entry for first only
> >    * translation type.
> > @@ -584,6 +607,9 @@ int intel_pasid_setup_first_level(struct
> > intel_iommu *iommu, return -EINVAL;
> >   		}
> >   		pasid_set_sre(pte);
> > +		if (pasid_enable_wpe(pte))
> > +			return -EINVAL;
> > +
> >   	}
> >   
> >   	if (flags & PASID_FLAG_FL5LP) {
> >   


Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/4] iommu/vt-d: Enable write protect for supervisor SVM

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4041 bytes --]

Hi Jacob,

I love your patch! Yet something to improve:

[auto build test ERROR on iommu/next]
[also build test ERROR on linux/master linus/master v5.12-rc1 next-20210303]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jacob-Pan/Misc-vSVA-fixes-for-VT-d/20210219-141141
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: ia64-randconfig-r023-20210304 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4c1de3403ecb6f91cc1bdc5e3ca81f16a2ffc0b5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jacob-Pan/Misc-vSVA-fixes-for-VT-d/20210219-141141
        git checkout 4c1de3403ecb6f91cc1bdc5e3ca81f16a2ffc0b5
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/ia64/include/asm/pgtable.h:154,
                    from include/linux/pgtable.h:6,
                    from include/linux/mm.h:33,
                    from include/linux/scatterlist.h:8,
                    from include/linux/dma-mapping.h:10,
                    from include/linux/iova.h:16,
                    from include/linux/intel-iommu.h:14,
                    from drivers/iommu/intel/pasid.c:15:
   arch/ia64/include/asm/mmu_context.h: In function 'reload_context':
   arch/ia64/include/asm/mmu_context.h:127:41: warning: variable 'old_rr4' set but not used [-Wunused-but-set-variable]
     127 |  unsigned long rr0, rr1, rr2, rr3, rr4, old_rr4;
         |                                         ^~~~~~~
   drivers/iommu/intel/pasid.c: In function 'pasid_enable_wpe':
>> drivers/iommu/intel/pasid.c:536:22: error: implicit declaration of function 'read_cr0' [-Werror=implicit-function-declaration]
     536 |  unsigned long cr0 = read_cr0();
         |                      ^~~~~~~~
   In file included from include/linux/build_bug.h:5,
                    from include/linux/bits.h:22,
                    from include/linux/bitops.h:6,
                    from drivers/iommu/intel/pasid.c:12:
>> drivers/iommu/intel/pasid.c:539:23: error: 'X86_CR0_WP' undeclared (first use in this function)
     539 |  if (unlikely(!(cr0 & X86_CR0_WP))) {
         |                       ^~~~~~~~~~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   drivers/iommu/intel/pasid.c:539:23: note: each undeclared identifier is reported only once for each function it appears in
     539 |  if (unlikely(!(cr0 & X86_CR0_WP))) {
         |                       ^~~~~~~~~~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   cc1: some warnings being treated as errors


vim +/read_cr0 +536 drivers/iommu/intel/pasid.c

   533	
   534	static inline int pasid_enable_wpe(struct pasid_entry *pte)
   535	{
 > 536		unsigned long cr0 = read_cr0();
   537	
   538		/* CR0.WP is normally set but just to be sure */
 > 539		if (unlikely(!(cr0 & X86_CR0_WP))) {
   540			pr_err_ratelimited("No CPU write protect!\n");
   541			return -EINVAL;
   542		}
   543		pasid_set_wpe(pte);
   544	
   545		return 0;
   546	};
   547	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25123 bytes --]

[-- Attachment #3: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/4] iommu/vt-d: Enable write protect propagation from guest

[Jacob Pan]

Write protect bit, when set, inhibits supervisor writes to the read-only
pages. In guest supervisor shared virtual addressing (SVA), write-protect
should be honored upon guest bind supervisor PASID request.

This patch extends the VT-d portion of the IOMMU UAPI to include WP bit.
WPE bit of the  supervisor PASID entry will be set to match CPU CR0.WP bit.

Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/pasid.c | 5 +++++
 include/uapi/linux/iommu.h  | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 0b7e0e726ade..c7a2ec930af4 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -763,6 +763,11 @@ intel_pasid_setup_bind_data(struct intel_iommu *iommu, struct pasid_entry *pte,
 			return -EINVAL;
 		}
 		pasid_set_sre(pte);
+		/* Enable write protect WP if guest requested */
+		if (pasid_data->flags & IOMMU_SVA_VTD_GPASID_WPE) {
+			if (pasid_enable_wpe(pte))
+				return -EINVAL;
+		}
 	}
 
 	if (pasid_data->flags & IOMMU_SVA_VTD_GPASID_EAFE) {
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index 68cb558fe8db..33f3dc7a91de 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -288,7 +288,8 @@ struct iommu_gpasid_bind_data_vtd {
 #define IOMMU_SVA_VTD_GPASID_PWT	(1 << 3) /* page-level write through */
 #define IOMMU_SVA_VTD_GPASID_EMTE	(1 << 4) /* extended mem type enable */
 #define IOMMU_SVA_VTD_GPASID_CD		(1 << 5) /* PASID-level cache disable */
-#define IOMMU_SVA_VTD_GPASID_LAST	(1 << 6)
+#define IOMMU_SVA_VTD_GPASID_WPE	(1 << 6) /* Write protect enable */
+#define IOMMU_SVA_VTD_GPASID_LAST	(1 << 7)
 	__u64 flags;
 	__u32 pat;
 	__u32 emt;
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 2/4] iommu/vt-d: Enable write protect propagation from guest

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Friday, February 19, 2021 5:31 AM
> 
> Write protect bit, when set, inhibits supervisor writes to the read-only
> pages. In guest supervisor shared virtual addressing (SVA), write-protect
> should be honored upon guest bind supervisor PASID request.
> 
> This patch extends the VT-d portion of the IOMMU UAPI to include WP bit.
> WPE bit of the  supervisor PASID entry will be set to match CPU CR0.WP bit.
> 
> Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>  drivers/iommu/intel/pasid.c | 5 +++++
>  include/uapi/linux/iommu.h  | 3 ++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 0b7e0e726ade..c7a2ec930af4 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -763,6 +763,11 @@ intel_pasid_setup_bind_data(struct intel_iommu
> *iommu, struct pasid_entry *pte,
>  			return -EINVAL;
>  		}
>  		pasid_set_sre(pte);
> +		/* Enable write protect WP if guest requested */
> +		if (pasid_data->flags & IOMMU_SVA_VTD_GPASID_WPE) {
> +			if (pasid_enable_wpe(pte))
> +				return -EINVAL;

We should call pasid_set_wpe directly, as this binding is about guest
page table and suppose the guest has done whatever check required
(e.g. gcr0.wp) before setting this bit. pasid_enable_wpe has an additional 
check on host cr0.wp thus is logically incorrect here.

Thanks
Kevin

> +		}
>  	}
> 
>  	if (pasid_data->flags & IOMMU_SVA_VTD_GPASID_EAFE) {
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index 68cb558fe8db..33f3dc7a91de 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -288,7 +288,8 @@ struct iommu_gpasid_bind_data_vtd {
>  #define IOMMU_SVA_VTD_GPASID_PWT	(1 << 3) /* page-level write
> through */
>  #define IOMMU_SVA_VTD_GPASID_EMTE	(1 << 4) /* extended mem
> type enable */
>  #define IOMMU_SVA_VTD_GPASID_CD		(1 << 5) /* PASID-level
> cache disable */
> -#define IOMMU_SVA_VTD_GPASID_LAST	(1 << 6)
> +#define IOMMU_SVA_VTD_GPASID_WPE	(1 << 6) /* Write protect
> enable */
> +#define IOMMU_SVA_VTD_GPASID_LAST	(1 << 7)
>  	__u64 flags;
>  	__u32 pat;
>  	__u32 emt;
> --
> 2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] iommu/vt-d: Enable write protect propagation from guest

[Jacob Pan]

Hi Kevin,

On Fri, 19 Feb 2021 06:19:04 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Friday, February 19, 2021 5:31 AM
> > 
> > Write protect bit, when set, inhibits supervisor writes to the read-only
> > pages. In guest supervisor shared virtual addressing (SVA),
> > write-protect should be honored upon guest bind supervisor PASID
> > request.
> > 
> > This patch extends the VT-d portion of the IOMMU UAPI to include WP bit.
> > WPE bit of the  supervisor PASID entry will be set to match CPU CR0.WP
> > bit.
> > 
> > Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >  drivers/iommu/intel/pasid.c | 5 +++++
> >  include/uapi/linux/iommu.h  | 3 ++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> > index 0b7e0e726ade..c7a2ec930af4 100644
> > --- a/drivers/iommu/intel/pasid.c
> > +++ b/drivers/iommu/intel/pasid.c
> > @@ -763,6 +763,11 @@ intel_pasid_setup_bind_data(struct intel_iommu
> > *iommu, struct pasid_entry *pte,
> >  			return -EINVAL;
> >  		}
> >  		pasid_set_sre(pte);
> > +		/* Enable write protect WP if guest requested */
> > +		if (pasid_data->flags & IOMMU_SVA_VTD_GPASID_WPE) {
> > +			if (pasid_enable_wpe(pte))
> > +				return -EINVAL;  
> 
> We should call pasid_set_wpe directly, as this binding is about guest
> page table and suppose the guest has done whatever check required
> (e.g. gcr0.wp) before setting this bit. pasid_enable_wpe has an
> additional check on host cr0.wp thus is logically incorrect here.
> 
If the host CPU does not support WP, can guest VCPU still support WP? If
so, I agree.

> Thanks
> Kevin
> 
> > +		}
> >  	}
> > 
> >  	if (pasid_data->flags & IOMMU_SVA_VTD_GPASID_EAFE) {
> > diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> > index 68cb558fe8db..33f3dc7a91de 100644
> > --- a/include/uapi/linux/iommu.h
> > +++ b/include/uapi/linux/iommu.h
> > @@ -288,7 +288,8 @@ struct iommu_gpasid_bind_data_vtd {
> >  #define IOMMU_SVA_VTD_GPASID_PWT	(1 << 3) /* page-level write
> > through */
> >  #define IOMMU_SVA_VTD_GPASID_EMTE	(1 << 4) /* extended mem
> > type enable */
> >  #define IOMMU_SVA_VTD_GPASID_CD		(1 << 5) /* PASID-level
> > cache disable */
> > -#define IOMMU_SVA_VTD_GPASID_LAST	(1 << 6)
> > +#define IOMMU_SVA_VTD_GPASID_WPE	(1 << 6) /* Write protect
> > enable */
> > +#define IOMMU_SVA_VTD_GPASID_LAST	(1 << 7)
> >  	__u64 flags;
> >  	__u32 pat;
> >  	__u32 emt;
> > --
> > 2.25.1  
> 


Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 2/4] iommu/vt-d: Enable write protect propagation from guest

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Saturday, February 20, 2021 1:09 AM
> 
> Hi Kevin,
> 
> On Fri, 19 Feb 2021 06:19:04 +0000, "Tian, Kevin" <kevin.tian@intel.com>
> wrote:
> 
> > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > Sent: Friday, February 19, 2021 5:31 AM
> > >
> > > Write protect bit, when set, inhibits supervisor writes to the read-only
> > > pages. In guest supervisor shared virtual addressing (SVA),
> > > write-protect should be honored upon guest bind supervisor PASID
> > > request.
> > >
> > > This patch extends the VT-d portion of the IOMMU UAPI to include WP bit.
> > > WPE bit of the  supervisor PASID entry will be set to match CPU CR0.WP
> > > bit.
> > >
> > > Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
> > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > ---
> > >  drivers/iommu/intel/pasid.c | 5 +++++
> > >  include/uapi/linux/iommu.h  | 3 ++-
> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> > > index 0b7e0e726ade..c7a2ec930af4 100644
> > > --- a/drivers/iommu/intel/pasid.c
> > > +++ b/drivers/iommu/intel/pasid.c
> > > @@ -763,6 +763,11 @@ intel_pasid_setup_bind_data(struct
> intel_iommu
> > > *iommu, struct pasid_entry *pte,
> > >  			return -EINVAL;
> > >  		}
> > >  		pasid_set_sre(pte);
> > > +		/* Enable write protect WP if guest requested */
> > > +		if (pasid_data->flags & IOMMU_SVA_VTD_GPASID_WPE) {
> > > +			if (pasid_enable_wpe(pte))
> > > +				return -EINVAL;
> >
> > We should call pasid_set_wpe directly, as this binding is about guest
> > page table and suppose the guest has done whatever check required
> > (e.g. gcr0.wp) before setting this bit. pasid_enable_wpe has an
> > additional check on host cr0.wp thus is logically incorrect here.
> >
> If the host CPU does not support WP, can guest VCPU still support WP? If
> so, I agree.
> 

If you change 'support' to 'enable', then the answer is yes.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] iommu/vt-d: Enable write protect propagation from guest

[Jacob Pan]

Hi Kevin,

On Sat, 20 Feb 2021 02:38:02 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Saturday, February 20, 2021 1:09 AM
> > 
> > Hi Kevin,
> > 
> > On Fri, 19 Feb 2021 06:19:04 +0000, "Tian, Kevin" <kevin.tian@intel.com>
> > wrote:
> >   
> > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > Sent: Friday, February 19, 2021 5:31 AM
> > > >
> > > > Write protect bit, when set, inhibits supervisor writes to the
> > > > read-only pages. In guest supervisor shared virtual addressing
> > > > (SVA), write-protect should be honored upon guest bind supervisor
> > > > PASID request.
> > > >
> > > > This patch extends the VT-d portion of the IOMMU UAPI to include WP
> > > > bit. WPE bit of the  supervisor PASID entry will be set to match
> > > > CPU CR0.WP bit.
> > > >
> > > > Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
> > > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > ---
> > > >  drivers/iommu/intel/pasid.c | 5 +++++
> > > >  include/uapi/linux/iommu.h  | 3 ++-
> > > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iommu/intel/pasid.c
> > > > b/drivers/iommu/intel/pasid.c index 0b7e0e726ade..c7a2ec930af4
> > > > 100644 --- a/drivers/iommu/intel/pasid.c
> > > > +++ b/drivers/iommu/intel/pasid.c
> > > > @@ -763,6 +763,11 @@ intel_pasid_setup_bind_data(struct  
> > intel_iommu  
> > > > *iommu, struct pasid_entry *pte,
> > > >  			return -EINVAL;
> > > >  		}
> > > >  		pasid_set_sre(pte);
> > > > +		/* Enable write protect WP if guest requested */
> > > > +		if (pasid_data->flags & IOMMU_SVA_VTD_GPASID_WPE) {
> > > > +			if (pasid_enable_wpe(pte))
> > > > +				return -EINVAL;  
> > >
> > > We should call pasid_set_wpe directly, as this binding is about guest
> > > page table and suppose the guest has done whatever check required
> > > (e.g. gcr0.wp) before setting this bit. pasid_enable_wpe has an
> > > additional check on host cr0.wp thus is logically incorrect here.
> > >  
> > If the host CPU does not support WP, can guest VCPU still support WP? If
> > so, I agree.
> >   
> 
> If you change 'support' to 'enable', then the answer is yes.

I agree, thanks for explaining. Will change it to pasid_set_wpe.

Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/4] iommu/vt-d: Reject unsupported page request modes

[Jacob Pan]

When supervisor/privilige mode SVM is used, we bind init_mm.pgd with
a supervisor PASID. There should not be any page fault for init_mm.
Execution request with DMA read is also not supported.

This patch checks PRQ descriptor for both unsupported configurations,
reject them both with invalid responses.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/svm.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 23a1e4f58c54..ff7ae7cc17d5 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -1113,7 +1113,17 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			       ((unsigned long long *)req)[1]);
 			goto no_pasid;
 		}
-
+		/* We shall not receive page request for supervisor SVM */
+		if (req->pm_req && (req->rd_req | req->wr_req)) {
+			pr_err("Unexpected page request in Privilege Mode");
+			/* No need to find the matching sdev as for bad_req */
+			goto no_pasid;
+		}
+		/* DMA read with exec requeset is not supported. */
+		if (req->exe_req && req->rd_req) {
+			pr_err("Execution request not supported\n");
+			goto no_pasid;
+		}
 		if (!svm || svm->pasid != req->pasid) {
 			rcu_read_lock();
 			svm = ioasid_find(NULL, req->pasid, NULL);
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/vt-d: Reject unsupported page request modes

[Lu Baolu]

On 2/19/21 5:31 AM, Jacob Pan wrote:
> When supervisor/privilige mode SVM is used, we bind init_mm.pgd with
> a supervisor PASID. There should not be any page fault for init_mm.
> Execution request with DMA read is also not supported.
> 
> This patch checks PRQ descriptor for both unsupported configurations,
> reject them both with invalid responses.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Fixes: 1c4f88b7f1f92 ("iommu/vt-d: Shared virtual address in scalable mode")
Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

> ---
>   drivers/iommu/intel/svm.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 23a1e4f58c54..ff7ae7cc17d5 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -1113,7 +1113,17 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>   			       ((unsigned long long *)req)[1]);
>   			goto no_pasid;
>   		}
> -
> +		/* We shall not receive page request for supervisor SVM */
> +		if (req->pm_req && (req->rd_req | req->wr_req)) {
> +			pr_err("Unexpected page request in Privilege Mode");
> +			/* No need to find the matching sdev as for bad_req */
> +			goto no_pasid;
> +		}
> +		/* DMA read with exec requeset is not supported. */
> +		if (req->exe_req && req->rd_req) {
> +			pr_err("Execution request not supported\n");
> +			goto no_pasid;
> +		}
>   		if (!svm || svm->pasid != req->pasid) {
>   			rcu_read_lock();
>   			svm = ioasid_find(NULL, req->pasid, NULL);
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 4/4] iommu/vt-d: Calculate and set flags for handle_mm_fault

[Jacob Pan]

Page requests are originated from the user page fault. Therefore, we
shall set FAULT_FLAG_USER. 

FAULT_FLAG_REMOTE indicates that we are walking an mm which is not
guaranteed to be the same as the current->mm and should not be subject
to protection key enforcement. Therefore, we should set FAULT_FLAG_REMOTE
to avoid faults when both SVM and PKEY are used.

References: commit 1b2ee1266ea6 ("mm/core: Do not enforce PKEY permissions on remote mm access")
Reviewed-by: Raj Ashok <ashok.raj@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/svm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index ff7ae7cc17d5..7bfd20a24a60 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -1086,6 +1086,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 	struct intel_iommu *iommu = d;
 	struct intel_svm *svm = NULL;
 	int head, tail, handled = 0;
+	unsigned int flags = 0;
 
 	/* Clear PPR bit before reading head/tail registers, to
 	 * ensure that we get a new interrupt if needed. */
@@ -1186,9 +1187,11 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 		if (access_error(vma, req))
 			goto invalid;
 
-		ret = handle_mm_fault(vma, address,
-				      req->wr_req ? FAULT_FLAG_WRITE : 0,
-				      NULL);
+		flags = FAULT_FLAG_USER | FAULT_FLAG_REMOTE;
+		if (req->wr_req)
+			flags |= FAULT_FLAG_WRITE;
+
+		ret = handle_mm_fault(vma, address, flags, NULL);
 		if (ret & VM_FAULT_ERROR)
 			goto invalid;
 
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu/vt-d: Calculate and set flags for handle_mm_fault

[Lu Baolu]

On 2/19/21 5:31 AM, Jacob Pan wrote:
> Page requests are originated from the user page fault. Therefore, we
> shall set FAULT_FLAG_USER.
> 
> FAULT_FLAG_REMOTE indicates that we are walking an mm which is not
> guaranteed to be the same as the current->mm and should not be subject
> to protection key enforcement. Therefore, we should set FAULT_FLAG_REMOTE
> to avoid faults when both SVM and PKEY are used.
> 
> References: commit 1b2ee1266ea6 ("mm/core: Do not enforce PKEY permissions on remote mm access")
> Reviewed-by: Raj Ashok <ashok.raj@intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

> ---
>   drivers/iommu/intel/svm.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index ff7ae7cc17d5..7bfd20a24a60 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -1086,6 +1086,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>   	struct intel_iommu *iommu = d;
>   	struct intel_svm *svm = NULL;
>   	int head, tail, handled = 0;
> +	unsigned int flags = 0;
>   
>   	/* Clear PPR bit before reading head/tail registers, to
>   	 * ensure that we get a new interrupt if needed. */
> @@ -1186,9 +1187,11 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>   		if (access_error(vma, req))
>   			goto invalid;
>   
> -		ret = handle_mm_fault(vma, address,
> -				      req->wr_req ? FAULT_FLAG_WRITE : 0,
> -				      NULL);
> +		flags = FAULT_FLAG_USER | FAULT_FLAG_REMOTE;
> +		if (req->wr_req)
> +			flags |= FAULT_FLAG_WRITE;
> +
> +		ret = handle_mm_fault(vma, address, flags, NULL);
>   		if (ret & VM_FAULT_ERROR)
>   			goto invalid;
>   
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu