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

Yan Zhao
2026-04-03T11:12:54Z

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);
-
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.