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

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Wed, 2026-10-14 at 17:40 +0800, Yan Zhao wrote:
> 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).

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.

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

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

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

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

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

As discussed above.

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