Re: [PATCH 5/5] mce: recover from "action required" errors reported in data path in usermode

[Chen Gong <gong.chen@linux.intel.com>]

于 2011/9/1 6:26, Luck, Tony 写道:
> From: Tony Luck<tony.luck@intel.com>
>
> Two new entries in the mce severity table - one notes that data errors
> observed by innocent bystanders (who happen to share a machine check
> bank with the cpu experiencing the error) should be left alone by using
> the "KEEP" severity.
>
> Then inline in the do_machine_check() handler we process the user-mode
> data error that was marked at MCE_AR_SEVERITY.  Even though we are in
> "machine check context" it is almost safe to do so. We have already
> released all the other cpus from rendezvous and we know that the cpu
> with the error was executing user code - so it cannot have interrupts
> locked out, or hold any locks. I.e. this is almost equivalent to a
> page fault. Only difference (and risk) is that on x86_64 we are still
> on the machine check stack - so if another machine check arrives, we
> are toast (we didn't clear MCG_STATUS - yet, so cpu will reset rather
> than taking a nested machine check on the same stack).
>
> Signed-off-by: Tony Luck<tony.luck@intel.com>
> ---
>
> Using the "KEEP" state avoids the complexity of my earlier solution
> that sorted the cpus by severity and ran the more serious ones first.
>
>   arch/x86/kernel/cpu/mcheck/mce-severity.c |   14 ++++++++++-
>   arch/x86/kernel/cpu/mcheck/mce.c          |   35 ++++++++++++++++++++--------
>   2 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce-severity.c b/arch/x86/kernel/cpu/mcheck/mce-severity.c
> index 7395d5f..c4d8b24 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce-severity.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce-severity.c
> @@ -54,6 +54,7 @@ static struct severity {
>   #define  MASK(x, y)	.mask = x, .result = y
>   #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
>
>   	MCESEV(
> @@ -102,11 +103,22 @@ static struct severity {
>   		SER, BITCLR(MCI_STATUS_S)
>   		),
>
> -	/* AR add known MCACODs here */
>   	MCESEV(
>   		PANIC, "Action required with lost events",
>   		SER, BITSET(MCI_STATUS_OVER|MCI_UC_SAR)
>   		),
> +
> +	/* known AR MCACODs: */
> +	MCESEV(
> +		KEEP, "HT thread notices Action required: data load error",
> +		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|0x0134),
> +		MCGMASK(MCG_STATUS_EIPV, 0)
> +		),
> +	MCESEV(
> +		AR, "Action required: data load error",
> +		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|0x0134),
> +		USER
> +		),

I don't think *AR* makes sense here because the following codes have a 
assumption that it means *user space* condition. If so, in the future a 
new *AR* severity for kernel usage is created, we can't distinguish 
which one can call "memory_failure" as below. At least, it should have a 
suffix such as AR_USER/AR_KERN:

enum severity_level {
         MCE_NO_SEVERITY,
         MCE_KEEP_SEVERITY,
         MCE_SOME_SEVERITY,
         MCE_AO_SEVERITY,
         MCE_UC_SEVERITY,
         MCE_AR_USER_SEVERITY,
	MCE_AR_KERN_SEVERITY,
         MCE_PANIC_SEVERITY,
};


>   	MCESEV(
>   		PANIC, "Action required: unknown MCACOD",
>   		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR)
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 135e12d..2c59a34 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -996,12 +996,6 @@ void do_machine_check(struct pt_regs *regs, long error_code)
>   			continue;
>   		}
>
> -		/*
> -		 * Kill on action required.
> -		 */
> -		if (severity == MCE_AR_SEVERITY)
> -			kill_it = 1;
> -
>   		mce_read_aux(&m, i);
>
>   		/*
> @@ -1022,6 +1016,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
>   		}
>   	}
>
> +	m = *final;
> +
>   	if (!no_way_out)
>   		mce_clear_state(toclear);
>
> @@ -1040,7 +1036,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
>   	 * support MCE broadcasting or it has been disabled.
>   	 */
>   	if (no_way_out&&  tolerant<  3)
> -		mce_panic("Fatal machine check on current CPU", final, msg);
> +		mce_panic("Fatal machine check on current CPU",&m, msg);
>
>   	/*
>   	 * If the error seems to be unrecoverable, something should be
> @@ -1049,11 +1045,24 @@ void do_machine_check(struct pt_regs *regs, long error_code)
>   	 * high, don't try to do anything at all.
>   	 */
>
> -	if (kill_it&&  tolerant<  3)
> +	if (worst != MCE_AR_SEVERITY&&  kill_it&&  tolerant<  3)
>   		force_sig(SIGBUS, current);
>
>   	if (worst>  0)
>   		mce_report_event(regs);
> +
> +	if (worst == MCE_AR_SEVERITY) {
> +		unsigned long pfn = m.addr>>  PAGE_SHIFT;
> +
> +		pr_err("Uncorrected hardware memory error in user-access at %llx",
> +			m.addr);

print in the MCE handler maybe makes a deadlock ? say, when other CPUs 
are printing something, suddently they received MCE broadcast from 
Monarch CPU, when Monarch CPU runs above codes, a deadlock happens ?
Please fix me if I miss something :-)

> +		if (__memory_failure(pfn, MCE_VECTOR, 0)<  0) {
> +			pr_err("Memory error not recovered");
> +			force_sig(SIGBUS, current);
> +		} else
> +			pr_err("Memory error recovered");
> +	}

as you mentioned in the comment, the biggest concern is that when 
__memory_failure runs too long, if another MCE happens at the same time, 
(assuming this MCE is happened on its sibling CPU which has the same 
banks), the 2nd MCE will crash the system. Why not delaying the process 
in a safer context, such as using user_return_notifer ?

> +
>   	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
>   out:
>   	atomic_dec(&mce_entry);
> @@ -1061,12 +1070,18 @@ out:
>   }
>   EXPORT_SYMBOL_GPL(do_machine_check);
>
> -/* dummy to break dependency. actual code is in mm/memory-failure.c */
> -void __attribute__((weak)) memory_failure(unsigned long pfn, int vector)
> +#ifndef CONFIG_MEMORY_FAILURE
> +void memory_failure(unsigned long pfn, int vector)
>   {
>   	pr_err("Action optional memory failure at %lx ignored\n", pfn);
>   }
>
> +int __memory_failure(unsigned long pfn, int trapno, int flags)
> +{
> +	return -ENXIO;
> +}
> +#endif
> +
>   static void mce_process_work(struct work_struct *dummy)
>   {
>   	unsigned long pfn;