Change in ...coreboot[master]: soc/intel/apl: Call mca_configure() on cold boots only

[Nico Huber (Code Review)]

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31200


Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

soc/intel/apl: Call mca_configure() on cold boots only

By BIOS Spec we must not do this on warm boots. It also addresses half
of the TODO comment.

TEST=Confirmed that warm boots on Kontron/mAL10 don't hang any more.

Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Signed-off-by: Nico Huber <[e-mail]>
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 5 insertions(+), 4 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/31200/1

diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c
index bcec28e..c3b689a 100644
--- a/src/soc/intel/apollolake/cpu.c
+++ b/src/soc/intel/apollolake/cpu.c
@@ -17,6 +17,7 @@
  * GNU General Public License for more details.
  */
 
+#include <arch/acpi.h>
 #include <assert.h>
 #include <console/console.h>
 #include "chip.h"
@@ -70,10 +71,10 @@
 void soc_core_init(struct device *cpu)
 {
 	/* Clear out pending MCEs */
-	/* TODO(adurbin): This should only be done on a cold boot. Also, some
-	 * of these banks are core vs package scope. For now every CPU clears
-	 * every bank. */
-	mca_configure(NULL);
+	/* TODO(adurbin): Some of these banks are core vs package
+			  scope. For now every CPU clears every bank. */
+	if (acpi_get_sleep_type() != ACPI_S0)
+		mca_configure(NULL);
 
 	/* Set core MSRs */
 	reg_script_run(core_msr_script);

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-MessageType: newchange

[Nico Huber (Code Review)]

Hello Werner Zeh, Aaron Durbin, Patrick Rudolph, Subrata Banik, build bot (Jenkins), 

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

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

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

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

soc/intel/apl: Call mca_configure() on cold boots only

By APL BIOS Spec, we must not do this on warm boots. Also, an
x5-E3940 hangs in tests after touching these MSRs on a warm boot.

The TODO comment seems stale and copied over. So the actual
requirements for SGX are unknown and we add a guard for that
case.

TEST=Confirmed that warm boots on Kontron/mAL10 don't hang any more.

Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Signed-off-by: Nico Huber <[e-mail]>
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 6 insertions(+), 4 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/31200/2
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

[Aaron Durbin (Code Review)]

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

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................


Patch Set 2: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Fri, 01 Feb 2019 16:40:49 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................


Patch Set 2: Code-Review+1


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Fri, 01 Feb 2019 18:07:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Nico Huber (Code Review)]

Hello Werner Zeh, Aaron Durbin, Patrick Rudolph, Subrata Banik, Paul Menzel, build bot (Jenkins), 

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

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

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

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

soc/intel/apl: Call mca_configure() on cold boots only

By APL BIOS Spec, we must not do this on warm boots.

The TODO comment seems stale and copied over. So the actual
requirements for SGX are unknown and we add a guard for that
case.

Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Signed-off-by: Nico Huber <[e-mail]>
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 6 insertions(+), 4 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/31200/3
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 3
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

[Nico Huber (Code Review)]

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

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................


Patch Set 3:

My testing was flawed. When I tried to actually check
MCEs from the OS after a warm boot, it hanged in the OS.

Still, this change makes sense, separate from my problem.


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 3
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Fri, 01 Feb 2019 19:47:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

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

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................


Patch Set 3: Code-Review+1


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 3
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Sun, 03 Feb 2019 17:58:26 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Werner Zeh (Code Review)]

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

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c@76 
PS3, Line 76: IS_ENABLED(SOC_INTEL_COMMON_BLOCK_SGX
What is the dependency to SGX here? If I get it right we are dealing with the internal machine check logic here which is capable of reporting different kinds of internal system errors. It makes sense to clear this errors on every cold boot and not touch this registers on warm boot so that an OS based driver could report these errors. But why clearing them always if SGX is enabled?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 3
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Mon, 04 Feb 2019 06:05:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Nico Huber (Code Review)]

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

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................


Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c@76 
PS3, Line 76: IS_ENABLED(SOC_INTEL_COMMON_BLOCK_SGX
> What is the dependency to SGX here? If I get it right we are dealing with the internal machine check […]
> The call was added for SGX support in dc194e2bc4e. I don't
> know the details nor where to look them up. Will add Pratik,
> maybe he can tell us more.

[Pratikkumar V Prajapati (Code Review)]

Pratikkumar V Prajapati has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................


Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c@76 
PS3, Line 76: IS_ENABLED(SOC_INTEL_COMMON_BLOCK_SGX
> The call was added for SGX support in dc194e2bc4e. I don't […]
> SGX configuration needs microcode reload and for that we need to have MCA regs cleared otherwise SGX wont get enabled.

[Werner Zeh (Code Review)]

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

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................


Patch Set 3: Code-Review+2
> Patch Set 3:
>
> (1 comment)

Ah, thank you for the clarification.


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 3
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Pratikkumar V Prajapati <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Tue, 05 Feb 2019 07:50:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

soc/intel/apl: Call mca_configure() on cold boots only

By APL BIOS Spec, we must not do this on warm boots.

The TODO comment seems stale and copied over. So the actual
requirements for SGX are unknown and we add a guard for that
case.

Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Signed-off-by: Nico Huber <[e-mail]>
Reviewed-on: https://review.coreboot.org/c/31200
Reviewed-by: Angel Pons <[e-mail]>
Reviewed-by: Werner Zeh <[e-mail]>
Reviewed-by: Aaron Durbin <[e-mail]>
Reviewed-by: Paul Menzel <[e-mail]>
Tested-by: build bot (Jenkins) <[e-mail]>
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 6 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Aaron Durbin: Looks good to me, approved
  Werner Zeh: Looks good to me, approved
  Angel Pons: Looks good to me, but someone else must approve



diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c
index bcec28e..d1c5f6f 100644
--- a/src/soc/intel/apollolake/cpu.c
+++ b/src/soc/intel/apollolake/cpu.c
@@ -17,6 +17,7 @@
  * GNU General Public License for more details.
  */
 
+#include <arch/acpi.h>
 #include <assert.h>
 #include <console/console.h>
 #include "chip.h"
@@ -70,10 +71,11 @@
 void soc_core_init(struct device *cpu)
 {
 	/* Clear out pending MCEs */
-	/* TODO(adurbin): This should only be done on a cold boot. Also, some
-	 * of these banks are core vs package scope. For now every CPU clears
-	 * every bank. */
-	mca_configure(NULL);
+	/* TODO(adurbin): Some of these banks are core vs package
+			  scope. For now every CPU clears every bank. */
+	if (IS_ENABLED(SOC_INTEL_COMMON_BLOCK_SGX) ||
+	    acpi_get_sleep_type() == ACPI_S5)
+		mca_configure(NULL);
 
 	/* Set core MSRs */
 	reg_script_run(core_msr_script);

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/31200
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Gerrit-Change-Number: 31200
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Pratikkumar V Prajapati <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: merged