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@xxxxxxxxx> >>> --- >>> 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.
