diff mbox series

x86/mce: Unify vendors grading logic and provide AMD machine error checks

Message ID 20220308184133.712761-1-carlos.bilbao@amd.com (mailing list archive)
State New, archived
Headers show
Series x86/mce: Unify vendors grading logic and provide AMD machine error checks | expand

Commit Message

Carlos Bilbao March 8, 2022, 6:41 p.m. UTC
AMD's severity grading covers very few machine errors. In the graded cases
there are no user-readable messages, complicating debugging of critical
hardware errors. Furthermore, with the current implementation AMD MCEs have
no support for the severities-coverage file. Adding new severities for AMD
with the current logic would be too convoluted.

Fix the above issues including AMD severities to the severity table, in
combination with Intel MCEs. Unify the severity grading logic of both
vendors. Label the vendor-specific cases (e.g. cases with different
registers) where checks cannot be implicit with the available features.

Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
---
 arch/x86/include/asm/mce.h         |   7 ++
 arch/x86/kernel/cpu/mce/severity.c | 188 +++++++++++++++--------------
 2 files changed, 103 insertions(+), 92 deletions(-)


base-commit: 7f1b8e0d6360178e3527d4f14e6921c254a86035

Comments

Borislav Petkov March 8, 2022, 7:32 p.m. UTC | #1
On Tue, Mar 08, 2022 at 12:41:34PM -0600, Carlos Bilbao wrote:
> AMD's severity grading covers very few machine errors. In the graded cases
> there are no user-readable messages, complicating debugging of critical
> hardware errors. Furthermore, with the current implementation AMD MCEs have
> no support for the severities-coverage file. Adding new severities for AMD
> with the current logic would be too convoluted.
> 
> Fix the above issues including AMD severities to the severity table, in
> combination with Intel MCEs. Unify the severity grading logic of both
> vendors. Label the vendor-specific cases (e.g. cases with different
> registers) where checks cannot be implicit with the available features.
> 
> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
> ---
>  arch/x86/include/asm/mce.h         |   7 ++
>  arch/x86/kernel/cpu/mce/severity.c | 188 +++++++++++++++--------------
>  2 files changed, 103 insertions(+), 92 deletions(-)

Sorry, maybe you're too new to this and you probably haven't read the
old discussions we have had about the severity grading turd. In order to
save you some time: adding more to that macro insanity is not going to
happen.

The AMD severity grading functions are *actually* readable vs this
abomination which I hate with passion.

If you want to add more logic, you should add to mce_severity_amd(),
perhaps call other helper functions which grade based on a certain
aspect of the error type, split the logic, use comments, etc, but
*definitely* not this.

Thx.
Carlos Bilbao March 9, 2022, 4:03 p.m. UTC | #2
On 3/8/2022 1:32 PM, Borislav Petkov wrote:
> On Tue, Mar 08, 2022 at 12:41:34PM -0600, Carlos Bilbao wrote:
>> AMD's severity grading covers very few machine errors. In the graded cases
>> there are no user-readable messages, complicating debugging of critical
>> hardware errors. Furthermore, with the current implementation AMD MCEs have
>> no support for the severities-coverage file. Adding new severities for AMD
>> with the current logic would be too convoluted.
>>
>> Fix the above issues including AMD severities to the severity table, in
>> combination with Intel MCEs. Unify the severity grading logic of both
>> vendors. Label the vendor-specific cases (e.g. cases with different
>> registers) where checks cannot be implicit with the available features.
>>
>> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
>> ---
>>  arch/x86/include/asm/mce.h         |   7 ++
>>  arch/x86/kernel/cpu/mce/severity.c | 188 +++++++++++++++--------------
>>  2 files changed, 103 insertions(+), 92 deletions(-)
> 
> Sorry, maybe you're too new to this and you probably haven't read the
> old discussions we have had about the severity grading turd. In order to
> save you some time: adding more to that macro insanity is not going to
> happen.
> 
> The AMD severity grading functions are *actually* readable vs this
> abomination which I hate with passion.
> 
> If you want to add more logic, you should add to mce_severity_amd(),
> perhaps call other helper functions which grade based on a certain
> aspect of the error type, split the logic, use comments, etc, but
> *definitely* not this.
> 
> Thx.
> 

Understood, sending a new patch in that direction.

Thanks,
Carlos
diff mbox series

Patch

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index cc73061e7255..d83adc2da522 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -50,6 +50,13 @@ 
 #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_T_DATA 0x4  /* Transaction type of error is Data */
+
+#define ERRORCODE_M_MSK GENMASK(7, 4)  /* Mask for memory transaction type */
+#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..b0fd3feb1b74 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -34,6 +34,7 @@ 
 enum context { IN_KERNEL = 1, IN_USER = 2, IN_KERNEL_RECOV = 3 };
 enum ser { SER_REQUIRED = 1, NO_SER = 2 };
 enum exception { EXCP_CONTEXT = 1, NO_EXCP = 2 };
