lkml.org 
[lkml]   [2022]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] x86/mce: Extend AMD severity grading function with new types of errors
From
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

\
 
 \ /
  Last update: 2022-03-31 16:42    [W:0.049 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site