Re: [PATCH v4 4/4] virt: tdx-guest: Allocate Quote buffer dynamically

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

On 9/15/26 02:26, Peter Fang wrote:
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index ec886c401fcb..7224a66b65d1 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -162,7 +162,7 @@ static void tdx_mr_deinit(const struct attribute_group *mr_grp)
>   * DICE-based attestation uses layered evidence that requires
>   * larger Quote size (~100K).
>   */
> -#define GET_QUOTE_BUF_SIZE		SZ_128K
> +#define GET_QUOTE_DEFAULT_BUF_SIZE	SZ_128K

The naming of "GET_QUOTE_DEFAULT_BUF_SIZE" is rather unfortunate. "GET"
is a verb. It is for functions.

Isn't it "TDX_DEFAULT_QUOTE_SIZE"?

I know the TDCALL leaf is "GET_QUOTE" but replaying that naming is
horribly confusing.

> @@ -222,11 +222,34 @@ static void free_quote_buf(void *buf, size_t len)
>  	free_pages_exact(buf, len);
>  }
>  
> +/*
> + * Return a buffer size large enough for a Quote. This covers all Quote
> + * types supported by the platform.
> + */
> +static size_t get_quote_buf_size(void)
> +{
> +	u32 quote_size = tdx_get_max_quote_size();
> +	size_t len;
> +
> +	if (quote_size)
> +		/* The reported size does not include the buffer header */
> +		len = TDX_QUOTE_BUF_LEN(quote_size);
> +	else
> +		/* Older TDX modules don't report the size, use the default */
> +		len = GET_QUOTE_DEFAULT_BUF_SIZE;
> +
> +	return len;
> +}

No, I'm sorry, that's not how we do multi-line if()'s. Brackets please.

I'd much rather see something structured this way:

static size_t get_quote_buf_size(void)
{
	static u32 full_quote_len;
	/* Size of the quote itself, no header: */
	u32 quote_data_len;

	if (full_quote_len)
		return full_quote_len;

	/* Ask the module for the max data size: */
	tdx_read_max_quote_data_len(&quote_data_len);

	/* Fall back to defaults on old TDX modules: */
	if (!quote_data_len)
		quote_data_len = GET_QUOTE_DEFAULT_DATA_SIZE;

	/* Add room for the header metadata: */
	full_quote_len = quote_data_len + sizeof(struct tdx_quote_buf);
	full_quote_len = PAGE_ALIGN(full_quote_len);

	return full_quote_len;
}

Having a static function variable beats a global every single time when
possible. This way, you can (guaranteed) see 100% of the code that reads
or writes the variable. Anybody can muck with a global and you need to
grep or search to look at all the other code in the file even for a
static global.

Also note that it talks about quote data and metadata instead of just
"lengths".

You also have a mess of naming there. Some 'len', some 'size'. Pick one.

>  static void *alloc_quote_buf(size_t len)
>  {
>  	unsigned int count = len >> PAGE_SHIFT;
>  	void *addr;
>  
> +	/*
> +	 * This fails if the requested size exceeds the buddy allocator's
> +	 * maximum order (order-10, 4MB).
> +	 */
...

I don't think you need to document the buddy allocator's behavior here.

Why did you even add this?