Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

Extended H/W error log driver

351 views
Skip to first unread message

Chen, Gong

unread,
Oct 11, 2013, 2:50:01 AM10/11/13
to
[PATCH 1/8] ACPI, APEI, CPER: Fix status check during error printing
[PATCH 2/8] ACPI, CPER: Update cper info
[PATCH 3/8] ACPI, x86: Extended error log driver for x86 platform
[PATCH 4/8] DMI: Parse memory device (type 17) in SMBIOS
[PATCH 5/8] ACPI, APEI, CPER: Add UEFI 2.4 support for memory error
[PATCH 6/8] ACPI, APEI, CPER: Enhance memory reporting capability
[PATCH 7/8] ACPI, APEI, CPER: Cleanup CPER memory error output format
[PATCH 8/8] ACPI / trace: Add trace interface for eMCA driver

This patch series adds an enhanced MCA event logging driver provided by Intel.
Please refer to this link: htpp://www.intel.com/content/www/us/en/architecture-and-technology/enhanced-mca-logging-xeon-paper.html

Certain usages such as Predictive Failure Analysis (PFA) require more
information about the error than what can be described in processor
machine check banks. Most server processors log additional information
about the error in processor uncore registers. Since the addresses
and layout of these registers vary widely from one processor to another,
system software cannot readily make use of them. To complicate matters
further, some of the additionalerror information cannot be constructed
without detailed knowledge about platform topology. This enhanced MCA
logging driver allows firmware to provide additional error information
to MCE/CMCI handler and thus addresses this gap.

After applying this patch series, when a memory corrected error happens,
we can get following information:

dmesg output:

[56005.785917] {3}Hardware error detected on CPU0
[56005.785959] {3}event severity: corrected
[56005.785975] {3}sub_event[0], severity: corrected
[56005.785977] {3}section_type: memory error
[56005.785981] {3}physical_address: 0x0000000851fe0000
[56005.786027] {3}DIMM location: Memriser1 CHANNEL A DIMM 0
[56005.786154] {4}Hardware error detected on CPU0
[56005.786159] {4}event severity: corrected
[56005.786162] {4}sub_event[0], severity: corrected
[56005.786166] {4}section_type: memory error


trace output:

# tracer: nop
#
# entries-in-buffer/entries-written: 4/4 #P:120
#
# _-----=> irqs-off
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / delay
# TASK-PID CPU# |||| TIMESTAMP FUNCTION
# | | | |||| | |
...
...
<idle>-0 [000] d.h. 56068.488759: extlog_mem_event: 3 corrected errors:unknown on Memriser1 CHANNEL A DIMM 0(FRU: 00000000-0000-0000-0000-000000000000 physical addr: 0x0000000851fe0000 node: 0 card: 0 module: 0 rank: 0 bank: 0 row: 28927 column: 1296)
<idle>-0 [000] d.h. 56068.488834: extlog_mem_event: 4 corrected errors:unknown
...
...

dmesg output are shrank to only keep the most important data. The trace
output will contain most of data. Not sure if all fields are meaningful
to users. Some fields like FRU ID/FRU TEXT depends on BIOS manufactor.
So welcome to add comments for what is needed or not.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Chen, Gong

unread,
Oct 11, 2013, 2:50:01 AM10/11/13
to
Keep up only the most important fields for memory error
reporting. The detail information will be moved to perf/trace
interface.

Suggested-by: Tony Luck <tony...@intel.com>
Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/apei/cper.c | 42 ++++++++++++++----------------------------
1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index 2a4389f..567410e 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -206,29 +206,29 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
printk("%s""physical_address_mask: 0x%016llx\n",
pfx, mem->physical_addr_mask);
if (mem->validation_bits & CPER_MEM_VALID_NODE)
- printk("%s""node: %d\n", pfx, mem->node);
+ pr_debug("node: %d\n", mem->node);
if (mem->validation_bits & CPER_MEM_VALID_CARD)
- printk("%s""card: %d\n", pfx, mem->card);
+ pr_debug("card: %d\n", mem->card);
if (mem->validation_bits & CPER_MEM_VALID_MODULE)
- printk("%s""module: %d\n", pfx, mem->module);
+ pr_debug("module: %d\n", mem->module);
if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
- printk("%s""rank: %d\n", pfx, mem->rank);
+ pr_debug("rank: %d\n", mem->rank);
if (mem->validation_bits & CPER_MEM_VALID_BANK)
- printk("%s""bank: %d\n", pfx, mem->bank);
+ pr_debug("bank: %d\n", mem->bank);
if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
- printk("%s""device: %d\n", pfx, mem->device);
+ pr_debug("device: %d\n", mem->device);
if (mem->validation_bits & CPER_MEM_VALID_ROW)
- printk("%s""row: %d\n", pfx, mem->row);
+ pr_debug("row: %d\n", mem->row);
if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
- printk("%s""column: %d\n", pfx, mem->column);
+ pr_debug("column: %d\n", mem->column);
if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
- printk("%s""bit_position: %d\n", pfx, mem->bit_pos);
+ pr_debug("bit_position: %d\n", mem->bit_pos);
if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
- printk("%s""requestor_id: 0x%016llx\n", pfx, mem->requestor_id);
+ pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id);
if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
- printk("%s""responder_id: 0x%016llx\n", pfx, mem->responder_id);
+ pr_debug("responder_id: 0x%016llx\n", mem->responder_id);
if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
- printk("%s""target_id: 0x%016llx\n", pfx, mem->target_id);
+ pr_debug("target_id: 0x%016llx\n", mem->target_id);
if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
u8 etype = mem->error_type;
printk("%s""error_type: %d, %s\n", pfx, etype,
@@ -296,15 +296,6 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
pfx, pcie->bridge.secondary_status, pcie->bridge.control);
}

-static const char *cper_estatus_section_flag_strs[] = {
- "primary",
- "containment warning",
- "reset",
- "error threshold exceeded",
- "resource not accessible",
- "latent error",
-};
-
static void cper_estatus_print_section(
const char *pfx, const struct acpi_generic_data *gdata, int sec_no)
{
@@ -312,11 +303,8 @@ static void cper_estatus_print_section(
__u16 severity;

severity = gdata->error_severity;
- printk("%s""section: %d, severity: %d, %s\n", pfx, sec_no, severity,
+ printk("%s""sub_event[%d], severity: %s\n", pfx, sec_no,
cper_severity_str(severity));
- printk("%s""flags: 0x%02x\n", pfx, gdata->flags);
- cper_print_bits(pfx, gdata->flags, cper_estatus_section_flag_strs,
- ARRAY_SIZE(cper_estatus_section_flag_strs));
if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
printk("%s""fru_id: %pUl\n", pfx, (uuid_le *)gdata->fru_id);
if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
@@ -360,10 +348,8 @@ void cper_estatus_print(const char *pfx,
int sec_no = 0;
__u16 severity;

- printk("%s""Generic Hardware Error Status\n", pfx);
severity = estatus->error_severity;
- printk("%s""severity: %d, %s\n", pfx, severity,
- cper_severity_str(severity));
+ printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
data_len = estatus->data_length;
gdata = (struct acpi_generic_data *)(estatus + 1);
while (data_len >= sizeof(*gdata)) {
--
1.8.4.rc3

Chen, Gong

unread,
Oct 11, 2013, 2:50:01 AM10/11/13
to
After H/W error happens under FFM enabled mode, lots of information
are shown but some important parts like DIMM location missed. This
patch is used to show these extra fileds.

Original-author: Tony Luck <tony...@intel.com>
Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/apei/cper.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index 680230c..2a4389f 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -28,6 +28,7 @@
#include <linux/module.h>
#include <linux/time.h>
#include <linux/cper.h>
+#include <linux/dmi.h>
#include <linux/acpi.h>
#include <linux/pci.h>
#include <linux/aer.h>
@@ -210,6 +211,8 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
printk("%s""card: %d\n", pfx, mem->card);
if (mem->validation_bits & CPER_MEM_VALID_MODULE)
printk("%s""module: %d\n", pfx, mem->module);
+ if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
+ printk("%s""rank: %d\n", pfx, mem->rank);
if (mem->validation_bits & CPER_MEM_VALID_BANK)
printk("%s""bank: %d\n", pfx, mem->bank);
if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
@@ -232,6 +235,15 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
cper_mem_err_type_strs[etype] : "unknown");
}
+ if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
+ const char *bank = NULL, *device = NULL;
+ dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
+ if (bank != NULL && device != NULL)
+ printk("%s""DIMM location: %s %s", pfx, bank, device);
+ else
+ printk("%s""DIMM DMI handle: 0x%.4x", pfx,
+ mem->mem_dev_handle);
+ }
}

static const char *cper_pcie_port_type_strs[] = {
--
1.8.4.rc3

Chen, Gong

unread,
Oct 11, 2013, 2:50:01 AM10/11/13
to
Use trace interface to elaborate all H/W error related
information.

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/Kconfig | 7 ++-
drivers/acpi/Makefile | 4 ++
drivers/acpi/acpi_extlog.c | 28 +++++++++++-
drivers/acpi/apei/cper.c | 13 ++++--
drivers/acpi/debug_extlog.h | 16 +++++++
drivers/acpi/extlog_trace.c | 105 ++++++++++++++++++++++++++++++++++++++++++++
drivers/acpi/extlog_trace.h | 77 ++++++++++++++++++++++++++++++++
include/linux/cper.h | 2 +
8 files changed, 246 insertions(+), 6 deletions(-)
create mode 100644 drivers/acpi/debug_extlog.h
create mode 100644 drivers/acpi/extlog_trace.c
create mode 100644 drivers/acpi/extlog_trace.h

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 1465fa8..9ea343e 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -372,12 +372,17 @@ config ACPI_BGRT

source "drivers/acpi/apei/Kconfig"

+config EXTLOG_TRACE
+ def_bool n
+
config ACPI_EXTLOG
tristate "Extended Error Log support"
depends on X86 && X86_MCE
+ select EXTLOG_TRACE
default n
help
This driver adds support for decoding extended errors from hardware.
- which allows the operating system to obtain data from trace.
+ which allows the operating system to obtain data from trace. It will
+ appear under /sys/kernel/debug/tracing/ras/ .

endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index bce34af..a6e41b7 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -83,4 +83,8 @@ obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o

obj-$(CONFIG_ACPI_APEI) += apei/

+# extended log support
+acpi-$(CONFIG_EXTLOG_TRACE) += extlog_trace.o
+CFLAGS_extlog_trace.o := -I$(src)
+
obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 3e3e286..ca51eb0 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -26,6 +26,7 @@
#include <asm/mce.h>

#include "apei/apei-internal.h"
+#include "debug_extlog.h"

#define EXT_ELOG_ENTRY_MASK 0xfffffffffffff /* elog entry address mask */

@@ -55,6 +56,8 @@ struct extlog_l1_head {

static u8 extlog_dsm_uuid[] = "663E35AF-CC10-41A4-88EA-5470AF055295";

+static const uuid_le invalid_uuid = NULL_UUID_LE;
+
/* L1 table related physical address */
static u64 elog_base;
static size_t elog_size;
@@ -143,7 +146,12 @@ static int print_extlog_rcd(const char *pfx,

static int extlog_print(const char *pfx, int cpu, int bank)
{
- 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);
@@ -154,7 +162,23 @@ static int extlog_print(const char *pfx, int cpu, int bank)
/* clear record status to enable BIOS to update it again */
estatus->block_status = 0;

- rc = print_extlog_rcd(pfx, (struct acpi_generic_status *)elog_buf, cpu);
+ tmp = (struct acpi_generic_status *)elog_buf;
+ gdata = (struct acpi_generic_data *)(tmp + 1);
+ rc = print_extlog_rcd(pfx, tmp, cpu);
+
+ /* 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 rc;
}
diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index 567410e..0b4cfad 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -56,11 +56,12 @@ static const char *cper_severity_strs[] = {
"info",
};

-static const char *cper_severity_str(unsigned int severity)
+const char *cper_severity_str(unsigned int severity)
{
return severity < ARRAY_SIZE(cper_severity_strs) ?
cper_severity_strs[severity] : "unknown";
}
+EXPORT_SYMBOL_GPL(cper_severity_str);

/*
* cper_print_bits - print strings for set bits
@@ -195,6 +196,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)
@@ -232,8 +240,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,
- 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;
diff --git a/drivers/acpi/debug_extlog.h b/drivers/acpi/debug_extlog.h
new file mode 100644
index 0000000..67bb2c5
--- /dev/null
+++ b/drivers/acpi/debug_extlog.h
@@ -0,0 +1,16 @@
+#ifndef __DEBUG_EXTLOG_H
+#define __DEBUG_EXTLOG_H
+
+#include <linux/cper.h>
+
+#ifdef CONFIG_EXTLOG_TRACE
+extern void trace_mem_error(const uuid_le *fru_id, char *fru_text,
+ u64 err_count, u32 severity, struct cper_sec_mem_err *mem);
+#else
+void trace_mem_error(const uuid_le *fru_id, char *fru_text,
+ u64 err_count, u32 severity, struct cper_sec_mem_err *mem)
+{
+}
+#endif
+
+#endif
diff --git a/drivers/acpi/extlog_trace.c b/drivers/acpi/extlog_trace.c
new file mode 100644
index 0000000..2b2824c
--- /dev/null
+++ b/drivers/acpi/extlog_trace.c
@@ -0,0 +1,105 @@
+#include <linux/export.h>
+#include <linux/dmi.h>
+#include "debug_extlog.h"
+
+#define CREATE_TRACE_POINTS
+#include "extlog_trace.h"
+
+static char mem_location[LOC_LEN];
+static char dimm_location[LOC_LEN];
+
+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;
+ 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;
+}
+
+static void dimm_err_location(struct cper_sec_mem_err *mem)
+{
+ memset(dimm_location, 0, LOC_LEN);
+ if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
+ const char *bank = NULL, *device = NULL;
+ dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
+ 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);
+ }
+}
+
+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 = 0;
+
+ if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
+ etype = mem->error_type;
+ if (mem->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
+ phy_addr = mem->physical_addr;
+ if (mem->validation_bits &
+ CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK)
+ phy_addr &= mem->physical_addr_mask;
+ }
+ 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);
+}
+EXPORT_SYMBOL_GPL(trace_mem_error);
diff --git a/drivers/acpi/extlog_trace.h b/drivers/acpi/extlog_trace.h
new file mode 100644
index 0000000..21f0887
--- /dev/null
+++ b/drivers/acpi/extlog_trace.h
@@ -0,0 +1,77 @@
+#if !defined(_TRACE_EXTLOG_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_EXTLOG_H
+
+#include <linux/tracepoint.h>
+#include <linux/cper.h>
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM extlog
+
+/*
+ * MCE Extended Error Log Trace event
+ *
+ * These events are generated when hardware detects a corrected or
+ * uncorrected event.
+ *
+ */
+
+/* memory trace event */
+
+#define LOC_LEN 512
+#define MSG_LEN ((LOC_LEN) * 2)
+
+TRACE_EVENT(extlog_mem_event,
+ TP_PROTO(u32 etype,
+ char *dimm_loc,
+ const uuid_le *fru_id,
+ char *fru_text,
+ u64 error_count,
+ u32 severity,
+ u64 phy_addr,
+ char *mem_loc),
+
+ TP_ARGS(etype, dimm_loc, 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)
+ __dynamic_array(char, msg, MSG_LEN)
+ ),
+
+ TP_fast_assign(
+ __entry->error_count = error_count;
+ __entry->severity = severity;
+ __entry->etype = etype;
+ if (dimm_loc[0] != '\0')
+ snprintf(__get_dynamic_array(dimm_info), LOC_LEN - 1,
+ "on %s", dimm_loc);
+ else
+ __assign_str(dimm_info, "");
+ if (phy_addr != 0)
+ snprintf(__get_dynamic_array(msg), MSG_LEN - 1,
+ "(FRU: %pUl %.20s physical addr: 0x%016llx%s)",
+ fru_id, fru_text, phy_addr, mem_loc);
+ else
+ __assign_str(msg, "");
+ ),
+
+ TP_printk("%llu %s error%s:%s %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),
+ __get_str(msg))
+);
+
+#endif /* _TRACE_EXTLOG_H */
+
+/* This part must be outside protection */
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE extlog_trace
+#include <trace/define_trace.h>
diff --git a/include/linux/cper.h b/include/linux/cper.h
index bd01c9a..c00eb55 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -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 *strs[], unsigned int strs_size);

--
1.8.4.rc3

Chen, Gong

unread,
Oct 11, 2013, 2:50:01 AM10/11/13
to
This error log driver (a.k.a eMCA driver) is implemented based on
http://www.intel.com/content/www/us/en/architecture-and-technology/enhanced-mca-logging-xeon-paper.html.
After errors are captured, more valuable information can be
got with this new enhanced error log driver.

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
arch/x86/include/asm/mce.h | 5 +
arch/x86/kernel/cpu/mcheck/mce.c | 10 ++
drivers/acpi/Kconfig | 8 +
drivers/acpi/Makefile | 2 +
drivers/acpi/acpi_extlog.c | 339 +++++++++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 3 +-
include/linux/acpi.h | 1 +
7 files changed, 367 insertions(+), 1 deletion(-)
create mode 100644 drivers/acpi/acpi_extlog.c

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index cbe6b9e..f8c33e2 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -12,6 +12,7 @@
#define MCG_CTL_P (1ULL<<8) /* MCG_CTL register available */
#define MCG_EXT_P (1ULL<<9) /* Extended registers available */
#define MCG_CMCI_P (1ULL<<10) /* CMCI supported */
+#define MCG_EXT_ERR_LOG (1ULL<<26) /* Extended error log supported */
#define MCG_EXT_CNT_MASK 0xff0000 /* Number of Extended registers */
#define MCG_EXT_CNT_SHIFT 16
#define MCG_EXT_CNT(c) (((c) & MCG_EXT_CNT_MASK) >> MCG_EXT_CNT_SHIFT)
@@ -204,6 +205,10 @@ extern void mce_disable_bank(int bank);
* Exception handler
*/

+extern spinlock_t mce_elog_lock;
+extern int (*mce_extlog_support)(void);
+/* Call the installed extended error log print function */
+extern int (*mce_ext_err_print)(const char *, int cpu, int bank);
/* Call the installed machine check handler for this CPU setup. */
extern void (*machine_check_vector)(struct pt_regs *, long error_code);
void do_machine_check(struct pt_regs *, long);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index b3218cd..6802637 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -48,6 +48,12 @@

#include "mce-internal.h"

+DEFINE_SPINLOCK(mce_elog_lock);
+EXPORT_SYMBOL_GPL(mce_elog_lock);
+
+int (*mce_ext_err_print)(const char *, int, int) = NULL;
+EXPORT_SYMBOL_GPL(mce_ext_err_print);
+
static DEFINE_MUTEX(mce_chrdev_read_mutex);

#define rcu_dereference_check_mce(p) \
@@ -624,6 +630,10 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
(m.status & (mca_cfg.ser ? MCI_STATUS_S : MCI_STATUS_UC)))
continue;

+ spin_lock(&mce_elog_lock);
+ if (mce_ext_err_print)
+ mce_ext_err_print(NULL, m.extcpu, i);
+ spin_unlock(&mce_elog_lock);
mce_read_aux(&m, i);

if (!(flags & MCP_TIMESTAMP))
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 22327e6..1465fa8 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -372,4 +372,12 @@ config ACPI_BGRT

source "drivers/acpi/apei/Kconfig"

+config ACPI_EXTLOG
+ tristate "Extended Error Log support"
+ depends on X86 && X86_MCE
+ default n
+ help
+ This driver adds support for decoding extended errors from hardware.
+ which allows the operating system to obtain data from trace.
+
endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index cdaf68b..bce34af 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -82,3 +82,5 @@ processor-$(CONFIG_CPU_FREQ) += processor_perflib.o
obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o

obj-$(CONFIG_ACPI_APEI) += apei/
+
+obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
new file mode 100644
index 0000000..3e3e286
--- /dev/null
+++ b/drivers/acpi/acpi_extlog.c
@@ -0,0 +1,339 @@
+/*
+ * Extended Error Log driver
+ *
+ * Copyright (C) 2013 Intel Corp.
+ * Author: Chen, Gong <gong...@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+#include <linux/cper.h>
+#include <linux/ratelimit.h>
+#include <asm/mce.h>
+
+#include "apei/apei-internal.h"
+
+#define EXT_ELOG_ENTRY_MASK 0xfffffffffffff /* elog entry address mask */
+
+#define EXTLOG_DSM_REV 0x0
+#define EXTLOG_FN_QUERY 0x0
+#define EXTLOG_FN_ADDR 0x1
+
+#define FLAG_OS_OPTIN BIT(0)
+#define EXTLOG_QUERY_L1_EXIST BIT(1)
+#define ELOG_ENTRY_VALID (1ULL<<63)
+#define ELOG_ENTRY_LEN 0x1000
+
+#define EMCA_BUG \
+ "Can not request iomem region <0x%016llx-0x%016llx> - eMCA disabled\n"
+
+struct extlog_l1_head {
+ u32 ver; /* Header Version */
+ u32 hdr_len; /* Header Length */
+ u64 total_len; /* entire L1 Directory length including this header */
+ u64 elog_base; /* MCA Error Log Directory base address */
+ u64 elog_len; /* MCA Error Log Directory length */
+ u32 flags; /* bit 0 - OS/VMM Opt-in */
+ u8 rev0[12];
+ u32 entries; /* Valid L1 Directory entries per logical processor */
+ u8 rev1[12];
+};
+
+static u8 extlog_dsm_uuid[] = "663E35AF-CC10-41A4-88EA-5470AF055295";
+
+/* L1 table related physical address */
+static u64 elog_base;
+static size_t elog_size;
+static u64 l1_dirbase;
+static size_t l1_size;
+
+/* L1 table related virtual address */
+static void __iomem *extlog_l1_addr;
+static void __iomem *elog_addr;
+
+static void *elog_buf;
+
+static u64 *l1_entry_base;
+static u32 l1_percpu_entry;
+
+#define ELOG_IDX(cpu, bank) \
+ (cpu_physical_id(cpu) * l1_percpu_entry + (bank))
+
+#define ELOG_ENTRY_DATA(idx) \
+ (*(l1_entry_base + (idx)))
+
+#define ELOG_ENTRY_ADDR(phyaddr) \
+ (phyaddr - elog_base + (u8 *)elog_addr)
+
+static struct acpi_generic_status *extlog_elog_entry_check(int cpu, int bank)
+{
+ int idx;
+ u64 data;
+ struct acpi_generic_status *estatus;
+
+ BUG_ON(cpu < 0);
+ idx = ELOG_IDX(cpu, bank);
+ data = ELOG_ENTRY_DATA(idx);
+ if ((data & ELOG_ENTRY_VALID) == 0)
+ return NULL;
+
+ data &= EXT_ELOG_ENTRY_MASK;
+ estatus = (struct acpi_generic_status *)ELOG_ENTRY_ADDR(data);
+
+ /* if no valid data in elog entry, just return */
+ if (estatus->block_status == 0)
+ return NULL;
+
+ return estatus;
+}
+
+static void __print_extlog_rcd(const char *pfx,
+ struct acpi_generic_status *estatus, int cpu)
+{
+ static atomic_t seqno;
+ unsigned int curr_seqno;
+ char pfx_seq[64];
+
+ if (pfx == NULL) {
+ if (estatus->error_severity <= CPER_SEV_CORRECTED)
+ pfx = KERN_INFO;
+ else
+ pfx = KERN_ERR;
+ }
+ curr_seqno = atomic_inc_return(&seqno);
+ snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
+ printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
+ cper_estatus_print(pfx_seq, estatus);
+}
+
+static int print_extlog_rcd(const char *pfx,
+ struct acpi_generic_status *estatus, int cpu)
+{
+ /* Not more than 2 messages every 5 seconds */
+ static DEFINE_RATELIMIT_STATE(ratelimit_corrected, 5*HZ, 2);
+ static DEFINE_RATELIMIT_STATE(ratelimit_uncorrected, 5*HZ, 2);
+ struct ratelimit_state *ratelimit;
+
+ if (estatus->error_severity == CPER_SEV_CORRECTED ||
+ (estatus->error_severity == CPER_SEV_INFORMATIONAL))
+ ratelimit = &ratelimit_corrected;
+ else
+ ratelimit = &ratelimit_uncorrected;
+ if (__ratelimit(ratelimit)) {
+ __print_extlog_rcd(pfx, estatus, cpu);
+ return 0;
+ }
+
+ return 1;
+}
+
+static int extlog_print(const char *pfx, int cpu, int bank)
+{
+ struct acpi_generic_status *estatus;
+ int rc;
+
+ estatus = extlog_elog_entry_check(cpu, bank);
+ if (estatus == NULL)
+ return -EINVAL;
+
+ memcpy(elog_buf, (void *)estatus, ELOG_ENTRY_LEN);
+ /* clear record status to enable BIOS to update it again */
+ estatus->block_status = 0;
+
+ rc = print_extlog_rcd(pfx, (struct acpi_generic_status *)elog_buf, cpu);
+
+ return rc;
+}
+
+static int extlog_get_dsm(acpi_handle handle, int rev, int func, u64 *ret)
+{
+ struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
+ struct acpi_object_list input;
+ union acpi_object params[4], *obj;
+ u8 uuid[16];
+ int i;
+
+ acpi_str_to_uuid(extlog_dsm_uuid, uuid);
+ input.count = 4;
+ input.pointer = params;
+ params[0].type = ACPI_TYPE_BUFFER;
+ params[0].buffer.length = 16;
+ params[0].buffer.pointer = uuid;
+ params[1].type = ACPI_TYPE_INTEGER;
+ params[1].integer.value = rev;
+ params[2].type = ACPI_TYPE_INTEGER;
+ params[2].integer.value = func;
+ params[3].type = ACPI_TYPE_PACKAGE;
+ params[3].package.count = 0;
+ params[3].package.elements = NULL;
+
+ if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DSM", &input, &buf)))
+ return -1;
+
+ *ret = 0;
+ obj = (union acpi_object *)buf.pointer;
+ if (obj->type == ACPI_TYPE_INTEGER)
+ *ret = obj->integer.value;
+ else if (obj->type == ACPI_TYPE_BUFFER) {
+ if (obj->buffer.length <= 8) {
+ for (i = 0; i < obj->buffer.length; i++)
+ *ret |= (obj->buffer.pointer[i] << (i * 8));
+ }
+ }
+ kfree(buf.pointer);
+
+ return 0;
+}
+
+static bool extlog_get_l1addr(void)
+{
+ acpi_handle handle;
+ u64 ret;
+
+ if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
+ goto fail;
+
+ if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_QUERY, &ret) ||
+ !(ret & EXTLOG_QUERY_L1_EXIST))
+ goto fail;
+
+ if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_ADDR, &ret))
+ goto fail;
+
+ l1_dirbase = ret;
+ /* Spec says L1 directory must be 4K aligned, bail out if it isn't */
+ if (l1_dirbase & ((1 << 12) - 1)) {
+ pr_warn(FW_BUG "L1 Directory is invalid at physical %llx\n",
+ l1_dirbase);
+ goto fail;
+ }
+
+ return true;
+fail:
+ return false;
+}
+
+static int __init extlog_init(void)
+{
+ struct extlog_l1_head *l1_head;
+ void __iomem *extlog_l1_hdr;
+ size_t l1_hdr_size;
+ unsigned long flags;
+ struct resource *r;
+ u64 cap;
+ int rc;
+
+ rc = -ENODEV;
+
+ rdmsrl(MSR_IA32_MCG_CAP, cap);
+ if (!(cap & MCG_EXT_ERR_LOG))
+ return rc;
+
+ if (!extlog_get_l1addr())
+ return rc;
+
+ rc = -EINVAL;
+ /* get L1 header to fetch necessary information */
+ l1_hdr_size = sizeof(struct extlog_l1_head);
+ r = request_mem_region(l1_dirbase, l1_hdr_size, "L1 DIR HDR");
+ if (!r) {
+ pr_warn(FW_BUG EMCA_BUG,
+ (unsigned long long)l1_dirbase,
+ (unsigned long long)l1_dirbase + l1_hdr_size);
+ goto err;
+ }
+
+ extlog_l1_hdr = acpi_os_map_memory(l1_dirbase, l1_hdr_size);
+ l1_head = (struct extlog_l1_head *)extlog_l1_hdr;
+ l1_size = l1_head->total_len;
+ l1_percpu_entry = l1_head->entries;
+ elog_base = l1_head->elog_base;
+ elog_size = l1_head->elog_len;
+ acpi_os_unmap_memory(extlog_l1_hdr, l1_hdr_size);
+ release_mem_region(l1_dirbase, l1_hdr_size);
+
+ /* remap L1 header again based on completed information */
+ r = request_mem_region(l1_dirbase, l1_size, "L1 Table");
+ if (!r) {
+ pr_warn(FW_BUG EMCA_BUG,
+ (unsigned long long)l1_dirbase,
+ (unsigned long long)l1_dirbase + l1_size);
+ goto err;
+ }
+ extlog_l1_addr = acpi_os_map_memory(l1_dirbase, l1_size);
+ l1_entry_base = (u64 *)((u8 *)extlog_l1_addr + l1_hdr_size);
+
+ /* remap elog table */
+ r = request_mem_region(elog_base, elog_size, "Elog Table");
+ if (!r) {
+ pr_warn(FW_BUG EMCA_BUG,
+ (unsigned long long)elog_base,
+ (unsigned long long)elog_base + elog_size);
+ goto err_release_l1_dir;
+ }
+ elog_addr = acpi_os_map_memory(elog_base, elog_size);
+
+ rc = -ENOMEM;
+ /* allocate buffer to save elog record */
+ elog_buf = kmalloc(ELOG_ENTRY_LEN, GFP_KERNEL);
+ if (elog_buf == NULL)
+ goto err_release_elog;
+
+ spin_lock_irqsave(&mce_elog_lock, flags);
+ mce_ext_err_print = extlog_print;
+ spin_unlock_irqrestore(&mce_elog_lock, flags);
+ /* enable OS to be involved to take over management from BIOS */
+ ((struct extlog_l1_head *)extlog_l1_addr)->flags |= FLAG_OS_OPTIN;
+
+ return 0;
+
+err_release_elog:
+ if (elog_addr)
+ acpi_os_unmap_memory(elog_addr, elog_size);
+ release_mem_region(elog_base, elog_size);
+err_release_l1_dir:
+ if (extlog_l1_addr)
+ acpi_os_unmap_memory(extlog_l1_addr, l1_size);
+ release_mem_region(l1_dirbase, l1_size);
+err:
+ pr_warn(FW_BUG "Extended error log disabled because of problems parsing f/w tables\n");
+ return rc;
+}
+
+static void __exit extlog_exit(void)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&mce_elog_lock, flags);
+ mce_ext_err_print = NULL;
+ spin_unlock_irqrestore(&mce_elog_lock, flags);
+ ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
+ if (extlog_l1_addr)
+ acpi_os_unmap_memory(extlog_l1_addr, l1_size);
+ if (elog_addr)
+ acpi_os_unmap_memory(elog_addr, elog_size);
+ release_mem_region(elog_base, elog_size);
+ release_mem_region(l1_dirbase, l1_size);
+ kfree(elog_buf);
+}
+
+module_init(extlog_init);
+module_exit(extlog_exit);
+
+MODULE_AUTHOR("Chen, Gong <gong...@intel.com>");
+MODULE_DESCRIPTION("Extended Error Log Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index b587ec8..e1bd9a1 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -174,7 +174,7 @@ static void acpi_print_osc_error(acpi_handle handle,
printk("\n");
}

