KVM: nVMX: Fix content of MSR_IA32_VMX_ENTRY/EXIT_CTLS

Commit Message

Jan Kiszka March 3, 2013, 12:04 p.m. UTC

From: Jan Kiszka <[e-mail]>

Properly set those bits to 1 that the spec demands in case bit 55 of
VMX_BASIC is 0 - like in our case.

Signed-off-by: Jan Kiszka <[e-mail]>
---
 arch/x86/kvm/vmx.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

Comments

Paolo Bonzini March 4, 2013, 1:24 p.m. UTC | #1

Il 03/03/2013 13:04, Jan Kiszka ha scritto:
> From: Jan Kiszka <[e-mail]>
> 
> Properly set those bits to 1 that the spec demands in case bit 55 of
> VMX_BASIC is 0 - like in our case.
> 
> Signed-off-by: Jan Kiszka <[e-mail]>
> ---
>  arch/x86/kvm/vmx.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 631cdb3..c204f0d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2050,21 +2050,28 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>  		PIN_BASED_EXT_INTR_MASK | PIN_BASED_NMI_EXITING |
>  		PIN_BASED_VIRTUAL_NMIS;
>  
> -	/* exit controls */
> -	nested_vmx_exit_ctls_low = 0;
> +	/*
> +	 * Exit controls
> +	 * If bit 55 of VMX_BASIC is off, bits 0-8 and 10, 11, 13, 14, 16 and
> +	 * 17 must be 1.
> +	 */
> +	nested_vmx_exit_ctls_low = 0x36dff;
>  	/* Note that guest use of VM_EXIT_ACK_INTR_ON_EXIT is not supported. */
>  #ifdef CONFIG_X86_64
>  	nested_vmx_exit_ctls_high = VM_EXIT_HOST_ADDR_SPACE_SIZE;
>  #else
>  	nested_vmx_exit_ctls_high = 0;
>  #endif
> +	nested_vmx_exit_ctls_high |= 0x36dff;

Can you use nested_vmx_exit_ctls_low on the RHS, or define a constant?

>  	/* entry controls */
>  	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
>  		nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high);
> -	nested_vmx_entry_ctls_low = 0;
> +	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
> +	nested_vmx_entry_ctls_low = 0x11ff;
>  	nested_vmx_entry_ctls_high &=
>  		VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_IA32E_MODE;
> +	nested_vmx_entry_ctls_high |= 0x11ff;

Same here.

Paolo

>  	/* cpu-based controls */
>  	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jan Kiszka March 4, 2013, 2:42 p.m. UTC | #2

On 2013-03-04 14:24, Paolo Bonzini wrote:
> Il 03/03/2013 13:04, Jan Kiszka ha scritto:
>> From: Jan Kiszka <[e-mail]>
>>
>> Properly set those bits to 1 that the spec demands in case bit 55 of
>> VMX_BASIC is 0 - like in our case.
>>
>> Signed-off-by: Jan Kiszka <[e-mail]>
>> ---
>>  arch/x86/kvm/vmx.c |   13 ++++++++++---
>>  1 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 631cdb3..c204f0d 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2050,21 +2050,28 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>>  		PIN_BASED_EXT_INTR_MASK | PIN_BASED_NMI_EXITING |
>>  		PIN_BASED_VIRTUAL_NMIS;
>>  
>> -	/* exit controls */
>> -	nested_vmx_exit_ctls_low = 0;
>> +	/*
>> +	 * Exit controls
>> +	 * If bit 55 of VMX_BASIC is off, bits 0-8 and 10, 11, 13, 14, 16 and
>> +	 * 17 must be 1.
>> +	 */
>> +	nested_vmx_exit_ctls_low = 0x36dff;
>>  	/* Note that guest use of VM_EXIT_ACK_INTR_ON_EXIT is not supported. */
>>  #ifdef CONFIG_X86_64
>>  	nested_vmx_exit_ctls_high = VM_EXIT_HOST_ADDR_SPACE_SIZE;
>>  #else
>>  	nested_vmx_exit_ctls_high = 0;
>>  #endif
>> +	nested_vmx_exit_ctls_high |= 0x36dff;
> 
> Can you use nested_vmx_exit_ctls_low on the RHS, or define a constant?

I'll define constants.

Jan

> 
>>  	/* entry controls */
>>  	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
>>  		nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high);
>> -	nested_vmx_entry_ctls_low = 0;
>> +	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
>> +	nested_vmx_entry_ctls_low = 0x11ff;
>>  	nested_vmx_entry_ctls_high &=
>>  		VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_IA32E_MODE;
>> +	nested_vmx_entry_ctls_high |= 0x11ff;
> 
> Same here.
> 
> Paolo
> 
>>  	/* cpu-based controls */
>>  	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
>>
>

