Re: [PATCH v2 08/17] x86/virt/tdx: Prepare Quote buffer during extension bringup

[Nikolay Borisov <nik.borisov@suse.com>]

On 6/18/26 11:13, Xu Yilun wrote:
> From: Peter Fang <peter.fang@intel.com>
> 
> During TDX attestation, the TDX guest asks the host to generate a
> signed, verifiable structure (a "Quote"). With the Quoting extension,
> the TDX module returns the Quote in pages that the host shares via an
> Extension-SEAMCALL.

nit: Just say the host provides the pages to the TDX module, no need to 
specify if it's via an ext seamcall or some otherway. One can infer this 
from the code itself.

> 
> The SEAMCALL accepts the host buffer pages as a linked list of 4KB
> "HPA_LINKED_LIST" nodes. Each entry holds the physical address of a 4KB
> data page, except for the last entry, which points to the next node. The
> TDX module reports the required Quote buffer size through a global
> metadata field. See [1] for details.

That HPA_LINKED_LIST is basically a linked list of arrays, each node 
into the linked list being the array, and each entry of that array being 
the HPA witht he last being a pointer to the next HPA_LINKED_LIST, so in 
a way a 2 level data structure. Can you be more explicit and just say 
something along the lines of :

"SEAMCALL accepts host buffer pages arranged in a custom data structure, 
consisting of nodes containing HPAs, arranged linearly, and each node is 
linked to the next one via the last entry"

