Re: [PATCH v2 03/31] x86/virt/tdx: Add tdx_page_array helpers for new TDX Module objects

[Dan Williams]

Xu Yilun wrote:
> Add struct tdx_page_array definition for new TDX Module object
> types - HPA_ARRAY_T and HPA_LIST_INFO. They are used as input/output
> parameters in newly defined SEAMCALLs. Also define some helpers to
> allocate, setup and free tdx_page_array.
> 
> HPA_ARRAY_T and HPA_LIST_INFO are similar in most aspects. They both
> represent a list of pages for TDX Module accessing. There are several
> use cases for these 2 structures:
> 
>  - As SEAMCALL inputs. They are claimed by TDX Module as control pages.
>    Control pages are private pages for TDX Module to hold its internal
>    control structures or private data. TDR, TDCS, TDVPR... are existing
>    control pages, just not added via tdx_page_array.
>  - As SEAMCALL outputs. They were TDX Module control pages and now are
>    released.
>  - As SEAMCALL inputs. They are just temporary buffers for exchanging
>    data blobs in one SEAMCALL. TDX Module will not hold them for long
>    time.
> 
> The 2 structures both need a 'root page' which contains a list of HPAs.
> They collapse the HPA of the root page and the number of valid HPAs
> into a 64 bit raw value for SEAMCALL parameters. The root page is
> always a medium for passing data pages, TDX Module never keeps the
> root page.
> 
> A main difference is HPA_ARRAY_T requires singleton mode when
> containing just 1 functional page (page0). In this mode the root page is
> not needed and the HPA field of the raw value directly points to the
> page0. But in this patch, root page is always allocated for user
> friendly kAPIs.

I think this undersells the fact that "singleton mode" is a premature
optimization that requires complication to take advantage of the benefit
(sometimes save a single page allocation). The Linux implementation
forfeits that small benefit for the larger gain of cleaner kAPIs.

> Another small difference is HPA_LIST_INFO contains a 'first entry' field
> which could be filled by TDX Module. This simplifies host by providing
> the same structure when re-invoke the interrupted SEAMCALL. No need for
> host to touch this field.
> 
> Typical usages of the tdx_page_array:
> 
> 1. Add control pages:
>  - struct tdx_page_array *array = tdx_page_array_create(nr_pages);
>  - seamcall(TDH_XXX_CREATE, array, ...);
> 
> 2. Release control pages:
>  - seamcall(TDX_XXX_DELETE, array, &nr_released, &released_hpa);

It is simply a bug if TDH_XXX_DELETE does not return every resource
passed to TDH_XXX_CREATE. The only "leak" case to worry about is that
TDH_XXX_DELETE fails. In that case it should be "fatal" (TDX_BUG_ON,
system can keep hobbling along, but panic_on_warn() would not be
unreasonable). If TDH_XXX_DELETE fails it indicates some catastrophic
misunderstanding between Linux and the TDX Module.

So the seamcall in this case has no need for @nr_released or
@released_hpa, those should already be known to the kernel.

What is missing is an architectural guarantee that TDH_XXX_DELETE
success == "all resources you arranged at TDH_XXX_CREATE time are free".
I would hope that is already the case and AUX_PAGE_PA is only an
unfortunate distraction. If it can ever be the case that CREATE and
DELETE are asymmetric on success then that needs to be corrected and
Linux will wait for a future module that can make that guarantee.

I think that cleans up a bulk of the logic here to abandon caring that
the module tries to remind us what we are releasing.