-static acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
+acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
{
int i;
static int opc_map_to_uuid[16] = {6, 4, 2, 0, 11, 9, 16, 14, 19, 21,
@@ -195,6 +195,7 @@ static acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
}
return AE_OK;
}
+EXPORT_SYMBOL_GPL(acpi_str_to_uuid);

acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
{
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a5db4ae..c30bac8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -311,6 +311,7 @@ struct acpi_osc_context {
#define OSC_INVALID_REVISION_ERROR 8
#define OSC_CAPABILITIES_MASK_ERROR 16

+acpi_status acpi_str_to_uuid(char *str, u8 *uuid);
acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);

/* platform-wide _OSC bits */
--
1.8.4.rc3

Chen, Gong

unread,
Oct 11, 2013, 2:50:01 AM10/11/13
to
This patch adds a new interface to decode memory device (type 17)
to help error reporting on DIMMs.

Original-author: Tony Luck <tony...@intel.com>
Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
arch/ia64/kernel/setup.c | 1 +
arch/x86/kernel/setup.c | 1 +
drivers/firmware/dmi_scan.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/dmi.h | 5 ++++
4 files changed, 67 insertions(+)

diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
index 4fc2e95..d86669b 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -1063,6 +1063,7 @@ check_bugs (void)
static int __init run_dmi_scan(void)
{
dmi_scan_machine();
+ dmi_memdev_walk();
dmi_set_dump_stack_arch_desc();
return 0;
}
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f0de629..918d489 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -993,6 +993,7 @@ void __init setup_arch(char **cmdline_p)
efi_init();

dmi_scan_machine();
+ dmi_memdev_walk();
dmi_set_dump_stack_arch_desc();

/*
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index fa0affb..ca3619d 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -25,6 +25,13 @@ static int dmi_initialized;
/* DMI system identification string used during boot */
static char dmi_ids_string[128] __initdata;

+static struct dmi_memdev_info {
+ const char *device;
+ const char *bank;
+ u16 handle;
+} *dmi_memdev;
+static int dmi_memdev_nr;
+
static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
{
const u8 *bp = ((u8 *) dm) + dm->length;
@@ -322,6 +329,42 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
}

+static void __init count_mem_devices(const struct dmi_header *dm, void *v)
+{
+ if (dm->type != DMI_ENTRY_MEM_DEVICE)
+ return;
+ dmi_memdev_nr++;
+}
+
+static void __init save_mem_devices(const struct dmi_header *dm, void *v)
+{
+ const char *d = (const char *)dm;
+ static int nr;
+
+ if (dm->type != DMI_ENTRY_MEM_DEVICE)
+ return;
+ if (nr >= dmi_memdev_nr) {
+ pr_warn_once("Too many DIMM entries in SMBIOS table\n");
+ return;
+ }
+ dmi_memdev[nr].handle = dm->handle;
+ dmi_memdev[nr].device = dmi_string(dm, d[0x10]);
+ dmi_memdev[nr].bank = dmi_string(dm, d[0x11]);
+ nr++;
+}
+
+void __init dmi_memdev_walk(void)
+{
+ if (!dmi_available)
+ return;
+
+ if (dmi_walk_early(count_mem_devices) == 0 && dmi_memdev_nr) {
+ dmi_memdev = dmi_alloc(sizeof(*dmi_memdev) * dmi_memdev_nr);
+ if (dmi_memdev)
+ dmi_walk_early(save_mem_devices);
+ }
+}
+
/*
* Process a DMI table entry. Right now all we care about are the BIOS
* and machine entries. For 2.5 we should pull the smbus controller info
@@ -815,3 +858,20 @@ bool dmi_match(enum dmi_field f, const char *str)
return !strcmp(info, str);
}
EXPORT_SYMBOL_GPL(dmi_match);
+
+void dmi_memdev_name(u16 handle, const char **bank, const char **device)
+{
+ int n;
+
+ if (dmi_memdev == NULL)
+ return;
+
+ for (n = 0; n < dmi_memdev_nr; n++) {
+ if (handle == dmi_memdev[n].handle) {
+ *bank = dmi_memdev[n].bank;
+ *device = dmi_memdev[n].device;
+ break;
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(dmi_memdev_name);
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index b6eb7a0..f820f0a 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -99,6 +99,7 @@ extern const char * dmi_get_system_info(int field);
extern const struct dmi_device * dmi_find_device(int type, const char *name,
const struct dmi_device *from);
extern void dmi_scan_machine(void);
+extern void dmi_memdev_walk(void);
extern void dmi_set_dump_stack_arch_desc(void);
extern bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp);
extern int dmi_name_in_vendors(const char *str);
@@ -107,6 +108,7 @@ extern int dmi_available;
extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
void *private_data);
extern bool dmi_match(enum dmi_field f, const char *str);
+extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);

#else

@@ -115,6 +117,7 @@ static inline const char * dmi_get_system_info(int field) { return NULL; }
static inline const struct dmi_device * dmi_find_device(int type, const char *name,
const struct dmi_device *from) { return NULL; }
static inline void dmi_scan_machine(void) { return; }
+static inline void dmi_memdev_walk(void) { }
static inline void dmi_set_dump_stack_arch_desc(void) { }
static inline bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp)
{
@@ -133,6 +136,8 @@ static inline int dmi_walk(void (*decode)(const struct dmi_header *, void *),
void *private_data) { return -1; }
static inline bool dmi_match(enum dmi_field f, const char *str)
{ return false; }
+static inline void dmi_memdev_name(u16 handle, const char **bank,
+ const char **device) { }
static inline const struct dmi_system_id *
dmi_first_match(const struct dmi_system_id *list) { return NULL; }

--
1.8.4.rc3

Chen, Gong

unread,
Oct 11, 2013, 2:50:02 AM10/11/13
to
In latest UEFI spec(by now it is 2.4) memory error definition
for CPER (UEFI 2.4 Appendix N Common Platform Error Record)
adds some new fields. These fields help people to locate
memory error on actual DIMM location.

Original-author: Tony Luck <tony...@intel.com>
Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/apei/cper.c | 3 ++-
include/linux/cper.h | 7 +++++++
2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index b2e4134..680230c 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -8,7 +8,7 @@
* various tables, such as ERST, BERT and HEST etc.
*
* For more information about CPER, please refer to Appendix N of UEFI
- * Specification version 2.3.
+ * Specification version 2.4.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License version
@@ -191,6 +191,7 @@ static const char *cper_mem_err_type_strs[] = {
"memory sparing",
"scrub corrected error",
"scrub uncorrected error",
+ "Physical Memory Map-out event",
};

static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
diff --git a/include/linux/cper.h b/include/linux/cper.h
index c230494..bd01c9a 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -232,6 +232,9 @@ enum {
#define CPER_MEM_VALID_RESPONDER_ID 0x1000
#define CPER_MEM_VALID_TARGET_ID 0x2000
#define CPER_MEM_VALID_ERROR_TYPE 0x4000
+#define CPER_MEM_VALID_RANK_NUMBER 0x8000
+#define CPER_MEM_VALID_CARD_HANDLE 0x10000
+#define CPER_MEM_VALID_MODULE_HANDLE 0x20000

#define CPER_PCIE_VALID_PORT_TYPE 0x0001
#define CPER_PCIE_VALID_VERSION 0x0002
@@ -347,6 +350,10 @@ struct cper_sec_mem_err {
__u64 responder_id;
__u64 target_id;
__u8 error_type;
+ __u8 reserved;
+ __u16 rank;
+ __u16 mem_array_handle;
+ __u16 mem_dev_handle;
};

struct cper_sec_pcie {
--
1.8.4.rc3

Chen, Gong

unread,
Oct 11, 2013, 2:50:02 AM10/11/13
to
Commit aaf9d93 only catches condition check before print,
but the similar check is needed during printing CPER error
sections.

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/apei/cper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index 33dc6a0..f827f02 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -353,7 +353,7 @@ void apei_estatus_print(const char *pfx,
cper_severity_str(severity));
data_len = estatus->data_length;
gdata = (struct acpi_hest_generic_data *)(estatus + 1);
- while (data_len > sizeof(*gdata)) {
+ while (data_len >= sizeof(*gdata)) {
gedata_len = gdata->error_data_length;
apei_estatus_print_section(pfx, gdata, sec_no);
data_len -= gedata_len + sizeof(*gdata);
--
1.8.4.rc3

Chen, Gong

unread,
Oct 11, 2013, 2:50:02 AM10/11/13
to
To satisfy the necessary of following patches and make related definition
more clear, update some definitions about CPER. No functional changes.

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/apei/apei-internal.h | 12 ++++-----
drivers/acpi/apei/cper.c | 46 ++++++++++++++++-----------------
drivers/acpi/apei/ghes.c | 54 +++++++++++++++++++--------------------
include/acpi/actbl1.h | 14 +++++-----
include/acpi/ghes.h | 2 +-
5 files changed, 64 insertions(+), 64 deletions(-)

diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
index f220d64..21ba34a 100644
--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -122,11 +122,11 @@ struct dentry;
struct dentry *apei_get_debugfs_dir(void);

#define apei_estatus_for_each_section(estatus, section) \
- for (section = (struct acpi_hest_generic_data *)(estatus + 1); \
+ for (section = (struct acpi_generic_data *)(estatus + 1); \
(void *)section - (void *)estatus < estatus->data_length; \
section = (void *)(section+1) + section->error_data_length)

-static inline u32 apei_estatus_len(struct acpi_hest_generic_status *estatus)
+static inline u32 cper_estatus_len(struct acpi_generic_status *estatus)
{
if (estatus->raw_data_length)
return estatus->raw_data_offset + \
@@ -135,10 +135,10 @@ static inline u32 apei_estatus_len(struct acpi_hest_generic_status *estatus)
return sizeof(*estatus) + estatus->data_length;
}

-void apei_estatus_print(const char *pfx,
- const struct acpi_hest_generic_status *estatus);
-int apei_estatus_check_header(const struct acpi_hest_generic_status *estatus);
-int apei_estatus_check(const struct acpi_hest_generic_status *estatus);
+void cper_estatus_print(const char *pfx,
+ const struct acpi_generic_status *estatus);
+int cper_estatus_check_header(const struct acpi_generic_status *estatus);
+int cper_estatus_check(const struct acpi_generic_status *estatus);

int apei_osc_setup(void);
#endif
diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index f827f02..b2e4134 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -5,7 +5,7 @@
* Author: Huang Ying <ying....@intel.com>
*
* CPER is the format used to describe platform hardware error by
- * various APEI tables, such as ERST, BERT and HEST etc.
+ * various tables, such as ERST, BERT and HEST etc.
*
* For more information about CPER, please refer to Appendix N of UEFI
* Specification version 2.3.
@@ -248,7 +248,7 @@ static const char *cper_pcie_port_type_strs[] = {
};

static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
- const struct acpi_hest_generic_data *gdata)
+ const struct acpi_generic_data *gdata)
{
if (pcie->validation_bits & CPER_PCIE_VALID_PORT_TYPE)
printk("%s""port_type: %d, %s\n", pfx, pcie->port_type,
@@ -283,17 +283,17 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
pfx, pcie->bridge.secondary_status, pcie->bridge.control);
}

-static const char *apei_estatus_section_flag_strs[] = {
+static const char *cper_estatus_section_flag_strs[] = {
"primary",
"containment warning",
"reset",
- "threshold exceeded",
+ "error threshold exceeded",
"resource not accessible",
"latent error",
};

-static void apei_estatus_print_section(
- const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
+static void cper_estatus_print_section(
+ const char *pfx, const struct acpi_generic_data *gdata, int sec_no)
{
uuid_le *sec_type = (uuid_le *)gdata->section_type;
__u16 severity;
@@ -302,8 +302,8 @@ static void apei_estatus_print_section(
printk("%s""section: %d, severity: %d, %s\n", pfx, sec_no, severity,
cper_severity_str(severity));
printk("%s""flags: 0x%02x\n", pfx, gdata->flags);
- cper_print_bits(pfx, gdata->flags, apei_estatus_section_flag_strs,
- ARRAY_SIZE(apei_estatus_section_flag_strs));
+ cper_print_bits(pfx, gdata->flags, cper_estatus_section_flag_strs,
+ ARRAY_SIZE(cper_estatus_section_flag_strs));
if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
printk("%s""fru_id: %pUl\n", pfx, (uuid_le *)gdata->fru_id);
if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
@@ -339,34 +339,34 @@ err_section_too_small:
pr_err(FW_WARN "error section length is too small\n");
}

-void apei_estatus_print(const char *pfx,
- const struct acpi_hest_generic_status *estatus)
+void cper_estatus_print(const char *pfx,
+ const struct acpi_generic_status *estatus)
{
- struct acpi_hest_generic_data *gdata;
+ struct acpi_generic_data *gdata;
unsigned int data_len, gedata_len;
int sec_no = 0;
__u16 severity;

- printk("%s""APEI generic hardware error status\n", pfx);
+ printk("%s""Generic Hardware Error Status\n", pfx);
severity = estatus->error_severity;
printk("%s""severity: %d, %s\n", pfx, severity,
cper_severity_str(severity));
data_len = estatus->data_length;
- gdata = (struct acpi_hest_generic_data *)(estatus + 1);
+ gdata = (struct acpi_generic_data *)(estatus + 1);
while (data_len >= sizeof(*gdata)) {
gedata_len = gdata->error_data_length;
- apei_estatus_print_section(pfx, gdata, sec_no);
+ cper_estatus_print_section(pfx, gdata, sec_no);
data_len -= gedata_len + sizeof(*gdata);
gdata = (void *)(gdata + 1) + gedata_len;
sec_no++;
}
}
-EXPORT_SYMBOL_GPL(apei_estatus_print);
+EXPORT_SYMBOL_GPL(cper_estatus_print);

-int apei_estatus_check_header(const struct acpi_hest_generic_status *estatus)
+int cper_estatus_check_header(const struct acpi_generic_status *estatus)
{
if (estatus->data_length &&
- estatus->data_length < sizeof(struct acpi_hest_generic_data))
+ estatus->data_length < sizeof(struct acpi_generic_data))
return -EINVAL;
if (estatus->raw_data_length &&
estatus->raw_data_offset < sizeof(*estatus) + estatus->data_length)
@@ -374,19 +374,19 @@ int apei_estatus_check_header(const struct acpi_hest_generic_status *estatus)

return 0;
}
-EXPORT_SYMBOL_GPL(apei_estatus_check_header);
+EXPORT_SYMBOL_GPL(cper_estatus_check_header);

-int apei_estatus_check(const struct acpi_hest_generic_status *estatus)
+int cper_estatus_check(const struct acpi_generic_status *estatus)
{
- struct acpi_hest_generic_data *gdata;
+ struct acpi_generic_data *gdata;
unsigned int data_len, gedata_len;
int rc;

- rc = apei_estatus_check_header(estatus);
+ rc = cper_estatus_check_header(estatus);
if (rc)
return rc;
data_len = estatus->data_length;
- gdata = (struct acpi_hest_generic_data *)(estatus + 1);
+ gdata = (struct acpi_generic_data *)(estatus + 1);
while (data_len >= sizeof(*gdata)) {
gedata_len = gdata->error_data_length;
if (gedata_len > data_len - sizeof(*gdata))
@@ -399,4 +399,4 @@ int apei_estatus_check(const struct acpi_hest_generic_status *estatus)

return 0;
}
-EXPORT_SYMBOL_GPL(apei_estatus_check);
+EXPORT_SYMBOL_GPL(cper_estatus_check);
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 8ec37bb..0db6e4f 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -75,13 +75,13 @@
#define GHES_ESTATUS_CACHE_LEN(estatus_len) \
(sizeof(struct ghes_estatus_cache) + (estatus_len))
#define GHES_ESTATUS_FROM_CACHE(estatus_cache) \
- ((struct acpi_hest_generic_status *) \
+ ((struct acpi_generic_status *) \
((struct ghes_estatus_cache *)(estatus_cache) + 1))

#define GHES_ESTATUS_NODE_LEN(estatus_len) \
(sizeof(struct ghes_estatus_node) + (estatus_len))
-#define GHES_ESTATUS_FROM_NODE(estatus_node) \
- ((struct acpi_hest_generic_status *) \
+#define GHES_ESTATUS_FROM_NODE(estatus_node) \
+ ((struct acpi_generic_status *) \
((struct ghes_estatus_node *)(estatus_node) + 1))

bool ghes_disable;
@@ -378,17 +378,17 @@ static int ghes_read_estatus(struct ghes *ghes, int silent)
ghes->flags |= GHES_TO_CLEAR;

rc = -EIO;
- len = apei_estatus_len(ghes->estatus);
+ len = cper_estatus_len(ghes->estatus);
if (len < sizeof(*ghes->estatus))
goto err_read_block;
if (len > ghes->generic->error_block_length)
goto err_read_block;
- if (apei_estatus_check_header(ghes->estatus))
+ if (cper_estatus_check_header(ghes->estatus))
goto err_read_block;
ghes_copy_tofrom_phys(ghes->estatus + 1,
buf_paddr + sizeof(*ghes->estatus),
len - sizeof(*ghes->estatus), 1);
- if (apei_estatus_check(ghes->estatus))
+ if (cper_estatus_check(ghes->estatus))
goto err_read_block;
rc = 0;

@@ -409,7 +409,7 @@ static void ghes_clear_estatus(struct ghes *ghes)
ghes->flags &= ~GHES_TO_CLEAR;
}

-static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int sev)
+static void ghes_handle_memory_failure(struct acpi_generic_data *gdata, int sev)
{
#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
unsigned long pfn;
@@ -438,10 +438,10 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
}

static void ghes_do_proc(struct ghes *ghes,
- const struct acpi_hest_generic_status *estatus)
+ const struct acpi_generic_status *estatus)
{
int sev, sec_sev;
- struct acpi_hest_generic_data *gdata;
+ struct acpi_generic_data *gdata;

sev = ghes_severity(estatus->error_severity);
apei_estatus_for_each_section(estatus, gdata) {
@@ -496,7 +496,7 @@ static void ghes_do_proc(struct ghes *ghes,

static void __ghes_print_estatus(const char *pfx,
const struct acpi_hest_generic *generic,
- const struct acpi_hest_generic_status *estatus)
+ const struct acpi_generic_status *estatus)
{
static atomic_t seqno;
unsigned int curr_seqno;
@@ -513,12 +513,12 @@ static void __ghes_print_estatus(const char *pfx,
snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}" HW_ERR, pfx, curr_seqno);
printk("%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
pfx_seq, generic->header.source_id);
- apei_estatus_print(pfx_seq, estatus);
+ cper_estatus_print(pfx_seq, estatus);
}

static int ghes_print_estatus(const char *pfx,
const struct acpi_hest_generic *generic,
- const struct acpi_hest_generic_status *estatus)
+ const struct acpi_generic_status *estatus)
{
/* Not more than 2 messages every 5 seconds */
static DEFINE_RATELIMIT_STATE(ratelimit_corrected, 5*HZ, 2);
@@ -540,15 +540,15 @@ static int ghes_print_estatus(const char *pfx,
* GHES error status reporting throttle, to report more kinds of
* errors, instead of just most frequently occurred errors.
*/
-static int ghes_estatus_cached(struct acpi_hest_generic_status *estatus)
+static int ghes_estatus_cached(struct acpi_generic_status *estatus)
{
u32 len;
int i, cached = 0;
unsigned long long now;
struct ghes_estatus_cache *cache;
- struct acpi_hest_generic_status *cache_estatus;
+ struct acpi_generic_status *cache_estatus;

- len = apei_estatus_len(estatus);
+ len = cper_estatus_len(estatus);
rcu_read_lock();
for (i = 0; i < GHES_ESTATUS_CACHES_SIZE; i++) {
cache = rcu_dereference(ghes_estatus_caches[i]);
@@ -571,19 +571,19 @@ static int ghes_estatus_cached(struct acpi_hest_generic_status *estatus)

static struct ghes_estatus_cache *ghes_estatus_cache_alloc(
struct acpi_hest_generic *generic,
- struct acpi_hest_generic_status *estatus)
+ struct acpi_generic_status *estatus)
{
int alloced;
u32 len, cache_len;
struct ghes_estatus_cache *cache;
- struct acpi_hest_generic_status *cache_estatus;
+ struct acpi_generic_status *cache_estatus;

alloced = atomic_add_return(1, &ghes_estatus_cache_alloced);
if (alloced > GHES_ESTATUS_CACHE_ALLOCED_MAX) {
atomic_dec(&ghes_estatus_cache_alloced);
return NULL;
}
- len = apei_estatus_len(estatus);
+ len = cper_estatus_len(estatus);
cache_len = GHES_ESTATUS_CACHE_LEN(len);
cache = (void *)gen_pool_alloc(ghes_estatus_pool, cache_len);
if (!cache) {
@@ -603,7 +603,7 @@ static void ghes_estatus_cache_free(struct ghes_estatus_cache *cache)
{
u32 len;

- len = apei_estatus_len(GHES_ESTATUS_FROM_CACHE(cache));
+ len = cper_estatus_len(GHES_ESTATUS_FROM_CACHE(cache));
len = GHES_ESTATUS_CACHE_LEN(len);
gen_pool_free(ghes_estatus_pool, (unsigned long)cache, len);
atomic_dec(&ghes_estatus_cache_alloced);
@@ -619,7 +619,7 @@ static void ghes_estatus_cache_rcu_free(struct rcu_head *head)

static void ghes_estatus_cache_add(
struct acpi_hest_generic *generic,
- struct acpi_hest_generic_status *estatus)
+ struct acpi_generic_status *estatus)
{
int i, slot = -1, count;
unsigned long long now, duration, period, max_period = 0;
@@ -751,7 +751,7 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
struct llist_node *llnode, *next;
struct ghes_estatus_node *estatus_node;
struct acpi_hest_generic *generic;
- struct acpi_hest_generic_status *estatus;
+ struct acpi_generic_status *estatus;
u32 len, node_len;

llnode = llist_del_all(&ghes_estatus_llist);
@@ -765,7 +765,7 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
estatus_node = llist_entry(llnode, struct ghes_estatus_node,
llnode);
estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
- len = apei_estatus_len(estatus);
+ len = cper_estatus_len(estatus);
node_len = GHES_ESTATUS_NODE_LEN(len);
ghes_do_proc(estatus_node->ghes, estatus);
if (!ghes_estatus_cached(estatus)) {
@@ -784,7 +784,7 @@ static void ghes_print_queued_estatus(void)
struct llist_node *llnode;
struct ghes_estatus_node *estatus_node;
struct acpi_hest_generic *generic;
- struct acpi_hest_generic_status *estatus;
+ struct acpi_generic_status *estatus;
u32 len, node_len;

llnode = llist_del_all(&ghes_estatus_llist);
@@ -797,7 +797,7 @@ static void ghes_print_queued_estatus(void)
estatus_node = llist_entry(llnode, struct ghes_estatus_node,
llnode);
estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
- len = apei_estatus_len(estatus);
+ len = cper_estatus_len(estatus);
node_len = GHES_ESTATUS_NODE_LEN(len);
generic = estatus_node->generic;
ghes_print_estatus(NULL, generic, estatus);
@@ -843,7 +843,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
#ifdef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG
u32 len, node_len;
struct ghes_estatus_node *estatus_node;
- struct acpi_hest_generic_status *estatus;
+ struct acpi_generic_status *estatus;
#endif
if (!(ghes->flags & GHES_TO_CLEAR))
continue;
@@ -851,7 +851,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
if (ghes_estatus_cached(ghes->estatus))
goto next;
/* Save estatus for further processing in IRQ context */
- len = apei_estatus_len(ghes->estatus);
+ len = cper_estatus_len(ghes->estatus);
node_len = GHES_ESTATUS_NODE_LEN(len);
estatus_node = (void *)gen_pool_alloc(ghes_estatus_pool,
node_len);
@@ -923,7 +923,7 @@ static int ghes_probe(struct platform_device *ghes_dev)

rc = -EIO;
if (generic->error_block_length <
- sizeof(struct acpi_hest_generic_status)) {
+ sizeof(struct acpi_generic_status)) {
pr_warning(FW_BUG GHES_PFX "Invalid error block length: %u for generic hardware error source: %d\n",
generic->error_block_length,
generic->header.source_id);
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 0bd750e..3c62a0a 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -596,7 +596,7 @@ struct acpi_hest_generic {

/* Generic Error Status block */

-struct acpi_hest_generic_status {
+struct acpi_generic_status {
u32 block_status;
u32 raw_data_offset;
u32 raw_data_length;
@@ -606,15 +606,15 @@ struct acpi_hest_generic_status {

/* Values for block_status flags above */

-#define ACPI_HEST_UNCORRECTABLE (1)
-#define ACPI_HEST_CORRECTABLE (1<<1)
-#define ACPI_HEST_MULTIPLE_UNCORRECTABLE (1<<2)
-#define ACPI_HEST_MULTIPLE_CORRECTABLE (1<<3)
-#define ACPI_HEST_ERROR_ENTRY_COUNT (0xFF<<4) /* 8 bits, error count */
+#define ACPI_GEN_ERR_UC (1)
+#define ACPI_GEN_ERR_CE (1<<1)
+#define ACPI_GEN_ERR_MULTI_UC (1<<2)
+#define ACPI_GEN_ERR_MULTI_CE (1<<3)
+#define ACPI_GEN_ERR_COUNT_SHIFT (0xFF<<4) /* 8 bits, error count */

/* Generic Error Data entry */

-struct acpi_hest_generic_data {
+struct acpi_generic_data {
u8 section_type[16];
u32 error_severity;
u16 revision;
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 720446c..dfd60d0 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -14,7 +14,7 @@

struct ghes {
struct acpi_hest_generic *generic;
- struct acpi_hest_generic_status *estatus;
+ struct acpi_generic_status *estatus;
u64 buffer_paddr;
unsigned long flags;
union {
--
1.8.4.rc3

Joe Perches

unread,
Oct 11, 2013, 3:10:02 AM10/11/13
to
On Fri, 2013-10-11 at 02:32 -0400, Chen, Gong wrote:
> This patch series adds an enhanced MCA event logging driver provided by Intel.
[]
> dmesg output:
>
> [56005.785917] {3}Hardware error detected on CPU0
> [56005.785959] {3}event severity: corrected
> [56005.785975] {3}sub_event[0], severity: corrected
> [56005.785977] {3}section_type: memory error
> [56005.785981] {3}physical_address: 0x0000000851fe0000
> [56005.786027] {3}DIMM location: Memriser1 CHANNEL A DIMM 0
> [56005.786154] {4}Hardware error detected on CPU0
> [56005.786159] {4}event severity: corrected
> [56005.786162] {4}sub_event[0], severity: corrected
> [56005.786166] {4}section_type: memory error
[]
> So welcome to add comments for what is needed or not.

Perhaps there can be a better standardized prefix other than {seq}
like KBUILD_MODNAME or some other useful identifier like subsystem
name.

Borislav Petkov

unread,
Oct 11, 2013, 4:00:02 AM10/11/13
to
On Fri, Oct 11, 2013 at 02:32:46AM -0400, Chen, Gong wrote:
> diff --git a/drivers/acpi/extlog_trace.h b/drivers/acpi/extlog_trace.h
> new file mode 100644
> index 0000000..21f0887
> --- /dev/null
> +++ b/drivers/acpi/extlog_trace.h
> @@ -0,0 +1,77 @@
> +#if !defined(_TRACE_EXTLOG_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_EXTLOG_H
> +
> +#include <linux/tracepoint.h>
> +#include <linux/cper.h>
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM extlog
> +
> +/*
> + * MCE Extended Error Log Trace event

Right, we have a perfectly good header for ras TPs:

include/ras/ras_event.h

Mind adding this TP there please?
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 11, 2013, 4:10:02 AM10/11/13
to
On Fri, Oct 11, 2013 at 02:32:38AM -0400, Chen, Gong wrote:
> [56005.785917] {3}Hardware error detected on CPU0
> [56005.785959] {3}event severity: corrected
> [56005.785975] {3}sub_event[0], severity: corrected
> [56005.785977] {3}section_type: memory error
> [56005.785981] {3}physical_address: 0x0000000851fe0000
> [56005.786027] {3}DIMM location: Memriser1 CHANNEL A DIMM 0

Very good guys, I've been waiting for years for this to be possible,
good job! :-)

Btw, what's "Memriser1"?

> [56005.786154] {4}Hardware error detected on CPU0
> [56005.786159] {4}event severity: corrected
> [56005.786162] {4}sub_event[0], severity: corrected

This sub_event[0] could use better decoding though.

> [56005.786166] {4}section_type: memory error
>
>
> trace output:
>
> # tracer: nop
> #
> # entries-in-buffer/entries-written: 4/4 #P:120
> #
> # _-----=> irqs-off
> # / _----=> need-resched
> # | / _---=> hardirq/softirq
> # || / _--=> preempt-depth
> # ||| / delay
> # TASK-PID CPU# |||| TIMESTAMP FUNCTION
> # | | | |||| | |
> ...
> ...
> <idle>-0 [000] d.h. 56068.488759: extlog_mem_event: 3 corrected errors:unknown

That "unknown" thing needs a " " in front of it and comes from
cper_mem_err_type_str, AFAICT. I'm guessing the value is 0 and
uninitialized or so?

> on Memriser1 CHANNEL A DIMM 0(FRU:

Also another " " missing here.

> 00000000-0000-0000-0000-000000000000 physical addr: 0x0000000851fe0000 node: 0 card: 0 module: 0 rank: 0 bank: 0 row: 28927 column: 1296)
> <idle>-0 [000] d.h. 56068.488834: extlog_mem_event: 4 corrected errors:unknown
> ...
> ...
>
> dmesg output are shrank to only keep the most important data. The trace
> output will contain most of data. Not sure if all fields are meaningful
> to users. Some fields like FRU ID/FRU TEXT depends on BIOS manufactor.
> So welcome to add comments for what is needed or not.

Yeah, I guess we again depend on BIOS people to fill those in. I'd
expect serious server manifacturers who care about RAS to do so...

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 11, 2013, 5:00:03 AM10/11/13
to
On Fri, Oct 11, 2013 at 02:32:39AM -0400, Chen, Gong wrote:
> Commit aaf9d93 only catches condition check before print,
> but the similar check is needed during printing CPER error
> sections.
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>

Reviewed-by: Borislav Petkov <b...@suse.de>

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 11, 2013, 5:10:02 AM10/11/13
to
On Fri, Oct 11, 2013 at 02:32:40AM -0400, Chen, Gong wrote:
> To satisfy the necessary of following patches and make related definition

"To prepare for the following patches... " you mean?

> more clear, update some definitions about CPER. No functional changes.
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> ---
> drivers/acpi/apei/apei-internal.h | 12 ++++-----
> drivers/acpi/apei/cper.c | 46 ++++++++++++++++-----------------
> drivers/acpi/apei/ghes.c | 54 +++++++++++++++++++--------------------
> include/acpi/actbl1.h | 14 +++++-----
> include/acpi/ghes.h | 2 +-
> 5 files changed, 64 insertions(+), 64 deletions(-)

[ … ]

> diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
> index f827f02..b2e4134 100644
> --- a/drivers/acpi/apei/cper.c
> +++ b/drivers/acpi/apei/cper.c
> @@ -5,7 +5,7 @@
> * Author: Huang Ying <ying....@intel.com>
> *
> * CPER is the format used to describe platform hardware error by
> - * various APEI tables, such as ERST, BERT and HEST etc.
> + * various tables, such as ERST, BERT and HEST etc.
> *
> * For more information about CPER, please refer to Appendix N of UEFI
> * Specification version 2.3.
> @@ -248,7 +248,7 @@ static const char *cper_pcie_port_type_strs[] = {
> };
>
> static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
> - const struct acpi_hest_generic_data *gdata)
> + const struct acpi_generic_data *gdata)
> {
> if (pcie->validation_bits & CPER_PCIE_VALID_PORT_TYPE)
> printk("%s""port_type: %d, %s\n", pfx, pcie->port_type,
> @@ -283,17 +283,17 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
> pfx, pcie->bridge.secondary_status, pcie->bridge.control);
> }
>
> -static const char *apei_estatus_section_flag_strs[] = {
> +static const char *cper_estatus_section_flag_strs[] = {

"static const char * const" while at it.

Btw, please run your patches through checkpatch.pl - it does catch
things like that.

> "primary",
> "containment warning",
> "reset",
> - "threshold exceeded",
> + "error threshold exceeded",

Right, this string is user-visible so if we have to be absolutely
honest, the patch does add "functional changes" so you can remove the
statement from the commit message. :)
Btw, what's the story with printk not using KERN_x levels in this file?
Why are we falling back to default printk levels for all printks here
and shouldn't we rather prioritize them by urgency into, say, KERN_ERR,
KERN_INFO, etc?

> severity = estatus->error_severity;
> printk("%s""severity: %d, %s\n", pfx, severity,
> cper_severity_str(severity));
> data_len = estatus->data_length;
> - gdata = (struct acpi_hest_generic_data *)(estatus + 1);
> + gdata = (struct acpi_generic_data *)(estatus + 1);
> while (data_len >= sizeof(*gdata)) {
> gedata_len = gdata->error_data_length;
> - apei_estatus_print_section(pfx, gdata, sec_no);
> + cper_estatus_print_section(pfx, gdata, sec_no);
> data_len -= gedata_len + sizeof(*gdata);
> gdata = (void *)(gdata + 1) + gedata_len;
> sec_no++;
> }
> }
> -EXPORT_SYMBOL_GPL(apei_estatus_print);
> +EXPORT_SYMBOL_GPL(cper_estatus_print);

[ ... ]

> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index 0bd750e..3c62a0a 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -596,7 +596,7 @@ struct acpi_hest_generic {
>
> /* Generic Error Status block */
>
> -struct acpi_hest_generic_status {
> +struct acpi_generic_status {
> u32 block_status;
> u32 raw_data_offset;
> u32 raw_data_length;
> @@ -606,15 +606,15 @@ struct acpi_hest_generic_status {
>
> /* Values for block_status flags above */
>
> -#define ACPI_HEST_UNCORRECTABLE (1)
> -#define ACPI_HEST_CORRECTABLE (1<<1)
> -#define ACPI_HEST_MULTIPLE_UNCORRECTABLE (1<<2)
> -#define ACPI_HEST_MULTIPLE_CORRECTABLE (1<<3)
> -#define ACPI_HEST_ERROR_ENTRY_COUNT (0xFF<<4) /* 8 bits, error count */
> +#define ACPI_GEN_ERR_UC (1)
> +#define ACPI_GEN_ERR_CE (1<<1)
> +#define ACPI_GEN_ERR_MULTI_UC (1<<2)
> +#define ACPI_GEN_ERR_MULTI_CE (1<<3)

Those can simply use BIT().

> +#define ACPI_GEN_ERR_COUNT_SHIFT (0xFF<<4) /* 8 bits, error count */

Other than the above nits, a patch which slims down struct names and
makes them more concrete is always welcome :)

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Luck, Tony

unread,
Oct 11, 2013, 11:00:02 AM10/11/13
to
>> [56005.785981] {3}physical_address: 0x0000000851fe0000
>> [56005.786027] {3}DIMM location: Memriser1 CHANNEL A DIMM 0
>
> Very good guys, I've been waiting for years for this to be possible,
> good job! :-)

It's such a simple goal - I can't believe it took this long to get here :-)

> Btw, what's "Memriser1"?

Each memory controller on this machine routes to a plug-in card that has
a bunch of DIMMs on it. The silkscreen labels on the motherboard for these
cards read "Memriser1" "Memriser2" etc.

-Tony

Borislav Petkov

unread,
Oct 11, 2013, 11:30:02 AM10/11/13
to
Let's keep them numerically sorted by the bit numbers so put
MCG_EXT_ERR_LOG after bit 24, MCG_SER_P.

Besides, this bit should be called MCG_ELOG_P as it is in the docs
although your name is much more descriptive than what the hw guys came
up with but I know they like to abbreviate to the lowest letter count
possible :)

> @@ -204,6 +205,10 @@ extern void mce_disable_bank(int bank);
> * Exception handler
> */
>
> +extern spinlock_t mce_elog_lock;

Uuh, I don't think we want to expose the naked spinlock. Rather, we'd
need a couple of functions

mce_elog_lock
mce_elog_unlock

which get called by users.

Actually, it would be even better to keep all those details private
to acpi_extlog.c and have machine_check_poll() call extlog_print()
and in the cases where there's no extlog support, you have an empty
extlog_print function.

> +extern int (*mce_extlog_support)(void);

Unused leftover?

> +/* Call the installed extended error log print function */
> +extern int (*mce_ext_err_print)(const char *, int cpu, int bank);
> /* Call the installed machine check handler for this CPU setup. */
> extern void (*machine_check_vector)(struct pt_regs *, long error_code);
> void do_machine_check(struct pt_regs *, long);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index b3218cd..6802637 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -48,6 +48,12 @@
>
> #include "mce-internal.h"
>
> +DEFINE_SPINLOCK(mce_elog_lock);
> +EXPORT_SYMBOL_GPL(mce_elog_lock);

Yeah, private to acpi_extlog and it can be simply 'elog_lock' there,
without the "mce_" prefix.

> +
> +int (*mce_ext_err_print)(const char *, int, int) = NULL;
> +EXPORT_SYMBOL_GPL(mce_ext_err_print);
> +
> static DEFINE_MUTEX(mce_chrdev_read_mutex);
>
> #define rcu_dereference_check_mce(p) \
> @@ -624,6 +630,10 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
> (m.status & (mca_cfg.ser ? MCI_STATUS_S : MCI_STATUS_UC)))
> continue;
>
> + spin_lock(&mce_elog_lock);
> + if (mce_ext_err_print)
> + mce_ext_err_print(NULL, m.extcpu, i);
> + spin_unlock(&mce_elog_lock);

private to acpi_extlog.c

> mce_read_aux(&m, i);
>
> if (!(flags & MCP_TIMESTAMP))
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 22327e6..1465fa8 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -372,4 +372,12 @@ config ACPI_BGRT
>
> source "drivers/acpi/apei/Kconfig"
>
> +config ACPI_EXTLOG
> + tristate "Extended Error Log support"
> + depends on X86 && X86_MCE

I guess we can depend only on X86_MCE

> + default n
> + help
> + This driver adds support for decoding extended errors from hardware.
> + which allows the operating system to obtain data from trace.

Let's make this description a bit better:

"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 plus an additional special
tracepoint which carries that information to userspace."
Please no more of that boilerplate. Simply say that this file is
licensed under GPLv2 and that's it.

> + */
> +
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <acpi/acpi_bus.h>
> +#include <linux/cper.h>
> +#include <linux/ratelimit.h>
> +#include <asm/mce.h>
> +
> +#include "apei/apei-internal.h"
> +
> +#define EXT_ELOG_ENTRY_MASK 0xfffffffffffff /* elog entry address mask */

Am I reading this correctly that these are bits [51:0]?

Btw, we have a GENMASK macro in drivers/edac/amd64_edac.h which can be used for
generating contiguous bitmasks:

#define EXT_ELOG_ENTRY_MASK GENMASK(0, 51)

which makes it much more readable.

You could lift it into a more global header like, say,
arch/x86/include/asm/edac.h maybe...
What is this supposed to guard? And why such a heavy hammer?

> + idx = ELOG_IDX(cpu, bank);
> + data = ELOG_ENTRY_DATA(idx);
> + if ((data & ELOG_ENTRY_VALID) == 0)
> + return NULL;
> +
> + data &= EXT_ELOG_ENTRY_MASK;
> + estatus = (struct acpi_generic_status *)ELOG_ENTRY_ADDR(data);
> +
> + /* if no valid data in elog entry, just return */
> + if (estatus->block_status == 0)
> + return NULL;
> +
> + return estatus;
> +}
> +
> +static void __print_extlog_rcd(const char *pfx,
> + struct acpi_generic_status *estatus, int cpu)

Please align args after the opening bracket.

> +{
> + static atomic_t seqno;
> + unsigned int curr_seqno;
> + char pfx_seq[64];
> +
> + if (pfx == NULL) {

if (!pfx)

> + if (estatus->error_severity <= CPER_SEV_CORRECTED)
> + pfx = KERN_INFO;
> + else
> + pfx = KERN_ERR;
> + }
> + curr_seqno = atomic_inc_return(&seqno);
> + snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
> + printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
> + cper_estatus_print(pfx_seq, estatus);
> +}
> +
> +static int print_extlog_rcd(const char *pfx,
> + struct acpi_generic_status *estatus, int cpu)a

Ditto.

> +{
> + /* Not more than 2 messages every 5 seconds */
> + static DEFINE_RATELIMIT_STATE(ratelimit_corrected, 5*HZ, 2);
> + static DEFINE_RATELIMIT_STATE(ratelimit_uncorrected, 5*HZ, 2);
> + struct ratelimit_state *ratelimit;
> +
> + if (estatus->error_severity == CPER_SEV_CORRECTED ||
> + (estatus->error_severity == CPER_SEV_INFORMATIONAL))

alignment:

if ( estatus->error_severity == CPER_SEV_CORRECTED ||
(estatus->error_severity == CPER_SEV_INFORMATIONAL))

> + ratelimit = &ratelimit_corrected;
> + else
> + ratelimit = &ratelimit_uncorrected;
> + if (__ratelimit(ratelimit)) {
> + __print_extlog_rcd(pfx, estatus, cpu);

Btw, __print_extlog_rcd is only called once, AFAICT. Why not fold it
in here?
I'm guessing this is how acpi does allocation - ACPI_ALLOCATE_BUFFER and
caller has to free it?

> +
> + return 0;
> +}
> +
> +static bool extlog_get_l1addr(void)
> +{
> + acpi_handle handle;
> + u64 ret;
> +
> + if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> + goto fail;
> +
> + if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_QUERY, &ret) ||
> + !(ret & EXTLOG_QUERY_L1_EXIST))
> + goto fail;
> +
> + if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_ADDR, &ret))
> + goto fail;
> +
> + l1_dirbase = ret;
> + /* Spec says L1 directory must be 4K aligned, bail out if it isn't */
> + if (l1_dirbase & ((1 << 12) - 1)) {

& (PAGE_SIZE - 1)

> + pr_warn(FW_BUG "L1 Directory is invalid at physical %llx\n",
> + l1_dirbase);
> + goto fail;
> + }
> +
> + return true;
> +fail:
> + return false;

You probably could drop the labels and simply do "return false" in the
error cases as it is obvious.

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 11, 2013, 11:30:03 AM10/11/13
to
On Fri, Oct 11, 2013 at 02:54:13PM +0000, Luck, Tony wrote:
> It's such a simple goal - I can't believe it took this long to get
> here :-)

Right, I'd guess some standard's body needed to be persuaded :-)

> > Btw, what's "Memriser1"?
>
> Each memory controller on this machine routes to a plug-in card that has
> a bunch of DIMMs on it. The silkscreen labels on the motherboard for these
> cards read "Memriser1" "Memriser2" etc.

Silkscreen labels? Good!

Borislav Petkov

unread,
Oct 11, 2013, 11:50:02 AM10/11/13
to
On Fri, Oct 11, 2013 at 02:32:43AM -0400, Chen, Gong wrote:
> In latest UEFI spec(by now it is 2.4) memory error definition
> for CPER (UEFI 2.4 Appendix N Common Platform Error Record)
> adds some new fields. These fields help people to locate
> memory error on actual DIMM location.
>
> Original-author: Tony Luck <tony...@intel.com>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>

Reviewed-by: Borislav Petkov <b...@suse.de>

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 11, 2013, 11:50:02 AM10/11/13
to
Just a nitpick: maybe better arg alignment:

printk("%s""DIMM DMI handle: 0x%.4x",
pfx, mem->mem_dev_handle);

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 11, 2013, 11:50:03 AM10/11/13
to
On Fri, Oct 11, 2013 at 02:32:42AM -0400, Chen, Gong wrote:
> This patch adds a new interface to decode memory device (type 17)
> to help error reporting on DIMMs.
>
> Original-author: Tony Luck <tony...@intel.com>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>

Reviewed-by: Borislav Petkov <b...@suse.de>

Just a question below:

> ---
> arch/ia64/kernel/setup.c | 1 +
> arch/x86/kernel/setup.c | 1 +
> drivers/firmware/dmi_scan.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/dmi.h | 5 ++++
> 4 files changed, 67 insertions(+)
>

[ ... ]
Is this and count_mem_devices() some sort of precaution against insane
DMI tables?

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 11, 2013, 11:50:03 AM10/11/13
to
On Fri, Oct 11, 2013 at 11:06:30AM +0200, Borislav Petkov wrote:
> > - printk("%s""APEI generic hardware error status\n", pfx);
> > + printk("%s""Generic Hardware Error Status\n", pfx);
>
> Btw, what's the story with printk not using KERN_x levels in this file?
> Why are we falling back to default printk levels for all printks here
> and shouldn't we rather prioritize them by urgency into, say, KERN_ERR,
> KERN_INFO, etc?

Ignore that - checkpatch complained about it but I kinda missed that
we're handing down the prefix.

Borislav Petkov

unread,
Oct 11, 2013, 12:10:01 PM10/11/13
to
Hmm, so this cper_print_mem is called for CPER_SEC_PLATFORM_MEM section
type.

With the change above, the other caller __ghes_print_estatus() won't see
the error messages if they're debug. Do we want that?

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 11, 2013, 12:20:01 PM10/11/13
to
On Fri, Oct 11, 2013 at 02:32:46AM -0400, Chen, Gong wrote:
Why the separate config item?

> +
> config ACPI_EXTLOG
> tristate "Extended Error Log support"
> depends on X86 && X86_MCE
> + select EXTLOG_TRACE
> default n
> help
> This driver adds support for decoding extended errors from hardware.
> - which allows the operating system to obtain data from trace.
> + which allows the operating system to obtain data from trace. It will
> + appear under /sys/kernel/debug/tracing/ras/ .
>
> endif # ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index bce34af..a6e41b7 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -83,4 +83,8 @@ obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
>
> obj-$(CONFIG_ACPI_APEI) += apei/
>
> +# extended log support
> +acpi-$(CONFIG_EXTLOG_TRACE) += extlog_trace.o

You can simply do

obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o extlog_trace.o

> +CFLAGS_extlog_trace.o := -I$(src)
> +
> obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o

[ ... ]
By reversing this test you can save yourself an indentation level and a
superfluous memset:

if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
return;

memset(dimm_location, 0, LOC_LEN);
dmi_memdev_name...
...


> + const char *bank = NULL, *device = NULL;
> + dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
> + 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);
> + }
> +}
> +
> +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 = 0;
> +
> + if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
> + etype = mem->error_type;
> + if (mem->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
> + phy_addr = mem->physical_addr;
> + if (mem->validation_bits &
> + CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK)

This mask could use some slimming:

CPER_MEM_VALID_PA_MASK or CPER_MEM_VALID_PHYS_ADDR_MASK or so so that it
fits in 80 cols.

> + phy_addr &= mem->physical_addr_mask;
> + }
> + 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);

arg alignment

> +}
> +EXPORT_SYMBOL_GPL(trace_mem_error);
> diff --git a/drivers/acpi/extlog_trace.h b/drivers/acpi/extlog_trace.h
> new file mode 100644
> index 0000000..21f0887
> --- /dev/null
> +++ b/drivers/acpi/extlog_trace.h
> @@ -0,0 +1,77 @@
> +#if !defined(_TRACE_EXTLOG_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_EXTLOG_H
> +
> +#include <linux/tracepoint.h>
> +#include <linux/cper.h>
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM extlog

Shouldn't that be TRACE_SYSTEM ras

...

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen Gong

unread,
Oct 13, 2013, 11:40:02 PM10/13/13
to
On Fri, Oct 11, 2013 at 05:24:52PM +0200, Borislav Petkov wrote:
> Date: Fri, 11 Oct 2013 17:24:52 +0200
> From: Borislav Petkov <b...@alien8.de>
> To: "Chen, Gong" <gong...@linux.intel.com>
> Cc: tony...@intel.com, linux-...@vger.kernel.org,
> linux...@vger.kernel.org
> Subject: Re: [PATCH 3/8] ACPI, x86: Extended error log driver for x86
> platform
> User-Agent: Mutt/1.5.21 (2010-09-15)
But this driver can be loaded as a module. If this module is unloaded,
extlog_print is gone. I can't keep such a pointer internally.

> > +extern int (*mce_extlog_support)(void);
>
> Unused leftover?
>
Yes, it should be deleted.
This macro is great and I'd loved to use it. But it looks like a litttle bit
weird to let eMCA depends on a header file like edac.h. Meanwhile, I found
in drivers/video/sis/init.c:3323 we have a very similar macro for this
purpose. So how about writing a separate patch to clean it up first?
Because I think in theory "CPU < 0" is impossible. When it hits such
situation, it should be a very serious H/W or firmware bug. At least,
It think it should be a WARN_ON.
Just make codes cleaner.
Yes it is.
signature.asc

Chen Gong

unread,
Oct 13, 2013, 11:40:02 PM10/13/13
to
On Fri, Oct 11, 2013 at 05:40:48PM +0200, Borislav Petkov wrote:
> Date: Fri, 11 Oct 2013 17:40:48 +0200
> Subject: Re: [PATCH 4/8] DMI: Parse memory device (type 17) in SMBIOS
> User-Agent: Mutt/1.5.21 (2010-09-15)
Yes, but we highly expect BIOS manufactors to make is valid and complete.
signature.asc

Chen Gong

unread,
Oct 14, 2013, 1:10:01 AM10/14/13
to
On Fri, Oct 11, 2013 at 06:02:08PM +0200, Borislav Petkov wrote:
> Date: Fri, 11 Oct 2013 18:02:08 +0200
> Subject: Re: [PATCH 7/8] ACPI, APEI, CPER: Cleanup CPER memory error output
> format
> User-Agent: Mutt/1.5.21 (2010-09-15)
Because most of data in CPER are empty or unimportant. To avoid too much
disturbance to end users, it is worthy to do that. Moreover, I reserve
another way via trace to show these data.
signature.asc

Chen Gong

unread,
Oct 14, 2013, 3:10:01 AM10/14/13
to
On Fri, Oct 11, 2013 at 10:04:27AM +0200, Borislav Petkov wrote:
> Date: Fri, 11 Oct 2013 10:04:27 +0200
> Subject: Re: Extended H/W error log driver
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On Fri, Oct 11, 2013 at 02:32:38AM -0400, Chen, Gong wrote:
> > [56005.785917] {3}Hardware error detected on CPU0
> > [56005.785959] {3}event severity: corrected
> > [56005.785975] {3}sub_event[0], severity: corrected
> > [56005.785977] {3}section_type: memory error
> > [56005.785981] {3}physical_address: 0x0000000851fe0000
> > [56005.786027] {3}DIMM location: Memriser1 CHANNEL A DIMM 0
>
> Very good guys, I've been waiting for years for this to be possible,
> good job! :-)
>
> Btw, what's "Memriser1"?
>
> > [56005.786154] {4}Hardware error detected on CPU0
> > [56005.786159] {4}event severity: corrected
> > [56005.786162] {4}sub_event[0], severity: corrected
>
> This sub_event[0] could use better decoding though.
>

What's your suggestion?

signature.asc

Chen Gong

unread,
Oct 14, 2013, 3:30:02 AM10/14/13
to
On Fri, Oct 11, 2013 at 06:14:36PM +0200, Borislav Petkov wrote:
> Date: Fri, 11 Oct 2013 18:14:36 +0200
> Subject: Re: [PATCH 8/8] ACPI / trace: Add trace interface for eMCA driver
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
...
> > +static void dimm_err_location(struct cper_sec_mem_err *mem)
> > +{
> > + memset(dimm_location, 0, LOC_LEN);
> > + if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
>
> By reversing this test you can save yourself an indentation level and a
> superfluous memset:
>
> if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
> return;
>
> memset(dimm_location, 0, LOC_LEN);
> dmi_memdev_name...
> ...
>
>
memset should be called before return, otherwise the values from last time
will happen again in this time.
signature.asc

Borislav Petkov

unread,
Oct 14, 2013, 6:30:02 AM10/14/13
to
On Sun, Oct 13, 2013 at 11:16:33PM -0400, Chen Gong wrote:
> But this driver can be loaded as a module. If this module is unloaded,
> extlog_print is gone. I can't keep such a pointer internally.

Sure you can - you define a weak extlog_print() function in a
compilation unit which is always builtin. Maybe mce.c or so.

> This macro is great and I'd loved to use it. But it looks like a
> litttle bit weird to let eMCA depends on a header file like edac.h.
> Meanwhile, I found in drivers/video/sis/init.c:3323 we have a very
> similar macro for this purpose. So how about writing a separate patch
> to clean it up first?

Actually, you're right. Those macros are much more generic and
could be exposed to the general public by putting them, say into
include/include/bitops.h, for example?

Btw, the sis one generates unsigneds (4 byte on x86) while the edac one
8 byte ULLs. So you could call them

GENMASK
and
GENMASK_ULL

How does that sound?

> Because I think in theory "CPU < 0" is impossible. When it hits such
> situation, it should be a very serious H/W or firmware bug. At least,
> It think it should be a WARN_ON.

Yes, I think a WARN_ON is much better than the heavy hammer. We can
always turn it into a FW_BUG later if it really starts to trigger
anywhere...

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 14, 2013, 6:40:02 AM10/14/13
to
On Sun, Oct 13, 2013 at 11:21:13PM -0400, Chen Gong wrote:
> Yes, but we highly expect BIOS manufactors to make is valid and
> complete.

Well, you can always do sanity checks with FW_BUG noisy printks. I don't
see any other way to move fw vendors - simply expecting them is not an
incentive enough, IMO. :-)

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 14, 2013, 6:40:02 AM10/14/13
to
On Mon, Oct 14, 2013 at 12:55:00AM -0400, Chen Gong wrote:
> Because most of data in CPER are empty or unimportant.

It is not about whether it is important or not - the question is whether
changing existing functionality which someone might rely upon is a
problem here? Someone might be expecting exactly those messages to
appear in dmesg.

Btw, what is the real reason for this patch, to save yourself the output
in dmesg? If so, you can disable this output when eMCA module has been
loaded but leave it intact otherwise.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 14, 2013, 7:00:02 AM10/14/13
to
Well, if this only enumerates the sections in CPER, then we can simply
drop it.

Btw, I was wondering, why are we dropping
cper_estatus_section_flag_strs? This is again the same issue with
changing output which people might already rely upon.

Chen Gong

unread,
Oct 14, 2013, 9:20:03 AM10/14/13
to
On Mon, Oct 14, 2013 at 12:26:35PM +0200, Borislav Petkov wrote:
> Date: Mon, 14 Oct 2013 12:26:35 +0200
> From: Borislav Petkov <b...@alien8.de>
> To: Chen Gong <gong...@linux.intel.com>
> Cc: tony...@intel.com, linux-...@vger.kernel.org,
> linux...@vger.kernel.org
> Subject: Re: [PATCH 3/8] ACPI, x86: Extended error log driver for x86
> platform
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On Sun, Oct 13, 2013 at 11:16:33PM -0400, Chen Gong wrote:
> > But this driver can be loaded as a module. If this module is unloaded,
> > extlog_print is gone. I can't keep such a pointer internally.
>
> Sure you can - you define a weak extlog_print() function in a
> compilation unit which is always builtin. Maybe mce.c or so.
>
Oh, yes. Let me do it.

> > This macro is great and I'd loved to use it. But it looks like a
> > litttle bit weird to let eMCA depends on a header file like edac.h.
> > Meanwhile, I found in drivers/video/sis/init.c:3323 we have a very
> > similar macro for this purpose. So how about writing a separate patch
> > to clean it up first?
>
> Actually, you're right. Those macros are much more generic and
> could be exposed to the general public by putting them, say into
> include/include/bitops.h, for example?
>
> Btw, the sis one generates unsigneds (4 byte on x86) while the edac one
> 8 byte ULLs. So you could call them
>
> GENMASK
> and
> GENMASK_ULL
>
> How does that sound?

This kind of mask is often unsigned. So how about following mode:

GENMASKL / GENMASKQ
or
GENMASK_L / GENMASK_Q

>
> > Because I think in theory "CPU < 0" is impossible. When it hits such
> > situation, it should be a very serious H/W or firmware bug. At least,
> > It think it should be a WARN_ON.
>
> Yes, I think a WARN_ON is much better than the heavy hammer. We can
> always turn it into a FW_BUG later if it really starts to trigger
> anywhere...
>
Agree.
signature.asc

Borislav Petkov

unread,
Oct 14, 2013, 9:30:03 AM10/14/13
to
On Mon, Oct 14, 2013 at 09:03:11AM -0400, Chen Gong wrote:
> This kind of mask is often unsigned. So how about following mode:
>
> GENMASKL / GENMASKQ
> or
> GENMASK_L / GENMASK_Q

Right, I think the "_ULL" thing is more accepted in the kernel, see
DIV_ROUND_UP{,_ULL}. Also, Joe had a patch to convert BIT to that
scheme too:

https://lkml.org/lkml/2013/9/19/307

There are macros with "_Q" but it does not necessarily mean Quadword but
something else like queues and stuff.

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Tony Luck

unread,
Oct 14, 2013, 1:00:01 PM10/14/13
to
On Mon, Oct 14, 2013 at 3:26 AM, Borislav Petkov <b...@alien8.de> wrote:
> On Sun, Oct 13, 2013 at 11:16:33PM -0400, Chen Gong wrote:
>> But this driver can be loaded as a module. If this module is unloaded,
>> extlog_print is gone. I can't keep such a pointer internally.
>
> Sure you can - you define a weak extlog_print() function in a
> compilation unit which is always builtin. Maybe mce.c or so.

"weak" is a good compile/link time tool to provide a default function
while allowing override with a architecture (or more generally a CONFIG_*)
specific one. But it is no help when loading/unloading modules.

Think about it ... we have a call to this function from some place in the
base kernel (originating from mce.o). Before the module is loaded you
want that to leap to your weak function.

Now load the module with the not-weak definition of the function - the
module loader would have to go do a relocation fix-up in the base kernel
to point to this new function. At module unload it would have to undo
that.

-Tony

Borislav Petkov

unread,
Oct 14, 2013, 1:10:02 PM10/14/13
to
On Mon, Oct 14, 2013 at 09:50:00AM -0700, Tony Luck wrote:
> Now load the module with the not-weak definition of the function -
> the module loader would have to go do a relocation fix-up in the base
> kernel to point to this new function. At module unload it would have
> to undo that.

Ok, then. How about a reg/dereg functionality, something like what I did
in drivers/edac/mce_amd.c, near the top? We're basically handing down a
proper function pointer to call and at module unload time we clear it.

Also, those register/unregister functions could be made to return an
errval so that code calling them can handle that gracefully.

Bottom line is, IMO we're much better off having a clearly defined
interface like that instead of exporting a naked function pointer.

Thoughts?

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Tony Luck

unread,
Oct 14, 2013, 1:20:02 PM10/14/13
to
On Mon, Oct 14, 2013 at 10:07 AM, Borislav Petkov <b...@alien8.de> wrote:
> Ok, then. How about a reg/dereg functionality, something like what I did
> in drivers/edac/mce_amd.c, near the top? We're basically handing down a
> proper function pointer to call and at module unload time we clear it.

Yes - register and unregister functions would be better than

> exporting a naked function pointer.

-Tony

Tony Luck

unread,
Oct 14, 2013, 1:20:02 PM10/14/13
to
On Mon, Oct 14, 2013 at 3:36 AM, Borislav Petkov <b...@alien8.de> wrote:
> On Mon, Oct 14, 2013 at 12:55:00AM -0400, Chen Gong wrote:
>> Because most of data in CPER are empty or unimportant.
>
> It is not about whether it is important or not - the question is whether
> changing existing functionality which someone might rely upon is a
> problem here? Someone might be expecting exactly those messages to
> appear in dmesg.

Pulling in a couple more people who have been touching error reporting
code in the last year or so (Hi Lance, Naveen ... feel free to drag more
people to look at this thread).

I prodded Chen Gong in to make this change because our console messages
are way to verbose (and scary) for simple corrected errors. There are 18
fields in the memory error section (as of UEFI 2.4 ... more are likely to be
added because there are issues that some of the 16-bit wide fields are too
small to handle increased internal values in modern DIMMs). Whether you
print that one item per line, or a few very long lines - it is way
more information
than the average user will ever want or need to see.

> Btw, what is the real reason for this patch, to save yourself the output
> in dmesg? If so, you can disable this output when eMCA module has been
> loaded but leave it intact otherwise.

This is an excellent idea - if the full data is being logged via TRACE, then
we can drop to virtually nothing on the console (just have something similar
to the "Machine check events logged" message so that people will get a
tip that they might want to go dig into other logs). Maybe something like:
%d corrected memory errors\n", count
[rate limited]

But we'd have to make sure that the existing user(s) of this code also
have a TRACE path.

-Tony

Borislav Petkov

unread,
Oct 14, 2013, 2:50:02 PM10/14/13
to
On Mon, Oct 14, 2013 at 10:12:35AM -0700, Tony Luck wrote:
> This is an excellent idea - if the full data is being logged via
> TRACE, then we can drop to virtually nothing on the console (just have
> something similar to the "Machine check events logged" message so that
> people will get a tip that they might want to go dig into other logs).
> Maybe something like: %d corrected memory errors\n", count [rate
> limited]
>
> But we'd have to make sure that the existing user(s) of this code also
> have a TRACE path.

It is basically the same idea as with the ras daemon - if we have a
userspace consumer of ras trace events, we disable dmesg output. So the
decision will be left to the userspace tool to disable dmesg output as a
last step of its initialization.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Tony Luck

unread,
Oct 14, 2013, 5:10:02 PM10/14/13
to
On Mon, Oct 14, 2013 at 11:47 AM, Borislav Petkov <b...@alien8.de> wrote:
> It is basically the same idea as with the ras daemon - if we have a
> userspace consumer of ras trace events, we disable dmesg output. So the
> decision will be left to the userspace tool to disable dmesg output as a
> last step of its initialization.

Do you have a suggested mechanism for this disabling of dmesg?

-Tony

Borislav Petkov

unread,
Oct 14, 2013, 6:00:02 PM10/14/13
to
On Mon, Oct 14, 2013 at 02:03:16PM -0700, Tony Luck wrote:
> Do you have a suggested mechanism for this disabling of dmesg?

Hmm, how about a 64-bit flag variable (we can use the remaining bits
for other stuff later) called x86_ras_flags which is private to
arch/x86/ras/core.c (a new file)...

[ btw, I'm thinking of something similar to efi's x86_efi_facility which we
nicely query with test_bit and set with set_bit and clear_bit, etc, etc ]

Also, look at arch/x86/platform/efi/efi.c::efi_enabled() how it hides
the actual variable and we can do something similar so that eMCA and
other users like cper.c can do

apei_estatus_print_section:

if (!ras_tracepoint_enabled())
cper_print_mem(...)

We set the bit in x86_ras_flags from, say, debugfs, i.e., a userspace
tool sets it and from that moment on all RAS output is rerouted to the
trace event. I.e., the output for which there is a trace event...

How does that look like?

Purely hypothetical, of course, I might me missing something but it
looks ok from here. As always, the devil is in the detail, of course.

HTH.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen Gong

unread,
Oct 15, 2013, 12:30:01 AM10/15/13
to
On Mon, Oct 14, 2013 at 12:55:33PM +0200, Borislav Petkov wrote:
> Date: Mon, 14 Oct 2013 12:55:33 +0200
> From: Borislav Petkov <b...@alien8.de>
> To: Chen Gong <gong...@linux.intel.com>
> Cc: tony...@intel.com, linux-...@vger.kernel.org,
> linux...@vger.kernel.org
> Subject: Re: Extended H/W error log driver
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On Mon, Oct 14, 2013 at 02:49:40AM -0400, Chen Gong wrote:
> > On Fri, Oct 11, 2013 at 10:04:27AM +0200, Borislav Petkov wrote:
> > > > [56005.786154] {4}Hardware error detected on CPU0
> > > > [56005.786159] {4}event severity: corrected
> > > > [56005.786162] {4}sub_event[0], severity: corrected
> > >
> > > This sub_event[0] could use better decoding though.
> > >
> > What's your suggestion?
>
> Well, if this only enumerates the sections in CPER, then we can simply
> drop it.
>
Some errors have multiple sub sections like below:

[ 1442.070522] {2}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 0
[ 1442.070528] {2}[Hardware Error]: event severity: corrected
[ 1442.070531] {2}[Hardware Error]: sub_event[0], severity: corrected
[ 1442.070534] {2}[Hardware Error]: section_type: memory error
[ 1442.070537] {2}[Hardware Error]: error_status: 0x0000000000000000
[ 1442.070539] {2}[Hardware Error]: sub_event[1], severity: corrected
[ 1442.070541] {2}[Hardware Error]: section_type: memory error
[ 1442.070543] {2}[Hardware Error]: error_status: 0x0000000000000000

> Btw, I was wondering, why are we dropping
> cper_estatus_section_flag_strs? This is again the same issue with
> changing output which people might already rely upon.
>

This depends on how we shrink the output information. Your reply in
another patch looks a good idea. Let me try it first.
signature.asc

Borislav Petkov

unread,
Oct 15, 2013, 5:30:02 AM10/15/13
to
On Tue, Oct 15, 2013 at 12:07:31AM -0400, Chen Gong wrote:
> Some errors have multiple sub sections like below:
>
> [ 1442.070522] {2}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 0
> [ 1442.070528] {2}[Hardware Error]: event severity: corrected
> [ 1442.070531] {2}[Hardware Error]: sub_event[0], severity: corrected
> [ 1442.070534] {2}[Hardware Error]: section_type: memory error
> [ 1442.070537] {2}[Hardware Error]: error_status: 0x0000000000000000
> [ 1442.070539] {2}[Hardware Error]: sub_event[1], severity: corrected
> [ 1442.070541] {2}[Hardware Error]: section_type: memory error
> [ 1442.070543] {2}[Hardware Error]: error_status: 0x0000000000000000

Right, and what do those sub sections mean to the user? Did we have
multiple errors?

It looks like this because we have memory errors section type but it is
not very telling. How about:


[ 1442.070522] {2}[Hardware Error]: APEI GHES id 0: Hardware errors logged
[ 1442.070528] {2}[Hardware Error]: event severity: corrected
[ 1442.070534] {2}[Hardware Error]: Error 0, type: corrected memory error.
[ 1442.070537] {2}[Hardware Error]: error_status: 0x0000000000000000
[ 1442.070539] {2}[Hardware Error]: Error 1, type: corrected memory error.
[ 1442.070543] {2}[Hardware Error]: error_status: 0x0000000000000000

I think this is much more human readable and understandable :-)

We can even add a hint for the user like:

"Above errors have been corrected by the hardware and require no further action."

Btw, this is valid for both dmesg and trace event output.

Because from my experience so far people just scream: "Look, I just had
an MCE" withot even reading what it says. And this just upsets support
people for no valid reason at all.

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen Gong

unread,
Oct 15, 2013, 5:40:02 AM10/15/13
to
On Mon, Oct 14, 2013 at 11:50:47PM +0200, Borislav Petkov wrote:
> Date: Mon, 14 Oct 2013 23:50:47 +0200
> From: Borislav Petkov <b...@alien8.de>
> To: Tony Luck <tony...@gmail.com>
> Cc: Chen Gong <gong...@linux.intel.com>, Linux Kernel Mailing List
> <linux-...@vger.kernel.org>, linux-acpi <linux...@vger.kernel.org>,
> Lance Ortiz <lance...@hp.com>, "Naveen N. Rao"
> <naveen...@linux.vnet.ibm.com>
> Subject: Re: [PATCH 7/8] ACPI, APEI, CPER: Cleanup CPER memory error output
> format
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On Mon, Oct 14, 2013 at 02:03:16PM -0700, Tony Luck wrote:
> > Do you have a suggested mechanism for this disabling of dmesg?
>
> Hmm, how about a 64-bit flag variable (we can use the remaining bits
> for other stuff later) called x86_ras_flags which is private to
> arch/x86/ras/core.c (a new file)...
>
> [ btw, I'm thinking of something similar to efi's x86_efi_facility which we
> nicely query with test_bit and set with set_bit and clear_bit, etc, etc ]
>
> Also, look at arch/x86/platform/efi/efi.c::efi_enabled() how it hides
> the actual variable and we can do something similar so that eMCA and
> other users like cper.c can do
>
> apei_estatus_print_section:
>
> if (!ras_tracepoint_enabled())
> cper_print_mem(...)
>
> We set the bit in x86_ras_flags from, say, debugfs, i.e., a userspace
> tool sets it and from that moment on all RAS output is rerouted to the
> trace event. I.e., the output for which there is a trace event...
>
> How does that look like?
>

Looks fine to me. But a little bit out of current patch series. How
about adding such interfaces after this patch series is merged?
And from your words, it looks like you prefer to reserve all current
fields to avoid breaking user space things. IOW, drop patch [7/8]
and use another patch with above idea to get the same purpose. This
is what you want, Boris?

signature.asc

Borislav Petkov

unread,
Oct 15, 2013, 6:20:01 AM10/15/13
to
On Tue, Oct 15, 2013 at 05:18:35AM -0400, Chen Gong wrote:
> Looks fine to me. But a little bit out of current patch series. How
> about adding such interfaces after this patch series is merged?

Ok.

> And from your words, it looks like you prefer to reserve all current
> fields to avoid breaking user space things. IOW, drop patch [7/8]

Well, I was simply wondering whether something is using those. And since
we don't know, apparently, we probably should keep them for now.

> and use another patch with above idea to get the same purpose. This
> is what you want, Boris?

Yeah, let's not break userspace. :-)

Thanks.

Naveen N. Rao

unread,
Oct 15, 2013, 7:30:01 AM10/15/13
to
On 10/14/2013 10:42 PM, Tony Luck wrote:
> On Mon, Oct 14, 2013 at 3:36 AM, Borislav Petkov <b...@alien8.de> wrote:
>> On Mon, Oct 14, 2013 at 12:55:00AM -0400, Chen Gong wrote:
>>> Because most of data in CPER are empty or unimportant.
>>
>> It is not about whether it is important or not - the question is whether
>> changing existing functionality which someone might rely upon is a
>> problem here? Someone might be expecting exactly those messages to
>> appear in dmesg.
>
> Pulling in a couple more people who have been touching error reporting
> code in the last year or so (Hi Lance, Naveen ... feel free to drag more
> people to look at this thread).
>
> I prodded Chen Gong in to make this change because our console messages
> are way to verbose (and scary) for simple corrected errors. There are 18
> fields in the memory error section (as of UEFI 2.4 ... more are likely to be
> added because there are issues that some of the 16-bit wide fields are too
> small to handle increased internal values in modern DIMMs). Whether you
> print that one item per line, or a few very long lines - it is way
> more information
> than the average user will ever want or need to see.

I completely agree and I am all for bringing down the verbosity of GHES
logs. In my testing, a corrected error event reported through GHES takes
upwards of 10 lines, which is far too much. Perhaps a single line per
GHES event with only a few important fields would be better?


Thanks,
Naveen

Naveen N. Rao

unread,
Oct 15, 2013, 7:50:02 AM10/15/13
to
On 10/14/2013 10:42 PM, Tony Luck wrote:
> On Mon, Oct 14, 2013 at 3:36 AM, Borislav Petkov <b...@alien8.de> wrote:
>> On Mon, Oct 14, 2013 at 12:55:00AM -0400, Chen Gong wrote:
>>> Because most of data in CPER are empty or unimportant.
>>
>> It is not about whether it is important or not - the question is whether
>> changing existing functionality which someone might rely upon is a
>> problem here? Someone might be expecting exactly those messages to
>> appear in dmesg.

If so, how will disabling dmesg output and switching to a trace event
help? The user-space program will still break unless they add support
for that trace event, right?

I'm not sure if we actually provide guarantees w.r.t kernel log
messages. The issue in this specific scenario is the sheer verbosity of
the log messages. I personally feel that it will be good if we can
simplify the log output.

Thanks,
Naveen

Borislav Petkov

unread,
Oct 15, 2013, 8:40:03 AM10/15/13
to
On Tue, Oct 15, 2013 at 05:11:59PM +0530, Naveen N. Rao wrote:
> If so, how will disabling dmesg output and switching to a trace event
> help? The user-space program will still break unless they add support
> for that trace event, right?

Well, as yourself this: what happens to a userspace program which relies
on this *exact* error message format and greps dmesg?

> I'm not sure if we actually provide guarantees w.r.t kernel log
> messages. The issue in this specific scenario is the sheer verbosity
> of the log messages.

No, the issue is what I said above: userspace programs expecting exactly
this output. Once we exposed it to userspace, we cannot simply assume
that nothing is using it.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Tony Luck

unread,
Oct 15, 2013, 12:20:02 PM10/15/13
to
On Tue, Oct 15, 2013 at 2:28 AM, Borislav Petkov <b...@alien8.de> wrote:
> We can even add a hint for the user like:
>
> "Above errors have been corrected by the hardware and require no further action."
>
> Btw, this is valid for both dmesg and trace event output.
>
> Because from my experience so far people just scream: "Look, I just had
> an MCE" withot even reading what it says. And this just upsets support
> people for no valid reason at all.

"Like", "+1", "metoo" ... or whatever it is we do on LKML to indicate
we approve.

-Tony

Tony Luck

unread,
Oct 15, 2013, 12:50:01 PM10/15/13
to
On Tue, Oct 15, 2013 at 9:42 AM, Joe Perches <j...@perches.com> wrote:
> No guarantees are made about dmesg output.

I'm with Joe here. Current users that parse dmesg have grumbled a bit
that format changes - but they know that's the way life is. Perhaps they'll
be too busy cheering about the TRACE formats that we are going to give
them that they won't whine too much about this.

-Tony

Joe Perches

unread,
Oct 15, 2013, 12:50:02 PM10/15/13
to
On Tue, 2013-10-15 at 14:29 +0200, Borislav Petkov wrote:
> Once we exposed it to userspace, we cannot simply assume
> that nothing is using it.

Yes, we can.

No guarantees are made about dmesg output.

If guarantees were made, no typo could ever be fixed
nor could any conversion from printk to dev_<level>
be done.

Naveen N. Rao

unread,
Oct 15, 2013, 1:00:01 PM10/15/13
to
On 2013/10/11 02:32AM, Chen Gong wrote:
> Use trace interface to elaborate all H/W error related
> information.
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> ---
<snip>
> +TRACE_EVENT(extlog_mem_event,
> + TP_PROTO(u32 etype,
> + char *dimm_loc,
> + const uuid_le *fru_id,
> + char *fru_text,
> + u64 error_count,
> + u32 severity,
> + u64 phy_addr,
> + char *mem_loc),

[Adding Mauro...]

This looks very similar to the trace event I wrote a while back,
which was similar to the one provided by ghes_edac:
http://thread.gmane.org/gmane.linux.kernel.pci/24616

Seems to me this has the same issues we previously discussed w.r.t
EDAC conflicts...


Regards,
Naveen

Borislav Petkov

unread,
Oct 15, 2013, 1:00:02 PM10/15/13
to
On Tue, Oct 15, 2013 at 09:49:06AM -0700, Tony Luck wrote:
> On Tue, Oct 15, 2013 at 9:42 AM, Joe Perches <j...@perches.com> wrote:
> > No guarantees are made about dmesg output.
>
> I'm with Joe here. Current users that parse dmesg have grumbled a bit
> that format changes - but they know that's the way life is. Perhaps they'll
> be too busy cheering about the TRACE formats that we are going to give
> them that they won't whine too much about this.

Ok, I'll make sure to forward all complaints to you and Joe then. :)

In any case, we talked about it, you think it is not worth preserving
it, we can drop the printks and see who cries out.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 15, 2013, 1:10:02 PM10/15/13
to
On Tue, Oct 15, 2013 at 10:24:35PM +0530, Naveen N. Rao wrote:
> On 2013/10/11 02:32AM, Chen Gong wrote:
> > Use trace interface to elaborate all H/W error related
> > information.
> >
> > Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> > ---
> <snip>
> > +TRACE_EVENT(extlog_mem_event,
> > + TP_PROTO(u32 etype,
> > + char *dimm_loc,
> > + const uuid_le *fru_id,
> > + char *fru_text,
> > + u64 error_count,
> > + u32 severity,
> > + u64 phy_addr,
> > + char *mem_loc),
>
> [Adding Mauro...]
>
> This looks very similar to the trace event I wrote a while back,
> which was similar to the one provided by ghes_edac:
> http://thread.gmane.org/gmane.linux.kernel.pci/24616
>
> Seems to me this has the same issues we previously discussed w.r.t
> EDAC conflicts...

Right, I'm inclined to leave this trace_mc_event in ras_event.h to edac
use alone because of all those layers which don't mean whit for GHES and
eMCA error sources.

And maybe define a trace_mem_event which is shared by GHES and eMCA and
not use the edac tracepoint there not load ghes_edac on such systems
which have sufficient decoding capability in firmware.

Thoughts?

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Naveen N. Rao

unread,
Oct 15, 2013, 1:30:02 PM10/15/13
to
On 2013/10/11 02:32AM, Chen Gong wrote:
> In latest UEFI spec(by now it is 2.4) memory error definition
> for CPER (UEFI 2.4 Appendix N Common Platform Error Record)
> adds some new fields. These fields help people to locate
> memory error on actual DIMM location.
>
> Original-author: Tony Luck <tony...@intel.com>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> ---
> drivers/acpi/apei/cper.c | 3 ++-
> include/linux/cper.h | 7 +++++++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
> index b2e4134..680230c 100644
> --- a/drivers/acpi/apei/cper.c
> +++ b/drivers/acpi/apei/cper.c
> @@ -8,7 +8,7 @@
> * various tables, such as ERST, BERT and HEST etc.
> *
> * For more information about CPER, please refer to Appendix N of UEFI
> - * Specification version 2.3.
> + * Specification version 2.4.
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License version
> @@ -191,6 +191,7 @@ static const char *cper_mem_err_type_strs[] = {
> "memory sparing",
> "scrub corrected error",
> "scrub uncorrected error",
> + "Physical Memory Map-out event",

All small letters to match the rest of the items:
"physical memory map-out event"

> };
>
> static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> diff --git a/include/linux/cper.h b/include/linux/cper.h
> index c230494..bd01c9a 100644
> --- a/include/linux/cper.h
> +++ b/include/linux/cper.h
> @@ -232,6 +232,9 @@ enum {
> #define CPER_MEM_VALID_RESPONDER_ID 0x1000
> #define CPER_MEM_VALID_TARGET_ID 0x2000
> #define CPER_MEM_VALID_ERROR_TYPE 0x4000
> +#define CPER_MEM_VALID_RANK_NUMBER 0x8000
> +#define CPER_MEM_VALID_CARD_HANDLE 0x10000
> +#define CPER_MEM_VALID_MODULE_HANDLE 0x20000
>
> #define CPER_PCIE_VALID_PORT_TYPE 0x0001
> #define CPER_PCIE_VALID_VERSION 0x0002
> @@ -347,6 +350,10 @@ struct cper_sec_mem_err {
> __u64 responder_id;
> __u64 target_id;
> __u8 error_type;
> + __u8 reserved;
> + __u16 rank;
> + __u16 mem_array_handle;
> + __u16 mem_dev_handle;

Nit: could you name those fields similar to what the spec has:
card_handle and module_handle, with perhaps a comment to indicate
relationship to SMBIOS type 16/17 tables?


Regards,
Naveen

> };
>
> struct cper_sec_pcie {
> --
> 1.8.4.rc3

Naveen N. Rao

unread,
Oct 15, 2013, 1:40:02 PM10/15/13
to
I thought the primary problem was the conflict with edac core itself.
So, if I'm not mistaken, we would have to prevent all edac drivers from
loading.

Thanks,
Naveen

Borislav Petkov

unread,
Oct 15, 2013, 1:50:02 PM10/15/13
to
On Tue, Oct 15, 2013 at 11:00:53PM +0530, Naveen N. Rao wrote:
> I thought the primary problem was the conflict with edac core itself.
> So, if I'm not mistaken, we would have to prevent all edac drivers
> from loading.

That too - I don't see the need for them if the firmware does the
decoding.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Naveen N. Rao

unread,
Oct 15, 2013, 2:20:02 PM10/15/13
to
On 2013/10/11 02:32AM, Chen Gong wrote:
> To satisfy the necessary of following patches and make related definition
> more clear, update some definitions about CPER. No functional changes.
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> ---
> drivers/acpi/apei/apei-internal.h | 12 ++++-----
> drivers/acpi/apei/cper.c | 46 ++++++++++++++++-----------------
> drivers/acpi/apei/ghes.c | 54 +++++++++++++++++++--------------------
> include/acpi/actbl1.h | 14 +++++-----
> include/acpi/ghes.h | 2 +-
> 5 files changed, 64 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
> index f220d64..21ba34a 100644
> --- a/drivers/acpi/apei/apei-internal.h
> +++ b/drivers/acpi/apei/apei-internal.h
> @@ -122,11 +122,11 @@ struct dentry;
> struct dentry *apei_get_debugfs_dir(void);
>
> #define apei_estatus_for_each_section(estatus, section) \
> - for (section = (struct acpi_hest_generic_data *)(estatus + 1); \
> + for (section = (struct acpi_generic_data *)(estatus + 1); \

This is a good one to rename, though I wonder if acpi_generic_error_data
is more appropriate?

> (void *)section - (void *)estatus < estatus->data_length; \
> section = (void *)(section+1) + section->error_data_length)
>
> -static inline u32 apei_estatus_len(struct acpi_hest_generic_status *estatus)
> +static inline u32 cper_estatus_len(struct acpi_generic_status *estatus)

Not sure I understand the rationale for these changes - we are still
dealing with ACPI/APEI generic error status/data structures. So, why
the cper_ prefix?

> {
> if (estatus->raw_data_length)
> return estatus->raw_data_offset + \
> @@ -135,10 +135,10 @@ static inline u32 apei_estatus_len(struct acpi_hest_generic_status *estatus)
> return sizeof(*estatus) + estatus->data_length;
> }
>
> -void apei_estatus_print(const char *pfx,
> - const struct acpi_hest_generic_status *estatus);
> -int apei_estatus_check_header(const struct acpi_hest_generic_status *estatus);
> -int apei_estatus_check(const struct acpi_hest_generic_status *estatus);
> +void cper_estatus_print(const char *pfx,
> + const struct acpi_generic_status *estatus);
> +int cper_estatus_check_header(const struct acpi_generic_status *estatus);
> +int cper_estatus_check(const struct acpi_generic_status *estatus);

Same here. All the above functions work on ACPI structures...

> /* Values for block_status flags above */
>
> -#define ACPI_HEST_UNCORRECTABLE (1)
> -#define ACPI_HEST_CORRECTABLE (1<<1)
> -#define ACPI_HEST_MULTIPLE_UNCORRECTABLE (1<<2)
> -#define ACPI_HEST_MULTIPLE_CORRECTABLE (1<<3)
> -#define ACPI_HEST_ERROR_ENTRY_COUNT (0xFF<<4) /* 8 bits, error count */
> +#define ACPI_GEN_ERR_UC (1)
> +#define ACPI_GEN_ERR_CE (1<<1)
> +#define ACPI_GEN_ERR_MULTI_UC (1<<2)
> +#define ACPI_GEN_ERR_MULTI_CE (1<<3)
> +#define ACPI_GEN_ERR_COUNT_SHIFT (0xFF<<4) /* 8 bits, error count */

I'd prefer ACPI_GENERIC_ERR_ since ACPI_GEN_ERR sounds far too much like
ACPI "Generated" :)


Thanks,
Naveen

Naveen N. Rao

unread,
Oct 15, 2013, 3:10:02 PM10/15/13
to
On 2013/10/11 02:32AM, Chen Gong wrote:
> This patch adds a new interface to decode memory device (type 17)
> to help error reporting on DIMMs.
>
> Original-author: Tony Luck <tony...@intel.com>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> ---
> arch/ia64/kernel/setup.c | 1 +
> arch/x86/kernel/setup.c | 1 +
> drivers/firmware/dmi_scan.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/dmi.h | 5 ++++
> 4 files changed, 67 insertions(+)

Acked-by: Naveen N. Rao <naveen...@linux.vnet.ibm.com>

Naveen N. Rao

unread,
Oct 15, 2013, 3:20:02 PM10/15/13
to
On 2013/10/15 09:15AM, Tony Luck wrote:
> On Tue, Oct 15, 2013 at 2:28 AM, Borislav Petkov <b...@alien8.de> wrote:
> > We can even add a hint for the user like:
> >
> > "Above errors have been corrected by the hardware and require no further action."
> >
> > Btw, this is valid for both dmesg and trace event output.
> >
> > Because from my experience so far people just scream: "Look, I just had
> > an MCE" withot even reading what it says. And this just upsets support
> > people for no valid reason at all.
>
> "Like", "+1", "metoo" ... or whatever it is we do on LKML to indicate
> we approve.

+2 ;)

While at it, I wonder if we're better off calling these "Hardware
events" rather than "Hardware errors".


Thanks,
Naveen

Naveen N. Rao

unread,
Oct 15, 2013, 3:20:02 PM10/15/13
to
On 2013/10/11 02:32AM, Chen Gong wrote:
> After H/W error happens under FFM enabled mode, lots of information
> are shown but some important parts like DIMM location missed. This
> patch is used to show these extra fileds.
>
> Original-author: Tony Luck <tony...@intel.com>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> ---
> drivers/acpi/apei/cper.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>

Acked-by: Naveen N. Rao <naveen...@linux.vnet.ibm.com>

> diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
> index 680230c..2a4389f 100644
> --- a/drivers/acpi/apei/cper.c
> +++ b/drivers/acpi/apei/cper.c
> @@ -28,6 +28,7 @@
> #include <linux/module.h>
> #include <linux/time.h>
> #include <linux/cper.h>
> +#include <linux/dmi.h>
> #include <linux/acpi.h>
> #include <linux/pci.h>
> #include <linux/aer.h>
> @@ -210,6 +211,8 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> printk("%s""card: %d\n", pfx, mem->card);
> if (mem->validation_bits & CPER_MEM_VALID_MODULE)
> printk("%s""module: %d\n", pfx, mem->module);
> + if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
> + printk("%s""rank: %d\n", pfx, mem->rank);
> if (mem->validation_bits & CPER_MEM_VALID_BANK)
> printk("%s""bank: %d\n", pfx, mem->bank);
> if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
> @@ -232,6 +235,15 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
> cper_mem_err_type_strs[etype] : "unknown");
> }
> + if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
> + const char *bank = NULL, *device = NULL;
> + dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
> + if (bank != NULL && device != NULL)
> + printk("%s""DIMM location: %s %s", pfx, bank, device);
> + else
> + printk("%s""DIMM DMI handle: 0x%.4x", pfx,
> + mem->mem_dev_handle);
> + }
> }
>
> static const char *cper_pcie_port_type_strs[] = {
> --
> 1.8.4.rc3

Borislav Petkov

unread,
Oct 15, 2013, 3:30:03 PM10/15/13
to
On Wed, Oct 16, 2013 at 12:40:40AM +0530, Naveen N. Rao wrote:
> +2 ;)

