Add machine check recovery for instruction fetch

Section 15.3.1.2 of the software developer manual has this to say about the RIPV bit in the IA32_MCG_STATUS register:

RIPV (restart IP valid) flag, bit 0 — Indicates (when set) that program execution can be restarted reliably at the instruction pointed to by the instruction pointer pushed on the stack when the machine-check exception is generated. When clear, the program cannot be reliably restarted at the pushed instruction pointer.

We need to save the state of this bit in do_machine_check() and use it in mce_notify_process() to force a signal; even if memory_failure() says it made a complete recovery ... e.g. replaced a clean LRU page).

Signed-off-by Tony Luck <[email protected]> ---

arch/x86/kernel/cpu/mcheck/mce.c | 9 ++++++--- 1 files changed,
6 insertions(+), 3 deletions(-)

(quoted diff) +0 -0

b/arch/x86/kernel/cpu/mcheck/mce.c index 66e1c51..3b8ebdc 100644 --- a/arch/x86/kernel/cpu/mcheck/mce.c +++ b/arch/x86/kernel/cpu/mcheck/mce.c @@ -947,9 +947,10 @@ struct
mce_info { atomic_t inuse; struct task_struct *t; __u64 paddr; +
int restartable; } mce_info[MCE_INFO_MAX];

-static void mce_save_info(__u64 addr) +static void mce_save_info(__u64 addr, int c) { struct mce_info *mi;

(quoted diff) +0 -0

@@ -957,6 +958,7 @@ static void mce_save_info(__u64 addr) if
 (atomic_cmpxchg(&mi->inuse, 0, 1) == 0) { mi->t = current; 

mi->paddr = addr; + mi->restartable = c; return; } } @@ -1136,7
+1138,7 @@ void do_machine_check(struct pt_regs *regs, long error_code) mce_panic("Fatal machine check on current CPU", &m, msg); if (worst == MCE_AR_SEVERITY) { /* schedule action before
return to userland */ - mce_save_info(m.addr); +
mce_save_info(m.addr, m.mcgstatus & MCG_STATUS_RIPV); set_thread_flag(TIF_MCE_NOTIFY); } else if (kill_it) { force_sig(SIGBUS, current); @@ -1185,7 +1187,8 @@ void mce_notify_process(void)

pr_err("Uncorrected hardware memory error in user-access at %llx",
mi->paddr); - if (memory_failure(pfn, MCE_VECTOR,
MF_ACTION_REQUIRED) < 0) { + if (memory_failure(pfn, MCE_VECTOR,
MF_ACTION_REQUIRED) < 0 || + mi->restartable == 0) {
pr_err("Memory error not recovered"); force_sig(SIGBUS, current); }

How about using following condition to decrease the execution time? if (mi->restartable == 0 ||
memory_failure(pfn, MCE_VECTOR, MF_ACTION_REQUIRED) < 0)

Since restart operation is impossible, whether recovery operation can be avoided?

Section 15.3.1.2 of the software developer manual has this to say about the RIPV bit in the IA32_MCG_STATUS register:

RIPV (restart IP valid) flag, bit 0 — Indicates (when set) that program execution can be restarted reliably at the instruction pointed to by the instruction pointer pushed on the stack when the machine-check exception is generated. When clear, the program cannot be reliably restarted at the pushed instruction pointer.

We need to save the state of this bit in do_machine_check() and use it in mce_notify_process() to force a signal; even if memory_failure() says it made a complete recovery ... e.g. replaced a clean LRU page).

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +0 -0

index 66e1c51..3b8ebdc 100644

arch/x86/kernel/cpu/mcheck/mce.c struct mce_info { · line 950

Quoted 1 line

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

+	int			restartable;

Is it me or does this look like a flag, or a bitfield? Instead of wasting a whole integer for a single bit of information.

It will probably end up the same size though due to compiler padding since this struct is currently 4 + 2*8 byte without the ->restartable thing.

Quoted 4 lines

arch/x86/kernel/cpu/mcheck/mce.c struct mce_info { · line 950

Quoted 1 line

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

+	int			restartable;

Is it me or does this look like a flag, or a bitfield? Instead of wasting a whole integer for a single bit of information.

I could make it "int flags;" and "#define MCE_INFO_RESTARTABLE 1" to make it clear that we have lots more bits available for special cases?

Quoted 3 lines

It will probably end up the same size though due to compiler padding since this struct is currently 4 + 2*8 byte without the ->restartable thing.

Yup - we can't save any memory (unless we introduce more complexity to the code ... low PAGE_SHIFT bits of the "paddr" field are simply thrown away ... so we could allocate a bit there ... but I don't think that the resulting ugliness is worth the memory savings).

I was wondering about that but don't have any other flags/use cases. Well, we can always change it later if needed.

Quoted 2 lines · Borislav Petkov

I was wondering about that but don't have any other flags/use cases. Well, we can always change it later if needed.

So is that an Ack? I was thinking that I should send this part to Linus now ... since restarting from a fault when RIPV isn't set could result in an application doing something bad (like getting the wrong answer).

Instruction recovery cases are very similar to the data recovery one we already have. Just trade out for a new MCACOD value.

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce-severity.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce-severity.c +3 -0

index 0c82091..163c4364ca 100644
@@ -126,6 +126,16 @@ static struct severity {
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_DATA),
 		USER
 		),
+	MCESEV(
+		KEEP, "HT thread notices Action required: instruction fetch error",
+		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_INSTR),

For IFU, on affected logical processors, RIPV and EIPV both are 0, since now new IFU entries are added into severity table, the old entry as below should be removed:

/* Neither return not error IP -- no chance to recover -> PANIC */

       MCESEV(
               PANIC, "Neither restart nor error IP",
               MCGMASK(MCG_STATUS_RIPV|MCG_STATUS_EIPV, 0)
               ),

Quoted 9 lines

For IFU, on affected logical processors, RIPV and EIPV both are 0, since now new IFU entries are added into severity table, the old entry as below should be removed:

/* Neither return not error IP -- no chance to recover -> PANIC */

       MCESEV(
               PANIC, "Neither restart nor error IP",
               MCGMASK(MCG_STATUS_RIPV|MCG_STATUS_EIPV, 0)
               ),

We need to keep this. If EIPV is not set, then CS and IP on the stack are not guaranteed ... so we can't tell whether the error happened in user or kernel mode. This makes recovery "challenging".

I'm trying to figure out a quirk for processors that do generate EIPV=RIPV=0 signature for IFU errors. There are some case where we can work around the lack of EIPV.

Quoted 3 lines

I'm trying to figure out a quirk for processors that do generate EIPV=RIPV=0 signature for IFU errors. There are some case where we can work around the lack of EIPV.

__mcheck_cpu_apply_quirks?

Quoted 5 lines

Quoted 3 lines

I'm trying to figure out a quirk for processors that do generate EIPV=RIPV=0 signature for IFU errors. There are some case where we can work around the lack of EIPV.

__mcheck_cpu_apply_quirks?

The quirk is a bit more extensive than just setting some flag in there for "if (c->x86 == 6 && c->x86_model == 45)". Sometime EIPV isn't set because the regs->ip and regs->cs really aren't valid. So I'll need to do something like:

	if (we are on Intel family 6 model 45 AND
          this is a UC=1, PCC=0, AR=1, S=1, ADDRV=1, MCACOD=0x150 error AND
	    (virtophys(regs->ip) >> PAGE_SHIFT) == (MC(bank)_ADDR >> PAGE_SHIFT)) {
		/* ok to trust CS & IP */
		m.mcgstatus |= MCG_STATUS_EIPV;
	}

before running through the mce_severity() table lookup.

x86 doesn't seem to have a "virtophys()" that I can find. Closest is lookup_address(vaddr, level) which gets a pte (which is almost all the work).

Sigh!