Change in ...coreboot[master]: soc/intel/skylake: Remove redundent mca_configure() in ramstage

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32553

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

soc/intel/skylake: Remove redundent mca_configure() in ramstage

This patch removes redundent mca_configure() function call
from ramstage to clear machine check exception. First time its
getting called from soc_core_init() function inside cpu.c file.

Change-Id: I7e54fd07816c6317588ab6db06365937c4300ccd
Signed-off-by: Subrata Banik 
---
M src/soc/intel/skylake/finalize.c
1 file changed, 0 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32553/1

diff --git a/src/soc/intel/skylake/finalize.c b/src/soc/intel/skylake/finalize.c
index 5d7e1e0..34738f2 100644
--- a/src/soc/intel/skylake/finalize.c
+++ b/src/soc/intel/skylake/finalize.c
@@ -20,7 +20,6 @@
 #include <bootstate.h>
 #include <console/console.h>
 #include <console/post_codes.h>
-#include <cpu/x86/mp.h>
 #include <cpu/x86/smm.h>
 #include <device/pci.h>
 #include <intelblocks/cpulib.h>
@@ -142,9 +141,6 @@

pch_finalize_script(dev);

-	printk(BIOS_DEBUG, "Clearing MCA.\n");
-	mp_run_on_all_cpus(mca_configure, NULL, 17 * USECS_PER_SEC);
-
    soc_lockdown(dev);

printk(BIOS_DEBUG, "Finalizing SMM.\n");

[Lijian Zhao (Code Review)]

Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 1: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32553/1/src/soc/intel/skylake/finalize.c 
File src/soc/intel/skylake/finalize.c:

https://review.coreboot.org/#/c/32553/1/src/soc/intel/skylake/finalize.c@a14... 
PS1, Line 146: 
Do we know why this change was added here? It seems like we should revert both these CLs:

https://review.coreboot.org/c/coreboot/+/26392/
https://review.coreboot.org/c/coreboot/+/26391/

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32553/1/src/soc/intel/skylake/finalize.c 
File src/soc/intel/skylake/finalize.c:

https://review.coreboot.org/#/c/32553/1/src/soc/intel/skylake/finalize.c@a14... 
PS1, Line 146:
> Do we know why this change was added here? It seems like we should revert both these CLs: […]

i don't see any crossbug and any details about failure in commit msg, i have added Pratik to share his view.

but, you point is valid that if we decide to revert then we should revert those 2 in series.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32553/1/src/soc/intel/skylake/finalize.c 
File src/soc/intel/skylake/finalize.c:

https://review.coreboot.org/#/c/32553/1/src/soc/intel/skylake/finalize.c@a14... 
PS1, Line 146:
> i don't see any crossbug and any details about failure in commit msg, i have added Pratik to share h […]

Let's wait to hear back from Pratik before merging this change.

[Pratikkumar V Prajapati (Code Review)]

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

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Patch Set 2:
>
> (1 comment)

On AML SOCs (for Nocturne), we have seen MCEs being dumped after booting to kernel. After debugging i found MCAs were not cleared before booting the kernel so i pushed those 2 patches and with that kernel was not reporting MCEs.

(I am trying to find the crosbug but i am not able to access them, seems like i have some access issues)

@Subrata, Have you checked on SKL/KBL/AML platforms that MCEs are not dumped if you dont clear them before booting?

Clearing from cpu.c might be too early, i think. in my understanding, Clearing MCEs at that point is needed only if we need to enable SGX, as when we reload microcode for SGX we need have MCA clean.

[Pratikkumar V Prajapati (Code Review)]

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

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> >
> > (1 comment)
>
> On AML SOCs (for Nocturne), we have seen MCEs being dumped after booting to kernel. After debugging i found MCAs were not cleared before booting the kernel so i pushed those 2 patches and with that kernel was not reporting MCEs.
>
> (I am trying to find the crosbug but i am not able to access them, seems like i have some access issues)
>
> @Subrata, Have you checked on SKL/KBL/AML platforms that MCEs are not dumped if you dont clear them before booting?
>
> Clearing from cpu.c might be too early, i think. in my understanding, Clearing MCEs at that point is needed only if we need to enable SGX, as when we reload microcode for SGX we need have MCA clean.

