[PATCH 0/2] x86/mce: Grade new machine errors for AMD MCEs and include messages for panic cases

[PATCH 0/2] x86/mce: Grade new machine errors for AMD MCEs and include messages for panic cases

[Carlos Bilbao]

This patchset includes grading of new types of machine errors on AMD's MCE
grading logic mce_severity_amd(), which helps the MCE handler determine
what actions to take. If the error is graded as a PANIC, the EDAC driver
will not decode; so we also include new error messages to describe the MCE
and help debugging critical errors.

Carlos Bilbao (2):
  x86/mce: Extend AMD severity grading function with new types of errors
  x86/mce: Add messages to describe panic machine errors on AMD's MCEs grading
 
 arch/x86/include/asm/mce.h         |   6 +
 arch/x86/kernel/cpu/mce/severity.c | 203 ++++++++++++++++++++++++-----
 2 files changed, 174 insertions(+), 35 deletions(-)

-- 
2.27.0

[PATCH 1/2] x86/mce: Extend AMD severity grading function with new types of errors

[Carlos Bilbao]

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)
+			ret = MCE_AR_SEVERITY;
+		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;
+		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;
+	}
 
 	/*
-	 * 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) {
+		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) {
+		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;
+	}
+
+	if (ctx == IN_KERNEL)
+		ret = MCE_PANIC_SEVERITY;
+
+amd_severity:
+
+	return ret;
 }
 
 static noinstr int mce_severity_intel(struct mce *m, struct pt_regs *regs, char **msg, bool is_excp)
-- 
2.27.0

Re: [PATCH 1/2] x86/mce: Extend AMD severity grading function with new types of errors

[Yazen Ghannam]

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

Re: [PATCH 1/2] x86/mce: Extend AMD severity grading function with new types of errors

[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

[PATCH 2/2] x86/mce: Add messages to describe panic machine errors on AMD's MCEs grading

[Carlos Bilbao]

When a machine error is graded as PANIC by AMD grading logic, the MCE
handler calls mce_panic(). The notification chain does not come into effect
so the AMD EDAC driver does not decode the errors. In these cases, the
messages displayed to the user are more cryptic and miss information
that might be relevant, like the context in which the error took place.

Fix the above issue including messages on AMD's grading logic for machine
errors graded as PANIC.

Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
---
 arch/x86/kernel/cpu/mce/severity.c | 33 ++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
index 4a089e9dbbaf..11be63eaf7e5 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -330,10 +330,12 @@ static __always_inline int mce_severity_amd_smca(struct mce *m, enum context err
  * Evaluate the severity of an overflow error for AMD systems, dependent on
  * the recoverable features available.
  */
-static noinstr int mce_grade_overflow_amd(struct mce *m, enum context ctx)
+static noinstr int mce_grade_overflow_amd(struct mce *m, enum context ctx, char **msg)
 {
 	int ret;
 
+	WARN_ON(!msg);
+
 	/*
 	 * On older systems where overflow_recov flag is not present, we
 	 * should simply panic if an error overflow occurs. If
@@ -343,6 +345,8 @@ static noinstr int mce_grade_overflow_amd(struct mce *m, enum context ctx)
 	if (mce_flags.overflow_recov) {
 		if (mce_flags.smca) {
 			ret = mce_severity_amd_smca(m, ctx);
+			if (ret == MCE_PANIC_SEVERITY)
+				*msg = "Uncorrected unrecoverable error";
 		} else {
 			/* kill current process */
 			ret = MCE_AR_SEVERITY;
@@ -351,8 +355,10 @@ static noinstr int mce_grade_overflow_amd(struct mce *m, enum context ctx)
 	}
 
 	/* at least one error was not logged */
-	if (m->status & MCI_STATUS_OVER)
+	if (m->status & MCI_STATUS_OVER) {
+		*msg = "Overflow uncorrected";
 		return MCE_PANIC_SEVERITY;
+	}
 
 	/*
 	 * For any other case, return MCE_UC_SEVERITY so that we log the
@@ -367,6 +373,7 @@ static noinstr int mce_grade_overflow_amd(struct mce *m, enum context ctx)
 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);
+	char *severity_msg;
 	int ret;
 
 	/*
@@ -411,13 +418,16 @@ static noinstr int mce_severity_amd(struct mce *m, struct pt_regs *regs, char **
 #ifdef CONFIG_MEMORY_FAILURE
 			ret = MCE_AR_SEVERITY;
 #else
+			severity_msg = "Consumed poisoned data in kernel recoverable area";
 			ret = MCE_PANIC_SEVERITY;
 #endif
 			break;
 		case IN_KERNEL:
+			severity_msg = "Attempt to consume poisoned data in kernel context";
 			ret = MCE_PANIC_SEVERITY;
 			break;
 		default:
+			severity_msg = "Attempt to consume poisoned data in unknown context";
 			ret = MCE_PANIC_SEVERITY;
 		}
 
@@ -426,6 +436,7 @@ static noinstr int mce_severity_amd(struct mce *m, struct pt_regs *regs, char **
 
 	/* Processor Context Corrupt, no need to fumble too much, die! */
 	if (m->status & MCI_STATUS_PCC) {
+		severity_msg = "Processor Context Corrupt";
 		ret = MCE_PANIC_SEVERITY;
 		goto amd_severity;
 	}
