[PATCH 0/2] Small fixes for IA32_FEATURE_CONTROL

[PATCH 0/2] Small fixes for IA32_FEATURE_CONTROL

[Paolo Bonzini]

Fix reading/writing MSR_IA32_FEATURE_CONTROL on AMD, and zero it on reset.

Paolo Bonzini (2):
  KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c
  KVM: x86: zero MSR_IA32_FEATURE_CONTROL on reset

 arch/x86/include/asm/kvm_host.h |  8 ++++++++
 arch/x86/kvm/vmx.c              | 41 ++++++++++-------------------------------
 arch/x86/kvm/x86.c              | 12 ++++++++++++
 arch/x86/kvm/x86.h              |  8 ++++++++
 4 files changed, 38 insertions(+), 31 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c

[Paolo Bonzini]

Because the MSR is listed in msrs_to_save, it is exported to userspace
for both AMD and Intel processors.  However, on AMD currently getting
it will fail.

vmx_set_msr must keep the case label in order to handle the "exit
nested on reset by writing 0" case.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  8 ++++++++
 arch/x86/kvm/vmx.c              | 41 ++++++++++-------------------------------
 arch/x86/kvm/x86.c              | 11 +++++++++++
 arch/x86/kvm/x86.h              |  8 ++++++++
 4 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index b771667e8e99..f77e5f5a6b01 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -486,6 +486,14 @@ struct kvm_vcpu_arch {
 	u64 ia32_xss;
 
 	/*
+	 * Only bits masked by msr_ia32_feature_control_valid_bits can be set in
+	 * msr_ia32_feature_control. FEATURE_CONTROL_LOCKED is always included
+	 * in msr_ia32_feature_control_valid_bits.
+	 */
+	u64 msr_ia32_feature_control;
+	u64 msr_ia32_feature_control_valid_bits;
+
+	/*
 	 * Paging state of the vcpu
 	 *
 	 * If the vcpu runs in guest mode with two level paging this still saves
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 15793a4c5df0..939cd8b835c2 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -611,14 +611,6 @@ struct vcpu_vmx {
 	bool guest_pkru_valid;
 	u32 guest_pkru;
 	u32 host_pkru;
-
-	/*
-	 * Only bits masked by msr_ia32_feature_control_valid_bits can be set in
-	 * msr_ia32_feature_control. FEATURE_CONTROL_LOCKED is always included
-	 * in msr_ia32_feature_control_valid_bits.
-	 */
-	u64 msr_ia32_feature_control;
-	u64 msr_ia32_feature_control_valid_bits;
 };
 
 enum segment_cache_field {
@@ -2932,14 +2924,6 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 	return 0;
 }
 
