Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

[Avi Kivity <avi@redhat.com>]

On 03/19/2012 06:53 PM, Nadav Har'El wrote:
> Hi, in a minute I'll send a new version of the MSR_IA32_FEATURE_CONTROL
> patch for nested VMX; I just wanted to reply first to your comments so
> you'll know what to expect:
>
> On Wed, Mar 07, 2012, Avi Kivity wrote about "Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling":
> > On 03/07/2012 05:58 PM, Nadav Har'El wrote:
> > > +	u64 msr_ia32_feature_control;
> > >  };
> > 
> > Need to add to the list of exported MSRs so it can be live migrated
> > (msrs_to_save).
>
> Did this.
>
> > The variable itself should live in vcpu->arch, even if
> > some bits are vendor specific.
>
> But not this. I understand what you explained about vmx.c being for
> Intel *hosts*, not about emulating Intel *guests*, but I do think that
> since none of the bits in this MSR are relevant on AMD hosts (which
> don't do nested VMX), it isn't useful to support this MSR outside vmx.c.
>
> So I left this variable it in vmx->nested. As I noted earlier, svm.c
> did exactly the same thing (nested.vm_cr_msr), so at least there's
> symmetry here.

Okay.

>
> > > @@ -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.
>
> Done, but not split into two patches: The patch removes the old case in
> vmx_get_vmx_msr() (and also removes vmx_set_vmx_msr() entirely) and
> instead adds the case in vmx_get_msr() and vmx_set_msr().
>
> > > +#define VMXON_NEEDED_FEATURES \
> > > +	  (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
> > 
> > Use const u64 instead of #define please, it jars my eyes.
>
> I would, if Linux coding style allowed to declare variables in the
> middle of blocks. Unfortunately it doesn't, so I left this #define.
> I don't think it's that bad.
>

Move it to the top of the file, or as a variable at the top of the
function please.

-- 
error compiling committee.c: too many arguments to function