| Message ID | f81ed362dcb88bdb60859b998d5b9a4ee258a5f3.1718538552.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 |
On 16.06.24 г. 15:01 ч., Kai Huang wrote: > Currently the kernel doesn't print any information regarding the TDX > module itself, e.g. module version. Printing such information is not > mandatory for initializing the TDX module, but in practice such > information is useful, especially to the developers. It's understood that it's not mandatory to print any information, just remove this sentence and leave the "In practice such...." > > For instance, there are 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 type: Debug or Production. > - TDX_FEATURES0: Supported TDX features. > - TDX module version, and the build date. > > And dump the information right after reading global metadata, so that > this information is printed no matter whether module initialization > fails or not. Instead of printing this on 3 separate rows why not print something like: "Initialising TDX Module $NUMERIC_VERSION ($BUILD_DATE $PRODUCTION_STATE), $TDX_FEATURES" That way: a) You convey the version information b) You explicitly state that initialisation has begun and make no guarantees that because this has been printed the module is indeed properly initialised. I'm thinking if someone could be mistaken that if this information is printed this surely means that the module is properly working, which is not the case. > > The actual dmesg will look like: > > virt/tdx: Production module. > virt/tdx: TDX_FEATURES0: 0xfbf > virt/tdx: Module version: 1.5.00.00.0481, build_date: 20230323 > > Link: https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@suse.com/T/#m352829aedf6680d4628c7e40dc40b332eda93355 [1] > Link: https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@suse.com/T/#m351ebcbc006d2e5bc3e7650206a087cb2708d451 [2] > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > arch/x86/virt/vmx/tdx/tdx.c | 67 +++++++++++++++++++++++++++++++++++++ > arch/x86/virt/vmx/tdx/tdx.h | 33 +++++++++++++++++- > 2 files changed, 99 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 4683884efcc6..ced40e3b516e 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -319,6 +319,61 @@ 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) What's the point of this define, simply use the raw TD_SYSINFO_MAP inside the respective function. It doesn't really add any value especially everything is encapsulated in one function. Literally you add it so that you don't have to type "struct tdx_sysinfo_module_info" on each of the 2 lines this define is used... > + > +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) DITTO > + > +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); > +} > + > +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; > + > + pr_info("%s module.\n", debug ? "Debug" : "Production"); > + > + pr_info("TDX_FEATURES0: 0x%llx\n", modinfo->tdx_features0); > + > + /* > + * 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("Module version: %u.%u.%02u.%02u.%04u, build_date: %u\n", > + modver->major, modver->minor, > + modver->update, modver->internal, > + modver->build_num, modver->build_date); > +} > + > #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ > TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_tdmr_info, _member) > > @@ -339,6 +394,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 +1186,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 6b61dc67b0af..d80ec797fbf1 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -31,6 +31,15 @@ > * > * See Table "Global Scope Metadata", TDX module 1.5 ABI spec. > */ nit: [Not related to this patch but still a problem in its own] Those fields are defined in the global_metadata.json which is part of the "Intel TDX Module v1.5 ABI Definitions" and not the 1.5 ABI spec, as the ABI spec is the pdf. > +#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 > + > #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 { > + 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; > }; > > #endif
On Tue, 2024-06-18 at 16:52 +0300, Nikolay Borisov wrote: > > On 16.06.24 г. 15:01 ч., Kai Huang wrote: > > Currently the kernel doesn't print any information regarding the TDX > > module itself, e.g. module version. Printing such information is not > > mandatory for initializing the TDX module, but in practice such > > information is useful, especially to the developers. > > It's understood that it's not mandatory to print any information, just > remove this sentence and leave the "In practice such...." Will do. > > > > > For instance, there are 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 type: Debug or Production. > > - TDX_FEATURES0: Supported TDX features. > > - TDX module version, and the build date. > > > > And dump the information right after reading global metadata, so that > > this information is printed no matter whether module initialization > > fails or not. > > Instead of printing this on 3 separate rows why not print something like: > > "Initialising TDX Module $NUMERIC_VERSION ($BUILD_DATE > $PRODUCTION_STATE), $TDX_FEATURES" > > That way: > a) You convey the version information > b) You explicitly state that initialisation has begun and make no > guarantees that because this has been printed the module is indeed > properly initialised. I'm thinking if someone could be mistaken that if > this information is printed this surely means that the module is > properly working, which is not the case. If the module fails to init, there will be message explicitly saying that _after_ the above message. But fine I can change to print in one line like below: virt/tdx: Initializing module: Production module, version 1.5.00.00.0481, build_date 20230323, TDX_FEATURES0 0xfbf If it reads more clear. [...] > > > > +#define TD_SYSINFO_MAP_MOD_INFO(_field_id, _member) \ > > + TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_module_info, _member) > > What's the point of this define, simply use the raw TD_SYSINFO_MAP > inside the respective function. It doesn't really add any value > especially everything is encapsulated in one function. Literally you add > it so that you don't have to type "struct tdx_sysinfo_module_info" on > each of the 2 lines this define is used... It makes the code shorter, w/o needing to type 'struct tdx_sysinfo_module_info' for each member. This way is also consistent with other structures (e.g., 'struct tdx_sysinfo_tdmr_info') which have more members. > > > + > > +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) > > DITTO I really want to avoid typing 'struct tdx_sysinfo_module_version' for each struct member. I don't think using TD_SYSINFO_MAP() directly is any better. > > > + > > +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); > > +} > > [...] > > --- a/arch/x86/virt/vmx/tdx/tdx.h > > +++ b/arch/x86/virt/vmx/tdx/tdx.h > > @@ -31,6 +31,15 @@ > > * > > * See Table "Global Scope Metadata", TDX module 1.5 ABI spec. > > */ > > nit: > > [Not related to this patch but still a problem in its own] > > Those fields are defined in the global_metadata.json which is part of > the "Intel TDX Module v1.5 ABI Definitions" and not the 1.5 ABI spec, > as the ABI spec is the pdf. I will update this. Thanks.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 4683884efcc6..ced40e3b516e 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -319,6 +319,61 @@ 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); +} + +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; + + pr_info("%s module.\n", debug ? "Debug" : "Production"); + + pr_info("TDX_FEATURES0: 0x%llx\n", modinfo->tdx_features0); + + /* + * 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("Module version: %u.%u.%02u.%02u.%04u, build_date: %u\n", + modver->major, modver->minor, + modver->update, modver->internal, + modver->build_num, modver->build_date); +} + #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \ TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_tdmr_info, _member) @@ -339,6 +394,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 +1186,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 6b61dc67b0af..d80ec797fbf1 100644 --- a/arch/x86/virt/vmx/tdx/tdx.h +++ b/arch/x86/virt/vmx/tdx/tdx.h @@ -31,6 +31,15 @@ * * See Table "Global Scope Metadata", TDX module 1.5 ABI spec. */ +#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 + #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 { + 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; }; #endif
Currently the kernel doesn't print any information regarding the TDX module itself, e.g. module version. Printing such information is not mandatory for initializing the TDX module, but in practice such information is useful, especially to the developers. For instance, there are 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 type: Debug or Production. - TDX_FEATURES0: Supported TDX features. - TDX module version, and the build date. 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: Production module. virt/tdx: TDX_FEATURES0: 0xfbf virt/tdx: Module version: 1.5.00.00.0481, build_date: 20230323 Link: https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@suse.com/T/#m352829aedf6680d4628c7e40dc40b332eda93355 [1] Link: https://lore.kernel.org/lkml/e2d844ad-182a-4fc0-a06a-d609c9cbef74@suse.com/T/#m351ebcbc006d2e5bc3e7650206a087cb2708d451 [2] Signed-off-by: Kai Huang <kai.huang@intel.com> --- arch/x86/virt/vmx/tdx/tdx.c | 67 +++++++++++++++++++++++++++++++++++++ arch/x86/virt/vmx/tdx/tdx.h | 33 +++++++++++++++++- 2 files changed, 99 insertions(+), 1 deletion(-)