From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v3 1/6] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state Date: Mon, 2 Sep 2013 11:21:36 +0300 Message-ID: <20130902082136.GM22899@redhat.com> References: <93ce2376292d9d6fc7a4f4d53919b0a07d4e7859.1375971992.git.jan.kiszka@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Paolo Bonzini , kvm , Xiao Guangrong , Jun Nakajima , Yang Zhang , Arthur Chunqi Li To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:57381 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757228Ab3IBIVo (ORCPT ); Mon, 2 Sep 2013 04:21:44 -0400 Content-Disposition: inline In-Reply-To: <93ce2376292d9d6fc7a4f4d53919b0a07d4e7859.1375971992.git.jan.kiszka@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > --- > 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.