[PATCH 0/2] Fix machine check recovery for instruction fault on Sandy Bridge

[PATCH 0/2] Fix machine check recovery for instruction fault on Sandy Bridge

[Tony Luck]

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(-)

-- 
1.7.10.2.552.gaa3bb87

[PATCH 1/2] x86/mce: Move MCACOD defines from mce-severity.c to <asm/mce.h>

[Tony Luck]

We will need some of these values in mce.c. Move them to the
appropriate header file so they are available.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/mce.h                | 8 ++++++++
 arch/x86/kernel/cpu/mcheck/mce-severity.c | 7 -------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 441520e..a3ac52b 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -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)
diff --git a/arch/x86/kernel/cpu/mcheck/mce-severity.c b/arch/x86/kernel/cpu/mcheck/mce-severity.c
index 413c2ce..1301762 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-severity.c
+++ b/arch/x86/kernel/cpu/mcheck/mce-severity.c
@@ -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",
-- 
1.7.10.2.552.gaa3bb87

[PATCH 2/2] x86/mce: Add quirk for instruction recovery on Sandy Bridge processors

[Tony Luck]

Sandy 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.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 43 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index da27c5d..e65e738 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -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;
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH 2/2] x86/mce: Add quirk for instruction recovery on Sandy Bridge processors

[Borislav Petkov]

On Thu, Jul 19, 2012 at 11:28:46AM -0700, Tony Luck wrote:
> Sandy 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.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>

Looks ok, just minor nitpick below.

> ---
>  arch/x86/kernel/cpu/mcheck/mce.c | 43 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index da27c5d..e65e738 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -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);

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?

-- 
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

[Tony Luck]

> 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

[Borislav Petkov]

On Fri, Jul 20, 2012 at 09:33:23AM -0700, Tony Luck wrote:
> > 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.

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.

-- 
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

[Luck, Tony]

> 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!

> Oh well, enough games.

Thanks for the review.

-Tony

[PATCH 0/2] Fix machine check recovery for instruction fault on Sandy Bridge

[Tony Luck]

[Unchanged since last posted - except to add Boris' Acked-by
 since after further discussion his nitpick didn't warrant a
 change. Ready for x86/mce branch ... and if possible to
 move to Linus in this merge window]

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(-)

-- 
1.7.10.2.552.gaa3bb87

[PATCH 2/2] x86/mce: Add quirk for instruction recovery on Sandy Bridge processors

[Tony Luck]

Sandy 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.

Acked-by: Borislav Petkov <bp@amd64.org>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 43 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index da27c5d..e65e738 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -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;
-- 
1.7.10.2.552.gaa3bb87