https://partnerissuetracker.corp.google.com/issues/79783714 is the tracker.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > >
> > > (1 comment)
> >
> > On AML SOCs (for Nocturne), we have seen MCEs being dumped after booting to kernel. After debugging i found MCAs were not cleared before booting the kernel so i pushed those 2 patches and with that kernel was not reporting MCEs.
> >
> > (I am trying to find the crosbug but i am not able to access them, seems like i have some access issues)
> >
> > @Subrata, Have you checked on SKL/KBL/AML platforms that MCEs are not dumped if you dont clear them before booting?
> >
> > Clearing from cpu.c might be too early, i think. in my understanding, Clearing MCEs at that point is needed only if we need to enable SGX, as when we reload microcode for SGX we need have MCA clean.
>
> https://partnerissuetracker.corp.google.com/issues/79783714 is the tracker.

I don't see a lot of details on the bug as to why the mca_configure was required. Also, if MCEs were cleared early, are you saying there were more errors that happened after that?

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG@7 
PS2, Line 7: redundent
redundant

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG@9 
PS2, Line 9: redundent
redundant

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG@10 
PS2, Line 10: its
it’s

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG@12 
PS2, Line 12: 
Tested how? Does it reduce the boot time noticeably?

[Pratikkumar V Prajapati (Code Review)]

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

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 2:
> > > >
> > > > (1 comment)
> > >
> > > On AML SOCs (for Nocturne), we have seen MCEs being dumped after booting to kernel. After debugging i found MCAs were not cleared before booting the kernel so i pushed those 2 patches and with that kernel was not reporting MCEs.
> > >
> > > (I am trying to find the crosbug but i am not able to access them, seems like i have some access issues)
> > >
> > > @Subrata, Have you checked on SKL/KBL/AML platforms that MCEs are not dumped if you dont clear them before booting?
> > >
> > > Clearing from cpu.c might be too early, i think. in my understanding, Clearing MCEs at that point is needed only if we need to enable SGX, as when we reload microcode for SGX we need have MCA clean.
> >
> > https://partnerissuetracker.corp.google.com/issues/79783714 is the tracker.
>
> I don't see a lot of details on the bug as to why the mca_configure was required. Also, if MCEs were cleared early, are you saying there were more errors that happened after that?

there were MCEs before giving control to Depthcharge

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 2:
> > > > > Patch Set 2:
> > > > >
> > > > > (1 comment)
> > > >
> > > > On AML SOCs (for Nocturne), we have seen MCEs being dumped
>
> after booting to kernel. After debugging i found MCAs were not
> cleared before booting the kernel so i pushed those 2 patches and
> with that kernel was not reporting MCEs.
> > > > (I am trying to find the crosbug but i am not able to access
>
> them, seems like i have some access issues)
> > > > @Subrata, Have you checked on SKL/KBL/AML platforms that MCEs
>
> are not dumped if you dont clear them before booting?
> > > > Clearing from cpu.c might be too early, i think. in my
>
> understanding, Clearing MCEs at that point is needed only if we
> need to enable SGX, as when we reload microcode for SGX we need
> have MCA clean.
> > > https://partnerissuetracker.corp.google.com/issues/79783714 is
>
> the tracker.
> > I don't see a lot of details on the bug as to why the
>
> mca_configure was required. Also, if MCEs were cleared early, are
> you saying there were more errors that happened after that?
>
> there were MCEs before giving control to Depthcharge

