lkml.org 
[lkml]   [2022]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 09/21] x86/virt/tdx: Get information about TDX module and convertible memory
From
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.

\
 
 \ /
  Last update: 2022-04-28 16:08    [W:1.815 / U:0.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site