Re: [PATCH v5 02/14] nEPT: Fix cr3 handling in nested exit and entry

[Orit Wasserman <owasserm@redhat.com>]

On 07/31/2013 05:48 PM, Gleb Natapov wrote:
> From: Nadav Har'El <nyh@il.ibm.com>
> 
> The existing code for handling cr3 and related VMCS fields during nested
> exit and entry wasn't correct in all cases:
> 
> If L2 is allowed to control cr3 (and this is indeed the case in nested EPT),
> during nested exit we must copy the modified cr3 from vmcs02 to vmcs12, and
> we forgot to do so. This patch adds this copy.
> 
> If L0 isn't controlling cr3 when running L2 (i.e., L0 is using EPT), and
> whoever does control cr3 (L1 or L2) is using PAE, the processor might have
> saved PDPTEs and we should also save them in vmcs12 (and restore later).
> 
> Signed-off-by: Nadav Har'El <nyh@il.ibm.com>
> Signed-off-by: Jun Nakajima <jun.nakajima@intel.com>
> Signed-off-by: Xinhao Xu <xinhao.xu@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  arch/x86/kvm/vmx.c |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 27efa6a..4e98764 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -7595,6 +7595,16 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  	kvm_set_cr3(vcpu, vmcs12->guest_cr3);
>  	kvm_mmu_reset_context(vcpu);
>  
> +	/*
> +	 * L1 may access the L2's PDPTR, so save them to construct vmcs12
> +	 */
> +	if (enable_ept) {
> +		vmcs_write64(GUEST_PDPTR0, vmcs12->guest_pdptr0);
> +		vmcs_write64(GUEST_PDPTR1, vmcs12->guest_pdptr1);
> +		vmcs_write64(GUEST_PDPTR2, vmcs12->guest_pdptr2);
> +		vmcs_write64(GUEST_PDPTR3, vmcs12->guest_pdptr3);
> +	}
> +
>  	kvm_register_write(vcpu, VCPU_REGS_RSP, vmcs12->guest_rsp);
>  	kvm_register_write(vcpu, VCPU_REGS_RIP, vmcs12->guest_rip);
>  }
> @@ -7917,6 +7927,22 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  	vmcs12->guest_pending_dbg_exceptions =
>  		vmcs_readl(GUEST_PENDING_DBG_EXCEPTIONS);
>  
> +	/*
> +	 * In some cases (usually, nested EPT), L2 is allowed to change its
> +	 * own CR3 without exiting. If it has changed it, we must keep it.
> +	 * Of course, if L0 is using shadow page tables, GUEST_CR3 was defined
> +	 * by L0, not L1 or L2, so we mustn't unconditionally copy it to vmcs12.
> +	 *
> +	 * Additionally, restore L2's PDPTR to vmcs12.
> +	 */
> +	if (enable_ept) {
> +		vmcs12->guest_cr3 = vmcs_read64(GUEST_CR3);
> +		vmcs12->guest_pdptr0 = vmcs_read64(GUEST_PDPTR0);
> +		vmcs12->guest_pdptr1 = vmcs_read64(GUEST_PDPTR1);
> +		vmcs12->guest_pdptr2 = vmcs_read64(GUEST_PDPTR2);
> +		vmcs12->guest_pdptr3 = vmcs_read64(GUEST_PDPTR3);
> +	}
> +
>  	vmcs12->vm_entry_controls =
>  		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
>  		(vmcs_read32(VM_ENTRY_CONTROLS) & VM_ENTRY_IA32E_MODE);
> 

Reviewed-by: Orit Wasserman <owasserm@redhat.com>