Change in coreboot[master]: cpu/x86: Put guard around align for smm_save_state_size

[Name of user not set (Code Review)]

[e-mail] has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38734 )

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

cpu/x86: Put guard around align for smm_save_state_size

The STM support aligns the smm_save_state_size.  However, this creates
issue for some platforms because of this value being hard coded to
0x400

Signed-off-by: Eugene D. Myers 
Change-Id: Ia584f7e9b86405a12eb6cbedc3a2615a8727f69e
---
M src/cpu/x86/mp_init.c
1 file changed, 3 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/38734/1

diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c
index 331f3b5..aba50bf 100644
--- a/src/cpu/x86/mp_init.c
+++ b/src/cpu/x86/mp_init.c
@@ -1053,8 +1053,9 @@
     * note: In the future, this will need to handle newer x86 processors
     * that require alignment of the save state on 32K boundaries.
     */
-	state->smm_save_state_size =
-		ALIGN_UP(state->smm_save_state_size, 0x1000);
+	if (CONFIG(STM))
+		state->smm_save_state_size =
+			ALIGN_UP(state->smm_save_state_size, 0x1000);

/*
     * Default to smm_initiate_relocation() if trigger callback isn't

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38734 )

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38734/1/src/cpu/x86/mp_init.c 
File src/cpu/x86/mp_init.c:

https://review.coreboot.org/c/coreboot/+/38734/1/src/cpu/x86/mp_init.c@1056 
PS1, Line 1056: 	if (CONFIG(STM))
this could be collapsed into the if() statement in line 1047. Also, since Patrick R. already mentioned it, maybe extend the comment to note that we have the hardcoded assumption of it being 0x400 across the tree?

[Name of user not set (Code Review)]

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

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 1:

Patch to fix the X11SSH-TF boot problem.

Issue is the hard code smm_save_state size, which I'll WRT the STM

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38734 )

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/38734/1/src/cpu/x86/mp_init.c 
File src/cpu/x86/mp_init.c:

https://review.coreboot.org/c/coreboot/+/38734/1/src/cpu/x86/mp_init.c@1056 
PS1, Line 1056: 	if (CONFIG(STM))
> this could be collapsed into the if() statement in line 1047. Also, since Patrick R. […]

Please also mark this code as broken. It works by luck on platforms with less than 5 cpu cores.

[Name of user not set (Code Review)]

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

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38734/1/src/cpu/x86/mp_init.c 
File src/cpu/x86/mp_init.c:

https://review.coreboot.org/c/coreboot/+/38734/1/src/cpu/x86/mp_init.c@1056 
PS1, Line 1056: 	if (CONFIG(STM))
> Please also mark this code as broken. It works by luck on platforms with less than 5 cpu cores.

Fixed and thanks for the point about 5 cores.  I will work that issue.

[Name of user not set (Code Review)]

Hello Patrick Rudolph, ron minnich, Stefan Reinauer, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

cpu/x86: Put guard around align for smm_save_state_size

The STM support aligns the smm_save_state_size.  However, this creates
issue for some platforms because of this value being hard coded to
0x400

Signed-off-by: Eugene D. Myers 
Change-Id: Ia584f7e9b86405a12eb6cbedc3a2615a8727f69e
---
M src/cpu/x86/mp_init.c
1 file changed, 13 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/38734/2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38734 )

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38734 )

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 2: Code-Review+2

[ron minnich (Code Review)]

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38734 )

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38734 )

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

cpu/x86: Put guard around align for smm_save_state_size

The STM support aligns the smm_save_state_size.  However, this creates
issue for some platforms because of this value being hard coded to
0x400

Signed-off-by: Eugene D. Myers 
Change-Id: Ia584f7e9b86405a12eb6cbedc3a2615a8727f69e
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38734
Reviewed-by: Patrick Rudolph 
Reviewed-by: Patrick Georgi 
Reviewed-by: ron minnich 
Tested-by: build bot (Jenkins) 
---
M src/cpu/x86/mp_init.c
1 file changed, 13 insertions(+), 8 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  ron minnich: Looks good to me, approved
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c
index 331f3b5..c747207 100644
--- a/src/cpu/x86/mp_init.c
+++ b/src/cpu/x86/mp_init.c
@@ -1044,17 +1044,22 @@
    /*
     * Make sure there is enough room for the SMM descriptor
     */
