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

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

On Wed, Jul 08, 2026 at 10:52:07AM +0300, Nikolay Borisov wrote:
> 
> 
> 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.

Good point. Thanks for the simplification.

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

Oh this is much easier to read. Thanks for the suggestion! I can
incorporate this.

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

Sure, I can make this change.

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

Hmm. I used the vmalloc family because I wanted to keep using hpas[i]
without switching to the next node's va. It felt a bit simpler to just
keep doing "i++". Maybe that actually hurts readability? I can replace
that part of the for loop with something like:

        if (i == HPAS_PER_NODE - 1) {
                next_node = kmalloc(PAGE_SIZE, GFP_KERNEL);
                hpas[i] = virt_to_phys(next_node);
                hpas = next_node;
                i = 0;
        }

And "hpas[i]" feels a bit closer to what the code does than "nodes[i]"?
It's filling this page with a bunch of HPAs.

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

I see your point. It’s a bit subtle here. In patch 11, there is a
kvmemdup() that depends on this qbuf being virtually contiguous. Based
on feedback from other folks, I think I might change this and use
runtime quote buffers allocated with kmalloc(). That would also avoid
the TLB flushing penalty of vfree(). The free path would be a bit more
complicated, but that should be okay.

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

Great suggestion. I can do that. Thanks.

> 
>