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

[Yan Zhao]

On Tue, Oct 13, 2026 at 06:16:46AM +0800, Edgecombe, Rick P wrote:
> On Mon, 2026-10-12 at 17:31 +0800, Yan Zhao wrote:
> > > we don't need to support anything other than 2MB, right?
> > Yeah, with tdh_mem_page_demote_2mb(), I'll hardcode the level to 2MB here.
> 
> Just to be sure... We don't need the level arg anymore, so don't need to warn
> about it.
Got it.

> > > > +	if (WARN_ON_ONCE(alloc_pamt_array(dpamt_pages, pamt_cache)))
> > > > +		return TDX_SW_ERROR;
> > > 
> > > Why handle this differently than other pamt flows? It also looks like this
> > > warning was added since the last version I saw...
> > Right. The warning was added since v4 due to the internal review.
> > 
> > I added this WARN_ON_ONCE() after alloc_pamt_array() fails based on the
> > following internal review comment: 
> > "Hmm, so this makes me wonder, what kind of error do we want to return to KVM.
> > Does KVM ever expect this to fail? We handle PAGE_AUG failing, but can we talk
> > about how KVM will call this. What kind of failures does it want to know about
> > and what kind of failures can it do something about? Otherwise to review this
> > patch you need to look ahead to the code and understand the KVM MMU. Is it
> > possible to give some justification in the log for why the error exposure is
> > the way it is?"
> 
> I don't see the connection to turning that into a warning.
Oh, sorry. I was a bit rushed with the last reply.

Turning it into a warning was because:
1. defensive programming:
   In KVM, kvm_mmu_memory_cache_alloc() triggers BUG_ON() when there's no page
   in a cache. So, I think we can make a warning when there's no page in
   pamt_cache.

2. your previous comment to "level != 2M" checking:
   "Hmm, this could all be a warnings I think. I guess it wades into Sean's new
    tendency to survive MMU errors. But do we think there is special risk here?
    What horrible thing can go wrong? I'd want a justification for why we need
    to handle this bug and not just report it via warning."

3. Sean's previous comments related to "two WARNs". 

> But besides that, please don't silently churn the code like this. Especially
> when you provide a version log, because it kind of implies that nothing else was
> changed.
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).

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

> > 
> > Link: https://lore.kernel.org/all/aYoS45AZNY0rUJQD@xxxxxxxxxx [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?

> 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@xxxxxxxxxxxxxxxxxxxxxxxxx:
   "
   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@xxxxxxxxx/
   [2] https://lore.kernel.org/all/baf6df2cc63d8e897455168c1bf07180fc9c1db8.camel@xxxxxxxxx
   ".

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


> 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@xxxxxxxxxx.
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));

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

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

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