-	if (CONFIG(STM))
+	if (CONFIG(STM)) {
    	state->smm_save_state_size +=
    		sizeof(TXT_PROCESSOR_SMM_DESCRIPTOR);

-	/* Currently, the CPU SMM save state size is based on a simplistic
-	 * algorithm. (align on 4K)
-	 * note: In the future, this will need to handle newer x86 processors
-	 * that require alignment of the save state on 32K boundaries.
-	 */
-	state->smm_save_state_size =
-		ALIGN_UP(state->smm_save_state_size, 0x1000);
+		/* Currently, the CPU SMM save state size is based on a simplistic
+		 * algorithm. (align on 4K)
+		 * note: In the future, this will need to handle newer x86 processors
+		 * that require alignment of the save state on 32K boundaries.
+		 * The alignment is done here because coreboot has a hard coded
+		 * value of 0x400 for this value.
+		 * Also, this alignment only works on CPUs less than 5 threads
+		 */
+		if (CONFIG(STM))
+			state->smm_save_state_size =
+				ALIGN_UP(state->smm_save_state_size, 0x1000);
+	}

/*
     * Default to smm_initiate_relocation() if trigger callback isn't

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38734 )

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 3:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/477
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/476
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/475

Please note: This test is under development and might not be accurate at all!

[Nico Huber (Code Review)]

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

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38734/3/src/cpu/x86/mp_init.c 
File src/cpu/x86/mp_init.c:

https://review.coreboot.org/c/coreboot/+/38734/3/src/cpu/x86/mp_init.c@1056 
PS3, Line 1056: 		 * value of 0x400 for this value.
Where?

[Name of user not set (Code Review)]

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

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 3:

(1 comment)

responded to query about save state size being hard coded.

https://review.coreboot.org/c/coreboot/+/38734/3/src/cpu/x86/mp_init.c 
File src/cpu/x86/mp_init.c:

https://review.coreboot.org/c/coreboot/+/38734/3/src/cpu/x86/mp_init.c@1056 
PS3, Line 1056: 		 * value of 0x400 for this value.
> Where?

Patrick Rudolph noted in a previous comment:  "It looks like coreboot assumes 0x400 as safe value for save state. It's hard-coded everywhere in assembly."

For starters, I found this value (0x400) at the beginning of smmrelocate.S.  I will, over time, look further as the STM smm descriptor is at a defined location in the smm save state.

At the moment, the STM support code works on a Purism librem 15v4 which is a 4-core Kabylake. For other platforms and cores > 4, this is an issue that has to be worked out.

[Nico Huber (Code Review)]

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

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38734/3/src/cpu/x86/mp_init.c 
File src/cpu/x86/mp_init.c:

PS3: 
So is this an Intel-only tech? If so, the changes to this file should be
moved to the .get_smm_info and .relocation_handler callbacks.

https://review.coreboot.org/c/coreboot/+/38734/3/src/cpu/x86/mp_init.c@1056 
PS3, Line 1056: 		 * value of 0x400 for this value.
> Patrick Rudolph noted in a previous comment:  "It looks like coreboot assumes 0x400 as safe value fo […]

You have to be careful what code you read. smmhandler.S, smmhandler.c,
smmrelocate.S are for non-TSEG (very old) platforms. For TSEG platforms,
focus on mp_init.c, and smm/smm_*.

I don't think it's the alignment that breaks things, it's the absolute size.
Have a look at smm_module_setup_stub(). It uses SMM_DEFAULT_SIZE. I guess
that should be made a parameter. I would start by adding an overflow check
before line 205.

[Name of user not set (Code Review)]

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

Change subject: cpu/x86: Put guard around align for smm_save_state_size
......................................................................

Patch Set 3:

(2 comments)

responeded to Nico Huber's comments.

https://review.coreboot.org/c/coreboot/+/38734/3/src/cpu/x86/mp_init.c 
File src/cpu/x86/mp_init.c:

PS3:
> So is this an Intel-only tech? If so, the changes to this file should be […]

Yes, this is intel-only.

I see your point, but the change will involved multiple files (one for each family).  I can do that  as a part of the solution for the issue below.

https://review.coreboot.org/c/coreboot/+/38734/3/src/cpu/x86/mp_init.c@1056 
PS3, Line 1056: 		 * value of 0x400 for this value.
> You have to be careful what code you read. smmhandler.S, smmhandler.c, […]

Thanks for the opinion and suggestion.

I've been looking this morning's issue, in particular that >4 processors breaks this.  So, your suggestions are helpful.