[v3,4/9] ACPI, x86: Extended error log driver for x86 platform

Commit Message

Borislav Petkov Oct. 18, 2013, 9:27 p.m. UTC

On Fri, Oct 18, 2013 at 08:57:22PM +0000, Luck, Tony wrote:
> Long term ... I'd be happy to see mce_log() go away. But we need to
> have a robust, well tested replacement in place for some time before
> such a move is up for discussion.

Basically a userspace daemon consuming the tracepoint or plural,
tracepoints.

> Yes - double error reporting should be avoided.

Right.

> Our first platforms to implement this only do so for memory errors.
> This could change in the future (the UEFI appendix N error record has
> defined sub-sections for lots of types of errors).

Ok.

> Currently EDAC hooked into the mce even notification chain provides a
> return code to indicate whether it completely processed the error, or
> whether to fall through to the rest of mce_log():
> 
> 	if (ret == NOTIFY_STOP)
> 		return;
> 
> Having both EDAC and this new extended error log both registered on this
> chain would probably not be helpful in most cases.

Not only that - you don't need EDAC because all the information is in
the MCA registers and the eMCA supplement, if there is one.

EDAC would be used on older systems which don't sport eMCA.

Now, concerning the current situation, we probably want to do something
like this:

--

Right, we've moved the eMCA print thingie to mce_log so that we get a
chance to run the first TP issuing the raw MCA registers and then run
the eMCA TP as a follow-up.

We've taught mce_ext_err_print() to return a true/false retval to denote:

* true: it has collected data successfully, no need to go down the reporting
  chain

* false: eMCA failed somehow, log the error down and trigger mcelog in
  userspace.

How does that sound?

> Not sure if we should handle that with user education to not load both
> an EDAC and ext_log driver or if there should be some enforcement.

Definitely enforcement. The flags thing I was telling you about recently
could be one way to do it.

> trace_mce_record() dumps the raw data from the machine check banks. I
> think there may still be a case for having this. Analysis tools that
> look at this trace as well should be smart enough to connect the dots.

Yes, sure. The more non-overlaping data we get, the better.

Thanks.

Comments

Tony Luck Oct. 18, 2013, 10:22 p.m. UTC | #1

@@ -154,6 +154,10 @@ void mce_log(struct mce *mce)
 	/* Emit the trace record: */
 	trace_mce_record(mce);
 
+	if (mce_ext_err_print)
+		if (mce_ext_err_print(NULL, m.extcpu, i))
+			return;
+
 	ret = atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, mce);
 	if (ret == NOTIFY_STOP)
 		return;

If we move mce_ext_err_print() this far ... then it's only one line further down
to have it be part of the x86_mce_decoder_chain as suggested by Naveen.

-Tony

Borislav Petkov Oct. 19, 2013, 9:57 a.m. UTC | #2

On Fri, Oct 18, 2013 at 10:22:26PM +0000, Luck, Tony wrote:
> @@ -154,6 +154,10 @@ void mce_log(struct mce *mce)
>  	/* Emit the trace record: */
>  	trace_mce_record(mce);
>  
> +	if (mce_ext_err_print)
> +		if (mce_ext_err_print(NULL, m.extcpu, i))
> +			return;
> +
>  	ret = atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, mce);
>  	if (ret == NOTIFY_STOP)
>  		return;
> 
> If we move mce_ext_err_print() this far ... then it's only one line further down
> to have it be part of the x86_mce_decoder_chain as suggested by Naveen.

Right, if you want mce_ext_err_print() to be the first and the only one
called on the chain, then you'd have to play with the priority.

But yes, this is possible and it would make it all even cleaner
and simpler by simply not needing the reg/dereg interfaces for
mce_ext_err_print but adding it to the chain.

Patch

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index b1b04123f3d9..382c78eaf474 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -154,6 +154,10 @@  void mce_log(struct mce *mce)
 	/* Emit the trace record: */
 	trace_mce_record(mce);
 
+	if (mce_ext_err_print)
+		if (mce_ext_err_print(NULL, m.extcpu, i))
+			return;
+
 	ret = atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, mce);
 	if (ret == NOTIFY_STOP)
 		return;