i can take AR to verify on SKL/KBL/AML

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 2:
> > > > > Patch Set 2:
> > > > > > Patch Set 2:
> > > > > >
> > > > > > (1 comment)
> > > > >
> > > > > On AML SOCs (for Nocturne), we have seen MCEs being dumped
> >
> > after booting to kernel. After debugging i found MCAs were not
> > cleared before booting the kernel so i pushed those 2 patches and
> > with that kernel was not reporting MCEs.
> > > > > (I am trying to find the crosbug but i am not able to
>
> access
> > them, seems like i have some access issues)
> > > > > @Subrata, Have you checked on SKL/KBL/AML platforms that
>
> MCEs
> > are not dumped if you dont clear them before booting?
> > > > > Clearing from cpu.c might be too early, i think. in my
> >
> > understanding, Clearing MCEs at that point is needed only if we
> > need to enable SGX, as when we reload microcode for SGX we need
> > have MCA clean.
> > > > https://partnerissuetracker.corp.google.com/issues/79783714
>
> is
> > the tracker.
> > > I don't see a lot of details on the bug as to why the
> >
> > mca_configure was required. Also, if MCEs were cleared early, are
> > you saying there were more errors that happened after that?
> >
> > there were MCEs before giving control to Depthcharge
>
> i can take AR to verify on SKL/KBL/AML

i have tested on KBL (eve) and AML (nocturne) platform and i don't see any machine check issue.

[Pratikkumar V Prajapati (Code Review)]

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

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Patch Set 2:
> > > > Patch Set 2:
> > > > > Patch Set 2:
> > > > > > Patch Set 2:
> > > > > >
> > > > > > > Patch Set 2:
> > > > > > >
> > > > > > > (1 comment)
> > > > > >
> > > > > > On AML SOCs (for Nocturne), we have seen MCEs being dumped
> > >
> > > after booting to kernel. After debugging i found MCAs were not
> > > cleared before booting the kernel so i pushed those 2 patches and
> > > with that kernel was not reporting MCEs.
> > > > > > (I am trying to find the crosbug but i am not able to
> >
> > access
> > > them, seems like i have some access issues)
> > > > > > @Subrata, Have you checked on SKL/KBL/AML platforms that
> >
> > MCEs
> > > are not dumped if you dont clear them before booting?
> > > > > > Clearing from cpu.c might be too early, i think. in my
> > >
> > > understanding, Clearing MCEs at that point is needed only if we
> > > need to enable SGX, as when we reload microcode for SGX we need
> > > have MCA clean.
> > > > > https://partnerissuetracker.corp.google.com/issues/79783714
> >
> > is
> > > the tracker.
> > > > I don't see a lot of details on the bug as to why the
> > >
> > > mca_configure was required. Also, if MCEs were cleared early, are
> > > you saying there were more errors that happened after that?
> > >
> > > there were MCEs before giving control to Depthcharge
> >
> > i can take AR to verify on SKL/KBL/AML
>
> i have tested on KBL (eve) and AML (nocturne) platform and i don't see any machine check issue.

I hope tested the AML sku mentioned in the tracker. MCEs were reported by Google and after patched Google verified as well and that's why the patches were +2'd and merged.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> > Patch Set 2:
> > > > > Patch Set 2:
> > > > > > Patch Set 2:
> > > > > >
> > > > > > > Patch Set 2:
> > > > > > >
> > > > > > > > Patch Set 2:
> > > > > > > >
> > > > > > > > (1 comment)
> > > > > > >
> > > > > > >
> > > > > > > On AML SOCs (for Nocturne), we have seen MCEs being
>
> dumped
> > > > after booting to kernel. After debugging i found MCAs were
>
> not
> > > > cleared before booting the kernel so i pushed those 2
>
> patches and
> > > > with that kernel was not reporting MCEs.
> > > > > > >
> > > > > > > (I am trying to find the crosbug but i am not able to
> > >
> > > access
> > > > them, seems like i have some access issues)
> > > > > > >
> > > > > > > @Subrata, Have you checked on SKL/KBL/AML platforms
>
> that
> > > MCEs
> > > > are not dumped if you dont clear them before booting?
> > > > > > >
> > > > > > > Clearing from cpu.c might be too early, i think. in my
> > > >
> > > > understanding, Clearing MCEs at that point is needed only if
>
> we
> > > > need to enable SGX, as when we reload microcode for SGX we
>
> need
> > > > have MCA clean.
> > > > > > https://partnerissuetracker.corp.google.com/issues/79783714
> > >
> > > is
> > > > the tracker.
> > > > > I don't see a lot of details on the bug as to why the
> > > >
> > > > mca_configure was required. Also, if MCEs were cleared
>
> early, are
> > > > you saying there were more errors that happened after that?
> > > >
> > > > there were MCEs before giving control to Depthcharge
> > >
> > > i can take AR to verify on SKL/KBL/AML
> >
> > i have tested on KBL (eve) and AML (nocturne) platform and i
>
> don't see any machine check issue.
>
> I hope tested the AML sku mentioned in the tracker. MCEs were
> reported by Google and after patched Google verified as well and
> that's why the patches were +2'd and merged.

