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 08:12 -0700, Sean Christopherson wrote:
> Hmm, yeah, I think I agree.  Super duper technically, that's a fix for an
> existing flaw.  Because very, very, VERY theoretically, the cache of page
> table pages could be exhausted.  E.g. if some other task managed to free non-
> leaf page tables while mmu_lock was dropped, thus forcing
> kvm_tdp_mmu_map_private_pfn() to allocate from its cache over and over.  In
> practice, that's likely impossible thanks to holding slots_lock, but given
> that (a) retry should be rare and (b) kvm_mmu_topup_memory_cache()
> is basically free if no work needs to be done, I don't see any reason to do
> topup outside of the retry loop.
> 
> The other thing we should address is the call to kvm_mmu_reload().  Like cache
> exhaustion, it *should* be impossible for the root to be
> invalidated/obsoleted, thanks to holding slots lock.  But as evidenced by the
> rash of recent shadow MMU bugs, we don't always get things perfect, and lack
> of defense-in-depth can be *extremely* painful.

Should we assert that we are holding slots lock then? Otherwise the reason for
the warnings would be confusing. To me at least.

> 
> I don't think I want to just move kvm_mmu_reload() into the loop, because KVM
> should provide stronger guarantees with respect to the validity of the loop,
> versus the population of the caches.  I.e. I want to WARN if the root becomes
> obsolete after the initial reload.  And more importantly, KVM really should
> check the validity of the root after acquiring mmu_lock.
> 
> We can't simply call is_page_fault_stale(), because mmu_invalidate_retry_gfn()
> is inherently fuzzy, i.e. could get false positives, even though the pfn
> provided by guest_memfd is guaranteed to be valid.  E.g. if shared gfns
> surrounding the to-be-mapped gfn are concurrently invalidated.
> 
> So, this?
> 
> 	r = kvm_mmu_reload(vcpu);
> 	if (r)
> 		return r;
> 
> 	do {
> 		if (signal_pending(current))
> 			return -EINTR;
> 
> 		if (kvm_test_request(KVM_REQ_VM_DEAD, vcpu))
> 			return -EIO;
> 
> 		r = mmu_topup_memory_caches(vcpu, false);
> 		if (r)
> 			return r;
> 
> 		cond_resched();
> 	
> 		guard(read_lock)(&kvm->mmu_lock);
> 
> 		if
> (WARN_ON_ONCE(kvm_test_request(KVM_REQ_MMU_FREE_OBSOLETE_ROOTS, vcpu)))
> 			return -EIO;
> 
> 		r = kvm_tdp_mmu_map(vcpu, &fault);
> 	} while (r == RET_PF_RETRY);
> 

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