[PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[Julian Stecklina]

The spec (Vol 3C, Chapter 34.1) regarding the IA32_FEATURE_CONTROL MSR says "Therefore the lock bit must be set after configuring support for Intel Virtualization Technology and prior to transferring control to an option ROM or the OS." and regarding bit 2: "This bit enables VMX for system executive that do not require SMX."

Signed-off-by: Julian Stecklina <js@alien8.de>
---
 arch/x86/kvm/vmx.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4ea7678..aef1e5b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2007,7 +2007,12 @@ 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;
+                /*
+                 * If nested VMX is enabled, set the lock bit (bit 0)
+                 * and the "Enable VMX outside SMX" bit (bit 2) in the
+                 * FEATURE_CONTROL MSR.
+                 */
+		*pdata = nested_vmx_allowed(vcpu) ? 0x5 : 0;
 		break;
 	case MSR_IA32_VMX_BASIC:
 		/*
-- 
1.7.9.2

Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[Avi Kivity]

On 03/06/2012 05:02 PM, Julian Stecklina wrote:
> The spec (Vol 3C, Chapter 34.1) regarding the IA32_FEATURE_CONTROL MSR says "Therefore the lock bit must be set after configuring support for Intel Virtualization Technology and prior to transferring control to an option ROM or the OS." and regarding bit 2: "This bit enables VMX for system executive that do not require SMX."
>
> Signed-off-by: Julian Stecklina <js@alien8.de>
> ---
>  arch/x86/kvm/vmx.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4ea7678..aef1e5b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2007,7 +2007,12 @@ 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;
> +                /*
> +                 * If nested VMX is enabled, set the lock bit (bit 0)
> +                 * and the "Enable VMX outside SMX" bit (bit 2) in the
> +                 * FEATURE_CONTROL MSR.
> +                 */
> +		*pdata = nested_vmx_allowed(vcpu) ? 0x5 : 0;
>  		break;
>  	case MSR_IA32_VMX_BASIC:
>  		/*

The way I read it, it should be done by the guest, not the host.


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

Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[Julian Stecklina]

Thus spake Avi Kivity <avi@redhat.com>:

> On 03/06/2012 05:02 PM, Julian Stecklina wrote:
>> The spec (Vol 3C, Chapter 34.1) regarding the IA32_FEATURE_CONTROL MSR says "Therefore the lock bit must be set after configuring support for Intel Virtualization Technology and prior to transferring control to an option ROM or the OS." and regarding bit 2: "This bit enables VMX for system executive that do not require SMX."
>>
>> Signed-off-by: Julian Stecklina <js@alien8.de>
>> ---
>>  arch/x86/kvm/vmx.c |    7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 4ea7678..aef1e5b 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2007,7 +2007,12 @@ 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;
>> +                /*
>> +                 * If nested VMX is enabled, set the lock bit (bit 0)
>> +                 * and the "Enable VMX outside SMX" bit (bit 2) in the
>> +                 * FEATURE_CONTROL MSR.
>> +                 */
>> +		*pdata = nested_vmx_allowed(vcpu) ? 0x5 : 0;
>>  		break;
>>  	case MSR_IA32_VMX_BASIC:
>>  		/*
>
> The way I read it, it should be done by the guest, not the host.

It should be done by the BIOS, before it hands off control to the OS
kernel. The question is whether you want to emulate this MSR at this
level or just set it to sane values when nested VMX should be enabled
and be happy. :)

Regards, Julian

Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[Nadav Har'El]

On Tue, Mar 06, 2012, Avi Kivity wrote about "Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.":
> >  	case MSR_IA32_FEATURE_CONTROL:
> > -		*pdata = 0;
> > +                /*
> > +                 * If nested VMX is enabled, set the lock bit (bit 0)
> > +                 * and the "Enable VMX outside SMX" bit (bit 2) in the
> > +                 * FEATURE_CONTROL MSR.
> > +                 */
> > +		*pdata = nested_vmx_allowed(vcpu) ? 0x5 : 0;

0x5 can be written as FEATURE_CONTROL_LOCKED |
                      FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX

> >  		break;
> >  	case MSR_IA32_VMX_BASIC:
> >  		/*
> 
> The way I read it, it should be done by the guest, not the host.

This is also how I understand it. Check out KVM's own hardware_enable()
to see how a guest does turn on these bits before using VMXON - it
doesn't need to rely on the BIOS to have done it earlier (the BIOS, can,
however, prevent the guest from doing this by setting only the lock bit).

What is true, however, is that the existing code is probably incomplete
in three ways (see section 20.7 of the SDM):

 1. It always returns 0 for this MSR, even if the guest previously set it
    to something else. 

 2. handle_vmon() does not check the previous setting of this MSR.
    If the guest (or its BIOS) doesn't set both FEATURE_CONTROL_LOCKED
    and FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX, it should get a
    #GP on an attempt to VMXON. This will allow L1's BIOS to disable
    nested VMX if it wishes (not that I think this is a very useful
    usecase...).

 3. vmx_set_vmx_msr to MSR_IA32_FEATURE_CONTROL should throw a #GP if
    FEATURE_CONTROL_LOCKED is on.
    
I'll create a patch to do this shortly.

-- 
Nadav Har'El                        |                    Tuesday, Mar 6 2012, 
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |Thousands of years ago, cats were
http://nadav.harel.org.il           |worshipped as gods. They never forgot.

Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[Julian Stecklina]

[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]

Am Dienstag, den 06.03.2012, 17:47 +0200 schrieb Nadav Har'El:
> On Tue, Mar 06, 2012, Avi Kivity wrote about "Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.":
> > >  	case MSR_IA32_FEATURE_CONTROL:
> > > -		*pdata = 0;
> > > +                /*
> > > +                 * If nested VMX is enabled, set the lock bit (bit 0)
> > > +                 * and the "Enable VMX outside SMX" bit (bit 2) in the
> > > +                 * FEATURE_CONTROL MSR.
> > > +                 */
> > > +		*pdata = nested_vmx_allowed(vcpu) ? 0x5 : 0;
> 
> 0x5 can be written as FEATURE_CONTROL_LOCKED |
>                       FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX

Nice. Didn't know those constants. Next time I'll try harder to find
those. :)

