trace, RAS: New eMCA trace event interface

To avoid the confuision of usage for RAS related trace event, add an unified RAS trace event stub.

Signed-off-by Chen, Gong <[email protected]>

---
 drivers/edac/edac_mc.c    |  3 ---
 kernel/trace/Makefile     |  1 +
 kernel/trace/ras-traces.c | 12 ++++++++++++
 3 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 kernel/trace/ras-traces.c

drivers/edac/edac_mc.c +0 -3

index 33edd67..28c1695 100644
@@ -33,9 +33,6 @@
 #include <asm/edac.h>
 #include "edac_core.h"
 #include "edac_module.h"
-
-#define CREATE_TRACE_POINTS
-#define TRACE_INCLUDE_PATH ../../include/ras
 #include <ras/ras_event.h>
 
 /* lock to memory controller's control array */

kernel/trace/Makefile +0 -0

index 1378e84..167193a 100644

Quoted 30 lines

To avoid the confuision of usage for RAS related trace event, add an unified RAS trace event stub.

Signed-off-by Chen, Gong <[email protected]>

---
 drivers/edac/edac_mc.c    |  3 ---
 kernel/trace/Makefile     |  1 +
 kernel/trace/ras-traces.c | 12 ++++++++++++
 3 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 kernel/trace/ras-traces.c

drivers/edac/edac_mc.c +0 -3

index 33edd67..28c1695 100644
@@ -33,9 +33,6 @@
 #include <asm/edac.h>
 #include "edac_core.h"
 #include "edac_module.h"
-
-#define CREATE_TRACE_POINTS
-#define TRACE_INCLUDE_PATH ../../include/ras
 #include <ras/ras_event.h>
 
 /* lock to memory controller's control array */

kernel/trace/Makefile +0 -0

index 1378e84..167193a 100644

kernel/trace/Makefile endif · line 55

Quoted 4 lines

kernel/trace/Makefile +1 -0

@@ -52,6 +52,7 @@ endif
 obj-$(CONFIG_EVENT_TRACING) += trace_events_filter.o
 obj-$(CONFIG_EVENT_TRACING) += trace_events_trigger.o
 obj-$(CONFIG_KPROBE_EVENT) += trace_kprobe.o
+obj-$(CONFIG_TRACEPOINTS) += ras-traces.o

Actually I was thinking of slowly concentrating all the RAS stuff into arch/x86/ras/.

Just add arch/x86/ras/trace.c instead please.

I would prefer to keep this out of arch/, because there are some parts of the ras infra that aren't x86 specific.

Going further, it also make sense to move the EDAC drivers into it.

Quoted 2 lines

I would prefer to keep this out of arch/, because there are some parts of the ras infra that aren't x86 specific.

Those are?

Quoted 1 line

Going further, it also make sense to move the EDAC drivers into it.

Why?

For example PCIe and memory errors are not x86-specific. Also, as ACPI may also be used on ARM, we may also start to have APEI errors there:

	https://lwn.net/Articles/574439/
	https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI

So, better to think on that on a long term.

In order to put all RAS drivers under the same place. We may eventually have a subdir there for EDAC, and one per RAS report mechanism, in order to keep it cleaner.

Quoted 6 lines

For example PCIe and memory errors are not x86-specific. Also, as ACPI may also be used on ARM, we may also start to have APEI errors there:

	https://lwn.net/Articles/574439/
	https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI

So, better to think on that on a long term.

kernel/ras/ could also be used in that case but I guess drivers/ras/ is fine too.

Quoted 4 lines

In order to put all RAS drivers under the same place. We may eventually have a subdir there for EDAC, and one per RAS report mechanism, in order to keep it cleaner.

That doesn't bring any advantages - edac drivers are just fine in drivers/edac/. And without benefits for a move, it would be a senseless code churn only.

Date: Thu, 06 Mar 2014 12:39:15 -0300
From: Mauro Carvalho Chehab <[email protected]>
To: Borislav Petkov <[email protected]>
Cc: "Chen, Gong" <[email protected]>, [email protected],
[email protected], [email protected]
Subject: Re: [PATCH 1/2] trace, RAS: Add basic RAS trace event
X-Mailer: Claws Mail 3.9.3 (GTK+ 2.24.22; x86_64-redhat-linux-gnu)

Em Thu, 06 Mar 2014 16:26:33 +0100
Borislav Petkov <[email protected]> escreveu:

Quoted 6 lines · Mauro Carvalho Chehab

Quoted 5 lines

For example PCIe and memory errors are not x86-specific. Also, as ACPI may also be used on ARM, we may also start to have APEI errors there:

	https://lwn.net/Articles/574439/
	https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI

Quoted 4 lines

Quoted 1 line

So, better to think on that on a long term.

kernel/ras/ could also be used in that case but I guess drivers/ras/ is fine too.

Both work for me, although drivers/ras seems more adequate, IMHO, as I expect that we'll have there both subsystem code and drivers.

Quoted 19 lines

