diff mbox

[3/4] KVM/X86: Enable Intel MPX for guest

Message ID DE8DF0795D48FD4CA783C40EC8292335013EF24A@SHSMSX101.ccr.corp.intel.com
State New
Headers show

Commit Message

Liu, Jinsong Nov. 29, 2013, 1:43 p.m. UTC 
From 11ae33723027c7b8e53a8c109f127800d7f0ad6e Mon Sep 17 00:00:00 2001
From: Liu Jinsong <jinsong.liu@intel.com>
Date: Fri, 29 Nov 2013 01:28:19 +0800
Subject: [PATCH 3/4] KVM/X86: Enable Intel MPX for guest

Enable Intel Memory Protection Extension for guest.

Signed-off-by: Xudong Hao <xudong.hao@intel.com>
Reviewed-by: Liu Jinsong <jinsong.liu@intel.com>
---
 arch/x86/kvm/cpuid.c |    4 ++--
 arch/x86/kvm/x86.c   |   14 ++++++++++++--
 arch/x86/kvm/x86.h   |    3 ++-
 3 files changed, 16 insertions(+), 5 deletions(-)

Comments

Paolo Bonzini Nov. 29, 2013, 2:33 p.m. UTC | #1 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a8ce117..e30d4ce 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -75,7 +75,7 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>  			(best->eax | ((u64)best->edx << 32)) &
>  			host_xcr0 & KVM_SUPPORTED_XCR0;
>  		vcpu->arch.guest_xstate_size = best->ebx =
> -			xstate_required_size(vcpu->arch.guest_supported_xcr0);
> +			xstate_required_size(vcpu->arch.xcr0);
>  	}
>  
>  	kvm_pmu_cpuid_update(vcpu);
> ...
>  	kvm_put_guest_xcr0(vcpu);
>  	vcpu->arch.xcr0 = xcr0;
> +
> +	if ((xcr0 ^ old_xcr0) & XSTATE_EXTEND_MASK)
> +		kvm_update_cpuid(vcpu);
> +
>  	return 0;
>  }

These hunks should be part of the previous patch.

> @@ -5960,6 +5967,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	preempt_disable();
>  
>  	kvm_x86_ops->prepare_guest_switch(vcpu);
> +	if (kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE) &&

Shouldn't be necessary, setting xcr0 fails unless OSXSAVE=1.

> +		(vcpu->arch.xcr0 & (u64)(XSTATE_BNDREGS | XSTATE_BNDCSR)))
> +		kvm_x86_ops->fpu_activate(vcpu);

Can you explain this?

>  	if (vcpu->fpu_active)
>  		kvm_load_guest_fpu(vcpu);
>  	kvm_load_guest_xcr0(vcpu);
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 587fb9e..985e40e 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -122,7 +122,8 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
>  	gva_t addr, void *val, unsigned int bytes,
>  	struct x86_exception *exception);
>  
> -#define KVM_SUPPORTED_XCR0	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
> +#define KVM_SUPPORTED_XCR0	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM \
> +				| XSTATE_BNDREGS | XSTATE_BNDCSR)
>  extern u64 host_xcr0;
>  
>  extern struct static_key kvm_no_apic_vcpu;
> 

Otherwise looks straightforward.

Paolo
Liu, Jinsong Nov. 29, 2013, 2:52 p.m. UTC | #2 
Paolo Bonzini wrote:
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index a8ce117..e30d4ce 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -75,7 +75,7 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>  			(best->eax | ((u64)best->edx << 32)) &
>>  			host_xcr0 & KVM_SUPPORTED_XCR0;
>>  		vcpu->arch.guest_xstate_size = best->ebx =
>> -			xstate_required_size(vcpu->arch.guest_supported_xcr0);
>> +			xstate_required_size(vcpu->arch.xcr0);
>>  	}
>> 
>>  	kvm_pmu_cpuid_update(vcpu);
>> ...
>>  	kvm_put_guest_xcr0(vcpu);
>>  	vcpu->arch.xcr0 = xcr0;
>> +
>> +	if ((xcr0 ^ old_xcr0) & XSTATE_EXTEND_MASK)
>> +		kvm_update_cpuid(vcpu);
>> +
>>  	return 0;
>>  }
> 
> These hunks should be part of the previous patch.
> 
>> @@ -5960,6 +5967,9 @@ static int vcpu_enter_guest(struct kvm_vcpu
>> *vcpu)  	preempt_disable(); 
>> 
>>  	kvm_x86_ops->prepare_guest_switch(vcpu);
>> +	if (kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE) &&
> 
> Shouldn't be necessary, setting xcr0 fails unless OSXSAVE=1.
> 
>> +		(vcpu->arch.xcr0 & (u64)(XSTATE_BNDREGS | XSTATE_BNDCSR)))
>> +		kvm_x86_ops->fpu_activate(vcpu);
> 
> Can you explain this?

