cpu/x86/smm: Improve smm stack setup

Both the relocation handler and the permanent handler use the same
stacks, so things can be simplified.

Change-Id: I7bdca775550e8280757a6c5a5150a0d638d5fc2d
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/58698
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: HAOUAS Elyes <ehaouas@noos.fr>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>

4 files changed, 47 insertions, 97 deletions

diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c
index fee5940b18..7d0a20fa70 100644
--- a/src/cpu/x86/mp_init.c
+++ b/src/cpu/x86/mp_init.c
@@ -760,10 +760,9 @@ static void adjust_smm_apic_id_map(struct smm_loader_params *smm_params)
 }
 
 static enum cb_err install_relocation_handler(int num_cpus, size_t real_save_state_size,
-				      size_t save_state_size, uintptr_t perm_smbase)
+				      size_t save_state_size)
 {
 	struct smm_loader_params smm_params = {
-		.per_cpu_stack_size = CONFIG_SMM_STUB_STACK_SIZE,
 		.num_concurrent_stacks = num_cpus,
 		.real_cpu_save_state_size = real_save_state_size,
 		.per_cpu_save_state_size = save_state_size,
@@ -771,11 +770,7 @@ static enum cb_err install_relocation_handler(int num_cpus, size_t real_save_sta
 		.handler = smm_do_relocation,
 	};
 
-	/* Allow callback to override parameters. */
-	if (mp_state.ops.adjust_smm_params != NULL)
-		mp_state.ops.adjust_smm_params(&smm_params, 0);
-
-	if (smm_setup_relocation_handler(perm_smbase, &smm_params)) {
+	if (smm_setup_relocation_handler(&smm_params)) {
 		printk(BIOS_ERR, "%s: smm setup failed\n", __func__);
 		return CB_ERR;
 	}
@@ -798,17 +793,12 @@ static enum cb_err install_permanent_handler(int num_cpus, uintptr_t smbase,
 	 * size and save state size for each CPU.
 	 */
 	struct smm_loader_params smm_params = {
-		.per_cpu_stack_size = CONFIG_SMM_MODULE_STACK_SIZE,
 		.num_concurrent_stacks = num_cpus,
 		.real_cpu_save_state_size = real_save_state_size,
 		.per_cpu_save_state_size = save_state_size,
 		.num_concurrent_save_states = num_cpus,
 	};
 
-	/* Allow callback to override parameters. */
-	if (mp_state.ops.adjust_smm_params != NULL)
-		mp_state.ops.adjust_smm_params(&smm_params, 1);
-
 	printk(BIOS_DEBUG, "Installing permanent SMM handler to 0x%08lx\n", smbase);
 
 	if (smm_load_module(smbase, smsize, &smm_params))
@@ -829,10 +819,15 @@ static void load_smm_handlers(void)
 	if (!is_smm_enabled())
 		return;
 
+	if (smm_setup_stack(mp_state.perm_smbase, mp_state.perm_smsize, mp_state.cpu_count,
+			    CONFIG_SMM_MODULE_STACK_SIZE)) {
+		printk(BIOS_ERR, "Unable to install SMM relocation handler.\n");
+		smm_disable();
+	}
+
 	/* Install handlers. */
 	if (install_relocation_handler(mp_state.cpu_count, real_save_state_size,
-				       smm_save_state_size, mp_state.perm_smbase) !=
-										CB_SUCCESS) {
+				       smm_save_state_size) != CB_SUCCESS) {
 		printk(BIOS_ERR, "Unable to install SMM relocation handler.\n");
 		smm_disable();
 	}
diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c
index ba179ca8b7..ce7cf20272 100644
--- a/src/cpu/x86/smm/smm_module_loader.c
+++ b/src/cpu/x86/smm/smm_module_loader.c
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
 #include <acpi/acpi_gnvs.h>
+#include <stddef.h>
 #include <stdint.h>
 #include <string.h>
 #include <rmodule.h>
@@ -239,32 +240,26 @@ static int smm_place_entry_code(uintptr_t smbase, unsigned int num_cpus,
 	return 1;
 }
 
-/*
- * Place stacks in base -> base + size region, but ensure the stacks don't
- * overlap the staggered entry points.
- */
-static uintptr_t smm_stub_place_stacks(const uintptr_t base, struct smm_loader_params *params)
-{
-	size_t total_stack_size;
-	uintptr_t stacks_top;
-
-	/* If stack space is requested assume the space lives in the lower
-	 * half of SMRAM. */
-	total_stack_size = params->per_cpu_stack_size *
-			   params->num_concurrent_stacks;
-	printk(BIOS_DEBUG, "%s: cpus: %zx : stack space: needed -> %zx\n",
-		__func__, params->num_concurrent_stacks,
-		total_stack_size);
-
-	/* There has to be at least one stack user. */
-	if (params->num_concurrent_stacks < 1)
-		return 0;
+static uintptr_t stack_top;
+static size_t g_stack_size;
 
-	/* Stacks extend down to SMBASE */
-	stacks_top = base + total_stack_size;
-	printk(BIOS_DEBUG, "%s: exit, stack_top %lx\n", __func__, stacks_top);
+int smm_setup_stack(const uintptr_t perm_smbase, const size_t perm_smram_size,
+		     const unsigned int total_cpus, const size_t stack_size)
+{
+	/* Need a minimum stack size and alignment. */
+	if (stack_size <= SMM_MINIMUM_STACK_SIZE || (stack_size & 3) != 0) {
+		printk(BIOS_ERR, "%s: need minimum stack size\n", __func__);
+		return -1;
+	}
 
-	return stacks_top;
+	const size_t total_stack_size = total_cpus * stack_size;
+	if (total_stack_size >= perm_smram_size) {
+		printk(BIOS_ERR, "%s: Stack won't fit smram\n", __func__);
+		return -1;
+	}
+	stack_top = perm_smbase + total_stack_size;
+	g_stack_size = stack_size;
+	return 0;
 }
 
 /*
@@ -284,7 +279,7 @@ static int smm_stub_place_staggered_entry_points(char *base,
 	if (params->num_concurrent_save_states > 1 || stub_entry_offset != 0) {
 		rc = smm_place_entry_code((uintptr_t)base,
 					  params->num_concurrent_save_states,
-					  (uintptr_t)params->stack_top, params);
+					  stack_top, params);
 	}
 	return rc;
 }
@@ -309,18 +304,15 @@ static int smm_stub_place_staggered_entry_points(char *base,
  * permanent SMM handler.
  * The CPU stack is decided at runtime in the stub and is treaded as a continuous
  * region. As this might not fit the default SMRAM region, the same region used
- * by the permanent handler can be used during relocation. This is done via the
- * smram_start argument.
+ * by the permanent handler can be used during relocation.
  */
 static int smm_module_setup_stub(const uintptr_t smbase, const size_t smm_size,
 				 struct smm_loader_params *params,
-				 void *const fxsave_area,
-				 const uintptr_t smram_start)
+				 void *const fxsave_area)
 {
 	size_t total_save_state_size;
 	size_t smm_stub_size;
 	uintptr_t smm_stub_loc;
-	uintptr_t stacks_top;
 	size_t size;
 	uintptr_t base;
 	size_t i;
@@ -361,13 +353,6 @@ static int smm_module_setup_stub(const uintptr_t smbase, const size_t smm_size,
 		return -1;
 	}
 
-	/* Need a minimum stack size and alignment. */
-	if (params->per_cpu_stack_size <= SMM_MINIMUM_STACK_SIZE ||
-	    (params->per_cpu_stack_size & 3) != 0) {
-		printk(BIOS_ERR, "%s: need minimum stack size\n", __func__);
-		return -1;
-	}
-
 	smm_stub_size = rmodule_memory_size(&smm_stub);
 
 	/* Put the stub at the main entry point */
@@ -379,16 +364,10 @@ static int smm_module_setup_stub(const uintptr_t smbase, const size_t smm_size,
 		return -1;
 	}
 
-	/* The stacks, if requested, live in the lower half of SMRAM space
-	 * for default handler, but for relocated handler it lives at the beginning
-	 * of SMRAM which is TSEG base
-	 */
-	stacks_top = smm_stub_place_stacks(smram_start, params);
-	if (stacks_top == 0) {
+	if (stack_top == 0) {
 		printk(BIOS_ERR, "%s: error assigning stacks\n", __func__);
 		return -1;
 	}
-	params->stack_top = stacks_top;
 	/* Load the stub. */
 	if (rmodule_load((void *)smm_stub_loc, &smm_stub)) {
 		printk(BIOS_ERR, "%s: load module failed\n", __func__);
@@ -402,19 +381,15 @@ static int smm_module_setup_stub(const uintptr_t smbase, const size_t smm_size,
 
 	/* Setup the parameters for the stub code. */
 	stub_params = rmodule_parameters(&smm_stub);
-	stub_params->stack_top = (uintptr_t)stacks_top;
-	stub_params->stack_size = params->per_cpu_stack_size;
+	stub_params->stack_top = stack_top;
+	stub_params->stack_size = g_stack_size;
 	stub_params->c_handler = (uintptr_t)params->handler;
 	stub_params->fxsave_area = (uintptr_t)fxsave_area;
 	stub_params->fxsave_area_size = FXSAVE_SIZE;
 
-	const size_t total_stack_size =
-		params->num_concurrent_stacks * params->per_cpu_stack_size;
-	printk(BIOS_DEBUG, "%s: stack_end = 0x%zx\n",
-		__func__, stub_params->stack_top - total_stack_size);
 	printk(BIOS_DEBUG,
 		"%s: stack_top = 0x%x\n", __func__, stub_params->stack_top);
-	printk(BIOS_DEBUG, "%s: stack_size = 0x%x\n",
+	printk(BIOS_DEBUG, "%s: per cpu stack_size = 0x%x\n",
 		__func__, stub_params->stack_size);
 	printk(BIOS_DEBUG, "%s: runtime.start32_offset = 0x%x\n", __func__,
 		stub_params->start32_offset);
@@ -439,7 +414,7 @@ static int smm_module_setup_stub(const uintptr_t smbase, const size_t smm_size,
  * assumption is that the stub will be entered from the default SMRAM
  * location: 0x30000 -> 0x40000.
  */
-int smm_setup_relocation_handler(const uintptr_t perm_smram,  struct smm_loader_params *params)
+int smm_setup_relocation_handler(struct smm_loader_params *params)
 {
 	uintptr_t smram = SMM_DEFAULT_BASE;
 	printk(BIOS_SPEW, "%s: enter\n", __func__);
@@ -459,7 +434,7 @@ int smm_setup_relocation_handler(const uintptr_t perm_smram,  struct smm_loader_
 
 	printk(BIOS_SPEW, "%s: exit\n", __func__);
 	return smm_module_setup_stub(smram, SMM_DEFAULT_SIZE,
-				     params, fxsave_area_relocation, perm_smram);
+				     params, fxsave_area_relocation);
 }
 
 /*
@@ -519,11 +494,9 @@ int smm_load_module(const uintptr_t smram_base, const size_t smram_size,
 	if (CONFIG(DEBUG_SMI))
 		memset((void *)smram_base, 0xcd, smram_size);
 
-	total_stack_size = params->per_cpu_stack_size *
-			   params->num_concurrent_stacks;
+	total_stack_size = stack_top - smram_base;
 	total_size += total_stack_size;
 	/* Stacks are the base of SMRAM */
-	params->stack_top = smram_base + total_stack_size;
 
 	/* MSEG starts at the top of SMRAM and works down */
 	if (CONFIG(STM)) {
@@ -580,8 +553,6 @@ int smm_load_module(const uintptr_t smram_base, const size_t smram_size,
 
 	printk(BIOS_DEBUG, "%s: smram_start: 0x%lx\n",  __func__, smram_base);
 	printk(BIOS_DEBUG, "%s: smram_end: %lx\n", __func__, smram_base + smram_size);
-	printk(BIOS_DEBUG, "%s: stack_top: %lx\n",
-		 __func__, params->stack_top);
 	printk(BIOS_DEBUG, "%s: handler start %p\n",
 		 __func__, params->handler);
 	printk(BIOS_DEBUG, "%s: handler_size %zx\n",
@@ -619,5 +590,5 @@ int smm_load_module(const uintptr_t smram_base, const size_t smram_size,
 			cpus[i].ss_start + params->per_cpu_save_state_size;
 	}
 
-	return smm_module_setup_stub(base, smram_size, params, fxsave_area, smram_base);
+	return smm_module_setup_stub(base, smram_size, params, fxsave_area);
 }
diff --git a/src/include/cpu/x86/mp.h b/src/include/cpu/x86/mp.h
index 934d217b78..1b4c956b59 100644
--- a/src/include/cpu/x86/mp.h
+++ b/src/include/cpu/x86/mp.h
@@ -45,17 +45,6 @@ struct mp_ops {
 	 */
 	void (*get_microcode_info)(const void **microcode, int *parallel);
 	/*
-	 * Optionally adjust SMM handler parameters to override the default
-	 * values.  The is_perm variable indicates if the parameters to adjust
-	 * are for the relocation handler or the permanent handler. This
-	 * function is therefore called twice -- once for each handler.
-	 * By default the parameters for each SMM handler are:
-	 *       stack_size     num_concurrent_stacks num_concurrent_save_states
-	 * relo: save_state_size    get_cpu_count()          1
-	 * perm: save_state_size    get_cpu_count()          get_cpu_count()
-	 */
-	void (*adjust_smm_params)(struct smm_loader_params *slp, int is_perm);
-	/*
 	 * Optionally provide a callback prior to the APs starting SMM
 	 * relocation or CPU driver initialization. However, note that
 	 * this callback is called after SMM handlers have been loaded.
@@ -92,13 +81,11 @@ struct mp_ops {
  * 3. get_smm_info()
  * 4. get_microcode_info()
  * 5. adjust_cpu_apic_entry() for each number of get_cpu_count()
- * 6. adjust_smm_params(is_perm = 0)
- * 7. adjust_smm_params(is_perm = 1)
- * 8. pre_mp_smm_init()
- * 9. per_cpu_smm_trigger() in parallel for all cpus which calls
+ * 6. pre_mp_smm_init()
+ * 7. per_cpu_smm_trigger() in parallel for all cpus which calls
  *    relocation_handler() in SMM.
- * 10. mp_initialize_cpu() for each cpu
- * 11. post_mp_init()
+ * 8. mp_initialize_cpu() for each cpu
+ * 9. post_mp_init()
  */
 enum cb_err mp_init_with_smm(struct bus *cpu_bus, const struct mp_ops *mp_ops);
 
diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h
index 848996f845..37247ece49 100644
--- a/src/include/cpu/x86/smm.h
+++ b/src/include/cpu/x86/smm.h
@@ -120,9 +120,6 @@ static inline bool smm_points_to_smram(const void *ptr, const size_t len)
 /* SMM Module Loading API */
 
 /* The smm_loader_params structure provides direction to the SMM loader:
- * - stack_top - optional external stack provided to loader. It must be at
- *               least per_cpu_stack_size * num_concurrent_stacks in size.
- * - per_cpu_stack_size - stack size per CPU for smm modules.
  * - num_concurrent_stacks - number of concurrent cpus in handler needing stack
  *                           optional for setting up relocation handler.
  * - per_cpu_save_state_size - the SMM save state size per cpu
@@ -135,8 +132,6 @@ static inline bool smm_points_to_smram(const void *ptr, const size_t len)
  *             handle sparse APIC id space.
  */
 struct smm_loader_params {
-	uintptr_t stack_top;
-	size_t per_cpu_stack_size;
 	size_t num_concurrent_stacks;
 
 	size_t real_cpu_save_state_size;
@@ -148,8 +143,10 @@ struct smm_loader_params {
 	struct smm_stub_params *stub_params;
 };
 
-/* Both of these return 0 on success, < 0 on failure. */
-int smm_setup_relocation_handler(const uintptr_t perm_smram, struct smm_loader_params *params);
+/* All of these return 0 on success, < 0 on failure. */
+int smm_setup_stack(const uintptr_t perm_smbase, const size_t perm_smram_size,
+		    const unsigned int total_cpus, const size_t stack_size);
+int smm_setup_relocation_handler(struct smm_loader_params *params);
 int smm_load_module(uintptr_t smram_base, size_t smram_size, struct smm_loader_params *params);
 
 u32 smm_get_cpu_smbase(unsigned int cpu_num);