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>
