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

[Edgecombe, Rick P]

On Fri, 2026-04-03 at 17:05 +0800, Yan Zhao wrote:
> Hmm, sorry for the confusion. I didn't express it clearly.
> 
> The ordering inside tdp_mmu_set_spte_atomic() for mirror root is:
> 
> Before this patch,
>     1. set mirror SPTE to frozen
>     2. invoke TDX op to update external PTE
>     3. set mirror SPTE to new_spte or restore old_spte
>     4. if 2 succeeds, invoke handle_changed_spte() to propagate changes to
>        child mirror SPTEs and child external PTEs
> 
> After this patch,
>    1. set mirror SPTE to frozen
>    2. invoke __handle_changed_spte(), which propagates changes to
>       (1) child mirror SPTEs and child external PTEs
>       (2) external PTE
>    3. set mirror SPTE to new_spte or restore old_spte 
> 
> So, the step to propagate changes to child mirror SPTEs and child external PTEs
> now occurs before the steps to update the external PTE and the mirror SPTE.

How about I add this info to the log. I think you are right, it's an important
change to call out.

Now it's making me think... Can you (if you haven't already) scrutinize for 
races/reasons that may trigger the KVM_BUG_ON() in handle_changed_spte() due to
BUSY or other? Like in the handle_removed_pt() path. I guess the write lock
saves us?

Hmm... zero step?

> 
> > Ya, I'm a bit confused too.  For me, the "tricky" part is understanding the need
> > to set the mirror SPTE to FROZE_SPTE while updating the external SPTE.  Once that
> > is understood, I don't find passing in @new_spte to be surprising in any way.
> I still find it tricky because it seems strange to me to invoke a function named
> handle_changed_spte() before the change actually occurs on the SPTE (i.e.,to me,
> the SPTE has only changed from xxx to FROZEN_SPTE, but handle_changed_spte()
> handles changes from xxx to new_spte).
> 
> Besides, another tricky point (currently benign to TDX) is that:
> before this patch, tdp_mmu_set_spte_atomic() cannot be used to atomically zap
> non-leaf mirror SPTEs, since TDX requires child PTEs to be zapped before the
> parent PTE;
> after this patch, performing atomic zapping of non-leaf mirror SPTEs seems to be
> allowed in TDP MMU since the above step 2.1 now occurs before step 2.2. However,
> if step 2.2 fails after step 2.1 succeeds, step 3 cannot easily restore the real
> old state.
> So, if we allow atomic zap on the mirror root in the future, it looks like we
> need to ensure atomic zapping of S-EPT cannot fail.

I think we shouldn't comment these kind of TDX specifics. It won't confuse non-
TDX developers working in the TDP MMU I think.