[v2,08/10] x86/virt/tdx: Print TDX module basic information

Commit Message

Huang, Kai July 17, 2024, 3:40 a.m. UTC

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/[e-mail]/T/#m352829aedf6680d4628c7e40dc40b332eda93355 [1]
Link: https://lore.kernel.org/lkml/[e-mail]/T/#m351ebcbc006d2e5bc3e7650206a087cb2708d451 [2]
Signed-off-by: Kai Huang <[e-mail]>
---

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(-)

Comments

Dan Williams Aug. 6, 2024, 4:19 a.m. UTC | #1

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/[e-mail]/T/#m352829aedf6680d4628c7e40dc40b332eda93355 [1]
> Link: https://lore.kernel.org/lkml/[e-mail]/T/#m351ebcbc006d2e5bc3e7650206a087cb2708d451 [2]
> Signed-off-by: Kai Huang <[e-mail]>
> ---
> 
> 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.

Huang, Kai Aug. 6, 2024, 11:51 a.m. UTC | #2

On Mon, 2024-08-05 at 21:19 -0700, Williams, Dan J wrote:
> 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/[e-mail]/T/#m352829aedf6680d4628c7e40dc40b332eda93355 [1]
> > Link: https://lore.kernel.org/lkml/[e-mail]/T/#m351ebcbc006d2e5bc3e7650206a087cb2708d451 [2]
> > Signed-off-by: Kai Huang <[e-mail]>
> > ---
> > 
> > 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.

For now the purpose is just to print whether module is debug or
production in the dmesg to let the user easily see, just like the module
version info.

Currently Linux depends on the BIOS to load the TDX module.  For that we
need to put the module at /boot/efi/EFI/TDX/ and name it TDX-SEAM.so.  So
given a machine, it's hard for the user to know whether a module is debug
one (the user may be able to get such info from the BIOS log, but it is
not always available for the user).

Yes I agree we should have a policy in the kernel to handle debug module,
but I don't see urgent need of it.  So I would prefer to leave it as
future work when needed.

