Re: [PATCH v3 2/8] x86/virt/tdx: Remove 'struct field_mapping' and implement TD_SYSINFO_MAP() macro

["Huang, Kai" <kai.huang@intel.com>]

On Thu, 2024-08-29 at 10:20 +0300, Adrian Hunter wrote:
> On 27/08/24 10:14, Kai Huang wrote:
> 
> Subject could be "Simplify the reading of Global Metadata Fields"

OK.

> 
> > TL;DR: Remove the 'struct field_mapping' structure and use another way
> > to implement the TD_SYSINFO_MAP() macro to improve the current metadata
> > reading code, e.g., switching to use build-time check for metadata field
> > size over the existing runtime check.
> 
> Perhaps:
> 
>   Remove 'struct field_mapping' and let read_sys_metadata_field16() accept
>   the member variable address, simplifying the code in preparation for adding
>   support for more metadata structures and field sizes.

Fine to me.  I would like also to mention there are improvements in the new
code (as suggested by Dan), so I will say:

"..., simplifying and improving the code in preparation ...".

> 
> > 
> > The TDX module provides a set of "global metadata fields".  They report
> 
> Global Metadata Fields

OK. 

> 
> > things like TDX module version, supported features, and fields related
> > to create/run TDX guests and so on.
> > 
> > For now the kernel only reads "TD Memory Region" (TDMR) related global
> > metadata fields, and all of those fields are organized in one structure.
> 
> The patch is self-evidently simpler (21 insertions(+), 36 deletions(-))
> so there doesn't seem to be any need for further elaboration.  Perhaps
> just round it off and stop there.
> 
>   ... and all of those fields are organized in one structure,
>   but that will change in the near future.

I think the code change here is not only to reduce the LoC (i.e., simplify the
code), but also improve the code, so I would like to keep the things mentioned
by Dan in the changelog.

> 
> > 
> > The kernel currently uses 'struct field_mapping' to facilitate reading
> > multiple metadata fields into one structure.  The 'struct field_mapping'
> > records the mapping between the field ID and the offset of the structure
> > to fill out.  The kernel initializes an array of 'struct field_mapping'
> > for each structure member (using the TD_SYSINFO_MAP() macro) and then
> > reads all metadata fields in a loop using that array.
> > 
> > Currently the kernel only reads TDMR related metadata fields into one
> > structure, and the function to read one metadata field takes the pointer
> > of that structure and the offset:
> > 
> >   static int read_sys_metadata_field16(u64 field_id,
> >                                        int offset,
> >                                        struct tdx_sys_info_tdmr *ts)
> >   {...}
> > 
> > Future changes will need to read more metadata fields into different
> > structures.  To support this the above function will need to be changed
> > to take 'void *':
> > 
> >   static int read_sys_metadata_field16(u64 field_id,
> >                                        int offset,
> >                                        void *stbuf)
> >   {...}
> > 
> > This approach loses type-safety, as Dan suggested.  A better way is to
> > make it be ..
> > 
> >   static int read_sys_metadata_field16(u64 field_id, u16 *val) {...}
> > 
> > .. and let the caller calculate the buffer in a type-safe way [1].
> > 
> > Also, the using of the 'struct field_mapping' loses the ability to be
> > able to do build-time check about the metadata field size (encoded in
> > the field ID) due to the field ID is "indirectly" initialized to the
> > 'struct field_mapping' and passed to the function to read.  Thus the
> > current code uses runtime check instead.
> > 
> > Dan suggested to remove the 'struct field_mapping', unroll the loop,
> > skip the array, and call the read_sys_metadata_field16() directly with
> > build-time check [1][2].  And to improve the readability, reimplement
> > the TD_SYSINFO_MAP() [3].
> > 
> > The new TD_SYSINFO_MAP() isn't perfect.  It requires the function that
> > uses it to define a local variable @ret to carry the error code and set
> > the initial value to 0.  It also hard-codes the variable name of the
> > structure pointer used in the function.  But overall the pros of this
> > approach beat the cons.
> > 
> > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m7cfb3c146214d94b24e978eeb8708d92c0b14ac6 [1]
> > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#mbe65f0903ff7835bc418a907f0d02d7a9e0b78be [2]
> > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@intel.com/T/#m80cde5e6504b3af74d933ea0cbfc3ca9d24697d3 [3]
> 
> Probably just one link would suffice, say the permalink to Dan's
> comment:
> 
> https://lore.kernel.org/kvm/66b16121c48f4_4fc729424@dwillia2-xfh.jf.intel.com.notmuch/

[1] and [2] are actually replies to different patches, so I would like to keep
them.  [1] and [3] are replies to the same patch, so I could remove [3], but I
think keeping all of them is also fine since it's more easy to find the exact
comment that I want to address.

