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

["Huang, Kai" <kai.huang@intel.com>]

On Wed, 2025-07-23 at 18:30 +0300, Hunter, Adrian wrote:
> On 23/07/2025 17:44, Edgecombe, Rick P wrote:
> > On Wed, 2025-07-23 at 17:37 +0300, Adrian Hunter wrote:
> > > > The latter seems better to me for the sake of less churn.
> > > 
> > > Why make tdx_quirk_reset_page() and tdx_quirk_reset_paddr() follow
> > > different rules.
> > > 
> > > How about this:
> > > 
> > > From: Adrian Hunter <adrian.hunter@intel.com>
> > > Subject: [PATCH] x86/tdx: Tidy reset_pamt functions
> > > 
> > > Rename reset_pamt functions to contain "quirk" to reflect the new
> > > functionality, and remove the now misleading comment.
> > 
> > This looks like the "former" option. Churn is not too bad, and it has the
> > benefit of clear code vs long comment. I'm ok either way. But it needs to go
> > cleanup first in the patch order.
> > 
> > The log should explain why it's ok to change now, with respect to the reasoning
> > in the comment that is being removed.
> 
> It makes more sense afterwards because then it can refer to the
> functional change:
> 
> From: Adrian Hunter <adrian.hunter@intel.com>
> Subject: [PATCH] x86/tdx: Tidy reset_pamt functions
> 
> tdx_quirk_reset_paddr() has been made to reflect that, in fact, the
> clearing is necessary only for hardware with a certain quirk.  Refer
> patch "x86/tdx: Skip clearing reclaimed pages unless X86_BUG_TDX_PW_MCE
> is present" for details.
> 
> Rename reset_pamt functions to contain "quirk" to reflect the new
> functionality, and remove the now misleading comment.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index ef22fc2b9af0..823850399bb7 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -664,17 +664,17 @@ void tdx_quirk_reset_page(struct page *page)
>  }
>  EXPORT_SYMBOL_GPL(tdx_quirk_reset_page);
>  
> -static void tdmr_reset_pamt(struct tdmr_info *tdmr)
> +static void tdmr_quirk_reset_pamt(struct tdmr_info *tdmr)
>  {
>  	tdmr_do_pamt_func(tdmr, tdx_quirk_reset_paddr);
>  }
>  
> -static void tdmrs_reset_pamt_all(struct tdmr_info_list *tdmr_list)
> +static void tdmrs_quirk_reset_pamt_all(struct tdmr_info_list *tdmr_list)
>  {
>  	int i;
>  
>  	for (i = 0; i < tdmr_list->nr_consumed_tdmrs; i++)
> -		tdmr_reset_pamt(tdmr_entry(tdmr_list, i));
> +		tdmr_quirk_reset_pamt(tdmr_entry(tdmr_list, i));
>  }
>  
>  static unsigned long tdmrs_count_pamt_kb(struct tdmr_info_list *tdmr_list)
> @@ -1146,15 +1146,7 @@ static int init_tdx_module(void)
>  	 * to the kernel.
>  	 */
>  	wbinvd_on_all_cpus();
> -	/*
> -	 * According to the TDX hardware spec, if the platform
> -	 * doesn't have the "partial write machine check"
> -	 * erratum, any kernel read/write will never cause #MC
> -	 * in kernel space, thus it's OK to not convert PAMTs
> -	 * back to normal.  But do the conversion anyway here
> -	 * as suggested by the TDX spec.
> -	 */
> -	tdmrs_reset_pamt_all(&tdx_tdmr_list);
> +	tdmrs_quirk_reset_pamt_all(&tdx_tdmr_list);
>  err_free_pamts:
>  	tdmrs_free_pamt_all(&tdx_tdmr_list);
>  err_free_tdmrs:
> -- 
> 2.48.1


Such renaming goes a little bit far IMHO.  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.

E.g.,:

static void tdx_quirk_reset_paddr(unsigned long base, unsigned long size)
{
	/* doing MOVDIR64B ... */
}

static void tdx_reset_paddr(unsigned long base, unsigned long size)
{
	if (!boot_cpu_has_bug(X86_BUG_TDX_PW_MCE))
		return;

	tdx_quirk_reset_paddr(base, size);
}

void tdx_reset_page(struct page *page)
{
	tdx_reset_paddr(page_to_phys(page), PAGE_SIZE);
}
EXPORT_SYMBOL_GPL(tdx_reset_page);