Re: [PATCH v3 02/10] x86/virt/tdx: Convert the version metadata reader

[Nikolay Borisov]

On 30.09.26 г. 8:38 ч., Chao Gao wrote:
> With the helper to read a table of metadata fields in place, the
> existing metadata readers can be standardized on it.
>
> Convert the version metadata reader: add a table that pairs each field ID
> with the 'struct tdx_sys_info_version' member that holds its value, and
> read all version fields by walking that table.
>
> Name the field IDs for readability, so the table entries don't carry raw
> hex literals.
>
> AI was used under supervision to review code and workshop logs.
>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> v3:
>   - Drop the get_tdx_sys_info_*() wrappers to remove an unnecessary layer
>     of abstraction. [Rick, Dave]
>   - Shorten the field ID defines from TDX_MD_FIELD_ID_* to TDX_FIELD_*
>     [Rick]
> ---
>   arch/x86/virt/vmx/tdx/tdx.c                 | 15 ++++++++++++---
>   arch/x86/virt/vmx/tdx/tdx.h                 | 10 ++++++++++
>   arch/x86/virt/vmx/tdx/tdx_global_metadata.c | 17 +----------------
>   3 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 67a80ed86bd3..9d8a55a5da31 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -408,8 +408,8 @@ struct field_mapping {
>   };
>   
>   /* Read each metadata field listed in @mappings[] into @data. */
> -static int __maybe_unused __read_sys_metadata_table(const struct field_mapping *mappings,
> -						    int num_mappings, void *data)
> +static int __read_sys_metadata_table(const struct field_mapping *mappings,
> +				     int num_mappings, void *data)
>   {
>   	int i, ret;
>   	u64 val;
> @@ -434,6 +434,15 @@ static int __maybe_unused __read_sys_metadata_table(const struct field_mapping *
>   	.size		= sizeof_field(_type, _member),		\
>   }
>   
> +#define TDX_SYSINFO_MAP_VERSION(_field_id, _member) \
> +	TDX_SYSINFO_MAP(_field_id, struct tdx_sys_info_version, _member)

nit: I personally dislike adding this level of indirection, albeit 
rather shallow just so you don't have to repeat 'struct xxxx' in every 
TDX_SYS_INFO_MAP. Same goes for the rest of the patches. In the past I 
remember TDX code also suffered from, in my opinion, excessive macro 
nesting.

Dave, what's your take on this?

> +
> +static const struct field_mapping version_mappings[] = {
> +	TDX_SYSINFO_MAP_VERSION(TDX_FIELD_MINOR_VERSION,  minor_version),
> +	TDX_SYSINFO_MAP_VERSION(TDX_FIELD_MAJOR_VERSION,  major_version),
> +	TDX_SYSINFO_MAP_VERSION(TDX_FIELD_UPDATE_VERSION, update_version),
> +};
> +

<snip>