Re: [PATCH 07/17] KVM: x86/tdp_mmu: Centralize updates to present external PTEs

[Yan Zhao]

On Fri, Apr 03, 2026 at 06:33:16PM +0800, Yan Zhao wrote:
> On Fri, Apr 03, 2026 at 07:46:21AM +0800, Edgecombe, Rick P wrote:
> > On Wed, 2026-04-01 at 16:34 +0800, Yan Zhao wrote:
> > > Thinking more about centralizing TDX hooks, could we be more aggressive? i.e.,
> > > let TDX just have a single hook set_external_spte() for propagation of changes
> > > from mirror page table to S-EPT?
> > > (below change is on code base with TDX huge page support).
> > 
> > I was asking Yan internally why this works but Sean's earlier attempt failed.
> > Yan, let's finish the discussion externally now that Sean is poking around.
> Hmm, I guess why Sean provided op reclaim_external_spt() (now named
> free_external_spt() in this series) is because the old_spte and new_spte
> required by op set_external_spte() are not available in handle_removed_pt(), and
> also because there's a "call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback)"
> in handle_removed_pt().
> 
> So, if I'm not missing something, we may have 2 options for further unification:
> 1. pass in required old_parent_spte and new_parent_spte to handle_removed_pt(),
>    and invoke op set_external_spte() (instead of reclaim_external_spt()) in
>    handle_removed_pt().
> 2. as I proposed in this thread (see below key changes), assert that RCU read
>    lock is always held during __handle_changed_spte() (which is a reasonable
>    assumption in TDP MMU) and invoke op set_external_spte() for reclaiming
>    external pt as well.
>    
>    Though invoking call_rcu() occurs before invoking op set_external_spte(),
>    tdp_mmu_free_sp_rcu_callback() should only occur after invoking
>    set_external_spte() due to __handle_changed_spte() holding RCU read lock.
>    
>    However, I agree it's odd to have call_rcu() invoked before reclaiming
>    external pt :)
> 
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -461,9 +461,6 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
>  		handle_changed_spte(kvm, sp, gfn, old_spte, FROZEN_SPTE, level, shared);
>  	}
> 
> -	if (is_mirror_sp(sp))
> -		kvm_x86_call(reclaim_external_spt)(kvm, base_gfn, sp);
I'm wondering if the reason Sean didn't unify this op into op set_external_spte()
is because of the return type.
A void return type can make it clear that freeing external spt isn't fallible.

>  	call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
>  }
> 
> @@ -563,9 +560,17 @@ static int __handle_changed_spte(struct kvm *kvm, struct kvm_mmu_page *sp,
>  	 * changes to the external SPTE.
>  	 */
>  	if (was_present && !was_leaf &&
> -	    (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed))) {
> +	    (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed)))
>  		handle_removed_pt(kvm, spte_to_child_pt(old_spte, level), shared);
> -	} else if (is_mirror_sp(sp)) {
> +
> +	if (is_mirror_sp(sp)) {
> +		/*
> +		 * Can also propagate changes to remove external pt. Since this
> +		 * occurs after the call_rcu() in handle_removed_pt(), the RCU
> +		 * read lock must be held.
> +		 */
> +		RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu read lock held");
> +
>  		r = kvm_x86_call(set_external_spte)(kvm, gfn, old_spte,
>  						    new_spte, level);
> 
>    
> 
> > I'd be inclined to kind to call the cleanup a win and leave further unification
> > for the future. At least not going turning over rocks.
> I'm ok with leaving it to future refactoring.