Messages in this thread | | | Date | Thu, 31 Mar 2022 09:41:39 -0500 | Subject | Re: [PATCH 1/2] x86/mce: Extend AMD severity grading function with new types of errors | From | Carlos Bilbao <> |
| |
On 3/30/2022 3:02 PM, Yazen Ghannam wrote: > On Mon, Mar 28, 2022 at 08:41:32AM -0500, Carlos Bilbao wrote: >> The MCE handler needs to understand the severity of the machine errors to >> act accordingly. In the case of AMD, very few errors are covered in the >> grading logic. >> >> Extend the MCEs severity grading of AMD to cover new types of machine >> errors. >> >> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> >> --- >> arch/x86/include/asm/mce.h | 6 + >> arch/x86/kernel/cpu/mce/severity.c | 180 +++++++++++++++++++++++------ >> 2 files changed, 150 insertions(+), 36 deletions(-) >> >> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h >> index cc73061e7255..6b1ef40f8580 100644 >> --- a/arch/x86/include/asm/mce.h >> +++ b/arch/x86/include/asm/mce.h >> @@ -50,6 +50,12 @@ >> #define MCI_STATUS_POISON BIT_ULL(43) /* access poisonous data */ >> #define MCI_STATUS_SCRUB BIT_ULL(40) /* Error detected during scrub operation */ >> >> +/* AMD Error codes from PPR(s) section 3.1 Machine Check Architecture */ >> +#define ERRORCODE_T_MSK GENMASK(3, 2) /* Mask for transaction type bits */ >> +#define ERRORCODE_M_MSK GENMASK(7, 4) /* Mask for memory transaction type */ >> +#define ERRORCODE_T_DATA 0x4 /* Transaction type of error is Data */ >> +#define ERRORCODE_M_FETCH 0x50 /* Memory transaction type of error is Instruction Fetch */ >> + >> /* >> * McaX field if set indicates a given bank supports MCA extensions: >> * - Deferred error interrupt type is specifiable by bank. >> diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c >> index 1add86935349..4a089e9dbbaf 100644 >> --- a/arch/x86/kernel/cpu/mce/severity.c >> +++ b/arch/x86/kernel/cpu/mce/severity.c >> @@ -327,59 +327,167 @@ static __always_inline int mce_severity_amd_smca(struct mce *m, enum context err >> } >> >> /* >> - * See AMD Error Scope Hierarchy table in a newer BKDG. For example >> - * 49125_15h_Models_30h-3Fh_BKDG.pdf, section "RAS Features" >> + * Evaluate the severity of an overflow error for AMD systems, dependent on >> + * the recoverity features available. >> */ >> -static noinstr int mce_severity_amd(struct mce *m, struct pt_regs *regs, char **msg, bool is_excp) >> +static noinstr int mce_grade_overflow_amd(struct mce *m, enum context ctx) >> { >> - enum context ctx = error_context(m, regs); >> + int ret; >> >> - /* Processor Context Corrupt, no need to fumble too much, die! */ >> - if (m->status & MCI_STATUS_PCC) >> + /* >> + * On older systems where overflow_recov flag is not present, we >> + * should simply panic if an error overflow occurs. If >> + * overflow_recov flag is present and set, then software can try >> + * to at least kill process to prolong system operation. >> + */ >> + if (mce_flags.overflow_recov) { >> + if (mce_flags.smca) { >> + ret = mce_severity_amd_smca(m, ctx); >> + } else { >> + /* kill current process */ >> + ret = MCE_AR_SEVERITY; >> + } >> + return ret; >> + } >> + >> + /* at least one error was not logged */ >> + if (m->status & MCI_STATUS_OVER) >> return MCE_PANIC_SEVERITY; >> >> - if (m->status & MCI_STATUS_UC) { >> + /* >> + * For any other case, return MCE_UC_SEVERITY so that we log the >> + * error and exit #MC handler. >> + */ >> + return MCE_UC_SEVERITY; >> +} >> >> - if (ctx == IN_KERNEL) >> - return MCE_PANIC_SEVERITY; >> +/* >> + * See AMD PPR(s) section 3.1 Machine Check Architecture >> + */ >> +static noinstr int mce_severity_amd(struct mce *m, struct pt_regs *regs, char **msg, bool is_excp) >> +{ >> + enum context ctx = error_context(m, regs); >> + int ret; >> >> - /* >> - * On older systems where overflow_recov flag is not present, we >> - * should simply panic if an error overflow occurs. If >> - * overflow_recov flag is present and set, then software can try >> - * to at least kill process to prolong system operation. >> - */ >> - if (mce_flags.overflow_recov) { >> - if (mce_flags.smca) >> - return mce_severity_amd_smca(m, ctx); >> + /* >> + * Default return values. The poll handler catches these and passes >> + * responsibility of decoding them to EDAC >> + */ >> + ret = MCE_KEEP_SEVERITY; >> >> - /* kill current process */ >> - return MCE_AR_SEVERITY; >> - } else { >> - /* at least one error was not logged */ >> - if (m->status & MCI_STATUS_OVER) >> - return MCE_PANIC_SEVERITY; >> + /* >> + * Evaluate the severity of deferred errors for AMD systems, for which only >> + * scrub error is interesting to notify an action requirement. >> + */ >> + if (m->status & MCI_STATUS_DEFERRED) { >> + if (m->status & MCI_STATUS_SCRUB) > > Multi-line if-else statements should have braces. This is true even for > conditions with a single line. This is a coding style point. > > In other words, if all conditions are a single line, then no braces needed. > But if any of the conditions has multiple lines, then all conditions need > braces. > >> + ret = MCE_AR_SEVERITY; > > This is wrong. Action required is for errors that need to be handled > immediately and where recovery is possible, like a poison consumption > machine check exception. > > Also, the SCRUB indicator does not contribute any new or interesting > information in terms of OS actions. > >> + else { >> + /* >> + * deferred error: poll handler catches these and adds to mce_ring so >> + * memory-failure can take recovery actions. >> + */ >> + ret = MCE_DEFERRED_SEVERITY; >> } >> + goto amd_severity; >> + } >> >> - /* >> - * For any other case, return MCE_UC_SEVERITY so that we log the >> - * error and exit #MC handler. >> - */ >> - return MCE_UC_SEVERITY; >> + /* If the UC bit is not set, the error has been corrected */ >> + if (!(m->status & MCI_STATUS_UC)) { >> + ret = MCE_KEEP_SEVERITY; >> + goto amd_severity; >> } >> >> /* >> - * deferred error: poll handler catches these and adds to mce_ring so >> - * memory-failure can take recovery actions. >> + * Evaluate the severity of memory poison for AMD systems, >> + * depending on the context in which the MCE happened. >> */ >> - if (m->status & MCI_STATUS_DEFERRED) >> - return MCE_DEFERRED_SEVERITY; >> + if (m->status & MCI_STATUS_POISON) { >> + switch (ctx) { >> + case IN_USER: >> + ret = MCE_AR_SEVERITY; >> + break; >> + case IN_KERNEL_RECOV: >> +#ifdef CONFIG_MEMORY_FAILURE >> + ret = MCE_AR_SEVERITY; >> +#else >> + ret = MCE_PANIC_SEVERITY; >> +#endif >> + break; >> + case IN_KERNEL: >> + ret = MCE_PANIC_SEVERITY; >> + break; > > Is this case needed if it's identical to the default? > >> + default: >> + ret = MCE_PANIC_SEVERITY; >> + } >> + >> + goto amd_severity; >> + } >> + >> + /* Processor Context Corrupt, no need to fumble too much, die! */ >> + if (m->status & MCI_STATUS_PCC) { >> + ret = MCE_PANIC_SEVERITY; >> + goto amd_severity; >> + } > > This should be the first condition checked. > >> >> /* >> - * corrected error: poll handler catches these and passes responsibility >> - * of decoding the error to EDAC >> + * Evaluate the severity of data load error for AMD systems, >> + * depending on the context in which the MCE happened. >> */ >> - return MCE_KEEP_SEVERITY; >> + if ((m->status & ERRORCODE_T_MSK) == ERRORCODE_T_DATA) { > > This information is not useful for determining severity. > >> + switch (ctx) { >> + case IN_USER: >> + case IN_KERNEL_RECOV: >> + ret = MCE_AR_SEVERITY; >> + break; >> + case IN_KERNEL: >> + ret = MCE_PANIC_SEVERITY; >> + break; >> + default: >> + ret = MCE_PANIC_SEVERITY; >> + } >> + >> + goto amd_severity; >> + } >> + >> + >> + /* >> + * Evaluate the severity of an instruction fetch error for AMD systems, >> + * depending on the context in which the MCE happened. >> + */ >> + if ((m->status & ERRORCODE_M_MSK) == ERRORCODE_M_FETCH) { > > This information is not useful for determining severity. > >> + switch (ctx) { >> + case IN_USER: >> + ret = MCE_AR_SEVERITY; >> + break; >> + case IN_KERNEL_RECOV: >> +#ifdef CONFIG_MEMORY_FAILURE >> + ret = MCE_AR_SEVERITY; >> +#else >> + ret = MCE_PANIC_SEVERITY; >> +#endif >> + break; >> + case IN_KERNEL: >> + ret = MCE_PANIC_SEVERITY; >> + break; >> + default: >> + ret = MCE_PANIC_SEVERITY; >> + } >> + >> + goto amd_severity; >> + } >> + >> + if (m->status & MCI_STATUS_OVER) { >> + ret = mce_grade_overflow_amd(m, ctx); >> + goto amd_severity; >> + } > > This condition should be checked earlier. > >> + >> + if (ctx == IN_KERNEL) >> + ret = MCE_PANIC_SEVERITY; > > This condition should be checked earlier. This would avoid the redundant > checks above. > > > Overall, the severity code should be audited and simplified first. Existing > cases should then be updated with useful information like messages. Finally, > new cases can be added if and only if they are unique, i.e. they're not > already covered by existing cases. > > The current code already follows the hardware documentation for the most part. > Here's an example of how it can be simplified. > > Pseudocode: > > if "PCC bit set" > PANIC > > if "Deferred bit set" > DEFERRED > > if "UC bit not set" > KEEP > > if "Overflow recovery not supported" and "Overflow bit set" > PANIC > > if "MCA recovery (SUCCOR) not supported" > PANIC > > if "In kernel context" > PANIC > > else > AR > > > Notes: > > 1) The Deferred and !UC cases are moved up compared to documentation. It > looks like you had a similar idea. This saves an indentation level since all > the following cases are for uncorrectable errors. > > 2) The TCC check is not included. This is because TCC has a similar function > to RIPV|EIPV *. And the only use is in conjunction with "kernel context". So > this is redundant as "UC in kernel context" is a PANIC. > > * A TCC check can be done as an alternative to mc_recoverable(). But > let's not worry about that now. > > 3) There is no SMCA check. This is because SMCA is used only to determine if > the TCC bit *may* be defined. > > So please do the code simplification in a first patch. Then add messages for > those existing cases in a second patch. >
Thanks Yazen, this approach definitely simplifies everything. I'll send a v2 of the patchset using this pseudocode.
> Thanks, > Yazen
Cheers, Carlos
| |