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

Dave Hansen
2024-11-22T18:04:23Z

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.