From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH] KVM : Fix read/write to IA32_FEATURE_CONTROL MSR in nested virt Date: Thu, 4 Jul 2013 13:43:03 +0300 Message-ID: <20130704104303.GG5113@redhat.com> References: <1372858868-24755-1-git-send-email-yzt356@gmail.com> <51D51D79.8050000@redhat.com> <20130704071034.GI32123@redhat.com> <20130704072449.GA5113@redhat.com> <1497E699-D43A-411F-B38F-E1D0E50D0093@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Paolo Bonzini , kvm , Jan Kiszka To: Gmail Return-path: Received: from mx1.redhat.com ([209.132.183.28]:55449 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750769Ab3GDKnG convert rfc822-to-8bit (ORCPT ); Thu, 4 Jul 2013 06:43:06 -0400 Content-Disposition: inline In-Reply-To: <1497E699-D43A-411F-B38F-E1D0E50D0093@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Jul 04, 2013 at 04:16:25PM +0800, Gmail wrote: > =E5=9C=A8 2013-7-4=EF=BC=8C15:24=EF=BC=8CGleb Natapov =E5=86=99=E9=81=93=EF=BC=9A >=20 > > On Thu, Jul 04, 2013 at 03:21:15PM +0800, Arthur Chunqi Li wrote: > >> On Thu, Jul 4, 2013 at 3:10 PM, Gleb Natapov wro= te: > >>> 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 environmen= t. > >>>>> 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(). > >>>>>=20 > >>>>> Signed-off-by: Arthur Chunqi Li > >>>>> --- > >>>>> arch/x86/kvm/vmx.c | 5 +---- > >>>>> 1 file changed, 1 insertion(+), 4 deletions(-) > >>>>>=20 > >>>>> 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_vcp= u *vcpu, u32 msr_index, u64 *pdata) > >>>>>=20 > >>>>> switch (msr_index) { > >>>>> case MSR_IA32_FEATURE_CONTROL: > >>>>> - *pdata =3D 0; > >>>>> + *pdata =3D 0x5; > >>>>> break; > >>>>=20 > >>>> This is not in the MSR_IA32_VMX_BASIC..MSR_IA32_VMX_TRUE_ENTRY_C= TLS > >>>> range, so you must check nested_vmx_allowed and return 0 if it i= s false. > >>>>=20 > >>> Or 1? > >> I think 1 is better here because this may return LOCK message when > >> query and tell OS not to write (if OS does such logical check) > >>>=20 > >>>> Otherwise looks good. > >>>>=20 > >>>> Paolo > >>>>=20 > >>>>> case MSR_IA32_VMX_BASIC: > >>>>> /* > >>>>> @@ -2356,9 +2356,6 @@ static int vmx_set_vmx_msr(struct kvm_vcp= u *vcpu, u32 msr_index, u64 data) > >>> Also this function is no longer needed. You can drop it. > >>>=20 > >>> And what about Nadav's patch Bandan pointed too? It is not entire= ly > >>> correct, but it is close to real HW. > >> I think Nadav's patch is much closer to the HW scenario. However, = I > >> think we don't need make things complex since KVM doen't support S= MX > >> now and this MSR is always set to 0x5. > >>=20 > > Set to 0x5 by BIOS on real HW. This way BIOS can control if VMX is > > exposed to an OS. > I know. So if we don't use solutions like Nadav's patch, some third-p= arty BIOSes emulator (if they are) may get error since we simply genera= te #GP(0) when write to this MSR. We can correct SIPI reset in Nadav's = patch and add initial codes to seabios, then the entire logical can fit= real HW. >=20 We do not support third-party BIOSes, we just try to be as close to rea= l HW as possible. Fixing Nadav's code sounds best. > Arthur > >=20 > >> Arthur > >>>=20 > >>>>> if (!nested_vmx_allowed(vcpu)) > >>>>> return 0; > >>>>>=20 > >>>>> - if (msr_index =3D=3D 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 ar= e readonly) > >>>>>=20 > >>>=20 > >>> -- > >>> Gleb. > >=20 > > -- > > Gleb. -- Gleb.