You're counting for 2 people, huh?

:-)

> While at it, I wonder if we're better off calling these "Hardware
> events" rather than "Hardware errors".

Oh, please no. That's that euphemistic lying which serves no one. And
here's what I mean by "euphemistic lying":

https://www.youtube.com/watch?v=vuEQixrBKCc

Let's call it what it really is but, at the same time, make sure it is
understandable to users what action they need to undertake (or none)
when they encounter it.

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Mauro Carvalho Chehab

unread,
Oct 15, 2013, 8:50:02 PM10/15/13
to
I see a few problems on this patchset:

Em Tue, 15 Oct 2013 23:00:53 +0530
"Naveen N. Rao" <naveen...@linux.vnet.ibm.com> escreveu:

> On 10/15/2013 10:30 PM, Borislav Petkov wrote:
> > On Tue, Oct 15, 2013 at 10:24:35PM +0530, Naveen N. Rao wrote:
> >> On 2013/10/11 02:32AM, Chen Gong wrote:
> >>> Use trace interface to elaborate all H/W error related
> >>> information.
> >>>
> >>> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> >>> ---
> >> <snip>
> >>> +TRACE_EVENT(extlog_mem_event,
> >>> + TP_PROTO(u32 etype,
> >>> + char *dimm_loc,
> >>> + const uuid_le *fru_id,

Using a custom typedef here seems problematic, as that can make userspace
interface more complicated.

> >>> + char *fru_text,
> >>> + u64 error_count,
> >>> + u32 severity,
> >>> + u64 phy_addr,
> >>> + char *mem_loc),

By looking on the rest of the changes, the mem_loc can now contain the
right location of the memory error, including on what DIMM the error
happened. It can also (optionally) contain the DIMM label.

Mangling those information on just one string field seems to be a very
bad idea to me, as it prevents to write a generic logic, on userspace,
that would apply a per-DIMM threshold policy.

Also, userspace needs to know what's the granularity for the error
that an eMCA driver will give, in order to adjust its policies.

> >>
> >> [Adding Mauro...]
> >>
> >> This looks very similar to the trace event I wrote a while back,
> >> which was similar to the one provided by ghes_edac:
> >> http://thread.gmane.org/gmane.linux.kernel.pci/24616
> >>
> >> Seems to me this has the same issues we previously discussed w.r.t
> >> EDAC conflicts...

Agreed.

> >
> > Right, I'm inclined to leave this trace_mc_event in ras_event.h to edac
> > use alone because of all those layers which don't mean whit for GHES and
> > eMCA error sources.

If you don't create the EDAC nodes, it means that userspace doesn't have any
glue about what error information will be provided.

The right thing to do is, IMHO, add some additional EDAC sysfs nodes that
shows what kind of error information will be provided by the device, e. g.:

- a complete hardware-based type of information directly obtained from
the hardware;

- a very poor BIOS-based type of error information, where the provided
data is not sufficient to pinpoint to the DIMM where the error actually
occurred (what's currently there at ghes_edac);

- an eMCA-based type of error information, where the BIOS and ACPI will
provide the complete error path, allowing userspace to properly parse
the errors as if they come from the hardware-based approach.

In any case, this is provided by the EDAC core functions that describe the
memory in details. So, IMHO, get rid of EDAC is a big mistake.

> > And maybe define a trace_mem_event which is shared by GHES and eMCA and
> > not use the edac tracepoint there not load ghes_edac on such systems
> > which have sufficient decoding capability in firmware.
> >
> > Thoughts?
>
> I thought the primary problem was the conflict with edac core itself.
> So, if I'm not mistaken, we would have to prevent all edac drivers from
> loading.

Yes, this is another aspect of this approach: whatever provided mechanism,
the Kernel should assure that one error path won't conflict with the other
ones. We know by experience that enabling both BIOS-based and hardware-based
mechanisms cause race conditions, with affects both ways.
It is also nice to allow the user to choose his preferred mechanism, when
more than one is properly supported on a given system.

Regards,
Mauro

(c/c Aristeu, as he might also being working with similar stuff)

Chen Gong

unread,
Oct 15, 2013, 10:00:01 PM10/15/13
to
On Tue, Oct 15, 2013 at 11:47:23PM +0530, Naveen N. Rao wrote:
> Date: Tue, 15 Oct 2013 23:47:23 +0530
> From: "Naveen N. Rao" <naveen...@linux.vnet.ibm.com>
> To: "Chen, Gong" <gong...@linux.intel.com>
> Cc: tony...@intel.com, b...@alien8.de, linux-...@vger.kernel.org,
> linux...@vger.kernel.org
> Subject: Re: [PATCH 2/8] ACPI, CPER: Update cper info
> User-Agent: Mutt/1.5.21 (2010-09-15)
Because CPER is not APEI specific, beside APEI, some others like eMCA
needs this.
signature.asc

Chen Gong

unread,
Oct 15, 2013, 10:00:03 PM10/15/13
to
On Tue, Oct 15, 2013 at 10:56:25PM +0530, Naveen N. Rao wrote:
> Date: Tue, 15 Oct 2013 22:56:25 +0530
> Subject: Re: [PATCH 5/8] ACPI, APEI, CPER: Add UEFI 2.4 support for memory
> error
> User-Agent: Mutt/1.5.21 (2010-09-15)
sure, of course.
On the contrary, what I'm thinking is reserve these names but
adding comments for what it is in the spec. I consider a
reasonable name is more meaningful than just following the
spec strictly.
signature.asc

Joe Perches

unread,
Oct 15, 2013, 10:00:03 PM10/15/13
to
On Fri, 2013-10-11 at 17:47 +0200, Borislav Petkov wrote:
> On Fri, Oct 11, 2013 at 11:06:30AM +0200, Borislav Petkov wrote:
> > > - printk("%s""APEI generic hardware error status\n", pfx);
> > > + printk("%s""Generic Hardware Error Status\n", pfx);
> >
> > Btw, what's the story with printk not using KERN_x levels in this file?
> > Why are we falling back to default printk levels for all printks here
> > and shouldn't we rather prioritize them by urgency into, say, KERN_ERR,
> > KERN_INFO, etc?
>
> Ignore that - checkpatch complained about it but I kinda missed that
> we're handing down the prefix.

I think it'd be better to rename pfx to level
as that's what printk.h calls them.

Chen Gong

unread,
Oct 15, 2013, 11:10:02 PM10/15/13
to
On Tue, Oct 15, 2013 at 06:57:18PM -0700, Joe Perches wrote:
> Date: Tue, 15 Oct 2013 18:57:18 -0700
> From: Joe Perches <j...@perches.com>
> To: Borislav Petkov <b...@alien8.de>
> Cc: "Chen, Gong" <gong...@linux.intel.com>, tony...@intel.com,
> linux-...@vger.kernel.org, linux...@vger.kernel.org
> Subject: Re: [PATCH 2/8] ACPI, CPER: Update cper info
> X-Mailer: Evolution 3.6.4-0ubuntu1
>
> On Fri, 2013-10-11 at 17:47 +0200, Borislav Petkov wrote:
> > On Fri, Oct 11, 2013 at 11:06:30AM +0200, Borislav Petkov wrote:
> > > > - printk("%s""APEI generic hardware error status\n", pfx);
> > > > + printk("%s""Generic Hardware Error Status\n", pfx);
> > >
> > > Btw, what's the story with printk not using KERN_x levels in this file?
> > > Why are we falling back to default printk levels for all printks here
> > > and shouldn't we rather prioritize them by urgency into, say, KERN_ERR,
> > > KERN_INFO, etc?
> >
> > Ignore that - checkpatch complained about it but I kinda missed that
> > we're handing down the prefix.
>
> I think it'd be better to rename pfx to level
> as that's what printk.h calls them.
>
No. pfx includes log level and prefix string both.
>
>
signature.asc

Joe Perches

unread,
Oct 15, 2013, 11:20:02 PM10/15/13
to
Perhaps it'd be better to separate them.

I haven't looked too hard, but is apei_status_print
the only place it's used with more than KERN_<LEVEL>?

Borislav Petkov

unread,
Oct 16, 2013, 5:20:02 AM10/16/13
to
On Tue, Oct 15, 2013 at 09:43:46PM -0300, Mauro Carvalho Chehab wrote:
> Using a custom typedef here seems problematic, as that can make userspace
> interface more complicated.

It is defined in a userspace header: include/uapi/linux/uuid.h

> > >>> + char *fru_text,
> > >>> + u64 error_count,
> > >>> + u32 severity,
> > >>> + u64 phy_addr,
> > >>> + char *mem_loc),
>
> By looking on the rest of the changes, the mem_loc can now contain the
> right location of the memory error, including on what DIMM the error
> happened. It can also (optionally) contain the DIMM label.

No, dimm_loc contains the label.

> Also, userspace needs to know what's the granularity for the error
> that an eMCA driver will give, in order to adjust its policies.

u32 error_count

> If you don't create the EDAC nodes, it means that userspace doesn't
> have any glue about what error information will be provided.

Of course it does - it is a tracepoint. There's no need for EDAC at all
with eMCA present on the system.

> In any case, this is provided by the EDAC core functions that describe
> the memory in details. So, IMHO, get rid of EDAC is a big mistake.

No one said we're getting rid of EDAC - we're basically disabling its
services on machines with eMCA because we simply don't need it.

> It is also nice to allow the user to choose his preferred mechanism,
> when more than one is properly supported on a given system.

We did this with firmware-first reporting so I don't see any need
for user interaction. When you have eMCA on the system, you disable
everything else reporting memory errors and go to sleep. So, similar to
firmware first.

If eMCA turns out to have f*cked BIOS implementations - which I don't
doubt - then we can add a chicken bit similar to "acpi=nocmcff"

It is that simple.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen Gong

unread,
Oct 16, 2013, 6:10:02 AM10/16/13
to
On Tue, Oct 15, 2013 at 10:24:35PM +0530, Naveen N. Rao wrote:
> Date: Tue, 15 Oct 2013 22:24:35 +0530
> linux...@vger.kernel.org, m.ch...@samsung.com
> Subject: Re: [PATCH 8/8] ACPI / trace: Add trace interface for eMCA driver
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On 2013/10/11 02:32AM, Chen Gong wrote:
> > Use trace interface to elaborate all H/W error related
> > information.
> >
> > Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> > ---
> <snip>
> > +TRACE_EVENT(extlog_mem_event,
> > + TP_PROTO(u32 etype,
> > + char *dimm_loc,
> > + const uuid_le *fru_id,
> > + char *fru_text,
> > + u64 error_count,
> > + u32 severity,
> > + u64 phy_addr,
> > + char *mem_loc),
>
> [Adding Mauro...]
>
> This looks very similar to the trace event I wrote a while back,
> which was similar to the one provided by ghes_edac:
> http://thread.gmane.org/gmane.linux.kernel.pci/24616
>
> Seems to me this has the same issues we previously discussed w.r.t
> EDAC conflicts...
>
This thread is so long. I have to say I'm lost ...
Anyway, it looks like there are many different opnions on this last
patch. I will send the 2nd version for further discussion soon.

signature.asc

Mauro Carvalho Chehab

unread,
Oct 16, 2013, 6:40:02 AM10/16/13
to
Em Wed, 16 Oct 2013 11:16:40 +0200
Borislav Petkov <b...@alien8.de> escreveu:

> On Tue, Oct 15, 2013 at 09:43:46PM -0300, Mauro Carvalho Chehab wrote:
> > > + const uuid_le *fru_id,
> > Using a custom typedef here seems problematic, as that can make userspace
> > interface more complicated.

> It is defined in a userspace header: include/uapi/linux/uuid.h

Hmm... a non-packed structure is defined there. Ok, it has 16 bytes.
I generally avoid using non-packed structs on uapi, as the compiler
alignment rules can cause troubles if kernelspace is compiled with 64
bits, and userspace with 32 bits. In this specific case, it looks ok.

>
> > > >>> + char *fru_text,
> > > >>> + u64 error_count,
> > > >>> + u32 severity,
> > > >>> + u64 phy_addr,
> > > >>> + char *mem_loc),
> >
> > By looking on the rest of the changes, the mem_loc can now contain the
> > right location of the memory error, including on what DIMM the error
> > happened. It can also (optionally) contain the DIMM label.
>
> No, dimm_loc contains the label.

Yeah, right. This is badly named, then.

Anyway, the dimm_loc is filled from DMI table by dmi_memdev_name().
Based on past experiences, this can be problematic, as manufactures tend
to re-use the same DMI table on machines with different layouts.

Tony/Chen,

Are there any changes with regards to that, like some enforcement policy
for BIOS manufacturers to make it right?

If not, then I think we still need EDAC's code to allow overriding those
labels by the ones actually found on the system.

> > Also, userspace needs to know what's the granularity for the error
> > that an eMCA driver will give, in order to adjust its policies.
>
> u32 error_count

I'm talking about granularity, not error count. I mean: how the memory
is organized, what happens with errors that could be affecting more than
one DIMM (for example, on mirrored memories), etc.

Userspace can only do something useful if it knows what kind of support
the hardware error mechanism is providing.

> > If you don't create the EDAC nodes, it means that userspace doesn't
> > have any glue about what error information will be provided.
>
> Of course it does - it is a tracepoint. There's no need for EDAC at all
> with eMCA present on the system.

Well, try to write some code on userspace to discover what's the error.

An error threshold mechanism on userspace will only work if userspace
knows that the error belongs to the same DIMM.

If several different DIMMs are showing errors at the very same channel, and
replacing those dimms don't happen, that likely means that the channel BUS
is damaged.

What I'm saying is that hiding those information from userspace doesn't
help at all to improve the quality of the error report mechanism.

I can't see any reason why hiding those information from userspace.

The tracepoint interface is not for that. Such data is provided via sysfs,
and should be used for the application to properly tune their algorithms.

> > In any case, this is provided by the EDAC core functions that describe
> > the memory in details. So, IMHO, get rid of EDAC is a big mistake.
>
> No one said we're getting rid of EDAC - we're basically disabling its
> services on machines with eMCA because we simply don't need it.

We do need, if we want do do a good job decoding the errors.

> > It is also nice to allow the user to choose his preferred mechanism,
> > when more than one is properly supported on a given system.
>
> We did this with firmware-first reporting so I don't see any need
> for user interaction. When you have eMCA on the system, you disable
> everything else reporting memory errors and go to sleep. So, similar to
> firmware first.
>
> If eMCA turns out to have f*cked BIOS implementations - which I don't
> doubt - then we can add a chicken bit similar to "acpi=nocmcff"
>
> It is that simple.
>

Works for me.

Regards,
Mauro

Borislav Petkov

unread,
Oct 16, 2013, 6:50:01 AM10/16/13
to
Btw, I don't know what's the problem but when I hit reply-to-all to your
emails, mutt drops your email address from the To: and makes the CC:
list become the To: list. Strange.

On Wed, Oct 16, 2013 at 05:50:30AM -0400, Chen Gong wrote:
> This thread is so long. I have to say I'm lost ... Anyway, it looks
> like there are many different opnions on this last patch.

Why, I think it is fine.

The only compatibility issue I see is if we're going to share the
tracepoint with Naveen's GHES reporting stuff we were discussing a
couple of weeks ago. But AFAIR, we still needed EDAC assistance there
while here there's no need for that.

> I will send the 2nd version for further discussion soon.

Yes please. :)

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 16, 2013, 6:50:02 AM10/16/13
to
On Wed, Oct 16, 2013 at 07:35:39AM -0300, Mauro Carvalho Chehab wrote:
> Well, try to write some code on userspace to discover what's the error.
>
> An error threshold mechanism on userspace will only work if userspace
> knows that the error belongs to the same DIMM.

