[Patch v4 2/4] Add SMEP handling when setting CR4

[Patch v4 2/4] Add SMEP handling when setting CR4

[Yang, Wei Y]

This patch adds SMEP handling when setting CR4.
 
 Signed-off-by: Yang, Wei <wei.y.yang@intel.com>
 Signed-off-by: Shan, Haitao <haitao.shan@intel.com>
 Signed-off-by: Li, Xin <xin.li@intel.com>

---
 arch/x86/kvm/x86.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 77c9d86..91bfc40 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -579,6 +579,14 @@ static bool guest_cpuid_has_xsave(struct kvm_vcpu *vcpu)
 	return best && (best->ecx & bit(X86_FEATURE_XSAVE));
 }

+static bool guest_cpuid_has_smep(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 7, 0);
+	return best && (best->ebx & bit(X86_FEATURE_SMEP));
+}
+
 static void update_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
@@ -598,14 +606,17 @@ static void update_cpuid(struct kvm_vcpu *vcpu)
 int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
 	unsigned long old_cr4 = kvm_read_cr4(vcpu);
-	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE | X86_CR4_PAE;
-
+	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE |
+				   X86_CR4_PAE | X86_CR4_SMEP;
 	if (cr4 & CR4_RESERVED_BITS)
 		return 1;

 	if (!guest_cpuid_has_xsave(vcpu) && (cr4 & X86_CR4_OSXSAVE))
 		return 1;

