Re: [PATCH v2 2/4] x86/tdx: Use PFN directly for unmapping guest private memory

Edgecombe, Rick P
2026-04-30T18:38:15Z

On Thu, 2026-04-30 at 11:17 -0700, Ackerley Tng wrote:
> Yan Zhao <yan.y.zhao@intel.com> writes:
>
> >
> > [...snip...]
> >
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index b24b81cea5ea..e5a37ea2d4a0 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -710,7 +710,7 @@ static __init int tdmrs_set_up_pamt_all(struct tdmr_info_list *tdmr_list,
> >    * to normal kernel memory. Systems with the X86_BUG_TDX_PW_MCE erratum need to
> >    * do the conversion explicitly via MOVDIR64B.
> >    */
> > -static void tdx_quirk_reset_paddr(unsigned long base, unsigned long size)
> > +void tdx_quirk_reset_paddr(unsigned long base, unsigned long size)
>
> Could this be updated to use phys_addr_t base and size_t size instead of
> generic unsigned long?

A type is a really good idea. It could look like a virtual address, despite the
paddr in the name.

I added it to the cleanup list. But I would prefer to keep this series focused
on resolving the critical controversy around the struct page/pfn type, rather
than adding in more things to debate. If you don't mind.

>
> >   {
> >    const void *zero_page = (const void *)page_address(ZERO_PAGE(0));
> >    unsigned long phys, end;
> > @@ -729,6 +729,7 @@ static void tdx_quirk_reset_paddr(unsigned long base, unsigned long size)
> >    */
> >    mb();
> >   }
> > +EXPORT_SYMBOL_FOR_KVM(tdx_quirk_reset_paddr);
> >
> >   void tdx_quirk_reset_page(struct page *page)
> >   {
> > @@ -1920,17 +1921,17 @@ u64 tdh_phymem_page_wbinvd_tdr(struct tdx_td *td)
> >   {
> >    struct tdx_module_args args = {};
> >
> > - args.rcx = mk_keyed_paddr(tdx_global_keyid, td->tdr_page);
> > + args.rcx = mk_keyed_paddr(tdx_global_keyid, page_to_pfn(td->tdr_page));
>
> Should mk_keyed_paddr() be updated to have a return type of phys_addr_t?
> I guess in this case since mk_keyed_paddr() is pretty much an internal
> function, returning u64 also makes sense to indicate that it should only
> be used to set 64 bit registers.

Yea, this is used to construct u64 inputs for seamcall args. So I think it
should keep returning u64s. Maybe instead it would be better to have a function
name pattern that denotes that purpose. We have some more helpers like this
coming.

But similarly, I'd like to not grow a snowballing cleanup series for this one.