Re: [PATCH v4 04/10] KVM: nVMX: Implement cache for L1 APIC pages

[Sean Christopherson <seanjc@google.com>]

On Fri, Jan 02, 2026, Fred Griffoul wrote:
> From: Fred Griffoul <fgriffo@amazon.co.uk>
> 
> Replace kvm_host_map usage with gfn_to_pfn_cache for L1 APIC
> virtualization pages (APIC access, virtual APIC, and posted interrupt
> descriptor pages) to improve performance with unmanaged guest memory.
> 
> The conversion involves several changes:
> 
> - Page loading in nested_get_vmcs12_pages(): load vmcs02 fields with
>   pfncache PFNs after each cache has been checked and possibly activated
>   or refreshed, during OUTSIDE_GUEST_MODE vCPU mode.
> 
> - Invalidation window handling: since nested_get_vmcs12_pages() runs in
>   OUTSIDE_GUEST_MODE, there's a window where caches can be invalidated
>   by MMU notifications before entering IN_GUEST_MODE. implement
>   is_nested_state_invalid() callback to monitor cache validity between
>   OUTSIDE_GUEST_MODE and IN_GUEST_MODE transitions. This triggers
>   KVM_REQ_GET_NESTED_STATE_PAGES when needed.
> 
> - Cache access in event callbacks: the virtual APIC and posted interrupt
>   descriptor pages are accessed by KVM in has_events() and
>   check_events() nested_ops callbacks. These use the kernel HVA following
>   the pfncache pattern of check/refresh, with both callbacks able to sleep
>   if cache refresh is required.
> 
> This eliminates expensive memremap/memunmap cycles for each L2 VM
> entry/exit, providing substantial performance improvements when using
> unmanaged memory.
> 
> Signed-off-by: Fred Griffoul <fgriffo@amazon.co.uk>
> ---
>  arch/x86/kvm/vmx/nested.c | 182 +++++++++++++++++++++++++++++---------
>  arch/x86/kvm/vmx/vmx.h    |   8 +-
>  include/linux/kvm_host.h  |   5 ++
>  3 files changed, 150 insertions(+), 45 deletions(-)

Please split this up; one "cache" per patch.  It's a bit gratutious, but the usage
of the vapic and pid structures is just complex enough to make this more than a
straightforward, "boring" conversion.

> @@ -3112,8 +3173,9 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
>  static int nested_vmx_check_controls_late(struct kvm_vcpu *vcpu,
>  					  struct vmcs12 *vmcs12)
>  {
> -	void *vapic = to_vmx(vcpu)->nested.virtual_apic_map.hva;
> -	u32 vtpr = vapic ? (*(u32 *)(vapic + APIC_TASKPRI)) >> 4 : 0;
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	void *vapic;
> +	u32 vtpr = 0;
>  
>  	/*
>  	 * Don't bother with the consistency checks if KVM isn't configured to
> @@ -3130,6 +3192,12 @@ static int nested_vmx_check_controls_late(struct kvm_vcpu *vcpu,
>  	if (!warn_on_missed_cc)
>  		return 0;
>  
> +	vapic = nested_lock_vapic(vmx);
> +	if (vapic) {
> +		vtpr = (*(u32 *)(vapic + APIC_TASKPRI)) >> 4;
> +		nested_unlock_vapic(vmx);

This is *very* dangerous.  vapic will still be a legal kernel pointer, and the
code _relies_ on it to be non-NULL, but the pointer could become stale at any time.
Happily, my CLASS() approach makes this a non-issue.

> +	}
> +
>  	if ((exec_controls_get(to_vmx(vcpu)) & CPU_BASED_TPR_SHADOW) &&
>  	    nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW) &&
>  	    !nested_cpu_has_vid(vmcs12) &&

...

> @@ -3543,7 +3609,16 @@ static bool vmx_get_nested_state_pages(struct kvm_vcpu *vcpu)
>  
>  static bool vmx_is_nested_state_invalid(struct kvm_vcpu *vcpu)
>  {
> -	return false;
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> +	/*
> +	 * @vcpu is in IN_GUEST_MODE, eliminating the need for individual gpc

This is straight up wrong.

gpc->active is effectively protected by vcpu->mutex.

And for gpc->valid, it's not completely wrong, but it's partially wrong and
definitely misleading.  It too is protected by vcpu->mutex, but only for %false
=> %true transitions.  And even for %true => %false transitions, IN_GUEST_MODE
doesn't exactly provide safety, rather it ensures that KVM will to kick the vCPU
out of the guest before completing the invalidation.

But that's all moot, because the very existence of this code feels wrong.  KVM
should be sending a "no action" request and then requiring vendor code to check
out-of-band data to proxy that into a "real" request.

Presumably older code that you're reverting used KVM_REQ_OUTSIDE_GUEST_MODE;
that doesn't necessary mean it was a good idea, it got yanked out for a reason :-)

IMO, the least awful way to deal with this is to tag KVM_REQ_GET_NESTED_STATE_PAGES
with KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP, and then pass the specific request
that needs to be made when invalidating a vCPU-mapped gpc as part of
__kvm_gpc_init() (though maybe call it vcpu_gpc_init() instead?).  That way KVM
doesn't need this wonky request => kvm_gpc_invalid() => request proxying, and we
should be avoid to entirely avoid the confusingly-named kvm_gpc_invalid().