Re: [PATCH v3 1/6] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state

[Gleb Natapov <gleb@redhat.com>]

On Thu, Aug 08, 2013 at 04:26:28PM +0200, Jan Kiszka wrote:
> Likely a typo, but a fatal one as kvm_set_cr0 performs checks on the
Not a typo :) That what Avi asked for do during initial nested VMX
review: http://markmail.org/message/hhidqyhbo2mrgxxc

But there is at least one transition check that kvm_set_cr0() does that
should not be done during vmexit emulation, namely CS.L bit check, so I
tend to agree that kvm_set_cr0() is not appropriate here, at lest not as
it is. But can we skip other checks kvm_set_cr0() does? For instance
what prevents us from loading CR0.PG = 1 EFER.LME = 1 and CR4.PAE = 0
during nested vmexit?  What _should_ prevent it is vmentry check from
26.2.4

If the "host address-space size" VM-exit control is 1, the following
must hold:
 - Bit 5 of the CR4 field (corresponding to CR4.PAE) is 1.

But I do not see that we do that check on vmentry.

What about NW/CD bit checks, or reserved bits checks? 27.5.1 says:
  The following bits are not modified:
   For CR0, ET, CD, NW; bits 63:32 (on processors that support Intel 64
   architecture), 28:19, 17, and 15:6; and any bits that are fixed in
   VMX operation (see Section 23.8).

But again current vmexit code does not emulate this properly and just
sets everything from host_cr0. vmentry should also preserve all those
bit but it looks like it doesn't too.


> state transition that may prevent loading L1's cr0.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/kvm/vmx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 57b4e12..d001b019 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8185,7 +8185,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>  	 * fpu_active (which may have changed).
>  	 * Note that vmx_set_cr0 refers to efer set above.
>  	 */
> -	kvm_set_cr0(vcpu, vmcs12->host_cr0);
> +	vmx_set_cr0(vcpu, vmcs12->host_cr0);
>  	/*
>  	 * If we did fpu_activate()/fpu_deactivate() during L2's run, we need
>  	 * to apply the same changes to L1's vmcs. We just set cr0 correctly,
> -- 
> 1.7.3.4

--
			Gleb.