OS_MCA Recovery from poisoned memory read

This is the body of OS_MCA_handler.

- init
mca_external_handler_init
init_record_index_pools
kmalloc

- exit
mca_external_handler_exit
kfree

- handler
mca_try_to_recover
mca_make_slidx : pick up pointers in SAL log record mca_make_peidx : pick up pointers in Processor section is_mca_global : treat only local MCA
recover_from_processor_error : treat only external bus error
recover_from_platform_error : treat only read error
recover_from_read_error : set new restarting address

- handler_bh
mca_handler_bhhook
mca_handler_bh
mca_page_isolate
force_sig(SIGKILL, current)

The MCA offending process will restart from handler_bh, and will suicide after isolating the poisoned page.

Thanks,
H.Seto

diff -Nur linux-2.6.8-rc3/arch/ia64/Kconfig linux-2.6.8-rc3-mcadrv-v2/arch/ia64/Kconfig
@@ -270,6 +270,9 @@
 	depends on IA32_SUPPORT
 	default y
 
+config IA64_MCA_RECOVERY
+	tristate "MCA recovery from errors other than TLB."
+
 config PERFMON
 	bool "Performance monitor support"
 	help

diff -Nur linux-2.6.8-rc3/arch/ia64/kernel/Makefile linux-2.6.8-rc3-mcadrv-v2/arch/ia64/kernel/Makefile
@@ -17,6 +17,8 @@
 obj-$(CONFIG_SMP)		+= smp.o smpboot.o
 obj-$(CONFIG_PERFMON)		+= perfmon_default_smpl.o
 obj-$(CONFIG_IA64_CYCLONE)	+= cyclone.o
+obj-$(CONFIG_IA64_MCA_RECOVERY)	+= mca_recovery.o
+mca_recovery-y			+= mca_drv.o mca_drv_asm.o
 
 # The gate DSO image is built using a special linker script.
 targets += gate.so gate-syms.o

diff -Nur linux-2.6.8-rc3/arch/ia64/kernel/mca_drv_asm.S linux-2.6.8-rc3-mcadrv-v2/arch/ia64/kernel/mca_drv_asm.S
@@ -0,0 +1,38 @@
+#include <linux/config.h>
+#include <linux/threads.h>
+
+#include <asm/asmmacro.h>
+#include <asm/processor.h>
+
+GLOBAL_ENTRY(mca_handler_bhhook)
+	invala						// clear RSE ?
+	;;						//
+	cover						// 
+	;;						//
+	clrrrb						//
+	;;						
+	alloc		r16=ar.pfs,0,2,1,0		// make a new frame
+	;;
+	mov		r13=IA64_KR(CURRENT)		// current task pointer
+	;;
+	adds		r12=IA64_TASK_THREAD_KSP_OFFSET,r13
+	;;
+	ld8		r12=[r12]			// stack pointer
+	;;
+	mov		loc0=r16
+	movl		loc1=mca_handler_bh		// recovery C function 
+	;;
+	mov		out0=r8				// poisoned address
+	mov		b6=loc1
+	;;
+	mov		loc1=rp
+	;;
+	br.call.sptk.many    rp=b6			// not return ... 
+	;;
+	mov		ar.pfs=loc0
+	mov 		rp=loc1
+	;;
+	mov		r8=r0
+	br.ret.sptk.many rp
+	;;
+END(mca_handler_bhhook)

Looks good. Some minor niggles, without actually testing the code.

Standard kernel doc headers would be better. See
Documentation/kernel-doc-nano-HOWTO.txt.

+ typedef struct ia64_fptr {
+ unsigned long fp;
+ unsigned long gp;
+ } ia64_fptr_t;

Duplicated here and in arch/ia64/kernel/mca.c. ia64_fptr should be in a common header.

+                       /* change resume address to bottom half */
+                       pmsa->pmsa_iip = mca_hdlr_bh->fp;
+                       pmsa->pmsa_gr[1-1] = mca_hdlr_bh->gp;
+                       /* set cpl with kernel mode */
+                       psr2 = (struct ia64_psr *)&pmsa->pmsa_ipsr;
+                       psr2->cpl = 0;

Also psr2->ri = 0; to ensure that pmsa_iip resumes at the start of the bottom half.

