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

[Edgecombe, Rick P]

On Fri, 2026-09-18 at 06:29 -0700, 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.
> 
> Also add read_sys_metadata_table() macro to derive the number of entries
> with ARRAY_SIZE() so that callers don't need to pass it separately.
> 
> AI was used under supervision to review code and workshop logs. It suggests
> adding read_sys_metadata_table() macro.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
>  arch/x86/virt/vmx/tdx/tdx.c                 | 21 +++++++++++++++++++--
>  arch/x86/virt/vmx/tdx/tdx.h                 | 10 ++++++++++
>  arch/x86/virt/vmx/tdx/tdx_global_metadata.c | 15 ---------------
>  3 files changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 5cd9c6eb98c2..0c8b571f5186 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;
> @@ -431,6 +431,23 @@ 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)
> +
> +static const struct field_mapping version_mappings[] = {
> +	TDX_SYSINFO_MAP_VERSION(MINOR_VERSION,  minor_version),
> +	TDX_SYSINFO_MAP_VERSION(MAJOR_VERSION,  major_version),
> +	TDX_SYSINFO_MAP_VERSION(UPDATE_VERSION, update_version),
> +};
> +
> +#define read_sys_metadata_table(_table, _data) \
> +	__read_sys_metadata_table(_table, ARRAY_SIZE(_table), _data)

Should this go in the previous patch?

> +
> +static int get_tdx_sys_info_version(struct tdx_sys_info_version *version)
> +{
> +	return read_sys_metadata_table(version_mappings, version);
> +}

I'm not sure we need all of these helpers that are each only called once?

> +
>  #include "tdx_global_metadata.c"
>  
>  static __init int check_features(struct tdx_sys_info *sysinfo)
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index a886c54decaa..44124f399703 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -60,6 +60,16 @@
>   */
>  #define TDX_VERSION_SHIFT		16
>  
> +/*
> + * TDX global metadata field IDs.
> + *
> + * See "global_metadata.pdf" in Intel TDX Module ABI Definitions.
> + */
> +/* Class "TDX Module Version" */
> +#define TDX_MD_FIELD_ID_MINOR_VERSION			0x0800000100000003ULL
> +#define TDX_MD_FIELD_ID_MAJOR_VERSION			0x0800000100000004ULL
> +#define TDX_MD_FIELD_ID_UPDATE_VERSION			0x0800000100000005ULL
> +
>  /* TDX page types */
>  #define	PT_NDA		0x0
>  #define	PT_RSVD		0x1
> diff --git a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> index 98ebf17aab1c..9510bf5f6e7d 100644
> --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> @@ -7,21 +7,6 @@
>   * Include this file to other C file instead.
>   */
>  
> -static int get_tdx_sys_info_version(struct tdx_sys_info_version *sysinfo_version)
> -{
> -	int ret = 0;
> -	u64 val;
> -
> -	if (!ret && !(ret = read_sys_metadata_field(0x0800000100000003, &val)))
> -		sysinfo_version->minor_version = val;
> -	if (!ret && !(ret = read_sys_metadata_field(0x0800000100000004, &val)))
> -		sysinfo_version->major_version = val;
> -	if (!ret && !(ret = read_sys_metadata_field(0x0800000100000005, &val)))
> -		sysinfo_version->update_version = val;
> -
> -	return ret;
> -}
> -
>  static __init int get_tdx_sys_info_features(struct tdx_sys_info_features *sysinfo_features)
>  {
>  	int ret = 0;