RAS: Correctable Errors Collector thing

From: Borislav Petkov <[email protected]>

Hi all,

this is something Tony and I have been working on behind the curtains recently. Here it is in a RFC form, it passes quick testing in kvm. Let me send it out before I start hammering on it on a real machine.

More indepth info about what it is and what it does is in patch 1/3.

As always, comments and suggestions are most welcome.

Thanks.

Borislav Petkov (3):
MCE, CE: Corrected errors collecting thing
MCE, CE: Wire in the CE collector
MCE, CE: Add debugging glue

 arch/x86/include/asm/mce.h          |   4 +
 arch/x86/kernel/cpu/mcheck/Makefile |   2 +-
 arch/x86/kernel/cpu/mcheck/ce.c     | 305 ++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/mcheck/mce.c    |  93 ++++++++++-
 4 files changed, 395 insertions(+), 9 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/mcheck/ce.c

1.9.0

From: Borislav Petkov <[email protected]>

A simple data structure for collecting correctable errors.

---
 arch/x86/include/asm/mce.h          |   4 +
 arch/x86/kernel/cpu/mcheck/Makefile |   2 +-
 arch/x86/kernel/cpu/mcheck/ce.c     | 281 ++++++++++++++++++++++++++++++++++++
 3 files changed, 286 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/kernel/cpu/mcheck/ce.c

index 6e4ce2df87cf..b4581392df66 100644
@@ -247,4 +247,8 @@ struct cper_sec_mem_err;
 extern void apei_mce_report_mem_error(int corrected,
 				      struct cper_sec_mem_err *mem_err);
 
+void __init ce_init(void);
+int ce_add_elem(u64 pfn);
+u64 ce_del_lru_elem(void);
+
 #endif /* _ASM_X86_MCE_H */

index bb34b03af252..97f03043a674 100644
@@ -1,4 +1,4 @@
-obj-y				=  mce.o mce-severity.o
+obj-y				=  mce.o mce-severity.o ce.o
 
 obj-$(CONFIG_X86_ANCIENT_MCE)	+= winchip.o p5.o
 obj-$(CONFIG_X86_MCE_INTEL)	+= mce_intel.o

1.9.0

From: Borislav Petkov <[email protected]>

Add the CE collector to the polling path which collects the correctable errors. Collect only DRAM ECC errors for now.

---
 arch/x86/kernel/cpu/mcheck/mce.c | 62 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 7 deletions(-)

index 882e79bb0cb6..732fbe61416d 100644
@@ -579,6 +579,46 @@ static void mce_read_aux(struct mce *m, int i)
 
 DEFINE_PER_CPU(unsigned, mce_poll_count);
 