No, in fact I'm also some wondering about it, but per it has been tested, I didn't update this code.
I will double check and drop it if need (or, maybe Xudong can elaborate more?)

Thanks,
Jinsong

> 
>>  	if (vcpu->fpu_active)
>>  		kvm_load_guest_fpu(vcpu);
>>  	kvm_load_guest_xcr0(vcpu);
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index 587fb9e..985e40e 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -122,7 +122,8 @@ int kvm_write_guest_virt_system(struct
>>  	x86_emulate_ctxt *ctxt, gva_t addr, void *val, unsigned int bytes,
>>  	struct x86_exception *exception);
>> 
>> -#define KVM_SUPPORTED_XCR0	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
>> +#define KVM_SUPPORTED_XCR0	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM \
>> +				| XSTATE_BNDREGS | XSTATE_BNDCSR)
>>  extern u64 host_xcr0;
>> 
>>  extern struct static_key kvm_no_apic_vcpu;
>> 
> 
> Otherwise looks straightforward.

Thanks, will update per your comments.
diff mbox

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index a8ce117..e30d4ce 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -75,7 +75,7 @@  void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 			(best->eax | ((u64)best->edx << 32)) &
 			host_xcr0 & KVM_SUPPORTED_XCR0;
 		vcpu->arch.guest_xstate_size = best->ebx =
-			xstate_required_size(vcpu->arch.guest_supported_xcr0);
+			xstate_required_size(vcpu->arch.xcr0);
 	}
 
 	kvm_pmu_cpuid_update(vcpu);
@@ -303,7 +303,7 @@  static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	/* cpuid 7.0.ebx */
 	const u32 kvm_supported_word9_x86_features =
 		F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
-		F(BMI2) | F(ERMS) | f_invpcid | F(RTM);
+		F(BMI2) | F(ERMS) | f_invpcid | F(RTM) | F(MPX);
 
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 21ef1ba..6e38698 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -576,13 +576,13 @@  static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
 
 int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 {
-	u64 xcr0;
+	u64 xcr0 = xcr;
+	u64 old_xcr0 = vcpu->arch.xcr0;
 	u64 valid_bits;
 
 	/* Only support XCR_XFEATURE_ENABLED_MASK(xcr0) now  */
 	if (index != XCR_XFEATURE_ENABLED_MASK)
 		return 1;
-	xcr0 = xcr;
 	if (!(xcr0 & XSTATE_FP))
 		return 1;
 	if ((xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE))
@@ -597,8 +597,15 @@  int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 	if (xcr0 & ~valid_bits)
 		return 1;
 
+	if ((!(xcr0 & XSTATE_BNDREGS)) != (!(xcr0 & XSTATE_BNDCSR)))
+		return 1;
+
 	kvm_put_guest_xcr0(vcpu);
 	vcpu->arch.xcr0 = xcr0;
+
+	if ((xcr0 ^ old_xcr0) & XSTATE_EXTEND_MASK)
+		kvm_update_cpuid(vcpu);
+
 	return 0;
 }
 
@@ -5960,6 +5967,9 @@  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	preempt_disable();
 
 	kvm_x86_ops->prepare_guest_switch(vcpu);
+	if (kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE) &&
+		(vcpu->arch.xcr0 & (u64)(XSTATE_BNDREGS | XSTATE_BNDCSR)))
+		kvm_x86_ops->fpu_activate(vcpu);
 	if (vcpu->fpu_active)
 		kvm_load_guest_fpu(vcpu);
 	kvm_load_guest_xcr0(vcpu);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 587fb9e..985e40e 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -122,7 +122,8 @@  int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
 	gva_t addr, void *val, unsigned int bytes,
 	struct x86_exception *exception);
 
-#define KVM_SUPPORTED_XCR0	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
+#define KVM_SUPPORTED_XCR0	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM \
+				| XSTATE_BNDREGS | XSTATE_BNDCSR)
 extern u64 host_xcr0;
 
 extern struct static_key kvm_no_apic_vcpu;