Re: [PATCH v1 06/26] x86/virt/tdx: Add tdx_page_array helpers for new TDX Module objects

Jonathan Cameron
2025-12-19T11:32:54Z

On Mon, 17 Nov 2025 10:22:50 +0800
Xu Yilun <yilun.xu@linux.intel.com> 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.
> - 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 as control
> pages.
>
> 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.
>
> 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);
> - tdx_page_array_ctrl_release(array, nr_released, released_hpa);
>
> 3. Exchange data blobs:
> - struct tdx_page_array *array = tdx_page_array_create(nr_pages);
> - seamcall(TDX_XXX, array, ...);
> - Read data from array.
> - tdx_page_array_free(array);
>
> 4. Note the root page contains 512 HPAs at most, if more pages are
> required, refilling 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_fill_root(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. tdx_page_array_ctrl_release() is
> responsible for checking if the output pages match the original input
> pages. If failed to match, the safer way is to leak the control pages,
> tdx_page_array_ctrl_leak() should be called.
>
> The usage of tdx_page_array will be in following patches.
>
> Co-developed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

One trivial comment below. I'm not going to look into tdx specifics
enough to do a detailed review of this patch.

> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 09c766e60962..9a5c32dc1767 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c

> +static bool tdx_page_array_validate_release(struct tdx_page_array *array,
> + unsigned int offset,
> + unsigned int nr_released,
> + u64 released_hpa)
> +{
> + unsigned int nents;
> + u64 *entries;
> + int i;
> +
> + if (offset >= array->nr_pages)
> + return false;
> +
> + nents = umin(array->nr_pages - offset, TDX_PAGE_ARRAY_MAX_NENTS);
> +
> + if (nents != nr_released) {
> + pr_err("%s nr_released [%d] doesn't match page array nents [%d]\n",
> + __func__, nr_released, nents);
> + return false;
> + }
> +
> + /*
> + * Unfortunately TDX has multiple page allocation protocols, check the
> + * "singleton" case required for HPA_ARRAY_T.
> + */
> + if (page_to_phys(array->pages[0]) == released_hpa &&
> + array->nr_pages == 1)
> + return true;
> +
> + /* Then check the "non-singleton" case */
> + if (page_to_phys(array->root) == released_hpa) {
> + entries = (u64 *)page_address(array->root);

page_address() returns a void * so the cast here isn't needed and (to me
at least) doesn't add value from readability point of view.

I haven't checked later patches, but if this code doesn't change to use
entries outside this scope then,
u64 *entries = page_address(array->root);
would be nice to restrict the scope and make the type here immediately
visible.

> + for (i = 0; i < nents; i++) {
> + struct page *page = array->pages[offset + i];
> + u64 val = page_to_phys(page);
> +
> + if (val != entries[i]) {
> + pr_err("%s entry[%d] [0x%llx] doesn't match page hpa [0x%llx]\n",
> + __func__, i, entries[i], val);
> + return false;
> + }
> + }
> +
> + return true;
> + }
> +
> + pr_err("%s failed to validate, released_hpa [0x%llx], root page hpa [0x%llx], page0 hpa [%#llx], number pages %u\n",
> + __func__, released_hpa, page_to_phys(array->root),
> + page_to_phys(array->pages[0]), array->nr_pages);
> +
> + return false;
> +}