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

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

On Mon, 2024-08-26 at 18:38 +0300, Adrian Hunter wrote:
> 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)))
> 
> 

Thanks for the comment, but I don't think this meets for our purpose.

We want a build time "error" when the "MD_FIELD_ELE_SIZE(_field_id) == _size"
fails, but not "still initializing the size to 0".  Otherwise, we might get
some unexpected issue (due to size is 0) at runtime, which is worse IMHO than
a runtime check as done in the current upstream code.

I have been trying to add a BUILD_BUG_ON() to the field_mapping structure
initializer, but I haven't found a reliable way to do so.

For now I have completed the new version based on Dan's suggestion, but still
need to work on changelog/coverletter etc, so I think I can send the new
version out and see whether people like it.  We can revert back if that's not
what people want.