Just read the first mail again:

<idle>-0 [000] d.h. 56068.488759: extlog_mem_event: 3 corrected errors:unknown on Memriser1 CHANNEL A DIMM 0(FRU: 00000000-0000
-0000-0000-000000000000 physical addr: 0x0000000851fe0000 node: 0 card: 0 module: 0 rank: 0 bank: 0 row: 28927 column: 1296)

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Mauro Carvalho Chehab

unread,
Oct 16, 2013, 8:00:02 AM10/16/13
to
Em Wed, 16 Oct 2013 12:42:21 +0200
Borislav Petkov <b...@alien8.de> escreveu:

> On Wed, Oct 16, 2013 at 07:35:39AM -0300, Mauro Carvalho Chehab wrote:
> > Well, try to write some code on userspace to discover what's the error.
> >
> > An error threshold mechanism on userspace will only work if userspace
> > knows that the error belongs to the same DIMM.
>
> Just read the first mail again:
>
> <idle>-0 [000] d.h. 56068.488759: extlog_mem_event: 3 corrected errors:unknown on Memriser1 CHANNEL A DIMM 0(FRU: 00000000-0000
> -0000-0000-000000000000 physical addr: 0x0000000851fe0000 node: 0 card: 0 module: 0 rank: 0 bank: 0 row: 28927 column: 1296)

