Change in coreboot[master]: [UNTESTED] intel/stm: Place resource list right below MSEG

[Nico Huber (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, [e-mail],

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/38765

to review the following change.

Change subject: [UNTESTED] intel/stm: Place resource list right below MSEG
......................................................................

[UNTESTED] intel/stm: Place resource list right below MSEG

Can we do this? what do I miss?

Change-Id: I9c5970433a3851b5c9662c8870efaf2943498d27
Signed-off-by: Nico Huber 
---
M src/cpu/x86/smm/smm_module_loader.c
M src/security/intel/stm/StmPlatformSmm.c
2 files changed, 8 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/38765/1

diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c
index 81020a4..2ef11340 100644
--- a/src/cpu/x86/smm/smm_module_loader.c
+++ b/src/cpu/x86/smm/smm_module_loader.c
@@ -315,13 +315,13 @@
 /* The SMM module is placed within the provided region in the following
  * manner:
  * +-----------------+ <- smram + size
- * |    stacks       |
- * +-----------------+ <- smram + size - total_stack_size
- * |  fxsave area    |
- * +-----------------+ <- smram + size - total_stack_size - fxsave_size
  * | BIOS resource   |
  * | list (STM)      |
- * +-----------------+ <- .. - CONFIG_BIOS_RESOURCE_LIST_SIZE
+ * +-----------------+ <- smram + size - CONFIG_BIOS_RESOURCE_LIST_SIZE
+ * |    stacks       |
+ * +-----------------+ <- .. - total_stack_size
+ * |  fxsave area    |
+ * +-----------------+ <- .. - total_stack_size - fxsave_size
  * |      ...        |
  * +-----------------+ <- smram + handler_size + SMM_DEFAULT_SIZE
  * |    handler      |
@@ -362,11 +362,10 @@

/* Stacks start at the top of the region. */
    base = smram;
+	base += size;

if (CONFIG(STM))
-		base += size - CONFIG_MSEG_SIZE;     // take out the mseg
-	else
-		base += size;
+		base -= CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE;

params->stack_top = base;

@@ -396,11 +395,6 @@
    /* Does the required amount of memory exceed the SMRAM region size? */
    total_size = total_stack_size + handler_size;
    total_size += fxsave_size + SMM_DEFAULT_SIZE;
-
-	// account for the bios resource list
-	if (CONFIG(STM))
-		total_size += CONFIG_BIOS_RESOURCE_LIST_SIZE;
-
    if (total_size > size)
    	return -1;

diff --git a/src/security/intel/stm/StmPlatformSmm.c b/src/security/intel/stm/StmPlatformSmm.c
index d7064b0..eb69301 100644
--- a/src/security/intel/stm/StmPlatformSmm.c
+++ b/src/security/intel/stm/StmPlatformSmm.c
@@ -166,12 +166,7 @@

// need to create the BIOS resource list once
    	// first calculate the location in SMRAM
-		addr_calc = (mseg - (CONFIG_SMM_MODULE_STACK_SIZE * num_cpus));
-
-		if (CONFIG(SSE))
-			addr_calc -= FXSAVE_SIZE * num_cpus;
-
-		addr_calc -= CONFIG_BIOS_RESOURCE_LIST_SIZE;
+		addr_calc = mseg - CONFIG_BIOS_RESOURCE_LIST_SIZE;
    	stm_resource_heap = (uint8_t *) addr_calc;
    	printk(BIOS_DEBUG, "STM: stm_resource_heap located at %p\n",
    			stm_resource_heap);

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38765 )

Change subject: [UNTESTED] intel/stm: Place resource list right below MSEG
......................................................................

Patch Set 1:

Eugene, I've put my thoughts into patches, probably less
ambiguous this way. This patch is the key to the series,
can we place the BIOS resource list like this?

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38765 )

Change subject: [UNTESTED] intel/stm: Place resource list right below MSEG
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Eugene, I've put my thoughts into patches, probably less
> ambiguous this way. This patch is the key to the series,
> can we place the BIOS resource list like this?

There should nothing preventing this.  However, I need to go through what you have done and test it.

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38765 )

Change subject: [UNTESTED] intel/stm: Place resource list right below MSEG
......................................................................

Patch Set 1: Code-Review+1

I've tested this patch with no problems.  The console log showed the expected placing of the BIOS resource list, the STM had no problems processing the list and when the SMI handler ran the change in the stack location had no apparent issues.
  This was tested on a Purism librem 15v4 (kabylake).

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38765 )

Change subject: [UNTESTED] intel/stm: Place resource list right below MSEG
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/38765/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38765/1//COMMIT_MSG@7 
PS1, Line 7: [UNTESTED] intel/stm: Place resource list right below MSEG
Might need an update

[S] Change in coreboot[master]: [UNTESTED] intel/stm: Place resource list right below MSEG

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38765?usp=email )

Change subject: [UNTESTED] intel/stm: Place resource list right below MSEG
......................................................................

Abandoned