Messages in this thread Patch in this message | | | Date | Thu, 14 Apr 2022 11:11:01 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH v5 2/2] x86/mce: Add support for Extended Physical Address MCA changes |
| |
On Wed, Apr 13, 2022 at 07:40:39PM +0000, Yazen Ghannam wrote: > I think the init logic breaks here. MCE now gets enabled before clearing old > errors. So it's possible that the old errors get overwritten by new ones.
Err, I don't understand. CR4.MCE bit description has:
"Regardless of whether machine-check exceptions are enabled, the processor records enabled-errors when they occur."
I'm guessing enabled errors are those for which the respective bits in the MCi_CTL banks are set. And I think the CPU comes out of reset with those bits set.
So the overwriting will happen regardless.
The only difference here is that "[s]etting MCE to 1 enables the machine-check exception mechanism." So you'll get a #MC raised vs shutdown on a fatal error.
Or am I missing an angle?
> > @@ -1791,6 +1761,9 @@ static void __mcheck_cpu_check_banks(void) > > if (!b->init) > > continue; > > > > + wrmsrl(mca_msr_reg(i, MCA_CTL), b->ctl); > > + wrmsrl(mca_msr_reg(i, MCA_STATUS), 0); > > Same idea here. STATUS should be cleared before turning on reporting in a bank > using MCA_CTL.
Look at the current code. Called in this order:
__mcheck_cpu_init_clear_banks: wrmsrl(mca_msr_reg(i, MCA_CTL), b->ctl); wrmsrl(mca_msr_reg(i, MCA_STATUS), 0); __mcheck_cpu_check_banks rdmsrl(mca_msr_reg(i, MCA_CTL), msrval); b->init = !!msrval;
STATUS *is* cleared after MCA_CTL now too. If this ordering is wrong - and it sounds like it is - then this needs to be a separate patch and Cc: <stable@vger.kernel.org> and needs to go in now.
> One downside though is that the system goes longer with CR4.MCE cleared. So > there's greater risk of encountering a shutdown due to a machine check error.
Yeah, I don't think the couple of msecs matter.
Thx.
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |