Re: [RFC PATCH v2 01/10] x86/virt/tdx: Add a helper to read a table of metadata fields

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Fri, 2026-09-18 at 06:29 -0700, Chao Gao wrote:
> The metadata field readers get_tdx_sys_info_<class>() in
> tdx_global_metadata.c were generated by an out-of-tree script. That has
> not worked out: the JSON file they were generated from is neither stable
> nor authoritative enough [1].  The goal now is to maintain the readers by
> hand and to establish one standard way of adding a metadata field.
> 
> Take get_tdx_sys_info_version() as an example:
> 
>       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;
> 
> Two patterns stand out: the read-check-store sequence repeats once per
> field, and the error of each read is chained into the reads that follow.
> Neither is common in hand-written code.
> 
> Eliminate both with a loop that reads each field, stores the value into
> its structure member, and returns on the first error.

"Prepare to eliminate both..."?

> 
> Add 'struct field_mapping' to describe one field as its ID plus the
> offset and size of the member that receives its value.  Add
> TDX_SYSINFO_MAP() to build such an entry from a field ID name, a
> structure type and a member name.  Add __read_sys_metadata_table() to
> read every field in a table. Annotate the helper __maybe_unused as there
> is no caller right now.
> 
> Following changes will convert the existing readers to use the new helper.
> 
> AI was used under supervision to review code and workshop logs.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Link: https://lore.kernel.org/kvm/1e7bcbad-eb26-44b7-97ca-88ab53467212@intel.com/ # [1]
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 063574ed8625..5cd9c6eb98c2 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -395,6 +395,42 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
>  	return 0;
>  }
>  
> +/*
> + * Map a TDX global metadata field to a structure member.
> + * @field_id: The TDX global metadata field ID.
> + * @size: The size of the structure member.
> + * @offset: The member's offset within its containing structure.
> + */
> +struct field_mapping {
> +	u64 field_id;
> +	size_t size;
> +	int offset;
> +};
> +
> +/* 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)
> +{
> +	int i, ret;
> +	u64 val;
> +
> +	for (i = 0; i < num_mappings; i++) {
> +		ret = read_sys_metadata_field(mappings[i].field_id, &val);
> +		if (ret)
> +			return ret;
> +		memcpy((char *)data + mappings[i].offset, &val, mappings[i].size);
> +	}
> +
> +	return 0;
> +}
> +
> +#define TDX_SYSINFO_MAP(_field, _type, _member)			\
> +{								\
> +	.field_id	= TDX_MD_FIELD_ID_##_field,		\

I find it a bit hard to search the code when it constructs the
"TDX_MD_FIELD_ID_" part of the define inside the macro. And since they are all
named "TDX_MD_FIELD_ID_FOO" it seems a bit verbose and repetitive. Can we have a
smaller pattern such that we can use the exact macro name in TDX_SYSINFO_MAP()?
Maybe like this:

#define TDX_FIELD_MODULE_HV 0x8900000100000000ULL

static const struct field_mapping handoff_mappings[] = {
	TDX_SYSINFO_MAP_HANDOFF(TDX_FIELD_MODULE_HV, module_hv),
};

> +	.offset		= offsetof(_type, _member),		\
> +	.size		= sizeof_field(_type, _member),		\
> +}
> +
>  #include "tdx_global_metadata.c"
>  
>  static __init int check_features(struct tdx_sys_info *sysinfo)