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

Commit Message

Kai Huang Dec. 9, 2022, 6:52 a.m. UTC

Start to transit out the "multi-steps" of initializing the TDX module as
listed in the skeleton infrastructure.  Do the first step to get the TDX
module information and the TDX-capable memory regions.

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.

CMRs tell the kernel which memory is TDX compatible.  The kernel takes
CMRs (plus a little more metadata) and constructs "TD Memory Regions"
(TDMRs).  TDMRs let the kernel grant TDX protections to some or all of
the CMR areas.

The TDX module information tells the kernel TDX module properties such
as metadata entry size, the maximum number of TDMRs, and the maximum
number of reserved areas per TDMR that the module allows, etc.

The list of CMRs, along with the TDX module information, is available to
the kernel by querying the TDX module.

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.

Note not all members in the 1024 bytes TDX module information are used
(even by the KVM).

Reviewed-by: Isaku Yamahata <[e-mail]>
Signed-off-by: Kai Huang <[e-mail]>
---

v7 -> v8: (Dave)
 - Improved changelog to tell this is the first patch to transit out the
   "multi-steps" init_tdx_module().
 - Removed all CMR check/trim code but to depend on later SEAMCALL.
 - Variable 'vertical alignment' in print TDX module information.
 - Added DECLARE_PADDED_STRUCT() for padded structure.
 - Made tdx_sysinfo and tdx_cmr_array[] to be function local variable
   (and rename them accordingly), and added -Wframe-larger-than=4096 flag
   to silence the build warning.

v6 -> v7:
 - Simplified the check of CMRs due to the fact that TDX actually
   verifies CMRs (that are passed by the BIOS) before enabling TDX.
 - Changed the function name from check_cmrs() -> trim_empty_cmrs().
 - Added CMR page aligned check so that later patch can just get the PFN
   using ">> PAGE_SHIFT".

v5 -> v6:
 - Added to also print TDX module's attribute (Isaku).
 - Removed all arguments in tdx_gete_sysinfo() to use static variables
   of 'tdx_sysinfo' and 'tdx_cmr_array' directly as they are all used
   directly in other functions in later patches.
 - Added Isaku's Reviewed-by.

- v3 -> v5 (no feedback on v4):
 - Renamed sanitize_cmrs() to check_cmrs().
 - Removed unnecessary sanity check against tdx_sysinfo and tdx_cmr_array
   actual size returned by TDH.SYS.INFO.
 - Changed -EFAULT to -EINVAL in couple places.
 - Added comments around tdx_sysinfo and tdx_cmr_array saying they are
   used by TDH.SYS.INFO ABI.
 - Changed to pass 'tdx_sysinfo' and 'tdx_cmr_array' as function
   arguments in tdx_get_sysinfo().
 - Changed to only print BIOS-CMR when check_cmrs() fails.

---
 arch/x86/virt/vmx/tdx/Makefile |  1 +
 arch/x86/virt/vmx/tdx/tdx.c    | 85 ++++++++++++++++++++++++++++++++--
 arch/x86/virt/vmx/tdx/tdx.h    | 76 ++++++++++++++++++++++++++++++
 3 files changed, 157 insertions(+), 5 deletions(-)

Comments

Dave Hansen Jan. 6, 2023, 5:46 p.m. UTC | #1

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.

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

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

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

> +	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?

>  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>

Kai Huang Jan. 9, 2023, 10:25 a.m. UTC | #2

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/[e-mail]/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/[e-mail]/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.

Dave Hansen Jan. 9, 2023, 7:52 p.m. UTC | #3

On 1/9/23 02:25, Huang, Kai wrote:
> On Fri, 2023-01-06 at 09:46 -0800, Dave Hansen wrote:
...
>>> 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/[e-mail]/T/#m168e619aac945fa418ccb1d6652113003243d895
> 
> Perhaps I misunderstood something but I was trying to address this.
> 
> Should I remove this sentence?

If someone goes looking at this patch, the see tdsysinfo_struct with
something like two dozen defined fields.  But, very few of them get used
in this patch.  Why?  Just saying that they are unused is a bit silly.

	The 'tdsysinfo_struct' is fairly large (1k) and contains a lot
	of info about the TD.  Fully define the entire structure, but
	only use the fields necessary to build the PAMT and TDMRs and
	pr_info() some basics about the module.

	The rest of the fields will get used... (by kvm?  never??)

...
>>> +	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;

