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

Yan Zhao
2026-04-08T02:29:52Z

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.