Re: [PATCH v2] KVM: nVMX: Fix read/write to MSR_IA32_FEATURE_CONTROL

[Gleb Natapov <gleb@redhat.com>]

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.