Re: [PATCH] KVM: nVMX: Fix content of MSR_IA32_VMX_ENTRY/EXIT_CTLS

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux