From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH] KVM : Fix read/write to IA32_FEATURE_CONTROL MSR in nested virt Date: Thu, 04 Jul 2013 13:01:15 +0200 Message-ID: <51D555FB.10305@redhat.com> References: <1372858868-24755-1-git-send-email-yzt356@gmail.com> <51D51D79.8050000@redhat.com> <20130704071034.GI32123@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Arthur Chunqi Li , kvm@vger.kernel.org, jan.kiszka@web.de To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:63210 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753488Ab3GDLBZ (ORCPT ); Thu, 4 Jul 2013 07:01:25 -0400 In-Reply-To: <20130704071034.GI32123@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Il 04/07/2013 09:10, Gleb Natapov ha scritto: > On Thu, Jul 04, 2013 at 09:00:09AM +0200, Paolo Bonzini wrote: >> Il 03/07/2013 15:41, Arthur Chunqi Li ha scritto: >>> Fix read/write to IA32_FEATURE_CONTROL MSR in nested environment. >>> Simply return 0x5 when read and generate #GP(0) when write. >>> Delete handling codes in vmx_set_vmx_msr() and generate #GP(0) in >>> handle_wrmsr(). >>> >>> Signed-off-by: Arthur Chunqi Li >>> --- >>> arch/x86/kvm/vmx.c | 5 +---- >>> 1 file changed, 1 insertion(+), 4 deletions(-) >>> >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>> index 260a919..e125f94 100644 >>> --- a/arch/x86/kvm/vmx.c >>> +++ b/arch/x86/kvm/vmx.c >>> @@ -2277,7 +2277,7 @@ 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; >>> + *pdata = 0x5; >>> break; >> >> This is not in the MSR_IA32_VMX_BASIC..MSR_IA32_VMX_TRUE_ENTRY_CTLS >> range, so you must check nested_vmx_allowed and return 0 if it is false. > > Or 1? "Return 0 from the whole function" and hence #GP(0) on reads. The MSR doesn't exist if VMX=SMX=0. >> Otherwise looks good. >> >> Paolo >> >>> case MSR_IA32_VMX_BASIC: >>> /* >>> @@ -2356,9 +2356,6 @@ static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data) > Also this function is no longer needed. You can drop it. > > And what about Nadav's patch Bandan pointed too? It is not entirely > correct, but it is close to real HW. I don't like that it requires a firmware change in order to use nested VMX (at least for hypervisors that read the MSR). "Worse emulation" and "better emulation + new firmware" are indistiguishable from the point of view of anyone except the firmware. IMO there is no reason for a better emulation that no one would care about _and_ could look like a regression when updating to a newer kernel. Paolo >>> if (!nested_vmx_allowed(vcpu)) >>> return 0; >>> >>> - if (msr_index == MSR_IA32_FEATURE_CONTROL) >>> - /* TODO: the right thing. */ >>> - 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) >>> > > -- > Gleb. >