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

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

On Fri, Oct 09, 2026 at 12:47:31AM +0800, Xu Yilun wrote:
>On Tue, Sep 29, 2026 at 10:38:36PM -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.
>>
>
>[...]
>
>> +#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(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),
>> +};
>
>[...]
>
>I think something like the following is quite clear to me:
>
>  static const struct field_mapping version_mappings[] = {
>	FIELD_MAP(0x0800000100000003ULL, struct tdx_sys_info_version, minor_version),
>	...
>  }
>
>  or even:
>
>	FIELD_MAP(0x0800000100000003ULL, version, minor_version),

Building the struct name from "version" needs another macro:

#define FIELD_MAP(_field, _type, _member) \
	__FIELD_MAP(_field, struct tdx_sys_info_##_type, _member)

This shortens each table entry by 20 characters. The struct name is no
longer greppable in these tables, but I don't think that matters here.

Dave and Nikolay earlier suggested dropping the per-class macros to avoid
excessive macro nesting, and this adds one level back. But for this single
generic macro, I think the shorter entries are worth it.

Dave, Nikolay, would you be OK with it, or should we just let the entries
run up to 100 columns?

>
>It clearly tells the mapping for the class, the field, and the field_id.
>
>So I'm not sure what's the downside of a literal hex here. To me, a
>TDX_FIELD_XX MACRO only creates duplicated names in one line and
>unnecessary touch points when we add a new field.

Naming ABI constants is the usual kernel convention. It is true that the
surrounding code makes it clear what 0x0800000100000003ULL is. But I think
it is even better for the constant to describe itself.

Not all fields go through FIELD_MAP() tables. The CPUID config arrays are
read in a loop, as TDX_FIELD_CPUID_CONFIG_LEAVES + i. A literal there has
no FIELD_MAP() nearby to explain it.

The TD-scoped metadata code already names its fields (TDCS_*), so this
keeps the two consistent.

I agree that naming the ABI constants adds one touch point per field. But
I think that is a small cost.

>
>
>> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
>> index db209541d3cd..10cbc2d77a5a 100644
>> --- a/arch/x86/virt/vmx/tdx/tdx.h
>> +++ b/arch/x86/virt/vmx/tdx/tdx.h
>> @@ -52,6 +52,16 @@
>>  #define TDH_PHYMEM_PAMT_REMOVE		59
>>  #define TDH_SYS_DISABLE			69
>>  
>> +/*
>> + * TDX global metadata field IDs.
>> + *
>> + * See "global_metadata.pdf" in Intel TDX Module ABI Definitions.
>> + */
>> +/* Class "TDX Module Version" */
>> +#define TDX_FIELD_MINOR_VERSION			0x0800000100000003ULL
>> +#define TDX_FIELD_MAJOR_VERSION			0x0800000100000004ULL
>> +#define TDX_FIELD_UPDATE_VERSION		0x0800000100000005ULL
>
>I think these are unnecessary touch points, metadata readers never use
>these MACROs.