public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling
@ 2012-03-07 15:58 Nadav Har'El
  2012-03-07 16:18 ` Avi Kivity
  0 siblings, 1 reply; 7+ messages in thread
From: Nadav Har'El @ 2012-03-07 15:58 UTC (permalink / raw)
  To: kvm; +Cc: Julian Stecklina, avi

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling
  2012-03-07 15:58 PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling Nadav Har'El
@ 2012-03-07 16:18 ` Avi Kivity
  2012-03-15 17:40   ` Nadav Har'El
  2012-03-19 16:53   ` Nadav Har'El
  0 siblings, 2 replies; 7+ messages in thread
From: Avi Kivity @ 2012-03-07 16:18 UTC (permalink / raw)
  To: Nadav Har'El; +Cc: kvm, Julian Stecklina

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling
  2012-03-07 16:18 ` Avi Kivity
@ 2012-03-15 17:40   ` Nadav Har'El
  2012-03-15 18:08     ` Avi Kivity
  2012-03-19 16:53   ` Nadav Har'El
  1 sibling, 1 reply; 7+ messages in thread
From: Nadav Har'El @ 2012-03-15 17:40 UTC (permalink / raw)
  To: Avi Kivity; +Cc: kvm, Julian Stecklina

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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling
  2012-03-15 17:40   ` Nadav Har'El
@ 2012-03-15 18:08     ` Avi Kivity
  0 siblings, 0 replies; 7+ messages in thread
From: Avi Kivity @ 2012-03-15 18:08 UTC (permalink / raw)
  To: Nadav Har'El; +Cc: kvm, Julian Stecklina, Alexander Graf, Roedel, Joerg

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling
  2012-03-07 16:18 ` Avi Kivity
  2012-03-15 17:40   ` Nadav Har'El
@ 2012-03-19 16:53   ` Nadav Har'El
  2012-03-19 17:03     ` Nadav Har'El
  2012-03-21 13:09     ` Avi Kivity
  1 sibling, 2 replies; 7+ messages in thread
From: Nadav Har'El @ 2012-03-19 16:53 UTC (permalink / raw)
  To: Avi Kivity; +Cc: kvm, Julian Stecklina

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling
  2012-03-19 16:53   ` Nadav Har'El
@ 2012-03-19 17:03     ` Nadav Har'El
  2012-03-21 13:09     ` Avi Kivity
  1 sibling, 0 replies; 7+ messages in thread
From: Nadav Har'El @ 2012-03-19 17:03 UTC (permalink / raw)
  To: Avi Kivity; +Cc: kvm, Julian Stecklina

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;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling
  2012-03-19 16:53   ` Nadav Har'El
  2012-03-19 17:03     ` Nadav Har'El
@ 2012-03-21 13:09     ` Avi Kivity
  1 sibling, 0 replies; 7+ messages in thread
From: Avi Kivity @ 2012-03-21 13:09 UTC (permalink / raw)
  To: Nadav Har'El; +Cc: kvm, Julian Stecklina

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2012-03-21 13:09 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-07 15:58 PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling Nadav Har'El
2012-03-07 16:18 ` Avi Kivity
2012-03-15 17:40   ` Nadav Har'El
2012-03-15 18:08     ` Avi Kivity
2012-03-19 16:53   ` Nadav Har'El
2012-03-19 17:03     ` Nadav Har'El
2012-03-21 13:09     ` Avi Kivity

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox