Kai Huang wrote: > Currently the kernel doesn't print any information regarding the TDX > module itself, e.g. module version. In practice such information is > useful, especially to the developers. > > For instance, there are a couple of use cases for dumping module basic > information: > > 1) When something goes wrong around using TDX, the information like TDX > module version, supported features etc could be helpful [1][2]. > > 2) For Linux, when the user wants to update the TDX module, one needs to > replace the old module in a specific location in the EFI partition > with the new one so that after reboot the BIOS can load it. However, > after kernel boots, currently the user has no way to verify it is > indeed the new module that gets loaded and initialized (e.g., error > could happen when replacing the old module). With the module version > dumped the user can verify this easily. > > So dump the basic TDX module information: > > - TDX module version, and the build date. > - TDX module type: Debug or Production. > - TDX_FEATURES0: Supported TDX features. > > And dump the information right after reading global metadata, so that > this information is printed no matter whether module initialization > fails or not. > > The actual dmesg will look like: > > virt/tdx: Initializing TDX module: 1.5.00.00.0481 (build_date 20230323, Production module), TDX_FEATURES0 0xfbf > > Link: https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@xxxxxxxx/T/#m352829aedf6680d4628c7e40dc40b332eda93355 [1] > Link: https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@xxxxxxxx/T/#m351ebcbc006d2e5bc3e7650206a087cb2708d451 [2] > Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx> > --- > > v1 -> v2 (Nikolay): > - Change the format to dump TDX basic info. > - Slightly improve changelog. > > --- > arch/x86/virt/vmx/tdx/tdx.c | 64 +++++++++++++++++++++++++++++++++++++ > arch/x86/virt/vmx/tdx/tdx.h | 33 ++++++++++++++++++- > 2 files changed, 96 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 3253cdfa5207..5ac0c411f4f7 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -319,6 +319,58 @@ static int stbuf_read_sysmd_multi(const struct field_mapping *fields, > return 0; > } > > +#define TD_SYSINFO_MAP_MOD_INFO(_field_id, _member) \ > + TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_module_info, _member) > + > +static int get_tdx_module_info(struct tdx_sysinfo_module_info *modinfo) > +{ > + static const struct field_mapping fields[] = { > + TD_SYSINFO_MAP_MOD_INFO(SYS_ATTRIBUTES, sys_attributes), > + TD_SYSINFO_MAP_MOD_INFO(TDX_FEATURES0, tdx_features0), > + }; > + > + return stbuf_read_sysmd_multi(fields, ARRAY_SIZE(fields), modinfo); > +} > + > +#define TD_SYSINFO_MAP_MOD_VERSION(_field_id, _member) \ > + TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_module_version, _member) > + > +static int get_tdx_module_version(struct tdx_sysinfo_module_version *modver) > +{ > + static const struct field_mapping fields[] = { > + TD_SYSINFO_MAP_MOD_VERSION(MAJOR_VERSION, major), > + TD_SYSINFO_MAP_MOD_VERSION(MINOR_VERSION, minor), > + TD_SYSINFO_MAP_MOD_VERSION(UPDATE_VERSION, update), > + TD_SYSINFO_MAP_MOD_VERSION(INTERNAL_VERSION, internal), > + TD_SYSINFO_MAP_MOD_VERSION(BUILD_NUM, build_num), > + TD_SYSINFO_MAP_MOD_VERSION(BUILD_DATE, build_date), > + }; > + > + return stbuf_read_sysmd_multi(fields, ARRAY_SIZE(fields), modver); Looks good if stbuf_read_sysmd_multi() is replaced with the work being done internal to TD_SYSINFO_MAP_MOD_VERSION(). > +} > + > +static void print_basic_sysinfo(struct tdx_sysinfo *sysinfo) > +{ > + struct tdx_sysinfo_module_version *modver = &sysinfo->module_version; > + struct tdx_sysinfo_module_info *modinfo = &sysinfo->module_info; > + bool debug = modinfo->sys_attributes & TDX_SYS_ATTR_DEBUG_MODULE; Why is this casually checking for debug modules, but doing nothing with that indication? Shouldn't the kernel have policy around whether it wants to interoperate with a debug module? I would expect that kernel operation with a debug module would need explicit opt-in consideration. > + > + /* > + * TDX module version encoding: > + * > + * <major>.<minor>.<update>.<internal>.<build_num> > + * > + * When printed as text, <major> and <minor> are 1-digit, > + * <update> and <internal> are 2-digits and <build_num> > + * is 4-digits. > + */ > + pr_info("Initializing TDX module: %u.%u.%02u.%02u.%04u (build_date %u, %s module), TDX_FEATURES0 0x%llx\n", > + modver->major, modver->minor, modver->update, > + modver->internal, modver->build_num, > + modver->build_date, debug ? "Debug" : "Production", > + modinfo->tdx_features0); Another nice thing about json scripting is that this flag fields could be pretty-printed with symbolic names for the flags, but that can come later. > +} > + > #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ > TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_tdmr_info, _member) > > @@ -339,6 +391,16 @@ static int get_tdx_tdmr_sysinfo(struct tdx_sysinfo_tdmr_info *tdmr_sysinfo) > > static int get_tdx_sysinfo(struct tdx_sysinfo *sysinfo) > { > + int ret; > + > + ret = get_tdx_module_info(&sysinfo->module_info); > + if (ret) > + return ret; > + > + ret = get_tdx_module_version(&sysinfo->module_version); > + if (ret) > + return ret; > + > return get_tdx_tdmr_sysinfo(&sysinfo->tdmr_info); > } > > @@ -1121,6 +1183,8 @@ static int init_tdx_module(void) > if (ret) > return ret; > > + print_basic_sysinfo(&sysinfo); > + > /* > * To keep things simple, assume that all TDX-protected memory > * will come from the page allocator. Make sure all pages in the > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > index b5eb7c35f1dc..861ddf2c2e88 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -31,6 +31,15 @@ > * > * See the "global_metadata.json" in the "TDX 1.5 ABI definitions". > */ > +#define MD_FIELD_ID_SYS_ATTRIBUTES 0x0A00000200000000ULL > +#define MD_FIELD_ID_TDX_FEATURES0 0x0A00000300000008ULL > +#define MD_FIELD_ID_BUILD_DATE 0x8800000200000001ULL > +#define MD_FIELD_ID_BUILD_NUM 0x8800000100000002ULL > +#define MD_FIELD_ID_MINOR_VERSION 0x0800000100000003ULL > +#define MD_FIELD_ID_MAJOR_VERSION 0x0800000100000004ULL > +#define MD_FIELD_ID_UPDATE_VERSION 0x0800000100000005ULL > +#define MD_FIELD_ID_INTERNAL_VERSION 0x0800000100000006ULL This is where I would rather not take your word for it, or go review these constants myself if these were autogenerated from parsing json. > + > #define MD_FIELD_ID_MAX_TDMRS 0x9100000100000008ULL > #define MD_FIELD_ID_MAX_RESERVED_PER_TDMR 0x9100000100000009ULL > #define MD_FIELD_ID_PAMT_4K_ENTRY_SIZE 0x9100000100000010ULL > @@ -124,8 +133,28 @@ struct tdmr_info_list { > * > * Note not all metadata fields in each class are defined, only those > * used by the kernel are. > + * > + * Also note the "bit definitions" are architectural. > */ > > +/* Class "TDX Module Info" */ > +struct tdx_sysinfo_module_info { This name feels too generic, perhaps 'tdx_sys_info_features' makes it clearer? > + u32 sys_attributes; > + u64 tdx_features0; > +}; > + > +#define TDX_SYS_ATTR_DEBUG_MODULE 0x1 > + > +/* Class "TDX Module Version" */ > +struct tdx_sysinfo_module_version { > + u16 major; > + u16 minor; > + u16 update; > + u16 internal; > + u16 build_num; > + u32 build_date; > +}; > + > /* Class "TDMR Info" */ > struct tdx_sysinfo_tdmr_info { > u16 max_tdmrs; > @@ -134,7 +163,9 @@ struct tdx_sysinfo_tdmr_info { > }; > > struct tdx_sysinfo { > - struct tdx_sysinfo_tdmr_info tdmr_info; > + struct tdx_sysinfo_module_info module_info; > + struct tdx_sysinfo_module_version module_version; > + struct tdx_sysinfo_tdmr_info tdmr_info; Compare that to: struct tdx_sys_info { struct tdx_sys_info_features features; struct tdx_sys_info_version version; struct tdx_sys_info_tdmr tdmr; }; ...and tell me which oine is easier to read.
