From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling Date: Wed, 07 Mar 2012 18:18:27 +0200 Message-ID: <4F578A53.20809@redhat.com> References: <20120307155846.GA17631@fermat.math.technion.ac.il> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Julian Stecklina To: "Nadav Har'El" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:1828 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755251Ab2CGQSs (ORCPT ); Wed, 7 Mar 2012 11:18:48 -0500 In-Reply-To: <20120307155846.GA17631@fermat.math.technion.ac.il> Sender: kvm-owner@vger.kernel.org List-ID: On 03/07/2012 05:58 PM, Nadav Har'El wrote: > The existing code emulates the guest's use of the IA32_FEATURE_CONTROL MSR > in a way that was enough to run nested VMX guests, but did not fully > conform to the VMX specification, and in particular did not allow a guest > BIOS to prevent the guest OS from using VMX by setting the lock bit on this > MSR. > > This patch emulates this MSR better, allowing the guest to lock it, and > verifying its setting on VMXON. Also make sure that this MSR (and of course, > VMXON state) is reset on guest vcpu reset (via SIPI). > > Signed-off-by: Nadav Har'El > Reported-by: Julian Stecklina > --- > arch/x86/kvm/vmx.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > --- .before/arch/x86/kvm/vmx.c 2012-03-07 17:52:02.000000000 +0200 > +++ .after/arch/x86/kvm/vmx.c 2012-03-07 17:52:02.000000000 +0200 > @@ -352,6 +352,7 @@ struct nested_vmx { > * we must keep them pinned while L2 runs. > */ > struct page *apic_access_page; > + u64 msr_ia32_feature_control; > }; Need to add to the list of exported MSRs so it can be live migrated (msrs_to_save). The variable itself should live in vcpu->arch, even if some bits are vendor specific. > > struct vcpu_vmx { > @@ -1999,7 +2000,7 @@ static int vmx_get_vmx_msr(struct kvm_vc > > switch (msr_index) { > case MSR_IA32_FEATURE_CONTROL: > - *pdata = 0; > + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control; > break; In a separate patch, please move this outside vmx_get_vmx_msr(). It's not a vmx msr. > case MSR_IA32_VMX_BASIC: > /* > @@ -2077,9 +2078,13 @@ static int vmx_set_vmx_msr(struct kvm_vc > 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) > @@ -3807,6 +3812,8 @@ static int vmx_vcpu_setup(struct vcpu_vm > return 0; > } > > +static void free_nested(struct vcpu_vmx *vmx); > + > static int vmx_vcpu_reset(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -3920,6 +3927,9 @@ static int vmx_vcpu_reset(struct kvm_vcp > /* HACK: Don't enable emulation on guest boot/reset */ > vmx->emulation_required = 0; > > + /* Reset nested-VMX settings: */ > + vmx->nested.msr_ia32_feature_control = 0; > + free_nested(vmx); > out: > return ret; > } > @@ -5031,6 +5041,14 @@ static int handle_vmon(struct kvm_vcpu * > return 1; > } > > +#define VMXON_NEEDED_FEATURES \ > + (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX) Use const u64 instead of #define please, it jars my eyes. > + if ((vmx->nested.msr_ia32_feature_control & VMXON_NEEDED_FEATURES) > + != VMXON_NEEDED_FEATURES) { > + kvm_inject_gp(vcpu, 0); > + return 1; > + } > + > INIT_LIST_HEAD(&(vmx->nested.vmcs02_pool)); > vmx->nested.vmcs02_num = 0; > -- error compiling committee.c: too many arguments to function