Re: [PATCH v3 09/21] x86/virt/tdx: Get information about TDX module and convertible memory

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

On Thu, 2022-04-28 at 07:06 -0700, Dave Hansen wrote:
> On 4/27/22 17:15, Kai Huang wrote:
> > On Wed, 2022-04-27 at 15:15 -0700, Dave Hansen wrote:
> > > On 4/5/22 21:49, Kai Huang wrote:
> > > > TDX provides increased levels of memory confidentiality and integrity.
> > > > This requires special hardware support for features like memory
> > > > encryption and storage of memory integrity checksums.  Not all memory
> > > > satisfies these requirements.
> > > > 
> > > > As a result, TDX introduced the concept of a "Convertible Memory Region"
> > > > (CMR).  During boot, the firmware builds a list of all of the memory
> > > > ranges which can provide the TDX security guarantees.  The list of these
> > > > ranges, along with TDX module information, is available to the kernel by
> > > > querying the TDX module via TDH.SYS.INFO SEAMCALL.
> > > > 
> > > > Host kernel can choose whether or not to use all convertible memory
> > > > regions as TDX memory.  Before TDX module is ready to create any TD
> > > > guests, all TDX memory regions that host kernel intends to use must be
> > > > configured to the TDX module, using specific data structures defined by
> > > > TDX architecture.  Constructing those structures requires information of
> > > > both TDX module and the Convertible Memory Regions.  Call TDH.SYS.INFO
> > > > to get this information as preparation to construct those structures.
> > > > 
> > > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > > > ---
> > > >  arch/x86/virt/vmx/tdx/tdx.c | 131 ++++++++++++++++++++++++++++++++++++
> > > >  arch/x86/virt/vmx/tdx/tdx.h |  61 +++++++++++++++++
> > > >  2 files changed, 192 insertions(+)
> > > > 
> > > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > > > index ef2718423f0f..482e6d858181 100644
> > > > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > > > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > > > @@ -80,6 +80,11 @@ static DEFINE_MUTEX(tdx_module_lock);
> > > >  
> > > >  static struct p_seamldr_info p_seamldr_info;
> > > >  
> > > > +/* Base address of CMR array needs to be 512 bytes aligned. */
> > > > +static struct cmr_info tdx_cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);
> > > > +static int tdx_cmr_num;
> > > > +static struct tdsysinfo_struct tdx_sysinfo;
> > > 
> > > I really dislike mixing hardware and software structures.  Please make
> > > it clear which of these are fully software-defined and which are part of
> > > the hardware ABI.
> > 
> > Both 'struct tdsysinfo_struct' and 'struct cmr_info' are hardware structures. 
> > They are defined in tdx.h which has a comment saying the data structures below
> > this comment is hardware structures:
> > 
> > 	+/*
> > 	+ * TDX architectural data structures
> > 	+ */
> > 
> > It is introduced in the P-SEAMLDR patch.
> > 
> > Should I explicitly add comments around the variables saying they are used by
> > hardware, something like:
> > 
> > 	/*
> > 	 * Data structures used by TDH.SYS.INFO SEAMCALL to return CMRs and
> > 	 * TDX module system information.
> > 	 */
> 
> I think we know they are data structures. :)
> 
> But, saying:
> 
> 	/* Used in TDH.SYS.INFO SEAMCALL ABI: */
> 
> *is* actually helpful.  It (probably) tells us where in the spec we can
> find the definition and tells how it gets used.  Plus, it tells us this
> isn't a software data structure.

Right.  I'll use your above comment.

> 
> > > > +	/* Get TDX module information and CMRs */
> > > > +	ret = tdx_get_sysinfo();
> > > > +	if (ret)
> > > > +		goto out;
> > > 
> > > Couldn't we get rid of that comment if you did something like:
> > > 
> > > 	ret = tdx_get_sysinfo(&tdx_cmr_array, &tdx_sysinfo);
> > 
> > Yes will do.
> > 
> > > and preferably make the variables function-local.
> > 
> > 'tdx_sysinfo' will be used by KVM too.
> 
> In other words, it's not a part of this series so I can't review whether
> this statement is correct or whether there's a better way to hand this
> information over to KVM.
> 
> This (minor) nugget influencing the design also isn't even commented or
> addressed in the changelog.

TDSYSINFO_STRUCT is 1024B and CMR array is 512B, so I don't think it should be
in the stack.  I can change to use dynamic allocation at the beginning and free
it at the end of the function.  KVM support patches can change it to static
variable in the file.

Or I can add a sentence saying KVM will need to use 'tdx_tdsysinfo' so use
static variable.  However currently KVM doesn't use CMR so no justification for
CMR array.

But I am thinking about memory hotplug interaction with TDX module
initialization.  That may use CMR info.  Let me send out proposal and close that
first to see whether this series needs to use CMR info out of this function.