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

> 
> > > +		.rdx = tdx_tdr_pa(td),
> > > +		.r8 = page_to_phys(new_sept_pt),
> > > +		.r12 = page_to_phys(dpamt_pages[0]),
> > > +		.r13 = page_to_phys(dpamt_pages[1]),
> > > +	};
> > > +	u64 ret;
> > > +
> > > +	ret = seamcall_saved_ret(TDH_MEM_PAGE_DEMOTE, &args);
> > > +
> > > +	*ext_err1 = args.rcx;
> > > +	*ext_err2 = args.rdx;
> > > +	return ret;
> > > +}
> > > +
> > > +u64 tdh_mem_page_demote(struct tdx_td *td, u64 gpa, enum pg_level level,
> > > kvm_pfn_t pfn,
> > > +			struct page *new_sept_pt, struct tdx_pamt_cache
> > > *pamt_cache,
> > > +			u64 *ext_err1, u64 *ext_err2)
> > > +{
> > > +	atomic_t *dpamt_refcount = tdx_find_dpamt_refcount(pfn);
> > > +	struct page *dpamt_pages[TDX_DPAMT_ENTRY_PAGE_CNT];
> > > +	u64 ret;
> > > +
> > > +	*ext_err1 = 0;
> > > +	*ext_err2 = 0;
> > > +
> > > +	if
> > > (WARN_ON_ONCE(!tdx_huge_page_demote_uninterruptible(&tdx_sysinfo) ||
> > > +			 !tdx_supports_dynamic_pamt(&tdx_sysinfo) ||
> > > +			 level != PG_LEVEL_2M))
> > > +		return TDX_SW_ERROR;
> > > +
> > > +	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.

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.

> 
> I also noticed in the tdx_pamt_get() path, the return value of
> alloc_pamt_array()
> is passed back to KVM directly, triggering KVM_BUG_ON(). However, since Sean
> previously pointed out double warnings (i.e., both in demote() and KVM are
> acceptable): "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." [5], I finally
> added WARN_ON_ONCE() in this case.

"Two warns isn't the end of the world" is saying something different than "add
two warns when you can". So why do that here? If we have code that doesn't have
a clear reason it is going to be hard to get this patch acked. Kind of what I
was trying to say in the quote above.

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

> 
> Link: https://lore.kernel.org/all/aYoS45AZNY0rUJQD@google.com [5]

It looks like Sean was saying to warn about subtle contract breaking. He also
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.

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.

> 
> 
> > > +
> > > +	/*
> > > +	 * 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);
> > > +
> > > +	/*
> > > +	 * The caller ensures that DEMOTE is only invoked on a 2MB
> > > mapping, so
> > > +	 * dpamt_refcount must be 0.
> > > +	 */
> > > +	if (WARN_ON_ONCE(atomic_read(dpamt_refcount))) {
> > > +		ret = TDX_SW_ERROR;
> > > +		goto out_free;
> > 
> > What if we don't handle this case? It won't get caught by the (ret !=
> > TDX_SUCCESS) check below? I'm getting deja vu about all the defensive
> It should be caught due to DEMOTE seamcall failure.
> However, it makes me uncomfortable to set dpamt_refcount to 512 without first
> asserting that it's 0.

Hmm, yea it seems like good check to have. But I'm doubting how much we should
handle the error. Vs just warn about it. Not sure...

> 
> > programming. I swear made this comment before.
> You are right. Re-reading your comment made me realize I had missed that, as
> the error handling in the previous internal version was convoluted.
> 
> > > +	}
> > > +
> > > +	ret = __tdh_mem_page_demote(td, gpa, level, new_sept_pt,
> > > dpamt_pages,
> > > +				    ext_err1, ext_err2);
> > > +	if (ret != TDX_SUCCESS) {
> > > +		WARN_ON_ONCE((ret & TDX_SEAMCALL_STATUS_MASK) !=
> > > TDX_OPERAND_BUSY);
> > > +		goto out_free;
> > > +	}
> > > +
> > > +	atomic_set(dpamt_refcount, PMD_SIZE / PAGE_SIZE);
> > 
> > This used to be PTRS_PER_PMD and we talked about setting it to 512 per the
> > spec.
> > in an internal review:
> >     Decrementing PTRS_PER_PMD but the comment explicitly says 512. The TDX
> > spec
> >     explicitly says 512, so I'd just do atomic_set(pamt_refcount, 512);
> I did replace PTRS_PER_PMD with 512 after your comment. Then internally,
> Sashiko
> suggested:
> 
> "This isn't a bug, but would it be worth expressing the 512 symbolically
> rather than as a bare literal here?
> 
> The seed value has to match the granularity used elsewhere in this file.
> tdx_find_dpamt_refcount() indexes with:
> 
> 	pfn >> (PMD_SHIFT - PAGE_SHIFT)
> 
> and the array is sized with PTRS_PER_PTE. Each later tdx_pamt_put() on a
> 4KB page decrements this refcount one-by-one, and only the final 1->0
> transition fires TDH_PHYMEM_PAMT_REMOVE, so this seed represents the count
> of 4KB sub-pages in the 2MB region.
> 
> On x86-64 PTRS_PER_PTE == 512 == PMD_SIZE/PAGE_SIZE, so the value is
> correct today. Using PTRS_PER_PTE (or a named define) instead of the
> literal 512 would keep this seed in lockstep with the indexing logic if
> the PMD/page geometry ever changes. Was a bare literal preferred here for a
> reason?"
> 
> Given that the TDX spec refers to 512 as: "The host VMM can split the mapping
> of a single 2MB or 1GB page to 512 consecutive 4KB or 2MB pages...", I think
> using PMD_SIZE / PAGE_SIZE would be more appropriate here (though personally I
> think PTRS_PER_PMD is also fine).
> 
> I didn't consider it as a big issue so I didn't mention it in the chagnelog.

I think it kinda is actually. It was a point of discussion that got settled and
then silently changed. This makes it hard to review successive versions of the
series. It all has to be reviewed from scratch.

The right thing to do would have been to re-open that thread where it got
closed. Sashiko doesn't get a style vote. but you do if you changed your mind.
But changing your mind is not a reason to silently change the code to something
other than discussed.

My pref would be 512 or PTRS_PER_PMD before PMD_SIZE / PAGE_SIZE.

> 
> > Now it has changed to PMD_SIZE / PAGE_SIZE. There seems to have been at
> > least a few changes that missed the version log. What happened? AI review?
> > We need to make sure we are not rolling back previous decisions silently...
> Will mark this kind of change next time.
> 
> > Can you share more widely if there are new unmentioned changes in the rest
> > of
> > the series?
> Besides the main changes listed in the "Main changes from v3" section of the
> changelog, and those already documented in the changelog of each patch, the
> following changes vs v3 are undocumented:
> 
> Patch 1: added a level arg and have the callee to decide whether to skip PAMT
>          add/put.
> 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.

>          - added a helper __tdh_mem_page_demote().

why?

> Patch 3: implementation is much simpler due to the the base code changes
>          introduced by the previous MMU refactor and folio/page to pfn series.
> Patch 4: - Moved is_mirror_sp() immediately above disallowed_hugepage_adjust()
>            instead of saving it in a local variable.
> 	 - added a comment explaining the disallowing reason.