Re: [PATCH] KVM : Fix read/write to IA32_FEATURE_CONTROL MSR in nested virt

[Gleb Natapov <gleb@redhat.com>]

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 <yzt356@gmail.com>
> > ---
> >  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?

> 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.

> >  	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.