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

[Xu Yilun <yilun.xu@linux.intel.com>]

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

You once had a comment wanting to see how a tdx_page_array collapses to
a 64-bit raw value for SEAMCALLs in the same patch. So I move the
helpers earlier. Do you want to change them back?

Personally, I'd like to keep them here, to better align with the
illustration in commit log about why we need the tdx_page_array.

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

It's a better name, thanks.

[...]

> > +{
> > +	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

It is the TDX Module's requirement, which is 'too subtle'. TDX Module
tries to keep align with the singleton definition for its output
hpa_array_t. If TDX Module wants to output multiple released pages, it
requires VMM to provide a root page HPA (in input register) so it can
write HPA list on the root page. But if it outputs one released page, it
directly writes page0 HPA in output register, and doesn't need a root
page HPA in input register and enforce its value 0.

That's why we return 0 on singleton mode, otherwise a root page HPA.

> get lucky because all of the calls that need this presently are
> multi-page cases?

Let me experiment, see if we have chance to simplify things.

> 
> I would feel better if this always returned the root HPA and was called

No, we can't. We must provide 0 for singleton mode. So I think maybe

      to_hpa_array_t_released()


Anyway, Linux doesn't need the output hpa_array_t. I've already raised
to Module team that don't enforce the medium page input. If VMM doesn't
provide the page, don't bother fill it.

> something like:
> 
>     to_output_clobber(), or to_aux_clobber()
> 
> ...to make it clear that whatever was there before gets destroyed.
>