x86, mce: Fix machine_check_poll() tests for which errors to log
Subsystems (3)
Tony Luck
maintainerPATCH 1/1 x86, mce: Fix machine_check_poll() tests for which errors to log
There has been a lurking "TBD" in the machine check poll routine ever since it was first split out from the machine check handler. The potential issue is that the poll routine may have just begun a read from the STATUS register in a machine check bank when the hardware logs an error in that bank and signals a machine check. That race used to be pretty small back when machine checks were broadcast, but the addition of local machine check means that the poll code could continue running and clear the error from the bank before the local machine check handler on another CPU gets around to reading it.
Fix the code to be sure to only process errors that need to be processed in the poll code, leaving other logged errors alone for the machine check handler to find and process.
--- arch/x86/kernel/cpu/mce/core.c | 42 ++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 7 deletions(-)
arch/x86/kernel/cpu/mce/core.c +35 -7
@@ -712,19 +712,47 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)barrier (); m.status =mce_rdmsrl (msr_ops.status (i)); + +/* If this entry is not valid, ignore it */ if (!(m.status &MCI_STATUS_VAL ))continue ;/* -* Uncorrected or signalled events are handled by the exception -* handler when it is enabled, so don't process those here. -* -* TBD do the same check for MCI_STATUS_EN here? +* If we are logging everything (at CPU online) or this +* is a corrected error, then we must log it. */ -if (!(flags &MCP_UC ) && - (m.status & (mca_cfg.ser ?MCI_STATUS_S :MCI_STATUS_UC ))) -continue ; +if ((flags &MCP_UC ) || (m.status &MCI_STATUS_UC ) ==0 ) + goto log_it; + +/* +* Older systems that do not support software error recovery +* should skip over uncorrected errors, but log everything else +*/ +if (!mca_cfg.ser) { +if (m.status &MCI_STATUS_UC ) +continue ; + goto log_it; + } + +/* Log "not enabled" (speculative) errors */ +if (!(m.status &MCI_STATUS_EN )) + goto log_it; + +/* +* Log UCNA (SDM: 15.6.3 "UCR Error Classification") +* UC == 1 && PCC == 0 && S == 0 +*/ +if (!(m.status &MCI_STATUS_PCC ) && !(m.status &MCI_STATUS_S )) + goto log_it; + +/* +* Skip anything else. Presumption is that our read of this +* bank is racing with a machine check. Leave the log alone +* for do_machine_check() to deal with it. +*/ +continue ; +log_it: error_seen = true;mce_read_aux (&m, i);
Signature · 1 line
2.19.1
Quoted 67 lines
-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Tony Luck
Sent: Monday, March 11, 2019 1:51 PM
To: Borislav Petkov <[email protected]>
Cc: Tony Luck <[email protected]>; [email protected]; [email protected]; Ashok Raj <[email protected]>
Subject: [PATCH] x86, mce: Fixmachine_check_poll()tests for which errors to logThere has been a lurking "TBD" in the machine check poll routine ever since it was first split out from the machine check handler. The potential issue is that the poll routine may have just begun a read from the STATUS register in a machine check bank when the hardware logs an error in that bank and signals a machine check. That race used to be pretty small back when machine checks were broadcast, but the addition of local machine check means that the poll code could continue running and clear the error from the bank before the local machine check handler on another CPU gets around to reading it.
Fix the code to be sure to only process errors that need to be processed in the poll code, leaving other logged errors alone for the machine check handler to find and process.
Fixes b79109c3bbcf ("x86, mce: separate correct machine check poller and fatal exception handler")Fixes ed7290d0ee8f ("x86, mce: implement new status bits")Reported-by Ashok Raj <[email protected]>Signed-off-by Tony Luck <[email protected]>--- arch/x86/kernel/cpu/mce/core.c | 42 ++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 7 deletions(-)arch/x86/kernel/cpu/mce/core.c +18
-7@@ -712,19 +712,47 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)barrier (); m.status =mce_rdmsrl (msr_ops.status (i)); + +/* If this entry is not valid, ignore it */ if (!(m.status &MCI_STATUS_VAL ))continue ;/* -* Uncorrected or signalled events are handled by the exception -* handler when it is enabled, so don't process those here. -* -* TBD do the same check for MCI_STATUS_EN here? +* If we are logging everything (at CPU online) or this +* is a corrected error, then we must log it. */ -if (!(flags &MCP_UC ) && - (m.status & (mca_cfg.ser ?MCI_STATUS_S :MCI_STATUS_UC ))) -continue ; +if ((flags &MCP_UC ) || (m.status &MCI_STATUS_UC ) ==0 ) + goto log_it; + +/* +* Older systems that do not support software error recovery +* should skip over uncorrected errors, but log everything else +*/ +if (!mca_cfg.ser) { +if (m.status &MCI_STATUS_UC ) +continue ; + goto log_it; + } + +/* Log "not enabled" (speculative) errors */
arch/x86/kernel/cpu/mce/core.c bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b) · line 747
Quoted 10 lines
arch/x86/kernel/cpu/mce/core.c +10
-0+if (!(m.status &MCI_STATUS_EN )) + goto log_it; + +/* +* Log UCNA (SDM: 15.6.3 "UCR Error Classification") +* UC == 1 && PCC == 0 && S == 0 +*/ +if (!(m.status &MCI_STATUS_PCC ) && !(m.status &MCI_STATUS_S )) + goto log_it; +
Can you please include a vendor check with this? MCi_STATUS[56] is not defined the same way on AMD systems.
Thanks,
Yazen
Luck, Tony
maintainerQuoted 6 lines · Yazen
arch/x86/kernel/cpu/mce/core.c bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b) · line 747
Quoted 3 lines
arch/x86/kernel/cpu/mce/core.c +3
-0+if (!(m.status &MCI_STATUS_PCC ) && !(m.status &MCI_STATUS_S )) + goto log_it; +Can you please include a vendor check with this? MCi_STATUS[56] is not defined the same way on AMD systems.
Original code also looked at MCi_STATUS[56] without a vendor check:
Quoted 1 line
Quoted 1 line
- (m.status & (mca_cfg.ser ?
MCI_STATUS_S:MCI_STATUS_UC)))
Was this OK because you don't set mca_cfg.ser?
If so, my new code will also skip out before getting to this test. But should probably have a better comment. Something like:
/*
* Newer Intel systems that support software error
* recovery need to make some extra checks. Other
* CPUs should skip over uncorrected errors, but log
* everything else
*/
if (!mca_cfg.ser) {
if (m.status & MCI_STATUS_UC)
continue;
goto log_it;
}-Tony
Quoted 26 lines
-----Original Message-----
From: Luck, Tony <[email protected]>
Sent: Monday, March 11, 2019 3:42 PM
To: Ghannam, Yazen <[email protected]>
Cc: Borislav Petkov <[email protected]>; [email protected]; [email protected]; Ashok Raj <[email protected]>
Subject: Re: [PATCH] x86, mce: Fixmachine_check_poll()tests for which errors to logQuoted 6 lines · Yazen
arch/x86/kernel/cpu/mce/core.c bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b) · line 747
Quoted 3 lines
arch/x86/kernel/cpu/mce/core.c +3
-0+if (!(m.status &MCI_STATUS_PCC ) && !(m.status &MCI_STATUS_S )) + goto log_it; +Can you please include a vendor check with this? MCi_STATUS[56] is not defined the same way on AMD systems.
Original code also looked at MCi_STATUS[56] without a vendor check:
Quoted 1 line
Quoted 1 line
- (m.status & (mca_cfg.ser ?
MCI_STATUS_S:MCI_STATUS_UC)))Was this OK because you don't set mca_cfg.ser?
If so, my new code will also skip out before getting to this test. But should probably have a better comment. Something like:
Quoted 12 lines
/*
* Newer Intel systems that support software error
* recovery need to make some extra checks. Other
* CPUs should skip over uncorrected errors, but log
* everything else
*/
if (!mca_cfg.ser) {
if (m.status & MCI_STATUS_UC)
continue;
goto log_it;
}Yes, you're right. Thanks for pointing that out.
-Yazen
Luck, Tony
maintainerPATCH v2 1/1 x86, mce: Fix machine_check_poll() tests for which errors
There has been a lurking "TBD" in the machine check poll routine ever since it was first split out from the machine check handler. The potential issue is that the poll routine may have just begun a read from the STATUS register in a machine check bank when the hardware logs an error in that bank and signals a machine check. That race used to be pretty small back when machine checks were broadcast, but the addition of local machine check means that the poll code could continue running and clear the error from the bank before the local machine check handler on another CPU gets around to reading it.
Fix the code to be sure to only process errors that need to be processed in the poll code, leaving other logged errors alone for the machine check handler to find and process.
---
V2: Update comment to make it clear that only Intel CPUs with software error recovery reach the final few tests on whether to log.
arch/x86/kernel/cpu/mce/core.c | 44 ++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 7 deletions(-)
arch/x86/kernel/cpu/mce/core.c +37 -7
@@ -712,19 +712,49 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)barrier (); m.status =mce_rdmsrl (msr_ops.status (i)); + +/* If this entry is not valid, ignore it */ if (!(m.status &MCI_STATUS_VAL ))continue ;/* -* Uncorrected or signalled events are handled by the exception -* handler when it is enabled, so don't process those here. -* -* TBD do the same check for MCI_STATUS_EN here? +* If we are logging everything (at CPU online) or this +* is a corrected error, then we must log it. */ -if (!(flags &MCP_UC ) && - (m.status & (mca_cfg.ser ?MCI_STATUS_S :MCI_STATUS_UC ))) -continue ; +if ((flags &MCP_UC ) || (m.status &MCI_STATUS_UC ) ==0 ) + goto log_it; + +/* +* Newer Intel systems that support software error +* recovery need to make additional checks. Other +* CPUs should skip over uncorrected errors, but log +* everything else. +*/ +if (!mca_cfg.ser) { +if (m.status &MCI_STATUS_UC ) +continue ; + goto log_it; + } + +/* Log "not enabled" (speculative) errors */ +if (!(m.status &MCI_STATUS_EN )) + goto log_it; + +/* +* Log UCNA (SDM: 15.6.3 "UCR Error Classification") +* UC == 1 && PCC == 0 && S == 0 +*/ +if (!(m.status &MCI_STATUS_PCC ) && !(m.status &MCI_STATUS_S )) + goto log_it; + +/* +* Skip anything else. Presumption is that our read of this +* bank is racing with a machine check. Leave the log alone +* for do_machine_check() to deal with it. +*/ +continue ; +log_it: error_seen = true;mce_read_aux (&m, i);
Signature · 1 line
2.19.1
tip-bot for Tony Luck
merge bot[tip:ras/core] x86/mce: Fix machine_check_poll() tests for error types
x86/mce: Fix machine_check_poll() tests for error types
Commit-ID: f19501aa07f18268ab14f458b51c1c6b7f72a134 Gitweb: https://git.kernel.org/tip/f19501aa07f18268ab14f458b51c1c6b7f72a134 Author: Tony Luck <[email protected]> AuthorDate: Tue, 12 Mar 2019 10:09:38 -0700 Committer: Borislav Petkov <[email protected]> CommitDate: Wed, 27 Mar 2019 10:53:49 +0100
x86/mce: Fix machine_check_poll() tests for error types
There has been a lurking "TBD" in the machine check poll routine ever since it was first split out from the machine check handler. The potential issue is that the poll routine may have just begun a read from the STATUS register in a machine check bank when the hardware logs an error in that bank and signals a machine check.
That race used to be pretty small back when machine checks were broadcast, but the addition of local machine check means that the poll code could continue running and clear the error from the bank before the local machine check handler on another CPU gets around to reading it.
Fix the code to be sure to only process errors that need to be processed in the poll code, leaving other logged errors alone for the machine check handler to find and process.
[ bp: Massage a bit and flip the "== 0" check to the usual !(..) test. ]
--- arch/x86/kernel/cpu/mce/core.c | 44 +++++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-)
arch/x86/kernel/cpu/mce/core.c +37 -7
@@ -712,19 +712,49 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)barrier (); m.status =mce_rdmsrl (msr_ops.status (i)); + +/* If this entry is not valid, ignore it */ if (!(m.status &MCI_STATUS_VAL ))continue ;/* -* Uncorrected or signalled events are handled by the exception -* handler when it is enabled, so don't process those here. -* -* TBD do the same check for MCI_STATUS_EN here? +* If we are logging everything (at CPU online) or this +* is a corrected error, then we must log it. */ -if (!(flags &MCP_UC ) && - (m.status & (mca_cfg.ser ?MCI_STATUS_S :MCI_STATUS_UC ))) -continue ; +if ((flags &MCP_UC ) || !(m.status &MCI_STATUS_UC )) + goto log_it; + +/* +* Newer Intel systems that support software error +* recovery need to make additional checks. Other +* CPUs should skip over uncorrected errors, but log +* everything else. +*/ +if (!mca_cfg.ser) { +if (m.status &MCI_STATUS_UC ) +continue ; + goto log_it; + } + +/* Log "not enabled" (speculative) errors */ +if (!(m.status &MCI_STATUS_EN )) + goto log_it; + +/* +* Log UCNA (SDM: 15.6.3 "UCR Error Classification") +* UC == 1 && PCC == 0 && S == 0 +*/ +if (!(m.status &MCI_STATUS_PCC ) && !(m.status &MCI_STATUS_S )) + goto log_it; + +/* +* Skip anything else. Presumption is that our read of this +* bank is racing with a machine check. Leave the log alone +* for do_machine_check() to deal with it. +*/ +continue ; +log_it: error_seen = true;mce_read_aux (&m, i);