Re: [PATCH v6 4/6] virt: tdx-guest: Add a helper for the Quote buffer size

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Wed, 2026-09-30 at 03:30 -0700, Peter Fang wrote:
> The Quote buffer size is a constant sprinkled across several places.
> Read it from a helper instead. And rename the constant to avoid using a
> verb.
> 
> This sets up the plumbing for a later change that makes the size
> dynamic.
> 
> There are several "sizes" at play here: the raw Quote data, the Quote
> buffer (the header metadata plus the raw data), and the allocation (the
> page-padded Quote buffer). The helper returns the Quote buffer size and
> leaves page alignment to callers that need it.
> 
> AI was used under supervision to collect/apply feedback, review code and
> workshop logs.
> 
> Signed-off-by: Peter Fang <peter.fang@intel.com>
> ---
> v6:
>  - Don't cache the size in the helper. [Rick]
>  - Let the callers page-align the buffer size. [Rick]
>  - Add a comment for the helper. [Dave]
> v5:
>  - Reworked v4 3/4 to use a helper instead of a global. [Dave, Xiaoyao]
>  - Use TDX_DEFAULT_QUOTE_SIZE instead of GET_QUOTE_DEFAULT_BUF_SIZE.
>    [Dave]
>  - Use "size" instead of "len" for buffer size variables. [Dave]
>  - Drop the RB tags, as the code changed substantially.
> ---
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 31 +++++++++++++++++--------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 77c63c3f820f..0cf078f09a73 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 TDX_DEFAULT_QUOTE_SIZE		SZ_128K
>  
>  #define GET_QUOTE_CMD_VER		1
>  
> @@ -206,26 +206,36 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>  			     USER_SOCKPTR(req->tdreport));
>  }
>  
> +/* Size of the header metadata plus the largest possible raw Quote. */
> +static size_t get_quote_buf_size(void)
> +{
> +	return TDX_DEFAULT_QUOTE_SIZE;
> +}
> +
>  static void free_quote_buf(struct tdx_quote_buf *buf)
>  {
> -	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> -	unsigned int count = len >> PAGE_SHIFT;
> +	size_t alloc_size = PAGE_ALIGN(get_quote_buf_size());
> +	unsigned int count;
> +
> +	count = alloc_size >> PAGE_SHIFT;

Why change the count to be set outside of the declarations here and below?

>  
>  	if (set_memory_encrypted((unsigned long)buf, count)) {
>  		pr_err("Failed to restore encryption mask for Quote buffer, leak it\n");
>  		return;
>  	}
>  
> -	free_pages_exact(buf, len);
> +	free_pages_exact(buf, alloc_size);
>  }
>  
>  static struct tdx_quote_buf *alloc_quote_buf(void)
>  {
> -	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> -	unsigned int count = len >> PAGE_SHIFT;
> +	size_t alloc_size = PAGE_ALIGN(get_quote_buf_size());
>  	struct tdx_quote_buf *buf;
> +	unsigned int count;
>  
> -	buf = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
> +	count = alloc_size >> PAGE_SHIFT;
> +
> +	buf = alloc_pages_exact(alloc_size, GFP_KERNEL | __GFP_ZERO);
>  	if (!buf)
>  		return NULL;
>  
> @@ -266,6 +276,7 @@ static int tdx_report_new_locked(struct tsm_report *report)
>  {
>  	u8 *buf;
>  	struct tsm_report_desc *desc = &report->desc;
> +	size_t quote_buf_size = get_quote_buf_size();

This local var is a performance optimization? Or a line shortener?

>  	u32 out_len;
>  	int ret;
>  	u64 err;
> @@ -281,7 +292,7 @@ static int tdx_report_new_locked(struct tsm_report *report)
>  	if (desc->inblob_len != TDX_REPORTDATA_LEN)
>  		return -EINVAL;
>  
> -	memset(quote_buf, 0, GET_QUOTE_BUF_SIZE);
> +	memset(quote_buf, 0, quote_buf_size);
>  
>  	/* Update Quote buffer header */
>  	quote_buf->version = GET_QUOTE_CMD_VER;
> @@ -292,7 +303,7 @@ static int tdx_report_new_locked(struct tsm_report *report)
>  	if (ret)
>  		return ret;
>  
> -	err = tdx_hcall_get_quote(quote_buf, GET_QUOTE_BUF_SIZE);
> +	err = tdx_hcall_get_quote(quote_buf, PAGE_ALIGN(quote_buf_size));

The point of leaving page alignment to the callers was to not churn the existing
code in this patch. But this caller is getting changed anyway for some reason. 

In patch 6, it changes to the dynamic buffer, which might not be page aligned.
But what if len passed through the GHCI call is not page aligned? Does it cause
a problem? Oh! GHCI docs say "R13 - Size of shared GPA. The size must be 4KB-
aligned."

So it needs new alignment only because of the dynamic buffer, and to fulfill the
GHCI spec. Or otherwise I guess you could claim that the page alignment is added
here because it was always required and now it's too hard to see that any
possible size is already aligned. I think it's weak. I'd put it in patch 6 and
explain why it is now needed at that point.

>  	if (err) {
>  		pr_err("GetQuote hypercall failed, status:%llx\n", err);
>  		return -EIO;
> @@ -311,7 +322,7 @@ static int tdx_report_new_locked(struct tsm_report *report)
>  
>  	out_len = READ_ONCE(quote_buf->out_len);
>  
> -	if (struct_size(quote_buf, data, out_len) > GET_QUOTE_BUF_SIZE)
> +	if (struct_size(quote_buf, data, out_len) > quote_buf_size)
>  		return -EFBIG;
>  
>  	buf = kvmemdup(quote_buf->data, out_len, GFP_KERNEL);