Hi Furquan, Do you have some time to check this CL on your side AML platform ? as Pratik suggested in past some one from Google has verified the need to mca_configue(NULL) ?

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Patch Set 2:
> > > Patch Set 2:
> > > > > > Patch Set 2:
> > > > > >
> > > > > > > Patch Set 2:
> > > > > > >
> > > > > > > > Patch Set 2:
> > > > > > > >
> > > > > > > > > Patch Set 2:
> > > > > > > > >
> > > > > > > > > (1 comment)
> > > > > > > >
> > > > > > > >
> > > > > > > > On AML SOCs (for Nocturne), we have seen MCEs being
> >
> > dumped
> > > > > after booting to kernel. After debugging i found MCAs were
> >
> > not
> > > > > cleared before booting the kernel so i pushed those 2
> >
> > patches and
> > > > > with that kernel was not reporting MCEs.
> > > > > > > >
> > > > > > > > (I am trying to find the crosbug but i am not able to
> > > >
> > > > access
> > > > > them, seems like i have some access issues)
> > > > > > > >
> > > > > > > > @Subrata, Have you checked on SKL/KBL/AML platforms
> >
> > that
> > > > MCEs
> > > > > are not dumped if you dont clear them before booting?
> > > > > > > >
> > > > > > > > Clearing from cpu.c might be too early, i think. in my
> > > > >
> > > > > understanding, Clearing MCEs at that point is needed only if
> >
> > we
> > > > > need to enable SGX, as when we reload microcode for SGX we
> >
> > need
> > > > > have MCA clean.
> > > > > > >
> > > > > > > https://partnerissuetracker.corp.google.com/issues/79783714
> > > >
> > > > is
> > > > > the tracker.
> > > > > > I don't see a lot of details on the bug as to why the
> > > > >
> > > > > mca_configure was required. Also, if MCEs were cleared
> >
> > early, are
> > > > > you saying there were more errors that happened after that?
> > > > >
> > > > > there were MCEs before giving control to Depthcharge
> > > >
> > > > i can take AR to verify on SKL/KBL/AML
> > >
> > > i have tested on KBL (eve) and AML (nocturne) platform and i
> >
> > don't see any machine check issue.
> >
> > I hope tested the AML sku mentioned in the tracker. MCEs were
> > reported by Google and after patched Google verified as well and
> > that's why the patches were +2'd and merged.
>
> Hi Furquan, Do you have some time to check this CL on your side AML platform ? as Pratik suggested in past some one from Google has verified the need to mca_configue(NULL) ?