Nadav Har'El March 4, 2013, 3:37 p.m. UTC | #3

On Sun, Mar 03, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Fix content of MSR_IA32_VMX_ENTRY/EXIT_CTLS":
>  	/* Note that guest use of VM_EXIT_ACK_INTR_ON_EXIT is not supported. */
>  #ifdef CONFIG_X86_64
>  	nested_vmx_exit_ctls_high = VM_EXIT_HOST_ADDR_SPACE_SIZE;
>  #else
>  	nested_vmx_exit_ctls_high = 0;
>  #endif
> +	nested_vmx_exit_ctls_high |= 0x36dff;

Can you please compose this 0x36dff out of constants? Is
VM_EXIT_HOST_ADDR_SPACE_SIZE one of them?

It's important to verify that we actually support all these bits - even
if we *should* support them, it doesn't mean we actually do (but if we
do, we should say we do).

> -	nested_vmx_entry_ctls_low = 0;
> +	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
> +	nested_vmx_entry_ctls_low = 0x11ff;

Setting nested_vmx_entry_ctls_low = 0 just means that although the spec
says only 1 setting is supported, we *also* support 0 setting. I'm not
sure why this is a bad thing. Our VMX will be even better than the
real processors' ;-)

Jan Kiszka March 4, 2013, 3:52 p.m. UTC | #4

On 2013-03-04 16:37, Nadav Har'El wrote:
> On Sun, Mar 03, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Fix content of MSR_IA32_VMX_ENTRY/EXIT_CTLS":
>>  	/* Note that guest use of VM_EXIT_ACK_INTR_ON_EXIT is not supported. */
>>  #ifdef CONFIG_X86_64
>>  	nested_vmx_exit_ctls_high = VM_EXIT_HOST_ADDR_SPACE_SIZE;
>>  #else
>>  	nested_vmx_exit_ctls_high = 0;
>>  #endif
>> +	nested_vmx_exit_ctls_high |= 0x36dff;
> 
> Can you please compose this 0x36dff out of constants? Is
> VM_EXIT_HOST_ADDR_SPACE_SIZE one of them?

Nope. These are bits enumerated by the spec to be always 1 unless that
bit 55 is 1 - what happens to them then remains unclear to me, but is
not important right now. I'm about to stick them all into a constant.

> 
> It's important to verify that we actually support all these bits - even
> if we *should* support them, it doesn't mean we actually do (but if we
> do, we should say we do).

No, we just implement what the spec demands here. Leaving any of them 0
would make our implementation non-conforming, not just "special" as it
is right now.

> 
>> -	nested_vmx_entry_ctls_low = 0;
>> +	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
>> +	nested_vmx_entry_ctls_low = 0x11ff;
> 
> Setting nested_vmx_entry_ctls_low = 0 just means that although the spec
> says only 1 setting is supported, we *also* support 0 setting. I'm not
> sure why this is a bad thing. Our VMX will be even better than the
> real processors' ;-)

Bad idea. We first need to make it correct, then we can think about
optimizations. This is very important if you want to support hypervisors
for which you have no code, just the knowledge that they work on real hw.

Jan

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 631cdb3..c204f0d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2050,21 +2050,28 @@  static __init void nested_vmx_setup_ctls_msrs(void)
 		PIN_BASED_EXT_INTR_MASK | PIN_BASED_NMI_EXITING |
 		PIN_BASED_VIRTUAL_NMIS;
 
-	/* exit controls */
-	nested_vmx_exit_ctls_low = 0;
+	/*
+	 * Exit controls
+	 * If bit 55 of VMX_BASIC is off, bits 0-8 and 10, 11, 13, 14, 16 and
+	 * 17 must be 1.
+	 */
+	nested_vmx_exit_ctls_low = 0x36dff;
 	/* Note that guest use of VM_EXIT_ACK_INTR_ON_EXIT is not supported. */
 #ifdef CONFIG_X86_64
 	nested_vmx_exit_ctls_high = VM_EXIT_HOST_ADDR_SPACE_SIZE;
 #else
 	nested_vmx_exit_ctls_high = 0;
 #endif
+	nested_vmx_exit_ctls_high |= 0x36dff;
 
 	/* entry controls */
 	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
 		nested_vmx_entry_ctls_low, nested_vmx_entry_ctls_high);
-	nested_vmx_entry_ctls_low = 0;
+	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
+	nested_vmx_entry_ctls_low = 0x11ff;
 	nested_vmx_entry_ctls_high &=
 		VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_IA32E_MODE;
+	nested_vmx_entry_ctls_high |= 0x11ff;
 
 	/* cpu-based controls */
 	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,