* [PATCH 0/2] Small fixes for IA32_FEATURE_CONTROL @ 2016-07-08 12:01 Paolo Bonzini 2016-07-08 12:01 ` [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c Paolo Bonzini 2016-07-08 12:01 ` [PATCH 2/2] KVM: x86: zero MSR_IA32_FEATURE_CONTROL on reset Paolo Bonzini 0 siblings, 2 replies; 7+ messages in thread From: Paolo Bonzini @ 2016-07-08 12:01 UTC (permalink / raw) To: linux-kernel, kvm; +Cc: rkrcmar, Haozhong Zhang 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c 2016-07-08 12:01 [PATCH 0/2] Small fixes for IA32_FEATURE_CONTROL Paolo Bonzini @ 2016-07-08 12:01 ` Paolo Bonzini 2016-07-08 12:41 ` Haozhong Zhang 2016-07-08 12:59 ` Radim Krčmář 2016-07-08 12:01 ` [PATCH 2/2] KVM: x86: zero MSR_IA32_FEATURE_CONTROL on reset Paolo Bonzini 1 sibling, 2 replies; 7+ messages in thread From: Paolo Bonzini @ 2016-07-08 12:01 UTC (permalink / raw) To: linux-kernel, kvm; +Cc: rkrcmar, Haozhong Zhang 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c 2016-07-08 12:01 ` [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c Paolo Bonzini @ 2016-07-08 12:41 ` Haozhong Zhang 2016-07-08 13:24 ` Paolo Bonzini 2016-07-08 12:59 ` Radim Krčmář 1 sibling, 1 reply; 7+ messages in thread From: Haozhong Zhang @ 2016-07-08 12:41 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm, rkrcmar 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c 2016-07-08 12:41 ` Haozhong Zhang @ 2016-07-08 13:24 ` Paolo Bonzini 0 siblings, 0 replies; 7+ messages in thread From: Paolo Bonzini @ 2016-07-08 13:24 UTC (permalink / raw) To: linux-kernel, kvm, rkrcmar 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c 2016-07-08 12:01 ` [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c Paolo Bonzini 2016-07-08 12:41 ` Haozhong Zhang @ 2016-07-08 12:59 ` Radim Krčmář 1 sibling, 0 replies; 7+ messages in thread From: Radim Krčmář @ 2016-07-08 12:59 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm, Haozhong Zhang 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] KVM: x86: zero MSR_IA32_FEATURE_CONTROL on reset 2016-07-08 12:01 [PATCH 0/2] Small fixes for IA32_FEATURE_CONTROL Paolo Bonzini 2016-07-08 12:01 ` [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c Paolo Bonzini @ 2016-07-08 12:01 ` Paolo Bonzini 2016-07-08 12:52 ` Haozhong Zhang 1 sibling, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2016-07-08 12:01 UTC (permalink / raw) To: linux-kernel, kvm; +Cc: rkrcmar, Haozhong Zhang 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] KVM: x86: zero MSR_IA32_FEATURE_CONTROL on reset 2016-07-08 12:01 ` [PATCH 2/2] KVM: x86: zero MSR_IA32_FEATURE_CONTROL on reset Paolo Bonzini @ 2016-07-08 12:52 ` Haozhong Zhang 0 siblings, 0 replies; 7+ messages in thread From: Haozhong Zhang @ 2016-07-08 12:52 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm, rkrcmar 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-07-08 13:24 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-08 12:01 [PATCH 0/2] Small fixes for IA32_FEATURE_CONTROL Paolo Bonzini 2016-07-08 12:01 ` [PATCH 1/2] KVM: x86: move MSR_IA32_FEATURE_CONTROL handling to x86.c Paolo Bonzini 2016-07-08 12:41 ` Haozhong Zhang 2016-07-08 13:24 ` Paolo Bonzini 2016-07-08 12:59 ` Radim Krčmář 2016-07-08 12:01 ` [PATCH 2/2] KVM: x86: zero MSR_IA32_FEATURE_CONTROL on reset Paolo Bonzini 2016-07-08 12:52 ` Haozhong Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox