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

Dan Williams
2026-04-17T23:36:54Z

Xu Yilun wrote:
[..]
> 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>
[..]
> +static struct tdx_page_array *
> +tdx_page_array_alloc(unsigned int nr_pages)
> +{
> + struct tdx_page_array *array = NULL;
> + struct page **pages = NULL;
> + u64 *root = NULL;
> + int ret;
> +
> + if (!nr_pages)
> + return NULL;
> +
> + array = kzalloc_obj(*array);
> + if (!array)
> + goto out_free;
> +
> + root = kzalloc(PAGE_SIZE, GFP_KERNEL);
> + if (!root)
> + goto out_free;
> +
> + pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);

This should now be:

kzalloc_objs(struct page *, nr_pages);

...oh nevermind you caught that in your incremental fixup. ...but a
couple more comments below:

[..]
> +
> +#define HPA_LIST_INFO_FIRST_ENTRY GENMASK_U64(11, 3)
> +#define HPA_LIST_INFO_PFN GENMASK_U64(51, 12)
> +#define HPA_LIST_INFO_LAST_ENTRY GENMASK_U64(63, 55)
> +
> +static u64 __maybe_unused hpa_list_info_assign_raw(struct tdx_page_array *array)

2 quick comments:

* I do not understand shipping a __maybe_unused helper in patch 3 that
does not get used until patch10.

* The "assign_raw" verb feels strange. I think this probably just want
to be called: to_hpa_list_info(struct tdx_page_array *)

> +{
> + return FIELD_PREP(HPA_LIST_INFO_FIRST_ENTRY, 0) |
> + FIELD_PREP(HPA_LIST_INFO_PFN,
> + PFN_DOWN(virt_to_phys(array->root))) |
> + FIELD_PREP(HPA_LIST_INFO_LAST_ENTRY, array->nents - 1);
> +}
> +
> +#define HPA_ARRAY_T_PFN GENMASK_U64(51, 12)
> +#define HPA_ARRAY_T_SIZE GENMASK_U64(63, 55)
> +
> +static u64 __maybe_unused hpa_array_t_assign_raw(struct tdx_page_array *array)

to_hpa_array_t()

> +{
> + unsigned long pfn;
> +
> + if (array->nents == 1)
> + pfn = page_to_pfn(array->pages[array->offset]);
> + else
> + pfn = PFN_DOWN(virt_to_phys(array->root));
> +
> + return FIELD_PREP(HPA_ARRAY_T_PFN, pfn) |
> + FIELD_PREP(HPA_ARRAY_T_SIZE, array->nents - 1);
> +}
> +
> +static u64 __maybe_unused hpa_array_t_release_raw(struct tdx_page_array *array)

It seems too subtle that this function sometimes returns zero and
sometimes returns a page that the TDX module will clobber with data that
we do not care about.

It is also not clear that "0" is what the module considers a valid value
that meets "checks its validity for forward compatibility". I guess we
get lucky because all of the calls that need this presently are
multi-page cases?

I would feel better if this always returned the root HPA and was called
something like:

to_output_clobber(), or to_aux_clobber()

...to make it clear that whatever was there before gets destroyed.