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

[Yan Zhao <yan.y.zhao@intel.com>]

On Thu, Oct 15, 2026 at 02:32:46AM +0800, Edgecombe, Rick P wrote:
> > Ok. Will do.
> > (I took the easy way out by only listing the major ones, as the changes from v3
> > to v4 involved Sean's v5,  many internal review versions, and changes due to
> > rebasing to the latest DPAMT).
> 
> Then just say:
>  - Lots of minor changes from AI review (Sashiko)
> 
> The serious problem is something that implies that it is unchanged but actually
> is changed.
I see. Thanks!
I'll avoid it next time.

> > > > Should I also add a comment such as "The caller must ensure there are enough
> > > > pages in the pamt_cache"?
> > > 
> > > I think it's obvious. That is the whole point of the cache.
> > So, are you suggesting just propagating the error to KVM without the warning in
> > demote()?
> 
> Maybe instead you can answer why do anything different here than the other
> alloc_pamt_array() callers? Is there something special? Then you can justify it.
> 
> Otherwise, it looks like trying to change the norms of the existing code.
> Regardless of whether it is even a good idea, I think you have enough on your
> hands to get this series upstream.
> > > > Link: https://lore.kernel.org/all/aYoS45AZNY0rUJQD@google.com [5]
> > > 
> > > It looks like Sean was saying to warn about subtle contract breaking. He also
> > No argues. Just want to ensure I can write the code correctly in the future:
> > 
> > If KVM invokes DEMOTE without topup enough pages in cache, is it a subtle
> > contract breaking?
> 
> Doesn't seem that subtle to me? It is not too different than passing NULL
> because your allocation failed.
Hmm, ok. Now I see the difference:
tdx_pamt_get() can't trigger WARN_ON_ONCE() when alloc_pamt_array() fails,
because cache is allowed to be NULL, so alloc_pamt_array() in tdx_pamt_get() is
possible and allowed to fail due to out of memory. So,
- in the NULL-cache paths, KVM handles the error and fails the ioctl;
- in the !NULL-cache case, KVM triggers KVM_BUG_ON().

However, the cache should always be !NULL in demote() and alloc_pamt_array() is
never expected to fail for demote(), so I added WARN_ON_ONCE() to catch this
contract violation.

So, does this difference justify the WARN_ON_ONCE() on alloc_pamt_array()
failure in demote()?

Besides, I noticed there's a WARN_ON_ONCE() in tdx_pamt_get() when
tdh_phymem_pamt_add() fails:
    "if (WARN_ON_ONCE(tdx_status != TDX_SUCCESS))",
Should I also add this when TDH_MEM_PAGE_DEMOTE() fails? 

> > > says in that thread:
> > >    Document what exactly?  Most of this should be common sense.  E.g. we don't
> > >    generally document that pointers must be non-NULL, because that goes without
> > >    saying 99.9% of the time.
> > Here are more background:
> > 1. I asked in https://lore.kernel.org/all/aW3G6yZuvclYABzP@yzhao56-desk.sh.intel.com:
> >    "
> >    BTW, I have another question about the SEAMCALL wrapper implementation, as Kai
> >    also pointed out in [2]: since the SEAMCALL wrappers now serve as APIs available
> >    to callers besides KVM, should the SEAMCALL wrappers return TDX_OPERAND_INVALID
> >    or WARN_ON() (or WARN_ON_ONCE()) on sanity check failure?
> > 
> >    By returning TDX_OPERAND_INVALID, the caller can check the return code, adjust
> >    the input or trigger WARN_ON() by itself;
> >    By triggering WARN_ON() directly in the SEAMCALL wrapper, we need to document
> >    this requirement for the SEAMCALL wrappers and have the caller invoke the API
> >    correctly.
> > 
> >    So, it looks that "WARN_ON() directly in the SEAMCALL wrapper" is the preferred
> >    approach, right?
> > 
> >    [1] https://lore.kernel.org/all/d119c824-4770-41d2-a926-4ab5268ea3a6@intel.com/
> >    [2] https://lore.kernel.org/all/baf6df2cc63d8e897455168c1bf07180fc9c1db8.camel@intel.com
> >    ".
> > 
> > 2. Sean replied:
> >    "Why not both?  But maybe TDX_SW_ERROR instead of TDX_OPERAND_INVALID?".
> > 
> > 3. I said:
> >    "The reason for 'not both' is that there's already TDX_BUG_ON_2() in KVM after
> >    the SEAMCALL wrapper returns a non-BUSY error. I'm not sure if having double
> >    WARN_ON_ONCE() calls is good, so I intended to let the caller decide whether to
> >    warn."
> > 
> > 4. Sean said "Two WARNs isn't the end of the world.  It might even be helpful in
> >    some cases, e.g. to more precisely document what went wrong."
> 
> Let's just mirror the existing, very recently discussed and agreed code unless
> we can say there is a new different problem with demote.
Please check if the difference mentioned above can justify the warning.