> 
> > >  		break;
> > >  	case MSR_IA32_VMX_BASIC:
> > >  		/*
> > 
> > The way I read it, it should be done by the guest, not the host.
> 
> This is also how I understand it. Check out KVM's own hardware_enable()
> to see how a guest does turn on these bits before using VMXON - it
> doesn't need to rely on the BIOS to have done it earlier (the BIOS, can,
> however, prevent the guest from doing this by setting only the lock bit).

After looking through the code (vmx_disabled_by_bios), it seems that KVM
doesn't bother with FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX if
FEATURE_CONTROL_LOCKED is not set. It seems like our kernel should do
the same. Sorry for the noise.

> What is true, however, is that the existing code is probably incomplete
> in three ways (see section 20.7 of the SDM):
> 
>  1. It always returns 0 for this MSR, even if the guest previously set it
>     to something else. 
> 
>  2. handle_vmon() does not check the previous setting of this MSR.
>     If the guest (or its BIOS) doesn't set both FEATURE_CONTROL_LOCKED
>     and FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX, it should get a
>     #GP on an attempt to VMXON. This will allow L1's BIOS to disable
>     nested VMX if it wishes (not that I think this is a very useful
>     usecase...).
> 
>  3. vmx_set_vmx_msr to MSR_IA32_FEATURE_CONTROL should throw a #GP if
>     FEATURE_CONTROL_LOCKED is on.
>     
> I'll create a patch to do this shortly.

This is greatly appreciated!

Regards, Julian


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[Nadav Har'El]

On Tue, Mar 06, 2012, Nadav Har'El wrote about "Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.":
>  2. handle_vmon() does not check the previous setting of this MSR.
>     If the guest (or its BIOS) doesn't set both FEATURE_CONTROL_LOCKED
>     and FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX, it should get a

By the way, am I right in my understanding that KVM doesn't support
SMX in the guest? I didn't see mention of CR4.SMXE or a GETSEC exit
handler, which is why I'm assuming that it doesn't...
If this assumption isn't true, I'll also need to worry about the
..._INSIDE_SMX case.

-- 
Nadav Har'El                        |                    Tuesday, Mar 6 2012, 
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |When you handle yourself, use your head;
http://nadav.harel.org.il           |when you handle others, use your heart.

Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[Avi Kivity]

On 03/06/2012 07:33 PM, Nadav Har'El wrote:
> On Tue, Mar 06, 2012, Nadav Har'El wrote about "Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.":
> >  2. handle_vmon() does not check the previous setting of this MSR.
> >     If the guest (or its BIOS) doesn't set both FEATURE_CONTROL_LOCKED
> >     and FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX, it should get a
>
> By the way, am I right in my understanding that KVM doesn't support
> SMX in the guest? 

Isn't nested vmx crazy enough?

btw, any updates on nested EPT? Nested vmx is pointless without it.

> I didn't see mention of CR4.SMXE or a GETSEC exit
> handler, which is why I'm assuming that it doesn't...
> If this assumption isn't true, I'll also need to worry about the
> ..._INSIDE_SMX case.



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

Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[Nadav Har'El]

On Wed, Mar 07, 2012, Avi Kivity wrote about "Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.":
> On 03/06/2012 07:33 PM, Nadav Har'El wrote:
> > By the way, am I right in my understanding that KVM doesn't support
> > SMX in the guest? 
> 
> Isn't nested vmx crazy enough?

:-)

> btw, any updates on nested EPT? Nested vmx is pointless without it.

Isn't "pointless" a bit strong? :-)

I have done most of the changes that you asked for, but still need to finish
the EPT violation overhaul. I was meaning to work on this now, when I got
sidetracked by all these other small issues.

Hopefully, I'll have a version ready for submittion by next week.

-- 
Nadav Har'El                        |                  Wednesday, Mar 7 2012, 
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |"[I'm] so full of action, my name should
http://nadav.harel.org.il           |be a verb" -- Big Daddy Kane ("Raw", 1987)

Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.

[Avi Kivity]

On 03/07/2012 01:10 PM, Nadav Har'El wrote:
> On Wed, Mar 07, 2012, Avi Kivity wrote about "Re: [PATCH] KVM: Enable VMX-related bits in MSR_IA32_FEATURE_CONTROL.":
> > On 03/06/2012 07:33 PM, Nadav Har'El wrote:
> > > By the way, am I right in my understanding that KVM doesn't support
> > > SMX in the guest? 
> > 
> > Isn't nested vmx crazy enough?
>
> :-)
>
> > btw, any updates on nested EPT? Nested vmx is pointless without it.
>
> Isn't "pointless" a bit strong? :-)

How's "unusable"?

> I have done most of the changes that you asked for, but still need to finish
> the EPT violation overhaul. I was meaning to work on this now, when I got
> sidetracked by all these other small issues.
>
> Hopefully, I'll have a version ready for submittion by next week.
>

Great, looking forward.

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