Re: [PATCH V4 1/2] x86/tdx: Eliminate duplicate code in tdx_clear_page()

[Huang, Kai]

On Wed, 2025-07-23 at 23:26 +0000, Edgecombe, Rick P wrote:
> On Wed, 2025-07-23 at 23:01 +0000, Huang, Kai wrote:
> > Such renaming goes a little bit far IMHO.
> > 
> 
> I agree it's not quite necessary churn.
> 
> >   I respect the value of having
> > "quirk" in the name, but it also seems quite reasonable to me to hide such
> > "quirk" at the last level but just having "reset TDX pages" concept in the
> > higher levels.
> 
> Assuming all the comments get corrected, this still leaves "reset" as an
> operation that sometimes eagerly resets the page, or sometimes leaves it to be
> lazily done later by a random access. 
> 

Thanks for the point.

Yeah I agree it's better to convey such information in the function name.


> Maybe instead of reset which is an action
> that sometimes is skipped, something that says what state we want the page to be
> at the end - ready to use.
> 
> tdx_make_page_ready()
> tdx_make_page_usable()
> ...or something in that direction.
> 
> But this is still churn. Kai, what do you think about the other option of just
> putting the X86_BUG_TDX_PW_MCE in tdx_reset_page() and letting the
> initialization error path (tdmrs_reset_pamt_all()) keep always zeroing the
> pages. So:
> 
> static void tdx_reset_paddr(unsigned long base, unsigned long size)
> {
> 	/* doing MOVDIR64B ... */
> }
> 
> static void tdmr_reset_pamt(struct tdmr_info *tdmr)
> {
> 	tdmr_do_pamt_func(tdmr, tdx_reset_paddr);
> }
> 
> void tdx_quirk_reset_page(struct page *page)
> {
> 	if (!boot_cpu_has_bug(X86_BUG_TDX_PW_MCE))
> 		return;
> 
> 	tdx_reset_paddr(page_to_phys(page), PAGE_SIZE);
> }
> EXPORT_SYMBOL_GPL(tdx_reset_page);

I don't think it's good idea to treat PAMT and other types of TDX memory
differently.  I would rather go with the renaming as shown in Adrian's
patch.

So no objection from me. :-)