* [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL
@ 2013-07-05 15:13 Arthur Chunqi Li
2013-07-07 6:59 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: Arthur Chunqi Li @ 2013-07-05 15:13 UTC (permalink / raw)
To: kvm; +Cc: gleb, pbonzini, jan.kiszka, Arthur Chunqi Li
Fix read/write to IA32_FEATURE_CONTROL MSR in nested environment.
This patch simulate this MSR in nested_vmx and the default value is
0x0. BIOS should set it to 0x5 before VMXON. After setting the lock
bit, write to it will cause #GP(0).
Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
---
arch/x86/kvm/vmx.c | 25 +++++++++++++++++++++----
arch/x86/kvm/x86.c | 3 ++-
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 260a919..5e3d44e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -373,6 +373,7 @@ struct nested_vmx {
* we must keep them pinned while L2 runs.
*/
struct page *apic_access_page;
+ u64 msr_ia32_feature_control;
};
#define POSTED_INTR_ON 0
@@ -2277,8 +2278,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
switch (msr_index) {
case MSR_IA32_FEATURE_CONTROL:
- *pdata = 0;
- break;
+ if (nested_vmx_allowed(vcpu)){
+ *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control;
+ break;
+ }
+ return 0;
case MSR_IA32_VMX_BASIC:
/*
* This MSR reports some information about VMX support. We
@@ -2356,9 +2360,13 @@ static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
if (!nested_vmx_allowed(vcpu))
return 0;
- if (msr_index == MSR_IA32_FEATURE_CONTROL)
- /* TODO: the right thing. */
+ if (msr_index == MSR_IA32_FEATURE_CONTROL){
+ if (to_vmx(vcpu)->nested.msr_ia32_feature_control
+ & FEATURE_CONTROL_LOCKED)
+ return 0;
+ to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
return 1;
+ }
/*
* No need to treat VMX capability MSRs specially: If we don't handle
* them, handle_wrmsr will #GP(0), which is correct (they are readonly)
@@ -5595,6 +5603,15 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
skip_emulated_instruction(vcpu);
return 1;
}
+
+#define VMXON_NEEDED_FEATURES \
+ (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
+ if ((vmx->nested.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
+ != VMXON_NEEDED_FEATURES) {
+ kvm_inject_gp(vcpu, 0);
+ return 1;
+ }
+
if (enable_shadow_vmcs) {
shadow_vmcs = alloc_vmcs();
if (!shadow_vmcs)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e8ba99c..2d4eb8e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -850,7 +850,8 @@ static u32 msrs_to_save[] = {
#ifdef CONFIG_X86_64
MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
#endif
- MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
+ MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
+ MSR_IA32_FEATURE_CONTROL
};
static unsigned num_msrs_to_save;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL
2013-07-05 15:13 [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL Arthur Chunqi Li
@ 2013-07-07 6:59 ` Gleb Natapov
2013-07-07 7:16 ` Arthur Chunqi Li
0 siblings, 1 reply; 8+ messages in thread
From: Gleb Natapov @ 2013-07-07 6:59 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, pbonzini, jan.kiszka
On Fri, Jul 05, 2013 at 11:13:17PM +0800, Arthur Chunqi Li wrote:
> Fix read/write to IA32_FEATURE_CONTROL MSR in nested environment.
>
> This patch simulate this MSR in nested_vmx and the default value is
> 0x0. BIOS should set it to 0x5 before VMXON. After setting the lock
> bit, write to it will cause #GP(0).
>
> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
The patch is heavily based on Nadav's one. You need to preserve
authorship. Add From: and Nadav's Sign-off-by.
> ---
> arch/x86/kvm/vmx.c | 25 +++++++++++++++++++++----
> arch/x86/kvm/x86.c | 3 ++-
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 260a919..5e3d44e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -373,6 +373,7 @@ struct nested_vmx {
> * we must keep them pinned while L2 runs.
> */
> struct page *apic_access_page;
> + u64 msr_ia32_feature_control;
> };
>
> #define POSTED_INTR_ON 0
> @@ -2277,8 +2278,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>
> switch (msr_index) {
> case MSR_IA32_FEATURE_CONTROL:
> - *pdata = 0;
> - break;
> + if (nested_vmx_allowed(vcpu)){
> + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control;
> + break;
> + }
> + return 0;
> case MSR_IA32_VMX_BASIC:
> /*
> * This MSR reports some information about VMX support. We
> @@ -2356,9 +2360,13 @@ static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
> if (!nested_vmx_allowed(vcpu))
> return 0;
>
> - if (msr_index == MSR_IA32_FEATURE_CONTROL)
> - /* TODO: the right thing. */
> + if (msr_index == MSR_IA32_FEATURE_CONTROL){
> + if (to_vmx(vcpu)->nested.msr_ia32_feature_control
> + & FEATURE_CONTROL_LOCKED)
> + return 0;
> + to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
You need to allow setting it from userspace, otherwise reset will not
clear it.
> return 1;
> + }
> /*
> * No need to treat VMX capability MSRs specially: If we don't handle
> * them, handle_wrmsr will #GP(0), which is correct (they are readonly)
> @@ -5595,6 +5603,15 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
> skip_emulated_instruction(vcpu);
> return 1;
> }
> +
> +#define VMXON_NEEDED_FEATURES \
Avi asked to use const u64 here.
> + (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
> + if ((vmx->nested.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
> + != VMXON_NEEDED_FEATURES) {
> + kvm_inject_gp(vcpu, 0);
> + return 1;
> + }
> +
> if (enable_shadow_vmcs) {
> shadow_vmcs = alloc_vmcs();
> if (!shadow_vmcs)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e8ba99c..2d4eb8e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -850,7 +850,8 @@ static u32 msrs_to_save[] = {
> #ifdef CONFIG_X86_64
> MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
> #endif
> - MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
> + MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> + MSR_IA32_FEATURE_CONTROL
> };
>
> static unsigned num_msrs_to_save;
> --
> 1.7.9.5
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL
2013-07-07 6:59 ` Gleb Natapov
@ 2013-07-07 7:16 ` Arthur Chunqi Li
2013-07-07 7:20 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: Arthur Chunqi Li @ 2013-07-07 7:16 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, Paolo Bonzini, Jan Kiszka
On Sun, Jul 7, 2013 at 2:59 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Fri, Jul 05, 2013 at 11:13:17PM +0800, Arthur Chunqi Li wrote:
>> Fix read/write to IA32_FEATURE_CONTROL MSR in nested environment.
>>
>> This patch simulate this MSR in nested_vmx and the default value is
>> 0x0. BIOS should set it to 0x5 before VMXON. After setting the lock
>> bit, write to it will cause #GP(0).
>>
>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> The patch is heavily based on Nadav's one. You need to preserve
> authorship. Add From: and Nadav's Sign-off-by.
>
>> ---
>> arch/x86/kvm/vmx.c | 25 +++++++++++++++++++++----
>> arch/x86/kvm/x86.c | 3 ++-
>> 2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 260a919..5e3d44e 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -373,6 +373,7 @@ struct nested_vmx {
>> * we must keep them pinned while L2 runs.
>> */
>> struct page *apic_access_page;
>> + u64 msr_ia32_feature_control;
>> };
>>
>> #define POSTED_INTR_ON 0
>> @@ -2277,8 +2278,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>>
>> switch (msr_index) {
>> case MSR_IA32_FEATURE_CONTROL:
>> - *pdata = 0;
>> - break;
>> + if (nested_vmx_allowed(vcpu)){
>> + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control;
>> + break;
>> + }
>> + return 0;
>> case MSR_IA32_VMX_BASIC:
>> /*
>> * This MSR reports some information about VMX support. We
>> @@ -2356,9 +2360,13 @@ static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
>> if (!nested_vmx_allowed(vcpu))
>> return 0;
>>
>> - if (msr_index == MSR_IA32_FEATURE_CONTROL)
>> - /* TODO: the right thing. */
>> + if (msr_index == MSR_IA32_FEATURE_CONTROL){
>> + if (to_vmx(vcpu)->nested.msr_ia32_feature_control
>> + & FEATURE_CONTROL_LOCKED)
>> + return 0;
>> + to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
> You need to allow setting it from userspace, otherwise reset will not
> clear it.
As to my memory, if the lock bit of IA32_FEATURE_CONTROL is set, this
MSR cannot be set or reset. So what do you mean setting it from
userspace?
>
>> return 1;
>> + }
>> /*
>> * No need to treat VMX capability MSRs specially: If we don't handle
>> * them, handle_wrmsr will #GP(0), which is correct (they are readonly)
>> @@ -5595,6 +5603,15 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
>> skip_emulated_instruction(vcpu);
>> return 1;
>> }
>> +
>> +#define VMXON_NEEDED_FEATURES \
> Avi asked to use const u64 here.
>
>> + (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
>> + if ((vmx->nested.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
>> + != VMXON_NEEDED_FEATURES) {
>> + kvm_inject_gp(vcpu, 0);
>> + return 1;
>> + }
>> +
>> if (enable_shadow_vmcs) {
>> shadow_vmcs = alloc_vmcs();
>> if (!shadow_vmcs)
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index e8ba99c..2d4eb8e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -850,7 +850,8 @@ static u32 msrs_to_save[] = {
>> #ifdef CONFIG_X86_64
>> MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
>> #endif
>> - MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
>> + MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
>> + MSR_IA32_FEATURE_CONTROL
>> };
>>
>> static unsigned num_msrs_to_save;
>> --
>> 1.7.9.5
>
> --
> Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL
2013-07-07 7:16 ` Arthur Chunqi Li
@ 2013-07-07 7:20 ` Gleb Natapov
2013-07-07 8:47 ` Arthur Chunqi Li
0 siblings, 1 reply; 8+ messages in thread
From: Gleb Natapov @ 2013-07-07 7:20 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Paolo Bonzini, Jan Kiszka
On Sun, Jul 07, 2013 at 03:16:08PM +0800, Arthur Chunqi Li wrote:
> On Sun, Jul 7, 2013 at 2:59 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Fri, Jul 05, 2013 at 11:13:17PM +0800, Arthur Chunqi Li wrote:
> >> Fix read/write to IA32_FEATURE_CONTROL MSR in nested environment.
> >>
> >> This patch simulate this MSR in nested_vmx and the default value is
> >> 0x0. BIOS should set it to 0x5 before VMXON. After setting the lock
> >> bit, write to it will cause #GP(0).
> >>
> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> > The patch is heavily based on Nadav's one. You need to preserve
> > authorship. Add From: and Nadav's Sign-off-by.
> >
> >> ---
> >> arch/x86/kvm/vmx.c | 25 +++++++++++++++++++++----
> >> arch/x86/kvm/x86.c | 3 ++-
> >> 2 files changed, 23 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index 260a919..5e3d44e 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -373,6 +373,7 @@ struct nested_vmx {
> >> * we must keep them pinned while L2 runs.
> >> */
> >> struct page *apic_access_page;
> >> + u64 msr_ia32_feature_control;
> >> };
> >>
> >> #define POSTED_INTR_ON 0
> >> @@ -2277,8 +2278,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
> >>
> >> switch (msr_index) {
> >> case MSR_IA32_FEATURE_CONTROL:
> >> - *pdata = 0;
> >> - break;
> >> + if (nested_vmx_allowed(vcpu)){
> >> + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control;
> >> + break;
> >> + }
> >> + return 0;
> >> case MSR_IA32_VMX_BASIC:
> >> /*
> >> * This MSR reports some information about VMX support. We
> >> @@ -2356,9 +2360,13 @@ static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
> >> if (!nested_vmx_allowed(vcpu))
> >> return 0;
> >>
> >> - if (msr_index == MSR_IA32_FEATURE_CONTROL)
> >> - /* TODO: the right thing. */
> >> + if (msr_index == MSR_IA32_FEATURE_CONTROL){
> >> + if (to_vmx(vcpu)->nested.msr_ia32_feature_control
> >> + & FEATURE_CONTROL_LOCKED)
> >> + return 0;
> >> + to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
> > You need to allow setting it from userspace, otherwise reset will not
> > clear it.
> As to my memory, if the lock bit of IA32_FEATURE_CONTROL is set, this
> MSR cannot be set or reset. So what do you mean setting it from
> userspace?
It can be reset by power cycling CPU obviously, so when userspace emulates
power cycle reset it has to be able to zero the MSR. BTW you need to write
corespondent QEMU patch to handle this MSR during reset and migration.
> >
> >> return 1;
> >> + }
> >> /*
> >> * No need to treat VMX capability MSRs specially: If we don't handle
> >> * them, handle_wrmsr will #GP(0), which is correct (they are readonly)
> >> @@ -5595,6 +5603,15 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
> >> skip_emulated_instruction(vcpu);
> >> return 1;
> >> }
> >> +
> >> +#define VMXON_NEEDED_FEATURES \
> > Avi asked to use const u64 here.
> >
> >> + (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
> >> + if ((vmx->nested.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
> >> + != VMXON_NEEDED_FEATURES) {
> >> + kvm_inject_gp(vcpu, 0);
> >> + return 1;
> >> + }
> >> +
> >> if (enable_shadow_vmcs) {
> >> shadow_vmcs = alloc_vmcs();
> >> if (!shadow_vmcs)
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index e8ba99c..2d4eb8e 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -850,7 +850,8 @@ static u32 msrs_to_save[] = {
> >> #ifdef CONFIG_X86_64
> >> MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
> >> #endif
> >> - MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
> >> + MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> >> + MSR_IA32_FEATURE_CONTROL
> >> };
> >>
> >> static unsigned num_msrs_to_save;
> >> --
> >> 1.7.9.5
> >
> > --
> > Gleb.
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL
2013-07-07 7:20 ` Gleb Natapov
@ 2013-07-07 8:47 ` Arthur Chunqi Li
2013-07-07 8:50 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: Arthur Chunqi Li @ 2013-07-07 8:47 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, Paolo Bonzini, Jan Kiszka
On Sun, Jul 7, 2013 at 3:20 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Sun, Jul 07, 2013 at 03:16:08PM +0800, Arthur Chunqi Li wrote:
>> On Sun, Jul 7, 2013 at 2:59 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> > On Fri, Jul 05, 2013 at 11:13:17PM +0800, Arthur Chunqi Li wrote:
>> >> Fix read/write to IA32_FEATURE_CONTROL MSR in nested environment.
>> >>
>> >> This patch simulate this MSR in nested_vmx and the default value is
>> >> 0x0. BIOS should set it to 0x5 before VMXON. After setting the lock
>> >> bit, write to it will cause #GP(0).
>> >>
>> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>> > The patch is heavily based on Nadav's one. You need to preserve
>> > authorship. Add From: and Nadav's Sign-off-by.
>> >
>> >> ---
>> >> arch/x86/kvm/vmx.c | 25 +++++++++++++++++++++----
>> >> arch/x86/kvm/x86.c | 3 ++-
>> >> 2 files changed, 23 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> >> index 260a919..5e3d44e 100644
>> >> --- a/arch/x86/kvm/vmx.c
>> >> +++ b/arch/x86/kvm/vmx.c
>> >> @@ -373,6 +373,7 @@ struct nested_vmx {
>> >> * we must keep them pinned while L2 runs.
>> >> */
>> >> struct page *apic_access_page;
>> >> + u64 msr_ia32_feature_control;
>> >> };
>> >>
>> >> #define POSTED_INTR_ON 0
>> >> @@ -2277,8 +2278,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>> >>
>> >> switch (msr_index) {
>> >> case MSR_IA32_FEATURE_CONTROL:
>> >> - *pdata = 0;
>> >> - break;
>> >> + if (nested_vmx_allowed(vcpu)){
>> >> + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control;
>> >> + break;
>> >> + }
>> >> + return 0;
>> >> case MSR_IA32_VMX_BASIC:
>> >> /*
>> >> * This MSR reports some information about VMX support. We
>> >> @@ -2356,9 +2360,13 @@ static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
>> >> if (!nested_vmx_allowed(vcpu))
>> >> return 0;
>> >>
>> >> - if (msr_index == MSR_IA32_FEATURE_CONTROL)
>> >> - /* TODO: the right thing. */
>> >> + if (msr_index == MSR_IA32_FEATURE_CONTROL){
>> >> + if (to_vmx(vcpu)->nested.msr_ia32_feature_control
>> >> + & FEATURE_CONTROL_LOCKED)
>> >> + return 0;
>> >> + to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
>> > You need to allow setting it from userspace, otherwise reset will not
>> > clear it.
>> As to my memory, if the lock bit of IA32_FEATURE_CONTROL is set, this
>> MSR cannot be set or reset. So what do you mean setting it from
>> userspace?
> It can be reset by power cycling CPU obviously, so when userspace emulates
> power cycle reset it has to be able to zero the MSR. BTW you need to write
> corespondent QEMU patch to handle this MSR during reset and migration.
What is the current call trace when QEMU do system_reset? Will it
trigger vmx_vcpu_reset() in vmx.c?
>
>> >
>> >> return 1;
>> >> + }
>> >> /*
>> >> * No need to treat VMX capability MSRs specially: If we don't handle
>> >> * them, handle_wrmsr will #GP(0), which is correct (they are readonly)
>> >> @@ -5595,6 +5603,15 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
>> >> skip_emulated_instruction(vcpu);
>> >> return 1;
>> >> }
>> >> +
>> >> +#define VMXON_NEEDED_FEATURES \
>> > Avi asked to use const u64 here.
>> >
>> >> + (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
>> >> + if ((vmx->nested.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
>> >> + != VMXON_NEEDED_FEATURES) {
>> >> + kvm_inject_gp(vcpu, 0);
>> >> + return 1;
>> >> + }
>> >> +
>> >> if (enable_shadow_vmcs) {
>> >> shadow_vmcs = alloc_vmcs();
>> >> if (!shadow_vmcs)
>> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> >> index e8ba99c..2d4eb8e 100644
>> >> --- a/arch/x86/kvm/x86.c
>> >> +++ b/arch/x86/kvm/x86.c
>> >> @@ -850,7 +850,8 @@ static u32 msrs_to_save[] = {
>> >> #ifdef CONFIG_X86_64
>> >> MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
>> >> #endif
>> >> - MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
>> >> + MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
>> >> + MSR_IA32_FEATURE_CONTROL
>> >> };
>> >>
>> >> static unsigned num_msrs_to_save;
>> >> --
>> >> 1.7.9.5
>> >
>> > --
>> > Gleb.
>
> --
> Gleb.
--
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL
2013-07-07 8:47 ` Arthur Chunqi Li
@ 2013-07-07 8:50 ` Gleb Natapov
2013-07-07 9:17 ` Arthur Chunqi Li
0 siblings, 1 reply; 8+ messages in thread
From: Gleb Natapov @ 2013-07-07 8:50 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Paolo Bonzini, Jan Kiszka
On Sun, Jul 07, 2013 at 04:47:05PM +0800, Arthur Chunqi Li wrote:
> On Sun, Jul 7, 2013 at 3:20 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Sun, Jul 07, 2013 at 03:16:08PM +0800, Arthur Chunqi Li wrote:
> >> On Sun, Jul 7, 2013 at 2:59 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >> > On Fri, Jul 05, 2013 at 11:13:17PM +0800, Arthur Chunqi Li wrote:
> >> >> Fix read/write to IA32_FEATURE_CONTROL MSR in nested environment.
> >> >>
> >> >> This patch simulate this MSR in nested_vmx and the default value is
> >> >> 0x0. BIOS should set it to 0x5 before VMXON. After setting the lock
> >> >> bit, write to it will cause #GP(0).
> >> >>
> >> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> >> > The patch is heavily based on Nadav's one. You need to preserve
> >> > authorship. Add From: and Nadav's Sign-off-by.
> >> >
> >> >> ---
> >> >> arch/x86/kvm/vmx.c | 25 +++++++++++++++++++++----
> >> >> arch/x86/kvm/x86.c | 3 ++-
> >> >> 2 files changed, 23 insertions(+), 5 deletions(-)
> >> >>
> >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> >> index 260a919..5e3d44e 100644
> >> >> --- a/arch/x86/kvm/vmx.c
> >> >> +++ b/arch/x86/kvm/vmx.c
> >> >> @@ -373,6 +373,7 @@ struct nested_vmx {
> >> >> * we must keep them pinned while L2 runs.
> >> >> */
> >> >> struct page *apic_access_page;
> >> >> + u64 msr_ia32_feature_control;
> >> >> };
> >> >>
> >> >> #define POSTED_INTR_ON 0
> >> >> @@ -2277,8 +2278,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
> >> >>
> >> >> switch (msr_index) {
> >> >> case MSR_IA32_FEATURE_CONTROL:
> >> >> - *pdata = 0;
> >> >> - break;
> >> >> + if (nested_vmx_allowed(vcpu)){
> >> >> + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control;
> >> >> + break;
> >> >> + }
> >> >> + return 0;
> >> >> case MSR_IA32_VMX_BASIC:
> >> >> /*
> >> >> * This MSR reports some information about VMX support. We
> >> >> @@ -2356,9 +2360,13 @@ static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
> >> >> if (!nested_vmx_allowed(vcpu))
> >> >> return 0;
> >> >>
> >> >> - if (msr_index == MSR_IA32_FEATURE_CONTROL)
> >> >> - /* TODO: the right thing. */
> >> >> + if (msr_index == MSR_IA32_FEATURE_CONTROL){
> >> >> + if (to_vmx(vcpu)->nested.msr_ia32_feature_control
> >> >> + & FEATURE_CONTROL_LOCKED)
> >> >> + return 0;
> >> >> + to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
> >> > You need to allow setting it from userspace, otherwise reset will not
> >> > clear it.
> >> As to my memory, if the lock bit of IA32_FEATURE_CONTROL is set, this
> >> MSR cannot be set or reset. So what do you mean setting it from
> >> userspace?
> > It can be reset by power cycling CPU obviously, so when userspace emulates
> > power cycle reset it has to be able to zero the MSR. BTW you need to write
> > corespondent QEMU patch to handle this MSR during reset and migration.
> What is the current call trace when QEMU do system_reset? Will it
> trigger vmx_vcpu_reset() in vmx.c?
It will not. QEMU inits vcpu state in userspace and write it into the
kernel with set_regs/set_srges ioctls.
> >
> >> >
> >> >> return 1;
> >> >> + }
> >> >> /*
> >> >> * No need to treat VMX capability MSRs specially: If we don't handle
> >> >> * them, handle_wrmsr will #GP(0), which is correct (they are readonly)
> >> >> @@ -5595,6 +5603,15 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
> >> >> skip_emulated_instruction(vcpu);
> >> >> return 1;
> >> >> }
> >> >> +
> >> >> +#define VMXON_NEEDED_FEATURES \
> >> > Avi asked to use const u64 here.
> >> >
> >> >> + (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
> >> >> + if ((vmx->nested.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
> >> >> + != VMXON_NEEDED_FEATURES) {
> >> >> + kvm_inject_gp(vcpu, 0);
> >> >> + return 1;
> >> >> + }
> >> >> +
> >> >> if (enable_shadow_vmcs) {
> >> >> shadow_vmcs = alloc_vmcs();
> >> >> if (!shadow_vmcs)
> >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> >> index e8ba99c..2d4eb8e 100644
> >> >> --- a/arch/x86/kvm/x86.c
> >> >> +++ b/arch/x86/kvm/x86.c
> >> >> @@ -850,7 +850,8 @@ static u32 msrs_to_save[] = {
> >> >> #ifdef CONFIG_X86_64
> >> >> MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
> >> >> #endif
> >> >> - MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
> >> >> + MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> >> >> + MSR_IA32_FEATURE_CONTROL
> >> >> };
> >> >>
> >> >> static unsigned num_msrs_to_save;
> >> >> --
> >> >> 1.7.9.5
> >> >
> >> > --
> >> > Gleb.
> >
> > --
> > Gleb.
>
>
>
> --
> Arthur Chunqi Li
> Department of Computer Science
> School of EECS
> Peking University
> Beijing, China
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL
2013-07-07 8:50 ` Gleb Natapov
@ 2013-07-07 9:17 ` Arthur Chunqi Li
2013-07-07 9:18 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: Arthur Chunqi Li @ 2013-07-07 9:17 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, Paolo Bonzini, Jan Kiszka
On Sun, Jul 7, 2013 at 4:50 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Sun, Jul 07, 2013 at 04:47:05PM +0800, Arthur Chunqi Li wrote:
>> On Sun, Jul 7, 2013 at 3:20 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> > On Sun, Jul 07, 2013 at 03:16:08PM +0800, Arthur Chunqi Li wrote:
>> >> On Sun, Jul 7, 2013 at 2:59 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> >> > On Fri, Jul 05, 2013 at 11:13:17PM +0800, Arthur Chunqi Li wrote:
>> >> >> Fix read/write to IA32_FEATURE_CONTROL MSR in nested environment.
>> >> >>
>> >> >> This patch simulate this MSR in nested_vmx and the default value is
>> >> >> 0x0. BIOS should set it to 0x5 before VMXON. After setting the lock
>> >> >> bit, write to it will cause #GP(0).
>> >> >>
>> >> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>> >> > The patch is heavily based on Nadav's one. You need to preserve
>> >> > authorship. Add From: and Nadav's Sign-off-by.
>> >> >
>> >> >> ---
>> >> >> arch/x86/kvm/vmx.c | 25 +++++++++++++++++++++----
>> >> >> arch/x86/kvm/x86.c | 3 ++-
>> >> >> 2 files changed, 23 insertions(+), 5 deletions(-)
>> >> >>
>> >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> >> >> index 260a919..5e3d44e 100644
>> >> >> --- a/arch/x86/kvm/vmx.c
>> >> >> +++ b/arch/x86/kvm/vmx.c
>> >> >> @@ -373,6 +373,7 @@ struct nested_vmx {
>> >> >> * we must keep them pinned while L2 runs.
>> >> >> */
>> >> >> struct page *apic_access_page;
>> >> >> + u64 msr_ia32_feature_control;
>> >> >> };
>> >> >>
>> >> >> #define POSTED_INTR_ON 0
>> >> >> @@ -2277,8 +2278,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>> >> >>
>> >> >> switch (msr_index) {
>> >> >> case MSR_IA32_FEATURE_CONTROL:
>> >> >> - *pdata = 0;
>> >> >> - break;
>> >> >> + if (nested_vmx_allowed(vcpu)){
>> >> >> + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control;
>> >> >> + break;
>> >> >> + }
>> >> >> + return 0;
>> >> >> case MSR_IA32_VMX_BASIC:
>> >> >> /*
>> >> >> * This MSR reports some information about VMX support. We
>> >> >> @@ -2356,9 +2360,13 @@ static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
>> >> >> if (!nested_vmx_allowed(vcpu))
>> >> >> return 0;
>> >> >>
>> >> >> - if (msr_index == MSR_IA32_FEATURE_CONTROL)
>> >> >> - /* TODO: the right thing. */
>> >> >> + if (msr_index == MSR_IA32_FEATURE_CONTROL){
>> >> >> + if (to_vmx(vcpu)->nested.msr_ia32_feature_control
>> >> >> + & FEATURE_CONTROL_LOCKED)
>> >> >> + return 0;
>> >> >> + to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
>> >> > You need to allow setting it from userspace, otherwise reset will not
>> >> > clear it.
>> >> As to my memory, if the lock bit of IA32_FEATURE_CONTROL is set, this
>> >> MSR cannot be set or reset. So what do you mean setting it from
>> >> userspace?
>> > It can be reset by power cycling CPU obviously, so when userspace emulates
>> > power cycle reset it has to be able to zero the MSR. BTW you need to write
>> > corespondent QEMU patch to handle this MSR during reset and migration.
>> What is the current call trace when QEMU do system_reset? Will it
>> trigger vmx_vcpu_reset() in vmx.c?
> It will not. QEMU inits vcpu state in userspace and write it into the
> kernel with set_regs/set_srges ioctls.
So how will this achieve isolation and protection? I mean if QEMU can
reset this MSR when reboot, can user in OS also reset it? Actually,
after setting to lock bit, it cannot be set until next reboot.
>
>> >
>> >> >
>> >> >> return 1;
>> >> >> + }
>> >> >> /*
>> >> >> * No need to treat VMX capability MSRs specially: If we don't handle
>> >> >> * them, handle_wrmsr will #GP(0), which is correct (they are readonly)
>> >> >> @@ -5595,6 +5603,15 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
>> >> >> skip_emulated_instruction(vcpu);
>> >> >> return 1;
>> >> >> }
>> >> >> +
>> >> >> +#define VMXON_NEEDED_FEATURES \
>> >> > Avi asked to use const u64 here.
>> >> >
>> >> >> + (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
>> >> >> + if ((vmx->nested.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
>> >> >> + != VMXON_NEEDED_FEATURES) {
>> >> >> + kvm_inject_gp(vcpu, 0);
>> >> >> + return 1;
>> >> >> + }
>> >> >> +
>> >> >> if (enable_shadow_vmcs) {
>> >> >> shadow_vmcs = alloc_vmcs();
>> >> >> if (!shadow_vmcs)
>> >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> >> >> index e8ba99c..2d4eb8e 100644
>> >> >> --- a/arch/x86/kvm/x86.c
>> >> >> +++ b/arch/x86/kvm/x86.c
>> >> >> @@ -850,7 +850,8 @@ static u32 msrs_to_save[] = {
>> >> >> #ifdef CONFIG_X86_64
>> >> >> MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
>> >> >> #endif
>> >> >> - MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
>> >> >> + MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
>> >> >> + MSR_IA32_FEATURE_CONTROL
>> >> >> };
>> >> >>
>> >> >> static unsigned num_msrs_to_save;
>> >> >> --
>> >> >> 1.7.9.5
>> >> >
>> >> > --
>> >> > Gleb.
>> >
>> > --
>> > Gleb.
>>
>>
>>
>> --
>> Arthur Chunqi Li
>> Department of Computer Science
>> School of EECS
>> Peking University
>> Beijing, China
>
> --
> Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL
2013-07-07 9:17 ` Arthur Chunqi Li
@ 2013-07-07 9:18 ` Gleb Natapov
0 siblings, 0 replies; 8+ messages in thread
From: Gleb Natapov @ 2013-07-07 9:18 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Paolo Bonzini, Jan Kiszka
On Sun, Jul 07, 2013 at 05:17:00PM +0800, Arthur Chunqi Li wrote:
> On Sun, Jul 7, 2013 at 4:50 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Sun, Jul 07, 2013 at 04:47:05PM +0800, Arthur Chunqi Li wrote:
> >> On Sun, Jul 7, 2013 at 3:20 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >> > On Sun, Jul 07, 2013 at 03:16:08PM +0800, Arthur Chunqi Li wrote:
> >> >> On Sun, Jul 7, 2013 at 2:59 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >> >> > On Fri, Jul 05, 2013 at 11:13:17PM +0800, Arthur Chunqi Li wrote:
> >> >> >> Fix read/write to IA32_FEATURE_CONTROL MSR in nested environment.
> >> >> >>
> >> >> >> This patch simulate this MSR in nested_vmx and the default value is
> >> >> >> 0x0. BIOS should set it to 0x5 before VMXON. After setting the lock
> >> >> >> bit, write to it will cause #GP(0).
> >> >> >>
> >> >> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> >> >> > The patch is heavily based on Nadav's one. You need to preserve
> >> >> > authorship. Add From: and Nadav's Sign-off-by.
> >> >> >
> >> >> >> ---
> >> >> >> arch/x86/kvm/vmx.c | 25 +++++++++++++++++++++----
> >> >> >> arch/x86/kvm/x86.c | 3 ++-
> >> >> >> 2 files changed, 23 insertions(+), 5 deletions(-)
> >> >> >>
> >> >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> >> >> index 260a919..5e3d44e 100644
> >> >> >> --- a/arch/x86/kvm/vmx.c
> >> >> >> +++ b/arch/x86/kvm/vmx.c
> >> >> >> @@ -373,6 +373,7 @@ struct nested_vmx {
> >> >> >> * we must keep them pinned while L2 runs.
> >> >> >> */
> >> >> >> struct page *apic_access_page;
> >> >> >> + u64 msr_ia32_feature_control;
> >> >> >> };
> >> >> >>
> >> >> >> #define POSTED_INTR_ON 0
> >> >> >> @@ -2277,8 +2278,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
> >> >> >>
> >> >> >> switch (msr_index) {
> >> >> >> case MSR_IA32_FEATURE_CONTROL:
> >> >> >> - *pdata = 0;
> >> >> >> - break;
> >> >> >> + if (nested_vmx_allowed(vcpu)){
> >> >> >> + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control;
> >> >> >> + break;
> >> >> >> + }
> >> >> >> + return 0;
> >> >> >> case MSR_IA32_VMX_BASIC:
> >> >> >> /*
> >> >> >> * This MSR reports some information about VMX support. We
> >> >> >> @@ -2356,9 +2360,13 @@ static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
> >> >> >> if (!nested_vmx_allowed(vcpu))
> >> >> >> return 0;
> >> >> >>
> >> >> >> - if (msr_index == MSR_IA32_FEATURE_CONTROL)
> >> >> >> - /* TODO: the right thing. */
> >> >> >> + if (msr_index == MSR_IA32_FEATURE_CONTROL){
> >> >> >> + if (to_vmx(vcpu)->nested.msr_ia32_feature_control
> >> >> >> + & FEATURE_CONTROL_LOCKED)
> >> >> >> + return 0;
> >> >> >> + to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
> >> >> > You need to allow setting it from userspace, otherwise reset will not
> >> >> > clear it.
> >> >> As to my memory, if the lock bit of IA32_FEATURE_CONTROL is set, this
> >> >> MSR cannot be set or reset. So what do you mean setting it from
> >> >> userspace?
> >> > It can be reset by power cycling CPU obviously, so when userspace emulates
> >> > power cycle reset it has to be able to zero the MSR. BTW you need to write
> >> > corespondent QEMU patch to handle this MSR during reset and migration.
> >> What is the current call trace when QEMU do system_reset? Will it
> >> trigger vmx_vcpu_reset() in vmx.c?
> > It will not. QEMU inits vcpu state in userspace and write it into the
> > kernel with set_regs/set_srges ioctls.
> So how will this achieve isolation and protection? I mean if QEMU can
> reset this MSR when reboot, can user in OS also reset it? Actually,
> after setting to lock bit, it cannot be set until next reboot.
There is a way to tell if msr write is done by QEMU or a guest OS.
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-07-07 9:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-05 15:13 [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL Arthur Chunqi Li
2013-07-07 6:59 ` Gleb Natapov
2013-07-07 7:16 ` Arthur Chunqi Li
2013-07-07 7:20 ` Gleb Natapov
2013-07-07 8:47 ` Arthur Chunqi Li
2013-07-07 8:50 ` Gleb Natapov
2013-07-07 9:17 ` Arthur Chunqi Li
2013-07-07 9:18 ` Gleb Natapov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox