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

[Yan Zhao]

On Sat, Oct 10, 2026 at 06:11:15AM +0800, Edgecombe, Rick P wrote:
> On Fri, 2026-10-09 at 16:39 +0800, Yan Zhao wrote:
> > 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@xxxxxxxxx>
> > > > ---
> > > >  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.
> 
> I wasn't suggesting to not change the function. I was suggesting to consider a
> solution that assumes less about the callers, at the cost of over allocating.
Ok :)

> > > > +
> > > > +	/*
> > > > +	 * 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.
> 
> Yea that is what I was suggesting.
Glad you also like it!

> > 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;
> 
> This is assuming too much about the caller.
Please check if the proposal in [*] (the one based on your suggestion) looks
good to you.

[*] https://lore.kernel.org/kvm/asnJcZsKdDzgaYzc@xxxxxxxxxxxxxxxxxxxxxxxxx

> > 
> >         /*
> >          * 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,
> > > 
>