| Message ID | 7af2b06ec26e2964d8d5da21e2e9fa412e4ed6f8.1721186590.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX host: metadata reading tweaks, bug fix and info dump | expand |
Kai Huang wrote: > The TDX module provides a set of "global metadata fields". They report > 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 to a 'struct tdx_tdmr_sysinfo' for initializing the TDX > module, and the metadata reading code can only work with that structure. > > Future changes will need to read other metadata fields that don't make > sense to populate to the "struct tdx_tdmr_sysinfo". It's essential to > provide a generic metadata read infrastructure which is not bound to any > specific structure. > > To start providing such infrastructure, unbind the metadata reading code > with the 'struct tdx_tdmr_sysinfo'. > > Note the kernel has a helper macro, TD_SYSINFO_MAP(), for marshaling the > metadata into the 'struct tdx_tdmr_sysinfo', and currently the macro > hardcodes the structure name. As part of unbinding the metadata reading > code with 'struct tdx_tdmr_sysinfo', it is extended to accept different > structures. > > Unfortunately, this will result in the usage of TD_SYSINFO_MAP() for > populating 'struct tdx_tdmr_sysinfo' to be changed to use the structure > name explicitly for each structure member and make the code longer. Add > a wrapper macro which hides the 'struct tdx_tdmr_sysinfo' internally to > make the code shorter thus better readability. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com> > --- > > v1 -> v2: > - 'st_member' -> 'member'. (Nikolay) > > --- > arch/x86/virt/vmx/tdx/tdx.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index d8fa9325bf5e..2ce03c3ea017 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -272,9 +272,9 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) > > static int read_sys_metadata_field16(u64 field_id, > int offset, > - struct tdx_tdmr_sysinfo *ts) > + void *stbuf) The loss of all type-safety sticks out, and points to the fact that @offset was awkward to pass in from the beginning. I would have expected a calling convention like: static int read_sys_metadata_field16(u64 field_id, u16 *val) ...and make the caller calculate the buffer in a type-safe way. The problem with the current code is that it feels like it is planning ahead for a dynamic metdata reading future, that is not coming, Instead it could be leaning further into initializing all metadata once. In other words what is the point of defining: static const struct field_mapping fields[] ...only to throw away all type-safety and run it in a loop. Why not unroll the loop, skip the array, and the runtime warning with something like? read_sys_metadata_field16(MD_FIELD_ID_MAX_TDMRS, &ts->max_tdmrs); read_sys_metadata_field16(MD_FIELD_ID_MAX_RESERVED_PER_TDMR, &ts->max_reserved_per_tdmr); ...etc The unrolled loop is the same amount of work as maintaining @fields.
On 6/08/2024 11:32 am, Williams, Dan J wrote: > Kai Huang wrote: >> The TDX module provides a set of "global metadata fields". They report >> 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 to a 'struct tdx_tdmr_sysinfo' for initializing the TDX >> module, and the metadata reading code can only work with that structure. >> >> Future changes will need to read other metadata fields that don't make >> sense to populate to the "struct tdx_tdmr_sysinfo". It's essential to >> provide a generic metadata read infrastructure which is not bound to any >> specific structure. >> >> To start providing such infrastructure, unbind the metadata reading code >> with the 'struct tdx_tdmr_sysinfo'. >> >> Note the kernel has a helper macro, TD_SYSINFO_MAP(), for marshaling the >> metadata into the 'struct tdx_tdmr_sysinfo', and currently the macro >> hardcodes the structure name. As part of unbinding the metadata reading >> code with 'struct tdx_tdmr_sysinfo', it is extended to accept different >> structures. >> >> Unfortunately, this will result in the usage of TD_SYSINFO_MAP() for >> populating 'struct tdx_tdmr_sysinfo' to be changed to use the structure >> name explicitly for each structure member and make the code longer. Add >> a wrapper macro which hides the 'struct tdx_tdmr_sysinfo' internally to >> make the code shorter thus better readability. >> >> Signed-off-by: Kai Huang <kai.huang@intel.com> >> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com> >> --- >> >> v1 -> v2: >> - 'st_member' -> 'member'. (Nikolay) >> >> --- >> arch/x86/virt/vmx/tdx/tdx.c | 25 ++++++++++++++----------- >> 1 file changed, 14 insertions(+), 11 deletions(-) >> >> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c >> index d8fa9325bf5e..2ce03c3ea017 100644 >> --- a/arch/x86/virt/vmx/tdx/tdx.c >> +++ b/arch/x86/virt/vmx/tdx/tdx.c >> @@ -272,9 +272,9 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) >> >> static int read_sys_metadata_field16(u64 field_id, >> int offset, >> - struct tdx_tdmr_sysinfo *ts) >> + void *stbuf) > > The loss of all type-safety sticks out, and points to the fact that > @offset was awkward to pass in from the beginning. I would have expected > a calling convention like: > > static int read_sys_metadata_field16(u64 field_id, u16 *val) > > ...and make the caller calculate the buffer in a type-safe way. > > The problem with the current code is that it feels like it is planning > ahead for a dynamic metdata reading future, that is not coming, Instead > it could be leaning further into initializing all metadata once. > > In other words what is the point of defining: > > static const struct field_mapping fields[] > > ...only to throw away all type-safety and run it in a loop. Why not > unroll the loop, skip the array, and the runtime warning with something > like? > > read_sys_metadata_field16(MD_FIELD_ID_MAX_TDMRS, &ts->max_tdmrs); > read_sys_metadata_field16(MD_FIELD_ID_MAX_RESERVED_PER_TDMR, &ts->max_reserved_per_tdmr); > ...etc > > 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
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!): diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 4e2b2e2ac9f9..a177fb7332ae 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -270,60 +270,42 @@ 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_tdmr_sysinfo *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; - 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_tdmr_sysinfo, _offset) } - -/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */ -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]), -}; - -static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) +/* + * 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); \ + }) + +static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *ts) { - int ret; - int i; + int ret = 0; - /* Populate 'tdmr_sysinfo' 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, - tdmr_sysinfo); - 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]); - return 0; + return ret; } /* Calculate the actual TDMR size */
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?
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)))
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.
On 27/08/24 01:40, Huang, Kai wrote: > 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". FWIW, it isn't 0, it is void. Assignment to void is an error. Could use anything that is correct syntax but would produce a compile-time error e.g. (1 / 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.
On Tue, 2024-08-27 at 07:54 +0300, Adrian Hunter wrote: > > > > > > 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". > > FWIW, it isn't 0, it is void. Assignment to void is an error. Could use > anything that is correct syntax but would produce a compile-time error > e.g. (1 / 0). Ah I missed the '(void)'. I didn't thought this way (and yet to try out). Thanks for the insight. I already sent out the v3 based on Dan's suggestion. Besides the pros mentioned by Dan, I also found Dan's suggestion yields less LoC of the final tdx.c despite it is trivial. So let's continue on the v3.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index d8fa9325bf5e..2ce03c3ea017 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -272,9 +272,9 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) static int read_sys_metadata_field16(u64 field_id, int offset, - struct tdx_tdmr_sysinfo *ts) + void *stbuf) { - u16 *ts_member = ((void *)ts) + offset; + u16 *member = stbuf + offset; u64 tmp; int ret; @@ -286,7 +286,7 @@ static int read_sys_metadata_field16(u64 field_id, if (ret) return ret; - *ts_member = tmp; + *member = tmp; return 0; } @@ -296,17 +296,20 @@ struct field_mapping { int offset; }; -#define TD_SYSINFO_MAP(_field_id, _member) \ - { .field_id = MD_FIELD_ID_##_field_id, \ - .offset = offsetof(struct tdx_tdmr_sysinfo, _member) } +#define TD_SYSINFO_MAP(_field_id, _struct, _member) \ + { .field_id = MD_FIELD_ID_##_field_id, \ + .offset = offsetof(_struct, _member) } + +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ + TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member) /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */ 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]), + 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]), }; static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)