> > > But besides that we are trying to get Dave's ack here. So I don't think
> > > extrapolating a comment Sean made is going to suffecient for having code that is
> > > different than the rest without explanation. So let's think about how to get
> > > that ack. Obviously we should take Sean's input as well. But while we are on the
> > > maintainer boundary, we can't call it settled.
> > Hmm, yes.
> > Not sure if Dave likes "two WARNs" or not as Sean suggested in
> > https://lore.kernel.org/all/aXzPIO2qZwuwaeLi@google.com.
> > He was in that thread, but maybe he didn't see Sean's mails.
> > 
> > To avoid extrapolating, what about suppressing most WARN_ON_ONCE() in demote()
> > in the next version? (see the code at tail of this mail).
> >  
> > > > Patch 2: - PTRS_PER_PMD to PMD_SIZE / PAGE_SIZE,
> > > >          - atomic_cmpxchg_release() to atomic_set() due to the spinlock
> > > > 	   protection.
> > > 
> > > Can you explain more on this one? I missed it.
> > There was no spin lock to protect DEMOTE SEAMCALL execution, so
> > atomic_cmpxchg_release() was used to ensure the original refcount is 0 while
> > updating new refcount to 512.
> > 
> > Now, with the dpamt_lock protection, atomic_set() is used to align with the way
> > of updating refcount in tdx_pamt_get() and tdx_pamt_put(), though the atomic_*()
> > version is actually unnecessary.
> > 
> > Or since the atomic_*() version is anyway unnecessary, what about adding the
> > asserting of 0 like this?
> > 	WARN_ON_ONCE(atomic_cmpxchg_release(dpamt_refcount, 0,
> > 					    PTRS_PER_PMD));
> 
> We want this patch to make the most sense on top of the existing code. Try to
> make it as simple as possible. Don't worry as much about if the code is 100%
> optimal. To get the code into a the desired state, it is way, way easier if 99%
> of it is already upstream. So keep in mind we can continue to develop it.
> 
> Doing everything inside a giant lock is pretty simple pattern so I don't see a
> giant risk. Do you?
Yeah, I'm not concerned about code optimization here, so, I used atomic_set() in
v4.
My question is if atomic_cmpxchg_release() would look better to you, since it
implicitly checks that dpamt_refcount is 0 before setting the value, allowing me
to drop 

	if (WARN_ON_ONCE(atomic_read(dpamt_refcount))) {
		ret = TDX_SW_ERROR;
		goto out_free;
	}

But from your later comment, it seems you prefer atomic_set()?

> > > >          - added a helper __tdh_mem_page_demote().
> > > 
> > > why?
> > To make the helper focused on invoking the DEMOTE SEAMCALL, without dealing with
> > locks, refcounts, page retrieval, etc.  similar to how tdh_phymem_pamt_add()
> > relates to tdx_pamt_get().
> > Do you think the helper is unnecessary?
> > 
> > Does the following implementation look good to you?
> > changes from v4:
> > - dropped __tdh_mem_page_demote() helper
> > - dropped "if (WARN_ON_ONCE(atomic_read(dpamt_refcount)". Used
> >   atomic_cmpxchg_release() and WARN_ON_ONCE() on 0 original refcount.
> > - used PTRS_PER_PMD.
> > - invoke reinsert_pamt_array() on failure.
> > - renamed to tdh_mem_page_demote_2mb() and dropped 2MB checking.
> > - dropped WARN_ON_ONCE() for DPAMT is not supported, alloc_pamt_array() failure
> >   and SEAMCALL failure.
> > 
> > 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),
> >         };
> >         atomic_t *dpamt_refcount = tdx_find_dpamt_refcount(pfn);
> >         struct page *dpamt_pages[TDX_DPAMT_ENTRY_PAGE_CNT];
> >         *ext_err1 = 0;
> >         *ext_err2 = 0;
> >         u64 ret;
> > 
> >         if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> >                 return TDX_SW_ERROR;
> 
> Do we need to handle this logically? The below will return some error in any
> case, I'd think. I'd make it a warning with no conditional. The comment of the
> limitation is important and the warning does that.
>
> In general DPAMT maintenance is also verified by the TDX module when the pages
> it covers get used. So if we get things wrong, there are already duplicate
> checks to catch things.
Ok. So like this?

	/* 
	 * For implementation simplicity, only support demotion when
	 * DPAMT is enabled.
	 */
	WARN_ON_ONCE(!tdx_supports_dynamic_pamt(&tdx_sysinfo));

> > 
> >         ret = alloc_pamt_array(dpamt_pages, pamt_cache);
> >         if (ret)
> >                 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);
If you prefer using atomic_set() to adjust dpamt_refcount, what about inserting
           WARN_ON_ONCE(atomic_read(dpamt_refcount);

here,

> >         ret = seamcall_saved_ret(TDH_MEM_PAGE_DEMOTE, &args);
> > 
> >         *ext_err1 = args.rcx;
> >         *ext_err2 = args.rdx;
> > 
> >         if (ret != TDX_SUCCESS)
> >                 goto out_free;
> > 
> >         atomic_set(dpamt_refcount, PTRS_PER_PMD);
> >         WARN_ON_ONCE(atomic_cmpxchg_release(dpamt_refcount, 0,
> >                                             PTRS_PER_PMD));
> 
> As discussed above.
then invoke 
	atomic_set(dpamt_refcount, PTRS_PER_PMD);           
here ?
 
> >         spin_unlock(&dpamt_lock);
> > 
> >         return TDX_SUCCESS;
> > 
> > out_free:
> >         spin_unlock(&dpamt_lock);
> >         reinsert_pamt_array(pamt_cache, dpamt_pages);
> > 
> >         return ret;
> > }
> > EXPORT_SYMBOL_FOR_KVM(tdh_mem_page_demote_2mb);
> > 
>