Re: Fwd: [PATCH] x86, MCE, AMD: save IA32_MCi_STATUS before machine_check_poll() resets it

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On Thu, Oct 09, 2014 at 12:57:50AM +0200, Borislav Petkov wrote:
> On Wed, Oct 08, 2014 at 04:52:06PM -0500, Aravind Gopalakrishnan wrote:
> > I am not understanding why m.bank is assigned this value..
> 
> That's a very good question, see below for some history.
> 
> > 
> > It only causes incorrect decoding-
> > [  608.832916] DEBUG: raise_amd_threshold_event
> > [  608.832926] [Hardware Error]: Corrected error, no action required.
> > [  608.833143] [Hardware Error]: CPU:26 (15:2:0)
> > MC165_STATUS[-|CE|MiscV|-|AddrV|-|-]: 0x8c00000000000000
> > [  608.833551] [Hardware Error]: MC165_ADDR: 0x0000000000000000
> > [  608.833777] [Hardware Error]: cache level: RESV, tx: INSN
> > [  608.834034] amd_inject module loaded ...
> > 
> > 
> > (Obviously, as in amd_decode_mce() we switch (m->bank) for decoding the
> > status and there is no bank 165)
> > 
> > OTOH, if m.bank = bank;
> > Then we get correct decoding info-
> 
> Yes, and I think we should do that only if we're using the *last* error
> to report the overflow with: we're reporting a thresholding counter
> overflow and the bank on which it was detected on should, of course, be
> part of the report.
> 

How do you mean "last error"?
The interrupt is only fired upon overflow..

> The "funny" bank is some sort of a software defined banks thing which
> got added in 2005 (see the patch I dug out below) and it was supposed
> to be used (I'm guessing here) for reporting thermal events using MCA
> (dumb idea, if you ask me) so since thermal events don't really have
> a bank, they decided to have some sort of a software-defined MCA bank
> which doesn't correspond to any hardware bank.
> 
> Then Jacob decided to use it for some reason too:
> 
> 95268664390b ("[PATCH] x86_64: mce_amd support for family 0x10 processors")
> 
> maybe because thresholding errors don't have a bank associated with them
> but if I'm not missing anything, they do!
> 

Right. The thresholding registers are nothing but _MISC(x) where x is a
bank value.

> Oh oh, ok, it just dawned on me! I think I know what it *might* have
> been: they wanted to report the overflowing with a special error
> signature which uses a software-defined bank. Ok, that actually makes
> sense: when you see an error for a sw-defined bank, you're reporting an
> thresholding counter overflow.
> 
> Which means that we shouldn't be populating m.status either, i.e. what
> we did earlier:
> 
> 	rdmsrl(MSR_IA32_MCx_STATUS(bank), m.status);
> 
> because this is a special error type.
>

How is it a "special error type"? It's still the same CE error that
we get notified with. Only difference being - now it's crossed a
specific 'threshold_limit'

So- I am not getting the rationale behind a S/W defined bank for reporting
this.

CE error if collected through polling gives proper decoding
info. So, why should this be any different for the same CE error for
which an interrupt is generated on crossing a threshold?

Thanks,
-Aravind

> Hmm, it is too late here to think straight, more tomorrow. But Aravind,
> that was a very good question, you actually made me dig into git history
> :-)
> 
> Good night.
>