Re: [PATCH v3 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

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

> > -static int read_sys_metadata_field16(u64 field_id, u16 *val)
> > +static int __read_sys_metadata_field(u64 field_id, void *val, int size)
> 
> The type of 'size' should be size_t.

OK will do.

> 
> >   {
> >   	u64 tmp;
> >   	int ret;
> >   
> > -	BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
> > -			MD_FIELD_ID_ELE_SIZE_16BIT);
> > -
> > -	ret = read_sys_metadata_field(field_id, &tmp);
> > +	ret = tdh_sys_rd(field_id, &tmp);
> >   	if (ret)
> >   		return ret;
> >   
> > -	*val = tmp;
> > +	memcpy(val, &tmp, size);
> >   
> >   	return 0;
> >   }
> >   
> > +/* Wrapper to read one global metadata to u8/u16/u32/u64 */
> > +#define read_sys_metadata_field(_field_id, _val)					\
> > +	({										\
> > +		BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != sizeof(typeof(*(_val))));	\
> > +		__read_sys_metadata_field(_field_id, _val, sizeof(typeof(*(_val))));	\
> > +	})
> > +
> >   /*
> > - * Assumes locally defined @ret and @sysinfo_tdmr to convey the error
> > - * code and the 'struct tdx_sys_info_tdmr' instance to fill out.
> > + * Read one global metadata field to a member of a structure instance,
> > + * assuming locally defined @ret to convey the error code.
> >    */
> > -#define TD_SYSINFO_MAP(_field_id, _member)						\
> > -	({										\
> > -		if (!ret)								\
> > -			ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id,	\
> > -					&sysinfo_tdmr->_member);			\
> > +#define TD_SYSINFO_MAP(_field_id, _stbuf, _member)				\
> > +	({									\
> > +		if (!ret)							\
> > +			ret = read_sys_metadata_field(MD_FIELD_ID_##_field_id,	\
> > +					&_stbuf->_member);			\
> >   	})
> >   
> >   static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
> >   {
> >   	int ret = 0;
> >   
> > -	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]);
> > +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)	\
> > +	TD_SYSINFO_MAP(_field_id, sysinfo_tdmr, _member)
> 
> nit: I guess its a personal preference but honestly I think the amount 
> of macro indirection (3 levels) here is crazy, despite each being rather 
> simple. Just use TD_SYSINFO_MAP directly, saving the typing of 
> "sysinfo_tdmr" doesn't seem like a big deal.

We have a different interpretation of "crazy" :-)

> 
> You can probably take it even a bit further and simply opencode 
> read_sys_metadata_field macro inside TD_SYSINFO_MAP and be left with 
> just it, no ? No other patch in this series uses read_sys_metadata_field 
> stand alone, if anything factoring it out could be deferred until the 
> first users gets introduced.

Joke aside, anyway, with what Adrian suggested we just need the
TD_SYSINFO_MAP() but don't need those wrappers anymore.  I'll go with this if
no one says differently.

> 
> > +
> > +	TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS,	        max_tdmrs);
> > +	TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> > +	TD_SYSINFO_MAP_TDMR_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
> > +	TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
> > +	TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
> >   
> >   	return ret;
> >   }
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> > index 148f9b4d1140..7458f6717873 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.h
> > +++ b/arch/x86/virt/vmx/tdx/tdx.h
> > @@ -53,7 +53,8 @@
> >   #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id)	\
> >   		(((_field_id) & GENMASK_ULL(33, 32)) >> 32)
> >   
> > -#define MD_FIELD_ID_ELE_SIZE_16BIT	1
> > +#define MD_FIELD_ELE_SIZE(_field_id)	\
> 
> That ELE seems a bit ambiguous, ELEM seems more natural and is in line 
> with other macros in the kernel.

OK will do.  Thanks for the review!