Re: [RFC PATCH 07/15] x86/virt/tdx: Prepare Quote buffer during extension bringup

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

On Thu, May 28, 2026 at 03:30:36PM -0700, Edgecombe, Rick P wrote:
> On Fri, 2026-05-22 at 11:41 +0800, Xu Yilun wrote:
> > From: Peter Fang <peter.fang@intel.com>
> > 
> > The host uses a Quote buffer to communicate with the TDX module when
> > generating Quotes.
> > 
> 
> Can this be put in common terms. This is going to mean nothing to someone
> reading this that doesn't already know the feature.

I'll add more background in common terms here.

> 
> >  Because the Quote buffer is shared with TDX guests,
> 
> Why capitalize "Quote"?

This is again the balance between using common terms vs TDX language. In
general, TDX docs capitalize terms a lot. TDX attestation docs always
refer to the attestation blob as "Quotes".

I mainly went with "Quotes" in the logs because that term has already
been used everywhere in the tdx-guest code/logs (see tdx-guest.c). So I
wanted to preserve some consistency at least in the logs. In the added
host code and prints, I'm starting to just use "quotes" because that
seems to be the more common convention in the TDX host code. I'm happy
to make adjustments if this doesn't make sense.

> 
> > prepare the required metadata during Quoting extension bringup.
> 
> What does prepare the required metadata mean?

That's a poor choice of word on my part. I'll rephrase it in the next
revision. I mainly just wanted to convey "prepare struct quote_data".

> 
> How does it being shared with TDX guest suggest this? Just that TDX guests will
> need them? Is the reason just that only one is needed, so do it during global
> init? 

Yes, that's exactly it. I'll make it clearer.

> 
> > +static struct quote_data {
> > +	void *buf;
> > +	u64 buf_len;
> > +	u64 *hpa_list;
> > +	phys_addr_t hpa_list_pa;
> > +} quote_data;
> 
> Hmm, I think this should separate the type and variable declaration. It's not a
> common pattern. I don't think there is an official rule.

Sure, I'll fix this.

> 
> > +	qlist = vmalloc_array(qlist_npages, PAGE_SIZE);
> > +	if (!qlist) {
> > +		err = -ENOMEM;
> > +		goto out_err;
> 
> Just return ENOMEM here. vfree() doesn't do any work if passed NULL, but it's
> weird flow.

Will do.

> 
> > +	}
> > +
> > +	/*
> > +	 * Make sure unfilled entries are always -1, which means NULL in TDX.
> 
> Huh?

I'll add more explanation here (see below).

> 
> > +	 * Only the last page needs to be filled. All the other pages will be
> > +	 * fully populated.
> > +	 */
> > +	memset((u8 *)qlist + (qlist_npages - 1) * PAGE_SIZE, 0xff, PAGE_SIZE);
> 
> What are the entries? And what is a -1 in u8? Or is it supposed to be u64?
> Please make this a lot clearer.

Yeah I was trying to create all-1 u64 entries. This is pretty
under-commented. I'll redo the comments.

> 
> > +	/* Populate HPA_LINKED_LIST as per TDX ABI spec */
> > +	for (i = 0, j = 0; j < nr_pages; i++) {
> > +		if ((i % HPAS_PER_PAGE) == HPAS_PER_PAGE - 1) {
> > +			/*
> > +			 * The last entry always points to the next page. The
> > +			 * address of the following entry must be on next page's
> > +			 * boundary.
> > +			 */
> 
> Can you maybe just explain this format that you are building in like one
> sentence at the beginning of the function? "The quote buffer is passed to the
> tdx module in a format that like... (some common terms that have no TDX
> jargon)."

Will do. This part is pretty under-commented as well.

> 
> > +	qdata->buf = qbuf;
> > +	qdata->buf_len = (u64)nr_pages * PAGE_SIZE;
> > +	qdata->hpa_list = qlist;
> > +
> > +	pfn = vmalloc_to_pfn(qlist);
> 
> Do we need a vmalloc_to_pa() helper? Maybe put it in terms of tdx format. Like
> vmalloc_pfn_to_tdxpa() and keep it here? The tdx update stuff does this a bunch
> too.

That's a really good idea. I'll do that.

> 
> > +	qdata->hpa_list_pa = PFN_PHYS(pfn);
> > +
> > +	return 0;
> > +
> > +out_err:
> > +	vfree(qlist);
> > +
> > +	return err;
> 
> It only returns -ENOMEM, so do we need the err var?

Good point. I think I had some other errors that I later removed. I'll
just return -ENOMEM directly here.

> 
> > +}
> > +
> >  static void tdx_quote_init(void)
> >  {
> >  	struct tdx_module_args args = {};
> > +	unsigned int nr_quote_pages;
> >  	u64 r;
> >  
> >  	do {
> > @@ -1218,7 +1295,13 @@ static void tdx_quote_init(void)
> >  		return;
> >  
> >  	/* Quoting metadata is valid only after initialization */
> > -	get_tdx_sys_info_quote(&tdx_sysinfo.quote);
> > +	if (get_tdx_sys_info_quote(&tdx_sysinfo.quote))
> > +		return;
> 
> How come this patch gets error handling? Why is it needed now when it wasn't
> before?

Previously, get_tdx_sys_info_quote() just happened to be the last
statement in tdx_quote_init() so getting an error didn't require an
early return. tdx_quote_init() wasn't doing much at the time. But now
the code can't see a valid max_quote_size if get_tdx_sys_info_quote()
fails.

> 
> > +
> > +	nr_quote_pages = PAGE_ALIGN(tdx_sysinfo.quote.max_quote_size) /
> > +			 PAGE_SIZE;
> > +	if (tdx_quote_create_buf(nr_quote_pages, &quote_data))
> > +		pr_err("Failed to create quote buffer\n");
> 
> Err... what happens in ENOMEM scenario? NULL pointer later?

Yes. struct quote_data remains uninitialized so it will have NULL
pointers. All the added APIs will take this into account so there won't
be NULL pointer accesses.

> 
> >  }
> >  
> >  /* Initialize the TDX Module Extensions then Extension-SEAMCALLs can be used */
>