Re: [PATCH v4 08/17] KVM: TDX: Adjust the topup count of DPAMT page pairs for splitting S-EPT

[Yan Zhao <yan.y.zhao@intel.com>]

On Thu, Oct 08, 2026 at 06:48:00AM +0800, Edgecombe, Rick P wrote:
> On Mon, 2026-09-28 at 17:10 +0800, Yan Zhao wrote:
> > KVM needs to allocate enough DPAMT page pairs in the pamt_cache for
> > consumption by both page table pages and guest pages. Since the DPAMT page
> > pair for the S-EPT root page is already allocated during TD initialization,
> > there is no need to allocate the DPAMT page pair for the S-EPT root page.
> > Therefore, previously the DPAMT page pairs required equals
> > "min_nr_spts - 1 + 1".
> > 
> > When splitting S-EPT, min_nr_spts does not include the root SPT. So, limit
> > the -1 calculation to when min_nr_spts equals root_level, though this will
> > cause one pair over-allocation in the normal page fault path because
> > PT64_ROOT_MAX_LEVEL is always passed even when launching a 4-level TD.
> > 
> > Additionally, KVM may need to retry tdh_mem_page_demote() a second time,
> > causing the DPAMT page pair for the guest private pages to be drawn from
> > the pamt_cache twice in the worst case. Therefore, add an extra +1 to cover
> > this worst-case scenario.
> > 
> > The slight over-allocation is acceptable since KVM already pre-allocates
> > more pages than needed (e.g., when mapping huge pages) in case of the
> > worst-case scenario.
> 
> This is assumes way too much about the callers and way too convoluted. The
> existing code already did but, now this is just too far. We need another
> solution.
> 
> Questions below on what that might be.
Thanks for the review!


> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > ---
> >  arch/x86/kvm/vmx/tdx.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 3186c4808cae..397a308b834c 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1630,16 +1630,34 @@ void tdx_load_mmu_pgd(struct kvm_vcpu *vcpu, hpa_t root_hpa, int pgd_level)
> >  
> >  static int tdx_topup_external_pamt_cache(struct kvm_vcpu *vcpu, int min_nr_spts)
> >  {
> > +	int dpamt_pairs;
> > +
> >  	if (WARN_ON_ONCE(!vcpu))
> >  		return -EIO;
> >  
> > +	/* Exclude the root SPT, as its DPAMT page pair is already installed */
> > +	if (min_nr_spts == vcpu->kvm->arch.mirror_root_level)
> > +		min_nr_spts -= 1;
> 
> Over allocating a bit is not the end of the world...
Before this patch, dpamt_pairs = min_nr_spts - 1 + 1.
However, when min_nr_spts is 1, the correct dpamt_pairs should be 2 instead of 1
(in the case when DEMOTE does not fail).
i.e., without this change, we would allocate 1 less page, which is a bug.

> > +
> > +	/*
> > +	 * Each S-EPT page table page + 4KB guest private page needs a pair of
> > +	 * DPAMT pages.
> > +	 */
> > +	dpamt_pairs = min_nr_spts + 1;
> > +
> >  	/*
> > -	 * Minus one page to exclude the root SPT, but plus one page for a
> > -	 * possible 4KB private mapping.
> > +	 * After each topup, KVM may invoke DEMOTE at most twice.
> > 
> 
> Why twice? You mean the BUSY retry attempt, right? If you do, couldn't we fix
> this problem within tdh_mem_page_demote()?
In tdh_mem_page_demote(), dpamt_pages are allocated from pamt_cache before
invoking the SEAMCALL, and freed after the SEAMCALL fails.
So, if KVM retries on the BUSY error for at most twice, an extra pair of
dpamt_pages are allocated from the pamt_cache.

If we want to fix this problem within tdh_mem_page_demote(), one possible
solution is to re-insert the dpamt_pages back to the pamt_cache list.

e.g., add the following diff to patch 2.

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 964739c5687b..e945885d5898 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -87,6 +87,7 @@ static DEFINE_RAW_SPINLOCK(sysinit_lock);

 static int alloc_pamt_array(struct page **pamt_pages, struct tdx_pamt_cache *cache);
 static void free_pamt_array(struct page **pamt_pages);
+static void reinsert_pamt_array(struct tdx_pamt_cache *cache, struct page **pamt_pages);

 /*
  * Do the module global initialization once and return its result.
@@ -1904,7 +1905,7 @@ u64 tdh_mem_page_demote(struct tdx_td *td, u64 gpa, enum pg_level level, kvm_pfn

 out_free:
        spin_unlock(&dpamt_lock);
-       free_pamt_array(dpamt_pages);
+       reinsert_pamt_array(pamt_cache, dpamt_pages);
        return ret;
 }
 EXPORT_SYMBOL_FOR_KVM(tdh_mem_page_demote);
@@ -2146,6 +2147,24 @@ static void free_pamt_array(struct page **pamt_pages)
        }
 }

+static void reinsert_pamt_array(struct tdx_pamt_cache *cache, struct page **pamt_pages)
+{
+       int i;
+
+       if (!cache)
+               return;
+
+       for (i = 0; i < TDX_DPAMT_ENTRY_PAGE_CNT; i++) {
+               /*
+                * Reset pages unconditionally to cover cases
+                * where they were passed to the TDX module.
+                */
+               tdx_quirk_reset_paddr(page_to_phys(pamt_pages[i]), PAGE_SIZE);
+
+               list_add(&pamt_pages[i]->lru, &cache->page_list);
+               cache->cnt++;
+       }
+}
 /* Helper for building DPAMT seamcall() arguments. */
 static u64 pamt_2mb_arg(kvm_pfn_t pfn)
 {

Reinsertion should be locklessly safe since the pamt_cache is either per-vCPU or
protected by the caller when it's per-VM.

> >  The first
> > +	 * DEMOTE invocation draws two pairs of pages from the cache: one for
> > +	 * the S-EPT page table page and one for the guest private memory.
> > +	 * Since these pages are not returned to the cache, the second DEMOTE
> > +	 * invocation still needs to consume one additional pair for the guest
> > +	 * private memory (the pair for the S-EPT page table page is reused for
> > +	 * the 2nd invocation).
> > 
> > 
> > 
> 
> Another idea, change the op to be:
> int topup_external_cache(struct kvm_vcpu *vcpu, bool root, bool private_page,
> int min_nr_spts);
> 
> Normal topup can set:
> root=true
> private_page=true
> min_nr_spts = PT64_ROOT_MAX_LEVEL - 1
> 
> Then we can calculate exactly what we need. And even better, the existing code
> won't nee a comment to explain the weirdness.
The existing code looks like this:
static int tdx_topup_external_pamt_cache(struct kvm_vcpu *vcpu, int min_nr_spts)
{
        /*
         * Minus one page to exclude the root SPT, but plus one page for a
         * possible 4KB private mapping.
         */
        min_nr_spts += -1 + 1;

        return tdx_topup_pamt_cache(&to_tdx(vcpu)->pamt_cache, min_nr_spts);
}

If we agree on the reinserting solution for DEMOTE, then
tdx_topup_external_pamt_cache() could look like this:

static int tdx_topup_external_pamt_cache(struct kvm *kvm, struct kvm_vcpu *vcpu,
                                         int min_nr_spts)
{
        struct tdx_pamt_cache *pamt_cache;
        int dpamt_pairs;

        pamt_cache = tdx_get_pamt_cache(kvm, vcpu);
        if (!pamt_cache)
                return -EIO;

        /* Exclude the root SPT, as its DPAMT page pair is already installed */
        if (min_nr_spts == kvm->arch.mirror_root_level)
                min_nr_spts -= 1;

        /*
         * Each S-EPT page table page + 4KB guest private page needs a pair of
         * DPAMT pages.
         */
        dpamt_pairs = min_nr_spts + 1;

        return tdx_topup_pamt_cache(pamt_cache, dpamt_pairs);
}

IMHO, it's clearer than having the caller indicate whether it's root or not.

> >  Increase the topup count to account for this
> > +	 * worst-case scenario.
> >  	 */
> > -	min_nr_spts += -1 + 1;
> > +	dpamt_pairs += 1;
> >  
> > -	return tdx_topup_pamt_cache(&to_tdx(vcpu)->pamt_cache, min_nr_spts);
> > +	return tdx_topup_pamt_cache(&to_tdx(vcpu)->pamt_cache, dpamt_pairs);
> >  }
> >  
> >  static int tdx_mem_page_add(struct kvm *kvm, gfn_t gfn, enum pg_level level,
>