-static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu,
-						 uint64_t val)
-{
-	uint64_t valid_bits = to_vmx(vcpu)->msr_ia32_feature_control_valid_bits;
-
-	return !(val & ~valid_bits);
-}
-
 /*
  * Reads an msr value (of 'msr_index') into 'pdata'.
  * Returns 0 on success, non-0 otherwise.
@@ -2983,14 +2967,11 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_IA32_MCG_EXT_CTL:
 		if (!msr_info->host_initiated &&
-		    !(to_vmx(vcpu)->msr_ia32_feature_control &
+		    !(vcpu->arch.msr_ia32_feature_control &
 		      FEATURE_CONTROL_LMCE))
 			return 1;
 		msr_info->data = vcpu->arch.mcg_ext_ctl;
 		break;
-	case MSR_IA32_FEATURE_CONTROL:
-		msr_info->data = to_vmx(vcpu)->msr_ia32_feature_control;
-		break;
 	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
 		if (!nested_vmx_allowed(vcpu))
 			return 1;
@@ -3081,18 +3062,18 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_IA32_MCG_EXT_CTL:
 		if ((!msr_info->host_initiated &&
-		     !(to_vmx(vcpu)->msr_ia32_feature_control &
+		     !(vcpu->arch.msr_ia32_feature_control &
 		       FEATURE_CONTROL_LMCE)) ||
 		    (data & ~MCG_EXT_CTL_LMCE_EN))
 			return 1;
 		vcpu->arch.mcg_ext_ctl = data;
 		break;
 	case MSR_IA32_FEATURE_CONTROL:
-		if (!vmx_feature_control_msr_valid(vcpu, data) ||
-		    (to_vmx(vcpu)->msr_ia32_feature_control &
+		if (!feature_control_msr_valid(vcpu, data) ||
+		    (vcpu->arch.msr_ia32_feature_control &
 		     FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
 			return 1;
-		vmx->msr_ia32_feature_control = data;
+		vcpu->arch.msr_ia32_feature_control = data;
 		if (msr_info->host_initiated && data == 0)
 			vmx_leave_nested(vcpu);
 		break;
@@ -6964,7 +6945,7 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
 		return 1;
 	}
 
-	if ((vmx->msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
+	if ((vcpu->arch.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
 			!= VMXON_NEEDED_FEATURES) {
 		kvm_inject_gp(vcpu, 0);
 		return 1;
@@ -9081,8 +9062,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 			goto free_vmcs;
 	}
 
-	vmx->msr_ia32_feature_control_valid_bits = FEATURE_CONTROL_LOCKED;
-
 	return &vmx->vcpu;
 
 free_vmcs:
@@ -9232,10 +9211,10 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 	}
 
 	if (nested_vmx_allowed(vcpu))
-		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |=
+		vcpu->arch.msr_ia32_feature_control_valid_bits |=
 			FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
 	else
-		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits &=
+		vcpu->arch.msr_ia32_feature_control_valid_bits &=
 			~FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
 }
 
@@ -11135,10 +11114,10 @@ out:
 static void vmx_setup_mce(struct kvm_vcpu *vcpu)
 {
 	if (vcpu->arch.mcg_cap & MCG_LMCE_P)
-		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |=
+		vcpu->arch.msr_ia32_feature_control_valid_bits |=
 			FEATURE_CONTROL_LMCE;
 	else
-		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits &=
+		vcpu->arch.msr_ia32_feature_control_valid_bits &=
 			~FEATURE_CONTROL_LMCE;
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 411f786950ab..388d9ffd7551 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2097,6 +2097,13 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_TSCDEADLINE:
 		kvm_set_lapic_tscdeadline_msr(vcpu, data);
 		break;
+	case MSR_IA32_FEATURE_CONTROL:
+		if (!feature_control_msr_valid(vcpu, data) ||
+		    (vcpu->arch.msr_ia32_feature_control &
+		     FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
+			return 1;
+		vcpu->arch.msr_ia32_feature_control = data;
+		break;
 	case MSR_IA32_TSC_ADJUST:
 		if (guest_cpuid_has_tsc_adjust(vcpu)) {
 			if (!msr_info->host_initiated) {
@@ -2364,6 +2371,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_TSC_ADJUST:
 		msr_info->data = (u64)vcpu->arch.ia32_tsc_adjust_msr;
 		break;
+	case MSR_IA32_FEATURE_CONTROL:
+		msr_info->data = vcpu->arch.msr_ia32_feature_control;
+		break;
 	case MSR_IA32_MISC_ENABLE:
 		msr_info->data = vcpu->arch.ia32_misc_enable_msr;
 		break;
@@ -7705,6 +7715,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 
 	vcpu->arch.ia32_tsc_adjust_msr = 0x0;
 	vcpu->arch.pv_time_enabled = false;
+	vcpu->arch.msr_ia32_feature_control_valid_bits = FEATURE_CONTROL_LOCKED;
 
 	vcpu->arch.guest_supported_xcr0 = 0;
 	vcpu->arch.guest_xstate_size = XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET;
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 7ce3634ab5fe..a46c43aa762c 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -157,6 +157,14 @@ static inline bool kvm_check_has_quirk(struct kvm *kvm, u64 quirk)
 	return !(kvm->arch.disabled_quirks & quirk);
 }
 
+static inline bool feature_control_msr_valid(struct kvm_vcpu *vcpu,
+						 uint64_t val)
+{
+	uint64_t valid_bits = vcpu->arch.msr_ia32_feature_control_valid_bits;
+
+	return !(val & ~valid_bits);
+}
+
 void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
 void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
 void kvm_set_pending_timer(struct kvm_vcpu *vcpu);
-- 
1.8.3.1

[PATCH 2/2] KVM: x86: zero MSR_IA32_FEATURE_CONTROL on reset

[Paolo Bonzini]

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 388d9ffd7551..c01b0b3a06aa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7494,6 +7494,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	if (!init_event) {
 		kvm_pmu_reset(vcpu);
 		vcpu->arch.smbase = 0x30000;
+		vcpu->arch.msr_ia32_feature_control = 0;
 	}
 
 	memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
-- 
1.8.3.1

Re: [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c

[Haozhong Zhang]

On 07/08/16 14:01, Paolo Bonzini wrote:
> Because the MSR is listed in msrs_to_save, it is exported to userspace
> for both AMD and Intel processors.  However, on AMD currently getting
> it will fail.
>

Is MSR_IA32_FEATURE_CONTROL already be excluded from msrs_to_save[] by
kvm_init_msr_list() on AMD hosts?

Haozhong

> vmx_set_msr must keep the case label in order to handle the "exit
> nested on reset by writing 0" case.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  8 ++++++++
>  arch/x86/kvm/vmx.c              | 41 ++++++++++-------------------------------
>  arch/x86/kvm/x86.c              | 11 +++++++++++
>  arch/x86/kvm/x86.h              |  8 ++++++++
>  4 files changed, 37 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index b771667e8e99..f77e5f5a6b01 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -486,6 +486,14 @@ struct kvm_vcpu_arch {
>  	u64 ia32_xss;
>  
>  	/*
> +	 * Only bits masked by msr_ia32_feature_control_valid_bits can be set in
> +	 * msr_ia32_feature_control. FEATURE_CONTROL_LOCKED is always included
> +	 * in msr_ia32_feature_control_valid_bits.
> +	 */
> +	u64 msr_ia32_feature_control;
> +	u64 msr_ia32_feature_control_valid_bits;
> +
> +	/*
>  	 * Paging state of the vcpu
>  	 *
>  	 * If the vcpu runs in guest mode with two level paging this still saves
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 15793a4c5df0..939cd8b835c2 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -611,14 +611,6 @@ struct vcpu_vmx {
>  	bool guest_pkru_valid;
>  	u32 guest_pkru;
>  	u32 host_pkru;
> -
> -	/*
> -	 * Only bits masked by msr_ia32_feature_control_valid_bits can be set in
> -	 * msr_ia32_feature_control. FEATURE_CONTROL_LOCKED is always included
> -	 * in msr_ia32_feature_control_valid_bits.
> -	 */
> -	u64 msr_ia32_feature_control;
> -	u64 msr_ia32_feature_control_valid_bits;
>  };
>  
>  enum segment_cache_field {
> @@ -2932,14 +2924,6 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>  	return 0;
>  }
>  
> -static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu,
> -						 uint64_t val)
> -{
> -	uint64_t valid_bits = to_vmx(vcpu)->msr_ia32_feature_control_valid_bits;
> -
> -	return !(val & ~valid_bits);
> -}
> -
>  /*
>   * Reads an msr value (of 'msr_index') into 'pdata'.
>   * Returns 0 on success, non-0 otherwise.
> @@ -2983,14 +2967,11 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		break;
>  	case MSR_IA32_MCG_EXT_CTL:
>  		if (!msr_info->host_initiated &&
> -		    !(to_vmx(vcpu)->msr_ia32_feature_control &
> +		    !(vcpu->arch.msr_ia32_feature_control &
>  		      FEATURE_CONTROL_LMCE))
>  			return 1;
>  		msr_info->data = vcpu->arch.mcg_ext_ctl;
>  		break;
> -	case MSR_IA32_FEATURE_CONTROL:
> -		msr_info->data = to_vmx(vcpu)->msr_ia32_feature_control;
> -		break;
>  	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
>  		if (!nested_vmx_allowed(vcpu))
>  			return 1;
> @@ -3081,18 +3062,18 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		break;
>  	case MSR_IA32_MCG_EXT_CTL:
>  		if ((!msr_info->host_initiated &&
> -		     !(to_vmx(vcpu)->msr_ia32_feature_control &
> +		     !(vcpu->arch.msr_ia32_feature_control &
>  		       FEATURE_CONTROL_LMCE)) ||
>  		    (data & ~MCG_EXT_CTL_LMCE_EN))
>  			return 1;
>  		vcpu->arch.mcg_ext_ctl = data;
>  		break;
>  	case MSR_IA32_FEATURE_CONTROL:
> -		if (!vmx_feature_control_msr_valid(vcpu, data) ||
> -		    (to_vmx(vcpu)->msr_ia32_feature_control &
> +		if (!feature_control_msr_valid(vcpu, data) ||
> +		    (vcpu->arch.msr_ia32_feature_control &
>  		     FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
>  			return 1;
> -		vmx->msr_ia32_feature_control = data;
> +		vcpu->arch.msr_ia32_feature_control = data;
>  		if (msr_info->host_initiated && data == 0)
>  			vmx_leave_nested(vcpu);
>  		break;
> @@ -6964,7 +6945,7 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
>  		return 1;
>  	}
>  
> -	if ((vmx->msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
> +	if ((vcpu->arch.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
>  			!= VMXON_NEEDED_FEATURES) {
>  		kvm_inject_gp(vcpu, 0);
>  		return 1;
> @@ -9081,8 +9062,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>  			goto free_vmcs;
>  	}
>  
> -	vmx->msr_ia32_feature_control_valid_bits = FEATURE_CONTROL_LOCKED;
> -
>  	return &vmx->vcpu;
>  
>  free_vmcs:
> @@ -9232,10 +9211,10 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
>  	}
>  
>  	if (nested_vmx_allowed(vcpu))
> -		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |=
> +		vcpu->arch.msr_ia32_feature_control_valid_bits |=
>  			FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
>  	else
> -		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits &=
> +		vcpu->arch.msr_ia32_feature_control_valid_bits &=
>  			~FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
>  }
>  
> @@ -11135,10 +11114,10 @@ out:
>  static void vmx_setup_mce(struct kvm_vcpu *vcpu)
>  {
>  	if (vcpu->arch.mcg_cap & MCG_LMCE_P)
> -		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |=
> +		vcpu->arch.msr_ia32_feature_control_valid_bits |=
>  			FEATURE_CONTROL_LMCE;
>  	else
> -		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits &=
> +		vcpu->arch.msr_ia32_feature_control_valid_bits &=
>  			~FEATURE_CONTROL_LMCE;
>  }
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 411f786950ab..388d9ffd7551 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2097,6 +2097,13 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_TSCDEADLINE:
>  		kvm_set_lapic_tscdeadline_msr(vcpu, data);
>  		break;
> +	case MSR_IA32_FEATURE_CONTROL:
> +		if (!feature_control_msr_valid(vcpu, data) ||
> +		    (vcpu->arch.msr_ia32_feature_control &
> +		     FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
> +			return 1;
> +		vcpu->arch.msr_ia32_feature_control = data;
> +		break;
>  	case MSR_IA32_TSC_ADJUST:
>  		if (guest_cpuid_has_tsc_adjust(vcpu)) {
>  			if (!msr_info->host_initiated) {
> @@ -2364,6 +2371,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_TSC_ADJUST:
>  		msr_info->data = (u64)vcpu->arch.ia32_tsc_adjust_msr;
>  		break;
> +	case MSR_IA32_FEATURE_CONTROL:
> +		msr_info->data = vcpu->arch.msr_ia32_feature_control;
> +		break;
>  	case MSR_IA32_MISC_ENABLE:
>  		msr_info->data = vcpu->arch.ia32_misc_enable_msr;
>  		break;
> @@ -7705,6 +7715,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  
>  	vcpu->arch.ia32_tsc_adjust_msr = 0x0;
>  	vcpu->arch.pv_time_enabled = false;
> +	vcpu->arch.msr_ia32_feature_control_valid_bits = FEATURE_CONTROL_LOCKED;
>  
>  	vcpu->arch.guest_supported_xcr0 = 0;
>  	vcpu->arch.guest_xstate_size = XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET;
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 7ce3634ab5fe..a46c43aa762c 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -157,6 +157,14 @@ static inline bool kvm_check_has_quirk(struct kvm *kvm, u64 quirk)
>  	return !(kvm->arch.disabled_quirks & quirk);
>  }
>  
> +static inline bool feature_control_msr_valid(struct kvm_vcpu *vcpu,
> +						 uint64_t val)
> +{
> +	uint64_t valid_bits = vcpu->arch.msr_ia32_feature_control_valid_bits;
> +
> +	return !(val & ~valid_bits);
> +}
> +
>  void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
>  void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
>  void kvm_set_pending_timer(struct kvm_vcpu *vcpu);
> -- 
> 1.8.3.1
> 
> 

Re: [PATCH 2/2] KVM: x86: zero MSR_IA32_FEATURE_CONTROL on reset

[Haozhong Zhang]

On 07/08/16 14:01, Paolo Bonzini wrote:
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 388d9ffd7551..c01b0b3a06aa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7494,6 +7494,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	if (!init_event) {
>  		kvm_pmu_reset(vcpu);
>  		vcpu->arch.smbase = 0x30000;
> +		vcpu->arch.msr_ia32_feature_control = 0;
>  	}
>  
>  	memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
> -- 
> 1.8.3.1
> 

Actually I'm not sure whether zeroing MSR_IA32_FEATURE_CONTROL on
reset is the accurate behavior.

I know as Laszlo reported that Intel SDM says

  VMXON is also controlled by the IA32_FEATURE_CONTROL MSR (MSR
  address 3AH). This MSR is *cleared to zero* when a logical processor
  is reset

However, when the later section "ARCHITECTURAL MSRS" in Chapter
"MODEL-SPECIFIC REGISTERS (MSRS)" explains the Lock bit of
MSR_IA32_FEATURE_CONTROL in Table "IA-32 Architectural MSRs", it says

  ... once the Lock bit is set, the entire IA32_FEATURE_CONTROL
  contents are *preserved* across RESET when PWRGOOD is not
  deasserted.

This looks like a conflict. I'm asking my Intel colleagues for the
accurate behavior.

Thanks,
Haozhong

Re: [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c

[Radim Krčmář]

2016-07-08 14:01+0200, Paolo Bonzini:
> Because the MSR is listed in msrs_to_save, it is exported to userspace
> for both AMD and Intel processors.  However, on AMD currently getting
> it will fail.
> 
> vmx_set_msr must keep the case label in order to handle the "exit
> nested on reset by writing 0" case.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -3081,18 +3062,18 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_FEATURE_CONTROL:
> -		if (!vmx_feature_control_msr_valid(vcpu, data) ||
> -		    (to_vmx(vcpu)->msr_ia32_feature_control &
> +		if (!feature_control_msr_valid(vcpu, data) ||
> +		    (vcpu->arch.msr_ia32_feature_control &
>  		     FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
>  			return 1;
> -		vmx->msr_ia32_feature_control = data;
> +		vcpu->arch.msr_ia32_feature_control = data;
>  		if (msr_info->host_initiated && data == 0)
>  			vmx_leave_nested(vcpu);
>  		break;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -2097,6 +2097,13 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> +	case MSR_IA32_FEATURE_CONTROL:
> +		if (!feature_control_msr_valid(vcpu, data) ||
> +		    (vcpu->arch.msr_ia32_feature_control &
> +		     FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
> +			return 1;
> +		vcpu->arch.msr_ia32_feature_control = data;

I'd avoid code duplication.  Either with

  kvm_x86_ops->msr_ia32_feature_control_write_trap(vcpu);

here, or with (simpler, but slightly harder to untangle)

  ret = kvm_set_msr_common(vcpu, msr_info);

in before vmx_leave_nested() in vmx_set_msr().

> +		break;

Re: [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c

[Paolo Bonzini]

On 08/07/2016 14:41, Haozhong Zhang wrote:
> On 07/08/16 14:01, Paolo Bonzini wrote:
>> > Because the MSR is listed in msrs_to_save, it is exported to userspace
>> > for both AMD and Intel processors.  However, on AMD currently getting
>> > it will fail.
>> >
> Is MSR_IA32_FEATURE_CONTROL already be excluded from msrs_to_save[] by
> kvm_init_msr_list() on AMD hosts?

Uhm... you're right, because the MSR doesn't exist on AMD hosts.

Facepalm...

Paolo