+	if (!guest_cpuid_has_smep(vcpu) && (cr4 & X86_CR4_SMEP))
+		return 1;
+
 	if (is_long_mode(vcpu)) {
 		if (!(cr4 & X86_CR4_PAE))
 			return 1;
--
1.7.4.1

Re: [Patch v4 2/4] Add SMEP handling when setting CR4

[Marcelo Tosatti]

On Sun, May 29, 2011 at 07:41:57PM +0800, Yang, Wei Y wrote:
> This patch adds SMEP handling when setting CR4.
>  
>  Signed-off-by: Yang, Wei <wei.y.yang@intel.com>
>  Signed-off-by: Shan, Haitao <haitao.shan@intel.com>
>  Signed-off-by: Li, Xin <xin.li@intel.com>
> 
> ---
>  arch/x86/kvm/x86.c |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 77c9d86..91bfc40 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -579,6 +579,14 @@ static bool guest_cpuid_has_xsave(struct kvm_vcpu *vcpu)
>  	return best && (best->ecx & bit(X86_FEATURE_XSAVE));
>  }
> 
> +static bool guest_cpuid_has_smep(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> +	return best && (best->ebx & bit(X86_FEATURE_SMEP));
> +}
> +
>  static void update_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best;
> @@ -598,14 +606,17 @@ static void update_cpuid(struct kvm_vcpu *vcpu)
>  int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
>  {
>  	unsigned long old_cr4 = kvm_read_cr4(vcpu);
> -	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE | X86_CR4_PAE;
> -
> +	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE |
> +				   X86_CR4_PAE | X86_CR4_SMEP;
>  	if (cr4 & CR4_RESERVED_BITS)
>  		return 1;
> 
>  	if (!guest_cpuid_has_xsave(vcpu) && (cr4 & X86_CR4_OSXSAVE))
>  		return 1;
> 
> +	if (!guest_cpuid_has_smep(vcpu) && (cr4 & X86_CR4_SMEP))
> +		return 1;
> +
>  	if (is_long_mode(vcpu)) {
>  		if (!(cr4 & X86_CR4_PAE))
>  			return 1;

A new field in vcpu->arch.mmu.base_role for smep is required
for shadow MMU (similar to nxe).

Re: [Patch v4 2/4] Add SMEP handling when setting CR4

[Avi Kivity]

On 05/31/2011 08:52 PM, Marcelo Tosatti wrote:
> On Sun, May 29, 2011 at 07:41:57PM +0800, Yang, Wei Y wrote:
> >  This patch adds SMEP handling when setting CR4.
> >
> >   Signed-off-by: Yang, Wei<wei.y.yang@intel.com>
> >   Signed-off-by: Shan, Haitao<haitao.shan@intel.com>
> >   Signed-off-by: Li, Xin<xin.li@intel.com>
> >
> >  ---
> >   arch/x86/kvm/x86.c |   15 +++++++++++++--
> >   1 files changed, 13 insertions(+), 2 deletions(-)
> >
> >  diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >  index 77c9d86..91bfc40 100644
> >  --- a/arch/x86/kvm/x86.c
> >  +++ b/arch/x86/kvm/x86.c
> >  @@ -579,6 +579,14 @@ static bool guest_cpuid_has_xsave(struct kvm_vcpu *vcpu)
> >   	return best&&  (best->ecx&  bit(X86_FEATURE_XSAVE));
> >   }
> >
> >  +static bool guest_cpuid_has_smep(struct kvm_vcpu *vcpu)
> >  +{
> >  +	struct kvm_cpuid_entry2 *best;
> >  +
> >  +	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> >  +	return best&&  (best->ebx&  bit(X86_FEATURE_SMEP));
> >  +}
> >  +
> >   static void update_cpuid(struct kvm_vcpu *vcpu)
> >   {
> >   	struct kvm_cpuid_entry2 *best;
> >  @@ -598,14 +606,17 @@ static void update_cpuid(struct kvm_vcpu *vcpu)
> >   int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> >   {
> >   	unsigned long old_cr4 = kvm_read_cr4(vcpu);
> >  -	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE | X86_CR4_PAE;
> >  -
> >  +	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE |
> >  +				   X86_CR4_PAE | X86_CR4_SMEP;
> >   	if (cr4&  CR4_RESERVED_BITS)
> >   		return 1;
> >
> >   	if (!guest_cpuid_has_xsave(vcpu)&&  (cr4&  X86_CR4_OSXSAVE))
> >   		return 1;
> >
> >  +	if (!guest_cpuid_has_smep(vcpu)&&  (cr4&  X86_CR4_SMEP))
> >  +		return 1;
> >  +
> >   	if (is_long_mode(vcpu)) {
> >   		if (!(cr4&  X86_CR4_PAE))
> >   			return 1;
>
> A new field in vcpu->arch.mmu.base_role for smep is required
> for shadow MMU (similar to nxe).

I plan to add that with my cr0.wp=0 fixup (it's only needed there, right?)

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Patch v4 2/4] Add SMEP handling when setting CR4

[Marcelo Tosatti]

On Tue, May 31, 2011 at 09:05:35PM +0300, Avi Kivity wrote:
> >>   	if (is_long_mode(vcpu)) {
> >>   		if (!(cr4&  X86_CR4_PAE))
> >>   			return 1;
> >
> >A new field in vcpu->arch.mmu.base_role for smep is required
> >for shadow MMU (similar to nxe).
> 
> I plan to add that with my cr0.wp=0 fixup (it's only needed there, right?)

Sptes instantiated when cr4.smep = 0 should not be used when cr4.smep =
1, so no (unlikely that guest kernel executes user=1 code anyway, but
for consistency with other base_role flags).

OK then, you'll fix that.

Re: [Patch v4 2/4] Add SMEP handling when setting CR4

[Avi Kivity]

On 05/31/2011 09:48 PM, Marcelo Tosatti wrote:
> On Tue, May 31, 2011 at 09:05:35PM +0300, Avi Kivity wrote:
> >  >>    	if (is_long_mode(vcpu)) {
> >  >>    		if (!(cr4&   X86_CR4_PAE))
> >  >>    			return 1;
> >  >
> >  >A new field in vcpu->arch.mmu.base_role for smep is required
> >  >for shadow MMU (similar to nxe).
> >
> >  I plan to add that with my cr0.wp=0 fixup (it's only needed there, right?)
>
> Sptes instantiated when cr4.smep = 0 should not be used when cr4.smep =
> 1, so no (unlikely that guest kernel executes user=1 code anyway, but
> for consistency with other base_role flags).

Why not?  The sptes are interpreted exactly the same.

sptes are interpreted differently when efer.nxe=1 - if bit 63 is set, it 
will fault when nxe=0 and will not fault when nxe=1 (for non-fetch 
accesses).  So we can't share those sptes.

> OK then, you'll fix that.
>

Sure.  I'll post the patches as soon as this hits 'next'.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Patch v4 2/4] Add SMEP handling when setting CR4

[Marcelo Tosatti]

On Tue, May 31, 2011 at 10:03:26PM +0300, Avi Kivity wrote:
> On 05/31/2011 09:48 PM, Marcelo Tosatti wrote:
> >On Tue, May 31, 2011 at 09:05:35PM +0300, Avi Kivity wrote:
> >>  >>    	if (is_long_mode(vcpu)) {
> >>  >>    		if (!(cr4&   X86_CR4_PAE))
> >>  >>    			return 1;
> >>  >
> >>  >A new field in vcpu->arch.mmu.base_role for smep is required
> >>  >for shadow MMU (similar to nxe).
> >>
> >>  I plan to add that with my cr0.wp=0 fixup (it's only needed there, right?)
> >
> >Sptes instantiated when cr4.smep = 0 should not be used when cr4.smep =
> >1, so no (unlikely that guest kernel executes user=1 code anyway, but
> >for consistency with other base_role flags).
> 
> Why not?  The sptes are interpreted exactly the same.
> 
> sptes are interpreted differently when efer.nxe=1 - if bit 63 is
> set, it will fault when nxe=0 and will not fault when nxe=1 (for
> non-fetch accesses).  So we can't share those sptes.

A) CR4.SMEP = 0, spte instantiated via fetch fault of user pte.
B) CR4.SMEP = 1, base_role unchanged.
C) spte instantiated in A) used, but access should fault instead.

Or if you have multiple CPUs with different settings.

> >OK then, you'll fix that.
> >
> 
> Sure.  I'll post the patches as soon as this hits 'next'.
> 
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v4 2/4] Add SMEP handling when setting CR4

[Avi Kivity]

On 06/01/2011 03:32 PM, Marcelo Tosatti wrote:
> On Tue, May 31, 2011 at 10:03:26PM +0300, Avi Kivity wrote:
> >  On 05/31/2011 09:48 PM, Marcelo Tosatti wrote:
> >  >On Tue, May 31, 2011 at 09:05:35PM +0300, Avi Kivity wrote:
> >  >>   >>     	if (is_long_mode(vcpu)) {
> >  >>   >>     		if (!(cr4&    X86_CR4_PAE))
> >  >>   >>     			return 1;
> >  >>   >
> >  >>   >A new field in vcpu->arch.mmu.base_role for smep is required
> >  >>   >for shadow MMU (similar to nxe).
> >  >>
> >  >>   I plan to add that with my cr0.wp=0 fixup (it's only needed there, right?)
> >  >
> >  >Sptes instantiated when cr4.smep = 0 should not be used when cr4.smep =
> >  >1, so no (unlikely that guest kernel executes user=1 code anyway, but
> >  >for consistency with other base_role flags).
> >
> >  Why not?  The sptes are interpreted exactly the same.
> >
> >  sptes are interpreted differently when efer.nxe=1 - if bit 63 is
> >  set, it will fault when nxe=0 and will not fault when nxe=1 (for
> >  non-fetch accesses).  So we can't share those sptes.
>
> A) CR4.SMEP = 0, spte instantiated via fetch fault of user pte.
> B) CR4.SMEP = 1, base_role unchanged.
> C) spte instantiated in A) used, but access should fault instead.
>
> Or if you have multiple CPUs with different settings.

Why would C) not fault?  The spte has U=1, any fetch access from kernel 
mode with SMEP=1 will fault.

-- 
error compiling committee.c: too many arguments to function