Re: [PATCH v7 08/11] KVM: TDX: Get/put PAMT pages when (un)mapping private memory

[Edgecombe, Rick P]

On Wed, 2026-07-22 at 10:31 -0700, Sean Christopherson wrote:
> On Wed, Jul 22, 2026, Rick P Edgecombe wrote:
> > On Wed, 2026-07-22 at 08:12 -0700, Sean Christopherson wrote:
> > 
> It's already there:

Doh, yep.

> > Ok. Is there something we can add to connect the slots lock to the root freeing?
> > Like maybe a helper to encode that rule? Or better to not wrap the delicate
> > details? A comment instead...
> 
> Ya, a comment.
> 
> LOL, hilarious.  I just discovered a (not fully functional) patch sitting in one
> of my many branches that adds the is_page_fault_stale() check, with this as the
> changelog:
> 
>     KVM: x86/mmu: Ensure page fault isn't stale/obsolete when mapping private PFN
>     
>     Add a sanity check in the helper used to map private pages into a TDX guest
>     to ensure KVM isn't attempting to map memory into an invalid/obsolete root.
>     It _should_ be impossible for the root to be invalid, as the only flow that
>     marks TDP MMU roots as invalid is "fast all zap", and doing a "fast zap" is
>     mutually exclusive with populating TDX memory thanks to slots_lock (this is
>     also why KVM doesn't retry kvm_mmu_reload()).
>     
>     Note, KVM will already WARN on an invalid root if CONFIG_KVM_PROVE_MMU=y,
>     but the check is inexpensive compared to the cost of populating memory into
>     a TDX guest, and not having a is_page_fault_stale() check _looks_ wrong.
> 
> I'll munge that into a mini-series to add the sanity checks.  No need to hold
> the D-PAMT series, I see this as orthogonal hardening.  I'm leaning towards the
> "have our cake and eat it too" option as the final resting state:

Sounds good to me, thanks. I think Yan was working on a patch, but hadn't
finished it.

> 
>         do {
>                 if (signal_pending(current))
>                         return -EINTR;
> 
>                 if (kvm_test_request(KVM_REQ_VM_DEAD, vcpu))
>                         return -EIO;
> 
>                 r = kvm_mmu_reload(vcpu);
>                 if (r)
>                         return r;

I'm ok either way, but I'd think to make the bugs easier to surface rather than
handle them. It seems at odds with how we discussed KVM_BUG_ON() in the past. A
reader may get the impression that kvm_mmu_reload() inside the retry is
required.

Hmm, does the new paradigm of AI finding ancient lurking bugs tilt the
safety/minimalism balance?

> 
>                 r = mmu_topup_memory_caches(vcpu, false);
>                 if (r)
>                         return r;
> 
>                 cond_resched();
> 
>                 guard(read_lock)(&kvm->mmu_lock);
> 
> 		/* Comment goes here. */
>                 WARN_ON_ONCE(kvm_test_request(KVM_REQ_MMU_FREE_OBSOLETE_ROOTS, vcpu));
> 
>                 /*
>                  * Snapshot the invalidation sequence counter after acquiring
>                  * mmu_lock, as guest_memfd guarantees the validity of the pfn,
>                  * i.e. any concurrent invalidations are guaranteed to be
>                  * irrelevant.
>                  */
>                 fault.mmu_seq = vcpu->kvm->mmu_invalidate_seq;
>                 if (is_page_fault_stale(vcpu, &fault))
>                         continue;
> 
>                 r = kvm_tdp_mmu_map(vcpu, &fault);
>         } while (r == RET_PF_RETRY);