Date: Thu, 06 Mar 2014 12:39:15 -0300
From: Mauro Carvalho Chehab <[email protected]>
To: Borislav Petkov <[email protected]>
Cc: "Chen, Gong" <[email protected]>, [email protected],
[email protected], [email protected]
Subject: Re: [PATCH 1/2] trace, RAS: Add basic RAS trace event
X-Mailer: Claws Mail 3.9.3 (GTK+ 2.24.22; x86_64-redhat-linux-gnu)

Em Thu, 06 Mar 2014 16:26:33 +0100
Borislav Petkov <[email protected]> escreveu:

Quoted 8 lines · Mauro Carvalho Chehab

Quoted 6 lines

For example PCIe and memory errors are not x86-specific. Also, as ACPI may also be used on ARM, we may also start to have APEI errors there:

	https://lwn.net/Articles/574439/
	https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI

So, better to think on that on a long term.

Quoted 6 lines

Quoted 2 lines

kernel/ras/ could also be used in that case but I guess drivers/ras/ is fine too.

Both work for me, although drivers/ras seems more adequate, IMHO, as I expect that we'll have there both subsystem code and drivers.

OK, I will move this trace stub to drivers/ras in next version.

BTW, any other comments for 2nd patch? I thought Mauro will have some comments for that patch. :-)

Add trace interface to elaborate all H/W error related information.

Signed-off-by Chen, Gong <[email protected]>

---
 drivers/acpi/Kconfig        |   3 +-
 drivers/acpi/Makefile       |   1 +
 drivers/acpi/acpi_extlog.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/firmware/efi/cper.c |  13 ++++-
 include/linux/cper.h        |   2 +
 include/ras/ras_event.h     |  62 +++++++++++++++++++++
 kernel/trace/ras-traces.c   |   1 +
 7 files changed, 208 insertions(+), 5 deletions(-)

drivers/acpi/Kconfig +2 -1

index 4770de5..3e569d4 100644
@@ -363,6 +363,7 @@ config ACPI_EXTLOG
 
 	  Enhanced MCA Logging allows firmware to provide additional error
 	  information to system software, synchronous with MCE or CMCI. This
-	  driver adds support for that functionality.
+	  driver adds support for that functionality with corresponding
+	  tracepoint which carries that information to userspace.
 
 endif	# ACPI

drivers/acpi/Makefile +1 -0

index 0331f91..f6abc4a 100644
@@ -82,4 +82,5 @@ obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
 
 obj-$(CONFIG_ACPI_APEI)		+= apei/
 
+CFLAGS_acpi_extlog.o := -I$(src)
 obj-$(CONFIG_ACPI_EXTLOG)	+= acpi_extlog.o

drivers/acpi/acpi_extlog.c +55 -0

index c4a5d87..fbdebad 100644
@@ -14,8 +14,10 @@
 #include <linux/edac.h>
 #include <asm/cpu.h>
 #include <asm/mce.h>
+#include <linux/dmi.h>
 
 #include "apei/apei-internal.h"
+#include <ras/ras_event.h>
 
 #define EXT_ELOG_ENTRY_MASK	GENMASK_ULL(51, 0) /* elog entry address mask */
 

@@ -44,6 +46,11 @@ struct extlog_l1_head {
 static int old_edac_report_status;
 
 static u8 extlog_dsm_uuid[] __initdata = "663E35AF-CC10-41A4-88EA-5470AF055295";
+static const uuid_le invalid_uuid = NULL_UUID_LE;
+
+static DEFINE_RAW_SPINLOCK(trace_lock);
+static char mem_location[LOC_LEN];
+static char dimm_location[LOC_LEN];
 
 /* L1 table related physical address */
 static u64 elog_base;

@@ -69,6 +76,106 @@ static u32 l1_percpu_entry;
 #define ELOG_ENTRY_ADDR(phyaddr) \
 	(phyaddr - elog_base + (u8 *)elog_addr)
 
+static void mem_err_location(struct cper_sec_mem_err *mem)
+{
+	char *p;
+	u32 n = 0;
+
+	memset(mem_location, 0, LOC_LEN);
+	p = mem_location;
+	if (mem->validation_bits & CPER_MEM_VALID_NODE)
+		n += sprintf(p + n, " node: %d", mem->node);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_CARD)
+		n += sprintf(p + n, " card: %d", mem->card);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
+		n += sprintf(p + n, " module: %d", mem->module);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
+		n += sprintf(p + n, " rank: %d", mem->rank);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_BANK)
+		n += sprintf(p + n, " bank: %d", mem->bank);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
+		n += sprintf(p + n, " device: %d", mem->device);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_ROW)
+		n += sprintf(p + n, " row: %d", mem->row);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
+		n += sprintf(p + n, " column: %d", mem->column);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
+		n += sprintf(p + n, " bit_position: %d", mem->bit_pos);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
+		n += sprintf(p + n, " requestor_id: 0x%016llx",
+				mem->requestor_id);
+	if (n >= LOC_LEN)
+		goto end;

drivers/acpi/acpi_extlog.c +6 -0

+
+static void dimm_err_location(struct cper_sec_mem_err *mem)
+{
+	const char *bank = NULL, *device = NULL;
+
+	memset(dimm_location, 0, LOC_LEN);

drivers/acpi/acpi_extlog.c +21 -1

+	if (mem->validation_bits & CPER_MEM_VALID_PA) {
+		phy_addr = mem->physical_addr;
+		if (mem->validation_bits & CPER_MEM_VALID_PA_MASK)
+			phy_addr &= mem->physical_addr_mask;
+	}
+
+	raw_spin_lock_irqsave(&trace_lock, flags);
+	mem_err_location(mem);
+	dimm_err_location(mem);
+
+	trace_extlog_mem_event(etype, dimm_location, fru_id, fru_text,
+			       err_count, severity, phy_addr, mem_location);
+	raw_spin_unlock_irqrestore(&trace_lock, flags);
+}
+
 static struct acpi_generic_status *extlog_elog_entry_check(int cpu, int bank)
 {
 	int idx;

@@ -137,7 +244,12 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 	struct mce *mce = (struct mce *)data;
 	int	bank = mce->bank;
 	int	cpu = mce->extcpu;
-	struct acpi_generic_status *estatus;
+	struct acpi_generic_status *estatus, *tmp;
+	struct acpi_generic_data *gdata;
+	const uuid_le *fru_id = &invalid_uuid;
+	char *fru_text = "";
+	uuid_le *sec_type;
+	static u64 err_count;
 	int rc;
 
 	estatus = extlog_elog_entry_check(cpu, bank);

drivers/acpi/acpi_extlog.c +13 -0

+	/* trace extended error log */
+	err_count++;
+	if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
+		fru_id = (uuid_le *)gdata->fru_id;
+	if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
+		fru_text = gdata->fru_text;
+	sec_type = (uuid_le *)gdata->section_type;
+	if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
+		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
+		if (gdata->error_data_length >= sizeof(*mem_err))
+			trace_mem_error(fru_id, fru_text, err_count,
+					gdata->error_severity, mem_err);
+	}
 
 	return NOTIFY_STOP;
 }

drivers/firmware/efi/cper.c +0 -0

index 1491dd4..9d3e2c4 100644

