Message-ID: <775bb92d9e201f5efc2a939def1ecaafdc9c6bc0.camel@intel.com>
Date: Wed, 19 Jun 2024 00:22:34 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "nik.borisov@...e.com" <nik.borisov@...e.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "Hansen, Dave"
	<dave.hansen@...el.com>, "bp@...en8.de" <bp@...en8.de>, "x86@...nel.org"
	<x86@...nel.org>, "peterz@...radead.org" <peterz@...radead.org>,
	"hpa@...or.com" <hpa@...or.com>, "mingo@...hat.com" <mingo@...hat.com>,
	"Williams, Dan J" <dan.j.williams@...el.com>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "seanjc@...gle.com" <seanjc@...gle.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "binbin.wu@...ux.intel.com"
	<binbin.wu@...ux.intel.com>, "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH 7/9] x86/virt/tdx: Print TDX module basic information

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.

Powered by blists - more mailing lists