From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v3 3/6] KVM: nVMX: Load nEPT state after EFER Date: Mon, 2 Sep 2013 21:09:35 +0300 Message-ID: <20130902180935.GJ10142@redhat.com> References: <20130902131612.GT22899@redhat.com> <5224D1C6.7050706@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]:53281 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758720Ab3IBSJn (ORCPT ); Mon, 2 Sep 2013 14:09:43 -0400 Content-Disposition: inline In-Reply-To: <5224D1C6.7050706@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Sep 02, 2013 at 07:58:30PM +0200, Jan Kiszka wrote: > On 2013-09-02 15:16, Gleb Natapov wrote: > > On Thu, Aug 08, 2013 at 04:26:30PM +0200, Jan Kiszka wrote: > >> We need to update EFER.NX before building the nEPT state via > >> nested_ept_init_mmu_context. Otherwise, we risk to create an MMU context > >> that claims to have NX disabled while the guest EPT used NX. This will > >> cause spurious faults for L2. > >> > > Hmm, I do not see how nested ept mmu depends on guests EFER.NX setting. > > It just sets mmu->nx to true. > > Don't ask me for the details behind this, but update_permission_bitmask > called by kvm_init_shadow_ept_mmu is using it e.g. And the It uses it only in !ept case though and never looks at a guest setting as far as I can tell. Is it possible that this was an artifact of all nEPT code and the latest one does not need this patch? > "before-after" effect was clearly visible when L2 and L1 were using > different NX settings. Maybe Arthur can write a test for this. > > Jan > > > > >> Signed-off-by: Jan Kiszka > >> --- > >> arch/x86/kvm/vmx.c | 10 +++++----- > >> 1 files changed, 5 insertions(+), 5 deletions(-) > >> > >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > >> index 6c42518..363fe19 100644 > >> --- a/arch/x86/kvm/vmx.c > >> +++ b/arch/x86/kvm/vmx.c > >> @@ -7727,11 +7727,6 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > >> vmx_flush_tlb(vcpu); > >> } > >> > >> - if (nested_cpu_has_ept(vmcs12)) { > >> - kvm_mmu_unload(vcpu); > >> - nested_ept_init_mmu_context(vcpu); > >> - } > >> - > >> if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_EFER) > >> vcpu->arch.efer = vmcs12->guest_ia32_efer; > >> else if (vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE) > >> @@ -7741,6 +7736,11 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > >> /* Note: modifies VM_ENTRY/EXIT_CONTROLS and GUEST/HOST_IA32_EFER */ > >> vmx_set_efer(vcpu, vcpu->arch.efer); > >> > >> + if (nested_cpu_has_ept(vmcs12)) { > >> + kvm_mmu_unload(vcpu); > >> + nested_ept_init_mmu_context(vcpu); > >> + } > >> + > >> /* > >> * This sets GUEST_CR0 to vmcs12->guest_cr0, with possibly a modified > >> * TS bit (for lazy fpu) and bits which we consider mandatory enabled. > >> -- > >> 1.7.3.4 > > > > -- > > Gleb. > > > > -- > Siemens AG, Corporate Technology, CT RTC ITP SES-DE > Corporate Competence Center Embedded Linux -- Gleb.