+Nick and +Caveh to help test this.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> > Patch Set 2:
> > > > Patch Set 2:
> > > > > > > Patch Set 2:
> > > > > > >
> > > > > > > > Patch Set 2:
> > > > > > > >
> > > > > > > > > Patch Set 2:
> > > > > > > > >
> > > > > > > > > > Patch Set 2:
> > > > > > > > > >
> > > > > > > > > > (1 comment)
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On AML SOCs (for Nocturne), we have seen MCEs
>
> being
> > > dumped
> > > > > > after booting to kernel. After debugging i found MCAs
>
> were
> > > not
> > > > > > cleared before booting the kernel so i pushed those 2
> > >
> > > patches and
> > > > > > with that kernel was not reporting MCEs.
> > > > > > > > >
> > > > > > > > > (I am trying to find the crosbug but i am not
>
> able to
> > > > > access
> > > > > > them, seems like i have some access issues)
> > > > > > > > >
> > > > > > > > > @Subrata, Have you checked on SKL/KBL/AML
>
> platforms
> > > that
> > > > > MCEs
> > > > > > are not dumped if you dont clear them before booting?
> > > > > > > > >
> > > > > > > > > Clearing from cpu.c might be too early, i think.
>
> in my
> > > > > > understanding, Clearing MCEs at that point is needed
>
> only if
> > > we
> > > > > > need to enable SGX, as when we reload microcode for SGX
>
> we
> > > need
> > > > > > have MCA clean.
> > > > > > > >
> > > > > > > > https://partnerissuetracker.corp.google.com/issues/79783714
> > > > >
> > > > > is
> > > > > > the tracker.
> > > > > > >
> > > > > > > I don't see a lot of details on the bug as to why the
> > > > > > mca_configure was required. Also, if MCEs were cleared
> > >
> > > early, are
> > > > > > you saying there were more errors that happened after
>
> that?
> > > > > > there were MCEs before giving control to Depthcharge
> > > > >
> > > > > i can take AR to verify on SKL/KBL/AML
> > > >
> > > > i have tested on KBL (eve) and AML (nocturne) platform and i
> > >
> > > don't see any machine check issue.
> > >
> > > I hope tested the AML sku mentioned in the tracker. MCEs were
> > > reported by Google and after patched Google verified as well
>
> and
> > > that's why the patches were +2'd and merged.
> >
> > Hi Furquan, Do you have some time to check this CL on your side
>
> AML platform ? as Pratik suggested in past some one from Google has
> verified the need to mca_configue(NULL) ?
>
> +Nick and +Caveh to help test this.

Thanks, we will wait for the feedback.

[Pratikkumar V Prajapati (Code Review)]

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

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > > Patch Set 2:
> > > > > > > Patch Set 2:
> > > > > > >
> > > > > > > > Patch Set 2:
> > > > > > > >
> > > > > > > > > Patch Set 2:
> > > > > > > > >
> > > > > > > > > > Patch Set 2:
> > > > > > > > > >
> > > > > > > > > > (1 comment)
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On AML SOCs (for Nocturne), we have seen MCEs being
> > >
> > > dumped
> > > > > > after booting to kernel. After debugging i found MCAs were
> > >
> > > not
> > > > > > cleared before booting the kernel so i pushed those 2
> > >
> > > patches and
> > > > > > with that kernel was not reporting MCEs.
> > > > > > > > >
> > > > > > > > > (I am trying to find the crosbug but i am not able to
> > > > >
> > > > > access
> > > > > > them, seems like i have some access issues)
> > > > > > > > >
> > > > > > > > > @Subrata, Have you checked on SKL/KBL/AML platforms
> > >
> > > that
> > > > > MCEs
> > > > > > are not dumped if you dont clear them before booting?
> > > > > > > > >
> > > > > > > > > Clearing from cpu.c might be too early, i think. in my
> > > > > > understanding, Clearing MCEs at that point is needed only if
> > >
> > > we
> > > > > > need to enable SGX, as when we reload microcode for SGX we
> > >
> > > need
> > > > > > have MCA clean.
> > > > > > > >
> > > > > > > > https://partnerissuetracker.corp.google.com/issues/79783714
> > > > >
> > > > > is
> > > > > > the tracker.
> > > > > > >
> > > > > > > I don't see a lot of details on the bug as to why the
> > > > > > mca_configure was required. Also, if MCEs were cleared
> > >
> > > early, are
> > > > > > you saying there were more errors that happened after that?
> > > > > >
> > > > > > there were MCEs before giving control to Depthcharge
> > > > >
> > > > > i can take AR to verify on SKL/KBL/AML
> > > >
> > > > i have tested on KBL (eve) and AML (nocturne) platform and i
> > >
> > > don't see any machine check issue.
> > >
> > > I hope tested the AML sku mentioned in the tracker. MCEs were
> > > reported by Google and after patched Google verified as well and
> > > that's why the patches were +2'd and merged.
> >
> > Hi Furquan, Do you have some time to check this CL on your side AML platform ? as Pratik suggested in past some one from Google has verified the need to mca_configue(NULL) ?
>
> +Nick and +Caveh to help test this.

