Messages in this thread | | | Date | Fri, 15 Apr 2022 14:56:54 +0000 | From | Yazen Ghannam <> | Subject | Re: [PATCH v5 2/2] x86/mce: Add support for Extended Physical Address MCA changes |
| |
On Thu, Apr 14, 2022 at 11:11:01AM +0200, Borislav Petkov wrote: > 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? >
I agree with you about the CR4.MCE statement. But MCi_CTL needs to be set by system software. The reset value is '0' at least on AMD systems.
Here's a example scenario.
1) OS has fully booted. a) MCi_CTL, MCG_CTL, CR4.MCE are all enabled. 2) Fatal MCA error occurs causing hardware-initialzed reset. No OS handling. a) MCA state info is warm reset persistent. b) MCi_STATUS, MCi_ADDR, etc. have valid info. c) MCi_CTL, MCG_CTL, CR4.MCE are all set to reset value: '0'. 3) OS, or optionally BIOS, polls MCA banks and logs any valid errors. a) Since MCi_CTL, etc. are cleared due to reset, any errors detected are from before the reset. 4) MCi_STATUS is cleared to discard old error information. 5) MCA is initiliazed (MCi_CTL, MCG_CTL, CR4.MCE, etc.). Any error detected now is a new error from this session.
> > > @@ -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. >
I agree. The Intel SDM and AMD APM have the following procedure, in summary.
1) Set MCG_CTL 2) Set MCi_CTL for all banks 3) Read MCi_STATUS and log valid errors. 4) Clear MCi_STATUS 5) Set CR4.MCE
I don't know of a reason why STATUS needs to be cleared after MCi_CTL is set. The only thing I can think of is that enabling MCi_CTL may cause spurious info logged in MCi_STATUS, and that needs to be cleared out. I'm asking AMD folks about it.
Of course, this contradicts the flow I outlined above, and also the flow given in the AMD Processor Programming Reference (PPR). I wonder if the architectural documents have gotten stale compared to current guidelines. I'm asking about this too.
Tony, Do you have any thoughts on this?
> > 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. >
Okay, yeah then maybe there should be another small patch to bring the init flow closer to x86 documentation.
Thanks, Yazen
| |