Re: [PATCH v3 5/8] x86/virt/tdx: Start to track all global metadata in one structure

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

On Fri, 2024-08-30 at 14:01 +0300, Nikolay Borisov wrote:
> 
> On 27.08.24 г. 10:14 ч., 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_sys_info' to track all
> > global metadata fields.
> > 
> > TDX categories global metadata fields into different "Class"es.  E.g.,
> > the TDMR related fields are under class "TDMR Info".  Instead of making
> > 'struct tdx_sys_info' 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 construct_tdmr() can just take the
> > structure which contains "TDMR Info" metadata fields.
> > 
> > Add a new function get_tdx_sys_info() as the placeholder to read all
> > metadata fields, and call it at the beginning of init_tdx_module().  For
> > now it only calls get_tdx_sys_info_tdmr() to read TDMR related fields.
> > 
> > Note there is a functional change: get_tdx_sys_info_tdmr() is moved from
> > after build_tdx_memlist() to before it, but it is fine to do so.
> > 
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> > 
> > v2 -> v3:
> >   - Split out the part to rename 'struct tdx_tdmr_sysinfo' to 'struct
> >     tdx_sys_info_tdmr'.
> > 
> > 
> > ---
> >   arch/x86/virt/vmx/tdx/tdx.c | 19 ++++++++++++-------
> >   arch/x86/virt/vmx/tdx/tdx.h | 36 +++++++++++++++++++++++++++++-------
> >   2 files changed, 41 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 1cd9035c783f..24eb289c80e8 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -318,6 +318,11 @@ static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
> >   	return ret;
> >   }
> >   
> > +static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
> 
> A more apt name for this function would be init_tdx_sys_info, because it 
> will be executed only once during module initialization and it's really 
> initialising those values.
> 
> Given how complex TDX turns out to be it will be best if one off init 
> functions are prefixed with 'init_'.
> 

Since the function is only called once, I don't see big difference between
get_xx() vs init_xx().

Is init_xx() slightly better?  Maybe.  But to me we are not at that point that
get_xx() needs to be renamed.

One reason I don't want to do such change is the current upstream code is
already using get_tdx_tdmr_sysinfo().  I don't want to rename using init_xx().