@@ -441,9 +452,11 @@ static noinstr int mce_severity_amd(struct mce *m, struct pt_regs *regs, char **
 			ret = MCE_AR_SEVERITY;
 			break;
 		case IN_KERNEL:
+			severity_msg = "Data load error in unrecoverable kernel context";
 			ret = MCE_PANIC_SEVERITY;
 			break;
 		default:
+			severity_msg = "Data load error in unknown context";
 			ret = MCE_PANIC_SEVERITY;
 		}
 
@@ -464,13 +477,16 @@ static noinstr int mce_severity_amd(struct mce *m, struct pt_regs *regs, char **
 #ifdef CONFIG_MEMORY_FAILURE
 			ret = MCE_AR_SEVERITY;
 #else
+			severity_msg = "Instruction fetch error in kernel recoverable area";
 			ret = MCE_PANIC_SEVERITY;
 #endif
 			break;
 		case IN_KERNEL:
+			severity_msg = "Instruction fetch error in kernel context";
 			ret = MCE_PANIC_SEVERITY;
 			break;
 		default:
+			severity_msg = "Instruction fetch error in unknown context";
 			ret = MCE_PANIC_SEVERITY;
 		}
 
@@ -478,15 +494,24 @@ static noinstr int mce_severity_amd(struct mce *m, struct pt_regs *regs, char **
 	}
 
 	if (m->status & MCI_STATUS_OVER) {
-		ret = mce_grade_overflow_amd(m, ctx);
+		ret = mce_grade_overflow_amd(m, ctx, &severity_msg);
 		goto amd_severity;
 	}
 
-	if (ctx == IN_KERNEL)
+	if (ctx == IN_KERNEL) {
+		severity_msg = "Uncorrectable error in kernel context";
 		ret = MCE_PANIC_SEVERITY;
+	}
 
 amd_severity:
 
+	/*
+	 * It only makes sense to provide a message on panic scenarios,
+	 * as otherwise EDAC will be notified and conduct the decoding.
+	 */
+	if (msg && ret == MCE_PANIC_SEVERITY)
+		*msg = severity_msg;
+
 	return ret;
 }
 
-- 
2.27.0

Re: [PATCH 0/2] x86/mce: Grade new machine errors for AMD MCEs and include messages for panic cases

[Borislav Petkov]

On Mon, Mar 28, 2022 at 08:41:30AM -0500, Carlos Bilbao wrote:
> This patchset includes grading of new types of machine errors on AMD's MCE
> grading logic mce_severity_amd(), which helps the MCE handler determine
> what actions to take. If the error is graded as a PANIC, the EDAC driver
> will not decode; so we also include new error messages to describe the MCE
> and help debugging critical errors.
> 
> Carlos Bilbao (2):
>   x86/mce: Extend AMD severity grading function with new types of errors
>   x86/mce: Add messages to describe panic machine errors on AMD's MCEs grading
>  
>  arch/x86/include/asm/mce.h         |   6 +
>  arch/x86/kernel/cpu/mce/severity.c | 203 ++++++++++++++++++++++++-----
>  2 files changed, 174 insertions(+), 35 deletions(-)

How is this submission different from

https://lore.kernel.org/r/20220311165114.482074-1-carlos.bilbao@amd.com

?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/2] x86/mce: Grade new machine errors for AMD MCEs and include messages for panic cases

[Carlos Bilbao]