Yes, if you remove the 'out:' label like you've shown in your reply,
it's actually _less_ code.  The labels are really only necessary when
you have common work to "undo" something before returning from the
function.  Here, you can just return.

Kai Huang Jan. 9, 2023, 10:07 p.m. UTC | #4

On Mon, 2023-01-09 at 11:52 -0800, Hansen, Dave wrote:
> On 1/9/23 02:25, Huang, Kai wrote:
> > On Fri, 2023-01-06 at 09:46 -0800, Dave Hansen wrote:
> ...
> > > > 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/[e-mail]/T/#m168e619aac945fa418ccb1d6652113003243d895
> > 
> > Perhaps I misunderstood something but I was trying to address this.
> > 
> > Should I remove this sentence?
> 
> If someone goes looking at this patch, the see tdsysinfo_struct with
> something like two dozen defined fields.  But, very few of them get used
> in this patch.  Why?  Just saying that they are unused is a bit silly.
> 
> 	The 'tdsysinfo_struct' is fairly large (1k) and contains a lot
> 	of info about the TD.  Fully define the entire structure, but
			  ^
		should be: "about the TDX module"?
			
> 	only use the fields necessary to build the PAMT and TDMRs and
> 	pr_info() some basics about the module.

Above looks great!  Thanks.

> 
> 	The rest of the fields will get used... (by kvm?  never??)

The current KVM TDX support series uses majority of the rest fields:

https://lore.kernel.org/lkml/99e5fc[e-mail]/

Only one field isn't used, but I don't want to assume it won't be used forever,
so I think "The rest of the fields will get used by KVM." is good enough.

> 
> ...
> > > > +	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;
> 
> Yes, if you remove the 'out:' label like you've shown in your reply,
> it's actually _less_ code.  The labels are really only necessary when
> you have common work to "undo" something before returning from the
> function.  Here, you can just return.
> 

Thanks will do.

I think this applies to construct_tdmrs() too (patch 09 - 11).  I'll check that
part too based on your above idea.

Dave Hansen Jan. 9, 2023, 10:11 p.m. UTC | #5

On 1/9/23 14:07, Huang, Kai wrote:
>>
>>       The 'tdsysinfo_struct' is fairly large (1k) and contains a lot
>>       of info about the TD.  Fully define the entire structure, but
>                           ^
>                 should be: "about the TDX module"?
> 

Yes, of course.  Thinko on my part.

Patch

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);
+	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.
+	 */
+	DECLARE_PADDED_STRUCT(tdsysinfo_struct, tdsysinfo,
+			TDSYSINFO_STRUCT_SIZE, TDSYSINFO_STRUCT_ALIGNMENT);
+	struct cmr_info cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);
+	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;
 }
 
 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)
+
+#define TDSYSINFO_STRUCT_SIZE		1024
+#define TDSYSINFO_STRUCT_ALIGNMENT	1024
+
+/*
+ * The size of this structure itself is flexible.  The actual structure
+ * passed to TDH.SYS.INFO must be padded to TDSYSINFO_STRUCT_SIZE and be
+ * aligned to TDSYSINFO_STRUCT_ALIGNMENT using DECLARE_PADDED_STRUCT().
+ */
+struct tdsysinfo_struct {
+	/* TDX-SEAM Module Info */
+	u32	attributes;
+	u32	vendor_id;
+	u32	build_date;
+	u16	build_num;
+	u16	minor_version;
+	u16	major_version;
+	u8	reserved0[14];
+	/* Memory Info */
+	u16	max_tdmrs;
+	u16	max_reserved_per_tdmr;
+	u16	pamt_entry_size;
+	u8	reserved1[10];
+	/* Control Struct Info */
+	u16	tdcs_base_size;
+	u8	reserved2[2];
+	u16	tdvps_base_size;
+	u8	tdvps_xfam_dependent_size;
+	u8	reserved3[9];
+	/* TD Capabilities */
+	u64	attributes_fixed0;
+	u64	attributes_fixed1;
+	u64	xfam_fixed0;
+	u64	xfam_fixed1;
+	u8	reserved4[32];
+	u32	num_cpuid_config;
+	/*
+	 * The actual number of CPUID_CONFIG depends on above
+	 * 'num_cpuid_config'.
+	 */
+	DECLARE_FLEX_ARRAY(struct cpuid_config, cpuid_configs);
+} __packed;
+
 /*
  * Do not put any hardware-defined TDX structure representations below
  * this comment!