Re: [PATCH v5 2/2] x86/mce: Add support for Extended Physical Address MCA changes

[Yazen Ghannam]

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@xxxxxxxxxxxxxxx> 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