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@xxxxxxxxx> > Reviewed-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx> > Reviewed-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx> > --- > > 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.
