Re: [PATCH 3/3] mce: acpi/apei: trace: Enable ghes memory error trace event

[Mauro Carvalho Chehab <m.chehab@samsung.com>]

Em Thu, 15 Aug 2013 15:44:54 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Thu, Aug 15, 2013 at 10:26:07AM -0300, Mauro Carvalho Chehab wrote:
> > I mean that the edac core needs to know that, on a given system, the
> > BIOS is accessing the hardware registers and sending the data via
> > ghes_edac.
> 
> Right, that's the firmware-first thing which Naveen did - see
> mce_disable_bank.
> 
> > No. As we want that fatal errors to also be properly reported, the
> > kernel will still need to know the memory layout.
> 
> Read what I said: if you have the silkscreen label you don't need the
> memory layout - you *already* *know* which DIMM is affected.

AFAIKT, APEI doesn't provide the silkscreen label. Some code (or some
datasheet) is needed to translate between what APEI provides into the
silkscreen label.

Naveen, please correct me if I'm wrong.

> Also, fatal errors are a whole different beast where we run in NMI
> context or we even don't get to run the #MC handler on some systems.

I see.

Yes, APEI currently prints only a raw event on high severity errors
at ghes_notify_nmi(), and doesn't call ghes_edac. Changing it would
require to parse the error at __ghes_print_estatus(). Not sure how
easy would be to change that.

Em Thu, 15 Aug 2013 15:51:06 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Thu, Aug 15, 2013 at 10:34:21AM -0300, Mauro Carvalho Chehab wrote:
> > Yes, but the thing is that it is not safe to use the hardware driver
> > if the BIOS is also reading the hardware error registers directly, as,
> > on several hardware, a read cause the error data to be cleaned on such
> > register.
> 
> Here's the deal:
> 
> * We parse some APEI table and disable those MCA banks which the BIOS
> wants to handle first.
> 
> * When the BIOS decides to report an error from that handling, it does
> so over another BIOS table.

OK.

> 
> * Now you have two possibilities:
> 
> ** On systems without an edac driver or where it doesn't make sense to
> have the ghes_edac driver, we call trace_mc_event() straight from APEI
> code (this is what we're currently discussung).
> 
> ** On other systems, where we need ghes_edac, we *don't* use the
> trace_mc_event() tracepoint in the APEI code but let it come from
> ghes_edac with additional information collected by edac.

I don't see why should we have those two alternatives, as, at worse
case (e. g. if ghes_edac can't enrich the APEI data with labels),
they'll basically provide the very same data to userspace, and the
EDAC extra overhead is small, on its error report logic.

The risk of doing the very same thing on two different places is that
the logic to encapsulate APEI data into trace_mc_event() would be
on two separate places. It risks that someone would change one of the
drivers and forget to apply the very same change on the other, causing
parse errors on userspace, depending on the source.

-- 

Cheers,
Mauro