[0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

[0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

[Borislav Petkov]

On Fri, Aug 25, 2017 at 12:24:04PM +0200, Borislav Petkov wrote:
> Next step is adding that to rasdaemon.

Ok, below is the dirty version of the changes that need to go into
rasdaemon, I'll clean that up later. With it, I get:

# ./rasdaemon -f
overriding event (541) ras:mc_event with new print handler
rasdaemon: ras:mc_event event enabled
rasdaemon: Enabled event ras:mc_event
overriding event (58) mce:mce_record with new print handler
rasdaemon: mce:mce_record event enabled
rasdaemon: Enabled event mce:mce_record
overriding event (542) ras:extlog_mem_event with new print handler
rasdaemon: ras:extlog_mem_event event enabled
rasdaemon: Enabled event ras:extlog_mem_event
rasdaemon: Listening to events for cpus 0 to 7
cpu 07:           <...>-104   [1433913776]     0.000006: mce_record:           2017-08-28 17:41:01 +0200 bank=4, status= 9c7d410092080813, MC4 Error (node 2): DRAM ECC error detected on the NB.
, cpu_type= generic CPU, cpu= 2, socketid= 0, misc= 0, addr= 6d3d483b, , apicid= 0

and looking at it now, I don't need that "MC%d Error..:" thing either.

All queued for the next version.

diff --git a/ras-mce-handler.c b/ras-mce-handler.c
index 2e520d3663ac..ff6f4b373e56 100644
--- a/ras-mce-handler.c
+++ b/ras-mce-handler.c
@@ -23,6 +23,7 @@
 #include <unistd.h>
 #include <stdint.h>
 #include "libtrace/kbuffer.h"
+#include "libtrace/event-utils.h"
 #include "ras-mce-handler.h"
 #include "ras-record.h"
 #include "ras-logger.h"
@@ -185,6 +186,10 @@ static int detect_cpu(struct ras_events *ras)
 	ret = 0;
 
 	if (!strcmp(mce->vendor, "AuthenticAMD")) {
+
+		ret = 0;
+		goto ret;
+
 		if (mce->family == 15)
 			mce->cputype = CPU_K8;
 		if (mce->family > 15) {
@@ -357,8 +362,9 @@ int ras_mce_event_handler(struct trace_seq *s,
 	unsigned long long val;
 	struct ras_events *ras = context;
 	struct mce_priv *mce = ras->mce_priv;
+	const char *decoded_mce;
 	struct mce_event e;
-	int rc = 0;
+	int rc = 0, len;
 
 	memset(&e, 0, sizeof(e));
 
@@ -422,6 +428,10 @@ int ras_mce_event_handler(struct trace_seq *s,
 	if (rc)
 		return rc;
 
+	decoded_mce = pevent_get_field_raw(s, event, "decoded_str", record, &len, 1);
+	if (decoded_mce)
+		strncpy(e.error_msg, decoded_mce, min(len, 4096));
+
 	if (!*e.error_msg && *e.mcastatus_msg)
 		mce_snprintf(e.error_msg, "%s", e.mcastatus_msg);
 

[0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

[Borislav Petkov]

Btw,

how about communicating stuff to the userspace daemon like this?

This'll simplify a lot of detection in userspace.

Thoughts?

diff --git a/drivers/ras/debugfs.c b/drivers/ras/debugfs.c
index 501603057dff..62d3da9d256f 100644
--- a/drivers/ras/debugfs.c
+++ b/drivers/ras/debugfs.c
@@ -1,5 +1,7 @@
 #include <linux/debugfs.h>
 
+#include "../../arch/x86/kernel/cpu/mcheck/mce-internal.h"
+
 struct dentry *ras_debugfs_dir;
 
 static atomic_t trace_count = ATOMIC_INIT(0);
@@ -12,7 +14,9 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
 
 static int trace_show(struct seq_file *m, void *v)
 {
-	return atomic_read(&trace_count);
+	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
+	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);
+	return 0;
 }
 
 static int trace_open(struct inode *inode, struct file *file)

[0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

[mark gross]

On Wed, Aug 30, 2017 at 01:48:43PM +0200, Borislav Petkov wrote:
> Btw,
> 
> how about communicating stuff to the userspace daemon like this?
> 
> This'll simplify a lot of detection in userspace.
> 
> Thoughts?
> 
> ---
> diff --git a/drivers/ras/debugfs.c b/drivers/ras/debugfs.c
> index 501603057dff..62d3da9d256f 100644
> --- a/drivers/ras/debugfs.c
> +++ b/drivers/ras/debugfs.c
> @@ -1,5 +1,7 @@
>  #include <linux/debugfs.h>
>  
> +#include "../../arch/x86/kernel/cpu/mcheck/mce-internal.h"
FWIW I this looks fishy in arch independent code.
I assume this include is for the definition of the mca_cfg global used in the
printf below. 


> +
>  struct dentry *ras_debugfs_dir;
>  
>  static atomic_t trace_count = ATOMIC_INIT(0);
> @@ -12,7 +14,9 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
>  
>  static int trace_show(struct seq_file *m, void *v)
>  {
> -	return atomic_read(&trace_count);
> +	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
> +	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);

do you want to worry about this debugfs interfaces as ABI?
debugfs changes have bitten me on one specific OS in irritating ways.

I'm not sure what the word is for debugfs interfaces as ABI these days so this
feedback may be not so useful.

--mark

> +	return 0;
>  }
>  
>  static int trace_open(struct inode *inode, struct file *file)
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

[Borislav Petkov]

On Wed, Aug 30, 2017 at 02:47:19PM -0700, mark gross wrote:
> FWIW I this looks fishy in arch independent code. I assume this
> include is for the definition of the mca_cfg global used in the printf
> below.

Yeah, it is a bit ad-hoc. I guess I can add an arch_has_decoder()
wrapper which each arch can define. That would be cleaner.

> do you want to worry about this debugfs interfaces as ABI?
> debugfs changes have bitten me on one specific OS in irritating ways.
> 
> I'm not sure what the word is for debugfs interfaces as ABI these days so this
> feedback may be not so useful.

Yeah, the moment something starts using it, it is ABI. The good thing
about it is, though, that it can get stuff appended to it, just like
tracepoints get fields added. And tools should be able to handle parsing
appended lines fine. Removing lines OTOH is always problematic. But
we'll make that append-only.

Thx!

[0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

[Steven Rostedt]

On Wed, 30 Aug 2017 14:47:19 -0700
mark gross <mgross@linux.intel.com> wrote:


> >  struct dentry *ras_debugfs_dir;
> >  
> >  static atomic_t trace_count = ATOMIC_INIT(0);
> > @@ -12,7 +14,9 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
> >  
> >  static int trace_show(struct seq_file *m, void *v)
> >  {
> > -	return atomic_read(&trace_count);
> > +	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
> > +	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);  
> 
> do you want to worry about this debugfs interfaces as ABI?

It's the old, if a tree falls in the forest issue. If you break the ABI
but nobody is around to notice, did it really break?

> debugfs changes have bitten me on one specific OS in irritating ways.
> 
> I'm not sure what the word is for debugfs interfaces as ABI these days so this
> feedback may be not so useful.

I discussed this with Boris before he sent it out. The current code is
actually broken. The trace_show() looks to be just a stub for a hack to
have userspace tell the kernel it's reading something (the
trace_count). The trace_show() function here returns the trace_count,
which is just wrong. The seq_file show function is suppose to return
less than zero on error, zero on success, or greater than zero if the
show is suppose to skip the current field but not error out. This
trace_show() function doesn't actually show anything. If you cat the
file, it will be blank. Returning zero or greater than zero has the
same effect. Which is nothing.

-- Steve
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

[Borislav Petkov]

On Thu, Aug 31, 2017 at 12:02:54AM +0200, Borislav Petkov wrote:
> Yeah, it is a bit ad-hoc. I guess I can add an arch_has_decoder()
> wrapper which each arch can define. That would be cleaner.

Something like this, I guess (ontop):

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 67f951f0c840..b4855edac729 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -388,4 +388,6 @@ static inline int mce_threshold_remove_device(unsigned int cpu) { return 0; };
 
 #endif
 
+#define ras_arch_has_decoder ras_arch_has_decoder
+bool ras_arch_has_decoder(void);
 #endif /* _ASM_X86_MCE_H */
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 3ecc2b2db8f3..6414fa70fd42 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -182,6 +182,12 @@ void mce_unregister_decode_chain(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(mce_unregister_decode_chain);
 
+bool ras_arch_has_decoder(void)
+{
+	return mca_cfg.has_decoder;
+}
+EXPORT_SYMBOL_GPL(ras_arch_has_decoder);
+
 static inline u32 ctl_reg(int bank)
 {
 	return MSR_IA32_MCx_CTL(bank);
diff --git a/drivers/ras/debugfs.c b/drivers/ras/debugfs.c
index 62d3da9d256f..b368e8ae4738 100644
--- a/drivers/ras/debugfs.c
+++ b/drivers/ras/debugfs.c
@@ -1,6 +1,6 @@
 #include <linux/debugfs.h>
 
-#include "../../arch/x86/kernel/cpu/mcheck/mce-internal.h"
+#include <asm/mce.h>
 
 struct dentry *ras_debugfs_dir;
 
@@ -15,7 +15,7 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
 static int trace_show(struct seq_file *m, void *v)
 {
 	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
-	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);
+	seq_printf(m, "has decoder:%d\n", ras_arch_has_decoder());
 	return 0;
 }
 
diff --git a/drivers/ras/debugfs.h b/drivers/ras/debugfs.h
index db72e4513191..9c75c4c5b84d 100644
--- a/drivers/ras/debugfs.h
+++ b/drivers/ras/debugfs.h
@@ -5,4 +5,9 @@
 
 extern struct dentry *ras_debugfs_dir;
 
+#ifndef ras_arch_has_decoder
+#define ras_arch_has_decoder ras_arch_has_decoder
+static inline bool ras_arch_has_decoder(void)	{ return false; }
+#endif
+
 #endif /* __RAS_DEBUGFS_H__ */

[0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

[mark gross]

On Wed, Aug 30, 2017 at 07:30:58PM -0400, Steven Rostedt wrote:
> On Wed, 30 Aug 2017 14:47:19 -0700
> mark gross <mgross@linux.intel.com> wrote:
> 
> 
> > >  struct dentry *ras_debugfs_dir;
> > >  
> > >  static atomic_t trace_count = ATOMIC_INIT(0);
> > > @@ -12,7 +14,9 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
> > >  
> > >  static int trace_show(struct seq_file *m, void *v)
> > >  {
> > > -	return atomic_read(&trace_count);
> > > +	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
> > > +	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);  
> > 
> > do you want to worry about this debugfs interfaces as ABI?
> 
> It's the old, if a tree falls in the forest issue. If you break the ABI
> but nobody is around to notice, did it really break?

perhaps, but what I was trying to point out was: "multi line debugFS printf's
like this are very easy to change to append other information and you might
want to worry about the ABI implications that could happen in the future."

--mark
> 
> > debugfs changes have bitten me on one specific OS in irritating ways.
> > 
> > I'm not sure what the word is for debugfs interfaces as ABI these days so this
> > feedback may be not so useful.
> 
> I discussed this with Boris before he sent it out. The current code is
> actually broken. The trace_show() looks to be just a stub for a hack to
> have userspace tell the kernel it's reading something (the
> trace_count). The trace_show() function here returns the trace_count,
> which is just wrong. The seq_file show function is suppose to return
> less than zero on error, zero on success, or greater than zero if the
> show is suppose to skip the current field but not error out. This
> trace_show() function doesn't actually show anything. If you cat the
> file, it will be blank. Returning zero or greater than zero has the
> same effect. Which is nothing.
> 
> -- Steve
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

[Borislav Petkov]

On Sun, Sep 03, 2017 at 04:37:04PM -0700, mark gross wrote:
> > It's the old, if a tree falls in the forest issue. If you break the ABI
> > but nobody is around to notice, did it really break?
> 
> perhaps, but what I was trying to point out was: "multi line debugFS printf's
> like this are very easy to change to append other information and you might
> want to worry about the ABI implications that could happen in the future."

Right, as I said to Mark earlier:

"Yeah, the moment something starts using it, it is ABI. The good thing
about it is, though, that it can get stuff appended to it, just like
tracepoints get fields added. And tools should be able to handle parsing
appended lines fine. Removing lines OTOH is always problematic. But
we'll make that append-only."

It is basically the same principle as with tracepoints where we can add
members easily.