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

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

On Thu, Sep 24, 2026 at 06:20:09AM +0800, Edgecombe, Rick P wrote:
>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..."?

Yes, that makes sense since this patch doesn't convert any readers.

>> +#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),
>};

Looks good. Being greppable is a good property. Will do in v3.