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

[Xu Yilun]

On Tue, Oct 13, 2026 at 04:22:37PM +0800, Binbin Wu wrote:
> On 10/12/2026 5:47 PM, 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.
> > 
> > AI was used under supervision to review code and workshop logs.
> > 
> > Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> 
> Reviewed-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
> 
> One nit below.
> 
> [...]
> 
> > @@ -437,6 +437,12 @@ static int __maybe_unused __read_sys_metadata_table(const struct field_mapping *
> >  #define FIELD_MAP(_field, _type, _member)	\
> >  	__FIELD_MAP(_field, struct tdx_sys_info_##_type, _member)
> >  
> > +static const struct field_mapping version_mappings[] = {
> > +	FIELD_MAP(TDX_FIELD_MINOR_VERSION,	version, minor_version),
> > +	FIELD_MAP(TDX_FIELD_MAJOR_VERSION,	version, major_version),
> > +	FIELD_MAP(TDX_FIELD_UPDATE_VERSION,	version, update_version),
> > +};
> 
> Nit:
> I personally prefer the version provided in
> https://lore.kernel.org/kvm/ar-e8WzRS7IUKWgH@xxxxxxxxx
> The full types keep the code greppable and __FIELD_MAP() could be dropped.
> 
> Also, it is a bit weird that FIELD_MAP() use _type as the name for the second
> input, but the shortened one is not really a type.

I think if we see the mapping array as a mirror of the table in
global_metadata.pdf, then this short pattern is easier to review. In
this case is it clearer we rename FIELD_MAP()'s args to use the same
terms in the SPEC? like the following:

	FIELD_MAP(_id, _class, _field)


BTW: I still think "without field id macro" makes the pattern even
clearer:

	FIELD_MAP(0x0800000100000003ULL, version, minor_version)

Direct and easier to map to global_metadata.pdf. Only leave 2 special
cases - CPUID_CONFIG_LEAVES/VALUES - but they are special enough anyway.

I'm still OK with the current patch. With or without macro is trivial.

Reviewed-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> 
>