PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

[Nadav Har'El]

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;
 };
 
 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;
 	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)
+	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;
 
-- 
Nadav Har'El                        |                  Wednesday, Mar 7 2012, 
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |In Fortran, God is real unless declared
http://nadav.harel.org.il           |an integer.

Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

[Avi Kivity]

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

Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

[Nadav Har'El]

On Wed, Mar 07, 2012, Avi Kivity wrote about "Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling":
> >  	struct page *apic_access_page;
> > +	u64 msr_ia32_feature_control;
> >  };
>...
> (msrs_to_save).  The variable itself should live in vcpu->arch, even if
> some bits are vendor specific.

Does this MSR exist in AMD? I was under the impression that it is an
Intel-only MSR, and that AMD has something different, the VM_CR MSR,
so it didn't make sense to put this in vcpu->arch. Is my impression
wrong?

I seems, by the way, that svm.c has vm_cr_msr in svm->nested, basically the
same what I did, not in vcpu->arch. Why is this bad?

Also, it seems that VM_CR is also not on the list on msrs_to_save.
A bug?

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

I agree, I'll move it. But if it's a VMX-only MSR, I want to leave it
in vmx.c, and not move it to x86.c.

-- 
Nadav Har'El                        |                  Thursday, Mar 15 2012, 
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |If I am not for myself, who will be for
http://nadav.harel.org.il           |me? If I am only for myself, who am I?

Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

[Avi Kivity]

On 03/15/2012 07:40 PM, Nadav Har'El wrote:
> On Wed, Mar 07, 2012, Avi Kivity wrote about "Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling":
> > >  	struct page *apic_access_page;
> > > +	u64 msr_ia32_feature_control;
> > >  };
> >...
> > (msrs_to_save).  The variable itself should live in vcpu->arch, even if
> > some bits are vendor specific.
>
> Does this MSR exist in AMD? I was under the impression that it is an
> Intel-only MSR, and that AMD has something different, the VM_CR MSR,
> so it didn't make sense to put this in vcpu->arch. Is my impression
> wrong?

vmx.c is not about guest-side Intel features, it's about vmx specific
implementation of x86 features.  Whenever possible we put things in
x86.c so if the other vendor picks up the feature, it will just work.

Nested vmx is of course impossible to put in x86.c.  This MSR is
somewhat on the borderline, there's nothing vendor specific about the
_implementation_, but it all the features it controls are very vendor
specific.  So I guess it can stay in vmx.c, if you can make it available
for save/restore.

> I seems, by the way, that svm.c has vm_cr_msr in svm->nested, basically the
> same what I did, not in vcpu->arch. Why is this bad?

The same reasoning applies.

> Also, it seems that VM_CR is also not on the list on msrs_to_save.
> A bug?

Yes.

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

Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

[Nadav Har'El]

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.

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

-- 
Nadav Har'El                        |                    Monday, Mar 19 2012, 
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |A conscience does not prevent sin. It
http://nadav.harel.org.il           |only prevents you from enjoying it.

Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

[Nadav Har'El]

The existing code emulated 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 |   43 +++++++++++++++++++++++--------------------
 arch/x86/kvm/x86.c |    3 ++-
 2 files changed, 25 insertions(+), 21 deletions(-)

--- .before/arch/x86/kvm/vmx.c	2012-03-19 18:34:24.000000000 +0200
+++ .after/arch/x86/kvm/vmx.c	2012-03-19 18:34:24.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;
 };
 
 struct vcpu_vmx {
@@ -1998,9 +1999,6 @@ static int vmx_get_vmx_msr(struct kvm_vc
 	}
 
 	switch (msr_index) {
-	case MSR_IA32_FEATURE_CONTROL:
-		*pdata = 0;
-		break;
 	case MSR_IA32_VMX_BASIC:
 		/*
 		 * This MSR reports some information about VMX support. We
@@ -2072,21 +2070,6 @@ static int vmx_get_vmx_msr(struct kvm_vc
 	return 1;
 }
 
-static int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
-{
-	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)
-	 */
-	return 0;
-}
-
 /*
  * Reads an msr value (of 'msr_index') into 'pdata'.
  * Returns 0 on success, non-0 otherwise.
@@ -2129,6 +2112,9 @@ static int vmx_get_msr(struct kvm_vcpu *
 	case MSR_IA32_SYSENTER_ESP:
 		data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
+	case MSR_IA32_FEATURE_CONTROL:
+		data = to_vmx(vcpu)->nested.msr_ia32_feature_control;
+		break;
 	case MSR_TSC_AUX:
 		if (!to_vmx(vcpu)->rdtscp_enabled)
 			return 1;
@@ -2197,6 +2183,12 @@ static int vmx_set_msr(struct kvm_vcpu *
 		}
 		ret = kvm_set_msr_common(vcpu, msr_index, data);
 		break;
+	case MSR_IA32_FEATURE_CONTROL:
+		if (to_vmx(vcpu)->nested.msr_ia32_feature_control
+				& FEATURE_CONTROL_LOCKED)
+			return 1;
+		to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
+		break;
 	case MSR_TSC_AUX:
 		if (!vmx->rdtscp_enabled)
 			return 1;
@@ -2205,8 +2197,6 @@ static int vmx_set_msr(struct kvm_vcpu *
 			return 1;
 		/* Otherwise falls through */
 	default:
-		if (vmx_set_vmx_msr(vcpu, msr_index, data))
-			break;
 		msr = find_msr_entry(vmx, msr_index);
 		if (msr) {
 			msr->data = data;
@@ -3807,6 +3797,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 +3912,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 +5026,14 @@ static int handle_vmon(struct kvm_vcpu *
 		return 1;
 	}
 
+#define VMXON_NEEDED_FEATURES \
+	  (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
+	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;
 
--- .before/arch/x86/kvm/x86.c	2012-03-19 18:34:24.000000000 +0200
+++ .after/arch/x86/kvm/x86.c	2012-03-19 18:34:24.000000000 +0200
@@ -799,7 +799,8 @@ static u32 msrs_to_save[] = {
 #ifdef CONFIG_X86_64
 	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
 #endif
-	MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
+	MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
+	MSR_IA32_FEATURE_CONTROL
 };
 
 static unsigned num_msrs_to_save;

Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling

[Avi Kivity]

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