BTW, how much time are we saving after removing mca_configure from finalize.c? (what could be any other reason to remove from  finalize.c?)
What if we remove mca_config from cpu.c and keep in finalize.c?

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > > > > Patch Set 2:
> > > > > > > > Patch Set 2:
> > > > > > > >
> > > > > > > > > Patch Set 2:
> > > > > > > > >
> > > > > > > > > > Patch Set 2:
> > > > > > > > > >
> > > > > > > > > > > Patch Set 2:
> > > > > > > > > > >
> > > > > > > > > > > (1 comment)
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On AML SOCs (for Nocturne), we have seen MCEs
>
> being
> > > > dumped
> > > > > > > after booting to kernel. After debugging i found MCAs
>
> were
> > > > not
> > > > > > > cleared before booting the kernel so i pushed those 2
> > > >
> > > > patches and
> > > > > > > with that kernel was not reporting MCEs.
> > > > > > > > > >
> > > > > > > > > > (I am trying to find the crosbug but i am not
>
> able to
> > > > > > access
> > > > > > > them, seems like i have some access issues)
> > > > > > > > > >
> > > > > > > > > > @Subrata, Have you checked on SKL/KBL/AML
>
> platforms
> > > > that
> > > > > > MCEs
> > > > > > > are not dumped if you dont clear them before booting?
> > > > > > > > > >
> > > > > > > > > > Clearing from cpu.c might be too early, i
>
> think. in my
> > > > > > > understanding, Clearing MCEs at that point is needed
>
> only if
> > > > we
> > > > > > > need to enable SGX, as when we reload microcode for
>
> SGX we
> > > > need
> > > > > > > have MCA clean.
> > > > > > > > >
> > > > > > > > > https://partnerissuetracker.corp.google.com/issues/79783714
> > > > > > is
> > > > > > > the tracker.
> > > > > > > >
> > > > > > > > I don't see a lot of details on the bug as to why
>
> the
> > > > > > > mca_configure was required. Also, if MCEs were
>
> cleared
> > > > early, are
> > > > > > > you saying there were more errors that happened after
>
> that?
> > > > > > >
> > > > > > > there were MCEs before giving control to Depthcharge
> > > > > >
> > > > > > i can take AR to verify on SKL/KBL/AML
> > > > >
> > > > > i have tested on KBL (eve) and AML (nocturne) platform and
>
> i
> > > > don't see any machine check issue.
> > > >
> > > > I hope tested the AML sku mentioned in the tracker. MCEs
>
> were
> > > > reported by Google and after patched Google verified as well
>
> and
> > > > that's why the patches were +2'd and merged.
> > >
> > > Hi Furquan, Do you have some time to check this CL on your side
>
> AML platform ? as Pratik suggested in past some one from Google has
> verified the need to mca_configue(NULL) ?
> > +Nick and +Caveh to help test this.
>
> BTW, how much time are we saving after removing mca_configure from
> finalize.c? (what could be any other reason to remove from 
> finalize.c?)
> What if we remove mca_config from cpu.c and keep in finalize.c?

yes, this can be done, but we would like to combine all CPU related operation bind with either pre/post/mp init flow.

This effort might help to reduce overhead while we decide to skip certain stages in future like RAMPAYLOAD approach.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:

Hi Nick/Caveh,
Do you get some time to verify this CL on AML ?

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Hi Nick/Caveh,
> Do you get some time to verify this CL on AML ?

Hi,
I don't have time today as I need to wrap up some work before sheriff duty next week, but will try to look at it early next week.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> > Patch Set 2:
> >
> > Hi Nick/Caveh,
> > Do you get some time to verify this CL on AML ?
>
> Hi,
> I don't have time today as I need to wrap up some work before
> sheriff duty next week, but will try to look at it early next week.

thanks nick.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> > > Patch Set 2:
> > >
> > > Hi Nick/Caveh,
> > > Do you get some time to verify this CL on AML ?
> >
> > Hi,
> > I don't have time today as I need to wrap up some work before
> > sheriff duty next week, but will try to look at it early next
>
> week.
>
> thanks nick.

