Re: [PATCH v3 3/6] KVM: nVMX: Load nEPT state after EFER

[Gleb Natapov <gleb@redhat.com>]

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 <jan.kiszka@siemens.com>
> >> ---
> >>  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.