Re: [PATCH v4 02/17] x86/virt/tdx: Add a SEAMCALL wrapper to demote a 2MB huge page

[Edgecombe, Rick P]

On Mon, 2026-10-19 at 19:04 +0800, Yan Zhao wrote:
> On Fri, Oct 16, 2026 at 02:30:49AM +0800, Edgecombe, Rick P wrote:
> > On Thu, 2026-10-15 at 17:49 +0800, Yan Zhao wrote:
> > > > u64 tdh_mem_page_demote_2mb(struct tdx_td *td, u64 gpa, kvm_pfn_t pfn,
> > > > struct page *new_sept_pt,
> > > >                              struct tdx_pamt_cache *pamt_cache, u64
> > > > *ext_err1, u64 *ext_err2)
> > > > {
> > > >          struct tdx_module_args args = {
> > > >                  .rcx = gpa | pg_level_to_tdx_sept_level(PG_LEVEL_2M),
> > > >                  .rdx = tdx_tdr_pa(td),
> > > >                  .r8 = page_to_phys(new_sept_pt),
> > > >          };
> > > >          struct page *dpamt_pages[TDX_DPAMT_ENTRY_PAGE_CNT];
> > > >          atomic_t *dpamt_refcount;
> > > >          u64 ret;
> > > > 
> > > >          *ext_err1 = 0;
> > > >          *ext_err2 = 0;
> > > > 
> > > >          if (WARN_ON_ONCE(!tdx_supports_dynamic_pamt(&tdx_sysinfo)))
> > > >                  return TDX_SW_ERROR;
> > 
> > Why did it change back? Based on the "mirror the other code for error handling
> > level", I guess there could be an argument for conditional.
> > 
> > Hmm, the other checks are about making the code work transparently work whether
> > PAMT is enabled or not. But this one is about handling code calling the function
> > incorrectly (since we don't support huge pages without dpamt). So I think I'd
> > still recommend what I suggested last time of no conditional return. With a
> > comment like you had. I also don't see anything blowing up too badly if the
> > warning is triggered.
> My concern is that without bailing out when DPAMT is not enabled, invoking
> tdx_find_dpamt_refcount() would cause a NULL pointer dereference.

If this function is called when PAMT is not available in the module (which
should be rare going forward), and it is called in a rare scenario from KVM that
is not caught during basic testing, then you would be left with an easily
debugable crash.

In the current code tdx_get_pamt_cache() in tdx_sept_split_leaf_spte() would
return NULL and you wouldn't even call tdh_mem_page_demote_2mb(). So it seems
like excess paranoia to me.

> 
> Or could I re-introduce the "dpamt" bool? This would allow huge pages to be
> supported even when DPAMT is not enabled (with no other places needing to
> change). The only downside is that it would introduce 3 additional 'if'
> comparisons:
> 
> u64 tdh_mem_page_demote_2mb(struct tdx_td *td, u64 gpa, kvm_pfn_t pfn, struct page *new_sept_pt,
>                             struct tdx_pamt_cache *pamt_cache, u64 *ext_err1, u64 *ext_err2)
> {
>         struct tdx_module_args args = {
>                 .rcx = gpa | pg_level_to_tdx_sept_level(PG_LEVEL_2M),
>                 .rdx = tdx_tdr_pa(td),
>                 .r8 = page_to_phys(new_sept_pt),
>         };
>         bool dpamt = tdx_supports_dynamic_pamt(&tdx_sysinfo);
>         struct page *dpamt_pages[TDX_DPAMT_ENTRY_PAGE_CNT];
>         atomic_t *dpamt_refcount;
>         u64 ret;
> 
>         *ext_err1 = 0;
>         *ext_err2 = 0;
> 
>         if (dpamt) {
>                 dpamt_refcount = tdx_find_dpamt_refcount(pfn);
> 
>                 ret = alloc_pamt_array(dpamt_pages, pamt_cache);
>                 if (ret)
>                         return TDX_SW_ERROR;
> 
>                 args.r12 = page_to_phys(dpamt_pages[0]);
>                 args.r13 = page_to_phys(dpamt_pages[1]);
> 
>                 /*
>                  * The dpamt_lock is used to avoid contention between the
>                  * TDH_MEM_PAGE_DEMOTE and TDH_PHYMEM_PAMT_{ADD/REMOVE} SEAMCALLs.
>                  */
>                 spin_lock(&dpamt_lock);
> 
>                 WARN_ON_ONCE(atomic_read(dpamt_refcount));
>         }
> 
> 	ret = seamcall_saved_ret(TDH_MEM_PAGE_DEMOTE, &args);
> 
>         *ext_err1 = args.rcx;
>         *ext_err2 = args.rdx;
> 
>         if (WARN_ON_ONCE(ret != TDX_SUCCESS))
>                 goto out_free;
> 
>         if (dpamt) {
>                 atomic_set(dpamt_refcount, PTRS_PER_PMD);
>                 spin_unlock(&dpamt_lock);
>         }
> 
>         return TDX_SUCCESS;
> 
> out_free:
>         if (dpamt) {
>                 spin_unlock(&dpamt_lock);
>                 free_pamt_array(pamt_cache, dpamt_pages);
>         }
>         return ret;
> }
> EXPORT_SYMBOL_FOR_KVM(tdh_mem_page_demote_2mb);
> 
> What do you think?

Adding support for a setup that will increasingly not exist in the real world as
the TDX modules get upgraded, for our basic initial implementation?

> 
> > > >          dpamt_refcount = tdx_find_dpamt_refcount(pfn);
> > > Can move this line after alloc_pamt_array() to mirror the sequence in
> > > tdx_pamt_get().
> > 
> > The less that stands out as having to think about, the better.
> > 
> > > 
> > > >          ret = alloc_pamt_array(dpamt_pages, pamt_cache);
> > > >          if (ret)
> Does the justification for WARN_ON_ONCE() on alloc_pamt_array() failure that I
> provided in https://lore.kernel.org/all/atBupzmYqvnB0Qcu@xxxxxxxxxxxxxxxxxxxxxxxxx
> look good to you?
> 
> Could you share your thoughts on it?

I don't see a comment above alloc_pamt_array(). If you mean the comment above
tdx_supports_dynamic_pamt(), I think it looks good.

> 
> > > >                  return TDX_SW_ERROR;
> > > 		  return ret;
> > > > 
> > > >          args.r12 = page_to_phys(dpamt_pages[0]),
> > > >          args.r13 = page_to_phys(dpamt_pages[1]),
> > > > 
> > > >          /*
> > > >           * The dpamt_lock is used to avoid contention between the
> > > >           * TDH_MEM_PAGE_DEMOTE and TDH_PHYMEM_PAMT_{ADD/REMOVE} SEAMCALLs.
> > > >           */
> > > >          spin_lock(&dpamt_lock);
> > > > 
> > > >          WARN_ON_ONCE(atomic_read(dpamt_refcount));
> > > > 
> > > >          ret = seamcall_saved_ret(TDH_MEM_PAGE_DEMOTE, &args);
> > > > 
> > > >          *ext_err1 = args.rcx;
> > > >          *ext_err2 = args.rdx;
> > > > 
> > > >          if (WARN_ON_ONCE(ret != TDX_SUCCESS))
> > > >                  goto out_free;
> > > > 
> > > >          atomic_set(dpamt_refcount, PTRS_PER_PMD);
> > > >          spin_unlock(&dpamt_lock);
> > > > 
> > > >          return TDX_SUCCESS;
> > > > 
> > > > out_free:
> > > >          spin_unlock(&dpamt_lock);
> > > >          reinsert_pamt_array(pamt_cache, dpamt_pages);
> > 
> > Is "reinsert" the right name here? It is just adding them to the cache and
> > resetting them. So why not just adjust free_pamt_array() to take a cache arg
> > instead of adding another function? In tdx_pamt_put() pass NULL so the pages go
> > back to the page allocator. Might need a prep patch. Would have to test.
> Will do.
> 
> > This patch will end up getting stripped down the minimum we need for this
> > series. Any new functions or extra code will get scrutiny.
> I agree!
> (my only concern is the risk of NULL pointer dereference).

As above, let's just match the existing code. Otherwise I think its just going
to get questions and not merged. We want this to have as little to talk about as
possible. If we want to change the defensive level (I don't), it is going to
need a bunch of prep patches instead of just embarking on a new paradigm for
additional huge page functions. I don't see a need to do that, and for the sake
of getting basic huge page support, I think that is a good thing. Right?

> 
> > > >          return ret;
> > > > }
> > > > EXPORT_SYMBOL_FOR_KVM(tdh_mem_page_demote_2mb);
> > 
> > Otherwise looks better to me.
> Thanks!