[coreboot-gerrit] Change in ...coreboot[master]: soc/intel/skylake: Remove redundent 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 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.