drivers/firmware/efi/cper.c +7 -0

 /*
  * cper_print_bits - print strings for set bits

@@ -196,6 +197,13 @@ static const char *cper_mem_err_type_strs[] = {
 	"physical memory map-out event",
 };
 
+const char *cper_mem_err_type_str(unsigned int etype)
+{
+	return etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
+		cper_mem_err_type_strs[etype] : "unknown";
+}
+EXPORT_SYMBOL_GPL(cper_mem_err_type_str);
+
 static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
 {
 	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)

include/linux/cper.h +2 -0

index 2fc0ec3..c6d87fc 100644
@@ -395,6 +395,8 @@ struct cper_sec_pcie {
 #pragma pack()
 
 u64 cper_next_record_id(void);
+const char *cper_severity_str(unsigned int);
+const char *cper_mem_err_type_str(unsigned int);
 void cper_print_bits(const char *prefix, unsigned int bits,
 		     const char * const strs[], unsigned int strs_size);
 

include/ras/ras_event.h +5 -0

index 21cdb0b..97f2192 100644
@@ -8,6 +8,68 @@
 #include <linux/tracepoint.h>
 #include <linux/edac.h>
 #include <linux/ktime.h>
+#include <linux/cper.h>
+
+/*
+ * MCE Extended Error Log trace event
+ *

include/ras/ras_event.h +47 -0

+	TP_PROTO(u32 etype,
+		 char *dimm_info,
+		 const uuid_le *fru_id,
+		 char *fru_text,
+		 u64 error_count,
+		 u32 severity,
+		 u64 phy_addr,
+		 char *mem_loc),
+
+	TP_ARGS(etype, dimm_info, fru_id, fru_text, error_count, severity,
+		phy_addr, mem_loc),
+
+	TP_STRUCT__entry(
+		__field(u32, etype)
+		__dynamic_array(char, dimm_info, LOC_LEN)
+		__field(u64, error_count)
+		__field(u32, severity)
+		__field(u64, paddr)
+		__string(mem_loc, mem_loc)
+		__dynamic_array(char, fru, LOC_LEN)
+	),
+
+	TP_fast_assign(
+		__entry->error_count = error_count;
+		__entry->severity = severity;
+		__entry->etype = etype;
+		if (dimm_info[0] != '\0')
+			snprintf(__get_dynamic_array(dimm_info), LOC_LEN - 1,
+				 "%s", dimm_info);
+		else
+			__assign_str(dimm_info, "");
+		__entry->paddr = phy_addr;
+		__assign_str(mem_loc, mem_loc);
+		snprintf(__get_dynamic_array(fru), LOC_LEN - 1,
+			 "FRU: %pUl %.20s", fru_id, fru_text);
+	),
+
+	TP_printk("%llu %s error%s: %s %s physical addr: 0x%016llx%s %s",
+		  __entry->error_count,
+		  cper_severity_str(__entry->severity),
+		  __entry->error_count > 1 ? "s" : "",
+		  cper_mem_err_type_str(__entry->etype),
+		  __get_str(dimm_info),
+		  __entry->paddr,
+		  __get_str(mem_loc),
+		  __get_str(fru))
+);
 
 /*
  * Hardware Events Report

kernel/trace/ras-traces.c +1 -0

index b0c6ed1..197b1ea 100644
@@ -9,4 +9,5 @@
 #define TRACE_INCLUDE_PATH ../../include/ras
 #include <ras/ras_event.h>
 
+EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
 EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);

Signature · 1 line

1.8.4.3

Quoted 48 lines

drivers/acpi/acpi_extlog.c +48 -0

+static void mem_err_location(struct cper_sec_mem_err *mem)
+{
+	char *p;
+	u32 n = 0;
+
+	memset(mem_location, 0, LOC_LEN);
+	p = mem_location;
+	if (mem->validation_bits & CPER_MEM_VALID_NODE)
+		n += sprintf(p + n, " node: %d", mem->node);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_CARD)
+		n += sprintf(p + n, " card: %d", mem->card);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
+		n += sprintf(p + n, " module: %d", mem->module);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
+		n += sprintf(p + n, " rank: %d", mem->rank);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_BANK)
+		n += sprintf(p + n, " bank: %d", mem->bank);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
+		n += sprintf(p + n, " device: %d", mem->device);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_ROW)
+		n += sprintf(p + n, " row: %d", mem->row);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
+		n += sprintf(p + n, " column: %d", mem->column);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
+		n += sprintf(p + n, " bit_position: %d", mem->bit_pos);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
+		n += sprintf(p + n, " requestor_id: 0x%016llx",
+				mem->requestor_id);
+	if (n >= LOC_LEN)
+		goto end;

drivers/acpi/acpi_extlog.c static u32 l1_percpu_entry; · line 136

Quoted 10 lines

drivers/acpi/acpi_extlog.c +10 -0

+	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
+		n += sprintf(p + n, " responder_id: 0x%016llx",
+				mem->responder_id);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
+		n += sprintf(p + n, " target_id: 0x%016llx", mem->target_id);
+end:
+	return;
+}

Looks like this wants to share with cper_print_mem() - definitely a lot of duplication there.

Quoted 10 lines

drivers/acpi/acpi_extlog.c +10 -0

+
+static void dimm_err_location(struct cper_sec_mem_err *mem)
+{
+	const char *bank = NULL, *device = NULL;
+
+	memset(dimm_location, 0, LOC_LEN);
+	if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
+		return;
+
+	dmi_memdev_name(mem->mem_dev_handle, &bank, &device);

drivers/acpi/acpi_extlog.c static u32 l1_percpu_entry; · line 152

Quoted 6 lines

drivers/acpi/acpi_extlog.c +6 -0

+	if (bank != NULL && device != NULL)
+		snprintf(dimm_location, LOC_LEN - 1, "%s %s", bank, device);
+	else
+		snprintf(dimm_location, LOC_LEN - 1, "DMI handle: 0x%.4x",
+			 mem->mem_dev_handle);
+}

This one too.

drivers/acpi/acpi_extlog.c static u32 l1_percpu_entry; · line 159

Quoted 7 lines

drivers/acpi/acpi_extlog.c +7 -0

+
+static void trace_mem_error(const uuid_le *fru_id, char *fru_text,
+			    u64 err_count, u32 severity,
+			    struct cper_sec_mem_err *mem)
+{
+	u32 etype = ~0U;
+	u64 phy_addr = ~0ull;

I'm assuming userspace knows that all 1s means field value is invalid?

drivers/acpi/acpi_extlog.c static u32 l1_percpu_entry; · line 163

Quoted 5 lines

drivers/acpi/acpi_extlog.c +4 -0

+	unsigned long flags;
+
+	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
+		etype = mem->error_type;

newline.

We probably need a mechanism to disable printking to dmesg once userspace has opened the tracepoint.

Quoted 7 lines

static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem) {

drivers/firmware/efi/cper.c +0 -0

 	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)

@@ -233,8 +241,7 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
 	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
 		u8 etype = mem->error_type;
 		printk("%s""error_type: %d, %s\n", pfx, etype,

drivers/firmware/efi/cper.c static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem) · line 247

Quoted 6 lines

drivers/firmware/efi/cper.c +1 -2

-		       etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
-		       cper_mem_err_type_strs[etype] : "unknown");
+			cper_mem_err_type_str(etype));
 	}
 	if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
 		const char *bank = NULL, *device = NULL;

Ditto.

include/ras/ras_event.h line 25

Quoted 3 lines

include/ras/ras_event.h +3 -0

+#define LOC_LEN		512
+
+TRACE_EVENT(extlog_mem_event,

So this is a mem thing so we're defining a tracepoint for memory events, specifically.

However, if extlog carries all kinds of errors outside, not only DRAM errors, we should do a TRACE_EVENT_CLASS which contains the shared args to every error type and then make a mem event ontop of it.

[...]

Quoted 72 lines

Quoted 48 lines

drivers/acpi/acpi_extlog.c +48 -0

+static void mem_err_location(struct cper_sec_mem_err *mem)
+{
+	char *p;
+	u32 n = 0;
+
+	memset(mem_location, 0, LOC_LEN);
+	p = mem_location;
+	if (mem->validation_bits & CPER_MEM_VALID_NODE)
+		n += sprintf(p + n, " node: %d", mem->node);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_CARD)
+		n += sprintf(p + n, " card: %d", mem->card);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
+		n += sprintf(p + n, " module: %d", mem->module);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
+		n += sprintf(p + n, " rank: %d", mem->rank);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_BANK)
+		n += sprintf(p + n, " bank: %d", mem->bank);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
+		n += sprintf(p + n, " device: %d", mem->device);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_ROW)
+		n += sprintf(p + n, " row: %d", mem->row);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
+		n += sprintf(p + n, " column: %d", mem->column);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
+		n += sprintf(p + n, " bit_position: %d", mem->bit_pos);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
+		n += sprintf(p + n, " requestor_id: 0x%016llx",
+				mem->requestor_id);
+	if (n >= LOC_LEN)
+		goto end;

drivers/acpi/acpi_extlog.c static u32 l1_percpu_entry; · line 136

Quoted 10 lines

drivers/acpi/acpi_extlog.c +10 -0

+	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
+		n += sprintf(p + n, " responder_id: 0x%016llx",
+				mem->responder_id);
+	if (n >= LOC_LEN)
+		goto end;
+	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
+		n += sprintf(p + n, " target_id: 0x%016llx", mem->target_id);
+end:
+	return;
+}

Looks like this wants to share with cper_print_mem() - definitely a lot of duplication there.

Quoted 10 lines

drivers/acpi/acpi_extlog.c +10 -0

+
+static void dimm_err_location(struct cper_sec_mem_err *mem)
+{
+	const char *bank = NULL, *device = NULL;
+
+	memset(dimm_location, 0, LOC_LEN);
+	if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
+		return;
+
+	dmi_memdev_name(mem->mem_dev_handle, &bank, &device);

Quoted 9 lines

drivers/acpi/acpi_extlog.c static u32 l1_percpu_entry; · line 152

Quoted 6 lines

drivers/acpi/acpi_extlog.c +6 -0

+	if (bank != NULL && device != NULL)
+		snprintf(dimm_location, LOC_LEN - 1, "%s %s", bank, device);
+	else
+		snprintf(dimm_location, LOC_LEN - 1, "DMI handle: 0x%.4x",
+			 mem->mem_dev_handle);
+}

This one too.

Not really. Firstly they service for different purpose. Secondly the format here can be changed/updated depending on further requirment. I can't assume they always keep the same format.

Quoted 9 lines

drivers/acpi/acpi_extlog.c static u32 l1_percpu_entry; · line 159

Quoted 7 lines

drivers/acpi/acpi_extlog.c +7 -0

+
+static void trace_mem_error(const uuid_le *fru_id, char *fru_text,
+			    u64 err_count, u32 severity,
+			    struct cper_sec_mem_err *mem)
+{
+	u32 etype = ~0U;
+	u64 phy_addr = ~0ull;

I'm assuming userspace knows that all 1s means field value is invalid?

Yep, I suppose so.

Quoted 8 lines

drivers/acpi/acpi_extlog.c static u32 l1_percpu_entry; · line 163

Quoted 6 lines

drivers/acpi/acpi_extlog.c +4 -0

+	unsigned long flags;
+
+	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
+		etype = mem->error_type;

newline.

Sure.

[...]

Quoted 2 lines

We probably need a mechanism to disable printking to dmesg once userspace has opened the tracepoint.

Do we really need to do that? IMHO, I think they are used for two different usages, just like dmesg & mcelog.

[...]

Quoted 7 lines

Quoted 7 lines

static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem) {

drivers/firmware/efi/cper.c +0 -0

 	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)

@@ -233,8 +241,7 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
 	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
 		u8 etype = mem->error_type;
 		printk("%s""error_type: %d, %s\n", pfx, etype,

Quoted 8 lines

drivers/firmware/efi/cper.c static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem) · line 247

Quoted 6 lines

drivers/firmware/efi/cper.c +1 -2

-		       etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
-		       cper_mem_err_type_strs[etype] : "unknown");
+			cper_mem_err_type_str(etype));
 	}
 	if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
 		const char *bank = NULL, *device = NULL;

Ditto.

I know you hope the print function in CPER & trace for cpi_extlog can be merged into one. I just have one concern about it. Can we ensure these two functions keeping align all the time? IOW, merge them for now until change happens one day?

[...]

Quoted 10 lines

include/ras/ras_event.h line 25

Quoted 3 lines

include/ras/ras_event.h +3 -0

+#define LOC_LEN		512
+
+TRACE_EVENT(extlog_mem_event,

So this is a mem thing so we're defining a tracepoint for memory events, specifically.

However, if extlog carries all kinds of errors outside, not only DRAM errors, we should do a TRACE_EVENT_CLASS which contains the shared args to every error type and then make a mem event ontop of it.

I agree.

Changing the format breaks any userspace application that relies on parsing them. That's an API breakage. Adding more data could be fine, if we take enough care when doing it, and properly document how userspace is supposed to parse it.

Well, actually, EDAC drivers use 0 to indicate an unknown physical address. The better is to use the same standard used there.

Quoted 4 lines

Changing the format breaks any userspace application that relies on parsing them. That's an API breakage. Adding more data could be fine, if we take enough care when doing it, and properly document how userspace is supposed to parse it.

Yes, we don't want code duplication if it can be helped. Besides, having one function doing the error format issual keeps us from the case when having two or more diverge from one another.

Quoted 2 lines

Well, actually, EDAC drivers use 0 to indicate an unknown physical address. The better is to use the same standard used there.

However, physical address 0 is a valid address... all FFF...Fs is hardly valid.

True, but if we decide to go on that direction, a change like that should then be done on all EDAC drivers, and that's an API change.

We also need to be sure that userspace will handle this change properly.

Quoted 5 lines

True, but if we decide to go on that direction, a change like that should then be done on all EDAC drivers, and that's an API change.

We also need to be sure that userspace will handle this change properly.

We don't have a choice:

[    0.000000] e820: BIOS-provided physical RAM map:
[    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009e7ff] usable

Since we have separate trace points for EDCA and extlog - we can use different conventions for "invalid". If you think that changing the ABI in EDAC would be a problem - then you can keep running the almost infinitesimal chance that you have a real error at 0x00000000000000 that you treat as unknown address.

Does X86 actually allocate that page for use? - I know on Itanium it got thrown out when rounding usable blocks of memory to 16MB boundaries for cache coherency reasons.

Do we really need to do that? IMHO, I think they are used for two different usages, just like dmesg & mcelog.

Not really. Firstly they service for different purpose. Secondly the format here can be changed/updated depending on further requirment. I can't assume they always keep the same format.

I can't imagine a reason why we'd want them to be different though. If a reason does come up in the future, then we can clone & edit the function to do different things - but until then we should share code.

Note: there will be changes periodically when UEFI changes the error record format (they have added new fields in the past - I expect they will add more in the future). We should be able to add more stuff to the end of the "mem_loc[]" string as long as we write user-mode parsers that won't be surprised by extra fields there.

-Tony

Here is (most of) the matching patch to rasdaemon to collect messages from this trace. Rasdaemon lives at:
git://git.fedorahosted.org/rasdaemon.git

Missing part is saving to sqlite data base - boiler plate is present, but the specifics to create tables and save to them need to be added. But this should serve as a useful test bed while we argue the punctuation in the fields supplied by the kernel trace.

Note: There are few platforms that fully support this at the moment. One is the "Brickland Ivy Bridge -EX" (Running BIOS 0046.R04 or newer). Another is the "Grantley Haswell -EP" (Running BIOS 0026 ... which I don't think is out yet). Be sure to enable eMCA options in BIOS setup EDKII>Advanced>System Event Log.

-Tony

---

From: "Luck, Tony" <[email protected]>

[RASDAEMON] Add support for extlog events reported by the kernel

Kernel may report extra platform specific information provided by BIOS. Capture it and log it.

TODO: log to the SQLITE data base - just top level hooks present in this patch.

Signed-off-by Tony Luck <[email protected]>

---

Makefile.am +5 -1

index c1668b4d939d..2f81d777823c 100644
@@ -17,13 +17,17 @@ if WITH_MCE
 			mce-intel-dunnington.c mce-intel-tulsa.c \
 			mce-intel-sb.c mce-intel-ivb.c
 endif
+if WITH_EXTLOG
+   rasdaemon_SOURCES += ras-extlog-handler.c
+endif
 if WITH_ABRT_REPORT
    rasdaemon_SOURCES += ras-report.c
 endif
 rasdaemon_LDADD = -lpthread $(SQLITE3_LIBS) libtrace/libtrace.a
 
 include_HEADERS = config.h  ras-events.h  ras-logger.h  ras-mc-handler.h \
-		  ras-aer-handler.h ras-mce-handler.h ras-record.h bitfield.h ras-report.h
+		  ras-aer-handler.h ras-mce-handler.h ras-record.h bitfield.h ras-report.h \
+		  ras-extlog-handler.h
 
 # This rule can't be called with more than one Makefile job (like make -j8)
 # I can't figure out a way to fix that

configure.ac +9 -0

index 5bd824c0dbe2..b050ad864cdf 100644
@@ -53,6 +53,15 @@ AS_IF([test "x$enable_mce" = "xyes"], [
 ])
 AM_CONDITIONAL([WITH_MCE], [test x$enable_mce = xyes])
 
+AC_ARG_ENABLE([extlog],
+    AS_HELP_STRING([--enable-extlog], [enable EXTLOG events (currently experimental)]))
+
+AS_IF([test "x$enable_extlog" = "xyes"], [
+  AC_DEFINE(HAVE_EXTLOG,1,"have EXTLOG events collect")
+  AC_SUBST([WITH_EXTLOG])
+])
+AM_CONDITIONAL([WITH_EXTLOG], [test x$enable_extlog = xyes])
+
 AC_ARG_ENABLE([abrt_report],
     AS_HELP_STRING([--enable-abrt-report], [enable report event to ABRT (currently experimental)]))
 

ras-events.c +16 -0

index ecbbd3afa66d..90ea3727f667 100644
@@ -30,6 +30,7 @@
 #include "ras-mc-handler.h"
 #include "ras-aer-handler.h"
 #include "ras-mce-handler.h"
+#include "ras-extlog-handler.h"
 #include "ras-record.h"
 #include "ras-logger.h"
 

@@ -203,6 +204,10 @@ int toggle_ras_mc_event(int enable)
 	rc |= __toggle_ras_mc_event(ras, "mce", "mce_record", enable);
 #endif
 
+#ifdef HAVE_EXTLOG
+	rc |= __toggle_ras_mc_event(ras, "ras", "extlog_mem_event", enable);
+#endif
+
 free_ras:
 	free(ras);
 	return rc;

@@ -688,6 +693,17 @@ int handle_ras_events(int record_events)
 		    "mce", "mce_record");
 	}
 #endif
+
+#ifdef HAVE_EXTLOG
+	rc = add_event_handler(ras, pevent, page_size, "ras", "extlog_mem_event",
+			       ras_extlog_mem_event_handler);
+	if (!rc)
+		num_events++;
+	else
+		log(ALL, LOG_ERR, "Can't get traces from %s:%s\n",
+		    "ras", "aer_event");
+#endif
+
 	if (!num_events) {
 		log(ALL, LOG_INFO,
 		    "Failed to trace all supported RAS events. Aborting.\n");

ras-extlog-handler.c new file +134 -0

new file mode 100644
index 000000000000..1166a5341703
@@ -0,0 +1,134 @@
+/*
+ * Copyright (C) 2014 Tony Luck <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+*/
+#include <ctype.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdint.h>
+#include "libtrace/kbuffer.h"
+#include "ras-extlog-handler.h"
+#include "ras-record.h"
+#include "ras-logger.h"
+#include "ras-report.h"
+
+static char *err_type(int etype)
+{
+	switch (etype) {
+	case 0: return "unknown";
+	case 1: return "no error";
+	case 2: return "single-bit ECC";
+	case 3: return "multi-bit ECC";
+	case 4: return "single-symbol chipkill ECC";
+	case 5: return "multi-symbol chipkill ECC";
+	case 6: return "master abort";
+	case 7: return "target abort";
+	case 8: return "parity error";
+	case 9: return "watchdog timeout";
+	case 10: return "invalid address";
+	case 11: return "mirror Broken";
+	case 12: return "memory sparing";
+	case 13: return "scrub corrected error";
+	case 14: return "scrub uncorrected error";
+	case 15: return "physical memory map-out event";
+	}
+	return "unknown-type";
+}
+
+static char *err_severity(int severity)
+{
+	switch (severity) {
+	case 0: return "recoverable";
+	case 1: return "fatal";
+	case 2: return "corrected";
+	case 3: return "informational";
+	}
+	return "unknown-severity";
+}
+
+static void report_extlog_mem_event(struct ras_events *ras,
+				    struct pevent_record *record,
+				    struct trace_seq *s,
+				    struct ras_extlog_event *ev)
+{
+	trace_seq_printf(s, "%d %s error: %s %s physical addr: 0x%llx (%s %s)",
+		ev->error_count, err_severity(ev->severity),
+		err_type(ev->etype), ev->dimm_info, ev->address,
+		ev->mem_loc, ev->fru);
+}
+
+int ras_extlog_mem_event_handler(struct trace_seq *s,
+			  struct pevent_record *record,
+			  struct event_format *event, void *context)
+{
+	int len;
+	unsigned long long val;
+	struct ras_events *ras = context;
+	time_t now;
+	struct tm *tm;
+	struct ras_extlog_event ev;
+
+	/*
+	 * Newer kernels (3.10-rc1 or upper) provide an uptime clock.
+	 * On previous kernels, the way to properly generate an event would
+	 * be to inject a fake one, measure its timestamp and diff it against
+	 * gettimeofday. We won't do it here. Instead, let's use uptime,
+	 * falling-back to the event report's time, if "uptime" clock is
+	 * not available (legacy kernels).
+	 */
+
+	if (ras->use_uptime)
+		now = record->ts/1000000000L + ras->uptime_diff;
+	else
+		now = time(NULL);
+
+	tm = localtime(&now);
+	if (tm)
+		strftime(ev.timestamp, sizeof(ev.timestamp),
+			 "%Y-%m-%d %H:%M:%S %z", tm);
+	trace_seq_printf(s, "%s ", ev.timestamp);
+
+	if (pevent_get_field_val(s,  event, "etype", record, &val, 1) < 0)
+		return -1;
+	ev.etype = val;
+	if (pevent_get_field_val(s,  event, "error_count", record, &val, 1) < 0)
+		return -1;
+	ev.error_count = val;
+	if (pevent_get_field_val(s,  event, "severity", record, &val, 1) < 0)
+		return -1;
+	ev.severity = val;
+	if (pevent_get_field_val(s,  event, "paddr", record, &val, 1) < 0)
+		return -1;
+	ev.address = val;
+
+	ev.dimm_info = pevent_get_field_raw(s, event, "dimm_info",
+					   record, &len, 1);
+	ev.mem_loc = pevent_get_field_raw(s, event, "mem_loc",
+					   record, &len, 1);
+	ev.fru = pevent_get_field_raw(s, event, "fru",
+					   record, &len, 1);
+
+	report_extlog_mem_event(ras, record, s, &ev);
+
+#ifdef HAVE_SQLITE3
+	ras_store_extlog_mem_record(ras, &ev);
+#endif
+
+	return 0;
+}

