Re: [PATCH v3 6/8] x86/virt/tdx: Print TDX module basic information

["Huang, Kai" <kai.huang@intel.com>]

On Fri, 2024-09-06 at 15:46 -0700, Dan Williams 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.
> 
> For this specific use case the kernel log is less useful then finding
> a place to put this in sysfs. This gets back to a proposal to have TDX
> instantiate a "tdx_tsm" device which among other things could host this
> version data.
> 
> The kernel log message is ok, but parsing the kernel log is not
> sufficient for this update validation flow concern.

Agree we eventually need /sysfs files for TDX module info like these.  For case
2) the developer who requested this use case to me just wanted to see the module
version dump in the kernel message so that he can verify, so I think this is at
least useful for developers.

Perhaps I can just trim the 2) to below?

  User may want to quickly know TDX module version to see whether the 
  loaded module is the expected one.


> 
> [..]
> > +static void print_basic_sys_info(struct tdx_sys_info *sysinfo)
> > +{
> > +	struct tdx_sys_info_features *features = &sysinfo->features;
> > +	struct tdx_sys_info_version *version = &sysinfo->version;
> > +
> > +	/*
> > +	 * 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), TDX_FEATURES0 0x%llx\n",
> > +			version->major, version->minor,	version->update,
> > +			version->internal, version->build_num,
> > +			version->build_date, features->tdx_features0);
> 
> I do not see the value in dumping a raw features value in the log.
> Either parse it or omit it. I would leave it for the tdx_tsm device to
> emit.

The raw features value is mainly for developers.  They can decode the value
based on the spec.  I think it will still be useful.

But sure I can remove it if you want.  With this I'll also defer reading the
tdx_features0 to the patch ("x86/virt/tdx: Don't initialize module that doesn't
support NO_RBP_MOD feature"), because if we don't print it, then we don't need
to read it in this patch.

I think I can move that patch before this patch, so tdx_features0 is always
introduced in that patch.  Then whether we decide to print tdx_features0 in this
patch doesn't impact that patch.