Change in coreboot[master]: cpu/x86/smm/smihandler.c: Simplify smm revision handling

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45471 )

Change subject: cpu/x86/smm/smihandler.c: Simplify smm revision handling
......................................................................

cpu/x86/smm/smihandler.c: Simplify smm revision handling

The ASEG smihandler bails out if an unsupported SMM save state
revision is detected. Now we have code to find the SMM save state
depending on the SMM save state revision so reuse this to do the same.

This also increases the loglevel when bailing out of SMM due to
unsupported SMM save state revision from BIOS_DEBUG to BIOS_WARNING,
given that the system likely still boots but won't have a functioning
smihandler.

Change-Id: I57198f0c85c0f7a1fa363d3bd236c3d41b68d2f0
Signed-off-by: Arthur Heymans 
---
M src/cpu/x86/smm/smihandler.c
1 file changed, 3 insertions(+), 50 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/45471/1

diff --git a/src/cpu/x86/smm/smihandler.c b/src/cpu/x86/smm/smihandler.c
index 077fa8d..dea4cf3 100644
--- a/src/cpu/x86/smm/smihandler.c
+++ b/src/cpu/x86/smm/smihandler.c
@@ -14,23 +14,6 @@
 #include <spi-generic.h>
 #endif

-typedef enum {
-	AMD64,
-	EM64T100,
-	EM64T101,
-	LEGACY
-} save_state_type_t;
-
-typedef struct {
-	save_state_type_t type;
-	union {
-	amd64_smm_state_save_area_t *amd64_state_save;
-	em64t100_smm_state_save_area_t *em64t100_state_save;
-	em64t101_smm_state_save_area_t *em64t101_state_save;
-	legacy_smm_state_save_area_t *legacy_state_save;
-	};
-} smm_state_save_area_t;
-
 static int do_driver_init = 1;

