Re: [PATCH v3 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Nikolay Borisov <nik.borisov@suse.com>]

On 27.08.24 г. 10:14 ч., Kai Huang wrote:
> The TDX module provides a set of "global metadata fields".  They report
> things like TDX module version, supported features, and fields related
> to create/run TDX guests and so on.
> 
> For now the kernel only reads "TD Memory Region" (TDMR) related fields
> for module initialization.  There are both immediate needs to read more
> fields for module initialization and near-future needs for other kernel
> components like KVM to run TDX guests.
> will be organized in different structures depending on their meanings.
> 
> For now the kernel only reads TDMR related fields.  The TD_SYSINFO_MAP()
> macro hard-codes the 'struct tdx_sys_info_tdmr' instance name.  To make
> it work with different instances of different structures, extend it to
> take the structure instance name as an argument.
> 
> This also means the current code which uses TD_SYSINFO_MAP() must type
> 'struct tdx_sys_info_tdmr' instance name explicitly for each use.  To
> make the code easier to read, add a wrapper TD_SYSINFO_MAP_TDMR_INFO()
> which hides the instance name.
> 
> TDX also support 8/16/32/64 bits metadata field element sizes.  For now
> all TDMR related fields are 16-bit long thus the kernel only has one
> helper:
> 
>    static int read_sys_metadata_field16(u64 field_id, u16 *val) {}
> 
> Future changes will need to read more metadata fields with different
> element sizes.  To make the code short, extend the helper to take a
> 'void *' buffer and the buffer size so it can work with all element
> sizes.
> 
> Note in this way the helper loses the type safety and the build-time
> check inside the helper cannot work anymore because the compiler cannot
> determine the exact value of the buffer size.
> 
> To resolve those, add a wrapper of the helper which only works with
> u8/u16/u32/u64 directly and do build-time check, where the compiler
> can easily know both the element size (from field ID) and the buffer
> size(using sizeof()), before calling the helper.
> 
> An alternative option is to provide one helper for each element size:
> 
>    static int read_sys_metadata_field8(u64 field_id, u8 *val) {}
>    static int read_sys_metadata_field16(u64 field_id, u16 *val) {}
>    ...
> 
> But this will result in duplicated code given those helpers will look
> exactly the same except for the type of buffer pointer.  It's feasible
> to define a macro for the body of the helper and define one entry for
> each element size to reduce the code, but it is a little bit
> over-engineering.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
> 
> v2 -> v3:
>   - Rename read_sys_metadata_field() to tdh_sys_rd() so the former can be
>     used as the high level wrapper.  Get rid of "stbuf_" prefix since
>     people don't like it.
>   
>   - Rewrite after removing 'struct field_mapping' and reimplementing
>     TD_SYSINFO_MAP().
>   
> ---
>   arch/x86/virt/vmx/tdx/tdx.c | 45 +++++++++++++++++++++----------------
>   arch/x86/virt/vmx/tdx/tdx.h |  3 ++-
>   2 files changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 7e75c1b10838..1cd9035c783f 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -250,7 +250,7 @@ static int build_tdx_memlist(struct list_head *tmb_list)
>   	return ret;
>   }
>   
> -static int read_sys_metadata_field(u64 field_id, u64 *data)
> +static int tdh_sys_rd(u64 field_id, u64 *data)
>   {
>   	struct tdx_module_args args = {};
>   	int ret;
> @@ -270,43 +270,50 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
>   	return 0;
>   }
>   
> -static int read_sys_metadata_field16(u64 field_id, u16 *val)
> +static int __read_sys_metadata_field(u64 field_id, void *val, int size)

The type of 'size' should be size_t.

>   {
>   	u64 tmp;
>   	int ret;
>   
> -	BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
> -			MD_FIELD_ID_ELE_SIZE_16BIT);
> -
> -	ret = read_sys_metadata_field(field_id, &tmp);
> +	ret = tdh_sys_rd(field_id, &tmp);
>   	if (ret)
>   		return ret;
>   
> -	*val = tmp;
> +	memcpy(val, &tmp, size);
>   
>   	return 0;
>   }
>   
> +/* Wrapper to read one global metadata to u8/u16/u32/u64 */
> +#define read_sys_metadata_field(_field_id, _val)					\
> +	({										\
> +		BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != sizeof(typeof(*(_val))));	\
> +		__read_sys_metadata_field(_field_id, _val, sizeof(typeof(*(_val))));	\
> +	})
> +
>   /*
> - * Assumes locally defined @ret and @sysinfo_tdmr to convey the error
> - * code and the 'struct tdx_sys_info_tdmr' instance to fill out.
> + * Read one global metadata field to a member of a structure instance,
> + * assuming locally defined @ret to convey the error code.
>    */
> -#define TD_SYSINFO_MAP(_field_id, _member)						\
> -	({										\
> -		if (!ret)								\
> -			ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id,	\
> -					&sysinfo_tdmr->_member);			\
> +#define TD_SYSINFO_MAP(_field_id, _stbuf, _member)				\
> +	({									\
> +		if (!ret)							\
> +			ret = read_sys_metadata_field(MD_FIELD_ID_##_field_id,	\
> +					&_stbuf->_member);			\
>   	})
>   
>   static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
>   {
>   	int ret = 0;
>   
> -	TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs);
> -	TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> -	TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
> -	TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
> -	TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
> +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)	\
> +	TD_SYSINFO_MAP(_field_id, sysinfo_tdmr, _member)

nit: I guess its a personal preference but honestly I think the amount 
of macro indirection (3 levels) here is crazy, despite each being rather 
simple. Just use TD_SYSINFO_MAP directly, saving the typing of 
"sysinfo_tdmr" doesn't seem like a big deal.

You can probably take it even a bit further and simply opencode 
read_sys_metadata_field macro inside TD_SYSINFO_MAP and be left with 
just it, no ? No other patch in this series uses read_sys_metadata_field 
stand alone, if anything factoring it out could be deferred until the 
first users gets introduced.

> +
> +	TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS,	        max_tdmrs);
> +	TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> +	TD_SYSINFO_MAP_TDMR_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
> +	TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
> +	TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
>   
>   	return ret;
>   }
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 148f9b4d1140..7458f6717873 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -53,7 +53,8 @@
>   #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id)	\
>   		(((_field_id) & GENMASK_ULL(33, 32)) >> 32)
>   
> -#define MD_FIELD_ID_ELE_SIZE_16BIT	1
> +#define MD_FIELD_ELE_SIZE(_field_id)	\

That ELE seems a bit ambiguous, ELEM seems more natural and is in line 
with other macros in the kernel.

> +	(1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id))
>   
>   struct tdmr_reserved_area {
>   	u64 offset;