On that log, "physical addr: 0x0000000851fe0000 node: 0 card: 0 module: 0 rank: 0 bank: 0 row: 28927 column: 1296"
is a string, instead of an hierarchical position, like what it is provided
on EDAC.

Worse than that, not all data may be available, as CPER allows to
ommit some data.

Also, I suspect that, if an error happens to affect more than one DIMM
(e. g. part of the location is not available for a given error),
that the DIMM label will also not be properly shown.

Also, writing the userspace counterpart that would work properly is
extremely hard, if the information about the memory layout is not known
in advance. So, in practice, if the above memory error is provided, all
userspace will likely be able to do is to store it and require someone
to manually identify what's happening.

On the other hand, if node, channel and dimm number information is
properly filled (like it happens on EDAC), usersapce can rely on those
data, in order to apply per dimm, per channel and per node thresholds.

It may even use the physical address to identify if the problem is only on
a certain region of a physical DIMM and poison that region, while it is
not possible to replace the damaged component.

Regards,
Mauro

Borislav Petkov

unread,
Oct 16, 2013, 8:30:01 AM10/16/13
to
On Wed, Oct 16, 2013 at 08:55:58AM -0300, Mauro Carvalho Chehab wrote:
> On that log, "physical addr: 0x0000000851fe0000 node: 0 card: 0 module: 0 rank: 0 bank: 0 row: 28927 column: 1296"
> is a string, instead of an hierarchical position, like what it is provided
> on EDAC.

So you can't split strings in userspace?

> Also, I suspect that, if an error happens to affect more than one DIMM
> (e. g. part of the location is not available for a given error), that
> the DIMM label will also not be properly shown.

That's unrelated to these patches and needs to be reported properly by
the firmware.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen, Gong

unread,
Oct 16, 2013, 11:20:02 AM10/16/13
to
This patch adds a new interface to decode memory device (type 17)
to help error reporting on DIMMs.

Original-author: Tony Luck <tony...@intel.com>
Signed-off-by: Chen, Gong <gong...@linux.intel.com>
Acked-by: Naveen N. Rao <naveen...@linux.vnet.ibm.com>
---
arch/ia64/kernel/setup.c | 1 +
arch/x86/kernel/setup.c | 1 +
drivers/firmware/dmi_scan.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/dmi.h | 5 ++++
4 files changed, 67 insertions(+)

diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
index 4fc2e95..d86669b 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -1063,6 +1063,7 @@ check_bugs (void)
static int __init run_dmi_scan(void)
{
dmi_scan_machine();
+ dmi_memdev_walk();
dmi_set_dump_stack_arch_desc();
return 0;
}
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f0de629..918d489 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -993,6 +993,7 @@ void __init setup_arch(char **cmdline_p)
efi_init();

dmi_scan_machine();
+ dmi_memdev_walk();
dmi_set_dump_stack_arch_desc();

/*
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index fa0affb..59579a7 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -25,6 +25,13 @@ static int dmi_initialized;
/* DMI system identification string used during boot */
static char dmi_ids_string[128] __initdata;

+static struct dmi_memdev_info {
+ const char *device;
+ const char *bank;
+ u16 handle;
+} *dmi_memdev;
+static int dmi_memdev_nr;
+
static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
{
const u8 *bp = ((u8 *) dm) + dm->length;
@@ -322,6 +329,42 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
}

+static void __init count_mem_devices(const struct dmi_header *dm, void *v)
+{
+ if (dm->type != DMI_ENTRY_MEM_DEVICE)
+ return;
+ dmi_memdev_nr++;
+}
+
+static void __init save_mem_devices(const struct dmi_header *dm, void *v)
+{
+ const char *d = (const char *)dm;
+ static int nr;
+
+ if (dm->type != DMI_ENTRY_MEM_DEVICE)
+ return;
+ if (nr >= dmi_memdev_nr) {
+ pr_warn(FW_BUG "Too many DIMM entries in SMBIOS table\n");
+ return;
+ }
+ dmi_memdev[nr].handle = dm->handle;
+ dmi_memdev[nr].device = dmi_string(dm, d[0x10]);
+ dmi_memdev[nr].bank = dmi_string(dm, d[0x11]);
+ nr++;
+}
+
+void __init dmi_memdev_walk(void)
+{
+ if (!dmi_available)
+ return;
+
+ if (dmi_walk_early(count_mem_devices) == 0 && dmi_memdev_nr) {
+ dmi_memdev = dmi_alloc(sizeof(*dmi_memdev) * dmi_memdev_nr);
+ if (dmi_memdev)
+ dmi_walk_early(save_mem_devices);
+ }
+}
+
/*
* Process a DMI table entry. Right now all we care about are the BIOS
* and machine entries. For 2.5 we should pull the smbus controller info
@@ -815,3 +858,20 @@ bool dmi_match(enum dmi_field f, const char *str)
return !strcmp(info, str);
}
EXPORT_SYMBOL_GPL(dmi_match);
+
+void dmi_memdev_name(u16 handle, const char **bank, const char **device)
+{
+ int n;
+
+ if (dmi_memdev == NULL)
+ return;
+
+ for (n = 0; n < dmi_memdev_nr; n++) {
+ if (handle == dmi_memdev[n].handle) {
+ *bank = dmi_memdev[n].bank;
+ *device = dmi_memdev[n].device;
+ break;
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(dmi_memdev_name);
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index b6eb7a0..f820f0a 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -99,6 +99,7 @@ extern const char * dmi_get_system_info(int field);
extern const struct dmi_device * dmi_find_device(int type, const char *name,
const struct dmi_device *from);
extern void dmi_scan_machine(void);
+extern void dmi_memdev_walk(void);
extern void dmi_set_dump_stack_arch_desc(void);
extern bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp);
extern int dmi_name_in_vendors(const char *str);
@@ -107,6 +108,7 @@ extern int dmi_available;
extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
void *private_data);
extern bool dmi_match(enum dmi_field f, const char *str);
+extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);

#else

@@ -115,6 +117,7 @@ static inline const char * dmi_get_system_info(int field) { return NULL; }
static inline const struct dmi_device * dmi_find_device(int type, const char *name,
const struct dmi_device *from) { return NULL; }
static inline void dmi_scan_machine(void) { return; }
+static inline void dmi_memdev_walk(void) { }
static inline void dmi_set_dump_stack_arch_desc(void) { }
static inline bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp)
{
@@ -133,6 +136,8 @@ static inline int dmi_walk(void (*decode)(const struct dmi_header *, void *),
void *private_data) { return -1; }
static inline bool dmi_match(enum dmi_field f, const char *str)
{ return false; }
+static inline void dmi_memdev_name(u16 handle, const char **bank,
+ const char **device) { }
static inline const struct dmi_system_id *
dmi_first_match(const struct dmi_system_id *list) { return NULL; }

--
1.8.4.rc3

Chen, Gong

unread,
Oct 16, 2013, 11:20:02 AM10/16/13
to
GENMASK is used to create a contiguous bitmask([hi:lo]). It is
implemented twice in current kernel. One is in EDAC driver, the other
is in SiS/XGI FB driver. Move it to a more generic place for other
usage.

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
Cc: Borislav Petkov <b...@alien8.de>
Cc: Thomas Winischhofer <tho...@winischhofer.net>
Cc: Jean-Christophe Plagniol-Villard <plag...@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.va...@ti.com>
---
drivers/edac/amd64_edac.c | 46 ++++++++++++++++++++++++----------------------
drivers/edac/amd64_edac.h | 8 --------
drivers/video/sis/init.c | 5 ++---
include/linux/bitops.h | 8 ++++++++
4 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 3c9e4e9..b53d0de 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -339,8 +339,8 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
if (pvt->fam == 0xf && pvt->ext_model < K8_REV_F) {
csbase = pvt->csels[dct].csbases[csrow];
csmask = pvt->csels[dct].csmasks[csrow];
- base_bits = GENMASK(21, 31) | GENMASK(9, 15);
- mask_bits = GENMASK(21, 29) | GENMASK(9, 15);
+ base_bits = GENMASK_ULL(31, 21) | GENMASK_ULL(15, 9);
+ mask_bits = GENMASK_ULL(29, 21) | GENMASK_ULL(15, 9);
addr_shift = 4;

/*
@@ -352,16 +352,16 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
csbase = pvt->csels[dct].csbases[csrow];
csmask = pvt->csels[dct].csmasks[csrow >> 1];

- *base = (csbase & GENMASK(5, 15)) << 6;
- *base |= (csbase & GENMASK(19, 30)) << 8;
+ *base = (csbase & GENMASK_ULL(15, 5)) << 6;
+ *base |= (csbase & GENMASK_ULL(30, 19)) << 8;

*mask = ~0ULL;
/* poke holes for the csmask */
- *mask &= ~((GENMASK(5, 15) << 6) |
- (GENMASK(19, 30) << 8));
+ *mask &= ~((GENMASK_ULL(15, 5) << 6) |
+ (GENMASK_ULL(30, 19) << 8));

- *mask |= (csmask & GENMASK(5, 15)) << 6;
- *mask |= (csmask & GENMASK(19, 30)) << 8;
+ *mask |= (csmask & GENMASK_ULL(15, 5)) << 6;
+ *mask |= (csmask & GENMASK_ULL(30, 19)) << 8;

