From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752989AbbDFL6x (ORCPT ); Mon, 6 Apr 2015 07:58:53 -0400 Received: from mail.skyhub.de ([78.46.96.112]:58815 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751192AbbDFL6u (ORCPT ); Mon, 6 Apr 2015 07:58:50 -0400 Date: Mon, 6 Apr 2015 13:56:40 +0200 From: Borislav Petkov To: Naoya Horiguchi Cc: "Luck, Tony" , Prarit Bhargava , Vivek Goyal , "linux-kernel@vger.kernel.org" , Junichi Nomura , Kiyoshi Ueda Subject: Re: [PATCH v7] x86: mce: kexec: switch MCE handler for kexec/kdump Message-ID: <20150406115640.GC4078@pd.tnic> References: <20150305012447.GA16001@hori1.linux.bs1.fc.nec.co.jp> <20150305064509.GA16012@hori1.linux.bs1.fc.nec.co.jp> <20150305085735.GE3915@pd.tnic> <20150305093752.GA11764@hori1.linux.bs1.fc.nec.co.jp> <20150306025911.GA3619@hori1.linux.bs1.fc.nec.co.jp> <20150306083421.GD3514@pd.tnic> <20150306090930.GA14982@hori1.linux.bs1.fc.nec.co.jp> <20150306092738.GE3514@pd.tnic> <20150306093212.GB14982@hori1.linux.bs1.fc.nec.co.jp> <20150306102216.GA22787@hori1.linux.bs1.fc.nec.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150306102216.GA22787@hori1.linux.bs1.fc.nec.co.jp> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 06, 2015 at 10:22:16AM +0000, Naoya Horiguchi wrote: > kexec disables (or "shoots down") all CPUs other than a crashing CPU before > entering the 2nd kernel. But the MCE handler is still enabled after that, > so if MCE happens and broadcasts over the CPUs after the main thread starts > the 2nd kernel (which might not initialize MCE device yet, or might decide > not to enable it,) MCE handler runs only on the other CPUs (not on the main You lost me here: "MCE device"? > thread,) leading to kernel panic with MCE synchronization. The user-visible > effect of this bug is kdump failure. > > Our standard MCE handler do_machine_check() assumes some about system's > status and it's hard to alter it to cover kexec/kdump context, so let's add > another kdump-specific one and switch to it. > > Note that this problem exists since current MCE handler was implemented in Did we have kdump then at all and was that a "problem" then even? > 2.6.32, and recently commit 716079f66eac ("mce: Panic when a core has reached > a timeout") made it more visible by changing the default behavior of the > synchronization timeout from "ignore" to "panic". > > Signed-off-by: Naoya Horiguchi > --- > ChangeLog: v6 -> v7 > - use rdmsrl() instead of rdmsrl_safe() with mca_cfg.disabled check > > ChangeLog v5 -> v6: > - drop "CC stable" tag > - stop using/exporting mce_gather_info(), mce_(rd|wr)msrl(), and mce_panic() > - drop quirk_no_way_out() part, because quirk_sandybridge_ifu() (only possible > callback) could just change a MCE_PANIC_SEVERITY case to a MCE_AR_SEVERITY > case, which doesn't affect the panic/return decision. > > ChangeLog v4 -> v5: > - drop MCE_UC/AR_SEVERITY re-ordering > - move most of code to arch/x86/kernel/crash.c > - export some MCE internal variables/routines via arch/x86/include/asm/mce.h > > ChangeLog v3 -> v4: > - fixed AR and UC order in enum severity_level because UC is severer than AR > by definition. Current code is not affected by this wrong order by chance. > - check severity in machine_check_under_kdump(), and call mce_panic() if the > resultant severity is as bad as or worse than MCE_AR_SEVERITY. > - use static global variable kdump_cpu instead of mca_cfg->kdump_cpu > - reduce "#ifdef CONFIG_KEXEC" > - add "#ifdef CONFIG_X86_MCE" for declaration of machine_check_under_kdump() > in mce.h > - update comment on switch_mce_handler_for_kdump() > > ChangeLog v2 -> v3 > - go to "switch MCE handler" approach > > ChangeLog v1 -> v2 > - clear MSR_IA32_MCG_CTL, MSR_IA32_MCx_CTL, and CR4.MCE instead of using > global flag to ignore MCE events. > - fixed the description of the problem > --- > arch/x86/include/asm/mce.h | 14 +++++ > arch/x86/kernel/cpu/mcheck/mce-internal.h | 13 ----- > arch/x86/kernel/crash.c | 87 +++++++++++++++++++++++++++++++ > 3 files changed, 101 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h > index 51b26e895933..192267fcee73 100644 > --- a/arch/x86/include/asm/mce.h > +++ b/arch/x86/include/asm/mce.h > @@ -248,4 +248,18 @@ struct cper_sec_mem_err; > extern void apei_mce_report_mem_error(int corrected, > struct cper_sec_mem_err *mem_err); > > +enum severity_level { > + MCE_NO_SEVERITY, > + MCE_DEFERRED_SEVERITY, > + MCE_UCNA_SEVERITY = MCE_DEFERRED_SEVERITY, > + MCE_KEEP_SEVERITY, > + MCE_SOME_SEVERITY, > + MCE_AO_SEVERITY, > + MCE_UC_SEVERITY, > + MCE_AR_SEVERITY, > + MCE_PANIC_SEVERITY, > +}; > + > +int mce_severity(struct mce *a, int tolerant, char **msg, bool is_excp); > + > #endif /* _ASM_X86_MCE_H */ > diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h > index 10b46906767f..909ee3ed95dd 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce-internal.h > +++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h > @@ -1,18 +1,6 @@ > #include > #include > > -enum severity_level { > - MCE_NO_SEVERITY, > - MCE_DEFERRED_SEVERITY, > - MCE_UCNA_SEVERITY = MCE_DEFERRED_SEVERITY, > - MCE_KEEP_SEVERITY, > - MCE_SOME_SEVERITY, > - MCE_AO_SEVERITY, > - MCE_UC_SEVERITY, > - MCE_AR_SEVERITY, > - MCE_PANIC_SEVERITY, > -}; > - > #define ATTR_LEN 16 > > /* One object for each MCE bank, shared by all CPUs */ > @@ -23,7 +11,6 @@ struct mce_bank { > char attrname[ATTR_LEN]; /* attribute name */ > }; > > -int mce_severity(struct mce *a, int tolerant, char **msg, bool is_excp); > struct dentry *mce_get_debugfs_dir(void); > > extern struct mce_bank *mce_banks; > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c > index 6f3baedcb6f6..4a46654091bb 100644 > --- a/arch/x86/kernel/crash.c > +++ b/arch/x86/kernel/crash.c > @@ -34,6 +34,7 @@ > #include > #include > #include > +#include > > /* Alignment required for elf header segment */ > #define ELF_CORE_HEADER_ALIGN 4096 > @@ -76,6 +77,90 @@ struct crash_memmap_data { > > int in_crash_kexec; > > +#ifdef CONFIG_X86_MCE > +static int kdump_cpu; > + > +/* > + * kdump-specific machine check handler > + * > + * When kexec/kdump is running, what the MCE handler is expected to do > + * changes depending on whether the CPU is running the main thread or not. > + * > + * The crashing CPU, controlling the whole system exclusively, should try to > + * get kdump as hard as possible even if an MCE happens concurrently, because > + * some types of MCEs (for example, uncorrected errors like SRAO,) > + * are not fatal or don't ruin reliablility of the kdump (consider that an > + * MCE can hit the other CPU, in which case corrupted data is never consumed.) > + * If an MCE critically breaks the kdump operation, we are unlucky so let's > + * accept the fate of whatever HW causes, hoping a dying message reaches admins. > + * > + * The other CPUs are supposed to be quiet during kexec/kdump, so after the > + * crashing CPU shot them down, they should not do anything except clearing > + * MCG_STATUS (without this the system is reset, which is undesirable.) > + * Note that this is also true after the crashing CPU enter the 2nd kernel. > + */ > +static void machine_check_under_kdump(struct pt_regs *regs, long error_code) > +{ > + struct mce m = {}; > + char *msg = NULL; > + char *nmsg = NULL; > + int i; > + int worst = 0; > + int severity; > + > + if (mca_cfg.disabled) > + return; > + if (!mca_cfg.banks) > + goto out; > + if (kdump_cpu != smp_processor_id()) > + goto clear_mcg_status; > + > + rdmsrl(MSR_IA32_MCG_STATUS, m.mcgstatus); > + if (regs && m.mcgstatus & (MCG_STATUS_RIPV|MCG_STATUS_EIPV)) > + m.cs = regs->cs; Newline here please. > + for (i = 0; i < mca_cfg.banks; i++) { > + rdmsrl(MSR_IA32_MCx_STATUS(i), m.status); > + severity = mce_severity(&m, mca_cfg.tolerant, &nmsg, true); MCE severity got reworked recently, please redo your patch against latest tip/master: In file included from arch/x86/kernel/cpu/mcheck/mce-severity.c:18:0: arch/x86/kernel/cpu/mcheck/mce-internal.h:15:14: error: ‘mce_severity’ redeclared as different kind of symbol extern int (*mce_severity)(struct mce *a, int tolerant, char **msg, bool is_excp); ^ In file included from arch/x86/kernel/cpu/mcheck/mce-severity.c:16:0: ./arch/x86/include/asm/mce.h:270:5: note: previous declaration of ‘mce_severity’ was here int mce_severity(struct mce *a, int tolerant, char **msg, bool is_excp); ^ arch/x86/kernel/cpu/mcheck/mce-severity.c:274:7: error: ‘mce_severity’ redeclared as different kind of symbol int (*mce_severity)(struct mce *m, int tolerant, char **msg, bool is_excp) = ^ In file included from arch/x86/kernel/cpu/mcheck/mce-severity.c:16:0: ./arch/x86/include/asm/mce.h:270:5: note: previous declaration of ‘mce_severity’ was here int mce_severity(struct mce *a, int tolerant, char **msg, bool is_excp); ^ make[4]: *** [arch/x86/kernel/cpu/mcheck/mce-severity.o] Error 1 make[4]: *** Waiting for unfinished jobs.... In file included from arch/x86/kernel/cpu/mcheck/mce.c:50:0: arch/x86/kernel/cpu/mcheck/mce-internal.h:15:14: error: ‘mce_severity’ redeclared as different kind of symbol extern int (*mce_severity)(struct mce *a, int tolerant, char **msg, bool is_excp); ^ In file included from arch/x86/kernel/cpu/mcheck/mce.c:47:0: ./arch/x86/include/asm/mce.h:270:5: note: previous declaration of ‘mce_severity’ was here int mce_severity(struct mce *a, int tolerant, char **msg, bool is_excp); ^ make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1 make[3]: *** [arch/x86/kernel/cpu/mcheck] Error 2 make[3]: *** Waiting for unfinished jobs.... make[2]: *** [arch/x86/kernel/cpu] Error 2 make[1]: *** [arch/x86/kernel] Error 2 make: *** [arch/x86] Error 2 make: *** Waiting for unfinished jobs.... > + if (severity > worst) { > + worst = severity; > + msg = nmsg; > + } > + } > + > + if (worst >= MCE_UC_SEVERITY) > + panic("unignorable machine check under kdumping: %s", msg); Just say: panic("kdump: Fatal machine check: %s", msg); > + > + pr_emerg("MCE triggered under kdumping. If you are lucky enough, you will have a kdump. Otherwise, this is a dying message.\n"); This message is also funny. How about: pr_emerg("kdump: Non-fatal MCE detected - kernel dump might be unreliable.\n"); > + > +clear_mcg_status: > + wrmsrl(MSR_IA32_MCG_STATUS, 0); > +out: > + sync_core(); > +} > + > +/* > + * Switch the MCE handler to kdump-specific one > + * > + * Standard MCE handler do_machine_check() is not designed for kexec/kdump > + * context, where we can't expect MCE recovering and logging to work fine > + * because the kernel might be unstable (all CPUs except one must be quiet > + * as possible.) "... all CPUs except one must be idle." > + * > + * Moreover, in such situation getting kdump is prior to doing something for ... " in such situations, getting a kernel dump is more important than handling MCEs..." > + * MCEs because what the users are really interested in is to find what caused find out > + * the crashing, not what caused the crashing to fail. ... the crash." Fullstop. > + * > + * So let's switch MCE handler to the one suitable for kexec/kdump situation. > + */ > +void switch_mce_handler_for_kdump(void) s/switch_mce_handler_for_kdump/kdump_setup_mce/ > +{ > + kdump_cpu = smp_processor_id(); > + machine_check_vector = machine_check_under_kdump; > +} > +#else > +static inline void switch_mce_handler_for_kdump(void) {} > +#endif > + > /* > * This is used to VMCLEAR all VMCSs loaded on the > * processor. And when loading kvm_intel module, the > @@ -166,6 +251,8 @@ void native_machine_crash_shutdown(struct pt_regs *regs) > /* The kernel is broken so disable interrupts */ > local_irq_disable(); > > + switch_mce_handler_for_kdump(); > + > kdump_nmi_shootdown_cpus(); > > /* > -- > 1.9.3 > -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. --