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@xxxxxxxxx/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?
