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

[Chao Gao <chao.gao@intel.com>]

On Thu, Sep 24, 2026 at 12:12:11PM -0700, Dave Hansen wrote:
>On 9/23/26 22:50, Chao Gao wrote:
>>>> +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?
>> I'd prefer to keep them, for a few reasons:
>> 
>> 1. This one is actually called twice: from get_tdx_sys_info() and from
>>    tdx_module_run_update().
>
>Neither has any real space constraints, I'll note.
>
>> 2. The helper name describes what is being read better than a bare
>>    read_sys_metadata_table() call.
>
>What part? that it comes from "sys_info" as opposed to it being
>"metadata". Meh. Why does it matter?
>
>> 3. Some helpers do more than read a table (see get_tdx_sys_info_td_conf()).
>>    Without per-class helpers, get_tdx_sys_info() would open-code
>>    read_sys_metadata_table() for some classes and call a helper for
>>    others, which looks inconsistent. With one helper per metadata class,
>>    every class is read the same way.
>
>I value lower levels of abstraction over naming consistency.
>
>Remove the helper, please.

Sure, will do.

Then I would go one step further and move the mapping table into
get_tdx_sys_info() too, so it sits next to its only use. Using the
"features" metadata class as an example:


static __init int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
{
	static const struct field_mapping feature_mappings[] __initconst = {
		TDX_SYSINFO_MAP_FEATURES(TDX_FIELD_TDX_FEATURES0, tdx_features0),
	};
	int ret = 0;

	ret = ret ?: read_sys_metadata_table(version_mappings, &sysinfo->version);
	...
	ret = ret ?: read_sys_metadata_table(feature_mappings, &sysinfo->features);


version_mappings[] will be the exception and stay at file scope because
tdx_module_run_update() reads it as well.

Let me know if you disagree.