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

[Nikolay Borisov]

On 2.10.26 г. 15:09 ч., Chao Gao wrote:
> On Thu, Oct 01, 2026 at 09:27:57AM -0700, Dave Hansen wrote:
> > On 10/1/26 09:24, Nikolay Borisov wrote:
> > > nit: I personally dislike adding this level of indirection, albeit
> > > rather shallow just so you don't have to repeat 'struct xxxx' in every
> > > TDX_SYS_INFO_MAP. Same goes for the rest of the patches. In the past I
> > > remember TDX code also suffered from, in my opinion, excessive macro
> > > nesting.
> > >
> > > Dave, what's your take on this?
> > >
> > > > +
> > > > +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'm kinda ambivalent on it. The width of the structure doesn't really
> > matter much. It's readable either way and it's already got *PLENTY* of
> > duplicate gunk in it.
> >
> > I'd try to remove the macro, and make sure the structure definition
> > doesn't get too icky.
>
> OK, I will remove the per-structure macros. I would also rename
> TDX_SYSINFO_MAP() to FIELD_MAP(), which keeps the table definitions
> within 100 columns apart from MAX_RESERVED_PER_TDMR and
> PAMT_PAGE_BITMAP_ENTRY_BITS.
>
> static const struct field_mapping version_mappings[] = {
> 	FIELD_MAP(TDX_FIELD_MINOR_VERSION,	struct tdx_sys_info_version, minor_version),
> 	FIELD_MAP(TDX_FIELD_MAJOR_VERSION,	struct tdx_sys_info_version, major_version),
> 	FIELD_MAP(TDX_FIELD_UPDATE_VERSION,	struct tdx_sys_info_version, update_version),
> };
>
> static const struct field_mapping feature_mappings[] __initconst = {
> 	FIELD_MAP(TDX_FIELD_TDX_FEATURES0, struct tdx_sys_info_features, tdx_features0),
> };
>
> static const struct field_mapping tdmr_mappings[] __initconst = {
> 	FIELD_MAP(TDX_FIELD_MAX_TDMRS,		struct tdx_sys_info_tdmr, max_tdmrs),
> 	FIELD_MAP(TDX_FIELD_PAMT_4K_ENTRY_SIZE,	struct tdx_sys_info_tdmr, pamt_4k_entry_size),
> 	FIELD_MAP(TDX_FIELD_PAMT_2M_ENTRY_SIZE,	struct tdx_sys_info_tdmr, pamt_2m_entry_size),
> 	FIELD_MAP(TDX_FIELD_PAMT_1G_ENTRY_SIZE,	struct tdx_sys_info_tdmr, pamt_1g_entry_size),
> 	FIELD_MAP(TDX_FIELD_MAX_RESERVED_PER_TDMR, struct tdx_sys_info_tdmr,
> 		  max_reserved_per_tdmr),
> };
>
> static const struct field_mapping dpamt_mappings[] __initconst = {
> 	FIELD_MAP(TDX_FIELD_PAMT_PAGE_BITMAP_ENTRY_BITS, struct tdx_sys_info_tdmr,
> 		  pamt_page_bitmap_entry_bits),
> };
>
> static const struct field_mapping td_ctrl_mappings[] __initconst = {
> 	FIELD_MAP(TDX_FIELD_TDR_BASE_SIZE,	struct tdx_sys_info_td_ctrl, tdr_base_size),
> 	FIELD_MAP(TDX_FIELD_TDCS_BASE_SIZE,	struct tdx_sys_info_td_ctrl, tdcs_base_size),
> 	FIELD_MAP(TDX_FIELD_TDVPS_BASE_SIZE,	struct tdx_sys_info_td_ctrl, tdvps_base_size),
> };
>
> static const struct field_mapping handoff_mappings[] = {
> 	FIELD_MAP(TDX_FIELD_MODULE_HV, struct tdx_sys_info_handoff, module_hv),
> };
>
> static const struct field_mapping td_conf_mappings[] __initconst = {
> 	FIELD_MAP(TDX_FIELD_ATTRIBUTES_FIXED0,	struct tdx_sys_info_td_conf, attributes_fixed0),
> 	FIELD_MAP(TDX_FIELD_ATTRIBUTES_FIXED1,	struct tdx_sys_info_td_conf, attributes_fixed1),
> 	FIELD_MAP(TDX_FIELD_XFAM_FIXED0,	struct tdx_sys_info_td_conf, xfam_fixed0),
> 	FIELD_MAP(TDX_FIELD_XFAM_FIXED1,	struct tdx_sys_info_td_conf, xfam_fixed1),
> 	FIELD_MAP(TDX_FIELD_NUM_CPUID_CONFIG,	struct tdx_sys_info_td_conf, num_cpuid_config),
> 	FIELD_MAP(TDX_FIELD_MAX_VCPUS_PER_TD,	struct tdx_sys_info_td_conf, max_vcpus_per_td),
> };


Looks good, and FIELD_MAP is generic enough.