> 
> > +
> > +	/*
> > +	 * 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.

I'll look into how to auto-generate based on JSON file.

> 
> > +}
> > +
> >  #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.

I'll look into how to auto-generate based on JSON file.

> 
> > +
> >  #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?

I wanted to name the structure following the "Class" name in the JSON
file.  Both 'sys_attributes' and 'tdx_featueres0' are under class "Module
Info".

I guess "attributes" are not necessarily features.

> > +	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.

I agree this is easier to read if we don't look at the JSON file.  On the
other hand, following JSON file's "Class" names IMHO we can more easily
find which class to look at for a given member.

So I think they both have pros/cons, and I have no hard opinion on this.

Huang, Kai Aug. 6, 2024, 12:48 p.m. UTC | #3

> > >  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.
> 
> I agree this is easier to read if we don't look at the JSON file.  On the
> other hand, following JSON file's "Class" names IMHO we can more easily
> find which class to look at for a given member.
> 
> So I think they both have pros/cons, and I have no hard opinion on this.
> 

Hi Dan,

Btw, if we aim (either now or eventually) to auto generate all metadata
fields based on JSON file, I think it would be easier to name the
structures based on the "Class" names.  Otherwise we will need to do some
class-specific tweaks.

Dan Williams Aug. 7, 2024, 9:56 p.m. UTC | #4

Huang, Kai wrote:
> On Mon, 2024-08-05 at 21:19 -0700, Williams, Dan J wrote:
> > 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/[e-mail]/T/#m352829aedf6680d4628c7e40dc40b332eda93355 [1]
> > > Link: https://lore.kernel.org/lkml/[e-mail]/T/#m351ebcbc006d2e5bc3e7650206a087cb2708d451 [2]
> > > Signed-off-by: Kai Huang <[e-mail]>
> > > ---
> > > 
> > > 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.
> 
> For now the purpose is just to print whether module is debug or
> production in the dmesg to let the user easily see, just like the module
> version info.
> 
> Currently Linux depends on the BIOS to load the TDX module.  For that we
> need to put the module at /boot/efi/EFI/TDX/ and name it TDX-SEAM.so.  So
> given a machine, it's hard for the user to know whether a module is debug
> one (the user may be able to get such info from the BIOS log, but it is
> not always available for the user).
> 
> Yes I agree we should have a policy in the kernel to handle debug module,
> but I don't see urgent need of it.  So I would prefer to leave it as
> future work when needed.

Then lets leave printing it as future work as well. It has no value
outside of folks that can get their hands on a platform and a
module-build that enables debug and to my knowledge that capability is
not openly available.

In the meantime I assume TDs will just need to be careful to check for
this detail in their attestation report. It serves no real purpose to
the VMM kernel.

[..]
> > This name feels too generic, perhaps 'tdx_sys_info_features' makes it
> > clearer?
> 
> I wanted to name the structure following the "Class" name in the JSON
> file.  Both 'sys_attributes' and 'tdx_featueres0' are under class "Module
> Info".

I am not sure how far we need to take fidelity to the naming choices
that the TDX module makes. It would likely be sufficient to
note the class name in a comment for the origin of the fields, i.e. the
script has some mapping like:

{ class name, field name } => { linux struct name, linux attribute name }

...where they are mostly 1:1, but Linux has the option of picking more
relevant names, especially since the class names are not directly
reusable as Linux data type names.

> I guess "attributes" are not necessarily features.

Sure, but given that attributes have no real value to the VMM kernel at
this point and features do, then name the data structure by its primary
use.

> > > +	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.
> 
> I agree this is easier to read if we don't look at the JSON file.  On the
> other hand, following JSON file's "Class" names IMHO we can more easily
> find which class to look at for a given member.
> 
> So I think they both have pros/cons, and I have no hard opinion on this.

Yeah, it is arbitrary. All I can offer is this quote from Ingo when I
did the initial ACPI NFIT enabling and spilled all of its awkward
terminology into the Linux implementation [1]:

"So why on earth is this whole concept and the naming itself
('drivers/block/nd/' stands for 'NFIT Defined', apparently) revolving
around a specific 'firmware' mindset and revolving around specific,
weirdly named, overly complicated looking firmware interfaces that come
with their own new weird glossary??"

The TDX "Class" names are not completely unreasonable, but if they only
get replicated as part of kdoc comments on the data structures I think
that's ok. 

[1]: http://lore.kernel.org/[e-mail]

Huang, Kai Aug. 7, 2024, 10:32 p.m. UTC | #5

>>>> +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.
>>
>> For now the purpose is just to print whether module is debug or
>> production in the dmesg to let the user easily see, just like the module
>> version info.
>>
>> Currently Linux depends on the BIOS to load the TDX module.  For that we
>> need to put the module at /boot/efi/EFI/TDX/ and name it TDX-SEAM.so.  So
>> given a machine, it's hard for the user to know whether a module is debug
>> one (the user may be able to get such info from the BIOS log, but it is
>> not always available for the user).
>>
>> Yes I agree we should have a policy in the kernel to handle debug module,
>> but I don't see urgent need of it.  So I would prefer to leave it as
>> future work when needed.
> 
> Then lets leave printing it as future work as well. It has no value
> outside of folks that can get their hands on a platform and a
> module-build that enables debug and to my knowledge that capability is
> not openly available.
> 
> In the meantime I assume TDs will just need to be careful to check for
> this detail in their attestation report. It serves no real purpose to
> the VMM kernel.

Sure I'll remove.

It's basically for kernel developers and customers who are trying to 
integrating TDX to their environment to easily find some basic module 
info when something went wrong or they just want to check.

So if we don't print debug, then the 'sys_attributes' member is no 
longer needed, that means if we want to keep 'struct 
tdx_sysinfo_module_info' (or a better name in the next version) then it 
will only have one member, which is 'tdx_features0'.

In the long term, we might need to query other 'tdx_featuresN' fields 
since TDX module actually provides a metadata field 'NUM_TDX_FEATURES' 
to report how many fields like 'TDX_FEATURES0' the module has.  But I 
don't see that coming in any near future.

So perhaps we don't need to restrictly follow 1:1 between 'linux 
structure' <-> 'TDX class', and put the 'tdx_features0' together with 
TDX module version members and rename that one to 'struct 
tdx_sys_module_info'?

> 
> [..]
>>> This name feels too generic, perhaps 'tdx_sys_info_features' makes it
>>> clearer?
>>
>> I wanted to name the structure following the "Class" name in the JSON
>> file.  Both 'sys_attributes' and 'tdx_featueres0' are under class "Module
>> Info".
> 
> I am not sure how far we need to take fidelity to the naming choices
> that the TDX module makes. It would likely be sufficient to
> note the class name in a comment for the origin of the fields, i.e. the
> script has some mapping like:
> 
> { class name, field name } => { linux struct name, linux attribute name }
> 
> ...where they are mostly 1:1, but Linux has the option of picking more
> relevant names, especially since the class names are not directly
> reusable as Linux data type names.

Yes this seems better.

> 
>> I guess "attributes" are not necessarily features.
> 
> Sure, but given that attributes have no real value to the VMM kernel at
> this point and features do, then name the data structure by its primary
> use.

Sure.

> 
>>>> +	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.
>>
>> I agree this is easier to read if we don't look at the JSON file.  On the
>> other hand, following JSON file's "Class" names IMHO we can more easily
>> find which class to look at for a given member.
>>
>> So I think they both have pros/cons, and I have no hard opinion on this.
> 
> Yeah, it is arbitrary. All I can offer is this quote from Ingo when I
> did the initial ACPI NFIT enabling and spilled all of its awkward
> terminology into the Linux implementation [1]:
> 
> "So why on earth is this whole concept and the naming itself
> ('drivers/block/nd/' stands for 'NFIT Defined', apparently) revolving
> around a specific 'firmware' mindset and revolving around specific,
> weirdly named, overly complicated looking firmware interfaces that come
> with their own new weird glossary??"
> 
> The TDX "Class" names are not completely unreasonable, but if they only
> get replicated as part of kdoc comments on the data structures I think
> that's ok.
> 
> [1]: http://lore.kernel.org/[e-mail]

Thanks for the info!

I agree we don't need exactly follow TDX "class" to name linux 
structures. We can add a comment to mention which structure/member 
corresponds to which class/member in TDX spec when needed.

Chenyi Qiang Aug. 8, 2024, 10:31 a.m. UTC | #6

On 7/17/2024 11:40 AM, 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/[e-mail]/T/#m352829aedf6680d4628c7e40dc40b332eda93355 [1]
> Link: https://lore.kernel.org/lkml/[e-mail]/T/#m351ebcbc006d2e5bc3e7650206a087cb2708d451 [2]
> Signed-off-by: Kai Huang <[e-mail]>
> ---
> 
> 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.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
> +
>  #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

One minor issue, TDX_SYS_ATTR_DEBUG_MODULE is indicated by bit 31 of
sys_attributes.

> +
> +/* 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

Huang, Kai Aug. 8, 2024, 11:52 p.m. UTC | #7

On Thu, 2024-08-08 at 18:31 +0800, Chenyi Qiang wrote:
> > +
> > +#define TDX_SYS_ATTR_DEBUG_MODULE	0x1
> 
> One minor issue, TDX_SYS_ATTR_DEBUG_MODULE is indicated by bit 31 of
> sys_attributes.

Facepalm :( Thanks!

Patch

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);
+}
+
+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;
+
+	/*
+	 * 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);
+}
+
 #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
+
 #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