Fix machine check recovery for instruction fault on Sandy Bridge
cover Fix machine check recovery for instruction fault on Sandy Bridge
This patch series adds a workaround for some strange asymmetry between how machine checks are reported for data and instruction fetches. For instruction fetch error the processor does not set the EIPV bit in the MCG_STATUS register on the affected processor, leading us to believe that the cs/ip values saved on the stack are not associated with the machine check ... which in turn makes us unable to determine whether the machine check was taken in kernel or user mode. The workaround is to fake the presence of the EIPV bit for this error on this processor model. Not pretty, but avoids having to make special cases later in the code.
Tony Luck (2):
x86/mce: Move MCACOD defines from mce-severity.c to <asm/mce.h> x86/mce: Add quirk for instruction recovery on Sandy Bridge
processors
arch/x86/include/asm/mce.h | 8 ++++++ arch/x86/kernel/cpu/mcheck/mce-severity.c | 7 ----- arch/x86/kernel/cpu/mcheck/mce.c | 43 ++++++++++++++++++++++++++++--- 3 files changed, 48 insertions(+), 10 deletions(-)
Signature · 1 line
1.7.10.2.552.gaa3bb87
PATCH 1/2 x86/mce: Move MCACOD defines from mce-severity.c to <asm/mce.h>
We will need some of these values in mce.c. Move them to the appropriate header file so they are available.
--- arch/x86/include/asm/mce.h | 8 ++++++++ arch/x86/kernel/cpu/mcheck/mce-severity.c | 7 ------- 2 files changed, 8 insertions(+), 7 deletions(-)
arch/x86/include/asm/mce.h +8 -0
@@ -33,6 +33,14 @@#define MCI_STATUS_PCC (1ULL<<57)/* processor context corrupt */ #define MCI_STATUS_S (1ULL<<56)/* Signaled machine check */ #define MCI_STATUS_AR (1ULL<<55)/* Action required */ +#define MCACOD 0xffff/* MCA Error Code */ + +/* Architecturally defined codes from SDM Vol. 3B Chapter 15 */ +#define MCACOD_SCRUB 0x00C0/* 0xC0-0xCF Memory Scrubbing */ +#define MCACOD_SCRUBMSK 0xfff0 +#define MCACOD_L3WB 0x017A/* L3 Explicit Writeback */ +#define MCACOD_DATA 0x0134/* Data Load */ +#define MCACOD_INSTR 0x0150/* Instruction Fetch */ /* MCi_MISC register defines */ #define MCI_MISC_ADDR_LSB (m) ((m) & 0x3f)
arch/x86/kernel/cpu/mcheck/mce-severity.c +0 -7
@@ -55,13 +55,6 @@ static struct severity {#define MCI_UC_S (MCI_STATUS_UC|MCI_STATUS_S)#define MCI_UC_SAR (MCI_STATUS_UC|MCI_STATUS_S|MCI_STATUS_AR)#define MCI_ADDR (MCI_STATUS_ADDRV|MCI_STATUS_MISCV) -#define MCACOD 0xffff -/* Architecturally defined codes from SDM Vol. 3B Chapter 15 */ -#define MCACOD_SCRUB 0x00C0/* 0xC0-0xCF Memory Scrubbing */ -#define MCACOD_SCRUBMSK 0xfff0 -#define MCACOD_L3WB 0x017A/* L3 Explicit Writeback */ -#define MCACOD_DATA 0x0134/* Data Load */ -#define MCACOD_INSTR 0x0150/* Instruction Fetch */ MCESEV( NO, "Invalid",
Signature · 1 line
1.7.10.2.552.gaa3bb87
PATCH 2/2 x86/mce: Add quirk for instruction recovery on Sandy Bridge processors
In reply to Tony LuckSandy Bridge processors follow the SDM (Vol 3B, Table 15-20) and set both the RIPV and EIPV bits in the MCG_STATUS register to zero for machine checks during instruction fetch. This is more than a little counter-intuitive and means that Linux cannot recover from these errors. Rather than insert special case code at several places in mce.c and mce-severity.c, we pretend the EIPV bit was set for just this case early in processing the machine check.
--- arch/x86/kernel/cpu/mcheck/mce.c | 43 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/mcheck/mce.c +40 -3
@@ -102,6 +102,8 @@ DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = { static DEFINE_PER_CPU(struct work_struct, mce_work); +static void (*quirk_no_way_out)(int bank,struct mce *m,struct pt_regs *regs); +/* * CPU/chipset specific EDAC code can register a notifier call here to print * MCE errors in a human-readable form.
@@ -649,14 +651,18 @@ EXPORT_SYMBOL_GPL(machine_check_poll);* Do a quick check if any of the events requires a panic. * This decides if we keep the events around or clear them. */ -static int mce_no_way_out (struct mce *m,char **msg,unsigned long *validp) +static int mce_no_way_out (struct mce *m,char **msg,unsigned long *validp, +struct pt_regs *regs) {int i, ret =0 ;for (i =0 ; i < banks; i++) { m->status =mce_rdmsrl (MSR_IA32_MCx_STATUS (i)); -if (m->status &MCI_STATUS_VAL ) +if (m->status &MCI_STATUS_VAL ) {__set_bit (i, validp); +if (quirk_no_way_out) +quirk_no_way_out (i, m, regs); + }if (mce_severity (m, tolerant, msg) >=MCE_PANIC_SEVERITY ) ret =1 ; }
@@ -1039,7 +1045,7 @@ void do_machine_check(struct pt_regs *regs, long error_code) *final = m;memset (valid_banks,0 ,sizeof (valid_banks)); - no_way_out =mce_no_way_out (&m, &msg, valid_banks); + no_way_out =mce_no_way_out (&m, &msg, valid_banks, regs);barrier ();
@@ -1415,6 +1421,34 @@ static void __mcheck_cpu_init_generic(void) } } +/* +* During IFU recovery Sandy Bridge -EP4S processors set the RIPV and +* EIPV bits in MCG_STATUS to zero on the affected logical processor (SDM +* Vol 3B Table 15-20). But this confuses both the code that determines +* whether the machine check occurred in kernel or user mode, and also +* the severity assessment code. Pretend that EIPV was set, and take the +* ip/cs values from the pt_regs that mce_gather_info() ignored earlier. +*/ +static void quirk_sandybridge_ifu (int bank,struct mce *m,struct pt_regs *regs) +{ +if (bank !=0 ) +return ; +if ((m->mcgstatus & (MCG_STATUS_EIPV |MCG_STATUS_RIPV )) !=0 ) +return ; +if ((m->status & (MCI_STATUS_OVER |MCI_STATUS_UC | +MCI_STATUS_EN |MCI_STATUS_MISCV |MCI_STATUS_ADDRV | +MCI_STATUS_PCC |MCI_STATUS_S |MCI_STATUS_AR | +MCACOD )) != + (MCI_STATUS_UC |MCI_STATUS_EN | +MCI_STATUS_MISCV |MCI_STATUS_ADDRV |MCI_STATUS_S | +MCI_STATUS_AR |MCACOD_INSTR )) +return ; + + m->mcgstatus |=MCG_STATUS_EIPV ; + m->ip = regs->ip; + m->cs = regs->cs; +} +/* Add per CPU specific workarounds here */ static int __cpuinit __mcheck_cpu_apply_quirks (struct cpuinfo_x86 *c) {
@@ -1512,6 +1546,9 @@ static int __cpuinit __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)*/ if (c->x86 ==6 && c->x86_model <=13 && mce_bootlog <0 ) mce_bootlog =0 ; + +if (c->x86 ==6 && c->x86_model ==45 ) + quirk_no_way_out = quirk_sandybridge_ifu; }if (monarch_timeout <0 ) monarch_timeout =0 ;
Signature · 1 line
1.7.10.2.552.gaa3bb87
Re: [PATCH 2/2] x86/mce: Add quirk for instruction recovery on Sandy Bridge processors
Quoted 9 lines · Tony Luck
Sandy Bridge processors follow the SDM (Vol 3B, Table 15-20) and set both the RIPV and EIPV bits in the
MCG_STATUSregister to zero for machine checks during instruction fetch. This is more than a little counter-intuitive and means that Linux cannot recover from these errors. Rather than insert special case code at several places inmce.candmce-severity.c, we pretend the EIPV bit was set for just this case early in processing the machine check.Signed-off-by Tony Luck <[email protected]>
Looks ok, just minor nitpick below.
Quoted 24 lines
--- arch/x86/kernel/cpu/mcheck/mce.c | 43 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-)arch/x86/kernel/cpu/mcheck/mce.c +4
-1@@ -102,6 +102,8 @@ DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = { static DEFINE_PER_CPU(struct work_struct, mce_work); +static void (*quirk_no_way_out)(int bank,struct mce *m,struct pt_regs *regs); +/* * CPU/chipset specific EDAC code can register a notifier call here to print * MCE errors in a human-readable form. @@ -649,14 +651,18 @@ EXPORT_SYMBOL_GPL(machine_check_poll);* Do a quick check if any of the events requires a panic. * This decides if we keep the events around or clear them. */ -static int mce_no_way_out (struct mce *m,char **msg,unsigned long *validp) +static int mce_no_way_out (struct mce *m,char **msg,unsigned long *validp, +struct pt_regs *regs)
arch/x86/kernel/cpu/mcheck/mce.c EXPORT_SYMBOL_GPL(machine_check_poll); · line 664
Quoted 10 lines
arch/x86/kernel/cpu/mcheck/mce.c +3
-1{int i, ret =0 ;for (i =0 ; i < banks; i++) { m->status =mce_rdmsrl (MSR_IA32_MCx_STATUS (i)); -if (m->status &MCI_STATUS_VAL ) +if (m->status &MCI_STATUS_VAL ) {__set_bit (i, validp); +if (quirk_no_way_out) +quirk_no_way_out (i, m, regs);
Maybe define a default empty quirk_no_way_out() on the remaining families/vendors so that the compiler can optimize it away and we save ourselves the if-test?
Signature · 8 lines
Regards/Gruss, Boris. Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach GM: Alberto Bozzo Reg: Dornach, Landkreis Muenchen HRB Nr. 43632 WEEE Registernr: 129 19551
Re: [PATCH 2/2] x86/mce: Add quirk for instruction recovery on Sandy Bridge processors
Quoted 3 lines
Maybe define a default empty
quirk_no_way_out()on the remaining families/vendors so that the compiler can optimize it away and we save ourselves the if-test?
Perhaps I misunderstood your suggestion. I don't see how the compiler will manage to optimize it all away. I just tried defining
static void quirk_no_way_out_nop(int bank, struct mce *m, struct pt_regs *regs) {
}
and providing that as an initial value for the quirk_no_way_out function pointer.
Then I deleted the "if (quirk_no_way_out)".
Looking at the assembly code produced, I now just have an unconditional call:
callq *0x9fe992(%rip) # ffffffff81a18668 <quirk_no_way_out>
I'd think that a call through a function pointer to an empty function is more expensive that testing whether that function pointer was NULL.
-Tony
Re: [PATCH 2/2] x86/mce: Add quirk for instruction recovery on Sandy Bridge processors
Quoted 16 lines · Tony Luck
Quoted 3 lines
Maybe define a default empty
quirk_no_way_out()on the remaining families/vendors so that the compiler can optimize it away and we save ourselves the if-test?Perhaps I misunderstood your suggestion. I don't see how the compiler will manage to optimize it all away. I just tried defining
static void
quirk_no_way_out_nop(int bank, struct mce *m, struct pt_regs *regs){
}and providing that as an initial value for the
quirk_no_way_outfunction pointer.Then I deleted the "if (
quirk_no_way_out)".
Quoted 7 lines
Looking at the assembly code produced, I now just have an unconditional call:
callq *0x9fe992(%rip) #
ffffffff81a18668<quirk_no_way_out>I'd think that a call through a function pointer to an empty function is more expensive that testing whether that function pointer was NULL.
Agreed.
Right, so I was thinking about something along the lines of what tglx did for the CMCI poll timer: http://lkml.org/lkml/2012/7/18/79 and I've been playing with this for a bit now and am seeing the same thing as you: obviously the compiler cannot optimize away an unconditional function call to an empty function *through* a function pointer (tried gcc 4.6 and 4.7). Maybe because it is a function pointer we're calling...
If you make the function pointer const like this:
static void default_quirk_no_way_out(int bank, struct mce *m,
struct pt_regs *regs) { }static void (* const quirk_no_way_out)(int bank, struct mce *m, struct pt_regs *regs) =
default_quirk_no_way_out;
The "call... " is not issued but you cannot assign to it later in __mcheck_cpu_apply_quirks().
Other things which could probably be used are alternatives or jump labels but one if-test is simply not worth the complexity.
Oh well, enough games.
Thanks.
Signature · 8 lines
Regards/Gruss, Boris. Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach GM: Alberto Bozzo Reg: Dornach, Landkreis Muenchen HRB Nr. 43632 WEEE Registernr: 129 19551
RE: [PATCH 2/2] x86/mce: Add quirk for instruction recovery on Sandy Bridge processors
Quoted 2 lines
Other things which could probably be used are alternatives or jump labels but one if-test is simply not worth the complexity.
It might be if this were a super-hot path. But if you are getting so many machine checks that you can see the effect of one extra "if" ... then you are hurting in so many other ways!
Quoted 1 line
Oh well, enough games.
Thanks for the review.
-Tony