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

[Peter Fang <peter.fang@intel.com>]

On Mon, Sep 21, 2026 at 03:47:02PM -0700, Dave Hansen wrote:
> 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.

TDX_DEFAULT_QUOTE_SIZE is much clearer. I'll rename it.

> 
> 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.

This is much better. I'll rewrite it.
quote_data_len doesn't change at runtime, so this pattern works well.

> 
> 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.

Yeah it's pretty messy. I'll clean this up.

> 
> >  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?

I'll remove it. No need to redocument the API. Thanks.