Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

[Avi Kivity <avi@redhat.com>]

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 <nyh@il.ibm.com>
> Reported-by: Julian Stecklina <js@alien8.de>
> ---
>  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