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

[Edgecombe, Rick P]

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.


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

This patch will end up getting stripped down the minimum we need for this
series. Any new functions or extra code will get scrutiny.

> > 
> >          return ret;
> > }
> > EXPORT_SYMBOL_FOR_KVM(tdh_mem_page_demote_2mb);

Otherwise looks better to me.