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

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

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

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.

> 
> > > +
> > > +	/*
> > > +	 * 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.

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

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