Re: [PATCH v2 02/10] x86/virt/tdx: Unbind global metadata read with 'struct tdx_tdmr_sysinfo'

[Adrian Hunter <adrian.hunter@intel.com>]

On 7/08/24 15:09, Huang, Kai wrote:
> On Mon, 2024-08-05 at 18:13 -0700, Dan Williams wrote:
>> Huang, Kai wrote:
>> [..]
>>>> The unrolled loop is the same amount of work as maintaining @fields.
>>>
>>> Hi Dan,
>>>
>>> Thanks for the feedback.
>>>
>>> AFAICT Dave didn't like this way:
>>>
>>> https://lore.kernel.org/lkml/cover.1699527082.git.kai.huang@intel.com/T/#me6f615d7845215c278753b57a0bce1162960209d
>>
>> I agree with Dave that the original was unreadable. However, I also
>> think he glossed over the loss of type-safety and the silliness of
>> defining an array to precisely map fields only to turn around and do a
>> runtime check that the statically defined array was filled out
>> correctly. So I think lets solve the readability problem *and* make the
>> array definition identical in appearance to unrolled type-safe
>> execution, something like (UNTESTED!):
>>
>>
> [...]
> 
>> +/*
>> + * Assumes locally defined @ret and @ts to convey the error code and the
>> + * 'struct tdx_tdmr_sysinfo' instance to fill out
>> + */
>> +#define TD_SYSINFO_MAP(_field_id, _offset)                              \
>> +	({                                                              \
>> +		if (ret == 0)                                           \
>> +			ret = read_sys_metadata_field16(                \
>> +				MD_FIELD_ID_##_field_id, &ts->_offset); \
>> +	})
>> +
> 
> We need to support u16/u32/u64 metadata field sizes, but not just u16.
> 
> E.g.:
> 
> struct tdx_sysinfo_module_info {                                        
>         u32 sys_attributes;                                             
>         u64 tdx_features0;                                              
> };
> 
> has both u32 and u64 in one structure.
> 
> To achieve type-safety for all field sizes, I think we need one helper
> for each field size.  E.g.,
> 
> #define READ_SYSMD_FIELD_FUNC(_size)                            \
> static inline int                                               \
> read_sys_metadata_field##_size(u64 field_id, u##_size *data)    \
> {                                                               \
>         u64 tmp;                                                \
>         int ret;                                                \
>                                                                 \
>         ret = read_sys_metadata_field(field_id, &tmp);          \
>         if (ret)                                                \
>                 return ret;                                     \
>                                                                 \
>         *data = tmp;                                            \
>         return 0;                                               \
> }                                                                       
> 
> /* For now only u16/u32/u64 are needed */
> READ_SYSMD_FIELD_FUNC(16)                                               
> READ_SYSMD_FIELD_FUNC(32)                                               
> READ_SYSMD_FIELD_FUNC(64)                                               
> 
> Is this what you were thinking?
> 
> (Btw, I recall that I tried this before for internal review, but AFAICT
> Dave didn't like this.)
> 
> For the build time check as you replied to the next patch, I agree it's
> better than the runtime warning check as done in the current code.
> 
> If we still use the type-less 'void *stbuf' function to read metadata
> fields for all sizes, then I think we can do below:
> 
> /*
>  * Read one global metadata field and store the data to a location of a 
>  * given buffer specified by the offset and size (in bytes).            
>  */
> static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,
>                                   int size)                             
> {       
>         void *member = stbuf + offset;                                  
>         u64 tmp;                                                        
>         int ret;                                                        
> 
>         ret = read_sys_metadata_field(field_id, &tmp);                  
>         if (ret)
>                 return ret;                                             
>         
>         memcpy(member, &tmp, size);                                     
>         
>         return 0;                                                       
> }                                                                       
> 
> /* Wrapper to read one metadata field to u8/u16/u32/u64 */              
> #define stbuf_read_sysmd_single(_field_id, _pdata)      \
>         stbuf_read_sysmd_field(_field_id, _pdata, 0, 	\
> 		sizeof(typeof(*(_pdata)))) 
> 
> #define CHECK_MD_FIELD_SIZE(_field_id, _st, _member)    \
>         BUILD_BUG_ON(MD_FIELD_ELE_SIZE(MD_FIELD_ID_##_field_id) != \
>                         sizeof(_st->_member))
> 
> #define TD_SYSINFO_MAP_TEST(_field_id, _st, _member)                    \
>         ({                                                              \
>                 if (ret) {                                              \
>                         CHECK_MD_FIELD_SIZE(_field_id, _st, _member);   \
>                         ret = stbuf_read_sysmd_single(                  \
>                                         MD_FIELD_ID_##_field_id,        \
>                                         &_st->_member);                 \
>                 }                                                       \
>          })
> 
> static int get_tdx_module_info(struct tdx_sysinfo_module_info *modinfo)
> {
>         int ret = 0;
> 
> #define TD_SYSINFO_MAP_MOD_INFO(_field_id, _member)     \
>         TD_SYSINFO_MAP_TEST(_field_id, modinfo, _member)
> 
>         TD_SYSINFO_MAP_MOD_INFO(SYS_ATTRIBUTES, sys_attributes);
>         TD_SYSINFO_MAP_MOD_INFO(TDX_FEATURES0,  tdx_features0);
> 
>         return ret;
> }
> 
> With the build time check above, I think it's OK to lose the type-safe
> inside the stbuf_read_sysmd_field(), and the code is simpler IMHO.
> 
> Any comments?

BUILD_BUG_ON() requires a function, but it is still
be possible to add a build time check in TD_SYSINFO_MAP
e.g.

#define TD_SYSINFO_CHECK_SIZE(_field_id, _size)			\
	__builtin_choose_expr(MD_FIELD_ELE_SIZE(_field_id) == _size, _size, (void)0)

#define _TD_SYSINFO_MAP(_field_id, _offset, _size)		\
	{ .field_id = _field_id,				\
	  .offset   = _offset,					\
	  .size	    = TD_SYSINFO_CHECK_SIZE(_field_id, _size) }

#define TD_SYSINFO_MAP(_field_id, _struct, _member)		\
	_TD_SYSINFO_MAP(MD_FIELD_ID_##_field_id,		\
			offsetof(_struct, _member),		\
			sizeof(typeof(((_struct *)0)->_member)))