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

[Dave Hansen <dave.hansen@intel.com>]

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.

>>> +	/* 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.