Re: [PATCH v6 03/11] x86/virt/tdx: Add tdx_alloc/free_control_page() helpers

[Edgecombe, Rick P]

On Mon, 2026-07-06 at 20:31 +0800, Chao Gao wrote:
> On Mon, May 25, 2026 at 07:35:07PM -0700, Rick Edgecombe wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
> > 
> > Add helpers to use when allocating or preparing pages that are handed to
> > the TDX-Module for use as control/S-EPT pages, and thus need Dynamic PAMT
> 
> For the whole series: s/TDX-Module/TDX module, to match the existing
> convention.

Doh, I actually changed the whole series to TDX-module before realizing that
most of the references are "TDX module". So I thought to standardize on that
going forward. I missed this one when changing back.

> 
> > +/*
> > + * Calculate the arg needed for operating on the DPAMT backing for
> > + * a given 4KB page.
> > + */
> > +static u64 pamt_2mb_arg(kvm_pfn_t pfn)
> > +{
> > +	unsigned long hpa_2mb = ALIGN_DOWN(pfn << PAGE_SHIFT, PMD_SIZE);
> 
> The changelog and the comment don't explain why the pfn is aligned down to a
> 2MB boundary. And ...
> 
> > +
> > +	return hpa_2mb | TDX_PS_2M;
> > +}
> > +
> > +/* Add PAMT backing for the given page. */
> 
> ... this adds PAMT backing for the whole 2MB region containing the given page,
> not just the given page itself. Is it worth clarifying in the comment?

Yea that is a good comment.

> 
> <snip>
> 
> > +/*
> > + * Return a page that can be gifted to the TDX-Module for use as a "control"
> > + * page, i.e. pages that are used for control and S-EPT structures for a given
> > + * TDX guest, and bound to said guest's HKID and thus obtain TDX protections,
> > + * including PAMT tracking.
> > + */
> 
> IIUC, S-EPT structures are not allocated through this helper. Why mention it?

Oh, yea that is removed with v6. I'll update it.

> 
> > +struct page *tdx_alloc_control_page(void)
> > +{
> > +	struct page *page;
> > +
> > +	page = alloc_page(GFP_KERNEL_ACCOUNT);
> > +	if (!page)
> > +		return NULL;
> > +
> > +	if (tdx_pamt_get(page_to_pfn(page))) {
> > +		__free_page(page);
> > +		return NULL;
> > +	}
> > +
> > +	return page;
> > +}
> > +EXPORT_SYMBOL_FOR_KVM(tdx_alloc_control_page);
> > +
> > +/*
> > + * Free a page that was gifted to the TDX-Module for use as a control/S-EPT
> > + * page. After this, the page is no longer protected by TDX.
> > + */
> 
> Ditto.
> 
> > +void tdx_free_control_page(struct page *page)
> > +{
> > +	if (!page)
> > +		return;
> > +
> > +	tdx_pamt_put(page_to_pfn(page));
> > +	__free_page(page);
> > +}
> > +EXPORT_SYMBOL_FOR_KVM(tdx_free_control_page);
> 
> With above nits fixed,
> 
> Reviewed-by: Chao Gao <chao.gao@xxxxxxxxx>