> 
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> > 
> > v2 -> v3:
> >  - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP().
> > 
> > ---
> >  arch/x86/virt/vmx/tdx/tdx.c | 57 ++++++++++++++-----------------------
> >  1 file changed, 21 insertions(+), 36 deletions(-)
> > 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index e979bf442929..7e75c1b10838 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -270,60 +270,45 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
> >  	return 0;
> >  }
> >  
> > -static int read_sys_metadata_field16(u64 field_id,
> > -				     int offset,
> > -				     struct tdx_sys_info_tdmr *ts)
> > +static int read_sys_metadata_field16(u64 field_id, u16 *val)
> >  {
> > -	u16 *ts_member = ((void *)ts) + offset;
> >  	u64 tmp;
> >  	int ret;
> >  
> > -	if (WARN_ON_ONCE(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
> > -			MD_FIELD_ID_ELE_SIZE_16BIT))
> > -		return -EINVAL;
> > +	BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
> > +			MD_FIELD_ID_ELE_SIZE_16BIT);
> 
> This gets removed next patch, so why do it

This patch I didn't add the build-time check in the (new) TD_SYSINFO_MAP(), so
I changed the runtime check to build-time check here since we can actually do
it here.  I think even for this middle state patch we should do the build-time
check.  I can move it to the (new) TD_SYSINFO_MAP() though if that's better.

> 
> >  
> >  	ret = read_sys_metadata_field(field_id, &tmp);
> >  	if (ret)
> >  		return ret;
> >  
> > -	*ts_member = tmp;
> > +	*val = tmp;
> >  
> >  	return 0;
> >  }
> >  
> > -struct field_mapping {
> > -	u64 field_id;
> > -	int offset;
> > -};
> > -
> > -#define TD_SYSINFO_MAP(_field_id, _offset) \
> > -	{ .field_id = MD_FIELD_ID_##_field_id,	   \
> > -	  .offset   = offsetof(struct tdx_sys_info_tdmr, _offset) }
> > -
> > -/* Map TD_SYSINFO fields into 'struct tdx_sys_info_tdmr': */
> > -static const struct field_mapping fields[] = {
> > -	TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs),
> > -	TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
> > -	TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]),
> > -	TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]),
> > -	TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]),
> > -};
> > +/*
> > + * Assumes locally defined @ret and @sysinfo_tdmr to convey the error
> > + * code and the 'struct tdx_sys_info_tdmr' instance to fill out.
> > + */
> > +#define TD_SYSINFO_MAP(_field_id, _member)						\
> 
> "MAP" made sense when it was in a struct whereas
> now it is reading.
> 
> > +	({										\
> > +		if (!ret)								\
> > +			ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id,	\
> > +					&sysinfo_tdmr->_member);			\
> > +	})
> >  
> >  static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
> >  {
> > -	int ret;
> > -	int i;
> > +	int ret = 0;
> >  
> > -	/* Populate 'sysinfo_tdmr' fields using the mapping structure above: */
> > -	for (i = 0; i < ARRAY_SIZE(fields); i++) {
> > -		ret = read_sys_metadata_field16(fields[i].field_id,
> > -						fields[i].offset,
> > -						sysinfo_tdmr);
> > -		if (ret)
> > -			return ret;
> > -	}
> > +	TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs);
> > +	TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> > +	TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
> > +	TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
> > +	TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
> 
> Another possibility is to put the macro at the invocation site:
> 
> #define READ_SYS_INFO(_field_id, _member)				\
> 	ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id,	\
> 					       &sysinfo_tdmr->_member)
> 
> 	READ_SYS_INFO(MAX_TDMRS,             max_tdmrs);
> 	READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> 	READ_SYS_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
> 	READ_SYS_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
> 	READ_SYS_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
> 
> #undef READ_SYS_INFO
> 
> And so on in later patches:
> 
> #define READ_SYS_INFO(_field_id, _member)				\
> 	ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id,	\
> 					     &sysinfo_version->_member)
> 
> 	READ_SYS_INFO(MAJOR_VERSION,    major);
> 	READ_SYS_INFO(MINOR_VERSION,    minor);
> 	READ_SYS_INFO(UPDATE_VERSION,   update);
> 	READ_SYS_INFO(INTERNAL_VERSION, internal);
> 	READ_SYS_INFO(BUILD_NUM,        build_num);
> 	READ_SYS_INFO(BUILD_DATE,       build_date);
> 
> #undef READ_SYS_INFO
> 

This is fine to me.  But AFAICT Kirill doesn't like the "#undef" part.

Kirill, do you have comments?

Otherwise I will go with what Adrian suggested.