Re: [PATCH 10/10] MCE: Add Action-Required support

Hidetoshi Seto
2011-06-10T08:09:40Z

(2011/06/10 6:38), Luck, Tony wrote:
> From: Tony Luck <tony.luck@intel.com>
>
> Implement core MCA recovery. This is used for errors
> that happen in the current execution context.
>
> The kernel has to first pass the error information
> to a function running on the current process stack.
> This is done using task_return_notifier_register().
>
> Just handle errors in user mode for now. Later we
> may be able to handle some kernel cases (e.g. when
> kernel is in copy_*_user())
>
> Based on some original code by Andi Kleen.
>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> arch/x86/kernel/cpu/mcheck/mce-severity.c | 35 +++++++-
> arch/x86/kernel/cpu/mcheck/mce.c | 118 +++++++++++++++++++++++++++--
> 2 files changed, 142 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce-severity.c b/arch/x86/kernel/cpu/mcheck/mce-severity.c
> index 352d16a..fe8a28c 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce-severity.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce-severity.c
> @@ -13,6 +13,7 @@
> #include <linux/seq_file.h>
> #include <linux/init.h>
> #include <linux/debugfs.h>
> +#include <linux/module.h>
> #include <asm/mce.h>
>
> #include "mce-internal.h"
> @@ -54,6 +55,9 @@ static struct severity {
> { .mcgmask = x, .mcgres = res, SEV(s), .msg = m, ## r }
> #define MASK(x, y, s, m, r...) \
> { .mask = x, .result = y, SEV(s), .msg = m, ## r }
> +#define ARMASK(x, y, s, m, r...) \
> + { .mcgmask = MCG_STATUS_RIPV, .mcgres = 0, \
> + .mask = x, .result = y, SEV(s), .msg = m, ## r }
> #define MCI_UC_S (MCI_STATUS_UC|MCI_STATUS_S)
> #define MCI_UC_SAR (MCI_STATUS_UC|MCI_STATUS_S|MCI_STATUS_AR)
> #define MCACOD 0xffff
> @@ -67,7 +71,7 @@ static struct severity {
> MCGMASK(MCG_STATUS_RIPV|MCG_STATUS_EIPV, 0, PANIC,
> "Neither restart nor error IP"),
> MCGMASK(MCG_STATUS_RIPV, 0, PANIC, "In kernel and no restart IP",
> - KERNEL),
> + KERNEL, NOSER),
> BITCLR(MCI_STATUS_UC, KEEP, "Corrected error", NOSER),
>
> /* ignore OVER for UCNA */
> @@ -77,10 +81,16 @@ static struct severity {
> "Illegal combination (UCNA with AR=1)", SER),
> MASK(MCI_STATUS_S, 0, KEEP, "Non signalled machine check", SER),
>
> - /* AR add known MCACODs here */
> MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_STATUS_OVER|MCI_UC_SAR, PANIC,
> "Action required with lost events", SER),
> - MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR, PANIC,
> +
> + /* known AR MCACODs: */
> + ARMASK(MCI_UC_SAR|MCI_STATUS_OVER|0xffff, MCI_UC_SAR|0x134, AR,
> + "Action required: data load error", SER),
> + ARMASK(MCI_UC_SAR|MCI_STATUS_OVER|0xffff, MCI_UC_SAR|0x150, AR,
> + "Action required: instruction fetch error", SER),
> +
> + ARMASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR, PANIC,
> "Action required; unknown MCACOD", SER),
>
> /* known AO MCACODs: */
> @@ -89,6 +99,7 @@ static struct severity {
> MASK(MCI_UC_SAR|MCI_STATUS_OVER|MCACOD, MCI_UC_S|0x17a, AO,
> "Action optional: last level cache writeback error", SER),
>
> +
> MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_S, SOME,
> "Action optional unknown MCACOD", SER),
> MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_S|MCI_STATUS_OVER, SOME,
> @@ -110,6 +121,17 @@ static int error_context(struct mce *m)
> return IN_KERNEL;
> }
>
> +static int kernel_ar_recoverable(struct mce *m, int tolerant)
> +{
> + if (tolerant >= 2)
> + return MCE_AR_SEVERITY;
> + if (!(m->mcgstatus & MCG_STATUS_EIPV) || !m->ip)
> + return MCE_PANIC_SEVERITY;
> + if (search_exception_tables(m->ip))
> + return MCE_AR_SEVERITY;
> + return MCE_PANIC_SEVERITY;
> +}
> +

You said "Just handle errors in user mode for now." but ...?

> int mce_severity(struct mce *a, int tolerant, char **msg)
> {
> enum context ctx = error_context(a);
> @@ -129,9 +151,12 @@ int mce_severity(struct mce *a, int tolerant, char **msg)
> if (msg)
> *msg = s->msg;
> s->covered = 1;
> - if (s->sev >= MCE_UC_SEVERITY && ctx == IN_KERNEL) {
> - if (panic_on_oops || tolerant < 1)
> + if (ctx == IN_KERNEL) {
> + if (s->sev >= MCE_UC_SEVERITY &&
> + (panic_on_oops || tolerant < 1))
> return MCE_PANIC_SEVERITY;
> + if (s->sev == MCE_AR_SEVERITY)
> + return kernel_ar_recoverable(a, tolerant);
> }
> return s->sev;
> }
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 9c72245..a7a8c53 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -80,6 +80,20 @@ static void mce_do_notify(struct user_return_notifier *urn);
> static DEFINE_PER_CPU(struct mce_notify, mce_notify);
>
> /*
> + * Task return notifiers are used for "action required"
> + * recovery of tasks - i.e. we prevent return to the task
> + * that encountered the machine check, but we ensure that
> + * we process the error in task context.
> + */
> +struct task_notify {
> + struct user_return_notifier urn;
> + unsigned long pfn;
> + atomic_t inuse;
> +};
> +static struct task_notify task_notifier[NR_CPUS];
> +static void mce_do_task(struct user_return_notifier *urn);
> +
> +/*
> * Tolerant levels:
> * 0: always panic on uncorrected errors, log corrected errors
> * 1: panic or SIGBUS on uncorrected errors, log corrected errors
> @@ -975,6 +989,84 @@ static void mce_clear_state(unsigned long *toclear)
> }
> }
>
> +/* Stub when hwpoison is not compiled in */
> +int __attribute__((weak)) __memory_failure(unsigned long pfn, int vector,
> + int precount)
> +{
> + return -1;
> +}
> +
> +/*
> + * Uncorrected error for current process.
> + */
> +static void mce_action_required(struct mce *m, char *msg, struct pt_regs *regs)
> +{
> + int i;
> +
> + if (!mce_usable_address(m))
> + mce_panic("No address for Action-Required Machine Check",
> + m, msg);
> + if (!(m->mcgstatus & MCG_STATUS_EIPV))
> + mce_panic("No EIPV for Action-Required Machine Check",
> + m, msg);

When can this happen?
Why not create new severity {PANIC, "Action Required: but No EIPV", ...}
in severity table?

> +
> + for (i = 0; i < NR_CPUS; i++)
> + if (!atomic_cmpxchg(&task_notifier[i].inuse, 0, 1))
> + break;
> + if (i == NR_CPUS)
> + mce_panic("Too many concurrent errors", m, msg);
> +
> + task_notifier[i].urn.on_user_return = mce_do_task;
> + task_notifier[i].pfn = m->addr >> PAGE_SHIFT;
> + task_return_notifier_register(&task_notifier[i].urn);
> +}
> +
> +#undef pr_fmt
> +#define pr_fmt(x) "MCE: %s:%d " x "\n", current->comm, current->pid
> +#define PADDR(x) ((u64)(x) << PAGE_SHIFT)
> +
> +/*
> + * No successfull recovery. Make sure at least that there's
> + * a SIGBUS.
> + */
> +static void ar_fallback(struct task_struct *me, unsigned long pfn)
> +{
> + if (signal_pending(me) && sigismember(&me->pending.signal, SIGBUS))
> + return;

Is it safe for _AR if SIGBUS is pending but blocked?
I think force_sig() is reasonable for such situation.

> +
> + /*
> + * For some reason hwpoison wasn't able to send a proper
> + * SIGBUS. Send a fallback signal. Unfortunately we don't
> + * know the virtual address here, so can't tell the program
> + * details.
> + */
> + force_sig(SIGBUS, me);
> + pr_err("Killed due to action-required memory corruption");
> +}
> +
> +/*
> + * Handle action-required on the process stack. hwpoison does the
> + * bulk of the work and with some luck might even be able to fix the
> + * problem.
> + *
> + * Logic changes here should be reflected in kernel_ar_recoverable().
> + */
> +static void handle_action_required(unsigned long pfn)
> +{
> + struct task_struct *me = current;
> +
> + pr_err("Uncorrected hardware memory error in user-access at %llx",
> + PADDR(pfn));
> + if (__memory_failure(pfn, MCE_VECTOR, 0) < 0) {
> + pr_err("Memory error not recovered");
> + ar_fallback(me, pfn);
> + } else
> + pr_err("Memory error recovered");
> +}
> +
> +#undef pr_fmt
> +#define pr_fmt(x) x
> +
> /*
> * The actual machine check handler. This only handles real
> * exceptions when something got corrupted coming in through int 18.
> @@ -1086,12 +1178,6 @@ void do_machine_check(struct pt_regs *regs, long error_code)
> continue;
> }
>
> - /*
> - * Kill on action required.
> - */
> - if (severity == MCE_AR_SEVERITY)
> - kill_it = 1;
> -
> mce_read_aux(&m, i);
>
> /*
> @@ -1136,6 +1222,15 @@ void do_machine_check(struct pt_regs *regs, long error_code)
> mce_panic("Fatal machine check on current CPU", &m, msg);
>
> /*
> + * Do recovery in current process if needed. This has to be delayed
> + * until we're back on the process stack.
> + */
> + if (worst == MCE_AR_SEVERITY) {
> + mce_action_required(&m, msg, regs);

Comprehensible name would be appreciated, e.g.:
mce_request_dpc_for_action_required(pfn);

And if we cannot request context for recovery, it is better to suppress
trailing attempts, e.g. before mce_end():

if (!no_way_out && severity == MCE_AR_SEVERITY) {
err = mce_request_dpc_for_action_required(pfn);
if (err) {
atomic_inc(&global_nwo);
severity = MCE_PANIC_SEVERITY; /* escalated */
}
}

Thanks,
H.Seto

> + kill_it = 0;
> + }
> +
> + /*
> * If the error seems to be unrecoverable, something should be
> * done. Try to kill as little as possible. If we can kill just
> * one task, do that. If the user has set the tolerance very
> @@ -1194,6 +1289,17 @@ static void mce_do_notify(struct user_return_notifier *urn)
> mce_process_ring();
> }
>
> +static void mce_do_task(struct user_return_notifier *urn)
> +{
> + struct task_notify *np = container_of(urn, struct task_notify, urn);
> + unsigned long pfn = np->pfn;
> +
> + task_return_notifier_unregister(urn);
> + atomic_set(&np->inuse, 0);
> +
> + handle_action_required(pfn);
> +}
> +
> static void mce_process_work(struct work_struct *dummy)
> {
> mce_process_ring();