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

[Edgecombe, Rick P]

Hi,

In general I'm struggling to understand the design decisions. It seems a very
specific design and quite a bit of code to manage an array of pages. Questions
below.

On Sat, 2026-03-28 at 00:01 +0800, Xu Yilun wrote:
> Add struct tdx_page_array definition for new TDX Module object
> types - HPA_ARRAY_T and HPA_LIST_INFO. 

This is unfortunate. I see you agree in the comments.

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

This is kind of verbose for what it seems to be trying to say. It's just that
these types can be input or output params. The TDX module could hold on to the
pages for a long time, or just transiently. For that latter part I think you are
trying to say sometimes they need flushing and sometimes they don't?

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

"singleton mode"? What is it? If it's the case of not needing populate loop, it
probably deserves more explanation. I'm not sure, but the populate loop seems to
drive a lot of the struct design?

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

Not clear what this is talking about. But I'm starting to wonder if we should be
so bold to claim that the differences between the types really simplify the
host. 

> 
> 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);
>  - tdx_page_array_ctrl_release(array, nr_released, released_hpa);

So release is mostly needed because of the need to do the wbvind seamcall? And
unlike tdx_page_array_free() it returns an error in case that fails. Or other
sanity checking. But all the callers do the same thing on error, call
tdx_page_array_ctrl_leak().

Just wondering if we could simplify it somehow. There are two helpers and the
caller has to know which one to call based on SEAMCALL specifics. What if the
seamcall wrapper set a bit in the page array while passing it out. The bit would
specify to the helper if it needs to do wbinvd or not. Then the wrappers could
encapsulate the type of free needed and not rely on the caller to know. And we
only need to have one function for it instead of two.


BTW, do we expect errors from the tdh_phymem_page_wbinvd_hkid() calls here? How
could the BUSY happen? If we don't think it can happen in normal runtime, we
could just warn and skip the special leak logic. In KVM side there is a place
where we can't really handle it for the wbinvd calls. And one where we can. If
we need a ton of code to handle a bug somewhere (on kernel side or TDX module),
it seems too defensive to me. At least it's not in sync with the rest of TDX.

Especially the quite large tdx_page_array_validate_release() logic should need a
justification that there is something very tricky that needs all this checking.

But maybe you can explain what the special risk is.

> 
> 3. Exchange data blobs:
>  - struct tdx_page_array *array = tdx_page_array_create(nr_pages);
>  - seamcall(TDX_XXX, array, ...);
>  - Read data from array.
> 
> 
> 4. Note the root page contains 512 HPAs at most, if more pages are
>    required, re-populate the tdx_page_array is needed.
> 
>  - struct tdx_page_array *array = tdx_page_array_alloc(nr_pages);
>  - for each 512-page bulk
>    - tdx_page_array_populate(array, offset);
>    - seamcall(TDH_XXX_ADD, array, ...);
> 
> In case 2, SEAMCALLs output the released page array in the form of
> HPA_ARRAY_T or PAGE_LIST_INFO. Use tdx_page_array_ctrl_release() to
> check if the output pages match the original input pages. If failed,
> TDX Module is buggy. In this case the safer way is to leak the
> control pages, call tdx_page_array_ctrl_leak().
> 
> The usage of tdx_page_array will be in following patches.
> 
> Co-developed-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
> Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/include/asm/tdx.h  |  37 +++++
>  arch/x86/virt/vmx/tdx/tdx.c | 299 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 336 insertions(+)
> 
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 65c4da396450..9173a432b312 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -139,6 +139,43 @@ void tdx_guest_keyid_free(unsigned int keyid);
>  
>  void tdx_quirk_reset_page(struct page *page);
>  
> +/**
> + * struct tdx_page_array - Represents a list of pages for TDX Module access
> + * @nr_pages: Total number of data pages in the collection
> + * @pages: Array of data page pointers containing all the data
> + *
> + * @offset: Internal: The starting index in @pages, positions the currently
> + *	    populated page window in @root.
> + * @nents: Internal: Number of valid HPAs for the page window in @root
> + * @root: Internal: A single 4KB page holding the 8-byte HPAs of the page
> + *	  window. The page window max size is constrained by the root page,
> + *	  which is 512 HPAs.
> + *
> + * This structure abstracts several TDX Module defined object types, e.g.,
> + * HPA_ARRAY_T and HPA_LIST_INFO. Typically they all use a "root page" as the
> + * medium to exchange a list of data pages between host and TDX Module. This
> + * structure serves as a unified parameter type for SEAMCALL wrappers, where
> + * these hardware object types are needed.
> + */
> +struct tdx_page_array {
> +	/* public: */
> +	unsigned int nr_pages;
> +	struct page **pages;
> +
> +	/* private: */
> +	unsigned int offset;
> +	unsigned int nents;
> +	u64 *root;

pages is going to be an array of struct pointers, and root is a single page of
PA's that gets re-used to copy and pass the PA's to the TDX module. Why do we
need both? Like just keep an array of PA's that would be the same size as the
struct page array. And not need the populate loop? 

Pausing for now. Still looking through the callers and it's the end of the day.