Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers

[Sean Christopherson <seanjc@google.com>]

Gah, forgot to hit "send" on this.

On Wed, Nov 26, 2025, Rick P Edgecombe wrote:
> On Tue, 2025-11-25 at 16:09 +0800, Binbin Wu wrote:
> > On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> > > +/*
> > > + * The TDX spec treats the registers like an array, as they are ordered
> > > + * in the struct. The array size is limited by the number or registers,
> > > + * so define the max size it could be for worst case allocations and sanity
> > > + * checking.
> > > + */
> > > +#define MAX_TDX_ARG_SIZE(reg) (sizeof(struct tdx_module_args) - \
> > > +			       offsetof(struct tdx_module_args, reg))
> > 
> > This should be the maximum number of registers could be used to pass the
> > addresses of the pages (or other information), it needs to be divided by sizeof(u64).
> 
> Oops, right.
> 
> > 
> > Also, "SIZE" in the name could be confusing.
> 
> Yes LENGTH is probably better.
> 
> > 
> > > +#define TDX_ARG_INDEX(reg) (offsetof(struct tdx_module_args, reg) / \
> > > +			    sizeof(u64))

Honestly, the entire scheme is a mess.  Four days of staring at this and I finally
undertand what the code is doing.  The whole "struct tdx_module_array_args" union
is completely unnecessary, the resulting args.args crud is ugly, having a pile of
duplicate accessors is brittle, the code obfuscates a simple concept, and the end
result doesn't provide any actual protection since the kernel will happily overflow
the buffer after the WARN.

It's also relying on the developer to correctly copy+paste the same register in
multiple locations: ~5 depending on how you want to count.

  static u64 *dpamt_args_array_ptr_r12(struct tdx_module_array_args *args)
                                   #1
  {
	WARN_ON_ONCE(tdx_dpamt_entry_pages() > MAX_TDX_ARGS(r12));
                                                            #2

	return &args->args_array[TDX_ARG_INDEX(r12)];
                                               #3


	u64 guest_memory_pamt_page[MAX_TDX_ARGS(r12)];
                                                #4


	u64 *args_array = dpamt_args_array_ptr_r12(&args);
                                                   #5

After all of that boilerplate, the caller _still_ has to do the actual memcpy(),
and for me at least, all of the above makes it _harder_ to understand what the code
is doing.

Drop the struct+union overlay and just provide a helper with wrappers to copy
to/from a tdx_module_args structure.  It's far from bulletproof, but it at least
avoids an immediate buffer overflow, and defers to the kernel owner with respect
to handling uninitialized stack data.

/*
 * For SEAMCALLs that pass a bundle of pages, the TDX spec treats the registers
 * like an array, as they are ordered in the struct.  The effective array size
 * is (obviously) limited by the number or registers, relative to the starting
 * register.  Fill the register array at a given starting register, with sanity
 * checks to avoid overflowing the args structure.
 */
static void dpamt_copy_regs_array(struct tdx_module_args *args, void *reg,
				  u64 *pamt_pa_array, bool copy_to_regs)
{
	int size = tdx_dpamt_entry_pages() * sizeof(*pamt_pa_array);

	if (WARN_ON_ONCE(reg + size > (void *)args) + sizeof(*args))
		return;

	/* Copy PAMT page PA's to/from the struct per the TDX ABI. */
	if (copy_to_regs)
		memcpy(reg, pamt_pa_array, size);
	else
		memcpy(pamt_pa_array, reg, size);
}

#define dpamt_copy_from_regs(dst, args, reg)	\
	dpamt_copy_regs_array(args, &(args)->reg, dst, false)

#define dpamt_copy_to_regs(args, reg, src)	\
	dpamt_copy_regs_array(args, &(args)->reg, src, true)

As far as the on-stack allocations go, why bother being precise?  Except for
paranoid setups which explicitly initialize the stack, "allocating" ~48 unused
bytes is literally free.  Not to mention the cost relative to the latency of a
SEAMCALL is in the noise.

/*
 * When declaring PAMT arrays on the stack, use the maximum theoretical number
 * of entries that can be squeezed into a SEAMCALL, as stack allocations are
 * practically free, i.e. any wasted space is a non-issue.
 */
#define MAX_NR_DPAMT_ARGS (sizeof(struct tdx_module_args) / sizeof(u64))


With that, callers don't have to regurgitate the same register multiple times,
and we don't need a new wrapper for every variation of SEAMCALL.  E.g.


	u64 pamt_pa_array[MAX_NR_DPAMT_ARGS];

	...

	bool dpamt = tdx_supports_dynamic_pamt(&tdx_sysinfo) && level == PG_LEVEL_2M;
	u64 pamt_pa_array[MAX_NR_DPAMT_ARGS];
	struct tdx_module_args args = {
		.rcx = gpa | pg_level_to_tdx_sept_level(level),
		.rdx = tdx_tdr_pa(td),
		.r8 = page_to_phys(new_sp),
	};
	u64 ret;

	if (!tdx_supports_demote_nointerrupt(&tdx_sysinfo))
		return TDX_SW_ERROR;

	if (dpamt) {
		if (alloc_pamt_array(pamt_pa_array, pamt_cache))
			return TDX_SW_ERROR;

		dpamt_copy_to_regs(&args, r12, pamt_pa_array);
	}

Which to me is easier to read and much more intuitive than:


	u64 guest_memory_pamt_page[MAX_TDX_ARGS(r12)];
	struct tdx_module_array_args args = {
		.args.rcx = gpa | pg_level_to_tdx_sept_level(level),
		.args.rdx = tdx_tdr_pa(td),
		.args.r8 = PFN_PHYS(page_to_pfn(new_sp)),
	};
	struct tdx_module_array_args retry_args;
	int i = 0;
	u64 ret;

	if (dpamt) {
		u64 *args_array = dpamt_args_array_ptr_r12(&args);

		if (alloc_pamt_array(guest_memory_pamt_page, pamt_cache))
			return TDX_SW_ERROR;

		/*
		 * Copy PAMT page PAs of the guest memory into the struct per the
		 * TDX ABI
		 */
		memcpy(args_array, guest_memory_pamt_page,
		       tdx_dpamt_entry_pages() * sizeof(*args_array));
	}