Re: [RFC PATCH 2/3] MCE, CE: Wire in the CE collector
On Tue, May 27, 2014 at 10:48:38AM -0700, Tony Luck wrote: > > + } else if (c->x86_vendor == X86_VENDOR_INTEL) > > + return m->status & BIT(7); > > Intel compound error codes aren't quite that simple. You need to look > at the low 16 bits of "status" (the MCACOD) field and see which is the > most significant bit set (ignoring bit 12, the "filter" bit). If the answer > is > bit 7 - then this is a memory error. But you can't just blindly check bit > 7 because if bit 8 is set, then this is a cache error, and if bit 11 is set, > then it is a bus/interconnect error - and either way bit 7 just gives more > detail on what cache/bus/interconnect error happened. In hex the test > you want is: > > return (m->status & 0xef80) == BIT(7); Thanks. > [compound error codes make my head hurt too] > > > if (!(flags & MCP_TIMESTAMP)) > > m.tsc = 0; > > - /* > > -* Don't get the IP here because it's unlikely to > > -* have anything to do with the actual error location. > > -*/ > > - if (!(flags & MCP_DONTLOG) && !mca_cfg.dont_log_ce) > > - mce_log(); > > + > > + __log_ce(, flags); > > I'm not happy with the total removal of mce_log() here. Skipping > it and doing *some* filtering in the kernel is OK (goal of this patch > set). But you just cut EDAC and/or EXTLOG out of the reporting path > completely. If the corrected error count for a page gets too high, your > new code will try to take the page offline ... but we won't have a report > from EDAC/EXTLOG telling us which DIMM that page belonged to. > > Perhaps __log_ce() needs a return value to say whether it took action > and then: > > if (__log_ce(, flags) && all-those-other-tests) > mce_log(); Right, this still needs a decision on how the collector fits in the whole picture - this patch was just the bare minimum to get the discussion going. The question is: do we want to tell userspace about every CE? Since we're doing everything ourselves in the kernel, I'd probably not upset needlessly but only when we've disabled a memory page. Concerning EXTLOG, shouldn't that be firmware-first anyway? If so, we won't have any control over it anyway. Concerning EDAC, we would want it to go first anyway, in order to do some decoding and *then* feed the final info to the collector. In both cases, we probably should have EDAC/EXTLOG run first so that we can have all the relevant error info, put into the collector and maybe then mce_log a subset of those errors. Let me draw a bit: A [ machine_check_poll ] -> [ EDAC/EXTLOG ] ---+> [ CE Collector ] .-.>.-. [ mce_log ] | | +> [ mce_log ] This is how the pipes could look like :-) The split at A is to denote that only a subset of the errors should go into the collector, i.e. ECC errors. After it, we want to decide what is going to go to the log. Maybe even add the ability to turn off the collector in the kernel if a userspace agent runs. Which would not be such a good idea as the kernel one should suffice for most if not all cases. Anyway, this should get us going towards hammering out the piping. Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/3] MCE, CE: Wire in the CE collector
> + } else if (c->x86_vendor == X86_VENDOR_INTEL) > + return m->status & BIT(7); Intel compound error codes aren't quite that simple. You need to look at the low 16 bits of "status" (the MCACOD) field and see which is the most significant bit set (ignoring bit 12, the "filter" bit). If the answer is bit 7 - then this is a memory error. But you can't just blindly check bit 7 because if bit 8 is set, then this is a cache error, and if bit 11 is set, then it is a bus/interconnect error - and either way bit 7 just gives more detail on what cache/bus/interconnect error happened. In hex the test you want is: return (m->status & 0xef80) == BIT(7); [compound error codes make my head hurt too] > if (!(flags & MCP_TIMESTAMP)) > m.tsc = 0; > - /* > -* Don't get the IP here because it's unlikely to > -* have anything to do with the actual error location. > -*/ > - if (!(flags & MCP_DONTLOG) && !mca_cfg.dont_log_ce) > - mce_log(); > + > + __log_ce(, flags); I'm not happy with the total removal of mce_log() here. Skipping it and doing *some* filtering in the kernel is OK (goal of this patch set). But you just cut EDAC and/or EXTLOG out of the reporting path completely. If the corrected error count for a page gets too high, your new code will try to take the page offline ... but we won't have a report from EDAC/EXTLOG telling us which DIMM that page belonged to. Perhaps __log_ce() needs a return value to say whether it took action and then: if (__log_ce(, flags) && all-those-other-tests) mce_log(); -Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 2/3] MCE, CE: Wire in the CE collector
From: Borislav Petkov
Add the CE collector to the polling path which collects the correctable
errors. Collect only DRAM ECC errors for now.
Signed-off-by: Borislav Petkov
---
arch/x86/kernel/cpu/mcheck/mce.c | 62 +++-
1 file changed, 55 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 882e79bb0cb6..732fbe61416d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -579,6 +579,46 @@ static void mce_read_aux(struct mce *m, int i)
DEFINE_PER_CPU(unsigned, mce_poll_count);
+static bool is_a_memory_error(struct mce *m)
+{
+ struct cpuinfo_x86 *c = _cpu_data;
+
+ if (c->x86_vendor == X86_VENDOR_AMD) {
+ /* ErrCodeExt[20:16] */
+ u8 xec = (m->status >> 16) & 0x1f;
+
+ return (xec == 0x0 || xec == 0x8);
+ } else if (c->x86_vendor == X86_VENDOR_INTEL)
+ return m->status & BIT(7);
+ else
+ return false;
+}
+
+static void __log_ce(struct mce *m, enum mcp_flags flags)
+{
+ int ret;
+
+ /*
+* Don't get the IP here because it's unlikely to have anything to do
+* with the actual error location.
+*/
+ if ((flags & MCP_DONTLOG) || mca_cfg.dont_log_ce)
+ return;
+
+ if (is_a_memory_error(m)) {
+ ret = ce_add_elem(m->addr >> PAGE_SHIFT);
+ if (ret < 0) {
+ u64 pfn = ce_del_lru_elem();
+ if (pfn)
+ mce_ring_add(pfn);
+
+ ce_add_elem(m->addr >> PAGE_SHIFT);
+ }
+ } else
+ mce_log(m);
+}
+
+
/*
* Poll for corrected events or events that happened before reset.
* Those are just logged through /dev/mcelog.
@@ -632,12 +672,8 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t
*b)
if (!(flags & MCP_TIMESTAMP))
m.tsc = 0;
- /*
-* Don't get the IP here because it's unlikely to
-* have anything to do with the actual error location.
-*/
- if (!(flags & MCP_DONTLOG) && !mca_cfg.dont_log_ce)
- mce_log();
+
+ __log_ce(, flags);
/*
* Clear state for this bank.
@@ -2530,5 +2566,17 @@ static int __init mcheck_debugfs_init(void)
return 0;
}
-late_initcall(mcheck_debugfs_init);
+#else
+static int __init mcheck_debugfs_init(void) {}
#endif
+
+static int __init mcheck_late_init(void)
+{
+ if (mcheck_debugfs_init())
+ pr_err("Error creating debugfs nodes!\n");
+
+ ce_init();
+
+ return 0;
+}
+late_initcall(mcheck_late_init);
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 2/3] MCE, CE: Wire in the CE collector
From: Borislav Petkov b...@suse.de
Add the CE collector to the polling path which collects the correctable
errors. Collect only DRAM ECC errors for now.
Signed-off-by: Borislav Petkov b...@suse.de
---
arch/x86/kernel/cpu/mcheck/mce.c | 62 +++-
1 file changed, 55 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 882e79bb0cb6..732fbe61416d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -579,6 +579,46 @@ static void mce_read_aux(struct mce *m, int i)
DEFINE_PER_CPU(unsigned, mce_poll_count);
+static bool is_a_memory_error(struct mce *m)
+{
+ struct cpuinfo_x86 *c = boot_cpu_data;
+
+ if (c-x86_vendor == X86_VENDOR_AMD) {
+ /* ErrCodeExt[20:16] */
+ u8 xec = (m-status 16) 0x1f;
+
+ return (xec == 0x0 || xec == 0x8);
+ } else if (c-x86_vendor == X86_VENDOR_INTEL)
+ return m-status BIT(7);
+ else
+ return false;
+}
+
+static void __log_ce(struct mce *m, enum mcp_flags flags)
+{
+ int ret;
+
+ /*
+* Don't get the IP here because it's unlikely to have anything to do
+* with the actual error location.
+*/
+ if ((flags MCP_DONTLOG) || mca_cfg.dont_log_ce)
+ return;
+
+ if (is_a_memory_error(m)) {
+ ret = ce_add_elem(m-addr PAGE_SHIFT);
+ if (ret 0) {
+ u64 pfn = ce_del_lru_elem();
+ if (pfn)
+ mce_ring_add(pfn);
+
+ ce_add_elem(m-addr PAGE_SHIFT);
+ }
+ } else
+ mce_log(m);
+}
+
+
/*
* Poll for corrected events or events that happened before reset.
* Those are just logged through /dev/mcelog.
@@ -632,12 +672,8 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t
*b)
if (!(flags MCP_TIMESTAMP))
m.tsc = 0;
- /*
-* Don't get the IP here because it's unlikely to
-* have anything to do with the actual error location.
-*/
- if (!(flags MCP_DONTLOG) !mca_cfg.dont_log_ce)
- mce_log(m);
+
+ __log_ce(m, flags);
/*
* Clear state for this bank.
@@ -2530,5 +2566,17 @@ static int __init mcheck_debugfs_init(void)
return 0;
}
-late_initcall(mcheck_debugfs_init);
+#else
+static int __init mcheck_debugfs_init(void) {}
#endif
+
+static int __init mcheck_late_init(void)
+{
+ if (mcheck_debugfs_init())
+ pr_err(Error creating debugfs nodes!\n);
+
+ ce_init();
+
+ return 0;
+}
+late_initcall(mcheck_late_init);
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/3] MCE, CE: Wire in the CE collector
+ } else if (c-x86_vendor == X86_VENDOR_INTEL) + return m-status BIT(7); Intel compound error codes aren't quite that simple. You need to look at the low 16 bits of status (the MCACOD) field and see which is the most significant bit set (ignoring bit 12, the filter bit). If the answer is bit 7 - then this is a memory error. But you can't just blindly check bit 7 because if bit 8 is set, then this is a cache error, and if bit 11 is set, then it is a bus/interconnect error - and either way bit 7 just gives more detail on what cache/bus/interconnect error happened. In hex the test you want is: return (m-status 0xef80) == BIT(7); [compound error codes make my head hurt too] if (!(flags MCP_TIMESTAMP)) m.tsc = 0; - /* -* Don't get the IP here because it's unlikely to -* have anything to do with the actual error location. -*/ - if (!(flags MCP_DONTLOG) !mca_cfg.dont_log_ce) - mce_log(m); + + __log_ce(m, flags); I'm not happy with the total removal of mce_log() here. Skipping it and doing *some* filtering in the kernel is OK (goal of this patch set). But you just cut EDAC and/or EXTLOG out of the reporting path completely. If the corrected error count for a page gets too high, your new code will try to take the page offline ... but we won't have a report from EDAC/EXTLOG telling us which DIMM that page belonged to. Perhaps __log_ce() needs a return value to say whether it took action and then: if (__log_ce(m, flags) all-those-other-tests) mce_log(m); -Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/3] MCE, CE: Wire in the CE collector
On Tue, May 27, 2014 at 10:48:38AM -0700, Tony Luck wrote: + } else if (c-x86_vendor == X86_VENDOR_INTEL) + return m-status BIT(7); Intel compound error codes aren't quite that simple. You need to look at the low 16 bits of status (the MCACOD) field and see which is the most significant bit set (ignoring bit 12, the filter bit). If the answer is bit 7 - then this is a memory error. But you can't just blindly check bit 7 because if bit 8 is set, then this is a cache error, and if bit 11 is set, then it is a bus/interconnect error - and either way bit 7 just gives more detail on what cache/bus/interconnect error happened. In hex the test you want is: return (m-status 0xef80) == BIT(7); Thanks. [compound error codes make my head hurt too] if (!(flags MCP_TIMESTAMP)) m.tsc = 0; - /* -* Don't get the IP here because it's unlikely to -* have anything to do with the actual error location. -*/ - if (!(flags MCP_DONTLOG) !mca_cfg.dont_log_ce) - mce_log(m); + + __log_ce(m, flags); I'm not happy with the total removal of mce_log() here. Skipping it and doing *some* filtering in the kernel is OK (goal of this patch set). But you just cut EDAC and/or EXTLOG out of the reporting path completely. If the corrected error count for a page gets too high, your new code will try to take the page offline ... but we won't have a report from EDAC/EXTLOG telling us which DIMM that page belonged to. Perhaps __log_ce() needs a return value to say whether it took action and then: if (__log_ce(m, flags) all-those-other-tests) mce_log(m); Right, this still needs a decision on how the collector fits in the whole picture - this patch was just the bare minimum to get the discussion going. The question is: do we want to tell userspace about every CE? Since we're doing everything ourselves in the kernel, I'd probably not upset needlessly but only when we've disabled a memory page. Concerning EXTLOG, shouldn't that be firmware-first anyway? If so, we won't have any control over it anyway. Concerning EDAC, we would want it to go first anyway, in order to do some decoding and *then* feed the final info to the collector. In both cases, we probably should have EDAC/EXTLOG run first so that we can have all the relevant error info, put into the collector and maybe then mce_log a subset of those errors. Let me draw a bit: A [ machine_check_poll ] - [ EDAC/EXTLOG ] ---+ [ CE Collector ] .-..-. [ mce_log ] | | + [ mce_log ] This is how the pipes could look like :-) The split at A is to denote that only a subset of the errors should go into the collector, i.e. ECC errors. After it, we want to decide what is going to go to the log. Maybe even add the ability to turn off the collector in the kernel if a userspace agent runs. Which would not be such a good idea as the kernel one should suffice for most if not all cases. Anyway, this should get us going towards hammering out the piping. Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
