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

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

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.

>  Because the Quote buffer is shared with TDX guests,

Why capitalize "Quote"?

> prepare the required metadata during Quoting extension bringup.

What does prepare the required metadata mean?

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? 

> 
> This mostly involves determining the physical addresses of the Quote
> buffer pages and arranging them in the HPA_LINKED_LIST format defined by
> the Intel TDX Module ABI specification.
> 
> Signed-off-by: Peter Fang <peter.fang@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 85 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 84 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index fb84fb6d952b..9d04293394d7 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -32,6 +32,7 @@
>  #include <linux/idr.h>
>  #include <linux/kvm_types.h>
>  #include <linux/bitfield.h>
> +#include <linux/vmalloc.h>
>  #include <asm/page.h>
>  #include <asm/special_insns.h>
>  #include <asm/msr-index.h>
> @@ -61,6 +62,13 @@ static LIST_HEAD(tdx_memlist);
>  static struct tdx_sys_info tdx_sysinfo __ro_after_init;
>  static bool tdx_module_initialized __ro_after_init;
>  
> +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.

> +
>  typedef void (*sc_err_func_t)(u64 fn, u64 err, struct tdx_module_args *args);
>  
>  static inline void seamcall_err(u64 fn, u64 err, struct tdx_module_args *args)
> @@ -1205,9 +1213,78 @@ static inline u64 tdx_tdr_pa(struct tdx_td *td)
>  	return page_to_phys(td->tdr_page);
>  }
>  
> +#define HPAS_PER_PAGE			(PAGE_SIZE / sizeof(u64))
> +
> +static int tdx_quote_create_buf(unsigned int nr_pages, struct quote_data *qdata)
> +{
> +	unsigned long pfn;
> +	u64 qlist_npages;
> +	int err, i, j;
> +	u64 *qlist;
> +	void *qbuf;
> +
> +	if (!nr_pages)
> +		return -EINVAL;
> +
> +	/* The last entry of a linked list page points to the next page	*/
> +	qlist_npages = (u64)DIV_ROUND_UP(nr_pages, HPAS_PER_PAGE - 1);
> +
> +	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.

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

Huh?

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

> +
> +	qbuf = vcalloc(nr_pages, PAGE_SIZE);
> +	if (!qbuf) {
> +		err = -ENOMEM;
> +		goto out_err;
> +	}
> +
> +	/* 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)."

> +			pfn = vmalloc_to_pfn(&qlist[i + 1]);
> +			qlist[i] = PFN_PHYS(pfn);
> +			continue;
> +		}
> +
> +		pfn = vmalloc_to_pfn((u8 *)qbuf + j * PAGE_SIZE);
> +		qlist[i] = PFN_PHYS(pfn);
> +		j++;
> +	}
> +
> +	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.

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

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

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

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