Re: [PATCH 2/4] KVM: nVMX: remove unnecessary code in prepare_vmcs02

[Sean Christopherson <seanjc@google.com>]

On Wed, May 27, 2026, Paolo Bonzini wrote:
> The late vmwrite of the PDPTRs in prepare_vmcs02() is redundant, because
> every write it does was already performed by prepare_vmcs02_rare earlier
> in the same prepare_vmcs02 call.
> 
> In particular:
> 
> - load_guest_pdptrs_vmcs12 == true implies that prepare_vmcs02_rare() ran
> 
> - the assignment to load_guest_pdptrs_vmcs12 matches the gate in
>   prepare_vmcs02_rare(), other than having one that checks
>   !hv_evmcs and the other !nested_vmx_is_evmptr12_valid(vmx)
> 
> - the condition for the late move is a strict subset of the one in
>   prepare_vmcs02_rare(), because the former checks
>   nested_cpu_has_ept(vmcs12), which implies enable_ept, and on top
>   it narrows it further by ANDing is_pae_paging(vcpu)
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index c1be8ef882b8..f86a12fc29ec 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2721,14 +2721,10 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	struct hv_enlightened_vmcs *evmcs = nested_vmx_evmcs(vmx);
> -	bool load_guest_pdptrs_vmcs12 = false;
>  
>  	if (vmx->nested.dirty_vmcs12 || nested_vmx_is_evmptr12_valid(vmx)) {
>  		prepare_vmcs02_rare(vmx, vmcs12);
>  		vmx->nested.dirty_vmcs12 = false;
> -
> -		load_guest_pdptrs_vmcs12 = !nested_vmx_is_evmptr12_valid(vmx) ||
> -			!(evmcs->hv_clean_fields & HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP1);
>  	}
>  
>  	if (vcpu->arch.nested_run_pending &&
> @@ -2831,15 +2827,6 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>  	if (enable_ept)
>  		vmcs_writel(GUEST_CR3, vmcs12->guest_cr3);
>  
> -	/* Late preparation of GUEST_PDPTRs now that EFER and CRs are set. */
> -	if (load_guest_pdptrs_vmcs12 && nested_cpu_has_ept(vmcs12) &&
> -	    is_pae_paging(vcpu)) {
> -		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);

Hah!  Fixing your own mistakes, but making it look like it was my fault.  :-D

In the original version[1] of what ended up being commit c7554efc8335 ("KVM: nVMX:
Copy PDPTRs to/from vmcs12 only when necessary"), the writes in what is now
prepare_vmcs02_rare() (and I at least am still amused by the meat pun[2]) were
removed.  When the mega-collection of optimizations was posted[3], the removal
of that code got dropped.

AFAICT, it was completely unintenional.  Maybe a rebase goof?

I don't care terribly about the performance, and I 100% agree the extra complexity
to do the "late" loading is ugly, but I still think I'd prefer to keep the "late"
loading so that there's symmetry with the vmcs02 => vmcs12 sync, and so that we
don't try to optimize the prepare_vmcs02_rare() code and consume incorrect state
via is_pae_paging().

Alternatively, expand the comment in prepare_vmcs02_rare()?

[1] https://lore.kernel.org/all/20190507160640.4812-16-sean.j.christopherson@intel.com
[2] https://lore.kernel.org/all/525f4ee1-c111-aaa0-2dcd-6c5ce26e3088@redhat.com
[3] https://lore.kernel.org/all/1560445409-17363-31-git-send-email-pbonzini@redhat.com