typedef enum { SMI_LOCKED, SMI_UNLOCKED } smi_semaphore;
@@ -162,9 +145,6 @@
 void smi_handler(void)
 {
    unsigned int node;
-	const uint32_t smm_rev = smm_revision();
-	smm_state_save_area_t state_save;
-	u32 smm_base = SMM_BASE; /* ASEG */

/* Are we ok to execute the handler? */
    if (!smi_obtain_lock()) {
@@ -190,36 +170,9 @@

printk(BIOS_SPEW, "\nSMI# #%d\n", node);

-	switch (smm_rev) {
-	case 0x00030002:
-	case 0x00030007:
-		state_save.type = LEGACY;
-		state_save.legacy_state_save =
-			smm_save_state(smm_base,
-				       SMM_LEGACY_ARCH_OFFSET, node);
-		break;
-	case 0x00030100:
-		state_save.type = EM64T100;
-		state_save.em64t100_state_save =
-			smm_save_state(smm_base,
-				       SMM_EM64T100_ARCH_OFFSET, node);
-		break;
-	case 0x00030101: /* SandyBridge, IvyBridge, and Haswell */
-		state_save.type = EM64T101;
-		state_save.em64t101_state_save =
-			smm_save_state(smm_base,
-				       SMM_EM64T101_ARCH_OFFSET, node);
-		break;
-	case 0x00020064:
-	case 0x00030064:
-		state_save.type = AMD64;
-		state_save.amd64_state_save =
-			smm_save_state(smm_base,
-				       SMM_AMD64_ARCH_OFFSET, node);
-		break;
-	default:
-		printk(BIOS_DEBUG, "smm_revision: 0x%08x\n", smm_rev);
-		printk(BIOS_DEBUG, "SMI# not supported on your CPU\n");
+	if (smm_get_save_state(node) == NULL) {
+		printk(BIOS_WARNING, "smm_revision: 0x%08x\n", smm_revision());
+		printk(BIOS_WARNING, "SMI# not supported on your CPU\n");
    	/* Don't release lock, so no further SMI will happen,
    	 * if we don't handle it anyways.
    	 */

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45471 )

Change subject: cpu/x86/smm/smihandler.c: Simplify smm revision handling
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45471/1/src/cpu/x86/smm/smihandler.... 
File src/cpu/x86/smm/smihandler.c:

https://review.coreboot.org/c/coreboot/+/45471/1/src/cpu/x86/smm/smihandler.... 
PS1, Line 173: smm_get_save_state(node) == NULL
This check makes little sense...

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: cpu/x86/smm/smihandler.c: Simplify smm revision handling
......................................................................

cpu/x86/smm/smihandler.c: Simplify smm revision handling

The ASEG smihandler bails out if an unsupported SMM save state
revision is detected. Now we have code to find the SMM save state
depending on the SMM save state revision so reuse this to do the same.

This also increases the loglevel when bailing out of SMM due to
unsupported SMM save state revision from BIOS_DEBUG to BIOS_WARNING,
given that the system likely still boots but won't have a functioning
smihandler.

Change-Id: I57198f0c85c0f7a1fa363d3bd236c3d41b68d2f0
Signed-off-by: Arthur Heymans 
---
M src/cpu/x86/smm/smihandler.c
1 file changed, 4 insertions(+), 50 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/45471/2

[Angel Pons (Code Review)]

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

Change subject: cpu/x86/smm/smihandler.c: Simplify smm revision handling
......................................................................

Patch Set 2: Code-Review+1

[Angel Pons (Code Review)]

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

Change subject: cpu/x86/smm/smihandler.c: Simplify smm revision handling
......................................................................

Patch Set 4: Code-Review+2

One unresolved comment to go

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Angel Pons,

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

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

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

Change subject: cpu/x86/smm/smihandler.c: Simplify smm revision handling
......................................................................

cpu/x86/smm/smihandler.c: Simplify smm revision handling

The ASEG smihandler bails out if an unsupported SMM save state
revision is detected. Now we have code to find the SMM save state
depending on the SMM save state revision so reuse this to do the same.

This also increases the loglevel when bailing out of SMM due to
unsupported SMM save state revision from BIOS_DEBUG to BIOS_WARNING,
given that the system likely still boots but won't have a functioning
smihandler.

Change-Id: I57198f0c85c0f7a1fa363d3bd236c3d41b68d2f0
Signed-off-by: Arthur Heymans 
---
M src/cpu/x86/smm/smihandler.c
1 file changed, 4 insertions(+), 50 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/45471/5

[Patrick Georgi (Code Review)]

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

Change subject: cpu/x86/smm/smihandler.c: Simplify smm revision handling
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45471/1/src/cpu/x86/smm/smihandler.... 
File src/cpu/x86/smm/smihandler.c:

https://review.coreboot.org/c/coreboot/+/45471/1/src/cpu/x86/smm/smihandler.... 
PS1, Line 173: smm_get_save_state(node) == NULL
> This check makes little sense...

it looks a bit weird, but is it wrong?

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45471 )

Change subject: cpu/x86/smm/smihandler.c: Simplify smm revision handling
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45471/1/src/cpu/x86/smm/smihandler.... 
File src/cpu/x86/smm/smihandler.c:

https://review.coreboot.org/c/coreboot/+/45471/1/src/cpu/x86/smm/smihandler.... 
PS1, Line 173: smm_get_save_state(node) == NULL
> it looks a bit weird, but is it wrong?

Done

[Patrick Georgi (Code Review)]

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

Change subject: cpu/x86/smm/smihandler.c: Simplify smm revision handling
......................................................................

cpu/x86/smm/smihandler.c: Simplify smm revision handling

The ASEG smihandler bails out if an unsupported SMM save state
revision is detected. Now we have code to find the SMM save state
depending on the SMM save state revision so reuse this to do the same.

This also increases the loglevel when bailing out of SMM due to
unsupported SMM save state revision from BIOS_DEBUG to BIOS_WARNING,
given that the system likely still boots but won't have a functioning
smihandler.

Change-Id: I57198f0c85c0f7a1fa363d3bd236c3d41b68d2f0
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45471
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/cpu/x86/smm/smihandler.c
1 file changed, 4 insertions(+), 50 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/cpu/x86/smm/smihandler.c b/src/cpu/x86/smm/smihandler.c
index 077fa8d..0d9131e 100644
--- a/src/cpu/x86/smm/smihandler.c
+++ b/src/cpu/x86/smm/smihandler.c
@@ -14,23 +14,6 @@
 #include <spi-generic.h>
 #endif

-typedef enum {
-	AMD64,
-	EM64T100,
-	EM64T101,
-	LEGACY
-} save_state_type_t;
-
-typedef struct {
-	save_state_type_t type;
-	union {
-	amd64_smm_state_save_area_t *amd64_state_save;
-	em64t100_smm_state_save_area_t *em64t100_state_save;
-	em64t101_smm_state_save_area_t *em64t101_state_save;
-	legacy_smm_state_save_area_t *legacy_state_save;
-	};
-} smm_state_save_area_t;
-
 static int do_driver_init = 1;

typedef enum { SMI_LOCKED, SMI_UNLOCKED } smi_semaphore;
@@ -162,9 +145,6 @@
 void smi_handler(void)
 {
    unsigned int node;
-	const uint32_t smm_rev = smm_revision();
-	smm_state_save_area_t state_save;
-	u32 smm_base = SMM_BASE; /* ASEG */

/* Are we ok to execute the handler? */
    if (!smi_obtain_lock()) {
@@ -190,36 +170,10 @@

printk(BIOS_SPEW, "\nSMI# #%d\n", node);

-	switch (smm_rev) {
-	case 0x00030002:
-	case 0x00030007:
-		state_save.type = LEGACY;
-		state_save.legacy_state_save =
-			smm_save_state(smm_base,
-				       SMM_LEGACY_ARCH_OFFSET, node);
-		break;
-	case 0x00030100:
-		state_save.type = EM64T100;
-		state_save.em64t100_state_save =
-			smm_save_state(smm_base,
-				       SMM_EM64T100_ARCH_OFFSET, node);
-		break;
-	case 0x00030101: /* SandyBridge, IvyBridge, and Haswell */
-		state_save.type = EM64T101;
-		state_save.em64t101_state_save =
-			smm_save_state(smm_base,
-				       SMM_EM64T101_ARCH_OFFSET, node);
-		break;
-	case 0x00020064:
-	case 0x00030064:
-		state_save.type = AMD64;
-		state_save.amd64_state_save =
-			smm_save_state(smm_base,
-				       SMM_AMD64_ARCH_OFFSET, node);
-		break;
-	default:
-		printk(BIOS_DEBUG, "smm_revision: 0x%08x\n", smm_rev);
-		printk(BIOS_DEBUG, "SMI# not supported on your CPU\n");
+	/* Use smm_get_save_state() to see if the smm revision is supported */
+	if (smm_get_save_state(node) == NULL) {
+		printk(BIOS_WARNING, "smm_revision: 0x%08x\n", smm_revision());
+		printk(BIOS_WARNING, "SMI# not supported on your CPU\n");
    	/* Don't release lock, so no further SMI will happen,
    	 * if we don't handle it anyways.
    	 */