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

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

On 2013-09-02 10:21, Gleb Natapov wrote:
> 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

Yeah, should rephrase this.

> 
> 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.

kvm_set_cr0() is for emulating explicit guest changes. It is not the
proper interface for implicit, vendor-dependent changes like this one.

> 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.
> 

Yes, there is surely more to improve. Do you think the lacking checks
can cause troubles for L0, or is this just imprecise emulation that can
be addressed separately?

Jan

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