+static bool is_a_memory_error(struct mce *m)
+{
+	struct cpuinfo_x86 *c = &boot_cpu_data;
+
+	if (c->x86_vendor == X86_VENDOR_AMD) {
+		/* ErrCodeExt[20:16] */
+		u8 xec = (m->status >> 16) & 0x1f;
+
+		return (xec == 0x0 || xec == 0x8);
+	} else if (c->x86_vendor == X86_VENDOR_INTEL)
+		return m->status & BIT(7);
+	else
+		return false;
+}
+
+static void __log_ce(struct mce *m, enum mcp_flags flags)
+{
+	int ret;
+
+	/*
+	 * Don't get the IP here because it's unlikely to have anything to do
+	 * with the actual error location.
+	 */
+	if ((flags & MCP_DONTLOG) || mca_cfg.dont_log_ce)
+		return;
+
+	if (is_a_memory_error(m)) {
+		ret = ce_add_elem(m->addr >> PAGE_SHIFT);
+		if (ret < 0) {
+			u64 pfn = ce_del_lru_elem();
+			if (pfn)
+				mce_ring_add(pfn);
+
+			ce_add_elem(m->addr >> PAGE_SHIFT);
+		}
+	} else
+		mce_log(m);
+}
+
+
 /*
  * Poll for corrected events or events that happened before reset.
  * Those are just logged through /dev/mcelog.

@@ -632,12 +672,8 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 
 		if (!(flags & MCP_TIMESTAMP))
 			m.tsc = 0;
-		/*
-		 * Don't get the IP here because it's unlikely to
-		 * have anything to do with the actual error location.
-		 */
-		if (!(flags & MCP_DONTLOG) && !mca_cfg.dont_log_ce)
-			mce_log(&m);
+
+		__log_ce(&m, flags);
 
 		/*
 		 * Clear state for this bank.

@@ -2530,5 +2566,17 @@ static int __init mcheck_debugfs_init(void)
 
 	return 0;
 }
-late_initcall(mcheck_debugfs_init);
+#else
+static int __init mcheck_debugfs_init(void) {}
 #endif
+
+static int __init mcheck_late_init(void)
+{
+	if (mcheck_debugfs_init())
+		pr_err("Error creating debugfs nodes!\n");
+
+	ce_init();
+
+	return 0;
+}
+late_initcall(mcheck_late_init);

1.9.0

Thanks.

Right, this still needs a decision on how the collector fits in the whole picture - this patch was just the bare minimum to get the discussion going.

The question is: do we want to tell userspace about every CE? Since we're doing everything ourselves in the kernel, I'd probably not upset needlessly but only when we've disabled a memory page.

Concerning EXTLOG, shouldn't that be firmware-first anyway? If so, we won't have any control over it anyway.

Concerning EDAC, we would want it to go first anyway, in order to do some decoding and *then* feed the final info to the collector.

In both cases, we probably should have EDAC/EXTLOG run first so that we can have all the relevant error info, put into the collector and maybe then mce_log a subset of those errors. Let me draw a bit:

A
[ machine_check_poll ] -----> [ EDAC/EXTLOG ] ---+----> [ CE Collector ] .-.>.-. [ mce_log ]

						 |
						 |
						 +----> [ mce_log ]

This is how the pipes could look like :-)

The split at A is to denote that only a subset of the errors should go into the collector, i.e. ECC errors. After it, we want to decide what is going to go to the log.

Maybe even add the ability to turn off the collector in the kernel if a userspace agent runs. Which would not be such a good idea as the kernel one should suffice for most if not all cases.

Anyway, this should get us going towards hammering out the piping.

Thanks.

Regards/Gruss,
    Boris.

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

From: Borislav Petkov <[email protected]>

For testing purposes only.

---
 arch/x86/kernel/cpu/mcheck/ce.c  | 24 ++++++++++++++++++++++++
 arch/x86/kernel/cpu/mcheck/mce.c | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

index 3020415f27f4..aa2fee325588 100644
@@ -94,6 +94,25 @@ static struct ce_array {
 
 static DEFINE_MUTEX(ce_lock);
 
+static void dump_array(struct ce_array *ca)
+{
+	u64 prev = 0;
+	int i;
+
+	pr_info("{ n: %d\n", ca->n);
+	for (i = 0; i < ca->n; i++) {
+		u64 this = PFN(ca->array[i]);
+
+		pr_info(" %03d: [%016llu|%03llx]\n", i, this, FULL_COUNT(ca->array[i]));
+
+		WARN_ON(prev > this);
+
+		prev = this;
+	}
+
+	pr_info("}\n");
+}
+
 /*
  * Decrement decay value. We're using DECAY_BITS bits to denote decay of an
  * element in the array. On insertion and any access, it gets maxed

@@ -202,6 +221,9 @@ u64 ce_del_lru_elem(void)
 
 	pfn = PFN(ca->array[min_idx]);
 
+	pr_err("%s: Deleting ca[%d]: %016llu|%03x\n",
+	       __func__, min_idx, pfn, min);
+
 	__del_elem(ca, min_idx);
 
 	mutex_unlock(&ce_lock);

@@ -263,7 +285,9 @@ int ce_add_elem(u64 pfn)
 		ret = 0;
 	}
 
+
 unlock:
+	dump_array(ca);
 	mutex_unlock(&ce_lock);
 
 	return ret;

index 732fbe61416d..59b726d4e116 100644
@@ -2552,9 +2552,34 @@ static int fake_panic_set(void *data, u64 val)
 DEFINE_SIMPLE_ATTRIBUTE(fake_panic_fops, fake_panic_get,
 			fake_panic_set, "%llu\n");
 
+static u64 cec_pfn;
+
+static int cec_pfn_get(void *data, u64 *val)
+{
+	*val = cec_pfn;
+	return 0;
+}
+
+static int cec_pfn_set(void *data, u64 val)
+{
+	int ret;
+
+	cec_pfn = val;
+
+	ret = ce_add_elem(val);
+	if (ret < 0) {
+		ce_del_lru_elem();
+		ret = ce_add_elem(val);
+	}
+
+	return ret;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(cec_pfn_ops, cec_pfn_get, cec_pfn_set, "0x%llx\n");
+
 static int __init mcheck_debugfs_init(void)
 {
-	struct dentry *dmce, *ffake_panic;
+	struct dentry *dmce, *ffake_panic, *cec_pfn;
 
 	dmce = mce_get_debugfs_dir();
 	if (!dmce)

@@ -2564,6 +2589,10 @@ static int __init mcheck_debugfs_init(void)
 	if (!ffake_panic)
 		return -ENOMEM;
 
+	cec_pfn = debugfs_create_file("cec_pfn", 0400, dmce, NULL, &cec_pfn_ops);
+	if (!cec_pfn)
+		return -ENOMEM;
+
 	return 0;
 }
 #else

1.9.0

What's the point of this patch set?
My understanding is that if there are some(COUNT_MASK) corrected DRAM ECC errors for a specific page frame, we can believe that the page frame is so ill that it should be isolated as soon as possible.

The question is: memory_failure can not be used for isolating the page frame which is being used by kernel, because it just poison the page and IGNORED. memory_failure is mostly used for handling AR/AO type errors related to the page frame which the userspace tasks are using now.

Although the relative page frame is very ill, it is not dead and can still work. However, memory_failure may kill the userspace tasks, especially for those page frames that are holding dynamic data rather than file-backed(file/swap) data.

So I do not think that it is a good idea to directly use memory_failure in this patch set.

thx!
cyc

I second that. You can't poison a page and potentially kill an application just because an arbitrarily chosen number of corrected errors has been exceeded. That would be an anti-RAS feature: less reliability and availability.
A possible alternative would be to soft-offline the page. This is currently done in APEI code when corrected memory error thresholds are exceeded and reported by UEFI via a generic hardware error source (GHES).
The example is in ghes_handle_memory_failure() where we call memory_failure_queue(pfn, 0, flags) with flags = MF_SOFT_OFFLINE

- Max

+1

This is what the existing mcelog(8) daemon does when it sees an excessive number of corrected errors on a page (using /sys/devices/system/memory/soft_offline_page as the user->kernel interface to get to this function).

-Tony ���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m����������?�I�

Ok, we can try that - it certainly makes sense. We're designing the plumbing of this thing as we go so fitting it correctly into the whole reporting/error handling chain is still open. It is RFC anyway so...

Thanks.

Regards/Gruss,
    Boris.

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