No need to wrap in #ifdef MODULE, these commands also work when the code is built in.

Use module_param() plus MODULE_PARM_DESC() instead of MODULE_PARM(). module_param() lets you specify the value at boot time for built in code, MODULE_PARM() does not.

+static isolate_status_t
+mca_page_isolate(unsigned long paddr)
+{
+ int i;
+ struct page *p;
+
+ /* whether physical address is valid or not */
+ if ( !ia64_phys_addr_valid(paddr) )

The calls to mca_page_isolate() are racy. That code is running in normal kernel context after exiting from the MCA handler. Other cpus could be modifying the page tables at the same time, there could even be two cpus running mca_handler_bh() at the same time for the same page.

Hidetoshi,
Wow! I'm impressed - this is amazing functionality.

I only noticed the three new driver files didn't have either a copyright nor license embedded in it.

I'm told the license is optional because the "linux-2,6/COPYING" contains a blanket statement for the rest of the kernel. But many lawyers still like to see it in each file.

grant

...

...

...

...

Thank you for your useful reply.

But, there is one thing that I want to confirm.

I agree that there could be multiple cpus running handler_bh at the same time, so (even though it would be a rare case) I think it would be better if I avoid the race using something like a spinlock.

ITOH, what the handler_bh should modify is not the page tables but the flag in a struct page which pfn_to_page convert from a physical address. Does the result of the translation from a physical address to a page that includes the address can be changed? (Do you suppose Memory Hotplugs?)

Thanks,
H.Seto

I had a quick look through mm/page_alloc.c and mm/memory.c. Since these are user pages, handler_bh should be able to get mm->page_table_lock. But what if the MCA occurred while the process was already holding mm->page_table_lock? Then mca_page_isolate() would deadlock.

mca_handler_bh() is running as an extension of the MCA event which means that it is not irq safe. It is not safe to get any external lock in mca_page_isolate() or mca_handler_bh(). Even calling printk() from mca_handler_bh() is risky, if the MCA occurred during printk handling then the printk call from mca_handler_bh() would deadlock on logbuf_lock.

mca_handler_bh() can only lock against itself. It is not safe to get any external locks.

I am also concerned about the code in mca_handler_bh() that calls schedule with SIGKILL set. Again that is running as an extension of the MCA event (not irq safe), which means that it could still own locks, or even have interrupts disabled.

AFAICT, my concerns about the MCA event and mca_handler_bh() not being irq safe are only a problem for the case when the MCA was triggered by user space code but was delivered when the cpu was in kernel code. Maybe we do not support the problem case.

* offending process affected process OS MCA do

*     kernel mode        kernel mode       down system
*     kernel mode        user   mode       kill the process
*     user   mode        kernel mode       kill the process <== problem
*     user   mode        user   mode       kill the process

Therefore the MCA handler couldn't recover the system if the process is running in kernel-mode since it possibly has such important kernel locks.

So we have to guarantee that the process is running in user-mode, which all processes can't have any locks of kernel, right?

It would be happy if this fixed patch satisfy your requirement.

Thanks,
H.Seto

diff -Nur linux-2.6.8-rc3/include/asm-ia64/mca.h linux-2.6.8-rc3-mcadrv-v3/include/asm-ia64/mca.h
@@ -22,6 +22,11 @@
 
 #define IA64_MCA_RENDEZ_TIMEOUT		(20 * 1000)	/* value in milliseconds - 20 seconds */
 
+typedef struct ia64_fptr {
+	unsigned long fp;
+	unsigned long gp;
+} ia64_fptr_t;
+
 typedef union cmcv_reg_u {
 	u64	cmcv_regval;
 	struct	{

@@ -114,6 +119,7 @@
 extern void ia64_monarch_init_handler(void);
 extern void ia64_slave_init_handler(void);
 extern void ia64_mca_cmc_vector_setup(void);
+extern int  (*ia64_mca_ucmc_other_recover_fp)(void *,ia64_mca_sal_to_os_state_t *,ia64_mca_os_to_sal_state_t *);
 
 #endif /* !__ASSEMBLY__ */
 #endif /* _ASM_IA64_MCA_H */

diff -Nur linux-2.6.8-rc3/arch/ia64/kernel/mca.c linux-2.6.8-rc3-mcadrv-v3/arch/ia64/kernel/mca.c
@@ -82,11 +82,6 @@
 # define IA64_MCA_DEBUG(fmt...)
 #endif
 
-typedef struct ia64_fptr {
-	unsigned long fp;
-	unsigned long gp;
-} ia64_fptr_t;
-
 /* Used by mca_asm.S */
 ia64_mca_sal_to_os_state_t	ia64_sal_to_os_handoff_state;
 ia64_mca_os_to_sal_state_t	ia64_os_to_sal_handoff_state;

@@ -828,6 +823,12 @@
 
 }
 
+/* This is a function pointer to other error recovery from MCA */
+int (*ia64_mca_ucmc_other_recover_fp)
+	(void*,ia64_mca_sal_to_os_state_t*,ia64_mca_os_to_sal_state_t*)
+	= NULL;
+EXPORT_SYMBOL(ia64_mca_ucmc_other_recover_fp);
+
 /*
  * ia64_mca_ucmc_handler
  *

@@ -849,11 +850,20 @@
 {
 	pal_processor_state_info_t *psp = (pal_processor_state_info_t *)
 		&ia64_sal_to_os_handoff_state.proc_state_param;
-	int recover = psp->tc && !(psp->cc || psp->bc || psp->rc || psp->uc);
+	int recover; 
 
 	/* Get the MCA error record and log it */
 	ia64_mca_log_sal_error_record(SAL_INFO_TYPE_MCA);
 
+	/* TLB error is only exist in this SAL error record */
+	recover = (psp->tc && !(psp->cc || psp->bc || psp->rc || psp->uc))
+	/* other error recovery */
+	   || (ia64_mca_ucmc_other_recover_fp 
+		&& ia64_mca_ucmc_other_recover_fp(
+			IA64_LOG_CURR_BUFFER(SAL_INFO_TYPE_MCA),
+			&ia64_sal_to_os_handoff_state,
+			&ia64_os_to_sal_handoff_state)); 
+
 	/*
 	 *  Wakeup all the processors which are spinning in the rendezvous
 	 *  loop.

diff -Nur linux-2.6.8-rc3/arch/ia64/Kconfig linux-2.6.8-rc3-mcadrv-v3/arch/ia64/Kconfig
@@ -270,6 +270,9 @@
 	depends on IA32_SUPPORT
 	default y
 
+config IA64_MCA_RECOVERY
+	tristate "MCA recovery from errors other than TLB."
+
 config PERFMON
 	bool "Performance monitor support"
 	help

diff -Nur linux-2.6.8-rc3/arch/ia64/kernel/Makefile linux-2.6.8-rc3-mcadrv-v3/arch/ia64/kernel/Makefile
@@ -17,6 +17,8 @@
 obj-$(CONFIG_SMP)		+= smp.o smpboot.o
 obj-$(CONFIG_PERFMON)		+= perfmon_default_smpl.o
 obj-$(CONFIG_IA64_CYCLONE)	+= cyclone.o
+obj-$(CONFIG_IA64_MCA_RECOVERY)	+= mca_recovery.o
+mca_recovery-y			+= mca_drv.o mca_drv_asm.o
 
 # The gate DSO image is built using a special linker script.
 targets += gate.so gate-syms.o

Surprising it looks like a discovery (locking problem within the mca handler) ?

Here is Zoltan's document including a design that exposes a solution how to avoid race (paragraphs 3.5, 3.5.1, 3.5.2 ...) and many other stuff :
http://mca-recovery.sourceforge.net
Linux Itanium MCA Recovery Design Document, third draft
http://mca-recovery.sourceforge.net/mca3.html

+======+============+=================+
|  |\/\/\/| |                       |                                  |
|  |      | |Christian Cotte-Barrot |org.  :BULL/                      |
|  | (~)(o) |Bull S.A.              |office:FREC/B1-401                |
| C      _) |1, rue de Provence     |mailto:                           |
|  | ,___|  |B.P. 208               |   [email protected]|
|  |   /    |38432 ECHIROLLES CEDEX |phone :+33 (0)476297725 (229 7725)|
| /----\    |FRANCE                 |fax   :+33 (0)476297518 (229 7518)|
+======+============+=================+