> 
> For simplicity, let all guests share a global buffer. Build the buffer's
> HPA_LINKED_LIST at Quoting extension bringup. This saves a bunch of
> va-to-pa conversions at runtime.
> 
> [1] Intel TDX Module ABI specification, Section "Physical Memory
>      Management Types"
> 
> Signed-off-by: Peter Fang <peter.fang@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> ---
>   arch/x86/include/asm/tdx_global_metadata.h  |   4 +
>   arch/x86/virt/vmx/tdx/tdx.c                 | 115 +++++++++++++++++++-
>   arch/x86/virt/vmx/tdx/tdx_global_metadata.c |  14 +++
>   3 files changed, 129 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/tdx_global_metadata.h b/arch/x86/include/asm/tdx_global_metadata.h
> index b3442b7c88bb..17cb13a1bb40 100644
> --- a/arch/x86/include/asm/tdx_global_metadata.h
> +++ b/arch/x86/include/asm/tdx_global_metadata.h
> @@ -57,4 +57,8 @@ struct tdx_sys_info_ext {
>   	bool ext_required;
>   };
>   
> +struct tdx_sys_info_quote {
> +	u32 max_quote_size;
> +};
> +
>   #endif
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 06c42b86b05e..9716424a301f 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>
> @@ -71,6 +72,24 @@ static LIST_HEAD(tdx_memlist);
>   
>   static struct tdx_sys_info tdx_sysinfo;
>   
> +/*
> + * Quote buffer shared with the TDX module for quote generation, in HPA linked
> + * list format.
> + *
> + * @buf: Virtual address of the quote buffer.
> + * @buf_len: Size of @buf in bytes.
> + * @hpa_entries: HPA entries, starting at the first list node.
> + * @hpa_entries_pa: Physical address for @hpa_entries.
> + */
> +struct tdx_quote_data {
> +	void		*buf;
> +	u64		buf_len;
> +	u64		*hpa_entries;
> +	phys_addr_t	hpa_entries_pa;
> +};
> +
> +static struct tdx_quote_data tdx_quote;
> +
>   static DEFINE_RAW_SPINLOCK(sysinit_lock);
>   
>   /*
> @@ -1167,6 +1186,81 @@ static __init int init_tdmrs(struct tdmr_info_list *tdmr_list)
>   	return 0;
>   }
>   
> +static inline phys_addr_t tdx_vmalloc_to_pa(const void *addr)
> +{
> +	unsigned long pfn = vmalloc_to_pfn(addr);
> +
> +	return PFN_PHYS(pfn);
> +}
> +
> +#define HPAS_PER_NODE			(PAGE_SIZE / sizeof(u64))
> +
> +/*
> + * Pass the quote buffer to the TDX module as an HPA linked list, where each
> + * node holds 4KB page HPAs and the last entry points to the next node.
> + */
> +static __init int tdx_quote_create_buf(unsigned int npages,
> +				       struct tdx_quote_data *qdata)
> +{
> +	unsigned int nnodes;
> +	u64 *hpas;
> +	void *qbuf;
> +	int i, j;
> +
> +	if (!npages)
> +		return -EINVAL;
> +
> +	/*
> +	 * Each node holds up to (HPAS_PER_NODE - 1) 4KB page HPAs.
> +	 * The last entry of the node points to the next node.
> +	 */
> +	nnodes = DIV_ROUND_UP(npages, HPAS_PER_NODE - 1);
nit: It's somewhat arbitrary but num_nodes seems more explicit, saving 3 
chars is not worth it.
> +
> +	hpas = vmalloc_array(nnodes, PAGE_SIZE);

hpas is basically the starting node, so name it "nodes". Also I'm 
slightly confused why you use vmalloc to allocate a contiguous address 
space when the nodes are linked in a list? I.e you keep referring to 
list in the changelog and the code seems to be working in an array chunks?

> +	if (!hpas)
> +		return -ENOMEM;
> +
> +	/*
> +	 * ~0ULL is the list terminator for HPA_LINKED_LIST.
> +	 *
> +	 * Pre-fill the last node with 0xff bytes so that unused entries are
> +	 * terminators. Overwrite populated entries later.
> +	 */
> +	memset((u8 *)hpas + (nnodes - 1) * PAGE_SIZE, 0xff, PAGE_SIZE);
> +
> +	qbuf = vcalloc(npages, PAGE_SIZE);
> +	if (!qbuf)
> +		goto out_nomem;

'qbuf' is rather arbitrary here. You simply pre-allocate a bunch of 
pages which you then initialize into the nodes. Can't this allocation be 
moved inside the loop itself, of course it will increase the number of 
allocs happening but this is during initialization so it's not 
performance critical, yes?



> +
> +	/* Populate the linked list */
> +	for (i = 0, j = 0; j < npages; i++) {

nit: This feedback was given before, but both variables can be defined 
in this loop.


> +		if ((i % HPAS_PER_NODE) == HPAS_PER_NODE - 1) {
> +			/*
> +			 * The last node entry always points to the next node.
> +			 * The address of the following entry must be on next
> +			 * node's page boundary.
> +			 */
> +			hpas[i] = tdx_vmalloc_to_pa(&hpas[i + 1]);
> +			continue;
> +		}
> +
> +		hpas[i] = tdx_vmalloc_to_pa((u8 *)qbuf + j * PAGE_SIZE);
> +		j++;
> +	}
 > +> +	qdata->buf = qbuf;
> +	qdata->buf_len = (u64)npages * PAGE_SIZE;
> +	qdata->hpa_entries = hpas;
> +	qdata->hpa_entries_pa = tdx_vmalloc_to_pa(hpas);
> +
> +	return 0;
> +
> +out_nomem:
> +	vfree(hpas);
> +
> +	return -ENOMEM;
> +}
> +
>   /* Initialize quoting extension */
>   static __init int tdx_quote_init(void)
>   {
> @@ -1185,12 +1279,25 @@ static __init int tdx_quote_init(void)
>   
>   static __init void init_tdx_quoting_extension(void)
>   {
> -	int ret;
> +	struct tdx_sys_info_quote sysinfo_quote;
> +	unsigned int nr_quote_pages;
> +
> +	if (!(tdx_addon_feature0 & TDX_FEATURES0_QUOTE))
> +		return;
>   
> -	if (tdx_addon_feature0 & TDX_FEATURES0_QUOTE) {
> -		ret = tdx_quote_init();
> -		WARN_ON_ONCE(ret);
> +	if (tdx_quote_init()) {
> +		WARN_ON_ONCE(1);
> +		return;
>   	}
> +
> +	/* Quoting metadata is valid only after initialization */
> +	if (get_tdx_sys_info_quote(&sysinfo_quote))
> +		return;
> +
> +	nr_quote_pages = PAGE_ALIGN(sysinfo_quote.max_quote_size) /
> +			 PAGE_SIZE;
> +	if (tdx_quote_create_buf(nr_quote_pages, &tdx_quote))
> +		pr_err("Failed to create quote buffer\n");
>   }
>   
>   /* Initialize TDX module extensions for extension SEAMCALLs */
> diff --git a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> index 84364da89649..1eb2985307c6 100644
> --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> @@ -151,3 +151,17 @@ static int get_tdx_sys_info_ext(struct tdx_sys_info_ext *sysinfo_ext)
>   
>   	return 0;
>   }
> +
> +static __init int get_tdx_sys_info_quote(struct tdx_sys_info_quote *sysinfo_quote)
> +{
> +	int ret;
> +	u64 val;
> +
> +	ret = read_sys_metadata_field(0x2300000200000002, &val);
> +	if (ret)
> +		return ret;
> +
> +	sysinfo_quote->max_quote_size = val;
> +
> +	return 0;
> +}