+enum vendor { INTEL_VENDOR = 1, AMD_VENDOR = 2 };
 
 static struct severity {
 	u64 mask;
@@ -48,6 +49,9 @@  static struct severity {
 	unsigned char cpu_model;
 	unsigned char cpu_minstepping;
 	unsigned char bank_lo, bank_hi;
+	unsigned char vendor;
+	unsigned char succor;
+	unsigned char overflow_recov;
 	char *msg;
 } severities[] = {
 #define MCESEV(s, m, c...) { .sev = MCE_ ## s ## _SEVERITY, .msg = m, ## c }
@@ -60,6 +64,9 @@  static struct severity {
 #define  NOSER		.ser = NO_SER
 #define  EXCP		.excp = EXCP_CONTEXT
 #define  NOEXCP		.excp = NO_EXCP
+#define  AMD_MCE	.vendor = AMD_VENDOR
+#define  SUCCOR		.succor = 1
+#define  OVERF_RECOV	.overflow_recov = 1
 #define  BITCLR(x)	.mask = x, .result = 0
 #define  BITSET(x)	.mask = x, .result = x
 #define  MCGMASK(x, y)	.mcgmask = x, .mcgres = y
@@ -77,6 +84,14 @@  static struct severity {
 		NO, "Not enabled",
 		EXCP, BITCLR(MCI_STATUS_EN)
 		),
+	MCESEV(
+		AR, "Action required: Error detected on a scrub operation",
+		BITSET(MCI_STATUS_SCRUB | MCI_STATUS_DEFERRED), AMD_MCE
+		),
+	MCESEV(
+		DEFERRED, "Deferred",
+		BITSET(MCI_STATUS_DEFERRED), AMD_MCE
+		),
 	MCESEV(
 		PANIC, "Processor context corrupt",
 		BITSET(MCI_STATUS_PCC)
@@ -103,6 +118,35 @@  static struct severity {
 		KEEP, "Corrected error",
 		NOSER, BITCLR(MCI_STATUS_UC)
 		),
+	MCESEV(
+		AR, "Action required: Attempt to consume poisoned data from user process",
+		BITSET(MCI_STATUS_POISON), AMD_MCE, USER
+		),
+	MCESEV(
+		PANIC,
+		"Attempt to consume poisoned memory in kernel context",
+		MASK(MCI_STATUS_POISON | MCI_STATUS_TCC, MCI_STATUS_POISON),
+		AMD_MCE, KERNEL
+		),
+
+#ifdef	CONFIG_MEMORY_FAILURE
+	MCESEV(
+		AR,
+		"Action required: Attempt to consume poisoned memory in recoverable kernel area",
+		MASK(MCI_STATUS_POISON | MCI_STATUS_TCC, MCI_STATUS_POISON),
+		AMD_MCE, KERNEL_RECOV, SUCCOR
+		),
+#endif
+	MCESEV(
+		PANIC,
+		"Attempt to consume poisoned memory in recoverable kernel area",
+		MASK(MCI_STATUS_POISON | MCI_STATUS_TCC, MCI_STATUS_POISON),
+		AMD_MCE, KERNEL_RECOV
+		),
+	MCESEV(
+		PANIC, "Uncorrectable error in kernel context",
+		KERNEL, BITSET(MCI_STATUS_UC), AMD_MCE
+		),
 	/*
 	 * known AO MCACODs reported via MCE or CMC:
 	 *
@@ -163,16 +207,31 @@  static struct severity {
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_DATA),
 		KERNEL_RECOV
 		),
+	MCESEV(
+		AR, "Action required: data load in error recoverable area of kernel",
+		MASK(MCI_STATUS_UC | ERRORCODE_T_MSK, MCI_STATUS_UC | ERRORCODE_T_DATA),
+		KERNEL_RECOV, AMD_MCE, SUCCOR
+		),
 	MCESEV(
 		AR, "Action required: data load error in a user process",
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_DATA),
 		USER
 		),
+	MCESEV(
+		AR, "Action required: data load error in a user process",
+		MASK(MCI_STATUS_UC | ERRORCODE_T_MSK, MCI_STATUS_UC | ERRORCODE_T_DATA),
+		USER, AMD_MCE
+		),
 	MCESEV(
 		AR, "Action required: instruction fetch error in a user process",
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_INSTR),
 		USER
 		),
+	MCESEV(
+		AR, "Action required: instruction fetch in a user process",
+		MASK(MCI_STATUS_UC | ERRORCODE_M_MSK, MCI_STATUS_UC | ERRORCODE_M_FETCH),
+		USER, AMD_MCE
+		),
 	MCESEV(
 		PANIC, "Data load in unrecoverable area of kernel",
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_DATA),
@@ -184,6 +243,21 @@  static struct severity {
 		KERNEL
 		),
 #endif
+	MCESEV(
+		PANIC, "Instruction fetch error in kernel",
+		MASK(MCI_STATUS_UC | ERRORCODE_M_MSK, MCI_STATUS_UC | ERRORCODE_M_FETCH),
+		KERNEL, AMD_MCE
+		),
+	MCESEV(
+		PANIC, "Data load in urecoverable area of kernel",
+		MASK(MCI_STATUS_UC | ERRORCODE_T_MSK, MCI_STATUS_UC | ERRORCODE_T_DATA),
+		KERNEL, AMD_MCE
+		),
+	MCESEV(
+		PANIC, "Uncorrectable error: Data load in error recoverable area of kernel",
+		MASK(MCI_STATUS_UC | ERRORCODE_T_MSK, MCI_STATUS_UC | ERRORCODE_T_DATA),
+		KERNEL_RECOV, AMD_MCE
+		),
 	MCESEV(
 		PANIC, "Action required: unknown MCACOD",
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR)
@@ -197,7 +271,16 @@  static struct severity {
 		SOME, "Action optional with lost events",
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_STATUS_OVER|MCI_UC_S)
 		),
-
+	/*
+	 * On newer AMD systems where overflow_recov is present, we should
+	 * not panic right away when an error overflow occurs. Instead, we
+	 * can try to at least kill the process to prolong system operation.
+	 */
+	MCESEV(
+		AR, "Uncorrected Recoverable Error",
+		BITSET(MCI_STATUS_OVER|MCI_STATUS_UC),
+		SUCCOR, OVERF_RECOV
+		),
 	MCESEV(
 		PANIC, "Overflowed uncorrected",
 		BITSET(MCI_STATUS_OVER|MCI_STATUS_UC)
@@ -301,94 +384,24 @@  static noinstr int error_context(struct mce *m, struct pt_regs *regs)
 	}
 }
 
-static __always_inline int mce_severity_amd_smca(struct mce *m, enum context err_ctx)
-{
-	u64 mcx_cfg;
-
-	/*
-	 * We need to look at the following bits:
-	 * - "succor" bit (data poisoning support), and
-	 * - TCC bit (Task Context Corrupt)
-	 * in MCi_STATUS to determine error severity.
-	 */
-	if (!mce_flags.succor)
-		return MCE_PANIC_SEVERITY;
-
-	mcx_cfg = mce_rdmsrl(MSR_AMD64_SMCA_MCx_CONFIG(m->bank));
-
-	/* TCC (Task context corrupt). If set and if IN_KERNEL, panic. */
-	if ((mcx_cfg & MCI_CONFIG_MCAX) &&
-	    (m->status & MCI_STATUS_TCC) &&
-	    (err_ctx == IN_KERNEL))
-		return MCE_PANIC_SEVERITY;
-
-	 /* ...otherwise invoke hwpoison handler. */
-	return MCE_AR_SEVERITY;
-}
-
-/*
- * See AMD Error Scope Hierarchy table in a newer BKDG. For example
- * 49125_15h_Models_30h-3Fh_BKDG.pdf, section "RAS Features"
- */
-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);
-
-	/* Processor Context Corrupt, no need to fumble too much, die! */
-	if (m->status & MCI_STATUS_PCC)
-		return MCE_PANIC_SEVERITY;
-
-	if (m->status & MCI_STATUS_UC) {
-
-		if (ctx == IN_KERNEL)
-			return MCE_PANIC_SEVERITY;
-
-		/*
-		 * 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);
-
-			/* 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;
-		}
-
-		/*
-		 * For any other case, return MCE_UC_SEVERITY so that we log the
-		 * error and exit #MC handler.
-		 */
-		return MCE_UC_SEVERITY;
-	}
-
-	/*
-	 * deferred error: poll handler catches these and adds to mce_ring so
-	 * memory-failure can take recovery actions.
-	 */
-	if (m->status & MCI_STATUS_DEFERRED)
-		return MCE_DEFERRED_SEVERITY;
-
-	/*
-	 * corrected error: poll handler catches these and passes responsibility
-	 * of decoding the error to EDAC
-	 */
-	return MCE_KEEP_SEVERITY;
-}
-
-static noinstr int mce_severity_intel(struct mce *m, struct pt_regs *regs, char **msg, bool is_excp)
+int noinstr mce_severity(struct mce *m, struct pt_regs *regs, char **msg, bool is_excp)
 {
 	enum exception excp = (is_excp ? EXCP_CONTEXT : NO_EXCP);
 	enum context ctx = error_context(m, regs);
+	enum vendor cpu_vendor = INTEL_VENDOR;
 	struct severity *s;
 
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
+	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
+		cpu_vendor = AMD_VENDOR;
+
 	for (s = severities;; s++) {
+		if (s->vendor && s->vendor != cpu_vendor)
+			continue;
+		if (s->succor && !mce_flags.succor)
+			continue;
+		if (s->overflow_recov && !mce_flags.overflow_recov)
+			continue;
 		if ((m->status & s->mask) != s->result)
 			continue;
 		if ((m->mcgstatus & s->mcgmask) != s->mcgres)
@@ -418,15 +431,6 @@  static noinstr int mce_severity_intel(struct mce *m, struct pt_regs *regs, char
 	}
 }
 
-int noinstr mce_severity(struct mce *m, struct pt_regs *regs, char **msg, bool is_excp)
-{
-	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
-	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
-		return mce_severity_amd(m, regs, msg, is_excp);
-	else
-		return mce_severity_intel(m, regs, msg, is_excp);
-}
-
 #ifdef CONFIG_DEBUG_FS
 static void *s_start(struct seq_file *f, loff_t *pos)
 {