Re: [RFC PATCH v5 09/45] KVM: x86: Rework .free_external_spt() into .reclaim_external_sp()

[Yan Zhao <yan.y.zhao@intel.com>]

On Wed, Feb 04, 2026 at 05:45:39PM +0800, Yan Zhao wrote:
> On Wed, Jan 28, 2026 at 05:14:41PM -0800, Sean Christopherson wrote:
> > Massage .free_external_spt() into .reclaim_external_sp() to free up (pun
> > intended) "free" for actually freeing memory, and to allow TDX to do more
> > than just "free" the S-EPT entry.  Specifically, nullify external_spt to
> > leak the S-EPT page if reclaiming the page fails, as that detail and
> > implementation choice has no business living in the TDP MMU.
> > 
> > Use "sp" instead of "spt" even though "spt" is arguably more accurate, as
> > "spte" and "spt" are dangerously close in name, and because the key
> > parameter is a kvm_mmu_page, not a pointer to an S-EPT page table.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/include/asm/kvm-x86-ops.h |  2 +-
> >  arch/x86/include/asm/kvm_host.h    |  4 ++--
> >  arch/x86/kvm/mmu/tdp_mmu.c         | 13 ++-----------
> >  arch/x86/kvm/vmx/tdx.c             | 27 ++++++++++++---------------
> >  4 files changed, 17 insertions(+), 29 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> > index 57eb1f4832ae..c17cedc485c9 100644
> > --- a/arch/x86/include/asm/kvm-x86-ops.h
> > +++ b/arch/x86/include/asm/kvm-x86-ops.h
> > @@ -95,8 +95,8 @@ KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
> >  KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
> >  KVM_X86_OP(load_mmu_pgd)
> >  KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
> > -KVM_X86_OP_OPTIONAL_RET0(free_external_spt)
> >  KVM_X86_OP_OPTIONAL(remove_external_spte)
> > +KVM_X86_OP_OPTIONAL(reclaim_external_sp)
> >  KVM_X86_OP(has_wbinvd_exit)
> >  KVM_X86_OP(get_l2_tsc_offset)
> >  KVM_X86_OP(get_l2_tsc_multiplier)
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index d12ca0f8a348..b35a07ed11fb 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1858,8 +1858,8 @@ struct kvm_x86_ops {
> >  				 u64 mirror_spte);
> >  
> >  	/* Update external page tables for page table about to be freed. */
> > -	int (*free_external_spt)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> > -				 void *external_spt);
> > +	void (*reclaim_external_sp)(struct kvm *kvm, gfn_t gfn,
> > +				    struct kvm_mmu_page *sp);
> Do you think "free" is still better than "reclaim" though TDX actually
> invokes tdx_reclaim_page() to reclaim it on the TDX side?
> 
> Naming it free_external_sp can be interpreted as freeing the sp->external_spt
> externally (vs freeing it in tdp_mmu_free_sp_rcu_callback(). This naming also
> allows for the future possibility of freeing sp->external_spt before the HKID is
> freed (though this is unlikely).
Oh. I found there's a free_external_sp() in patch 20.

So, maybe reclaim_external_sp() --> remove_external_spt() ?

Still think "sp" is not good :)

> >  	/* Update external page table from spte getting removed, and flush TLB. */
> >  	void (*remove_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index 27ac520f2a89..18764dbc97ea 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -456,17 +456,8 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> >  				    old_spte, FROZEN_SPTE, level, shared);
> >  	}
> >  
> > -	if (is_mirror_sp(sp) &&
> > -	    WARN_ON(kvm_x86_call(free_external_spt)(kvm, base_gfn, sp->role.level,
> > -						    sp->external_spt))) {
> > -		/*
> > -		 * Failed to free page table page in mirror page table and
> > -		 * there is nothing to do further.
> > -		 * Intentionally leak the page to prevent the kernel from
> > -		 * accessing the encrypted page.
> > -		 */
> > -		sp->external_spt = NULL;
> > -	}
> > +	if (is_mirror_sp(sp))
> > +		kvm_x86_call(reclaim_external_sp)(kvm, base_gfn, sp);
> >
> >  	call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
> >  }
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 30494f9ceb31..66bc3ceb5e17 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1783,27 +1783,24 @@ static void tdx_track(struct kvm *kvm)
> >  	kvm_make_all_cpus_request(kvm, KVM_REQ_OUTSIDE_GUEST_MODE);
> >  }
> >  
> > -static int tdx_sept_free_private_spt(struct kvm *kvm, gfn_t gfn,
> > -				     enum pg_level level, void *private_spt)
> > +static void tdx_sept_reclaim_private_sp(struct kvm *kvm, gfn_t gfn,
> > +					struct kvm_mmu_page *sp)
> Passing in "sp" and having "reclaim_private_sp" in the function name is bit
> confusing.
> Strictly speaking, only sp->external_spt is private, while the sp and sp->spt
> are just mirroring the external spt.
> 
> But I understand it's for setting sp->external_spt to NULL on error.
> 
> >  {
> > -	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > -
> >  	/*
> > -	 * free_external_spt() is only called after hkid is freed when TD is
> > -	 * tearing down.
> >  	 * KVM doesn't (yet) zap page table pages in mirror page table while
> >  	 * TD is active, though guest pages mapped in mirror page table could be
> >  	 * zapped during TD is active, e.g. for shared <-> private conversion
> >  	 * and slot move/deletion.
> > +	 *
> > +	 * In other words, KVM should only free mirror page tables after the
> > +	 * TD's hkid is freed, when the TD is being torn down.
> > +	 *
> > +	 * If the S-EPT PTE can't be removed for any reason, intentionally leak
> > +	 * the page to prevent the kernel from accessing the encrypted page.
> >  	 */
> > -	if (KVM_BUG_ON(is_hkid_assigned(kvm_tdx), kvm))
> > -		return -EIO;
> > -
> > -	/*
> > -	 * The HKID assigned to this TD was already freed and cache was
> > -	 * already flushed. We don't have to flush again.
> > -	 */
> > -	return tdx_reclaim_page(virt_to_page(private_spt));
> > +	if (KVM_BUG_ON(is_hkid_assigned(to_kvm_tdx(kvm)), kvm) ||
> > +	    tdx_reclaim_page(virt_to_page(sp->external_spt)))
> > +		sp->external_spt = NULL;
> >  }
> >  
> >  static void tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
> > @@ -3617,7 +3614,7 @@ void __init tdx_hardware_setup(void)
> >  	vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size, sizeof(struct kvm_tdx));
> >  
> >  	vt_x86_ops.set_external_spte = tdx_sept_set_private_spte;
> > -	vt_x86_ops.free_external_spt = tdx_sept_free_private_spt;
> > +	vt_x86_ops.reclaim_external_sp = tdx_sept_reclaim_private_sp;
> >  	vt_x86_ops.remove_external_spte = tdx_sept_remove_private_spte;
> >  	vt_x86_ops.protected_apic_has_interrupt = tdx_protected_apic_has_interrupt;
> >  }
> > -- 
> > 2.53.0.rc1.217.geba53bf80e-goog
> >