HI Nick,
Please let me know your observation.

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2: Code-Review+1

Boots on nocturne.

[Pratikkumar V Prajapati (Code Review)]

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

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+1
>
> Boots on nocturne.

Any observation for Machine check errors?

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:

Searching dmesg for "achine check" and for "mce" did not show any error logs.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:
> Searching dmesg for "achine check" and for "mce" did not show any
> error logs.

thanks Nick, can we merge this CL now ?

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/skylake: Remove redundent mca_configure() in ramstage
......................................................................

Patch Set 2:

Please address my comments.

Change in ...coreboot[master]: soc/intel/skylake: Remove redundant mca_configure() in ramstage

[Subrata Banik (Code Review)]

Hello Pratikkumar V Prajapati, Patrick Rudolph, Nick Vaccaro, Aamir Bohra, caveh jalali, Rizwan Qureshi, build bot (Jenkins), Lijian Zhao, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/skylake: Remove redundant mca_configure() in ramstage
......................................................................

soc/intel/skylake: Remove redundant mca_configure() in ramstage

This patch removes redundant mca_configure() function call
from ramstage to clear machine check exception. First time it's
getting called from soc_core_init() function inside cpu.c file.

TEST=Build and boot SKL/KBL/AML platform without any machine-check
exception.

Change-Id: I7e54fd07816c6317588ab6db06365937c4300ccd
Signed-off-by: Subrata Banik 
---
M src/soc/intel/skylake/finalize.c
1 file changed, 0 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32553/3

Change in ...coreboot[master]: soc/intel/skylake: Remove redundant mca_configure() in ramstage

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundant mca_configure() in ramstage
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG@7 
PS2, Line 7: redundent
> redundant

Done

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG@9 
PS2, Line 9: redundent
> redundant

Done

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG@10 
PS2, Line 10: its
> it’s

Done

https://review.coreboot.org/#/c/32553/2//COMMIT_MSG@12 
PS2, Line 12:
> Tested how? Does it reduce the boot time noticeably?

Done

Change in ...coreboot[master]: soc/intel/skylake: Remove redundant mca_configure() in ramstage

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundant mca_configure() in ramstage
......................................................................

Patch Set 3:

some need to give +2 here

Change in ...coreboot[master]: soc/intel/skylake: Remove redundant mca_configure() in ramstage

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundant mca_configure() in ramstage
......................................................................

Patch Set 3: Code-Review+2

Change in ...coreboot[master]: soc/intel/skylake: Remove redundant mca_configure() in ramstage

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32553 )

Change subject: soc/intel/skylake: Remove redundant mca_configure() in ramstage
......................................................................

soc/intel/skylake: Remove redundant mca_configure() in ramstage

This patch removes redundant mca_configure() function call
from ramstage to clear machine check exception. First time it's
getting called from soc_core_init() function inside cpu.c file.

TEST=Build and boot SKL/KBL/AML platform without any machine-check
exception.

Change-Id: I7e54fd07816c6317588ab6db06365937c4300ccd
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32553
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Nick Vaccaro 
---
M src/soc/intel/skylake/finalize.c
1 file changed, 0 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Nick Vaccaro: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/skylake/finalize.c b/src/soc/intel/skylake/finalize.c
index 5d7e1e0..34738f2 100644
--- a/src/soc/intel/skylake/finalize.c
+++ b/src/soc/intel/skylake/finalize.c
@@ -20,7 +20,6 @@
 #include <bootstate.h>
 #include <console/console.h>
 #include <console/post_codes.h>
-#include <cpu/x86/mp.h>
 #include <cpu/x86/smm.h>
 #include <device/pci.h>
 #include <intelblocks/cpulib.h>
@@ -142,9 +141,6 @@

pch_finalize_script(dev);

-	printk(BIOS_DEBUG, "Clearing MCA.\n");
-	mp_run_on_all_cpus(mca_configure, NULL, 17 * USECS_PER_SEC);
-
    soc_lockdown(dev);

printk(BIOS_DEBUG, "Finalizing SMM.\n");