On Mon, Sep 2, 2013 at 4:21 PM, Gleb Natapov <[email protected]> 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 > > 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. Hi Jan and Gleb, Our nested VMX testing framework may not support such testing modes. Here we need to catch the failed result (ZF flag) close to vmresume, but vmresume/vmlaunch is well encapsulated in our framework. If we simply write a vmresume inline function, the VMX will act unexpectedly when it doesn't cause "vmresume fail".
Do you have any ideas about this? Arthur > > 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 <[email protected]> >> --- >> 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. -- Arthur Chunqi Li Department of Computer Science School of EECS Peking University Beijing, China -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
