Messages in this thread | | | From | "Luck, Tony" <> | Subject | RE: [PATCH 1/2 v2] x86, mce, severity: extend the the mce_severity | Date | Thu, 6 Nov 2014 17:27:14 +0000 |
| |
>> +int mce_severity(struct mce *m, int tolerant, char **msg, bool is_excp) > > You're adding a function argument which is carrying redundant info which > is already present in *m... > >> { >> + enum exception excp = (is_excp ? EXCP_CONTEXT : NO_EXCP); > > ... and so this should be: > > excp = ((m->mcg_status & MCG_STATUS_MCIP) ? EXCP_CONTEXT : NO_EXCP);
That only works if you trust that MCG_STATUS.MCIP is correctly set to indicate whether we are in MCE or CMCI context. The current code doesn't do that - we check for, and flag it as a fatal error if we find ourselves in the MCE handler with MCIP==0. If you add the code you suggest, then it completely neuters the severity check:
MCESEV( PANIC, "MCIP not set in MCA handler", MCGMASK(MCG_STATUS_MCIP, 0) ),
I'm also a bit worried about the check for DEFERRED errors in the severity table. That isn't conditional on an: if (intel) do_onething(); else /*amd/ do_anotherthing(); So if we can misinterpret some bits on an Intel cpu as if we had a deferred error.
Overall, this might have seemed like a good idea to begin with, but we are piling more complexity into mce_severity() [a routine which everyone agrees is already tough to understand].
It doesn't even buy us some simple code in the polling path. We still have to do more checks on MCi_STATUS.MCACOD above and beyond what we get back from mce_severity()
Boris: Do you still want to keep pushing this way? Or should we look back fondly at version 1 of this patch?
-Tony
| |