[RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Hi,

here's a first stab at adding a tracepoint which dumps the decoded MCE
string to userspace. The main idea is to have the decoding functionality
in the kernel and depending on whether you have userspace consumers
listening or not, to dump the error to the tracepoint or to dmesg.

In either case, we do the decoding in the kernel and don't need special
userspace. Furthermore, adding new CPU support will have to be done only
in one place.

First 6 patches are cleanups which are good to have regardless, IMO.

Any constructive comments and suggestions are appreciated.

Thanks.

P.S., Thanks to Rostedt for the input!

Borislav Petkov (8):
  EDAC, mce_amd: Rename decode_smca_errors() to decode_smca_error()
  EDAC, mce_amd: Get rid of most struct cpuinfo_x86 uses
  EDAC, mce_amd: Get rid of local var in amd_filter_mce()
  seq_buf: Add seq_buf_clear_buf()
  seq_buf: Export seq_buf_printf() to modules
  EDAC, mce_amd: Convert to seq_buf
  EDAC, mce_amd: Add a simple tracepoint dumping a decoded string
  EDAC, mce_amd: Issue the decoded info through the TP or printk

 drivers/edac/mce_amd.c  | 285 +++++++++++++++++++++++++++---------------------
 drivers/ras/ras.c       |   1 +
 include/linux/seq_buf.h |   7 ++
 include/ras/ras_event.h |  16 +++
 lib/seq_buf.c           |   1 +
 5 files changed, 186 insertions(+), 124 deletions(-)

-- 
2.14.0.rc0

[RFC PATCH 1/8] EDAC, mce_amd: Rename decode_smca_errors() to decode_smca_error()

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Singular fits better because it decodes a single error.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/edac/mce_amd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index e8e9d7df0a6e..94cadd7f69e9 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -849,7 +849,7 @@ static void decode_mc6_mce(struct mce *m)
 }
 
 /* Decode errors according to Scalable MCA specification */
-static void decode_smca_errors(struct mce *m)
+static void decode_smca_error(struct mce *m)
 {
 	struct smca_hwid *hwid;
 	unsigned int bank_type;
@@ -998,7 +998,7 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 
 		pr_cont("\n");
 
-		decode_smca_errors(m);
+		decode_smca_error(m);
 		goto err_code;
 	}
 
-- 
2.14.0.rc0

[RFC PATCH 2/8] EDAC, mce_amd: Get rid of most struct cpuinfo_x86 uses

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

struct mce.cpuid contains CPUID(1).EAX which contains family, model and
stepping and thus has enough information for our purposes. Thus get rid
of some external dependencies which are not really needed.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/edac/mce_amd.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index 94cadd7f69e9..c0ae47a5c7b7 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -1,6 +1,8 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 
+#include <asm/cpu.h>
+
 #include "mce_amd.h"
 
 static struct amd_decoder_ops *fam_ops;
