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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d976bfa87b7c608ebaf1a209952d6ac0020d8470.camel@intel.com>
Date: Fri, 30 Aug 2024 11:57:18 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Hansen, Dave" <dave.hansen@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "bp@...en8.de" <bp@...en8.de>, "peterz@...radead.org"
	<peterz@...radead.org>, "hpa@...or.com" <hpa@...or.com>, "mingo@...hat.com"
	<mingo@...hat.com>, "kirill.shutemov@...ux.intel.com"
	<kirill.shutemov@...ux.intel.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "Williams, Dan J"
	<dan.j.williams@...el.com>, "nik.borisov@...e.com" <nik.borisov@...e.com>
CC: "Gao, Chao" <chao.gao@...el.com>, "Edgecombe, Rick P"
	<rick.p.edgecombe@...el.com>, "x86@...nel.org" <x86@...nel.org>,
	"binbin.wu@...ux.intel.com" <binbin.wu@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Hunter,
 Adrian" <adrian.hunter@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH v3 5/8] x86/virt/tdx: Start to track all global metadata
 in one structure

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@...el.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().