ras-extlog-handler.h new file +31 -0

new file mode 100644
index 000000000000..54e8cec93af9
@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2014 Tony Luck <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+*/
+
+#ifndef __RAS_EXTLOG_HANDLER_H
+#define __RAS_EXTLOG_HANDLER_H
+
+#include <stdint.h>
+
+#include "ras-events.h"
+#include "libtrace/event-parse.h"
+
+extern int ras_extlog_mem_event_handler(struct trace_seq *s,
+			  struct pevent_record *record,
+			  struct event_format *event, void *context);
+
+#endif

ras-record.c +29 -0

index daa3cb102883..6b4cf548bb46 100644
@@ -157,6 +157,28 @@ int ras_store_aer_event(struct ras_events *ras, struct ras_aer_event *ev)
 }
 #endif
 
+#ifdef HAVE_EXTLOG
+static const struct db_fields extlog_event_fields[] = {
+		{ .name="id",			.type="INTEGER PRIMARY KEY" },
+		{ .name="timestamp",		.type="TEXT" },
+};
+
+static const struct db_table_descriptor extlog_event_tab = {
+	.name = "extlog_event",
+	.fields = extlog_event_fields,
+	.num_fields = ARRAY_SIZE(extlog_event_fields),
+};
+
+int ras_store_extlog_mem_record(struct ras_events *ras, struct ras_extlog_event *ev)
+{
+
+	/*
+	 * TODO: Define the right fields above (extlog_event_fields[]) and
+	 * add code here to save them to the database
+	 */
+	return 0;
+}
+#endif
 
 /*
  * Table and functions to handle mce:mce_record

@@ -384,6 +406,13 @@ int ras_mc_event_opendb(unsigned cpu, struct ras_events *ras)
 					 &aer_event_tab);
 #endif
 
+#ifdef HAVE_EXTLOG
+	rc = ras_mc_create_table(priv, &extlog_event_tab);
+	if (rc == SQLITE_OK)
+		rc = ras_mc_prepare_stmt(priv, &priv->stmt_extlog_record,
+					 &extlog_event_tab);
+#endif
+
 #ifdef HAVE_MCE
 	rc = ras_mc_create_table(priv, &mce_record_tab);
 	if (rc == SQLITE_OK)

ras-record.h +17 -0

index 6f146a875b8e..715bfbbedacc 100644
@@ -40,8 +40,20 @@ struct ras_aer_event {
 	const char *msg;
 };
 
+struct ras_extlog_event {
+	char timestamp[64];
+	int etype;
+	int error_count;
+	int severity;
+	unsigned long long address;
+	const char *dimm_info;
+	const char *mem_loc;
+	const char *fru;
+};
+
 struct ras_mc_event;
 struct ras_aer_event;
+struct ras_extlog_event;
 struct mce_event;
 
 #ifdef HAVE_SQLITE3

@@ -57,18 +69,23 @@ struct sqlite3_priv {
 #ifdef HAVE_MCE
 	sqlite3_stmt	*stmt_mce_record;
 #endif
+#ifdef HAVE_EXTLOG
+	sqlite3_stmt	*stmt_extlog_record;
+#endif
 };
 
 int ras_mc_event_opendb(unsigned cpu, struct ras_events *ras);
 int ras_store_mc_event(struct ras_events *ras, struct ras_mc_event *ev);
 int ras_store_aer_event(struct ras_events *ras, struct ras_aer_event *ev);
 int ras_store_mce_record(struct ras_events *ras, struct mce_event *ev);
+int ras_store_extlog_mem_record(struct ras_events *ras, struct ras_extlog_event *ev);
 
 #else
 static inline int ras_mc_event_opendb(unsigned cpu, struct ras_events *ras) { return 0; };
 static inline int ras_store_mc_event(struct ras_events *ras, struct ras_mc_event *ev) { return 0; };
 static inline int ras_store_aer_event(struct ras_events *ras, struct ras_aer_event *ev) { return 0; };
 static inline int ras_store_mce_record(struct ras_events *ras, struct mce_event *ev) { return 0; };
+static inline int ras_store_extlog_mem_record(struct ras_events *ras, struct ras_extlog_event *ev) { return 0; };
 
 #endif
 

As I just pointed, we'll also need some code at the rasdaemon that would associate the CPER error location data into the corresponding DIMM label.

Quoted 2 lines · Mauro Carvalho Chehab

As I just pointed, we'll also need some code at the rasdaemon that would associate the CPER error location data into the corresponding DIMM label.

In most cases the kernel will provide a label (that it got from SMBIOS). I assume
this is for the case that the SMBIOS table is bad?

I haven't looked too hard at how EDAC does this - the "--register-labels" option to edac-ctl?

Since we don't have a sysfs interface for extlog - perhaps any translation can be handled entirely in rasdaemon(8).