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

[Yan Zhao]

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.

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?

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

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

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