Re: [RFC PATCH 1/6] x86/virt/tdx: Add SEAMCALL wrappers for TDX KeyID management

[Dave Hansen]

On 11/15/24 12:20, Rick Edgecombe wrote:
> +struct tdx_td {
> +	hpa_t tdr;
> +	hpa_t *tdcs;
> +};

This is a step in the right direction because it gives the wrappers some
more type safety.

But an hpa_t is _barely_ better than a u64.  If the 'tdr' is a page,
then it needs to be _stored_ as a page:

	struct page *tdr_page;

Also, please don't forget to spell these things out:

	/* TD root structure: */
	struct page *tdr_page;

And the tdcs is an array of pages, right?  So it should be:

	struct page **tdcs_pages;

Or heck, I _think_ it can theoretically be defined as a variable-length
array:

	struct page *tdcs_pages[];

and use the helpers that we have for that.

Putting it all together, you would have this:

struct tdx_td {
	/* TD root structure: */
	struct page *tdr_page;

	int tdcs_nr_pages;
	/* TD control structure: */
	struct page *tdcs_pages[];
};

That's *MUCH* harder to misuse.  It's 100% obvious that you have a
single page, plus a variable-length array of pages.  This is all from
just looking at the structure definition.

You know that 'tdr' is not just some random physical address.  It's a
whole physical page.  It's page-aligned.  It was allocated, from the
allocator.  It doesn't point to special memory.

Ditto for "hpa_t *tdcs".  It's not obvious from the data structure that
it's an array or if it's an array how it got allocated or how large it is.