Re: [PATCH v8 06/16] x86/virt/tdx: Get information about TDX module and TDX-capable memory

From: "Huang, Kai" <kai.huang@intel.com>
To: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "Luck, Tony" <tony.luck@intel.com>,
	"bagasdotme@gmail.com" <bagasdotme@gmail.com>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>,
	"Chatre, Reinette" <reinette.chatre@intel.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"Shahar, Sagi" <sagis@google.com>,
	"imammedo@redhat.com" <imammedo@redhat.com>,
	"Gao, Chao" <chao.gao@intel.com>,
	"Brown, Len" <len.brown@intel.com>,
	"sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"Huang, Ying" <ying.huang@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>
Subject: Re: [PATCH v8 06/16] x86/virt/tdx: Get information about TDX module and TDX-capable memory
Date: Mon, 9 Jan 2023 10:25:55 +0000	[thread overview]
Message-ID: <e605ce95f1b92fae630bf6abb801774bc28d8072.camel@intel.com> (raw)
In-Reply-To: <d1093b3e-cdab-b138-d488-19b9456be978@intel.com>

On Fri, 2023-01-06 at 09:46 -0800, Dave Hansen wrote:
> On 12/8/22 22:52, Kai Huang wrote:
> > For now, both the TDX module information and CMRs are only used during
> > the module initialization, so declare them as local.  However, they are
> > 1024 bytes and 512 bytes respectively.  Putting them to the stack
> > exceeds the default "stack frame size" that the kernel assumes as safe,
> > and the compiler yields a warning about this.  Add a kernel build flag
> > to extend the safe stack size to 4K for tdx.c to silence the warning --
> > the initialization function is only called once so it's safe to have a
> > 4K stack.
> 
> Gah.  This has gone off in a really odd direction.
> 
> The fact that this is called once really has nothing to do with how
> tolerant of a large stack we should be.  If a function is called once
> from a deep call stack, it can't consume a lot of stack space.  If it's
> called a billion times from a shallow stack depth, it can use all the
> stack it wants.

Agreed.

