Change in ...coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Name of user not set #1002358 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33985

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/1

diff --git a/src/include/cpu/x86/msr.h b/src/include/cpu/x86/msr.h
index d1e9169..c9d92a7 100644
--- a/src/include/cpu/x86/msr.h
+++ b/src/include/cpu/x86/msr.h
@@ -30,6 +30,7 @@
 #define IA32_BIOS_SIGN_ID		0x8b
 #define IA32_MPERF			0xe7
 #define IA32_APERF			0xe8
+/* STM */
 #define IA32_SMM_MONITOR_CTL_MSR        0x9B
 #define IA32_SMM_MONITOR_VALID          (1<<0)
 #define IA32_MCG_CAP			0x179
diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h
index b2d7445..3bd6e41 100644
--- a/src/include/cpu/x86/smm.h
+++ b/src/include/cpu/x86/smm.h
@@ -512,6 +512,7 @@
 struct smm_runtime {
    u32 smbase;
    u32 save_state_size;
+	/* useg to get the mseg address into smm for setup */
    u32 mseg;
    /* used so that the STM can start the SMI handler in 32bit mode */
    u32 start32_offset;

[Name of user not set (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/2

[Name of user not set (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/3

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Patrick Georgi (Code Review)]

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 4: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/33985/4/src/include/cpu/x86/smm.h 
File src/include/cpu/x86/smm.h:

https://review.coreboot.org/c/coreboot/+/33985/4/src/include/cpu/x86/smm.h@6... 
PS4, Line 67: useg
"used"? But I'd just say "Monitor Segment, when using STM" to explain what mseg is. That it's kept here for setup can be easily derived from the code.

https://review.coreboot.org/c/coreboot/+/33985/4/src/include/cpu/x86/smm.h@6... 
PS4, Line 69: 	/* used so that the STM can start the SMI handler in 32bit mode */
would rephrase as "STM's 32bit mode entry point into SMI handler" or something like that.

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Hello Aaron Durbin, ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 461 insertions(+), 29 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/5

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Hello Aaron Durbin, ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 461 insertions(+), 29 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/6

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Hello Aaron Durbin, ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 29 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/7

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Name of user not set #1002358 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/33985/4/src/include/cpu/x86/smm.h 
File src/include/cpu/x86/smm.h:

https://review.coreboot.org/c/coreboot/+/33985/4/src/include/cpu/x86/smm.h@6... 
PS4, Line 67: useg
> "used"? But I'd just say "Monitor Segment, when using STM" to explain what mseg is. […]

Variable has been removed in the next patch set

https://review.coreboot.org/c/coreboot/+/33985/4/src/include/cpu/x86/smm.h@6... 
PS4, Line 69: 	/* used so that the STM can start the SMI handler in 32bit mode */
> would rephrase as "STM's 32bit mode entry point into SMI handler" or something like that.

Rephrased as suggested

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Hello Aaron Durbin, ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 29 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/8

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Hello Aaron Durbin, ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 30 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/9

Change in coreboot[master]: include/cpu/x86: Add STM Support

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33985/9/src/include/cpu/x86/msr.h 
File src/include/cpu/x86/msr.h:

https://review.coreboot.org/c/coreboot/+/33985/9/src/include/cpu/x86/msr.h@3... 
PS9, Line 312: #ifdef  USEMSRSETBIT
exactly one space required after that #ifdef

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Hello Aaron Durbin, ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 30 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/10

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Hello Aaron Durbin, ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/11

Change in coreboot[master]: include/cpu/x86: Add STM Support

[ron minnich (Code Review)]

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 14:

(1 comment)

Can you address my question re the comment?

https://review.coreboot.org/c/coreboot/+/33985/14/src/include/cpu/x86/smm.h 
File src/include/cpu/x86/smm.h:

https://review.coreboot.org/c/coreboot/+/33985/14/src/include/cpu/x86/smm.h@... 
PS14, Line 515: 	/* useg to get the mseg address into smm for setup */
This one confuses me. Is the word supposed to be "used" instead of "useg"?

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Hello Aaron Durbin, ron minnich, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33985/15

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Name of user not set #1002358 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 15:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33985/14/src/include/cpu/x86/smm.h 
File src/include/cpu/x86/smm.h:

https://review.coreboot.org/c/coreboot/+/33985/14/src/include/cpu/x86/smm.h@... 
PS14, Line 515: 	/* useg to get the mseg address into smm for setup */
> This one confuses me. […]

Comment should have been deleted earlier - fixed in next patch release

Change in coreboot[master]: include/cpu/x86: Add STM Support

[ron minnich (Code Review)]

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 15: Code-Review+2

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 16: Code-Review+2

Change in coreboot[master]: include/cpu/x86: Add STM Support

[ron minnich (Code Review)]

ron minnich has submitted this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

include/cpu/x86: Add STM Support

Addtions to include/cpu/x86 include for STM support.

Change-Id: I2b8e68b2928aefc7996b6a9560c52f71c7c0e1d0
Signed-off-by: Eugene D. Myers 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33985
Tested-by: build bot (Jenkins) 
Reviewed-by: Martin Roth 
Reviewed-by: ron minnich 
---
M src/include/cpu/x86/msr.h
M src/include/cpu/x86/smm.h
2 files changed, 9 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  ron minnich: Looks good to me, approved
  Martin Roth: Looks good to me, approved

diff --git a/src/include/cpu/x86/msr.h b/src/include/cpu/x86/msr.h
index 2710e7f..0da8b56 100644
--- a/src/include/cpu/x86/msr.h
+++ b/src/include/cpu/x86/msr.h
@@ -30,6 +30,10 @@
 #define IA32_BIOS_SIGN_ID		0x8b
 #define IA32_MPERF			0xe7
 #define IA32_APERF			0xe8
+/* STM */
+#define IA32_SMM_MONITOR_CTL_MSR        0x9B
+#define SMBASE_RO_MSR			0x98
+#define IA32_SMM_MONITOR_VALID          (1<<0)
 #define IA32_MCG_CAP			0x179
 #define  MCG_CTL_P			(1 << 3)
 #define  MCA_BANKS_MASK			0xff
@@ -48,6 +52,8 @@
 #define IA32_PAT			0x277
 #define IA32_MC0_CTL			0x400
 #define IA32_MC0_STATUS			0x401
+#define IA32_VMX_BASIC_MSR              0x480
+#define IA32_VMX_MISC_MSR               0x485
 #define  MCA_STATUS_HI_VAL		(1UL << (63 - 32))
 #define  MCA_STATUS_HI_OVERFLOW		(1UL << (62 - 32))
 #define  MCA_STATUS_HI_UC		(1UL << (61 - 32))
diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h
index cf107b1..9efe2e0 100644
--- a/src/include/cpu/x86/smm.h
+++ b/src/include/cpu/x86/smm.h
@@ -64,6 +64,9 @@
 struct smm_runtime {
    u32 smbase;
    u32 save_state_size;
+	u32 num_cpus;
+	/* STM's 32bit entry into SMI handler */
+	u32 start32_offset;
    /* The apic_id_to_cpu provides a mapping from APIC id to CPU number.
     * The CPU number is indicated by the index into the array by matching
     * the default APIC id and value at the index. The stub loader

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 17:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/smm.h 
File src/include/cpu/x86/smm.h:

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/smm.h@... 
PS17, Line 69: 	u32 start32_offset;
We should keep the addition of fields with the rest of the patches that utilize them. As things currently stand it seems these are just unused and can cause confusion.

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 17:

This patch breaks our Apollo Lake boards in SMM relocation. Here is the part of the console log:
...
POST: 0x93
Setting up local APIC...
 apic_id: 0x00 done.
Detected 2 core, 2 thread CPU.
Will perform SMM setup.
FMAP: area COREBOOT found @ 305000 (11853824 bytes)
CBFS @ 305000 size b4e000
CBFS @ 305000 size b4e000
CBFS: 'IAFW Locator' located CBFS at [305000:e53000)
CBFS: Locating 'cpu_microcode_blob.bin'
CBFS: 'cpu_microcode_blob.bin' not found.
CPU: Intel(R) Atom(TM) Processor E3930 @ 1.30GHz.
Loading module at 0x00030000 with entry 0x00030000. filesize: 0x170 memsize: 0x170
Processing 16 relocs. Offset value of 0x00030000
Attempting to start 1 APs
Waiting for 10ms after sending INIT.
Waiting for 1st SIPI to complete...done.
AP: slot 1 apic_id 4.
Waiting for 2nd SIPI to complete...done.
Loading module at 0x00038000 with entry 0x00038000. filesize: 0x1a8 memsize: 0x1a8
Processing 13 relocs. Offset value of 0x00038000
SMM Module: stub loaded at 0x00038000. Will call 0x3ab55390(0x00000000)
Installing SMM handler to 0x3b000000
Loading module at 0x3b010000 with entry 0x3b01100a. filesize: 0x3fd0 memsize: 0x7ff0
Processing 377 relocs. Offset value of 0x3b010000
Loading module at 0x3b008000 with entry 0x3b008000. filesize: 0x1a8 memsize: 0x1a8
Processing 13 relocs. Offset value of 0x3b008000
SMM Module: placing jmp sequence at 0x3b007c00 rel16 0x03fd
SMM Module: stub loaded at 0x3b008000. Will call 0x3b01100a(0x00000000)
Clearing SMI status registers
PM1_STS: WAK 
TCO_STS: SECOND_TO <<-- Here the mainboards die!!!

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Angel Pons (Code Review)]

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

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 17:

(2 comments)

Was this change reviewed?

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/msr.h 
File src/include/cpu/x86/msr.h:

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/msr.h@... 
PS17, Line 36: (1<<0)
Really? This escaped review? Also, note that defines for bits within a register are indented with an extra space.

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/smm.h 
File src/include/cpu/x86/smm.h:

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/smm.h@... 
PS17, Line 69: 	u32 start32_offset;
> We should keep the addition of fields with the rest of the patches that utilize them. […]

They might also be breaking Werner's APL boards because of the increased size.

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 17:

CB:37809 fixes it.

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Aaron Durbin (Code Review)]

Aaron Durbin has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

Change in coreboot[master]: include/cpu/x86: Add STM Support

[Name of user not set (Code Review)]

Name of user not set #1002358 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33985 )

Change subject: include/cpu/x86: Add STM Support
......................................................................

Patch Set 17:

(2 comments)

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/msr.h 
File src/include/cpu/x86/msr.h:

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/msr.h@... 
PS17, Line 36: (1<<0)
> Really? This escaped review? Also, note that defines for bits within a register are indented with an […]

Corrected in #37821 patch set 2

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/smm.h 
File src/include/cpu/x86/smm.h:

https://review.coreboot.org/c/coreboot/+/33985/17/src/include/cpu/x86/smm.h@... 
PS17, Line 69: 	u32 start32_offset;
> They might also be breaking Werner's APL boards because of the increased size.

Ack