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.