> 
> All I really wanted here was this:
> 
> static int init_tdx_module(void)
> {
> -	struct cmr_info cmr_array[MAX_CMRS] ...;+	static struct cmr_info
> cmr_array[MAX_CMRS] ...;
> 
> Just make the function variable static instead of having it be a global.
>  That's *IT*.

Yes will do.

Btw, I think putting hardware-used (physically contiguous large) data structure
on the stack isn't a good idea anyway.  The reason is as replied in below link:

https://lore.kernel.org/linux-mm/cc195eb6499cf021b4ce2e937200571915bfe66f.camel@intel.com/T/#m48506450cd22f84ab718d3b5bf8ddbff8fcc3362

Kernel stack can be vmalloc()-ed, so it doesn't guarantee data structure is
physically contiguous if data structure is across page boundary.

This particular TDX case works because the size and the alignment make sure both
cmr_array[] and tdsysinfo cannot cross page boundary.

> 
> > Note not all members in the 1024 bytes TDX module information are used
> > (even by the KVM).
> 
> I'm not sure what this has to do with anything.

You mentioned in v7 that:

>> This is also a great place to mention that the tdsysinfo_struct contains
>> a *lot* of gunk which will not be used for a bit or that may never get
>> used.

https://lore.kernel.org/linux-mm/cc195eb6499cf021b4ce2e937200571915bfe66f.camel@intel.com/T/#m168e619aac945fa418ccb1d6652113003243d895

Perhaps I misunderstood something but I was trying to address this.

Should I remove this sentence?

> 
> > diff --git a/arch/x86/virt/vmx/tdx/Makefile b/arch/x86/virt/vmx/tdx/Makefile
> > index 38d534f2c113..f8a40d15fdfc 100644
> > --- a/arch/x86/virt/vmx/tdx/Makefile
> > +++ b/arch/x86/virt/vmx/tdx/Makefile
> > @@ -1,2 +1,3 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +CFLAGS_tdx.o += -Wframe-larger-than=4096
> >  obj-y += tdx.o seamcall.o
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index b7cedf0589db..6fe505c32599 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/errno.h>
> >  #include <linux/printk.h>
> >  #include <linux/mutex.h>
> > +#include <asm/pgtable_types.h>
> >  #include <asm/msr.h>
> >  #include <asm/tdx.h>
> >  #include "tdx.h"
> > @@ -107,9 +108,8 @@ bool platform_tdx_enabled(void)
> >   * leaf function return code and the additional output respectively if
> >   * not NULL.
> >   */
> > -static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> > -				    u64 *seamcall_ret,
> > -				    struct tdx_module_output *out)
> > +static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> > +		    u64 *seamcall_ret, struct tdx_module_output *out)
> >  {
> >  	u64 sret;
> >  
> > @@ -150,12 +150,85 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> >  	}
> >  }
> >  
> > +static inline bool is_cmr_empty(struct cmr_info *cmr)
> > +{
> > +	return !cmr->size;
> > +}
> > +
> > +static void print_cmrs(struct cmr_info *cmr_array, int nr_cmrs)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < nr_cmrs; i++) {
> > +		struct cmr_info *cmr = &cmr_array[i];
> > +
> > +		/*
> > +		 * The array of CMRs reported via TDH.SYS.INFO can
> > +		 * contain tail empty CMRs.  Don't print them.
> > +		 */
> > +		if (is_cmr_empty(cmr))
> > +			break;
> > +
> > +		pr_info("CMR: [0x%llx, 0x%llx)\n", cmr->base,
> > +				cmr->base + cmr->size);
> > +	}
> > +}
> > +
> > +/*
> > + * Get the TDX module information (TDSYSINFO_STRUCT) and the array of
> > + * CMRs, and save them to @sysinfo and @cmr_array, which come from the
> > + * kernel stack.  @sysinfo must have been padded to have enough room
> > + * to save the TDSYSINFO_STRUCT.
> > + */
> > +static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
> > +			   struct cmr_info *cmr_array)
> > +{
> > +	struct tdx_module_output out;
> > +	u64 sysinfo_pa, cmr_array_pa;
> > +	int ret;
> > +
> > +	/*
> > +	 * Cannot use __pa() directly as @sysinfo and @cmr_array
> > +	 * come from the kernel stack.
> > +	 */
> > +	sysinfo_pa = slow_virt_to_phys(sysinfo);
> > +	cmr_array_pa = slow_virt_to_phys(cmr_array);
> 
> Note: they won't be on the kernel stack if they're 'static'.
> 
> > +	ret = seamcall(TDH_SYS_INFO, sysinfo_pa, TDSYSINFO_STRUCT_SIZE,
> > +			cmr_array_pa, MAX_CMRS, NULL, &out);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pr_info("TDX module: atributes 0x%x, vendor_id 0x%x, major_version %u, minor_version %u, build_date %u, build_num %u",
> > +		sysinfo->attributes,	sysinfo->vendor_id,
> > +		sysinfo->major_version, sysinfo->minor_version,
> > +		sysinfo->build_date,	sysinfo->build_num);
> > +
> > +	/* R9 contains the actual entries written to the CMR array. */
> > +	print_cmrs(cmr_array, out.r9);
> > +
> > +	return 0;
> > +}
> > +
> >  static int init_tdx_module(void)
> >  {
> > +	/*
> > +	 * @tdsysinfo and @cmr_array are used in TDH.SYS.INFO SEAMCALL ABI.
> > +	 * They are 1024 bytes and 512 bytes respectively but it's fine to
> > +	 * keep them in the stack as this function is only called once.
> > +	 */
> 
> Again, more silliness about being called once.
> 
> > +	DECLARE_PADDED_STRUCT(tdsysinfo_struct, tdsysinfo,
> > +			TDSYSINFO_STRUCT_SIZE, TDSYSINFO_STRUCT_ALIGNMENT);
> > +	struct cmr_info cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);
> 
> One more thing about being on the stack: These aren't implicitly zeroed.
>  They might have stack gunk from other calls in them.  I _think_ that's
> OK because of, for instance, the 'out.r9' that limits how many CMRs get
> read.  But, not being zeroed is a potential source of bugs and it's also
> something that reviewers (and you) need to think about to make sure it
> doesn't have side-effects.