return;
} else {
@@ -370,9 +370,11 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
addr_shift = 8;

if (pvt->fam == 0x15)
- base_bits = mask_bits = GENMASK(19,30) | GENMASK(5,13);
+ base_bits = mask_bits =
+ GENMASK_ULL(30,19) | GENMASK_ULL(13,5);
else
- base_bits = mask_bits = GENMASK(19,28) | GENMASK(5,13);
+ base_bits = mask_bits =
+ GENMASK_ULL(28,19) | GENMASK_ULL(13,5);
}

*base = (csbase & base_bits) << addr_shift;
@@ -561,7 +563,7 @@ static u64 sys_addr_to_dram_addr(struct mem_ctl_info *mci, u64 sys_addr)
* section 3.4.2 of AMD publication 24592: AMD x86-64 Architecture
* Programmer's Manual Volume 1 Application Programming.
*/
- dram_addr = (sys_addr & GENMASK(0, 39)) - dram_base;
+ dram_addr = (sys_addr & GENMASK_ULL(39, 0)) - dram_base;

edac_dbg(2, "using DRAM Base register to translate SysAddr 0x%lx to DramAddr 0x%lx\n",
(unsigned long)sys_addr, (unsigned long)dram_addr);
@@ -597,7 +599,7 @@ static u64 dram_addr_to_input_addr(struct mem_ctl_info *mci, u64 dram_addr)
* concerning translating a DramAddr to an InputAddr.
*/
intlv_shift = num_node_interleave_bits(dram_intlv_en(pvt, 0));
- input_addr = ((dram_addr >> intlv_shift) & GENMASK(12, 35)) +
+ input_addr = ((dram_addr >> intlv_shift) & GENMASK_ULL(35, 12)) +
(dram_addr & 0xfff);

edac_dbg(2, " Intlv Shift=%d DramAddr=0x%lx maps to InputAddr=0x%lx\n",
@@ -849,7 +851,7 @@ static u64 get_error_address(struct amd64_pvt *pvt, struct mce *m)
end_bit = 39;
}

- addr = m->addr & GENMASK(start_bit, end_bit);
+ addr = m->addr & GENMASK_ULL(end_bit, start_bit);

/*
* Erratum 637 workaround
@@ -861,7 +863,7 @@ static u64 get_error_address(struct amd64_pvt *pvt, struct mce *m)
u16 mce_nid;
u8 intlv_en;

- if ((addr & GENMASK(24, 47)) >> 24 != 0x00fdf7)
+ if ((addr & GENMASK_ULL(47, 24)) >> 24 != 0x00fdf7)
return addr;

mce_nid = amd_get_nb_id(m->extcpu);
@@ -871,7 +873,7 @@ static u64 get_error_address(struct amd64_pvt *pvt, struct mce *m)
intlv_en = tmp >> 21 & 0x7;

/* add [47:27] + 3 trailing bits */
- cc6_base = (tmp & GENMASK(0, 20)) << 3;
+ cc6_base = (tmp & GENMASK_ULL(20, 0)) << 3;

/* reverse and add DramIntlvEn */
cc6_base |= intlv_en ^ 0x7;
@@ -880,18 +882,18 @@ static u64 get_error_address(struct amd64_pvt *pvt, struct mce *m)
cc6_base <<= 24;

if (!intlv_en)
- return cc6_base | (addr & GENMASK(0, 23));
+ return cc6_base | (addr & GENMASK_ULL(23, 0));

amd64_read_pci_cfg(pvt->F1, DRAM_LOCAL_NODE_BASE, &tmp);

/* faster log2 */
- tmp_addr = (addr & GENMASK(12, 23)) << __fls(intlv_en + 1);
+ tmp_addr = (addr & GENMASK_ULL(23, 12)) << __fls(intlv_en + 1);

/* OR DramIntlvSel into bits [14:12] */
- tmp_addr |= (tmp & GENMASK(21, 23)) >> 9;
+ tmp_addr |= (tmp & GENMASK_ULL(23, 21)) >> 9;

/* add remaining [11:0] bits from original MC4_ADDR */
- tmp_addr |= addr & GENMASK(0, 11);
+ tmp_addr |= addr & GENMASK_ULL(11, 0);

return cc6_base | tmp_addr;
}
@@ -952,12 +954,12 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)

amd64_read_pci_cfg(f1, DRAM_LOCAL_NODE_LIM, &llim);

- pvt->ranges[range].lim.lo &= GENMASK(0, 15);
+ pvt->ranges[range].lim.lo &= GENMASK_ULL(15, 0);

/* {[39:27],111b} */
pvt->ranges[range].lim.lo |= ((llim & 0x1fff) << 3 | 0x7) << 16;

- pvt->ranges[range].lim.hi &= GENMASK(0, 7);
+ pvt->ranges[range].lim.hi &= GENMASK_ULL(7, 0);

/* [47:40] */
pvt->ranges[range].lim.hi |= llim >> 13;
@@ -1330,7 +1332,7 @@ static u64 f1x_get_norm_dct_addr(struct amd64_pvt *pvt, u8 range,
chan_off = dram_base;
}

- return (sys_addr & GENMASK(6,47)) - (chan_off & GENMASK(23,47));
+ return (sys_addr & GENMASK_ULL(47,6)) - (chan_off & GENMASK_ULL(47,23));
}

/*
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index d2443cf..6dc1fcc 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -160,14 +160,6 @@
#define OFF false

/*
- * Create a contiguous bitmask starting at bit position @lo and ending at
- * position @hi. For example
- *
- * GENMASK(21, 39) gives us the 64bit vector 0x000000ffffe00000.
- */
-#define GENMASK(lo, hi) (((1ULL << ((hi) - (lo) + 1)) - 1) << (lo))
-
-/*
* PCI-defined configuration space registers
*/
#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F1 0x141b
diff --git a/drivers/video/sis/init.c b/drivers/video/sis/init.c
index f082ae5..4f26bc2 100644
--- a/drivers/video/sis/init.c
+++ b/drivers/video/sis/init.c
@@ -3320,9 +3320,8 @@ SiSSetMode(struct SiS_Private *SiS_Pr, unsigned short ModeNo)
}

#ifndef GETBITSTR
-#define BITMASK(h,l) (((unsigned)(1U << ((h)-(l)+1))-1)<<(l))
-#define GENMASK(mask) BITMASK(1?mask,0?mask)
-#define GETBITS(var,mask) (((var) & GENMASK(mask)) >> (0?mask))
+#define GENBITSMASK(mask) GENMASK(1?mask,0?mask)
+#define GETBITS(var,mask) (((var) & GENBITSMASK(mask)) >> (0?mask))
#define GETBITSTR(val,from,to) ((GETBITS(val,from)) << (0?to))
#endif

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index a3b6b82..bd0c459 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -10,6 +10,14 @@
#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
#endif

+/*
+ * Create a contiguous bitmask starting at bit position @l and ending at
+ * position @h. For example
+ * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
+ */
+#define GENMASK(h, l) (((U32_C(1) << ((h) - (l) + 1)) - 1) << (l))
+#define GENMASK_ULL(h, l) (((U64_C(1) << ((h) - (l) + 1)) - 1) << (l))
+
extern unsigned int __sw_hweight8(unsigned int w);
extern unsigned int __sw_hweight16(unsigned int w);
extern unsigned int __sw_hweight32(unsigned int w);

Chen, Gong

unread,
Oct 16, 2013, 11:20:02 AM10/16/13
to
To prepare for the following patches and make related
definition more clear, update some definitions about CPER.

v2 -> v1: Update some more definitions suggested by Boris

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/apei/apei-internal.h | 12 ++++----
drivers/acpi/apei/cper.c | 58 +++++++++++++++++++--------------------
drivers/acpi/apei/ghes.c | 54 ++++++++++++++++++------------------
include/acpi/actbl1.h | 14 +++++-----
include/acpi/ghes.h | 2 +-
include/linux/cper.h | 2 +-
6 files changed, 71 insertions(+), 71 deletions(-)

diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
index f220d64..21ba34a 100644
--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -122,11 +122,11 @@ struct dentry;
struct dentry *apei_get_debugfs_dir(void);

