Re: [PATCHv2 04/12] x86/virt/tdx: Add tdx_alloc/free_page() helpers

[Dave Hansen <dave.hansen@intel.com>]

On 6/9/25 12:13, Kirill A. Shutemov wrote:
>  arch/x86/include/asm/tdx.h       |   3 +
>  arch/x86/include/asm/tdx_errno.h |   6 +
>  arch/x86/virt/vmx/tdx/tdx.c      | 205 +++++++++++++++++++++++++++++++
>  arch/x86/virt/vmx/tdx/tdx.h      |   2 +
>  4 files changed, 216 insertions(+)

Please go through this whole series and add appropriate comments and
explanations.

There are 4 lines of comments in the 216 lines of new code.

I'll give some examples:

> +static int tdx_nr_pamt_pages(void)

Despite the naming this function does not return the number of TDX
PAMT pages. It returns the number of pages needed for each *dynamic*
PAMT granule.

The naming is not consistent with something used only for dynamic PAMT
support. This kind of comment would help, but is not a replacement for
good naming:

/*
 * How many pages are needed for the TDX
 * dynamic page metadata for a 2M region?
 */

Oh, and what the heck is with the tdx_supports_dynamic_pamt() check?
Isn't it illegal to call these functions without dynamic PAMT in
place? Wouldn't the TDH_PHYMEM_PAMT_ADD blow up if you hand it 0's
in args.rdx?

> +static int tdx_nr_pamt_pages(void)
> +{
> +	if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> +		return 0;
> +
> +	return tdx_sysinfo.tdmr.pamt_4k_entry_size * PTRS_PER_PTE / PAGE_SIZE;
> +}
> +
> +static u64 tdh_phymem_pamt_add(unsigned long hpa,
> +			       struct list_head *pamt_pages)
> +{
> +	struct tdx_module_args args = {
> +		.rcx = hpa,
> +	};
> +	struct page *page;
> +	u64 *p;
> +
> +	WARN_ON_ONCE(!IS_ALIGNED(hpa & PAGE_MASK, PMD_SIZE));
> +
> +	p = &args.rdx;
> +	list_for_each_entry(page, pamt_pages, lru) {
> +		*p = page_to_phys(page);
> +		p++;
> +	}

This is sheer voodoo. Voodoo on its own is OK. But uncommented voodoo
is not.

Imagine what would happen if, for instance, someone got confused and did:

	tdx_alloc_pamt_pages(&pamd_pages);
	tdx_alloc_pamt_pages(&pamd_pages);
	tdx_alloc_pamt_pages(&pamd_pages);

It would *work* because the allocation function would just merrily
shove lots of pages on the list. But when it's consumed you'd run off
the end of the data structure in this function far, far away from the
bug site.

The least you can do here is comment what's going on. Because treating
a structure like an array is obtuse at best.

Even better would be to have a check to ensure that the pointer magic
doesn't run off the end of the struct:

	if (p - &args.rcx >= sizeof(args)/sizeof(u64)) {
		WARN_ON_ONCE(1);
		break;
	}

or some other pointer voodoo.

> +
> +	return seamcall(TDH_PHYMEM_PAMT_ADD, &args);
> +}