Agreed.

As mentioned above, will change to use 'static' but keep the variables in the
function.

> 
> > +	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
> > +	int ret;
> > +
> > +	ret = tdx_get_sysinfo(sysinfo, cmr_array);
> > +	if (ret)
> > +		goto out;
> > +
> >  	/*
> >  	 * TODO:
> >  	 *
> > -	 *  - Get TDX module information and TDX-capable memory regions.
> >  	 *  - Build the list of TDX-usable memory regions.
> >  	 *  - Construct a list of TDMRs to cover all TDX-usable memory
> >  	 *    regions.
> > @@ -166,7 +239,9 @@ static int init_tdx_module(void)
> >  	 *
> >  	 *  Return error before all steps are done.
> >  	 */
> > -	return -EINVAL;
> > +	ret = -EINVAL;
> > +out:
> > +	return ret;
> >  }
> 
> I'm going to be lazy and not look into the future.  But, you don't need
> the "out:" label here, yet.  It doesn'serve any purpose like this, so
> why introduce it here?

The 'out' label is here because of below code:

	ret = tdx_get_sysinfo(...);
	if (ret)
		goto out;

If I don't have 'out' label here in this patch, do you mean something below?

	ret = tdx_get_sysinfo(...);
	if (ret)
		return ret;

	/*
	 * TODO:
	 * ...
	 * Return error before all steps are done.
	 */
	return -EINVAL;

> 
> >  static int __tdx_enable(void)
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> > index 884357a4133c..6d32f62e4182 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.h
> > +++ b/arch/x86/virt/vmx/tdx/tdx.h
> > @@ -3,6 +3,8 @@
> >  #define _X86_VIRT_TDX_H
> >  
> >  #include <linux/types.h>
> > +#include <linux/stddef.h>
> > +#include <linux/compiler_attributes.h>
> >  
> >  /*
> >   * This file contains both macros and data structures defined by the TDX
> > @@ -14,6 +16,80 @@
> >  /* MSR to report KeyID partitioning between MKTME and TDX */
> >  #define MSR_IA32_MKTME_KEYID_PARTITIONING	0x00000087
> >  
> > +/*
> > + * TDX module SEAMCALL leaf functions
> > + */
> > +#define TDH_SYS_INFO		32
> > +
> > +struct cmr_info {
> > +	u64	base;
> > +	u64	size;
> > +} __packed;
> > +
> > +#define MAX_CMRS			32
> > +#define CMR_INFO_ARRAY_ALIGNMENT	512
> > +
> > +struct cpuid_config {
> > +	u32	leaf;
> > +	u32	sub_leaf;
> > +	u32	eax;
> > +	u32	ebx;
> > +	u32	ecx;
> > +	u32	edx;
> > +} __packed;
> > +
> > +#define DECLARE_PADDED_STRUCT(type, name, size, alignment)	\
> > +	struct type##_padded {					\
> > +		union {						\
> > +			struct type name;			\
> > +			u8 padding[size];			\
> > +		};						\
> > +	} name##_padded __aligned(alignment)
> > +
> > +#define PADDED_STRUCT(name)	(name##_padded.name)
> 
> These don't turn out looking _that_ nice in practice, but I do vastly
> prefer them to hard-coded padding.
> 
> <snip>

Agreed.  Thanks for your original code.