Change in coreboot[master]: [UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info()
Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, cedarhouse1@comcast.net,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38769
to review the following change.
Change subject: [UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info() ......................................................................
[UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info()
Also keep related calculations local to STM code.
Change-Id: I3aa5915ddeffdabe792b9c30cd72cc39a8f54ca6 --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/security/intel/stm/SmmStm.h M src/security/intel/stm/StmPlatformSmm.c 4 files changed, 39 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/38769/1
diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 5b34d4b..45f7337 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -746,22 +746,8 @@ /* Setup code checks this callback for validity. */ mp_state.ops.relocation_handler(cpu, curr_smbase, perm_smbase);
- if (CONFIG(STM)) { - if (is_smm_enabled()) { - uintptr_t mseg; - - mseg = mp_state.perm_smbase + - (mp_state.perm_smsize - CONFIG_MSEG_SIZE); - - stm_setup(mseg, p->cpu, - perm_smbase, - mp_state.perm_smbase, - runtime->start32_offset); - } else { - printk(BIOS_DEBUG, - "STM not loaded because SMM is not enabled!\n"); - } - } + stm_setup(cpu, mp_state.perm_smbase, mp_state.perm_smsize, + perm_smbase, runtime->start32_offset); }
static void adjust_smm_apic_id_map(struct smm_loader_params *smm_params) diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index d4a384a..6482c35 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -17,7 +17,6 @@ #include <cpu/x86/cache.h> #include <commonlib/helpers.h> #include <console/console.h> -#include <security/intel/stm/SmmStm.h>
#define FXSAVE_SIZE 512
@@ -314,13 +313,10 @@ /* The SMM module is placed within the provided region in the following * manner: * +-----------------+ <- smram + size - * | BIOS resource | - * | list (STM) | - * +-----------------+ <- smram + size - CONFIG_BIOS_RESOURCE_LIST_SIZE * | stacks | - * +-----------------+ <- .. - total_stack_size + * +-----------------+ <- smram + size - total_stack_size * | fxsave area | - * +-----------------+ <- .. - total_stack_size - fxsave_size + * +-----------------+ <- smram + size - total_stack_size - fxsave_size * | ... | * +-----------------+ <- smram + handler_size + SMM_DEFAULT_SIZE * | handler | @@ -362,10 +358,6 @@ /* Stacks start at the top of the region. */ base = smram; base += size; - - if (CONFIG(STM)) - base -= CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE; - params->stack_top = base;
/* SMM module starts at offset SMM_DEFAULT_SIZE with the load alignment diff --git a/src/security/intel/stm/SmmStm.h b/src/security/intel/stm/SmmStm.h index f6ab0bf..58d74c0 100644 --- a/src/security/intel/stm/SmmStm.h +++ b/src/security/intel/stm/SmmStm.h @@ -30,13 +30,14 @@ #if CONFIG(STM) void stm_update_smm_info( uintptr_t *perm_smbase, size_t *perm_smsize, size_t *smm_save_state_size); +void stm_setup( + int cpu, uintptr_t perm_smbase, size_t perm_smsize, + uintptr_t staggered_smbase, size_t offset32); #else static inline void stm_update_smm_info(uintptr_t *, size_t *, size_t *) {} +static inline void stm_setup(int, uintptr_t, size_t, uintptr_t, size_t) {} #endif
-void stm_setup( - uintptr_t mseg, int cpu, uintptr_t smbase, - uintptr_t smbase_base, uint32_t offset32);
/* * Add resources in list to database. Allocate new memory areas as needed. diff --git a/src/security/intel/stm/StmPlatformSmm.c b/src/security/intel/stm/StmPlatformSmm.c index c389afa..c3c3ee8 100644 --- a/src/security/intel/stm/StmPlatformSmm.c +++ b/src/security/intel/stm/StmPlatformSmm.c @@ -154,10 +154,34 @@
static int stm_load_status = 0;
+/* + * We cut off some space from the top of SMRAM and also increase + * the per CPU save-state for the TXT SMM descriptor: + * +--------------------+ <- smram + size + * | MSEG | + * +--------------------+ <- smram + size - MSEG size + * | BIOS Resource List | + * +--------------------+ <- smram + size - MSEG size - BIOS Resource list size + * | SMM module | + * | ... | + * | +-------------+ | + * | + + | + * | + ... + | + * | +-------------+ | + * | + TXT Desc. + | + * | + CPU 1 save + | + * | +-------------+ | + * | + TXT Desc. + | + * | + CPU 0 save + | + * | +-------------+ | + * +--------------------+ <- smram + */ void stm_update_smm_info( uintptr_t *const perm_smbase, size_t *const perm_smsize, size_t *const smm_save_state_size) { + *perm_smsize -= CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE; + *smm_save_state_size += sizeof(TXT_PROCESSOR_SMM_DESCRIPTOR);
/* @@ -169,19 +193,22 @@ *smm_save_state_size = ALIGN_UP(*smm_save_state_size, 0x1000); }
-void stm_setup(uintptr_t mseg, int cpu, uintptr_t smbase, - uintptr_t base_smbase, uint32_t offset32) +void stm_setup( + const int cpu, const uintptr_t perm_smbase, const size_t perm_smsize, + const uintptr_t staggered_smbase, const size_t offset32) { msr_t InitMseg; msr_t MsegChk; uintptr_t addr_calc; // used to calculate the stm resource heap area
+ const uintptr_t mseg = perm_smbase + perm_smsize + CONFIG_BIOS_RESOURCE_LIST_SIZE; + printk(BIOS_DEBUG, "STM: set up for cpu %d\n", cpu); if (cpu == 0) {
// need to create the BIOS resource list once // first calculate the location in SMRAM - addr_calc = mseg - CONFIG_BIOS_RESOURCE_LIST_SIZE; + addr_calc = perm_smbase + perm_smsize; stm_resource_heap = (uint8_t *) addr_calc; printk(BIOS_DEBUG, "STM: stm_resource_heap located at %p\n", stm_resource_heap); @@ -204,7 +231,7 @@ cpu, MsegChk.hi, MsegChk.lo);
// setup the descriptor for this cpu - setup_smm_descriptor((void *)smbase, (void *) base_smbase, + setup_smm_descriptor((void *)staggered_smbase, (void *)perm_smbase, cpu, offset32); } else { printk(BIOS_DEBUG,
Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, cedarhouse1@comcast.net, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38769
to look at the new patch set (#2).
Change subject: [UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info() ......................................................................
[UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info()
Also keep related calculations local to STM code.
Change-Id: I3aa5915ddeffdabe792b9c30cd72cc39a8f54ca6 --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/security/intel/stm/SmmStm.h M src/security/intel/stm/StmPlatformSmm.c 4 files changed, 39 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/38769/2
Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, cedarhouse1@comcast.net, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38769
to look at the new patch set (#3).
Change subject: [UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info() ......................................................................
[UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info()
Also keep related calculations local to STM code.
Change-Id: I3aa5915ddeffdabe792b9c30cd72cc39a8f54ca6 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/security/intel/stm/SmmStm.h M src/security/intel/stm/StmPlatformSmm.c 4 files changed, 39 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/38769/3
Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, cedarhouse1@comcast.net, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38769
to look at the new patch set (#4).
Change subject: [UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info() ......................................................................
[UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info()
Also keep related calculations local to STM code.
Change-Id: I3aa5915ddeffdabe792b9c30cd72cc39a8f54ca6 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/security/intel/stm/SmmStm.h M src/security/intel/stm/StmPlatformSmm.c 4 files changed, 39 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/38769/4
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38769 )
Change subject: [UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info() ......................................................................
Patch Set 4:
Or should we make it an SMM_SUBREGION_*, in `tseg_region.c`?
cedarhouse1@comcast.net has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38769 )
Change subject: [UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info() ......................................................................
Patch Set 4:
Patch Set 4:
Or should we make it an SMM_SUBREGION_*, in `tseg_region.c`?
Doing this would allow for easier management of the TSEG region and letting the developer/user know when the TSEG is oversubscribed when adding the MSEG.
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38769?usp=email )
Change subject: [UNTESTED] intel/stm: Move SMRAM allocations into stm_update_smm_info() ......................................................................
Abandoned
-
Name of user not set (Code Review) -
Nico Huber (Code Review)
-
Stefan Reinauer (Code Review)