* [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
* [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 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 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
* 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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.