| Message ID | 1373037197-28945-1-git-send-email-yzt356@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)