Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/58698 )
Change subject: cpu/x86/smm: Improve smm stack setup ......................................................................
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 --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/include/cpu/x86/mp.h M src/include/cpu/x86/smm.h 4 files changed, 47 insertions(+), 97 deletions(-)
Approvals: build bot (Jenkins): Verified HAOUAS Elyes: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index fee5940..7d0a20f 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -760,10 +760,9 @@ }
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 @@ .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 @@ * 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 @@ 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 ba179ca..ce7cf20 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 @@ 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) +static uintptr_t stack_top; +static size_t g_stack_size; + +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) { - size_t total_stack_size; - uintptr_t stacks_top; + /* 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; + }
- /* 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; - - /* Stacks extend down to SMBASE */ - stacks_top = base + total_stack_size; - printk(BIOS_DEBUG, "%s: exit, stack_top %lx\n", __func__, stacks_top); - - 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 @@ 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 @@ * 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 @@ 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 @@ 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 @@
/* 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 @@ * 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 @@
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 @@ 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 @@
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 @@ 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 934d217..1b4c956 100644 --- a/src/include/cpu/x86/mp.h +++ b/src/include/cpu/x86/mp.h @@ -45,17 +45,6 @@ */ 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 @@ * 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 848996f..37247ec 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -120,9 +120,6 @@ /* 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 @@ * 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_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);