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

[Dan Williams <djbw@kernel.org>]

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.