@@ -744,7 +746,7 @@ static void decode_mc3_mce(struct mce *m)
 
 static void decode_mc4_mce(struct mce *m)
 {
-	struct cpuinfo_x86 *c = &boot_cpu_data;
+	unsigned int fam = x86_family(m->cpuid);
 	int node_id = amd_get_nb_id(m->extcpu);
 	u16 ec = EC(m->status);
 	u8 xec = XEC(m->status, 0x1f);
@@ -758,7 +760,7 @@ static void decode_mc4_mce(struct mce *m)
 		/* special handling for DRAM ECCs */
 		if (xec == 0x0 || xec == 0x8) {
 			/* no ECCs on F11h */
-			if (c->x86 == 0x11)
+			if (fam == 0x11)
 				goto wrong_mc4_mce;
 
 			pr_cont("%s.\n", mc4_mce_desc[xec]);
@@ -779,7 +781,7 @@ static void decode_mc4_mce(struct mce *m)
 		return;
 
 	case 0x19:
-		if (boot_cpu_data.x86 == 0x15 || boot_cpu_data.x86 == 0x16)
+		if (fam == 0x15 || fam == 0x16)
 			pr_cont("Compute Unit Data Error.\n");
 		else
 			goto wrong_mc4_mce;
@@ -802,11 +804,11 @@ static void decode_mc4_mce(struct mce *m)
 
 static void decode_mc5_mce(struct mce *m)
 {
-	struct cpuinfo_x86 *c = &boot_cpu_data;
+	unsigned int fam = x86_family(m->cpuid);
 	u16 ec = EC(m->status);
 	u8 xec = XEC(m->status, xec_mask);
 
-	if (c->x86 == 0xf || c->x86 == 0x11)
+	if (fam == 0xf || fam == 0x11)
 		goto wrong_mc5_mce;
 
 	pr_emerg(HW_ERR "MC5 Error: ");
@@ -859,7 +861,7 @@ static void decode_smca_error(struct mce *m)
 	if (m->bank >= ARRAY_SIZE(smca_banks))
 		return;
 
-	if (boot_cpu_data.x86 >= 0x17 && m->bank == 4)
+	if (x86_family(m->cpuid) >= 0x17 && m->bank == 4)
 		pr_emerg(HW_ERR "Bank 4 is reserved on Fam17h.\n");
 
 	hwid = smca_banks[m->bank].hwid;
@@ -942,7 +944,7 @@ static int
 amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 {
 	struct mce *m = (struct mce *)data;
-	struct cpuinfo_x86 *c = &cpu_data(m->extcpu);
+	unsigned int fam = x86_family(m->cpuid);
 	int ecc;
 
 	if (amd_filter_mce(m))
@@ -952,7 +954,7 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 
 	pr_emerg(HW_ERR "CPU:%d (%x:%x:%x) MC%d_STATUS[%s|%s|%s|%s|%s",
 		m->extcpu,
-		c->x86, c->x86_model, c->x86_mask,
+		fam, x86_model(m->cpuid), x86_stepping(m->cpuid),
 		m->bank,
 		((m->status & MCI_STATUS_OVER)	? "Over"  : "-"),
 		((m->status & MCI_STATUS_UC)	? "UE"	  :
@@ -961,11 +963,11 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 		((m->status & MCI_STATUS_PCC)	? "PCC"	  : "-"),
 		((m->status & MCI_STATUS_ADDRV)	? "AddrV" : "-"));
 
-	if (c->x86 >= 0x15) {
+	if (fam >= 0x15) {
 		pr_cont("|%s", (m->status & MCI_STATUS_DEFERRED ? "Deferred" : "-"));
 
 		/* F15h, bank4, bit 43 is part of McaStatSubCache. */
-		if (c->x86 != 0x15 || m->bank != 4)
+		if (fam != 0x15 || m->bank != 4)
 			pr_cont("|%s", (m->status & MCI_STATUS_POISON ? "Poison" : "-"));
 	}
 
-- 
2.14.0.rc0

[RFC PATCH 3/8] EDAC, mce_amd: Get rid of local var in amd_filter_mce()

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

... and use the macro for that.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/edac/mce_amd.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index c0ae47a5c7b7..a11a671c7a38 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -913,12 +913,10 @@ static inline void amd_decode_err_code(u16 ec)
  */
 static bool amd_filter_mce(struct mce *m)
 {
-	u8 xec = (m->status >> 16) & 0x1f;
-
 	/*
 	 * NB GART TLB error reporting is disabled by default.
 	 */
-	if (m->bank == 4 && xec == 0x5 && !report_gart_errors)
+	if (m->bank == 4 && XEC(m->status, 0x1f) == 0x5 && !report_gart_errors)
 		return true;
 
 	return false;
-- 
2.14.0.rc0

[RFC PATCH 4/8] seq_buf: Add seq_buf_clear_buf()

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

This is the version which clears the supplied buffer too. Useful when
we're done with the buffer and want to clean it up and prepare it for
reuse.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/seq_buf.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index fb7eb9ccb1cd..8705a482e76c 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -28,6 +28,13 @@ static inline void seq_buf_clear(struct seq_buf *s)
 	s->readpos = 0;
 }
 
+/* Like seq_buf_clear() but zero out the buffer too. */
+static inline void seq_buf_clear_buf(struct seq_buf *s)
+{
+	seq_buf_clear(s);
+	memset(s->buffer, 0, s->size);
+}
+
 static inline void
 seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
 {
-- 
2.14.0.rc0

Re: [RFC PATCH 4/8] seq_buf: Add seq_buf_clear_buf()

[Steven Rostedt]

On Tue, 25 Jul 2017 17:45:57 +0200
Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> This is the version which clears the supplied buffer too. Useful when
> we're done with the buffer and want to clean it up and prepare it for
> reuse.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  include/linux/seq_buf.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index fb7eb9ccb1cd..8705a482e76c 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -28,6 +28,13 @@ static inline void seq_buf_clear(struct seq_buf *s)
>  	s->readpos = 0;
>  }
>  
> +/* Like seq_buf_clear() but zero out the buffer too. */
> +static inline void seq_buf_clear_buf(struct seq_buf *s)
> +{
> +	seq_buf_clear(s);
> +	memset(s->buffer, 0, s->size);
> +}
> +
>  static inline void
>  seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
>  {

[RFC PATCH 5/8] seq_buf: Export seq_buf_printf() to modules

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Will be used in a module in a later patch.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 lib/seq_buf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index cb18469e1f49..b5217cf1824d 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -90,6 +90,7 @@ int seq_buf_printf(struct seq_buf *s, const char *fmt, ...)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(seq_buf_printf);
 
 #ifdef CONFIG_BINARY_PRINTF
 /**
-- 
2.14.0.rc0

Re: [RFC PATCH 5/8] seq_buf: Export seq_buf_printf() to modules

[Steven Rostedt]

On Tue, 25 Jul 2017 17:45:58 +0200
Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Will be used in a module in a later patch.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  lib/seq_buf.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/seq_buf.c b/lib/seq_buf.c
> index cb18469e1f49..b5217cf1824d 100644
> --- a/lib/seq_buf.c
> +++ b/lib/seq_buf.c
> @@ -90,6 +90,7 @@ int seq_buf_printf(struct seq_buf *s, const char *fmt, ...)
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(seq_buf_printf);
>  
>  #ifdef CONFIG_BINARY_PRINTF
>  /**

[RFC PATCH 6/8] EDAC, mce_amd: Convert to seq_buf

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Convert the part which decodes the error description to the sequence
buffer facility and thus save ourselves the many printk() invocations
building the decoded string.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/edac/mce_amd.c | 203 ++++++++++++++++++++++++++-----------------------
 1 file changed, 108 insertions(+), 95 deletions(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index a11a671c7a38..1f5e9bb161f3 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -1,3 +1,4 @@
+#include <linux/seq_buf.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 
@@ -306,6 +307,11 @@ static struct smca_mce_desc smca_mce_descs[] = {
 	[SMCA_SMU]	= { smca_smu_mce_desc,	ARRAY_SIZE(smca_smu_mce_desc)	},
 };
 
+/* 128 because, well, nice and round - two cachelines. */
+#define BUF_LEN        128
+static char __err_buf[BUF_LEN];
+static struct seq_buf sb;
+
 static bool f12h_mc0_mce(u16 ec, u8 xec)
 {
 	bool ret = false;
@@ -315,9 +321,9 @@ static bool f12h_mc0_mce(u16 ec, u8 xec)
 		ret = true;
 
 		if (ll == LL_L2)
-			pr_cont("during L1 linefill from L2.\n");
+			seq_buf_printf(&sb, "during L1 linefill from L2.\n");
 		else if (ll == LL_L1)
-			pr_cont("Data/Tag %s error.\n", R4_MSG(ec));
+			seq_buf_printf(&sb, "Data/Tag %s error.\n", R4_MSG(ec));
 		else
 			ret = false;
 	}
@@ -327,7 +333,7 @@ static bool f12h_mc0_mce(u16 ec, u8 xec)
 static bool f10h_mc0_mce(u16 ec, u8 xec)
 {
 	if (R4(ec) == R4_GEN && LL(ec) == LL_L1) {
-		pr_cont("during data scrub.\n");
+		seq_buf_printf(&sb, "during data scrub.\n");
 		return true;
 	}
 	return f12h_mc0_mce(ec, xec);
@@ -336,7 +342,7 @@ static bool f10h_mc0_mce(u16 ec, u8 xec)
 static bool k8_mc0_mce(u16 ec, u8 xec)
 {
 	if (BUS_ERROR(ec)) {
-		pr_cont("during system linefill.\n");
+		seq_buf_printf(&sb, "during system linefill.\n");
 		return true;
 	}
 
@@ -356,14 +362,14 @@ static bool cat_mc0_mce(u16 ec, u8 xec)
 		switch (r4) {
 		case R4_DRD:
 		case R4_DWR:
-			pr_cont("Data/Tag parity error due to %s.\n",
-				(r4 == R4_DRD ? "load/hw prf" : "store"));
+			seq_buf_printf(&sb, "Data/Tag parity error due to %s.\n",
+					(r4 == R4_DRD ? "load/hw prf" : "store"));
 			break;
 		case R4_EVICT:
-			pr_cont("Copyback parity error on a tag miss.\n");
+			seq_buf_printf(&sb, "Copyback parity error on a tag miss.\n");
 			break;
 		case R4_SNOOP:
-			pr_cont("Tag parity error during snoop.\n");
+			seq_buf_printf(&sb, "Tag parity error during snoop.\n");
 			break;
 		default:
 			ret = false;
@@ -373,17 +379,17 @@ static bool cat_mc0_mce(u16 ec, u8 xec)
 		if ((II(ec) != II_MEM && II(ec) != II_IO) || LL(ec) != LL_LG)
 			return false;
 
-		pr_cont("System read data error on a ");
+		seq_buf_printf(&sb, "System read data error on a ");
 
 		switch (r4) {
 		case R4_RD:
-			pr_cont("TLB reload.\n");
+			seq_buf_printf(&sb, "TLB reload.\n");
 			break;
 		case R4_DWR:
-			pr_cont("store.\n");
+			seq_buf_printf(&sb, "store.\n");
 			break;
 		case R4_DRD:
-			pr_cont("load.\n");
+			seq_buf_printf(&sb, "load.\n");
 			break;
 		default:
 			ret = false;
@@ -403,28 +409,28 @@ static bool f15h_mc0_mce(u16 ec, u8 xec)
 
 		switch (xec) {
 		case 0x0:
-			pr_cont("Data Array access error.\n");
+			seq_buf_printf(&sb, "Data Array access error.\n");
 			break;
 
 		case 0x1:
-			pr_cont("UC error during a linefill from L2/NB.\n");
+			seq_buf_printf(&sb, "UC error during a linefill from L2/NB.\n");
 			break;
 
 		case 0x2:
 		case 0x11:
-			pr_cont("STQ access error.\n");
+			seq_buf_printf(&sb, "STQ access error.\n");
 			break;
 
 		case 0x3:
-			pr_cont("SCB access error.\n");
+			seq_buf_printf(&sb, "SCB access error.\n");
 			break;
 
 		case 0x10:
-			pr_cont("Tag error.\n");
+			seq_buf_printf(&sb, "Tag error.\n");
 			break;
 
 		case 0x12:
-			pr_cont("LDQ access error.\n");
+			seq_buf_printf(&sb, "LDQ access error.\n");
 			break;
 
 		default:
@@ -433,12 +439,12 @@ static bool f15h_mc0_mce(u16 ec, u8 xec)
 	} else if (BUS_ERROR(ec)) {
 
 		if (!xec)
-			pr_cont("System Read Data Error.\n");
+			seq_buf_printf(&sb, "System Read Data Error.\n");
 		else
-			pr_cont(" Internal error condition type %d.\n", xec);
+			seq_buf_printf(&sb, " Internal error condition type %d.\n", xec);
 	} else if (INT_ERROR(ec)) {
 		if (xec <= 0x1f)
-			pr_cont("Hardware Assert.\n");
+			seq_buf_printf(&sb, "Hardware Assert.\n");
 		else
 			ret = false;
 
@@ -453,13 +459,13 @@ static void decode_mc0_mce(struct mce *m)
 	u16 ec = EC(m->status);
 	u8 xec = XEC(m->status, xec_mask);
 
-	pr_emerg(HW_ERR "MC0 Error: ");
+	seq_buf_printf(&sb, HW_ERR "MC0 Error: ");
 
 	/* TLB error signatures are the same across families */
 	if (TLB_ERROR(ec)) {
 		if (TT(ec) == TT_DATA) {
-			pr_cont("%s TLB %s.\n", LL_MSG(ec),
-				((xec == 2) ? "locked miss"
+			seq_buf_printf(&sb, "%s TLB %s.\n", LL_MSG(ec),
+					((xec == 2) ? "locked miss"
 					    : (xec ? "multimatch" : "parity")));
 			return;
 		}
@@ -478,19 +484,19 @@ static bool k8_mc1_mce(u16 ec, u8 xec)
 		return false;
 
 	if (ll == 0x2)
-		pr_cont("during a linefill from L2.\n");
+		seq_buf_printf(&sb, "during a linefill from L2.\n");
 	else if (ll == 0x1) {
 		switch (R4(ec)) {
 		case R4_IRD:
-			pr_cont("Parity error during data load.\n");
+			seq_buf_printf(&sb, "Parity error during data load.\n");
 			break;
 
 		case R4_EVICT:
-			pr_cont("Copyback Parity/Victim error.\n");
+			seq_buf_printf(&sb, "Copyback Parity/Victim error.\n");
 			break;
 
 		case R4_SNOOP:
-			pr_cont("Tag Snoop error.\n");
+			seq_buf_printf(&sb, "Tag Snoop error.\n");
 			break;
 
 		default:
@@ -515,13 +521,13 @@ static bool cat_mc1_mce(u16 ec, u8 xec)
 		return false;
 
 	if (r4 == R4_IRD)
-		pr_cont("Data/tag array parity error for a tag hit.\n");
+		seq_buf_printf(&sb, "Data/tag array parity error for a tag hit.\n");
 	else if (r4 == R4_SNOOP)
-		pr_cont("Tag error during snoop/victimization.\n");
+		seq_buf_printf(&sb, "Tag error during snoop/victimization.\n");
 	else if (xec == 0x0)
-		pr_cont("Tag parity error from victim castout.\n");
+		seq_buf_printf(&sb, "Tag parity error from victim castout.\n");
 	else if (xec == 0x2)
-		pr_cont("Microcode patch RAM parity error.\n");
+		seq_buf_printf(&sb, "Microcode patch RAM parity error.\n");
 	else
 		ret = false;
 
@@ -537,19 +543,19 @@ static bool f15h_mc1_mce(u16 ec, u8 xec)
 
 	switch (xec) {
 	case 0x0 ... 0xa:
-		pr_cont("%s.\n", f15h_mc1_mce_desc[xec]);
+		seq_buf_printf(&sb, "%s.\n", f15h_mc1_mce_desc[xec]);
 		break;
 
 	case 0xd:
-		pr_cont("%s.\n", f15h_mc1_mce_desc[xec-2]);
+		seq_buf_printf(&sb, "%s.\n", f15h_mc1_mce_desc[xec-2]);
 		break;
 
 	case 0x10:
-		pr_cont("%s.\n", f15h_mc1_mce_desc[xec-4]);
+		seq_buf_printf(&sb, "%s.\n", f15h_mc1_mce_desc[xec-4]);
 		break;
 
 	case 0x11 ... 0x15:
-		pr_cont("Decoder %s parity error.\n", f15h_mc1_mce_desc[xec-4]);
+		seq_buf_printf(&sb, "Decoder %s parity error.\n", f15h_mc1_mce_desc[xec-4]);
 		break;
 
 	default:
@@ -563,18 +569,18 @@ static void decode_mc1_mce(struct mce *m)
 	u16 ec = EC(m->status);
 	u8 xec = XEC(m->status, xec_mask);
 
-	pr_emerg(HW_ERR "MC1 Error: ");
+	seq_buf_printf(&sb, HW_ERR "MC1 Error: ");
 
 	if (TLB_ERROR(ec))
-		pr_cont("%s TLB %s.\n", LL_MSG(ec),
-			(xec ? "multimatch" : "parity error"));
+		seq_buf_printf(&sb, "%s TLB %s.\n", LL_MSG(ec),
+				(xec ? "multimatch" : "parity error"));
 	else if (BUS_ERROR(ec)) {
 		bool k8 = (boot_cpu_data.x86 == 0xf && (m->status & BIT_64(58)));
 
-		pr_cont("during %s.\n", (k8 ? "system linefill" : "NB data read"));
+		seq_buf_printf(&sb, "during %s.\n", (k8 ? "system linefill" : "NB data read"));
 	} else if (INT_ERROR(ec)) {
 		if (xec <= 0x3f)
-			pr_cont("Hardware Assert.\n");
+			seq_buf_printf(&sb, "Hardware Assert.\n");
 		else
 			goto wrong_mc1_mce;
 	} else if (fam_ops->mc1_mce(ec, xec))
@@ -593,27 +599,27 @@ static bool k8_mc2_mce(u16 ec, u8 xec)
 	bool ret = true;
 
 	if (xec == 0x1)
-		pr_cont(" in the write data buffers.\n");
+		seq_buf_printf(&sb, " in the write data buffers.\n");
 	else if (xec == 0x3)
-		pr_cont(" in the victim data buffers.\n");
+		seq_buf_printf(&sb, " in the victim data buffers.\n");
 	else if (xec == 0x2 && MEM_ERROR(ec))
-		pr_cont(": %s error in the L2 cache tags.\n", R4_MSG(ec));
+		seq_buf_printf(&sb, ": %s error in the L2 cache tags.\n", R4_MSG(ec));
 	else if (xec == 0x0) {
 		if (TLB_ERROR(ec))
-			pr_cont("%s error in a Page Descriptor Cache or Guest TLB.\n",
-				TT_MSG(ec));
+			seq_buf_printf(&sb, "%s error in a Page Descriptor Cache or Guest TLB.\n",
+					TT_MSG(ec));
 		else if (BUS_ERROR(ec))
-			pr_cont(": %s/ECC error in data read from NB: %s.\n",
-				R4_MSG(ec), PP_MSG(ec));
+			seq_buf_printf(&sb, ": %s/ECC error in data read from NB: %s.\n",
+					R4_MSG(ec), PP_MSG(ec));
 		else if (MEM_ERROR(ec)) {
 			u8 r4 = R4(ec);
 
 			if (r4 >= 0x7)
-				pr_cont(": %s error during data copyback.\n",
-					R4_MSG(ec));
+				seq_buf_printf(&sb, ": %s error during data copyback.\n",
+						R4_MSG(ec));
 			else if (r4 <= 0x1)
-				pr_cont(": %s parity/ECC error during data "
-					"access from L2.\n", R4_MSG(ec));
+				seq_buf_printf(&sb,
+": %s parity/ECC error during data access from L2.\n", R4_MSG(ec));
 			else
 				ret = false;
 		} else
@@ -630,24 +636,24 @@ static bool f15h_mc2_mce(u16 ec, u8 xec)
 
 	if (TLB_ERROR(ec)) {
 		if (xec == 0x0)
-			pr_cont("Data parity TLB read error.\n");
+			seq_buf_printf(&sb, "Data parity TLB read error.\n");
 		else if (xec == 0x1)
-			pr_cont("Poison data provided for TLB fill.\n");
+			seq_buf_printf(&sb, "Poison data provided for TLB fill.\n");
 		else
 			ret = false;
 	} else if (BUS_ERROR(ec)) {
 		if (xec > 2)
 			ret = false;
 
-		pr_cont("Error during attempted NB data read.\n");
+		seq_buf_printf(&sb, "Error during attempted NB data read.\n");
 	} else if (MEM_ERROR(ec)) {
 		switch (xec) {
 		case 0x4 ... 0xc:
-			pr_cont("%s.\n", f15h_mc2_mce_desc[xec - 0x4]);
+			seq_buf_printf(&sb, "%s.\n", f15h_mc2_mce_desc[xec - 0x4]);
 			break;
 
 		case 0x10 ... 0x14:
-			pr_cont("%s.\n", f15h_mc2_mce_desc[xec - 0x7]);
+			seq_buf_printf(&sb, "%s.\n", f15h_mc2_mce_desc[xec - 0x7]);
 			break;
 
 		default:
@@ -655,7 +661,7 @@ static bool f15h_mc2_mce(u16 ec, u8 xec)
 		}
 	} else if (INT_ERROR(ec)) {
 		if (xec <= 0x3f)
-			pr_cont("Hardware Assert.\n");
+			seq_buf_printf(&sb, "Hardware Assert.\n");
 		else
 			ret = false;
 	}
@@ -672,29 +678,29 @@ static bool f16h_mc2_mce(u16 ec, u8 xec)
 
 	switch (xec) {
 	case 0x04 ... 0x05:
-		pr_cont("%cBUFF parity error.\n", (r4 == R4_RD) ? 'I' : 'O');
+		seq_buf_printf(&sb, "%cBUFF parity error.\n", (r4 == R4_RD) ? 'I' : 'O');
 		break;
 
 	case 0x09 ... 0x0b:
 	case 0x0d ... 0x0f:
-		pr_cont("ECC error in L2 tag (%s).\n",
-			((r4 == R4_GEN)   ? "BankReq" :
-			((r4 == R4_SNOOP) ? "Prb"     : "Fill")));
+		seq_buf_printf(&sb, "ECC error in L2 tag (%s).\n",
+				((r4 == R4_GEN)   ? "BankReq" :
+				((r4 == R4_SNOOP) ? "Prb"     : "Fill")));
 		break;
 
 	case 0x10 ... 0x19:
 	case 0x1b:
-		pr_cont("ECC error in L2 data array (%s).\n",
-			(((r4 == R4_RD) && !(xec & 0x3)) ? "Hit"  :
-			((r4 == R4_GEN)   ? "Attr" :
-			((r4 == R4_EVICT) ? "Vict" : "Fill"))));
+		seq_buf_printf(&sb, "ECC error in L2 data array (%s).\n",
+				(((r4 == R4_RD) && !(xec & 0x3)) ? "Hit"  :
+				((r4 == R4_GEN)   ? "Attr" :
+				((r4 == R4_EVICT) ? "Vict" : "Fill"))));
 		break;
 
 	case 0x1c ... 0x1d:
 	case 0x1f:
-		pr_cont("Parity error in L2 attribute bits (%s).\n",
-			((r4 == R4_RD)  ? "Hit"  :
-			((r4 == R4_GEN) ? "Attr" : "Fill")));
+		seq_buf_printf(&sb, "Parity error in L2 attribute bits (%s).\n",
+				((r4 == R4_RD)  ? "Hit"  :
+				((r4 == R4_GEN) ? "Attr" : "Fill")));
 		break;
 
 	default:
@@ -709,10 +715,10 @@ static void decode_mc2_mce(struct mce *m)
 	u16 ec = EC(m->status);
 	u8 xec = XEC(m->status, xec_mask);
 
-	pr_emerg(HW_ERR "MC2 Error: ");
+	seq_buf_printf(&sb, HW_ERR "MC2 Error: ");
 
 	if (!fam_ops->mc2_mce(ec, xec))
-		pr_cont(HW_ERR "Corrupted MC2 MCE info?\n");
+		pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
 }
 
 static void decode_mc3_mce(struct mce *m)
@@ -726,7 +732,7 @@ static void decode_mc3_mce(struct mce *m)
 		return;
 	}
 
-	pr_emerg(HW_ERR "MC3 Error");
+	seq_buf_printf(&sb, HW_ERR "MC3 Error");
 
 	if (xec == 0x0) {
 		u8 r4 = R4(ec);
@@ -734,7 +740,7 @@ static void decode_mc3_mce(struct mce *m)
 		if (!BUS_ERROR(ec) || (r4 != R4_DRD && r4 != R4_DWR))
 			goto wrong_mc3_mce;
 
-		pr_cont(" during %s.\n", R4_MSG(ec));
+		seq_buf_printf(&sb, " during %s.\n", R4_MSG(ec));
 	} else
 		goto wrong_mc3_mce;
 
@@ -752,7 +758,7 @@ static void decode_mc4_mce(struct mce *m)
 	u8 xec = XEC(m->status, 0x1f);
 	u8 offset = 0;
 
-	pr_emerg(HW_ERR "MC4 Error (node %d): ", node_id);
+	seq_buf_printf(&sb, HW_ERR "MC4 Error (node %d): ", node_id);
 
 	switch (xec) {
 	case 0x0 ... 0xe:
@@ -763,7 +769,7 @@ static void decode_mc4_mce(struct mce *m)
 			if (fam == 0x11)
 				goto wrong_mc4_mce;
 
-			pr_cont("%s.\n", mc4_mce_desc[xec]);
+			seq_buf_printf(&sb, "%s.\n", mc4_mce_desc[xec]);
 
 			if (decode_dram_ecc)
 				decode_dram_ecc(node_id, m);
@@ -773,16 +779,16 @@ static void decode_mc4_mce(struct mce *m)
 
 	case 0xf:
 		if (TLB_ERROR(ec))
-			pr_cont("GART Table Walk data error.\n");
+			seq_buf_printf(&sb, "GART Table Walk data error.\n");
 		else if (BUS_ERROR(ec))
-			pr_cont("DMA Exclusion Vector Table Walk error.\n");
+			seq_buf_printf(&sb, "DMA Exclusion Vector Table Walk error.\n");
 		else
 			goto wrong_mc4_mce;
 		return;
 
 	case 0x19:
 		if (fam == 0x15 || fam == 0x16)
-			pr_cont("Compute Unit Data Error.\n");
+			seq_buf_printf(&sb, "Compute Unit Data Error.\n");
 		else
 			goto wrong_mc4_mce;
 		return;
@@ -795,7 +801,7 @@ static void decode_mc4_mce(struct mce *m)
 		goto wrong_mc4_mce;
 	}
 
-	pr_cont("%s.\n", mc4_mce_desc[xec - offset]);
+	seq_buf_printf(&sb, "%s.\n", mc4_mce_desc[xec - offset]);
 	return;
 
  wrong_mc4_mce:
@@ -811,20 +817,20 @@ static void decode_mc5_mce(struct mce *m)
 	if (fam == 0xf || fam == 0x11)
 		goto wrong_mc5_mce;
 
-	pr_emerg(HW_ERR "MC5 Error: ");
+	seq_buf_printf(&sb, HW_ERR "MC5 Error: ");
 
 	if (INT_ERROR(ec)) {
 		if (xec <= 0x1f) {
-			pr_cont("Hardware Assert.\n");
+			seq_buf_printf(&sb, "Hardware Assert.\n");
 			return;
 		} else
 			goto wrong_mc5_mce;
 	}
 
 	if (xec == 0x0 || xec == 0xc)
-		pr_cont("%s.\n", mc5_mce_desc[xec]);
+		seq_buf_printf(&sb, "%s.\n", mc5_mce_desc[xec]);
 	else if (xec <= 0xd)
-		pr_cont("%s parity error.\n", mc5_mce_desc[xec]);
+		seq_buf_printf(&sb, "%s parity error.\n", mc5_mce_desc[xec]);
 	else
 		goto wrong_mc5_mce;
 
@@ -838,12 +844,12 @@ static void decode_mc6_mce(struct mce *m)
 {
 	u8 xec = XEC(m->status, xec_mask);
 
-	pr_emerg(HW_ERR "MC6 Error: ");
+	seq_buf_printf(&sb, HW_ERR "MC6 Error: ");
 
 	if (xec > 0x5)
 		goto wrong_mc6_mce;
 
-	pr_cont("%s parity error.\n", mc6_mce_desc[xec]);
+	seq_buf_printf(&sb, "%s parity error.\n", mc6_mce_desc[xec]);
 	return;
 
  wrong_mc6_mce:
@@ -871,13 +877,13 @@ static void decode_smca_error(struct mce *m)
 	bank_type = hwid->bank_type;
 	ip_name = smca_get_long_name(bank_type);
 
-	pr_emerg(HW_ERR "%s Extended Error Code: %d\n", ip_name, xec);
+	seq_buf_printf(&sb, HW_ERR "%s Extended Error Code: %d\n", ip_name, xec);
 
 	/* Only print the decode of valid error codes */
 	if (xec < smca_mce_descs[bank_type].num_descs &&
 			(hwid->xec_bitmap & BIT_ULL(xec))) {
-		pr_emerg(HW_ERR "%s Error: ", ip_name);
-		pr_cont("%s.\n", smca_mce_descs[bank_type].descs[xec]);
+		seq_buf_printf(&sb, HW_ERR "%s Error: ", ip_name);
+		seq_buf_printf(&sb, "%s.\n", smca_mce_descs[bank_type].descs[xec]);
 	}
 
 	if (bank_type == SMCA_UMC && xec == 0 && decode_dram_ecc)
@@ -887,25 +893,25 @@ static void decode_smca_error(struct mce *m)
 static inline void amd_decode_err_code(u16 ec)
 {
 	if (INT_ERROR(ec)) {
-		pr_emerg(HW_ERR "internal: %s\n", UU_MSG(ec));
+		seq_buf_printf(&sb, HW_ERR "internal: %s\n", UU_MSG(ec));
 		return;
 	}
 
-	pr_emerg(HW_ERR "cache level: %s", LL_MSG(ec));
+	seq_buf_printf(&sb, HW_ERR "cache level: %s", LL_MSG(ec));
 
 	if (BUS_ERROR(ec))
-		pr_cont(", mem/io: %s", II_MSG(ec));
+		seq_buf_printf(&sb, ", mem/io: %s", II_MSG(ec));
 	else
-		pr_cont(", tx: %s", TT_MSG(ec));
+		seq_buf_printf(&sb, ", tx: %s", TT_MSG(ec));
 
 	if (MEM_ERROR(ec) || BUS_ERROR(ec)) {
-		pr_cont(", mem-tx: %s", R4_MSG(ec));
+		seq_buf_printf(&sb, ", mem-tx: %s", R4_MSG(ec));
 
 		if (BUS_ERROR(ec))
-			pr_cont(", part-proc: %s (%s)", PP_MSG(ec), TO_MSG(ec));
+			seq_buf_printf(&sb, ", part-proc: %s (%s)", PP_MSG(ec), TO_MSG(ec));
 	}
 
-	pr_cont("\n");
+	seq_buf_printf(&sb, "\n");
 }
 
 /*
@@ -948,6 +954,9 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 	if (amd_filter_mce(m))
 		return NOTIFY_STOP;
 
+	/* \0 terminated */
+	seq_buf_init(&sb, __err_buf, BUF_LEN);
+
 	pr_emerg(HW_ERR "%s\n", decode_error_status(m));
 
 	pr_emerg(HW_ERR "CPU:%d (%x:%x:%x) MC%d_STATUS[%s|%s|%s|%s|%s",
@@ -1044,6 +1053,10 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
  err_code:
 	amd_decode_err_code(m->status & 0xffff);
 
+	pr_emerg("%.*s\n", (int)sb.len, sb.buffer);
+
+	seq_buf_clear_buf(&sb);
+
 	return NOTIFY_STOP;
 }
 
-- 
2.14.0.rc0

Re: [RFC PATCH 6/8] EDAC, mce_amd: Convert to seq_buf

[Steven Rostedt]

On Tue, 25 Jul 2017 17:45:59 +0200
Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Convert the part which decodes the error description to the sequence
> buffer facility and thus save ourselves the many printk() invocations
> building the decoded string.
> 
> No functionality change.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  drivers/edac/mce_amd.c | 203 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 108 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
> index a11a671c7a38..1f5e9bb161f3 100644
> --- a/drivers/edac/mce_amd.c
> +++ b/drivers/edac/mce_amd.c
> @@ -1,3 +1,4 @@
> +#include <linux/seq_buf.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  
> @@ -306,6 +307,11 @@ static struct smca_mce_desc smca_mce_descs[] = {
>  	[SMCA_SMU]	= { smca_smu_mce_desc,	ARRAY_SIZE(smca_smu_mce_desc)	},
>  };
>  
> +/* 128 because, well, nice and round - two cachelines. */
> +#define BUF_LEN        128
> +static char __err_buf[BUF_LEN];
> +static struct seq_buf sb;

What happens if two CPUs have mce's at the same time? Wouldn't one
corrupt the other buffer. 128 isn't too big to put on the stack is it?

-- Steve

> +
>  static bool f12h_mc0_mce(u16 ec, u8 xec)
>  {

Re: [RFC PATCH 6/8] EDAC, mce_amd: Convert to seq_buf

[Borislav Petkov]

On Thu, Jul 27, 2017 at 09:47:08PM -0400, Steven Rostedt wrote:
> What happens if two CPUs have mce's at the same time? Wouldn't one
> corrupt the other buffer. 128 isn't too big to put on the stack is it?

Yeah, putting it on the stack is probably safer, just in case.

What is even better, though, is if I extended
arch/x86/kernel/cpu/mcheck/mce-genpool.c to allocate a second buffer for the
decoded strings. We use it for the struct mces right now.

And 1-2 pages should be fine:

8192 / 128 = 64 decoded strings in flight.

I guess that should cover most situations. Famous last words.

In any case, thanks for pointing this out.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [RFC PATCH 6/8] EDAC, mce_amd: Convert to seq_buf

[Borislav Petkov]

On Fri, Jul 28, 2017 at 09:09:33AM +0200, Borislav Petkov wrote:
> On Thu, Jul 27, 2017 at 09:47:08PM -0400, Steven Rostedt wrote:
> > What happens if two CPUs have mce's at the same time? Wouldn't one
> > corrupt the other buffer. 128 isn't too big to put on the stack is it?
> 
> Yeah, putting it on the stack is probably safer, just in case.
> 
> What is even better, though, is if I extended
> arch/x86/kernel/cpu/mcheck/mce-genpool.c to allocate a second buffer for the
> decoded strings. We use it for the struct mces right now.

Here's a conversion to a 2-page backed genpool. Seems to work:

---
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index 90eabb1e4a27..fd4a615200a8 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -1,3 +1,4 @@
+#include <linux/genalloc.h>
 #include <linux/seq_buf.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -37,6 +38,16 @@ void amd_unregister_ecc_decoder(void (*f)(int, struct mce *))
 }
 EXPORT_SYMBOL_GPL(amd_unregister_ecc_decoder);
 
+/* 128 because, well, nice and round - two cachelines. */
+#define ELEM_ORDER	7
+#define ELEM_SIZE	(1 << 7)
+#define DEC_POOL_SIZE	(2 * PAGE_SIZE)
+
+static char __err_buf[DEC_POOL_SIZE];
+static struct gen_pool *dec_pool;
+
+static struct seq_buf sb;
+
 /*
  * string representation for the different MCA reported error types, see F3x48
  * or MSR0000_0411.
@@ -309,11 +320,6 @@ static struct smca_mce_desc smca_mce_descs[] = {
 	[SMCA_SMU]	= { smca_smu_mce_desc,	ARRAY_SIZE(smca_smu_mce_desc)	},
 };
 
-/* 128 because, well, nice and round - two cachelines. */
-#define BUF_LEN        128
-static char __err_buf[BUF_LEN];
-static struct seq_buf sb;
-
 static bool f12h_mc0_mce(u16 ec, u8 xec)
 {
 	bool ret = false;
@@ -1050,6 +1056,7 @@ static int
 amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 {
 	struct mce *m = (struct mce *)data;
+	char *dec_buf;
 
 	if (amd_filter_mce(m))
 		return NOTIFY_STOP;
@@ -1057,8 +1064,15 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 	if (!ras_userspace_consumers())
 		__decode_mce(m);
 
+	dec_buf = (void *)gen_pool_alloc(dec_pool, ELEM_SIZE);
+	if (!dec_buf) {
+		pr_warn_ratelimited("Decode buffer full!\n");
+		return NOTIFY_STOP;
+	}
+
 	/* \0 terminated */
-	seq_buf_init(&sb, __err_buf, BUF_LEN);
+	seq_buf_init(&sb, dec_buf, ELEM_SIZE);
+	seq_buf_clear_buf(&sb);
 
 	if (boot_cpu_has(X86_FEATURE_SMCA))
 		decode_smca_error(m);
@@ -1074,12 +1088,9 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 
 		while ((l = strsep(&sb.buffer, "\n")))
 			pr_emerg(HW_ERR "%s\n", l);
-
-		/* Restore original address because strsep() mangles it. */
-		sb.buffer = __err_buf;
 	}
 
-	seq_buf_clear_buf(&sb);
+	gen_pool_free(dec_pool, (unsigned long)dec_buf, ELEM_SIZE);
 
 	return NOTIFY_STOP;
 }
@@ -1092,6 +1103,7 @@ static struct notifier_block amd_mce_dec_nb = {
 static int __init mce_amd_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
+	int ret;
 
 	if (c->x86_vendor != X86_VENDOR_AMD)
 		return -ENODEV;
@@ -1100,6 +1112,16 @@ static int __init mce_amd_init(void)
 	if (!fam_ops)
 		return -ENOMEM;
 
+	dec_pool = gen_pool_create(ELEM_ORDER, -1);
+	if (!dec_pool)
+		goto err_out;
+
+	ret = gen_pool_add(dec_pool, (unsigned long)__err_buf, DEC_POOL_SIZE, -1);
+	if (ret) {
+		gen_pool_destroy(dec_pool);
+		goto err_out;
+	}
+
 	switch (c->x86) {
 	case 0xf:
 		fam_ops->mc0_mce = k8_mc0_mce;
@@ -1177,6 +1199,7 @@ static void __exit mce_amd_exit(void)
 {
 	mce_unregister_decode_chain(&amd_mce_dec_nb);
 	kfree(fam_ops);
+	gen_pool_destroy(dec_pool);
 }
 
 MODULE_DESCRIPTION("AMD MCE decoder");

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [RFC PATCH 6/8] EDAC, mce_amd: Convert to seq_buf

[Steven Rostedt]

On Fri, 28 Jul 2017 12:51:40 +0200
Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jul 28, 2017 at 09:09:33AM +0200, Borislav Petkov wrote:
> > On Thu, Jul 27, 2017 at 09:47:08PM -0400, Steven Rostedt wrote:  
> > > What happens if two CPUs have mce's at the same time? Wouldn't one
> > > corrupt the other buffer. 128 isn't too big to put on the stack is it?  
> > 
> > Yeah, putting it on the stack is probably safer, just in case.
> > 
> > What is even better, though, is if I extended
> > arch/x86/kernel/cpu/mcheck/mce-genpool.c to allocate a second buffer for the
> > decoded strings. We use it for the struct mces right now.  
> 
> Here's a conversion to a 2-page backed genpool. Seems to work:

Interesting, this is the first I heard of the genpool. I probably could
have used this in other code. Good to know (learn something new every
day :-)

I'll have to take a look at it when I get home later today (still
bouncing between airports).

-- Steve

Re: [RFC PATCH 6/8] EDAC, mce_amd: Convert to seq_buf

[Borislav Petkov]

On Fri, Jul 28, 2017 at 08:59:59AM -0400, Steven Rostedt wrote:
> Interesting, this is the first I heard of the genpool. I probably could
> have used this in other code. Good to know (learn something new every
> day :-)

That's a safe assumption for each one of us. :)

> I'll have to take a look at it when I get home later today (still
> bouncing between airports).

Yeah, I'm not even using all the features - just the NMI-safeness.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[RFC PATCH 7/8] EDAC, mce_amd: Add a simple tracepoint dumping a decoded string

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

It is a single string which gets dynamically generated when the error
gets decoded. Dump it to userspace through that tracepoint so that
consumers can get the already decoded string and the kernel has the
decoding functionality too, even if there are no userspace consumers.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 drivers/edac/mce_amd.c  |  7 ++++++-
 drivers/ras/ras.c       |  1 +
 include/ras/ras_event.h | 16 ++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index 1f5e9bb161f3..ce7e20ca6773 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -1,6 +1,8 @@
 #include <linux/seq_buf.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/ras.h>
+#include <ras/ras_event.h>
 
 #include <asm/cpu.h>
 
@@ -1053,7 +1055,10 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
  err_code:
 	amd_decode_err_code(m->status & 0xffff);
 
-	pr_emerg("%.*s\n", (int)sb.len, sb.buffer);
+	if (ras_userspace_consumers())
+		trace_mce_decode(sb.buffer);
+	else
+		pr_emerg("%.*s\n", (int)sb.len, sb.buffer);
 
 	seq_buf_clear_buf(&sb);
 
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index 5429d3795732..a09e39b3f711 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -42,6 +42,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
 EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
 EXPORT_TRACEPOINT_SYMBOL_GPL(non_standard_event);
 EXPORT_TRACEPOINT_SYMBOL_GPL(arm_event);
+EXPORT_TRACEPOINT_SYMBOL_GPL(mce_decode);
 
 static int __init parse_ras_param(char *str)
 {
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 429f46fb61e4..113df73f7ba0 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -407,6 +407,22 @@ TRACE_EVENT(memory_failure_event,
 	)
 );
 #endif /* CONFIG_MEMORY_FAILURE */
+
+TRACE_EVENT(mce_decode,
+	TP_PROTO(const char *param_str),
+
+	TP_ARGS(param_str),
+
+	TP_STRUCT__entry(
+		__string(str, param_str)
+	),
+
+	TP_fast_assign(
+		__assign_str(str, param_str);
+	),
+
+	TP_printk("%s", __get_str(str))
+);
 #endif /* _TRACE_HW_EVENT_MC_H */
 
 /* This part must be outside protection */
-- 
2.14.0.rc0

Re: [RFC PATCH 7/8] EDAC, mce_amd: Add a simple tracepoint dumping a decoded string

[Steven Rostedt]

On Tue, 25 Jul 2017 17:46:00 +0200
Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> It is a single string which gets dynamically generated when the error
> gets decoded. Dump it to userspace through that tracepoint so that
> consumers can get the already decoded string and the kernel has the
> decoding functionality too, even if there are no userspace consumers.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  drivers/edac/mce_amd.c  |  7 ++++++-
>  drivers/ras/ras.c       |  1 +
>  include/ras/ras_event.h | 16 ++++++++++++++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
> index 1f5e9bb161f3..ce7e20ca6773 100644
> --- a/drivers/edac/mce_amd.c
> +++ b/drivers/edac/mce_amd.c
> @@ -1,6 +1,8 @@
>  #include <linux/seq_buf.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/ras.h>
> +#include <ras/ras_event.h>
>  
>  #include <asm/cpu.h>
>  
> @@ -1053,7 +1055,10 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
>   err_code:
>  	amd_decode_err_code(m->status & 0xffff);
>  
> -	pr_emerg("%.*s\n", (int)sb.len, sb.buffer);
> +	if (ras_userspace_consumers())
> +		trace_mce_decode(sb.buffer);
> +	else
> +		pr_emerg("%.*s\n", (int)sb.len, sb.buffer);
>  
>  	seq_buf_clear_buf(&sb);
>  
> diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
> index 5429d3795732..a09e39b3f711 100644
> --- a/drivers/ras/ras.c
> +++ b/drivers/ras/ras.c
> @@ -42,6 +42,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(non_standard_event);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(arm_event);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(mce_decode);
>  
>  static int __init parse_ras_param(char *str)
>  {
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 429f46fb61e4..113df73f7ba0 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -407,6 +407,22 @@ TRACE_EVENT(memory_failure_event,
>  	)
>  );
>  #endif /* CONFIG_MEMORY_FAILURE */
> +
> +TRACE_EVENT(mce_decode,
> +	TP_PROTO(const char *param_str),
> +
> +	TP_ARGS(param_str),
> +
> +	TP_STRUCT__entry(
> +		__string(str, param_str)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(str, param_str);
> +	),
> +
> +	TP_printk("%s", __get_str(str))
> +);
>  #endif /* _TRACE_HW_EVENT_MC_H */
>  
>  /* This part must be outside protection */

Re: [RFC PATCH 7/8] EDAC, mce_amd: Add a simple tracepoint dumping a decoded string

[Borislav Petkov]

On Thu, Jul 27, 2017 at 09:47:59PM -0400, Steven Rostedt wrote:
> On Tue, 25 Jul 2017 17:46:00 +0200
> Borislav Petkov <bp@alien8.de> wrote:
> 
> > From: Borislav Petkov <bp@suse.de>
> > 
> > It is a single string which gets dynamically generated when the error
> > gets decoded. Dump it to userspace through that tracepoint so that
> > consumers can get the already decoded string and the kernel has the
> > decoding functionality too, even if there are no userspace consumers.
> > 
> > Signed-off-by: Borislav Petkov <bp@suse.de>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> 
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Right, see

 https://lkml.kernel.org/r/20170727130909.GB28548@nazgul.tnic

I had the idea while talking to Ingo yesterday.

I should simply extend trace_mce_record() with one more arg. Much cleaner.

Thx.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[RFC PATCH 8/8] EDAC, mce_amd: Issue the decoded info through the TP or printk

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

... depending on whether we have userspace consumers. Handle the HW_ERR
prefix accordingly - for printk() I need to do some strsep() monkey
business.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/edac/mce_amd.c | 85 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 52 insertions(+), 33 deletions(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index ce7e20ca6773..90eabb1e4a27 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -461,7 +461,7 @@ static void decode_mc0_mce(struct mce *m)
 	u16 ec = EC(m->status);
 	u8 xec = XEC(m->status, xec_mask);
 
-	seq_buf_printf(&sb, HW_ERR "MC0 Error: ");
+	seq_buf_printf(&sb, "MC0 Error: ");
 
 	/* TLB error signatures are the same across families */
 	if (TLB_ERROR(ec)) {
@@ -571,7 +571,7 @@ static void decode_mc1_mce(struct mce *m)
 	u16 ec = EC(m->status);
 	u8 xec = XEC(m->status, xec_mask);
 
-	seq_buf_printf(&sb, HW_ERR "MC1 Error: ");
+	seq_buf_printf(&sb, "MC1 Error: ");
 
 	if (TLB_ERROR(ec))
 		seq_buf_printf(&sb, "%s TLB %s.\n", LL_MSG(ec),
@@ -717,7 +717,7 @@ static void decode_mc2_mce(struct mce *m)
 	u16 ec = EC(m->status);
 	u8 xec = XEC(m->status, xec_mask);
 
-	seq_buf_printf(&sb, HW_ERR "MC2 Error: ");
+	seq_buf_printf(&sb, "MC2 Error: ");
 
 	if (!fam_ops->mc2_mce(ec, xec))
 		pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
@@ -734,7 +734,7 @@ static void decode_mc3_mce(struct mce *m)
 		return;
 	}
 
-	seq_buf_printf(&sb, HW_ERR "MC3 Error");
+	seq_buf_printf(&sb, "MC3 Error");
 
 	if (xec == 0x0) {
 		u8 r4 = R4(ec);
@@ -760,7 +760,7 @@ static void decode_mc4_mce(struct mce *m)
 	u8 xec = XEC(m->status, 0x1f);
 	u8 offset = 0;
 
-	seq_buf_printf(&sb, HW_ERR "MC4 Error (node %d): ", node_id);
+	seq_buf_printf(&sb, "MC4 Error (node %d): ", node_id);
 
 	switch (xec) {
 	case 0x0 ... 0xe:
@@ -819,7 +819,7 @@ static void decode_mc5_mce(struct mce *m)
 	if (fam == 0xf || fam == 0x11)
 		goto wrong_mc5_mce;
 
-	seq_buf_printf(&sb, HW_ERR "MC5 Error: ");
+	seq_buf_printf(&sb, "MC5 Error: ");
 
 	if (INT_ERROR(ec)) {
 		if (xec <= 0x1f) {
@@ -846,7 +846,7 @@ static void decode_mc6_mce(struct mce *m)
 {
 	u8 xec = XEC(m->status, xec_mask);
 
-	seq_buf_printf(&sb, HW_ERR "MC6 Error: ");
+	seq_buf_printf(&sb, "MC6 Error: ");
 
 	if (xec > 0x5)
 		goto wrong_mc6_mce;
@@ -879,12 +879,12 @@ static void decode_smca_error(struct mce *m)
 	bank_type = hwid->bank_type;
 	ip_name = smca_get_long_name(bank_type);
 
-	seq_buf_printf(&sb, HW_ERR "%s Extended Error Code: %d\n", ip_name, xec);
+	seq_buf_printf(&sb, "%s Extended Error Code: %d\n", ip_name, xec);
 
 	/* Only print the decode of valid error codes */
 	if (xec < smca_mce_descs[bank_type].num_descs &&
 			(hwid->xec_bitmap & BIT_ULL(xec))) {
-		seq_buf_printf(&sb, HW_ERR "%s Error: ", ip_name);
+		seq_buf_printf(&sb, "%s Error: ", ip_name);
 		seq_buf_printf(&sb, "%s.\n", smca_mce_descs[bank_type].descs[xec]);
 	}
 
@@ -895,11 +895,11 @@ static void decode_smca_error(struct mce *m)
 static inline void amd_decode_err_code(u16 ec)
 {
 	if (INT_ERROR(ec)) {
-		seq_buf_printf(&sb, HW_ERR "internal: %s\n", UU_MSG(ec));
+		seq_buf_printf(&sb, "internal: %s\n", UU_MSG(ec));
 		return;
 	}
 
-	seq_buf_printf(&sb, HW_ERR "cache level: %s", LL_MSG(ec));
+	seq_buf_printf(&sb, "cache level: %s", LL_MSG(ec));
 
 	if (BUS_ERROR(ec))
 		seq_buf_printf(&sb, ", mem/io: %s", II_MSG(ec));
@@ -912,8 +912,6 @@ static inline void amd_decode_err_code(u16 ec)
 		if (BUS_ERROR(ec))
 			seq_buf_printf(&sb, ", part-proc: %s (%s)", PP_MSG(ec), TO_MSG(ec));
 	}
-
-	seq_buf_printf(&sb, "\n");
 }
 
 /*
@@ -946,19 +944,11 @@ static const char *decode_error_status(struct mce *m)
 	return "Corrected error, no action required.";
 }
 
-static int
-amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
+static void __decode_mce(struct mce *m)
 {
-	struct mce *m = (struct mce *)data;
 	unsigned int fam = x86_family(m->cpuid);
 	int ecc;
 
-	if (amd_filter_mce(m))
-		return NOTIFY_STOP;
-
-	/* \0 terminated */
-	seq_buf_init(&sb, __err_buf, BUF_LEN);
-
 	pr_emerg(HW_ERR "%s\n", decode_error_status(m));
 
 	pr_emerg(HW_ERR "CPU:%d (%x:%x:%x) MC%d_STATUS[%s|%s|%s|%s|%s",
@@ -999,7 +989,12 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 	pr_cont("]: 0x%016llx\n", m->status);
 
 	if (m->status & MCI_STATUS_ADDRV)
-		pr_emerg(HW_ERR "Error Addr: 0x%016llx\n", m->addr);
+		pr_emerg(HW_ERR "Error Addr: 0x%016llx ", m->addr);
+
+	if (m->tsc)
+		pr_cont(HW_ERR "TSC: %llu", m->tsc);
+
+	pr_cont("\n");
 
 	if (boot_cpu_has(X86_FEATURE_SMCA)) {
 		pr_emerg(HW_ERR "IPID: 0x%016llx", m->ipid);
@@ -1008,16 +1003,14 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 			pr_cont(", Syndrome: 0x%016llx", m->synd);
 
 		pr_cont("\n");
-
-		decode_smca_error(m);
-		goto err_code;
 	}
 
-	if (m->tsc)
-		pr_emerg(HW_ERR "TSC: %llu\n", m->tsc);
+}
 
+static void decode_legacy_error(struct mce *m)
+{
 	if (!fam_ops)
-		goto err_code;
+		return;
 
 	switch (m->bank) {
 	case 0:
@@ -1051,14 +1044,40 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 	default:
 		break;
 	}
+}
+
+static int
+amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
+{
+	struct mce *m = (struct mce *)data;
+
+	if (amd_filter_mce(m))
+		return NOTIFY_STOP;
+
+	if (!ras_userspace_consumers())
+		__decode_mce(m);
+
+	/* \0 terminated */
+	seq_buf_init(&sb, __err_buf, BUF_LEN);
+
+	if (boot_cpu_has(X86_FEATURE_SMCA))
+		decode_smca_error(m);
+	else
+		decode_legacy_error(m);
 
- err_code:
 	amd_decode_err_code(m->status & 0xffff);
 
-	if (ras_userspace_consumers())
+	if (ras_userspace_consumers()) {
 		trace_mce_decode(sb.buffer);
-	else
-		pr_emerg("%.*s\n", (int)sb.len, sb.buffer);
+	} else {
+		char *l;
+
+		while ((l = strsep(&sb.buffer, "\n")))
+			pr_emerg(HW_ERR "%s\n", l);
+
+		/* Restore original address because strsep() mangles it. */
+		sb.buffer = __err_buf;
+	}
 
 	seq_buf_clear_buf(&sb);
 
-- 
2.14.0.rc0

Re: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Hi,
> 
> here's a first stab at adding a tracepoint which dumps the decoded MCE
> string to userspace. The main idea is to have the decoding functionality
> in the kernel and depending on whether you have userspace consumers
> listening or not, to dump the error to the tracepoint or to dmesg.
> 
> In either case, we do the decoding in the kernel and don't need special
> userspace. Furthermore, adding new CPU support will have to be done only
> in one place.
> 
> First 6 patches are cleanups which are good to have regardless, IMO.
> 
> Any constructive comments and suggestions are appreciated.
> 
> Thanks.
> 
> P.S., Thanks to Rostedt for the input!
> 
> Borislav Petkov (8):
>   EDAC, mce_amd: Rename decode_smca_errors() to decode_smca_error()
>   EDAC, mce_amd: Get rid of most struct cpuinfo_x86 uses
>   EDAC, mce_amd: Get rid of local var in amd_filter_mce()
>   seq_buf: Add seq_buf_clear_buf()
>   seq_buf: Export seq_buf_printf() to modules
>   EDAC, mce_amd: Convert to seq_buf
>   EDAC, mce_amd: Add a simple tracepoint dumping a decoded string
>   EDAC, mce_amd: Issue the decoded info through the TP or printk
> 
>  drivers/edac/mce_amd.c  | 285 +++++++++++++++++++++++++++---------------------
>  drivers/ras/ras.c       |   1 +
>  include/linux/seq_buf.h |   7 ++
>  include/ras/ras_event.h |  16 +++
>  lib/seq_buf.c           |   1 +
>  5 files changed, 186 insertions(+), 124 deletions(-)

Looks pretty nice to me conceptually. Do you have a couple of examples of 
real-life events that get logged? It's hard to decode it from the new tracepoint 
alone.

Thanks,

	Ingo

Re: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Borislav Petkov]

On Thu, Jul 27, 2017 at 09:10:34AM +0200, Ingo Molnar wrote:
> Looks pretty nice to me conceptually. Do you have a couple of examples of 
> real-life events that get logged? It's hard to decode it from the new tracepoint 
> alone.

Here's what comes out in dmesg:

[  932.370319] mce: [Hardware Error]: Machine check events logged
[  932.374474] [Hardware Error]: Corrected error, no action required.
[  932.381684] [Hardware Error]: CPU:1 (0:0:0) MC5_STATUS[Over|CE|MiscV|-|AddrV|CECC]: 0xdc00410000020f0f
[  932.384256] [Hardware Error]: Error Addr: 0x0000000056071033 [Hardware Error]: TSC: 2703436211649
[  932.386608] [Hardware Error]: MC5 Error: AG payload array parity error.
[  932.388425] [Hardware Error]: cache level: L3/GEN, mem/io: GEN, mem-tx: GEN, part-proc: GEN (timed out)

(whoops, that TSC thing should be on a new line).

and the TP dumps only the last two lines:

[  932.386608] [Hardware Error]: MC5 Error: AG payload array parity error.
[  932.388425] [Hardware Error]: cache level: L3/GEN, mem/io: GEN, mem-tx: GEN, part-proc: GEN (timed out)

but come to think of it, it should dump only the MC? Error line because
the last line can be easily deduced from the error code. I'll change
that.

Btw, the reason why I'm dumping only MC? line is to keep the string
going into the TP relatively small. It is 128 bytes now. I tried dumping
the whole decoded string but that easily overflowed 256 bytes and 256
bytes is already a bit too much to log into the trace buffers.

So I'm concentrating only on the not-very-trivial stuff to decode.

The rest is being deduced directly from the MCi_STATUS value anyway
which we can easily do in userspace and that is straightforward. And
that u64 value we already dump with trace_mce_record().

So the idea is, userspace opens trace_mce_record() to get the raw MCE
data and then this second TP to get the decoded string of what that
error is.

Later, we could extend that same behavior to Intel for the common
errors, at least, so that we can dump at least *some* string explaining
what the error is.

Anyway, something like that is swirling in my head right now...

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Jul 27, 2017 at 09:10:34AM +0200, Ingo Molnar wrote:
> > Looks pretty nice to me conceptually. Do you have a couple of examples of 
> > real-life events that get logged? It's hard to decode it from the new tracepoint 
> > alone.
> 
> Here's what comes out in dmesg:
> 
> [  932.370319] mce: [Hardware Error]: Machine check events logged
> [  932.374474] [Hardware Error]: Corrected error, no action required.
> [  932.381684] [Hardware Error]: CPU:1 (0:0:0) MC5_STATUS[Over|CE|MiscV|-|AddrV|CECC]: 0xdc00410000020f0f
> [  932.384256] [Hardware Error]: Error Addr: 0x0000000056071033 [Hardware Error]: TSC: 2703436211649
> [  932.386608] [Hardware Error]: MC5 Error: AG payload array parity error.
> [  932.388425] [Hardware Error]: cache level: L3/GEN, mem/io: GEN, mem-tx: GEN, part-proc: GEN (timed out)
> 
> (whoops, that TSC thing should be on a new line).
> 
> and the TP dumps only the last two lines:
> 
> [  932.386608] [Hardware Error]: MC5 Error: AG payload array parity error.
> [  932.388425] [Hardware Error]: cache level: L3/GEN, mem/io: GEN, mem-tx: GEN, part-proc: GEN (timed out)
> 
> but come to think of it, it should dump only the MC? Error line because
> the last line can be easily deduced from the error code. I'll change
> that.

I think the more human readable the raw trace data is, the better - but it should 
also have a structure that makes it straightforward to be parsed by tooling.

> Btw, the reason why I'm dumping only MC? line is to keep the string
> going into the TP relatively small. It is 128 bytes now. I tried dumping
> the whole decoded string but that easily overflowed 256 bytes and 256
> bytes is already a bit too much to log into the trace buffers.

I don't think so: we routinely log several KB per event worth of call chains via 
perf tracing just fine.

So I'd suggest logging more than less, and making it more verbose is definitely 
the way to go.

For example I absolutely hate systemd's obscure error patterns and its general 
passive-aggressive behavior towards failure analysis and troubleshooting for 
example.

Just a random example of systemd (version 232) troubleshooting UI fail:

  root@triton:~# systemctl start rsyslog; echo $?
  0
  root@triton:~# systemctl start rsyslog; echo $?
  0

Was the rsyslog service started successfully? Yes/no?

And if I try to stop it:

  root@triton:~# systemctl stop rsyslog; echo $?
  Warning: Stopping rsyslog.service, but it can still be activated by:
    syslog.socket
  0

... the text suggests that it got stopped, and the command returns 0 which usually 
means success. It warns about some service possible activating it, but it does not 
tell whether it actually _is_ active after this command, or got stopped. I mean, 
systemd should be very much aware of what the real situation is, and could 
volunteer all that helpful info - instead it's being passively obtuse.

Turns out the text is misleading and the service did not get stopped:

  root@triton:~# ps aux | grep -i rsys
  syslog   27311  0.0  0.0 256416  3256 ?        Ssl  10:32   0:00 /usr/sbin/rsyslogd -n

Or another UI idiocy:

  root@triton:~# systemctl -v
  systemctl: invalid option -- 'v'

  root@triton:~# systemctl --version
  systemd 232
  +PAM +AUDIT +SELINUX +IMA +APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ +LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD +IDN

i.e. the systemd developers implemented --version, but they did not do the trivial 
mapping of it to the customary (and unused) -v alias...

etc. etc.

So let's not repeat these kinds of mistakes... be as informative and helpful to 
the user as programmatically possible.

Thanks,

	Ingo

Re: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Borislav Petkov]

On Thu, Jul 27, 2017 at 10:39:27AM +0200, Ingo Molnar wrote:
> I don't think so: we routinely log several KB per event worth of call chains via 
> perf tracing just fine.
> 
> So I'd suggest logging more than less, and making it more verbose is definitely 
> the way to go.

No no, you're missing the point: we do dump *all* that information - it
is just spread between two tracepoints - trace_mce_record() and this new
one.

BUT(!), I just realized, I *think* I can address this much more
elegantly: extend trace_mce_record() by adding the decoded string as
its last argument. And that's fine, I'm being told, because adding
arguments to the tracepoints is not a big deal, removing them is hard.
And actually, we have added args before, come to think of it:

5828c46f2c07 x86/mce/AMD: Save MCA_IPID in MCE struct on SMCA systems
db819d60f672 x86/mce: Add support for new MCA_SYND register

So lemme look into that - it would be much nicer this way because I
don't need to add a second TP at all.

I agree with the rest but you're obviously preaching to the choir.

:-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> BUT(!), I just realized, I *think* I can address this much more elegantly: 
> extend trace_mce_record() by adding the decoded string as its last argument. And 
> that's fine, I'm being told, because adding arguments to the tracepoints is not 
> a big deal, removing them is hard. And actually, we have added args before, come 
> to think of it:

Yeah, structured, append-only ABIs are elegant - that's what perf uses too.

> I agree with the rest but you're obviously preaching to the choir.
> 
> :-)

Had do vent my (non kernel tree integrated) Linux tooling frustration!! ;-)

Thanks,

	Ingo

Re: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Borislav Petkov]

On Fri, Jul 28, 2017 at 08:37:53AM +0200, Ingo Molnar wrote:
> Yeah, structured, append-only ABIs are elegant - that's what perf uses too.

Yeah.

> Had do vent my (non kernel tree integrated) Linux tooling frustration!! ;-)

I know *exactly* what you mean!

:-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Borislav Petkov]

Ok,

here's a working version. It looks pretty straight-forward (to me, at
least) and it does what it is supposed to when I inject an MCE:

# tracer: nop
#
#                              _-----=> irqs-off
#                             / _----=> need-resched
#                            | / _---=> hardirq/softirq
#                            || / _--=> preempt-depth
#                            ||| /     delay
#           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
#              | |       |   ||||       |         |
     kworker/1:2-76    [001] ....   316.852022: mce_record: CPU: 1, MCGc/s: 100010a/0, MC5: dc00410000020f0f, IPID: 0000000000000000, ADDR/MISC/SYND: 0000000056071033/0000000000000000/0000000000000000, RIP: 00:<0000000000000000>, TSC: 0, PROCESSOR: 2:600f20, TIME: 1501261272, SOCKET: 1, APIC: 1 MC5 Error: AG payload array parity error.
cache level: L3/GEN, mem/io: GEN, mem-tx: GEN, part-proc: GEN (timed out)
     kworker/1:2-76    [001] ....   328.372055: mce_record: CPU: 1, MCGc/s: 100010a/0, MC5: dc00410000020f0f, IPID: 0000000000000000, ADDR/MISC/SYND: 0000000056071033/0000000000000000/0000000000000000, RIP: 00:<0000000000000000>, TSC: 0, PROCESSOR: 2:600f20, TIME: 1501261283, SOCKET: 1, APIC: 1 MC5 Error: AG payload array parity error.
cache level: L3/GEN, mem/io: GEN, mem-tx: GEN, part-proc: GEN (timed out)

diff ontop of the current pile:

---
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 181264989db5..ba4c99ace544 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -146,6 +146,7 @@ struct mca_config {
 	bool ser;
 	bool recovery;
 	bool bios_cmci_threshold;
+	bool has_decoder;
 	u8 banks;
 	s8 bootlog;
 	int tolerant;
@@ -195,7 +196,8 @@ enum mce_notifier_prios {
 	MCE_PRIO_SRAO		= INT_MAX - 1,
 	MCE_PRIO_EXTLOG		= INT_MAX - 2,
 	MCE_PRIO_NFIT		= INT_MAX - 3,
-	MCE_PRIO_EDAC		= INT_MAX - 4,
+	MCE_PRIO_DECODER	= INT_MAX - 4,
+	MCE_PRIO_EDAC		= INT_MAX - 5,
 	MCE_PRIO_MCELOG		= 1,
 	MCE_PRIO_LOWEST		= 0,
 };
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 6dde0497efc7..9486a2ca6556 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -162,6 +162,9 @@ void mce_register_decode_chain(struct notifier_block *nb)
 
 	atomic_inc(&num_notifiers);
 
+	if (nb->priority == MCE_PRIO_DECODER)
+		mca_cfg.has_decoder = true;
+
 	blocking_notifier_chain_register(&x86_mce_decoder_chain, nb);
 }
 EXPORT_SYMBOL_GPL(mce_register_decode_chain);
@@ -556,7 +559,8 @@ static int mce_first_notifier(struct notifier_block *nb, unsigned long val,
 		return NOTIFY_STOP;
 
 	/* Emit the trace record: */
-	trace_mce_record(m);
+	if (!mca_cfg.has_decoder)
+		trace_mce_record(m, "");
 
 	set_bit(0, &mce_need_notify);
 
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index fd4a615200a8..d0b28edd72b8 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -7,6 +7,8 @@
 
 #include <asm/cpu.h>
 
+#include <trace/events/mce.h>
+
 #include "mce_amd.h"
 
 static struct amd_decoder_ops *fam_ops;
@@ -1082,7 +1084,7 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 	amd_decode_err_code(m->status & 0xffff);
 
 	if (ras_userspace_consumers()) {
-		trace_mce_decode(sb.buffer);
+		trace_mce_record(m, sb.buffer);
 	} else {
 		char *l;
 
@@ -1097,7 +1099,7 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 
 static struct notifier_block amd_mce_dec_nb = {
 	.notifier_call	= amd_decode_mce,
-	.priority	= MCE_PRIO_EDAC,
+	.priority	= MCE_PRIO_DECODER,
 };
 
 static int __init mce_amd_init(void)
diff --git a/include/trace/events/mce.h b/include/trace/events/mce.h
index 70f02149808c..82ff4014fab1 100644
--- a/include/trace/events/mce.h
+++ b/include/trace/events/mce.h
@@ -10,9 +10,9 @@
 
 TRACE_EVENT(mce_record,
 
-	TP_PROTO(struct mce *m),
+	TP_PROTO(struct mce *m, const char *param_str),
 
-	TP_ARGS(m),
+	TP_ARGS(m, param_str),
 
 	TP_STRUCT__entry(
 		__field(	u64,		mcgcap		)
@@ -32,6 +32,7 @@ TRACE_EVENT(mce_record,
 		__field(	u8,		cs		)
 		__field(	u8,		bank		)
 		__field(	u8,		cpuvendor	)
+		__string(	str,		param_str	)
 	),
 
 	TP_fast_assign(
@@ -52,9 +53,10 @@ TRACE_EVENT(mce_record,
 		__entry->cs		= m->cs;
 		__entry->bank		= m->bank;
 		__entry->cpuvendor	= m->cpuvendor;
+		__assign_str(str, param_str);
 	),
 
-	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, ADDR/MISC/SYND: %016Lx/%016Lx/%016Lx, RIP: %02x:<%016Lx>, TSC: %llx, PROCESSOR: %u:%x, TIME: %llu, SOCKET: %u, APIC: %x",
+	TP_printk("CPU: %d, MCGc/s: %llx/%llx, MC%d: %016Lx, IPID: %016Lx, ADDR/MISC/SYND: %016Lx/%016Lx/%016Lx, RIP: %02x:<%016Lx>, TSC: %llx, PROCESSOR: %u:%x, TIME: %llu, SOCKET: %u, APIC: %x %s",
 		__entry->cpu,
 		__entry->mcgcap, __entry->mcgstatus,
 		__entry->bank, __entry->status,
@@ -65,7 +67,8 @@ TRACE_EVENT(mce_record,
 		__entry->cpuvendor, __entry->cpuid,
 		__entry->walltime,
 		__entry->socketid,
-		__entry->apicid)
+		__entry->apicid,
+		__get_str(str))
 );
 
 #endif /* _TRACE_MCE_H */

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Borislav Petkov]

On Fri, Jul 28, 2017 at 05:08:50PM +0200, Borislav Petkov wrote:
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 6dde0497efc7..9486a2ca6556 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -162,6 +162,9 @@ void mce_register_decode_chain(struct notifier_block *nb)
>  
>  	atomic_inc(&num_notifiers);
>  
> +	if (nb->priority == MCE_PRIO_DECODER)
> +		mca_cfg.has_decoder = true;
> +
>  	blocking_notifier_chain_register(&x86_mce_decoder_chain, nb);
>  }
>  EXPORT_SYMBOL_GPL(mce_register_decode_chain);

Forgot the counterpart:

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 9486a2ca6556..974e9c931e6f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -173,6 +173,9 @@ void mce_unregister_decode_chain(struct notifier_block *nb)
 {
 	atomic_dec(&num_notifiers);
 
+	if (nb->priority == MCE_PRIO_DECODER)
+		mca_cfg.has_decoder = false;
+
 	blocking_notifier_chain_unregister(&x86_mce_decoder_chain, nb);
 }
 EXPORT_SYMBOL_GPL(mce_unregister_decode_chain);

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Luck, Tony]

> Later, we could extend that same behavior to Intel for the common
> errors, at least, so that we can dump at least *some* string explaining
> what the error is.

s/common errors/architectural errors/

That means we don't need to keep updating for every Xeon that documents
some MCi_STATUS.MSCOD bits.  Decoding the MCACOD bits will explain
which component is involved (cache, bus, memory) and some detail on the
type of access.

-Tony

Re: [RFC PATCH 0/8] EDAC, mce_amd: Add a tracepoint for the decoded error

[Borislav Petkov]

On Thu, Jul 27, 2017 at 04:42:19PM +0000, Luck, Tony wrote:
> s/common errors/architectural errors/
> 
> That means we don't need to keep updating for every Xeon that documents
> some MCi_STATUS.MSCOD bits.  Decoding the MCACOD bits will explain
> which component is involved (cache, bus, memory) and some detail on the
> type of access.

Yeah, exactly. Let's take care of the low-hanging fruits first.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--