#define apei_estatus_for_each_section(estatus, section) \
- for (section = (struct acpi_hest_generic_data *)(estatus + 1); \
+ for (section = (struct acpi_generic_data *)(estatus + 1); \
(void *)section - (void *)estatus < estatus->data_length; \
section = (void *)(section+1) + section->error_data_length)

-static inline u32 apei_estatus_len(struct acpi_hest_generic_status *estatus)
+static inline u32 cper_estatus_len(struct acpi_generic_status *estatus)
{
if (estatus->raw_data_length)
return estatus->raw_data_offset + \
@@ -135,10 +135,10 @@ static inline u32 apei_estatus_len(struct acpi_hest_generic_status *estatus)
return sizeof(*estatus) + estatus->data_length;
}

-void apei_estatus_print(const char *pfx,
- const struct acpi_hest_generic_status *estatus);
-int apei_estatus_check_header(const struct acpi_hest_generic_status *estatus);
-int apei_estatus_check(const struct acpi_hest_generic_status *estatus);
+void cper_estatus_print(const char *pfx,
+ const struct acpi_generic_status *estatus);
+int cper_estatus_check_header(const struct acpi_generic_status *estatus);
+int cper_estatus_check(const struct acpi_generic_status *estatus);

int apei_osc_setup(void);
#endif
diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index f827f02..eb5f6d6 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -5,7 +5,7 @@
* Author: Huang Ying <ying....@intel.com>
*
* CPER is the format used to describe platform hardware error by
- * various APEI tables, such as ERST, BERT and HEST etc.
+ * various tables, such as ERST, BERT and HEST etc.
*
* For more information about CPER, please refer to Appendix N of UEFI
* Specification version 2.3.
@@ -73,7 +73,7 @@ static const char *cper_severity_str(unsigned int severity)
* printed, with @pfx is printed at the beginning of each line.
*/
void cper_print_bits(const char *pfx, unsigned int bits,
- const char *strs[], unsigned int strs_size)
+ const char * const strs[], unsigned int strs_size)
{
int i, len = 0;
const char *str;
@@ -98,32 +98,32 @@ void cper_print_bits(const char *pfx, unsigned int bits,
printk("%s\n", buf);
}

-static const char *cper_proc_type_strs[] = {
+static const char * const cper_proc_type_strs[] = {
"IA32/X64",
"IA64",
};

-static const char *cper_proc_isa_strs[] = {
+static const char * const cper_proc_isa_strs[] = {
"IA32",
"IA64",
"X64",
};

-static const char *cper_proc_error_type_strs[] = {
+static const char * const cper_proc_error_type_strs[] = {
"cache error",
"TLB error",
"bus error",
"micro-architectural error",
};

-static const char *cper_proc_op_strs[] = {
+static const char * const cper_proc_op_strs[] = {
"unknown or generic",
"data read",
"data write",
"instruction execution",
};

-static const char *cper_proc_flag_strs[] = {
+static const char * const cper_proc_flag_strs[] = {
"restartable",
"precise IP",
"overflow",
@@ -248,7 +248,7 @@ static const char *cper_pcie_port_type_strs[] = {
};

static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
- const struct acpi_hest_generic_data *gdata)
+ const struct acpi_generic_data *gdata)
{
if (pcie->validation_bits & CPER_PCIE_VALID_PORT_TYPE)
printk("%s""port_type: %d, %s\n", pfx, pcie->port_type,
@@ -283,17 +283,17 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
pfx, pcie->bridge.secondary_status, pcie->bridge.control);
}

-static const char *apei_estatus_section_flag_strs[] = {
+static const char * const cper_estatus_section_flag_strs[] = {
"primary",
"containment warning",
"reset",
- "threshold exceeded",
+ "error threshold exceeded",
"resource not accessible",
"latent error",
};

-static void apei_estatus_print_section(
- const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
+static void cper_estatus_print_section(
+ const char *pfx, const struct acpi_generic_data *gdata, int sec_no)
{
uuid_le *sec_type = (uuid_le *)gdata->section_type;
__u16 severity;
@@ -302,8 +302,8 @@ static void apei_estatus_print_section(
printk("%s""section: %d, severity: %d, %s\n", pfx, sec_no, severity,
cper_severity_str(severity));
printk("%s""flags: 0x%02x\n", pfx, gdata->flags);
- cper_print_bits(pfx, gdata->flags, apei_estatus_section_flag_strs,
- ARRAY_SIZE(apei_estatus_section_flag_strs));
+ cper_print_bits(pfx, gdata->flags, cper_estatus_section_flag_strs,
+ ARRAY_SIZE(cper_estatus_section_flag_strs));
if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
printk("%s""fru_id: %pUl\n", pfx, (uuid_le *)gdata->fru_id);
if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
@@ -339,34 +339,34 @@ err_section_too_small:
pr_err(FW_WARN "error section length is too small\n");
}

-void apei_estatus_print(const char *pfx,
- const struct acpi_hest_generic_status *estatus)
+void cper_estatus_print(const char *pfx,
+ const struct acpi_generic_status *estatus)
{
- struct acpi_hest_generic_data *gdata;
+ struct acpi_generic_data *gdata;
unsigned int data_len, gedata_len;
int sec_no = 0;
__u16 severity;

- printk("%s""APEI generic hardware error status\n", pfx);
+ printk("%s""Generic Hardware Error Status\n", pfx);
severity = estatus->error_severity;
printk("%s""severity: %d, %s\n", pfx, severity,
cper_severity_str(severity));
data_len = estatus->data_length;
- gdata = (struct acpi_hest_generic_data *)(estatus + 1);
+ gdata = (struct acpi_generic_data *)(estatus + 1);
while (data_len >= sizeof(*gdata)) {
gedata_len = gdata->error_data_length;
- apei_estatus_print_section(pfx, gdata, sec_no);
+ cper_estatus_print_section(pfx, gdata, sec_no);
data_len -= gedata_len + sizeof(*gdata);
gdata = (void *)(gdata + 1) + gedata_len;
sec_no++;
}
}
-EXPORT_SYMBOL_GPL(apei_estatus_print);
+EXPORT_SYMBOL_GPL(cper_estatus_print);

-int apei_estatus_check_header(const struct acpi_hest_generic_status *estatus)
+int cper_estatus_check_header(const struct acpi_generic_status *estatus)
{
if (estatus->data_length &&
- estatus->data_length < sizeof(struct acpi_hest_generic_data))
+ estatus->data_length < sizeof(struct acpi_generic_data))
return -EINVAL;
if (estatus->raw_data_length &&
estatus->raw_data_offset < sizeof(*estatus) + estatus->data_length)
@@ -374,19 +374,19 @@ int apei_estatus_check_header(const struct acpi_hest_generic_status *estatus)

return 0;
}
-EXPORT_SYMBOL_GPL(apei_estatus_check_header);
+EXPORT_SYMBOL_GPL(cper_estatus_check_header);

-int apei_estatus_check(const struct acpi_hest_generic_status *estatus)
+int cper_estatus_check(const struct acpi_generic_status *estatus)
{
- struct acpi_hest_generic_data *gdata;
+ struct acpi_generic_data *gdata;
unsigned int data_len, gedata_len;
int rc;

- rc = apei_estatus_check_header(estatus);
+ rc = cper_estatus_check_header(estatus);
if (rc)
return rc;
data_len = estatus->data_length;
- gdata = (struct acpi_hest_generic_data *)(estatus + 1);
+ gdata = (struct acpi_generic_data *)(estatus + 1);
while (data_len >= sizeof(*gdata)) {
gedata_len = gdata->error_data_length;
if (gedata_len > data_len - sizeof(*gdata))
@@ -399,4 +399,4 @@ int apei_estatus_check(const struct acpi_hest_generic_status *estatus)

return 0;
}
-EXPORT_SYMBOL_GPL(apei_estatus_check);
+EXPORT_SYMBOL_GPL(cper_estatus_check);
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 8ec37bb..0db6e4f 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -75,13 +75,13 @@
#define GHES_ESTATUS_CACHE_LEN(estatus_len) \
(sizeof(struct ghes_estatus_cache) + (estatus_len))
#define GHES_ESTATUS_FROM_CACHE(estatus_cache) \
- ((struct acpi_hest_generic_status *) \
+ ((struct acpi_generic_status *) \
((struct ghes_estatus_cache *)(estatus_cache) + 1))

#define GHES_ESTATUS_NODE_LEN(estatus_len) \
(sizeof(struct ghes_estatus_node) + (estatus_len))
-#define GHES_ESTATUS_FROM_NODE(estatus_node) \
- ((struct acpi_hest_generic_status *) \
+#define GHES_ESTATUS_FROM_NODE(estatus_node) \
+ ((struct acpi_generic_status *) \
((struct ghes_estatus_node *)(estatus_node) + 1))

bool ghes_disable;
@@ -378,17 +378,17 @@ static int ghes_read_estatus(struct ghes *ghes, int silent)
ghes->flags |= GHES_TO_CLEAR;

rc = -EIO;
- len = apei_estatus_len(ghes->estatus);
+ len = cper_estatus_len(ghes->estatus);
if (len < sizeof(*ghes->estatus))
goto err_read_block;
if (len > ghes->generic->error_block_length)
goto err_read_block;
- if (apei_estatus_check_header(ghes->estatus))
+ if (cper_estatus_check_header(ghes->estatus))
goto err_read_block;
ghes_copy_tofrom_phys(ghes->estatus + 1,
buf_paddr + sizeof(*ghes->estatus),
len - sizeof(*ghes->estatus), 1);
- if (apei_estatus_check(ghes->estatus))
+ if (cper_estatus_check(ghes->estatus))
goto err_read_block;
rc = 0;

@@ -409,7 +409,7 @@ static void ghes_clear_estatus(struct ghes *ghes)
ghes->flags &= ~GHES_TO_CLEAR;
}

-static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int sev)
+static void ghes_handle_memory_failure(struct acpi_generic_data *gdata, int sev)
{
#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
unsigned long pfn;
@@ -438,10 +438,10 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
}

static void ghes_do_proc(struct ghes *ghes,
- const struct acpi_hest_generic_status *estatus)
+ const struct acpi_generic_status *estatus)
{
int sev, sec_sev;
- struct acpi_hest_generic_data *gdata;
+ struct acpi_generic_data *gdata;

sev = ghes_severity(estatus->error_severity);
apei_estatus_for_each_section(estatus, gdata) {
@@ -496,7 +496,7 @@ static void ghes_do_proc(struct ghes *ghes,

static void __ghes_print_estatus(const char *pfx,
const struct acpi_hest_generic *generic,
- const struct acpi_hest_generic_status *estatus)
+ const struct acpi_generic_status *estatus)
{
static atomic_t seqno;
unsigned int curr_seqno;
@@ -513,12 +513,12 @@ static void __ghes_print_estatus(const char *pfx,
snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}" HW_ERR, pfx, curr_seqno);
printk("%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
pfx_seq, generic->header.source_id);
- apei_estatus_print(pfx_seq, estatus);
+ cper_estatus_print(pfx_seq, estatus);
}

static int ghes_print_estatus(const char *pfx,
const struct acpi_hest_generic *generic,
- const struct acpi_hest_generic_status *estatus)
+ const struct acpi_generic_status *estatus)
{
/* Not more than 2 messages every 5 seconds */
static DEFINE_RATELIMIT_STATE(ratelimit_corrected, 5*HZ, 2);
@@ -540,15 +540,15 @@ static int ghes_print_estatus(const char *pfx,
* GHES error status reporting throttle, to report more kinds of
* errors, instead of just most frequently occurred errors.
*/
-static int ghes_estatus_cached(struct acpi_hest_generic_status *estatus)
+static int ghes_estatus_cached(struct acpi_generic_status *estatus)
{
u32 len;
int i, cached = 0;
unsigned long long now;
struct ghes_estatus_cache *cache;
- struct acpi_hest_generic_status *cache_estatus;
+ struct acpi_generic_status *cache_estatus;

- len = apei_estatus_len(estatus);
+ len = cper_estatus_len(estatus);
rcu_read_lock();
for (i = 0; i < GHES_ESTATUS_CACHES_SIZE; i++) {
cache = rcu_dereference(ghes_estatus_caches[i]);
@@ -571,19 +571,19 @@ static int ghes_estatus_cached(struct acpi_hest_generic_status *estatus)

static struct ghes_estatus_cache *ghes_estatus_cache_alloc(
struct acpi_hest_generic *generic,
- struct acpi_hest_generic_status *estatus)
+ struct acpi_generic_status *estatus)
{
int alloced;
u32 len, cache_len;
struct ghes_estatus_cache *cache;
- struct acpi_hest_generic_status *cache_estatus;
+ struct acpi_generic_status *cache_estatus;

alloced = atomic_add_return(1, &ghes_estatus_cache_alloced);
if (alloced > GHES_ESTATUS_CACHE_ALLOCED_MAX) {
atomic_dec(&ghes_estatus_cache_alloced);
return NULL;
}
- len = apei_estatus_len(estatus);
+ len = cper_estatus_len(estatus);
cache_len = GHES_ESTATUS_CACHE_LEN(len);
cache = (void *)gen_pool_alloc(ghes_estatus_pool, cache_len);
if (!cache) {
@@ -603,7 +603,7 @@ static void ghes_estatus_cache_free(struct ghes_estatus_cache *cache)
{
u32 len;

- len = apei_estatus_len(GHES_ESTATUS_FROM_CACHE(cache));
+ len = cper_estatus_len(GHES_ESTATUS_FROM_CACHE(cache));
len = GHES_ESTATUS_CACHE_LEN(len);
gen_pool_free(ghes_estatus_pool, (unsigned long)cache, len);
atomic_dec(&ghes_estatus_cache_alloced);
@@ -619,7 +619,7 @@ static void ghes_estatus_cache_rcu_free(struct rcu_head *head)

static void ghes_estatus_cache_add(
struct acpi_hest_generic *generic,
- struct acpi_hest_generic_status *estatus)
+ struct acpi_generic_status *estatus)
{
int i, slot = -1, count;
unsigned long long now, duration, period, max_period = 0;
@@ -751,7 +751,7 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
struct llist_node *llnode, *next;
struct ghes_estatus_node *estatus_node;
struct acpi_hest_generic *generic;
- struct acpi_hest_generic_status *estatus;
+ struct acpi_generic_status *estatus;
u32 len, node_len;

llnode = llist_del_all(&ghes_estatus_llist);
@@ -765,7 +765,7 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
estatus_node = llist_entry(llnode, struct ghes_estatus_node,
llnode);
estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
- len = apei_estatus_len(estatus);
+ len = cper_estatus_len(estatus);
node_len = GHES_ESTATUS_NODE_LEN(len);
ghes_do_proc(estatus_node->ghes, estatus);
if (!ghes_estatus_cached(estatus)) {
@@ -784,7 +784,7 @@ static void ghes_print_queued_estatus(void)
struct llist_node *llnode;
struct ghes_estatus_node *estatus_node;
struct acpi_hest_generic *generic;
- struct acpi_hest_generic_status *estatus;
+ struct acpi_generic_status *estatus;
u32 len, node_len;

llnode = llist_del_all(&ghes_estatus_llist);
@@ -797,7 +797,7 @@ static void ghes_print_queued_estatus(void)
estatus_node = llist_entry(llnode, struct ghes_estatus_node,
llnode);
estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
- len = apei_estatus_len(estatus);
+ len = cper_estatus_len(estatus);
node_len = GHES_ESTATUS_NODE_LEN(len);
generic = estatus_node->generic;
ghes_print_estatus(NULL, generic, estatus);
@@ -843,7 +843,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
#ifdef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG
u32 len, node_len;
struct ghes_estatus_node *estatus_node;
- struct acpi_hest_generic_status *estatus;
+ struct acpi_generic_status *estatus;
#endif
if (!(ghes->flags & GHES_TO_CLEAR))
continue;
@@ -851,7 +851,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
if (ghes_estatus_cached(ghes->estatus))
goto next;
/* Save estatus for further processing in IRQ context */
- len = apei_estatus_len(ghes->estatus);
+ len = cper_estatus_len(ghes->estatus);
node_len = GHES_ESTATUS_NODE_LEN(len);
estatus_node = (void *)gen_pool_alloc(ghes_estatus_pool,
node_len);
@@ -923,7 +923,7 @@ static int ghes_probe(struct platform_device *ghes_dev)

rc = -EIO;
if (generic->error_block_length <
- sizeof(struct acpi_hest_generic_status)) {
+ sizeof(struct acpi_generic_status)) {
pr_warning(FW_BUG GHES_PFX "Invalid error block length: %u for generic hardware error source: %d\n",
generic->error_block_length,
generic->header.source_id);
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 0bd750e..556c83ee 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -596,7 +596,7 @@ struct acpi_hest_generic {

/* Generic Error Status block */

-struct acpi_hest_generic_status {
+struct acpi_generic_status {
u32 block_status;
u32 raw_data_offset;
u32 raw_data_length;
@@ -606,15 +606,15 @@ struct acpi_hest_generic_status {

/* Values for block_status flags above */

-#define ACPI_HEST_UNCORRECTABLE (1)
-#define ACPI_HEST_CORRECTABLE (1<<1)
-#define ACPI_HEST_MULTIPLE_UNCORRECTABLE (1<<2)
-#define ACPI_HEST_MULTIPLE_CORRECTABLE (1<<3)
-#define ACPI_HEST_ERROR_ENTRY_COUNT (0xFF<<4) /* 8 bits, error count */
+#define ACPI_GEN_ERR_UC BIT(0)
+#define ACPI_GEN_ERR_CE BIT(1)
+#define ACPI_GEN_ERR_MULTI_UC BIT(2)
+#define ACPI_GEN_ERR_MULTI_CE BIT(3)
+#define ACPI_GEN_ERR_COUNT_SHIFT (0xFF<<4) /* 8 bits, error count */

/* Generic Error Data entry */

-struct acpi_hest_generic_data {
+struct acpi_generic_data {
u8 section_type[16];
u32 error_severity;
u16 revision;
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 720446c..dfd60d0 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -14,7 +14,7 @@

struct ghes {
struct acpi_hest_generic *generic;
- struct acpi_hest_generic_status *estatus;
+ struct acpi_generic_status *estatus;
u64 buffer_paddr;
unsigned long flags;
union {
diff --git a/include/linux/cper.h b/include/linux/cper.h
index c230494..09ebe21 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -389,6 +389,6 @@ struct cper_sec_pcie {

u64 cper_next_record_id(void);
void cper_print_bits(const char *prefix, unsigned int bits,
- const char *strs[], unsigned int strs_size);
+ const char * const strs[], unsigned int strs_size);

#endif

Chen, Gong

unread,
Oct 16, 2013, 11:20:02 AM10/16/13
to
Use trace interface to elaborate all H/W error related
information.

v2 -> v1: move trace interface into ras_event.h

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/Kconfig | 7 ++-
drivers/acpi/Makefile | 4 ++
drivers/acpi/acpi_extlog.c | 28 +++++++++++-
drivers/acpi/apei/cper.c | 13 ++++--
drivers/acpi/extlog_trace.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
drivers/acpi/extlog_trace.h | 16 +++++++
include/linux/cper.h | 2 +
include/ras/ras_event.h | 61 +++++++++++++++++++++++++
8 files changed, 232 insertions(+), 6 deletions(-)
create mode 100644 drivers/acpi/extlog_trace.c
create mode 100644 drivers/acpi/extlog_trace.h

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 819c06b..0cc8fc8 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -372,13 +372,18 @@ config ACPI_BGRT

source "drivers/acpi/apei/Kconfig"

+config EXTLOG_TRACE
+ def_bool n
+
config ACPI_EXTLOG
tristate "Extended Error Log support"
depends on X86_MCE
+ select EXTLOG_TRACE
default n
help
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 plus an additional special
+ tracepoint which carries that information to userspace.

endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index bce34af..a6e41b7 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -83,4 +83,8 @@ obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o

obj-$(CONFIG_ACPI_APEI) += apei/

+# extended log support
+acpi-$(CONFIG_EXTLOG_TRACE) += extlog_trace.o
+CFLAGS_extlog_trace.o := -I$(src)
+
obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index d55b072..01e6a89 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -15,6 +15,7 @@
#include <asm/mce.h>

#include "apei/apei-internal.h"
+#include "extlog_trace.h"

#define EXT_ELOG_ENTRY_MASK GENMASK_ULL(52, 0) /* elog entry address mask */

@@ -44,6 +45,8 @@ struct extlog_l1_head {

static u8 extlog_dsm_uuid[] = "663E35AF-CC10-41A4-88EA-5470AF055295";

+static const uuid_le invalid_uuid = NULL_UUID_LE;
+
/* L1 table related physical address */
static u64 elog_base;
static size_t elog_size;
@@ -132,7 +135,12 @@ static int print_extlog_rcd(const char *pfx,

static int extlog_print(const char *pfx, int cpu, int bank)
{
- 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);
@@ -143,7 +151,23 @@ static int extlog_print(const char *pfx, int cpu, int bank)
/* clear record status to enable BIOS to update it again */
estatus->block_status = 0;

- rc = print_extlog_rcd(pfx, (struct acpi_generic_status *)elog_buf, cpu);
+ tmp = (struct acpi_generic_status *)elog_buf;
+ gdata = (struct acpi_generic_data *)(tmp + 1);
+ rc = print_extlog_rcd(pfx, tmp, cpu);
+
+ /* 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 rc;
}
diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index f5bc227..44bde6a 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -59,11 +59,12 @@ static const char *cper_severity_strs[] = {
"info",
};

-static const char *cper_severity_str(unsigned int severity)
+const char *cper_severity_str(unsigned int severity)
{
return severity < ARRAY_SIZE(cper_severity_strs) ?
cper_severity_strs[severity] : "unknown";
}
+EXPORT_SYMBOL_GPL(cper_severity_str);

/*
* cper_print_bits - print strings for set bits
@@ -198,6 +199,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)
@@ -235,8 +243,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,
- 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;
diff --git a/drivers/acpi/extlog_trace.c b/drivers/acpi/extlog_trace.c
new file mode 100644
index 0000000..2864080
--- /dev/null
+++ b/drivers/acpi/extlog_trace.c
@@ -0,0 +1,107 @@
+#include <linux/export.h>
+#include <linux/dmi.h>
+#include "extlog_trace.h"
+
+#define CREATE_TRACE_POINTS
+#define TRACE_INCLUDE_PATH ../../include/ras
+#include <ras/ras_event.h>
+
+static char mem_location[LOC_LEN];
+static char dimm_location[LOC_LEN];
+
+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;
+ 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;
+}
+
+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);
+ 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);
+}
+
+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 = 0;
+
+ if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
+ etype = mem->error_type;
+ 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;
+ }
+ 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);
+}
+EXPORT_SYMBOL_GPL(trace_mem_error);
diff --git a/drivers/acpi/extlog_trace.h b/drivers/acpi/extlog_trace.h
new file mode 100644
index 0000000..67bb2c5
--- /dev/null
+++ b/drivers/acpi/extlog_trace.h
@@ -0,0 +1,16 @@
+#ifndef __DEBUG_EXTLOG_H
+#define __DEBUG_EXTLOG_H
+
+#include <linux/cper.h>
+
+#ifdef CONFIG_EXTLOG_TRACE
+extern void trace_mem_error(const uuid_le *fru_id, char *fru_text,
+ u64 err_count, u32 severity, struct cper_sec_mem_err *mem);
+#else
+void trace_mem_error(const uuid_le *fru_id, char *fru_text,
+ u64 err_count, u32 severity, struct cper_sec_mem_err *mem)
+{
+}
+#endif
+
+#endif
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 2fc0ec3..c6d87fc 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -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);

diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 21cdb0b..fce14b6 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -8,6 +8,67 @@
#include <linux/tracepoint.h>
#include <linux/edac.h>
#include <linux/ktime.h>
+#include <linux/cper.h>
+
+/*
+ * MCE Extended Error Log trace event
+ *
+ * These events are generated when hardware detects a corrected or
+ * uncorrected event.
+ *
+ */
+
+/* memory trace event */
+
+#define LOC_LEN 512
+#define MSG_LEN ((LOC_LEN) * 2)
+
+TRACE_EVENT(extlog_mem_event,
+ TP_PROTO(u32 etype,
+ char *dimm_loc,
+ const uuid_le *fru_id,
+ char *fru_text,
+ u64 error_count,
+ u32 severity,
+ u64 phy_addr,
+ char *mem_loc),
+
+ TP_ARGS(etype, dimm_loc, 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)
+ __dynamic_array(char, msg, MSG_LEN)
+ ),
+
+ TP_fast_assign(
+ __entry->error_count = error_count;
+ __entry->severity = severity;
+ __entry->etype = etype;
+ if (dimm_loc[0] != '\0')
+ snprintf(__get_dynamic_array(dimm_info), LOC_LEN - 1,
+ "on %s", dimm_loc);
+ else
+ __assign_str(dimm_info, "");
+ if (phy_addr != 0)
+ snprintf(__get_dynamic_array(msg), MSG_LEN - 1,
+ "(FRU: %pUl %.20s physical addr: 0x%016llx%s)",
+ fru_id, fru_text, phy_addr, mem_loc);
+ else
+ __assign_str(msg, "");
+ ),
+
+ TP_printk("%llu %s error%s: %s %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),
+ __get_str(msg))
+);

/*
* Hardware Events Report

Chen, Gong

unread,
Oct 16, 2013, 11:20:03 AM10/16/13
to
Commit aaf9d93 only catches condition check before print,
but the similar check is needed during printing CPER error
sections.

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
Reviewed-by: Borislav Petkov <b...@suse.de>
---
drivers/acpi/apei/cper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index 33dc6a0..f827f02 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -353,7 +353,7 @@ void apei_estatus_print(const char *pfx,
cper_severity_str(severity));
data_len = estatus->data_length;
gdata = (struct acpi_hest_generic_data *)(estatus + 1);
- while (data_len > sizeof(*gdata)) {
+ while (data_len >= sizeof(*gdata)) {
gedata_len = gdata->error_data_length;
apei_estatus_print_section(pfx, gdata, sec_no);
data_len -= gedata_len + sizeof(*gdata);

Chen, Gong

unread,
Oct 16, 2013, 11:20:03 AM10/16/13
to
After H/W error happens under FFM enabled mode, lots of information
are shown but some important parts like DIMM location missed. This
patch is used to show these extra fileds.

Original-author: Tony Luck <tony...@intel.com>
Signed-off-by: Chen, Gong <gong...@linux.intel.com>
Acked-by: Naveen N. Rao <naveen...@linux.vnet.ibm.com>
---
drivers/acpi/apei/cper.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index 946ef52..b1a8a55 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -28,6 +28,7 @@
#include <linux/module.h>
#include <linux/time.h>
#include <linux/cper.h>
+#include <linux/dmi.h>
#include <linux/acpi.h>
#include <linux/pci.h>
#include <linux/aer.h>
@@ -210,6 +211,8 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
printk("%s""card: %d\n", pfx, mem->card);
if (mem->validation_bits & CPER_MEM_VALID_MODULE)
printk("%s""module: %d\n", pfx, mem->module);
+ if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
+ printk("%s""rank: %d\n", pfx, mem->rank);
if (mem->validation_bits & CPER_MEM_VALID_BANK)
printk("%s""bank: %d\n", pfx, mem->bank);
if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
@@ -232,6 +235,15 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
cper_mem_err_type_strs[etype] : "unknown");
}
+ if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
+ const char *bank = NULL, *device = NULL;
+ dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
+ if (bank != NULL && device != NULL)
+ printk("%s""DIMM location: %s %s", pfx, bank, device);
+ else
+ printk("%s""DIMM DMI handle: 0x%.4x",
+ pfx, mem->mem_dev_handle);
+ }
}

static const char *cper_pcie_port_type_strs[] = {

Chen, Gong

unread,
Oct 16, 2013, 11:20:03 AM10/16/13
to
Keep up only the most important fields for memory error
reporting. The detail information will be moved to perf/trace
interface.

Suggested-by: Tony Luck <tony...@intel.com>
Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/apei/cper.c | 69 +++++++++++++++++++++++-------------------------
1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index b1a8a55..f5bc227 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -33,6 +33,9 @@
#include <linux/pci.h>
#include <linux/aer.h>

+#define INDENT_SP " "
+#define HW_CE_INFO \
+ "Above error has been corrected by h/w and no further action required"
/*
* CPER record ID need to be unique even after reboot, because record
* ID is used as index for ERST storage, while CPER records from
@@ -206,29 +209,29 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
printk("%s""physical_address_mask: 0x%016llx\n",
pfx, mem->physical_addr_mask);
if (mem->validation_bits & CPER_MEM_VALID_NODE)
- printk("%s""node: %d\n", pfx, mem->node);
+ pr_debug("node: %d\n", mem->node);
if (mem->validation_bits & CPER_MEM_VALID_CARD)
- printk("%s""card: %d\n", pfx, mem->card);
+ pr_debug("card: %d\n", mem->card);
if (mem->validation_bits & CPER_MEM_VALID_MODULE)
- printk("%s""module: %d\n", pfx, mem->module);
+ pr_debug("module: %d\n", mem->module);
if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
- printk("%s""rank: %d\n", pfx, mem->rank);
+ pr_debug("rank: %d\n", mem->rank);
if (mem->validation_bits & CPER_MEM_VALID_BANK)
- printk("%s""bank: %d\n", pfx, mem->bank);
+ pr_debug("bank: %d\n", mem->bank);
if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
- printk("%s""device: %d\n", pfx, mem->device);
+ pr_debug("device: %d\n", mem->device);
if (mem->validation_bits & CPER_MEM_VALID_ROW)
- printk("%s""row: %d\n", pfx, mem->row);
+ pr_debug("row: %d\n", mem->row);
if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
- printk("%s""column: %d\n", pfx, mem->column);
+ pr_debug("column: %d\n", mem->column);
if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
- printk("%s""bit_position: %d\n", pfx, mem->bit_pos);
+ pr_debug("bit_position: %d\n", mem->bit_pos);
if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
- printk("%s""requestor_id: 0x%016llx\n", pfx, mem->requestor_id);
+ pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id);
if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
- printk("%s""responder_id: 0x%016llx\n", pfx, mem->responder_id);
+ pr_debug("responder_id: 0x%016llx\n", mem->responder_id);
if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
- printk("%s""target_id: 0x%016llx\n", pfx, mem->target_id);
+ pr_debug("target_id: 0x%016llx\n", mem->target_id);
if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
u8 etype = mem->error_type;
printk("%s""error_type: %d, %s\n", pfx, etype,
@@ -296,55 +299,45 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
pfx, pcie->bridge.secondary_status, pcie->bridge.control);
}

-static const char * const cper_estatus_section_flag_strs[] = {
- "primary",
- "containment warning",
- "reset",
- "error threshold exceeded",
- "resource not accessible",
- "latent error",
-};
-
static void cper_estatus_print_section(
const char *pfx, const struct acpi_generic_data *gdata, int sec_no)
{
uuid_le *sec_type = (uuid_le *)gdata->section_type;
__u16 severity;
+ char newpfx[64];

severity = gdata->error_severity;
- printk("%s""section: %d, severity: %d, %s\n", pfx, sec_no, severity,
+ printk("%s""Error %d, type: %s\n", pfx, sec_no,
cper_severity_str(severity));
- printk("%s""flags: 0x%02x\n", pfx, gdata->flags);
- cper_print_bits(pfx, gdata->flags, cper_estatus_section_flag_strs,
- ARRAY_SIZE(cper_estatus_section_flag_strs));
if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
printk("%s""fru_id: %pUl\n", pfx, (uuid_le *)gdata->fru_id);
if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
printk("%s""fru_text: %.20s\n", pfx, gdata->fru_text);

+ snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
if (!uuid_le_cmp(*sec_type, CPER_SEC_PROC_GENERIC)) {
struct cper_sec_proc_generic *proc_err = (void *)(gdata + 1);
- printk("%s""section_type: general processor error\n", pfx);
+ printk("%s""section_type: general processor error\n", newpfx);
if (gdata->error_data_length >= sizeof(*proc_err))
- cper_print_proc_generic(pfx, proc_err);
+ cper_print_proc_generic(newpfx, proc_err);
else
goto err_section_too_small;
} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
- printk("%s""section_type: memory error\n", pfx);
+ printk("%s""section_type: memory error\n", newpfx);
if (gdata->error_data_length >= sizeof(*mem_err))
- cper_print_mem(pfx, mem_err);
+ cper_print_mem(newpfx, mem_err);
else
goto err_section_too_small;
} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PCIE)) {
struct cper_sec_pcie *pcie = (void *)(gdata + 1);
- printk("%s""section_type: PCIe error\n", pfx);
+ printk("%s""section_type: PCIe error\n", newpfx);
if (gdata->error_data_length >= sizeof(*pcie))
- cper_print_pcie(pfx, pcie, gdata);
+ cper_print_pcie(newpfx, pcie, gdata);
else
goto err_section_too_small;
} else
- printk("%s""section type: unknown, %pUl\n", pfx, sec_type);
+ printk("%s""section type: unknown, %pUl\n", newpfx, sec_type);

return;

@@ -358,21 +351,25 @@ void cper_estatus_print(const char *pfx,
struct acpi_generic_data *gdata;
unsigned int data_len, gedata_len;
int sec_no = 0;
+ char newpfx[64];
__u16 severity;

- printk("%s""Generic Hardware Error Status\n", pfx);
severity = estatus->error_severity;
- printk("%s""severity: %d, %s\n", pfx, severity,
- cper_severity_str(severity));
+ printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
data_len = estatus->data_length;
gdata = (struct acpi_generic_data *)(estatus + 1);
+ snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
while (data_len >= sizeof(*gdata)) {
gedata_len = gdata->error_data_length;
- cper_estatus_print_section(pfx, gdata, sec_no);
+ cper_estatus_print_section(newpfx, gdata, sec_no);
data_len -= gedata_len + sizeof(*gdata);
gdata = (void *)(gdata + 1) + gedata_len;
sec_no++;
}
+ if (severity != CPER_SEV_FATAL)
+ printk("%s%s\n", pfx,
+ "Above error has been corrected by h/w "
+ "and require no further action");
}
EXPORT_SYMBOL_GPL(cper_estatus_print);

Chen, Gong

unread,
Oct 16, 2013, 11:20:03 AM10/16/13
to
In latest UEFI spec(by now it is 2.4) memory error definition
for CPER (UEFI 2.4 Appendix N Common Platform Error Record)
adds some new fields. These fields help people to locate
memory error on actual DIMM location.

Original-author: Tony Luck <tony...@intel.com>
Signed-off-by: Chen, Gong <gong...@linux.intel.com>
Reviewed-by: Borislav Petkov <b...@suse.de>
---
arch/x86/kernel/cpu/mcheck/mce-apei.c | 3 +--
drivers/acpi/apei/cper.c | 7 ++++---
drivers/acpi/apei/ghes.c | 4 ++--
drivers/edac/ghes_edac.c | 5 ++---
include/linux/cper.h | 11 +++++++++--
5 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce-apei.c b/arch/x86/kernel/cpu/mcheck/mce-apei.c
index cd8b166..de8b60a 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-apei.c
+++ b/arch/x86/kernel/cpu/mcheck/mce-apei.c
@@ -42,8 +42,7 @@ void apei_mce_report_mem_error(int corrected, struct cper_sec_mem_err *mem_err)
struct mce m;

/* Only corrected MC is reported */
- if (!corrected || !(mem_err->validation_bits &
- CPER_MEM_VALID_PHYSICAL_ADDRESS))
+ if (!corrected || !(mem_err->validation_bits & CPER_MEM_VALID_PA))
return;

mce_setup(&m);
diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index eb5f6d6..946ef52 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -8,7 +8,7 @@
* various tables, such as ERST, BERT and HEST etc.
*
* For more information about CPER, please refer to Appendix N of UEFI
- * Specification version 2.3.
+ * Specification version 2.4.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License version
@@ -191,16 +191,17 @@ static const char *cper_mem_err_type_strs[] = {
"memory sparing",
"scrub corrected error",
"scrub uncorrected error",
+ "physical memory map-out event",
};

static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
{
if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status);
- if (mem->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)
+ if (mem->validation_bits & CPER_MEM_VALID_PA)
printk("%s""physical_address: 0x%016llx\n",
pfx, mem->physical_addr);
- if (mem->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK)
+ if (mem->validation_bits & CPER_MEM_VALID_PA_MASK)
printk("%s""physical_address_mask: 0x%016llx\n",
pfx, mem->physical_addr_mask);
if (mem->validation_bits & CPER_MEM_VALID_NODE)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 0db6e4f..a30bc31 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -419,7 +419,7 @@ static void ghes_handle_memory_failure(struct acpi_generic_data *gdata, int sev)

if (sec_sev == GHES_SEV_CORRECTED &&
(gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&
- (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
+ (mem_err->validation_bits & CPER_MEM_VALID_PA)) {
pfn = mem_err->physical_addr >> PAGE_SHIFT;
if (pfn_valid(pfn))
memory_failure_queue(pfn, 0, MF_SOFT_OFFLINE);
@@ -430,7 +430,7 @@ static void ghes_handle_memory_failure(struct acpi_generic_data *gdata, int sev)
}
if (sev == GHES_SEV_RECOVERABLE &&
sec_sev == GHES_SEV_RECOVERABLE &&
- mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
+ mem_err->validation_bits & CPER_MEM_VALID_PA) {
pfn = mem_err->physical_addr >> PAGE_SHIFT;
memory_failure_queue(pfn, 0, 0);
}
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index bb53467..0ad797b 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -297,15 +297,14 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
}

/* Error address */
- if (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
+ if (mem_err->validation_bits & CPER_MEM_VALID_PA) {
e->page_frame_number = mem_err->physical_addr >> PAGE_SHIFT;
e->offset_in_page = mem_err->physical_addr & ~PAGE_MASK;
}

/* Error grain */
- if (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK) {
+ if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
- }

/* Memory error location, mapped on e->location */
p = e->location;
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 09ebe21..2fc0ec3 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -218,8 +218,8 @@ enum {
#define CPER_PROC_VALID_IP 0x1000

#define CPER_MEM_VALID_ERROR_STATUS 0x0001
-#define CPER_MEM_VALID_PHYSICAL_ADDRESS 0x0002
-#define CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK 0x0004
+#define CPER_MEM_VALID_PA 0x0002
+#define CPER_MEM_VALID_PA_MASK 0x0004
#define CPER_MEM_VALID_NODE 0x0008
#define CPER_MEM_VALID_CARD 0x0010
#define CPER_MEM_VALID_MODULE 0x0020
@@ -232,6 +232,9 @@ enum {
#define CPER_MEM_VALID_RESPONDER_ID 0x1000
#define CPER_MEM_VALID_TARGET_ID 0x2000
#define CPER_MEM_VALID_ERROR_TYPE 0x4000
+#define CPER_MEM_VALID_RANK_NUMBER 0x8000
+#define CPER_MEM_VALID_CARD_HANDLE 0x10000
+#define CPER_MEM_VALID_MODULE_HANDLE 0x20000

#define CPER_PCIE_VALID_PORT_TYPE 0x0001
#define CPER_PCIE_VALID_VERSION 0x0002
@@ -347,6 +350,10 @@ struct cper_sec_mem_err {
__u64 responder_id;
__u64 target_id;
__u8 error_type;
+ __u8 reserved;
+ __u16 rank;
+ __u16 mem_array_handle; /* card handle in UEFI 2.4 */
+ __u16 mem_dev_handle; /* module handle in UEFI 2.4 */
};

struct cper_sec_pcie {

Chen, Gong

unread,
Oct 16, 2013, 11:20:03 AM10/16/13
to
This error log driver (a.k.a eMCA driver) is implemented based on
http://www.intel.com/content/www/us/en/architecture-and-technology/enhanced-mca-logging-xeon-paper.htm l.
After errors are captured, more valuable information can be
got with this new enhanced error log driver.

v2 -> v1: eliminate spin_lock & minor fixes suggested by Boris

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
arch/x86/include/asm/mce.h | 5 +
arch/x86/kernel/cpu/mcheck/mce.c | 20 +++
drivers/acpi/Kconfig | 9 ++
drivers/acpi/Makefile | 2 +
drivers/acpi/acpi_extlog.c | 319 +++++++++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 3 +-
include/linux/acpi.h | 1 +
7 files changed, 358 insertions(+), 1 deletion(-)
create mode 100644 drivers/acpi/acpi_extlog.c

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index cbe6b9e..072b2f8 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -16,6 +16,7 @@
#define MCG_EXT_CNT_SHIFT 16
#define MCG_EXT_CNT(c) (((c) & MCG_EXT_CNT_MASK) >> MCG_EXT_CNT_SHIFT)
#define MCG_SER_P (1ULL<<24) /* MCA recovery/new status bits */
+#define MCG_ELOG_P (1ULL<<26) /* Extended error log supported */

/* MCG_STATUS register defines */
#define MCG_STATUS_RIPV (1ULL<<0) /* restart ip valid */
@@ -186,6 +187,10 @@ enum mcp_flags {
MCP_UC = (1 << 1), /* log uncorrected errors */
MCP_DONTLOG = (1 << 2), /* only clear, don't log */
};
+
+void register_elog_handler(int (*f)(const char *, int, int));
+void unregister_elog_handler(int (*f)(const char *, int, int));
+
void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);

int mce_notify_irq(void);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index b3218cd..c11a53f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -48,6 +48,8 @@

#include "mce-internal.h"

+static int (*mce_ext_err_print)(const char *, int, int) = NULL;
+
static DEFINE_MUTEX(mce_chrdev_read_mutex);

#define rcu_dereference_check_mce(p) \
@@ -576,6 +578,21 @@ static void mce_read_aux(struct mce *m, int i)

DEFINE_PER_CPU(unsigned, mce_poll_count);

+void register_elog_handler(int (*f)(const char *, int, int))
+{
+ mce_ext_err_print = f;
+}
+EXPORT_SYMBOL_GPL(register_elog_handler);
+
+void unregister_elog_handler(int (*f)(const char *, int, int))
+{
+ if (f) {
+ WARN_ON(mce_ext_err_print != f);
+ mce_ext_err_print = NULL;
+ }
+}
+EXPORT_SYMBOL_GPL(unregister_elog_handler);
+
/*
* Poll for corrected events or events that happened before reset.
* Those are just logged through /dev/mcelog.
@@ -624,6 +641,9 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
(m.status & (mca_cfg.ser ? MCI_STATUS_S : MCI_STATUS_UC)))
continue;

+ if (mce_ext_err_print)
+ mce_ext_err_print(NULL, m.extcpu, i);
+
mce_read_aux(&m, i);

if (!(flags & MCP_TIMESTAMP))
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 22327e6..819c06b 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -372,4 +372,13 @@ config ACPI_BGRT

source "drivers/acpi/apei/Kconfig"

+config ACPI_EXTLOG
+ tristate "Extended Error Log support"
+ depends on X86_MCE
+ default n
+ help
+ 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.
+
endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index cdaf68b..bce34af 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -82,3 +82,5 @@ processor-$(CONFIG_CPU_FREQ) += processor_perflib.o
obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o

obj-$(CONFIG_ACPI_APEI) += apei/
+
+obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
new file mode 100644
index 0000000..d55b072
--- /dev/null
+++ b/drivers/acpi/acpi_extlog.c
@@ -0,0 +1,319 @@
+/*
+ * Extended Error Log driver
+ *
+ * Copyright (C) 2013 Intel Corp.
+ * Author: Chen, Gong <gong...@intel.com>
+ *
+ * This file is licensed under GPLv2.
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+#include <linux/cper.h>
+#include <linux/ratelimit.h>
+#include <asm/mce.h>
+
+#include "apei/apei-internal.h"
+
+#define EXT_ELOG_ENTRY_MASK GENMASK_ULL(52, 0) /* elog entry address mask */
+
+#define EXTLOG_DSM_REV 0x0
+#define EXTLOG_FN_QUERY 0x0
+#define EXTLOG_FN_ADDR 0x1
+
+#define FLAG_OS_OPTIN BIT(0)
+#define EXTLOG_QUERY_L1_EXIST BIT(1)
+#define ELOG_ENTRY_VALID (1ULL<<63)
+#define ELOG_ENTRY_LEN 0x1000
+
+#define EMCA_BUG \
+ "Can not request iomem region <0x%016llx-0x%016llx> - eMCA disabled\n"
+
+struct extlog_l1_head {
+ u32 ver; /* Header Version */
+ u32 hdr_len; /* Header Length */
+ u64 total_len; /* entire L1 Directory length including this header */
+ u64 elog_base; /* MCA Error Log Directory base address */
+ u64 elog_len; /* MCA Error Log Directory length */
+ u32 flags; /* bit 0 - OS/VMM Opt-in */
+ u8 rev0[12];
+ u32 entries; /* Valid L1 Directory entries per logical processor */
+ u8 rev1[12];
+};
+
+static u8 extlog_dsm_uuid[] = "663E35AF-CC10-41A4-88EA-5470AF055295";
+
+/* L1 table related physical address */
+static u64 elog_base;
+static size_t elog_size;
+static u64 l1_dirbase;
+static size_t l1_size;
+
+/* L1 table related virtual address */
+static void __iomem *extlog_l1_addr;
+static void __iomem *elog_addr;
+
+static void *elog_buf;
+
+static u64 *l1_entry_base;
+static u32 l1_percpu_entry;
+
+#define ELOG_IDX(cpu, bank) \
+ (cpu_physical_id(cpu) * l1_percpu_entry + (bank))
+
+#define ELOG_ENTRY_DATA(idx) \
+ (*(l1_entry_base + (idx)))
+
+#define ELOG_ENTRY_ADDR(phyaddr) \
+ (phyaddr - elog_base + (u8 *)elog_addr)
+
+static struct acpi_generic_status *extlog_elog_entry_check(int cpu, int bank)
+{
+ int idx;
+ u64 data;
+ struct acpi_generic_status *estatus;
+
+ WARN_ON(cpu < 0);
+ idx = ELOG_IDX(cpu, bank);
+ data = ELOG_ENTRY_DATA(idx);
+ if ((data & ELOG_ENTRY_VALID) == 0)
+ return NULL;
+
+ data &= EXT_ELOG_ENTRY_MASK;
+ estatus = (struct acpi_generic_status *)ELOG_ENTRY_ADDR(data);
+
+ /* if no valid data in elog entry, just return */
+ if (estatus->block_status == 0)
+ return NULL;
+
+ return estatus;
+}
+
+static void __print_extlog_rcd(const char *pfx,
+ struct acpi_generic_status *estatus, int cpu)
+{
+ static atomic_t seqno;
+ unsigned int curr_seqno;
+ char pfx_seq[64];
+
+ if (!pfx) {
+ if (estatus->error_severity <= CPER_SEV_CORRECTED)
+ pfx = KERN_INFO;
+ else
+ pfx = KERN_ERR;
+ }
+ curr_seqno = atomic_inc_return(&seqno);
+ snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
+ printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
+ cper_estatus_print(pfx_seq, estatus);
+}
+
+static int print_extlog_rcd(const char *pfx,
+ struct acpi_generic_status *estatus, int cpu)
+{
+ /* Not more than 2 messages every 5 seconds */
+ static DEFINE_RATELIMIT_STATE(ratelimit_corrected, 5*HZ, 2);
+ static DEFINE_RATELIMIT_STATE(ratelimit_uncorrected, 5*HZ, 2);
+ struct ratelimit_state *ratelimit;
+
+ if (estatus->error_severity == CPER_SEV_CORRECTED ||
+ (estatus->error_severity == CPER_SEV_INFORMATIONAL))
+ ratelimit = &ratelimit_corrected;
+ else
+ ratelimit = &ratelimit_uncorrected;
+ if (__ratelimit(ratelimit)) {
+ __print_extlog_rcd(pfx, estatus, cpu);
+ return 0;
+ }
+
+ return 1;
+}
+
+static int extlog_print(const char *pfx, int cpu, int bank)
+{
+ struct acpi_generic_status *estatus;
+ int rc;
+
+ estatus = extlog_elog_entry_check(cpu, bank);
+ if (estatus == NULL)
+ return -EINVAL;
+
+ memcpy(elog_buf, (void *)estatus, ELOG_ENTRY_LEN);
+ /* clear record status to enable BIOS to update it again */
+ estatus->block_status = 0;
+
+ rc = print_extlog_rcd(pfx, (struct acpi_generic_status *)elog_buf, cpu);
+
+ return rc;
+}
+
+static int extlog_get_dsm(acpi_handle handle, int rev, int func, u64 *ret)
+{
+ struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
+ struct acpi_object_list input;
+ union acpi_object params[4], *obj;
+ u8 uuid[16];
+ int i;
+
+ acpi_str_to_uuid(extlog_dsm_uuid, uuid);
+ input.count = 4;
+ input.pointer = params;
+ params[0].type = ACPI_TYPE_BUFFER;
+ params[0].buffer.length = 16;
+ params[0].buffer.pointer = uuid;
+ params[1].type = ACPI_TYPE_INTEGER;
+ params[1].integer.value = rev;
+ params[2].type = ACPI_TYPE_INTEGER;
+ params[2].integer.value = func;
+ params[3].type = ACPI_TYPE_PACKAGE;
+ params[3].package.count = 0;
+ params[3].package.elements = NULL;
+
+ if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DSM", &input, &buf)))
+ return -1;
+
+ *ret = 0;
+ obj = (union acpi_object *)buf.pointer;
+ if (obj->type == ACPI_TYPE_INTEGER)
+ *ret = obj->integer.value;
+ else if (obj->type == ACPI_TYPE_BUFFER) {
+ if (obj->buffer.length <= 8) {
+ for (i = 0; i < obj->buffer.length; i++)
+ *ret |= (obj->buffer.pointer[i] << (i * 8));
+ }
+ }
+ kfree(buf.pointer);
+
+ return 0;
+}
+
+static bool extlog_get_l1addr(void)
+{
+ acpi_handle handle;
+ u64 ret;
+
+ if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
+ return false;
+
+ if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_QUERY, &ret) ||
+ !(ret & EXTLOG_QUERY_L1_EXIST))
+ return false;
+
+ if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_ADDR, &ret))
+ return false;
+
+ l1_dirbase = ret;
+ /* Spec says L1 directory must be 4K aligned, bail out if it isn't */
+ if (l1_dirbase & ((1 << 12) - 1)) {
+ pr_warn(FW_BUG "L1 Directory is invalid at physical %llx\n",
+ l1_dirbase);
+ return false;
+ }
+
+ return true;
+}
+
+static int __init extlog_init(void)
+{
+ struct extlog_l1_head *l1_head;
+ void __iomem *extlog_l1_hdr;
+ size_t l1_hdr_size;
+ struct resource *r;
+ u64 cap;
+ int rc;
+
+ rc = -ENODEV;
+
+ rdmsrl(MSR_IA32_MCG_CAP, cap);
+ if (!(cap & MCG_ELOG_P))
+ return rc;
+
+ if (!extlog_get_l1addr())
+ return rc;
+
+ rc = -EINVAL;
+ /* get L1 header to fetch necessary information */
+ l1_hdr_size = sizeof(struct extlog_l1_head);
+ r = request_mem_region(l1_dirbase, l1_hdr_size, "L1 DIR HDR");
+ if (!r) {
+ pr_warn(FW_BUG EMCA_BUG,
+ (unsigned long long)l1_dirbase,
+ (unsigned long long)l1_dirbase + l1_hdr_size);
+ goto err;
+ }
+
+ extlog_l1_hdr = acpi_os_map_memory(l1_dirbase, l1_hdr_size);
+ l1_head = (struct extlog_l1_head *)extlog_l1_hdr;
+ l1_size = l1_head->total_len;
+ l1_percpu_entry = l1_head->entries;
+ elog_base = l1_head->elog_base;
+ elog_size = l1_head->elog_len;
+ acpi_os_unmap_memory(extlog_l1_hdr, l1_hdr_size);
+ release_mem_region(l1_dirbase, l1_hdr_size);
+
+ /* remap L1 header again based on completed information */
+ r = request_mem_region(l1_dirbase, l1_size, "L1 Table");
+ if (!r) {
+ pr_warn(FW_BUG EMCA_BUG,
+ (unsigned long long)l1_dirbase,
+ (unsigned long long)l1_dirbase + l1_size);
+ goto err;
+ }
+ extlog_l1_addr = acpi_os_map_memory(l1_dirbase, l1_size);
+ l1_entry_base = (u64 *)((u8 *)extlog_l1_addr + l1_hdr_size);
+
+ /* remap elog table */
+ r = request_mem_region(elog_base, elog_size, "Elog Table");
+ if (!r) {
+ pr_warn(FW_BUG EMCA_BUG,
+ (unsigned long long)elog_base,
+ (unsigned long long)elog_base + elog_size);
+ goto err_release_l1_dir;
+ }
+ elog_addr = acpi_os_map_memory(elog_base, elog_size);
+
+ rc = -ENOMEM;
+ /* allocate buffer to save elog record */
+ elog_buf = kmalloc(ELOG_ENTRY_LEN, GFP_KERNEL);
+ if (elog_buf == NULL)
+ goto err_release_elog;
+
+ register_elog_handler(extlog_print);
+ /* enable OS to be involved to take over management from BIOS */
+ ((struct extlog_l1_head *)extlog_l1_addr)->flags |= FLAG_OS_OPTIN;
+
+ return 0;
+
+err_release_elog:
+ if (elog_addr)
+ acpi_os_unmap_memory(elog_addr, elog_size);
+ release_mem_region(elog_base, elog_size);
+err_release_l1_dir:
+ if (extlog_l1_addr)
+ acpi_os_unmap_memory(extlog_l1_addr, l1_size);
+ release_mem_region(l1_dirbase, l1_size);
+err:
+ pr_warn(FW_BUG "Extended error log disabled because of problems parsing f/w tables\n");
+ return rc;
+}
+
+static void __exit extlog_exit(void)
+{
+ unregister_elog_handler(extlog_print);
+ ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
+ if (extlog_l1_addr)
+ acpi_os_unmap_memory(extlog_l1_addr, l1_size);
+ if (elog_addr)
+ acpi_os_unmap_memory(elog_addr, elog_size);
+ release_mem_region(elog_base, elog_size);
+ release_mem_region(l1_dirbase, l1_size);
+ kfree(elog_buf);
+}
+
+module_init(extlog_init);
+module_exit(extlog_exit);
+
+MODULE_AUTHOR("Chen, Gong <gong...@intel.com>");
+MODULE_DESCRIPTION("Extended Error Log Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index b587ec8..e1bd9a1 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -174,7 +174,7 @@ static void acpi_print_osc_error(acpi_handle handle,
printk("\n");
}

-static acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
+acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
{
int i;
static int opc_map_to_uuid[16] = {6, 4, 2, 0, 11, 9, 16, 14, 19, 21,
@@ -195,6 +195,7 @@ static acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
}
return AE_OK;
}
+EXPORT_SYMBOL_GPL(acpi_str_to_uuid);

acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
{
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a5db4ae..c30bac8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -311,6 +311,7 @@ struct acpi_osc_context {
#define OSC_INVALID_REVISION_ERROR 8
#define OSC_CAPABILITIES_MASK_ERROR 16

+acpi_status acpi_str_to_uuid(char *str, u8 *uuid);
acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);

/* platform-wide _OSC bits */

Chen, Gong

unread,
Oct 16, 2013, 11:20:03 AM10/16/13
to
[PATCH v2 1/9] ACPI, APEI, CPER: Fix status check during error printing
[PATCH v2 2/9] ACPI, CPER: Update cper info
[PATCH v2 3/9] bitops: Introduce a more generic BITMASK macro
[PATCH v2 4/9] ACPI, x86: Extended error log driver for x86 platform
[PATCH v2 5/9] DMI: Parse memory device (type 17) in SMBIOS
[PATCH v2 6/9] ACPI, APEI, CPER: Add UEFI 2.4 support for memory error
[PATCH v2 7/9] ACPI, APEI, CPER: Enhance memory reporting capability
[PATCH v2 8/9] ACPI, APEI, CPER: Cleanup CPER memory error output format
[PATCH v2 9/9] ACPI / trace: Add trace interface for eMCA driver

This patch series adds an enhanced MCA event logging driver provided by Intel.
Please refer to this link: htpp://www.intel.com/content/www/us/en/architecture-and-technology/enhanced-mca-logging-xeon-paper.html

Certain usages such as Predictive Failure Analysis (PFA) require more
information about the error than what can be described in processor
machine check banks. Most server processors log additional information
about the error in processor uncore registers. Since the addresses
and layout of these registers vary widely from one processor to another,
system software cannot readily make use of them. To complicate matters
further, some of the additionalerror information cannot be constructed
without detailed knowledge about platform topology. This enhanced MCA
logging driver allows firmware to provide additional error information
to MCE/CMCI handler and thus addresses this gap.

After applying this patch series, when a memory corrected error happens,
we can get following information:

dmesg output:

[ 949.545817] {1}Hardware error detected on CPU15
[ 949.549786] {1}event severity: corrected
[ 949.549786] {1} Error 0, type: corrected
[ 949.549786] {1} section_type: memory error
[ 949.549786] {1} physical_address: 0x0000001057eb0000
[ 949.549786] {1} DIMM location: Memriser3 CHANNEL A DIMM 0
[ 949.549786] {1}Above error has been corrected by h/w and require no further action
[ 949.549786] mce: [Hardware Error]: Machine check events logged
[ 1010.902124] {2}Hardware error detected on CPU15
[ 1010.906064] {2}event severity: corrected
[ 1010.906064] {2} Error 0, type: corrected
[ 1010.906064] {2} section_type: memory error
[ 1010.906064] {2} physical_address: 0x0000001057eb0000
[ 1010.906064] {2} DIMM location: Memriser3 CHANNEL A DIMM 0
[ 1010.906064] {2}Above error has been corrected by h/w and require no further action
[ 1010.906064] mce: [Hardware Error]: Machine check events logged




trace output:

# tracer: nop
#
# entries-in-buffer/entries-written: 2/2 #P:120
#
# _-----=> irqs-off
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / delay
# TASK-PID CPU# |||| TIMESTAMP FUNCTION
# | | | |||| | |
<idle>-0 [015] d.h. 951.584641: extlog_mem_event: 1 corrected error: unknown on Memriser3 CHANNEL A DIMM 0 (FRU: 00000000-0000-0000-0000-000000000000 physical addr: 0x0000001057eb0000 node: 1 card: 0 module: 0 rank: 0 bank: 0 row: 28917 column: 1400)
<idle>-0 [015] d.h. 1013.008596: extlog_mem_event: 2 corrected errors: unknown on Memriser3 CHANNEL A DIMM 0 (FRU: 00000000-0000-0000-0000-000000000000 physical addr: 0x0000001057eb0000 node: 1 card: 0 module: 0 rank: 0 bank: 0 row: 28917 column: 1400)


dmesg output format has been updated based on the suggestion from Boris.
For trace output format we still need further discussion. In the last
patch(support trace interface) I have to reserve previous Kconfig format
because I find once I put trace_event interface in the module, it will
not work. I will paste another trace patch(it only works when acpi_extlog is
builtin) for your answer.

Chen Gong

unread,
Oct 16, 2013, 11:30:02 AM10/16/13
to
[...]
>
> dmesg output format has been updated based on the suggestion from Boris.
> For trace output format we still need further discussion. In the last
> patch(support trace interface) I have to reserve previous Kconfig format
> because I find once I put trace_event interface in the module, it will
> not work. I will paste another trace patch(it only works when acpi_extlog is
> builtin) for your answer.
> --

I put my bogus trace patch here.


=========================8<==========================

Subject: ACPI / trace: Add trace interface for eMCA driver (bogus patch)

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

Signed-off-by: Chen, Gong <gong...@linux.intel.com>
---
drivers/acpi/Kconfig | 3 +-
drivers/acpi/acpi_extlog.c | 131 ++++++++++++++++++++++++++++++++++++++++++++-
drivers/acpi/apei/cper.c | 13 +++--
include/linux/cper.h | 2 +
include/ras/ras_event.h | 61 +++++++++++++++++++++
5 files changed, 204 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 819c06b..eee0258 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -379,6 +379,7 @@ config ACPI_EXTLOG
help
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 plus an additional special
+ tracepoint which carries that information to userspace.

endif # ACPI
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index d55b072..108f4ae 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -11,11 +11,19 @@
#include <linux/acpi.h>
#include <acpi/acpi_bus.h>
#include <linux/cper.h>
+#include <linux/dmi.h>
#include <linux/ratelimit.h>
#include <asm/mce.h>

#include "apei/apei-internal.h"

+#define CREATE_TRACE_POINTS
+#define TRACE_INCLUDE_PATH ../../include/ras
+#include <ras/ras_event.h>
+
+static char mem_location[LOC_LEN];
+static char dimm_location[LOC_LEN];
+
#define EXT_ELOG_ENTRY_MASK GENMASK_ULL(52, 0) /* elog entry address mask */

#define EXTLOG_DSM_REV 0x0
@@ -44,6 +52,8 @@ struct extlog_l1_head {

static u8 extlog_dsm_uuid[] = "663E35AF-CC10-41A4-88EA-5470AF055295";

+static const uuid_le invalid_uuid = NULL_UUID_LE;
+
/* L1 table related physical address */
static u64 elog_base;
static size_t elog_size;
@@ -130,9 +140,110 @@ static int print_extlog_rcd(const char *pfx,
return 1;
+ 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);
+ 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);
+}
+
+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 = 0;
+
+ if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
+ etype = mem->error_type;
+ 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;
+ }
+ 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);
+}
+
static int extlog_print(const char *pfx, int cpu, int bank)
{
- 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);
@@ -143,7 +254,23 @@ static int extlog_print(const char *pfx, int cpu, int bank)
/* clear record status to enable BIOS to update it again */
estatus->block_status = 0;

- rc = print_extlog_rcd(pfx, (struct acpi_generic_status *)elog_buf, cpu);
+ tmp = (struct acpi_generic_status *)elog_buf;
+ gdata = (struct acpi_generic_data *)(tmp + 1);
+ rc = print_extlog_rcd(pfx, tmp, cpu);
+
+ /* 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 rc;
}
diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index f5bc227..44bde6a 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -59,11 +59,12 @@ static const char *cper_severity_strs[] = {
"info",
};

-static const char *cper_severity_str(unsigned int severity)
+const char *cper_severity_str(unsigned int severity)
{
return severity < ARRAY_SIZE(cper_severity_strs) ?
cper_severity_strs[severity] : "unknown";
}
+EXPORT_SYMBOL_GPL(cper_severity_str);

/*
* cper_print_bits - print strings for set bits
@@ -198,6 +199,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)
@@ -235,8 +243,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,
- 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;
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 2fc0ec3..c6d87fc 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -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);

diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 21cdb0b..579dbb0 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -8,6 +8,67 @@
#include <linux/tracepoint.h>
#include <linux/edac.h>
#include <linux/ktime.h>
+#include <linux/cper.h>
+
+/*
+ * MCE Extended Error Log Trace event
signature.asc

Mauro Carvalho Chehab

unread,
Oct 16, 2013, 12:00:04 PM10/16/13
to
Em Wed, 16 Oct 2013 10:56:06 -0400
"Chen, Gong" <gong...@linux.intel.com> escreveu:

> Use trace interface to elaborate all H/W error related
> information.
>
> v2 -> v1: move trace interface into ras_event.h
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>

For the already explained reasons:

Nacked-by: Mauro Carvalho Chehab <m.ch...@samsung.com>

You should re-use the existing tracepoint and better integrate it with
EDAC instead of reinventing the wheel.

Regards,
Mauro
Cheers,
Mauro

Borislav Petkov

unread,
Oct 16, 2013, 12:10:02 PM10/16/13
to
On Wed, Oct 16, 2013 at 10:55:57AM -0400, Chen, Gong wrote:
> [PATCH v2 1/9] ACPI, APEI, CPER: Fix status check during error printing
> [PATCH v2 2/9] ACPI, CPER: Update cper info
> [PATCH v2 3/9] bitops: Introduce a more generic BITMASK macro
> [PATCH v2 4/9] ACPI, x86: Extended error log driver for x86 platform
> [PATCH v2 5/9] DMI: Parse memory device (type 17) in SMBIOS
> [PATCH v2 6/9] ACPI, APEI, CPER: Add UEFI 2.4 support for memory error
> [PATCH v2 7/9] ACPI, APEI, CPER: Enhance memory reporting capability
> [PATCH v2 8/9] ACPI, APEI, CPER: Cleanup CPER memory error output format
> [PATCH v2 9/9] ACPI / trace: Add trace interface for eMCA driver
>
> This patch series adds an enhanced MCA event logging driver provided by Intel.
> Please refer to this link: htpp://www.intel.com/content/www/us/en/architecture-and-technology/enhanced-mca-logging-xeon-paper.html
>
> Certain usages such as Predictive Failure Analysis (PFA) require more
> information about the error than what can be described in processor
> machine check banks. Most server processors log additional information
> about the error in processor uncore registers. Since the addresses
> and layout of these registers vary widely from one processor to another,
> system software cannot readily make use of them. To complicate matters
> further, some of the additionalerror information cannot be constructed

space between "additional" and "error".

> without detailed knowledge about platform topology. This enhanced MCA
> logging driver allows firmware to provide additional error information
> to MCE/CMCI handler and thus addresses this gap.

This paragraph sounds like a very good description of the feature and
should actually be the Kconfig text in patch 4/9.

>
> After applying this patch series, when a memory corrected error happens,
> we can get following information:
>
> dmesg output:
>
> [ 949.545817] {1}Hardware error detected on CPU15
> [ 949.549786] {1}event severity: corrected
> [ 949.549786] {1} Error 0, type: corrected
> [ 949.549786] {1} section_type: memory error
> [ 949.549786] {1} physical_address: 0x0000001057eb0000
> [ 949.549786] {1} DIMM location: Memriser3 CHANNEL A DIMM 0
> [ 949.549786] {1}Above error has been corrected by h/w and require no further action
> [ 949.549786] mce: [Hardware Error]: Machine check events logged
> [ 1010.902124] {2}Hardware error detected on CPU15
> [ 1010.906064] {2}event severity: corrected
> [ 1010.906064] {2} Error 0, type: corrected
> [ 1010.906064] {2} section_type: memory error
> [ 1010.906064] {2} physical_address: 0x0000001057eb0000
> [ 1010.906064] {2} DIMM location: Memriser3 CHANNEL A DIMM 0
> [ 1010.906064] {2}Above error has been corrected by h/w and require no further action
> [ 1010.906064] mce: [Hardware Error]: Machine check events logged

Yep, looks almost very good. One nit: can you raise the action line
higher, like this:

> [ 949.545817] {1}Hardware error detected on CPU15
> [ 949.549786] {1}It has been corrected by h/w and requires no further action

<here come the error details>

I mean, this is only the printk output and with a userspace consumer of
the tracepoint, none of this will go to dmesg but in cases when there's
no userspace consumer, it is still readable and understandable.

> For trace output format we still need further discussion. In the last
> patch(support trace interface) I have to reserve previous Kconfig
> format because I find once I put trace_event interface in the module,
> it will not work. I will paste another trace patch(it only works when
> acpi_extlog is builtin) for your answer.

I think to be able to define TRACE_EVENTs in modules, you need
https://lwn.net/Articles/383362/

Steve, that still true?

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 16, 2013, 12:30:02 PM10/16/13
to
On Wed, Oct 16, 2013 at 10:55:59AM -0400, Chen, Gong wrote:
> To prepare for the following patches and make related
> definition more clear, update some definitions about CPER.
>
> v2 -> v1: Update some more definitions suggested by Boris
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>

Acked-by: Borislav Petkov <b...@suse.de>

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Joe Perches

unread,
Oct 16, 2013, 12:50:01 PM10/16/13
to
On Wed, 2013-10-16 at 18:05 +0200, Borislav Petkov wrote:
> On Wed, Oct 16, 2013 at 10:55:57AM -0400, Chen, Gong wrote:
[]
> > After applying this patch series, when a memory corrected error happens,
> > we can get following information:
> >
> > dmesg output:
> >
> > [ 949.545817] {1}Hardware error detected on CPU15
> > [ 949.549786] {1}event severity: corrected
> > [ 949.549786] {1} Error 0, type: corrected
> > [ 949.549786] {1} section_type: memory error
> > [ 949.549786] {1} physical_address: 0x0000001057eb0000
> > [ 949.549786] {1} DIMM location: Memriser3 CHANNEL A DIMM 0
> > [ 949.549786] {1}Above error has been corrected by h/w and require no further action
> > [ 949.549786] mce: [Hardware Error]: Machine check events logged

> Yep, looks almost very good. One nit: can you raise the action line
> higher, like this:
>
> > [ 949.545817] {1}Hardware error detected on CPU15
> > [ 949.549786] {1}It has been corrected by h/w and requires no further action
>
> <here come the error details>

Perhaps this would be nicer still with the "mce:" prefix
on all the log lines with the overall description emitted
first. It could help make grepping the log a bit easier.

[ xxx.xxxxxx] mce: [Hardware Error]: Machine check events logged
[ xxx.xxxxxx] mce: {1}Hardware error detected on CPU15
[ xxx.xxxxxx] mce: {1}Above error has been corrected by h/w and require no further action
[ xxx.xxxxxx] mce: {1}event severity: corrected
[ xxx.xxxxxx] mce: {1} Error 0, type: corrected
[ xxx.xxxxxx] mce: {1} section_type: memory error
[ xxx.xxxxxx] mce: {1} physical_address: 0x0000001057eb0000
[ xxx.xxxxxx] mce: {1} DIMM location: Memriser3 CHANNEL A DIMM 0

grammar: s/require/requires or maybe required

Borislav Petkov

unread,
Oct 16, 2013, 12:50:02 PM10/16/13
to
On Wed, Oct 16, 2013 at 10:56:00AM -0400, Chen, Gong wrote:
> GENMASK is used to create a contiguous bitmask([hi:lo]). It is
> implemented twice in current kernel. One is in EDAC driver, the other
> is in SiS/XGI FB driver. Move it to a more generic place for other
> usage.
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> Cc: Borislav Petkov <b...@alien8.de>
> Cc: Thomas Winischhofer <tho...@winischhofer.net>
> Cc: Jean-Christophe Plagniol-Villard <plag...@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.va...@ti.com>

Acked-by: Borislav Petkov <b...@suse.de>

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Mauro Carvalho Chehab

unread,
Oct 16, 2013, 12:50:02 PM10/16/13
to
Em Wed, 16 Oct 2013 10:56:03 -0400
"Chen, Gong" <gong...@linux.intel.com> escreveu:

> In latest UEFI spec(by now it is 2.4) memory error definition
> for CPER (UEFI 2.4 Appendix N Common Platform Error Record)
> adds some new fields. These fields help people to locate
> memory error on actual DIMM location.
>
> Original-author: Tony Luck <tony...@intel.com>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> Reviewed-by: Borislav Petkov <b...@suse.de>

Reviewed-by: Mauro Carvalho Chehab <m.ch...@samsung.com>
Cheers,
Mauro

Mauro Carvalho Chehab

unread,
Oct 16, 2013, 1:00:02 PM10/16/13
to
Em Wed, 16 Oct 2013 10:55:58 -0400
"Chen, Gong" <gong...@linux.intel.com> escreveu:

> Commit aaf9d93 only catches condition check before print,
> but the similar check is needed during printing CPER error
> sections.
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> Reviewed-by: Borislav Petkov <b...@suse.de>

Reviewed-by: Mauro Carvalho Chehab <m.ch...@samsung.com>

> ---
> drivers/acpi/apei/cper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
> index 33dc6a0..f827f02 100644
> --- a/drivers/acpi/apei/cper.c
> +++ b/drivers/acpi/apei/cper.c
> @@ -353,7 +353,7 @@ void apei_estatus_print(const char *pfx,
> cper_severity_str(severity));
> data_len = estatus->data_length;
> gdata = (struct acpi_hest_generic_data *)(estatus + 1);
> - while (data_len > sizeof(*gdata)) {
> + while (data_len >= sizeof(*gdata)) {
> gedata_len = gdata->error_data_length;
> apei_estatus_print_section(pfx, gdata, sec_no);
> data_len -= gedata_len + sizeof(*gdata);


--

Cheers,
Mauro

Steven Rostedt

unread,
Oct 16, 2013, 1:00:02 PM10/16/13
to
On Wed, 16 Oct 2013 18:05:50 +0200
Borislav Petkov <b...@alien8.de> wrote:


> > For trace output format we still need further discussion. In the last
> > patch(support trace interface) I have to reserve previous Kconfig
> > format because I find once I put trace_event interface in the module,
> > it will not work. I will paste another trace patch(it only works when

What did not work?

> > acpi_extlog is builtin) for your answer.
>
> I think to be able to define TRACE_EVENTs in modules, you need
> https://lwn.net/Articles/383362/
>
> Steve, that still true?
>

Take a look at samples/trace_events/

That's a module that uses TRACE_EVENTs.

What exactly is the problem here?


-- Steve

Mauro Carvalho Chehab

unread,
Oct 16, 2013, 1:00:02 PM10/16/13
to
Em Wed, 16 Oct 2013 10:55:59 -0400
"Chen, Gong" <gong...@linux.intel.com> escreveu:

> To prepare for the following patches and make related
> definition more clear, update some definitions about CPER.
>
> v2 -> v1: Update some more definitions suggested by Boris
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>

Reviewed-by: Mauro Carvalho Chehab <m.ch...@samsung.com>

Cheers,
Mauro

Borislav Petkov

unread,
Oct 16, 2013, 1:10:03 PM10/16/13
to
On Wed, Oct 16, 2013 at 10:56:02AM -0400, Chen, Gong wrote:
> This patch adds a new interface to decode memory device (type 17)
> to help error reporting on DIMMs.
>
> Original-author: Tony Luck <tony...@intel.com>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> Acked-by: Naveen N. Rao <naveen...@linux.vnet.ibm.com>

Acked-by: Borislav Petkov <b...@suse.de>

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 16, 2013, 1:10:04 PM10/16/13
to
On Wed, Oct 16, 2013 at 10:56:01AM -0400, Chen, Gong wrote:
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index b3218cd..c11a53f 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -48,6 +48,8 @@
>
> #include "mce-internal.h"
>
> +static int (*mce_ext_err_print)(const char *, int, int) = NULL;

Applying: ACPI, x86: Extended error log driver for x86 platform
ERROR: do not initialise statics to 0 or NULL
#32: FILE: arch/x86/kernel/cpu/mcheck/mce.c:51:
+static int (*mce_ext_err_print)(const char *, int, int) = NULL;

> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 22327e6..819c06b 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -372,4 +372,13 @@ config ACPI_BGRT
>
> source "drivers/acpi/apei/Kconfig"
>
> +config ACPI_EXTLOG
> + tristate "Extended Error Log support"
> + depends on X86_MCE
> + default n
> + help
> + 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.

Yep, you can use the description from your 0/9 mail here.
So was this supposed to be 0xfffffffffffff, right?

Because that's bits 0 up to and including 51. In which case, it should
be GENMASK_ULL(51,0).

The rest are checkpatch minor issues below.
CHECK: braces {} should be used on all arms of this statement
#280: FILE: drivers/acpi/acpi_extlog.c:178:
+ if (obj->type == ACPI_TYPE_INTEGER)
[...]
+ else if (obj->type == ACPI_TYPE_BUFFER) {
[...]

> + if (obj->buffer.length <= 8) {
> + for (i = 0; i < obj->buffer.length; i++)
> + *ret |= (obj->buffer.pointer[i] << (i * 8));
> + }
> + }
> + kfree(buf.pointer);
> +
> + return 0;
> +}
> +
> +static bool extlog_get_l1addr(void)
> +{
> + acpi_handle handle;
> + u64 ret;
> +
> + if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> + return false;
> +
> + if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_QUERY, &ret) ||
> + !(ret & EXTLOG_QUERY_L1_EXIST))

CHECK: Alignment should match open parenthesis
#302: FILE: drivers/acpi/acpi_extlog.c:200:
+ if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_QUERY, &ret) ||
+ !(ret & EXTLOG_QUERY_L1_EXIST))

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Mauro Carvalho Chehab

unread,
Oct 16, 2013, 1:10:04 PM10/16/13
to
Em Wed, 16 Oct 2013 10:56:00 -0400
"Chen, Gong" <gong...@linux.intel.com> escreveu:

> GENMASK is used to create a contiguous bitmask([hi:lo]). It is
> implemented twice in current kernel. One is in EDAC driver, the other
> is in SiS/XGI FB driver. Move it to a more generic place for other
> usage.
>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> Cc: Borislav Petkov <b...@alien8.de>
> Cc: Thomas Winischhofer <tho...@winischhofer.net>
> Cc: Jean-Christophe Plagniol-Villard <plag...@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.va...@ti.com>

Acked-by: Mauro Carvalho Chehab <m.ch...@samsung.com>

Btw, there's another incarnation of it at sb_edac.c (GET_BITFIELD).
It could make sense to also replace it by the newly bitops.h macro.

Regards,
Mauro
Cheers,
Mauro

Borislav Petkov

unread,
Oct 16, 2013, 1:20:02 PM10/16/13
to
On Wed, Oct 16, 2013 at 10:56:04AM -0400, Chen, Gong wrote:
> After H/W error happens under FFM enabled mode, lots of information
> are shown but some important parts like DIMM location missed. This
> patch is used to show these extra fileds.
>
> Original-author: Tony Luck <tony...@intel.com>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> Acked-by: Naveen N. Rao <naveen...@linux.vnet.ibm.com>

Acked-by: Borislav Petkov <b...@suse.de>

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

unread,
Oct 16, 2013, 1:30:04 PM10/16/13
to
On Wed, Oct 16, 2013 at 10:56:05AM -0400, Chen, Gong wrote:
> Keep up only the most important fields for memory error
> reporting. The detail information will be moved to perf/trace
> interface.
>
> Suggested-by: Tony Luck <tony...@intel.com>
> Signed-off-by: Chen, Gong <gong...@linux.intel.com>
> ---
> drivers/acpi/apei/cper.c | 69 +++++++++++++++++++++++-------------------------
> 1 file changed, 33 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
> index b1a8a55..f5bc227 100644
> --- a/drivers/acpi/apei/cper.c
> +++ b/drivers/acpi/apei/cper.c
> @@ -33,6 +33,9 @@
> #include <linux/pci.h>
> #include <linux/aer.h>
>
> +#define INDENT_SP " "
> +#define HW_CE_INFO \
> + "Above error has been corrected by h/w and no further action required"

Leftover?


> + if (severity != CPER_SEV_FATAL)
> + printk("%s%s\n", pfx,
> + "Above error has been corrected by h/w "
> + "and require no further action");

Let's write it out and correct grammar:

"Above error has been corrected by the hardware and requires no further action."

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
It is loading more messages.
0 new messages