Re: [PATCH 6/9] x86/virt/tdx: Start to track all global metadata in one structure

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

On Tue, 2024-06-18 at 16:17 +0300, Nikolay Borisov wrote:
> 
> On 16.06.24 г. 15:01 ч., Kai Huang wrote:
> > The TDX module provides a set of "global metadata fields".  They report
> > things like TDX module version, supported features, and fields related
> > to create/run TDX guests and so on.
> > 
> > Currently the kernel only reads "TD Memory Region" (TDMR) related fields
> > for module initialization.  There are immediate needs which require the
> > TDX module initialization to read more global metadata including module
> > version, supported features and "Convertible Memory Regions" (CMRs).
> > 
> > Also, KVM will need to read more metadata fields to support baseline TDX
> > guests.  In the longer term, other TDX features like TDX Connect (which
> > supports assigning trusted IO devices to TDX guest) may also require
> > other kernel components such as pci/vt-d to access global metadata.
> > 
> > To meet all those requirements, the idea is the TDX host core-kernel to
> > to provide a centralized, canonical, and read-only structure for the
> > global metadata that comes out from the TDX module for all kernel
> > components to use.
> > 
> > As the first step, introduce a new 'struct tdx_sysinfo' to track all
> > global metadata fields.
> > 
> > TDX categories global metadata fields into different "Class"es.  E.g.,
> > the current TDMR related fields are under class "TDMR Info".  Instead of
> > making 'struct tdx_sysinfo' a plain structure to contain all metadata
> > fields, organize them in smaller structures based on the "Class".
> > 
> > This allows those metadata fields to be used in finer granularity thus
> > makes the code more clear.  E.g., the current construct_tdmr() can just
> > take the structure which contains "TDMR Info" metadata fields.
> > 
> > Start with moving 'struct tdx_tdmr_sysinfo' to 'struct tdx_sysinfo', and
> > rename 'struct tdx_tdmr_sysinfo' to 'struct tdx_sysinfo_tdmr_info' to
> > make it consistent with the "class name".
> > 
> > Add a new function get_tdx_sysinfo() as the place to read all metadata
> > fields, and call it at the beginning of init_tdx_module().  Move the
> > existing get_tdx_tdmr_sysinfo() to get_tdx_sysinfo().
> > 
> > Note there is a functional change: get_tdx_tdmr_sysinfo() is moved from
> > after build_tdx_memlist() to before it, but it is fine to do so.
> 
> Why? I don't see build_tdx_memlist() using any tdmr data.

This is a preparation patch to track more metadata fields in one 'struct
tdx_sysinfo'.  Eventually (as mentioned in the changelog) there will be
more fields not related to TDX module initialization in that structure.  
It makes more sense to read metadata first, and then the consumers can use
them.

You also observed that build_tdx_memlist() doesn't use any tdmr data, thus
the order doesn't matter from functionality's perspective.

Switching the order is part of "start to track all global metadata in one
structure".

> 
> > 
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> 
> This patch semantically does 3 independent things :
> 
> 1. Renames tdx_tdmr_sysinfo to tdx_sysinfo_tdmr_info
> 2. Introduces tdx_sysinfo and puts the aforementioned struct in it.
> 3. Moves get_tdx_tdmr_sysinfo

What's wrong of doing them together, as "start to track all global
metadata in one structure"?

> 
> > ---
> >   arch/x86/virt/vmx/tdx/tdx.c | 29 +++++++++++++++++------------
> >   arch/x86/virt/vmx/tdx/tdx.h | 32 +++++++++++++++++++++++++-------
> >   2 files changed, 42 insertions(+), 19 deletions(-)
> > 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index fad42014ca37..4683884efcc6 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -320,11 +320,11 @@ static int stbuf_read_sysmd_multi(const struct field_mapping *fields,
> >   }
> >   
> >   #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)	\
> > -	TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)
> > +	TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_tdmr_info, _member)
> >   
> > -static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
> > +static int get_tdx_tdmr_sysinfo(struct tdx_sysinfo_tdmr_info *tdmr_sysinfo)
> >   {
> > -	/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
> > +	/* Map TD_SYSINFO fields into 'struct tdx_sysinfo_tdmr_info': */
> >   	static const struct field_mapping fields[] = {
> >   		TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS,		max_tdmrs),
> >   		TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
> > @@ -337,6 +337,11 @@ static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
> >   	return stbuf_read_sysmd_multi(fields, ARRAY_SIZE(fields), tdmr_sysinfo);
> >   }
> >   
> > +static int get_tdx_sysinfo(struct tdx_sysinfo *sysinfo)
> 
> What's the point of this function, directly calling 
> get_tdx_tdmr_sysinfo(&sysinfo->tdmr_info); isn't any less obvious.

The patch title says "start to track all global metadata in one
structure".  'struct tdx_sysinf' is that structure.

> 
> > +{
> > +	return get_tdx_tdmr_sysinfo(&sysinfo->tdmr_info);
> > +}
> > +
> >   /* Calculate the actual TDMR size */
> >   static int tdmr_size_single(u16 max_reserved_per_tdmr)
> >   {
> 
> <snip>