From mboxrd@z Thu Jan 1 00:00:00 1970 From: Orit Wasserman Subject: Re: [PATCH v5 03/14] nEPT: Fix wrong test in kvm_set_cr3 Date: Thu, 01 Aug 2013 15:07:45 +0300 Message-ID: <51FA4F91.2080606@redhat.com> References: <1375282131-9713-1-git-send-email-gleb@redhat.com> <1375282131-9713-4-git-send-email-gleb@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Xiao Guangrong , Jun Nakajima , Yang Zhang , pbonzini@redhat.com To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:45263 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753193Ab3HAMGt (ORCPT ); Thu, 1 Aug 2013 08:06:49 -0400 In-Reply-To: <1375282131-9713-4-git-send-email-gleb@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 07/31/2013 05:48 PM, Gleb Natapov wrote: > From: Nadav Har'El > > kvm_set_cr3() attempts to check if the new cr3 is a valid guest physical > address. The problem is that with nested EPT, cr3 is an *L2* physical > address, not an L1 physical address as this test expects. > > As the comment above this test explains, it isn't necessary, and doesn't > correspond to anything a real processor would do. So this patch removes it. > > Note that this wrong test could have also theoretically caused problems > in nested NPT, not just in nested EPT. However, in practice, the problem > was avoided: nested_svm_vmexit()/vmrun() do not call kvm_set_cr3 in the > nested NPT case, and instead set the vmcb (and arch.cr3) directly, thus > circumventing the problem. Additional potential calls to the buggy function > are avoided in that we don't trap cr3 modifications when nested NPT is > enabled. However, because in nested VMX we did want to use kvm_set_cr3() > (as requested in Avi Kivity's review of the original nested VMX patches), > we can't avoid this problem and need to fix it. > > Reviewed-by: Xiao Guangrong > Signed-off-by: Nadav Har'El > Signed-off-by: Jun Nakajima > Signed-off-by: Xinhao Xu > Signed-off-by: Yang Zhang > Signed-off-by: Gleb Natapov > --- > arch/x86/kvm/x86.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index d2caeb9..e2fef8b 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -682,17 +682,6 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > */ > } > > - /* > - * Does the new cr3 value map to physical memory? (Note, we > - * catch an invalid cr3 even in real-mode, because it would > - * cause trouble later on when we turn on paging anyway.) > - * > - * A real CPU would silently accept an invalid cr3 and would > - * attempt to use it - with largely undefined (and often hard > - * to debug) behavior on the guest side. > - */ > - if (unlikely(!gfn_to_memslot(vcpu->kvm, cr3 >> PAGE_SHIFT))) > - return 1; > vcpu->arch.cr3 = cr3; > __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail); > vcpu->arch.mmu.new_cr3(vcpu); > Reviewed-by: Orit Wasserman