On 3/28/2022 8:55 AM, Borislav Petkov wrote:
> On Mon, Mar 28, 2022 at 08:41:30AM -0500, Carlos Bilbao wrote:
>> This patchset includes grading of new types of machine errors on AMD's MCE
>> grading logic mce_severity_amd(), which helps the MCE handler determine
>> what actions to take. If the error is graded as a PANIC, the EDAC driver
>> will not decode; so we also include new error messages to describe the MCE
>> and help debugging critical errors.
>>
>> Carlos Bilbao (2):
>>   x86/mce: Extend AMD severity grading function with new types of errors
>>   x86/mce: Add messages to describe panic machine errors on AMD's MCEs grading
>>  
>>  arch/x86/include/asm/mce.h         |   6 +
>>  arch/x86/kernel/cpu/mce/severity.c | 203 ++++++++++++++++++++++++-----
>>  2 files changed, 174 insertions(+), 35 deletions(-)
> 
> How is this submission different from
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20220311165114.482074-1-carlos.bilbao%40amd.com&data=04%7C01%7Ccarlos.bilbao%40amd.com%7C7355a7de9b224557d6cf08da10c29e56%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637840725342715892%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=A809HxzZHYiZQ6Arlje0o9KAFt4c77I2Q4vOfCl9%2Fis%3D&reserved=0
> 
> ?

Just fixed a typo in the first patch -I should have included a change log.

Thanks,
Carlos

> 

Re: [PATCH 0/2] x86/mce: Grade new machine errors for AMD MCEs and include messages for panic cases

[Borislav Petkov]

On Mon, Mar 28, 2022 at 08:57:04AM -0500, Carlos Bilbao wrote:
> Just fixed a typo in the first patch -I should have included a change log.

Well, we have the merge window open currently:

Merge window
^^^^^^^^^^^^

Please do not expect large patch series to be handled during the merge
window or even during the week before.  Such patches should be submitted in
mergeable state *at* *least* a week before the merge window opens.
Exceptions are made for bug fixes and *sometimes* for small standalone
drivers for new hardware or minimally invasive patches for hardware
enablement.

During the merge window, the maintainers instead focus on following the
upstream changes, fixing merge window fallout, collecting bug fixes, and
allowing themselves a breath. Please respect that.

The release candidate -rc1 is the starting point for new patches to be
applied which are targeted for the next merge window.

---

and since you're new to this, I'd suggest you take the
time to read Documentation/process/ and especially
Documentation/process/submitting-patches.rst while waiting.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/2] x86/mce: Grade new machine errors for AMD MCEs and include messages for panic cases

[Carlos Bilbao]

On 3/28/2022 9:04 AM, Borislav Petkov wrote:
> On Mon, Mar 28, 2022 at 08:57:04AM -0500, Carlos Bilbao wrote:
>> Just fixed a typo in the first patch -I should have included a change log.
> 
> Well, we have the merge window open currently:
> 
> Merge window
> ^^^^^^^^^^^^
> 
> Please do not expect large patch series to be handled during the merge
> window or even during the week before.  Such patches should be submitted in
> mergeable state *at* *least* a week before the merge window opens.
> Exceptions are made for bug fixes and *sometimes* for small standalone
> drivers for new hardware or minimally invasive patches for hardware
> enablement.
> 
> During the merge window, the maintainers instead focus on following the
> upstream changes, fixing merge window fallout, collecting bug fixes, and
> allowing themselves a breath. Please respect that.
> 
> The release candidate -rc1 is the starting point for new patches to be
> applied which are targeted for the next merge window.
> 
> ---
> 
> and since you're new to this, I'd suggest you take the
> time to read Documentation/process/ and especially
> Documentation/process/submitting-patches.rst while waiting.
> 
> Thx.
> 

Thanks Borislav -would you like me to resend this once -rc1 opens?

Best,
Carlos

Re: [PATCH 0/2] x86/mce: Grade new machine errors for AMD MCEs and include messages for panic cases

[Borislav Petkov]

On Mon, Mar 28, 2022 at 09:09:04AM -0500, Carlos Bilbao wrote:
> Thanks Borislav -would you like me to resend this once -rc1 opens?

No need - I have it already. Btw, those documents also explain when
to resend: when the patches need to be changed or when they've been
forgotten. Most people usually send a "ping email" as a reply to the
thread to remind the maintainer to handle them, in that latter case.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH 0/2] x86/mce: Grade new machine errors for AMD MCEs and include messages for panic cases

[Carlos Bilbao]

This patchset includes grading of new types of machine errors on AMD's MCE
grading logic mce_severity_amd(), which helps the MCE handler determine
what actions to take. If the error is graded as a PANIC, the EDAC driver
will not decode; so we also include new error messages to describe the MCE
and help debugging critical errors.

Carlos Bilbao (2):
  x86/mce: Extend AMD severity grading function with new types of errors
  x86/mce: Add messages to describe panic machine errors on AMD's MCEs grading
 
 arch/x86/include/asm/mce.h         |   6 +
 arch/x86/kernel/cpu/mce/severity.c | 203 ++++++++++++++++++++++++-----
 2 files changed, 174 insertions(+), 35 deletions(-)

-- 
2.27.0