[PATCH v1 0/3] Update SMCA Error Decoding for AMD EPYC Processors

[PATCH v1 0/3] Update SMCA Error Decoding for AMD EPYC Processors

[Avadhut Naik]

Modern AMD EPYC processors support Scalable MCA (SMCA) Error decoding.
Currently however, on Family 19h and 1Ah based, AMD EPYC processors, not
all SMCA errors are being decoded. This patchset attempts to address the
very issue by updating error description structures and handling errata
of some SMCA bank types.

The first patch adds new error descriptions for various SMCA bank types
while also rewording existing and removing unused error descriptions.

The second patch handles the mismatch, encountered on some AMD CPUs, between
the HWID read from the MCA_IPID register and the HWID expected by the kernel
for XGMI Controller SMCA bank type during SMCA initialization.

The third patch tackles the erratum no. 1384, encountered on Genoa and a
few other CPUs due to bit reassignments in Control register of the Coherent
Slave (CS) SMCA bank type.

Avadhut Naik (2):
  x86/MCE/AMD: Add HWID Fixup for PCS_XGMI SMCA
  x86/MCE/AMD: Handle reassigned bit definitions for CS SMCA

Yazen Ghannam (1):
  EDAC/mce_amd: Update SMCA bank error descriptions

 arch/x86/include/asm/mce.h    |  1 +
 arch/x86/kernel/cpu/mce/amd.c | 50 +++++++++++++++++++++-
 drivers/edac/mce_amd.c        | 79 +++++++++++++++++++++++++----------
 3 files changed, 107 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH v1 1/3] EDAC/mce_amd: Update SMCA bank error descriptions

[Avadhut Naik]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Add new error descriptions to extend existing SMCA error decoding
functionality for modern AMD processors. Additionally, also reword
some existing error descriptions and remove the unused ones.

[avadnaik: Add error descriptions for MPDMA SMCA bank, rework the commit
message]

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Avadhut Naik <avadnaik@amd.com>
---
 drivers/edac/mce_amd.c | 53 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index cc5c63feb26a..869dcca5e2f4 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -192,24 +192,24 @@ static const char * const smca_ls2_mce_desc[] = {
 	"A SystemReadDataError error was reported on read data returned from L2 for an SCB store",
 	"A SystemReadDataError error was reported on read data returned from L2 for a WCB store",
 	"A hardware assertion error was reported",
-	"A parity error was detected in an STLF, SCB EMEM entry or SRB store data by any access",
+	"A parity error was detected in an STLF, SCB EMEM entry, store data mask or SRB store data by any access",
 };
 
 static const char * const smca_if_mce_desc[] = {
-	"Op Cache Microtag Probe Port Parity Error",
+	"Op Cache Microtag Parity Error",
 	"IC Microtag or Full Tag Multi-hit Error",
 	"IC Full Tag Parity Error",
 	"IC Data Array Parity Error",
-	"Decoupling Queue PhysAddr Parity Error",
+	"PRQ Parity Error",
 	"L0 ITLB Parity Error",
-	"L1 ITLB Parity Error",
-	"L2 ITLB Parity Error",
+	"L1-TLB Parity Error",
+	"L2-TLB Parity Error",
 	"BPQ Thread 0 Snoop Parity Error",
 	"BPQ Thread 1 Snoop Parity Error",
-	"L1 BTB Multi-Match Error",
-	"L2 BTB Multi-Match Error",
+	"BP L1-BTB Multi-Hit Error",
+	"BP L2-BTB Multi-Hit Error",
 	"L2 Cache Response Poison Error",
-	"System Read Data Error",
+	"L2 Cache Error Response",
 	"Hardware Assertion Error",
 	"L1-TLB Multi-Hit",
 	"L2-TLB Multi-Hit",
@@ -222,12 +222,13 @@ static const char * const smca_l2_mce_desc[] = {
 	"L2M Tag or State Array ECC Error",
 	"L2M Data Array ECC Error",
 	"Hardware Assert Error",
+	"SDP Read Response Parity Error",
 };
 
 static const char * const smca_de_mce_desc[] = {
-	"Micro-op cache tag parity error",
-	"Micro-op cache data parity error",
-	"Instruction buffer parity error",
+	"Micro-op cache tag array parity error",
+	"Micro-op cache data array parity error",
+	"IBB Register File parity error",
 	"Micro-op queue parity error",
 	"Instruction dispatch queue parity error",
 	"Fetch address FIFO parity error",
@@ -247,7 +248,7 @@ static const char * const smca_ex_mce_desc[] = {
 	"Checkpoint queue parity error",
 	"Retire dispatch queue parity error",
 	"Retire status queue parity error",
-	"Scheduling queue parity error",
+	"Scheduler queue parity error",
 	"Branch buffer queue parity error",
 	"Hardware Assertion error",
 	"Spec Map parity error",
@@ -262,6 +263,7 @@ static const char * const smca_fp_mce_desc[] = {
 	"Retire queue (RQ) parity error",
 	"Status register file (SRF) parity error",
 	"Hardware assertion",
+	"Physical K mask register file (KRF) parity error",
 };
 
 static const char * const smca_l3_mce_desc[] = {
@@ -270,9 +272,10 @@ static const char * const smca_l3_mce_desc[] = {
 	"L3M Tag ECC Error",
 	"L3M Tag Multi-way-hit Error",
 	"L3M Data ECC Error",
-	"SDP Parity Error or SystemReadDataError from XI",
-	"L3 Victim Queue Parity Error",
+	"SDP Parity Error from XI",
+	"L3 Victim Queue Data Fabric Error",
 	"L3 Hardware Assertion",
+	"XI WCB Parity Poison Creation Event",
 };
 
 static const char * const smca_cs_mce_desc[] = {
@@ -302,6 +305,10 @@ static const char * const smca_cs2_mce_desc[] = {
 	"SDP read response had an unexpected RETRY error",
 	"Counter overflow error",
 	"Counter underflow error",
+	"Illegal Request on the no data channel",
+	"Address Violation on the no data channel",
+	"Security Violation on the no data channel",
+	"Hardware Assert Error",
 };
 
 static const char * const smca_pie_mce_desc[] = {
@@ -309,7 +316,9 @@ static const char * const smca_pie_mce_desc[] = {
 	"Register security violation",
 	"Link Error",
 	"Poison data consumption",
-	"A deferred error was detected in the DF"
+	"A deferred error was detected in the DF",
+	"Watch Dog Timer",
+	"An SRAM ECC error was detected in the CNLI block",
 };
 
 static const char * const smca_umc_mce_desc[] = {
@@ -321,6 +330,10 @@ static const char * const smca_umc_mce_desc[] = {
 	"Write data CRC error",
 	"DCQ SRAM ECC error",
 	"AES SRAM ECC error",
+	"ECS Row Error",
+	"ECS Error",
+	"UMC Throttling Error",
+	"Read CRC Error",
 };
 
 static const char * const smca_umc2_mce_desc[] = {
@@ -443,17 +456,12 @@ static const char * const smca_mpdma_mce_desc[] = {
 	"MPDMA TVF SDP Master Memory 4 ECC or parity error",
 	"MPDMA TVF SDP Master Memory 5 ECC or parity error",
 	"MPDMA TVF SDP Master Memory 6 ECC or parity error",
+	"SDP Watchdog Timer expired",
 	"MPDMA PTE Command FIFO ECC or parity error",
 	"MPDMA PTE Hub Data FIFO ECC or parity error",
 	"MPDMA PTE Internal Data FIFO ECC or parity error",
 	"MPDMA PTE Command Memory DMA ECC or parity error",
 	"MPDMA PTE Command Memory Internal ECC or parity error",
-	"MPDMA PTE DMA Completion FIFO ECC or parity error",
-	"MPDMA PTE Tablewalk Completion FIFO ECC or parity error",
-	"MPDMA PTE Descriptor Completion FIFO ECC or parity error",
-	"MPDMA PTE ReadOnly Completion FIFO ECC or parity error",
-	"MPDMA PTE DirectWrite Completion FIFO ECC or parity error",
-	"SDP Watchdog Timer expired",
 };
 
 static const char * const smca_nbio_mce_desc[] = {
@@ -461,7 +469,8 @@ static const char * const smca_nbio_mce_desc[] = {
 	"PCIE error",
 	"SDP ErrEvent error",
 	"SDP Egress Poison Error",
-	"IOHC Internal Poison Error",
+	"Internal Poison Error",
+	"Internal system fatal error event",
 };
 
 static const char * const smca_pcie_mce_desc[] = {
-- 
2.34.1

[PATCH v1 2/3] x86/MCE/AMD: Add HWID Fixup for PCS_XGMI SMCA

[Avadhut Naik]

On AMD systems, during Scalable MCA (SMCA) initialization, the HWID and
McaType tuple, read from MCA_IPID register of a SMCA bank type, is used
by the kernel for populating the per-CPU smca_banks array. This very array
is, in turn being utilized by the edac_mce_amd module for determining the
SMCA bank type while decoding a machine check error.

However, on some AMD CPUs, the HWID read from the MCA_IPID register
for XGMI Controller SMCA bank type does not match the value expected by
the kernel. Consequently, the smca_banks array is not populated for the
bank type resulting in the machine check errors on the bank type not being
decoded.

As a solution, set the HWID, obtained from the MCA_IPID register, of the
XGMI Controller SMCA bank type on affected CPUs, to the value expected by
the kernel to ensure that the machine check errors on the bank type are
correctly decoded.

Signed-off-by: Avadhut Naik <avadnaik@amd.com>
---
 arch/x86/kernel/cpu/mce/amd.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 23c5072fbbb7..b0cce0ce056c 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -249,6 +249,30 @@ static void default_deferred_error_interrupt(void)
 }
 void (*deferred_error_int_vector)(void) = default_deferred_error_interrupt;
 
+/*
+ * Errata encountered on AMD CPUs for some SMCA bank types requires fixup
+ * of HWID, read from MCA_IPID register, for accurate SMCA error decoding.
+ */
+static inline void fixup_hwid(unsigned int *hwid_mcatype)
+{
+	struct cpuinfo_x86 *c = &boot_cpu_data;
+
+	if (c->x86 == 0x19) {
+		switch (c->x86_model) {
+		/*
+		 * Handle discrepancy in HWID of kernel and MCA_IPID register
+		 * for XGMI Controller SMCA bank type
+		 */
+		case 0x30 ... 0x3F:
+			if (*hwid_mcatype == HWID_MCATYPE(0x80, 0x0))
+				*hwid_mcatype = HWID_MCATYPE(0x50, 0x0);
+			break;
+		default:
+			break;
+		}
+	}
+}
+
 static void smca_set_misc_banks_map(unsigned int bank, unsigned int cpu)
 {
 	u32 low, high;
@@ -321,6 +345,8 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
 	hwid_mcatype = HWID_MCATYPE(high & MCI_IPID_HWID,
 				    (high & MCI_IPID_MCATYPE) >> 16);
 
+	fixup_hwid(&hwid_mcatype);
+
 	for (i = 0; i < ARRAY_SIZE(smca_hwid_mcatypes); i++) {
 		s_hwid = &smca_hwid_mcatypes[i];
 
-- 
2.34.1

[PATCH v1 3/3] x86/MCE/AMD: Handle reassigned bit definitions for CS SMCA

[Avadhut Naik]

Currently, on AMD systems with Scalable MCA (SMCA), each machine check
error of a SMCA bank type has an associated bit position in the bank's
control (CTL) register used for enabling / disabling reporting of the
very error. An error's bit position in the CTL register is also used
during error decoding for offsetting into the corresponding bank's error
description structure. As new errors are being added in newer AMD systems
for existing SMCA bank types, the underlying SMCA architecture guarantees
that the bit positions of existing errors are not altered.

However, on some AMD systems viz. Genoa, some of the existing bit
definitions in the CTL register of the Coherent Slave (CS) SMCA bank type
are reassigned without defining new HWID and McaType. Consequently, the
very errors whose bit definitions have been reassigned in the CTL register
are being erroneously decoded.

As a solution, create a new software defined SMCA bank type by utilizing
one of the hardware-reserved values for HWID. The new SMCA bank type will
only be employed for CS error decoding on affected CPU models.

Additionally, since the existing error description structure for the CS
SMCA bank type is still valid, add new error description structure to
compensate for the reassigned bit definitions.

Signed-off-by: Avadhut Naik <avadnaik@amd.com>
---
 arch/x86/include/asm/mce.h    |  1 +
 arch/x86/kernel/cpu/mce/amd.c | 24 +++++++++++++++++++++++-
 drivers/edac/mce_amd.c        | 26 ++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 9646ed6e8c0b..d0442b4147b5 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -308,6 +308,7 @@ enum smca_bank_types {
 	SMCA_L3_CACHE,	/* L3 Cache */
 	SMCA_CS,	/* Coherent Slave */
 	SMCA_CS_V2,
+	SMCA_CS_V2_QUIRK,
 	SMCA_PIE,	/* Power, Interrupts, etc. */
 	SMCA_UMC,	/* Unified Memory Controller */
 	SMCA_UMC_V2,
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index b0cce0ce056c..317307772048 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -101,7 +101,7 @@ static struct smca_bank_name smca_names[] = {
 	[SMCA_EX]			= { "execution_unit",	"Execution Unit" },
 	[SMCA_FP]			= { "floating_point",	"Floating Point Unit" },
 	[SMCA_L3_CACHE]			= { "l3_cache",		"L3 Cache" },
-	[SMCA_CS ... SMCA_CS_V2]	= { "coherent_slave",	"Coherent Slave" },
+	[SMCA_CS ... SMCA_CS_V2_QUIRK]	= { "coherent_slave",	"Coherent Slave" },
 	[SMCA_PIE]			= { "pie",		"Power, Interrupts, etc." },
 
 	/* UMC v2 is separate because both of them can exist in a single system. */
@@ -178,6 +178,8 @@ static const struct smca_hwid smca_hwid_mcatypes[] = {
 	{ SMCA_CS,	 HWID_MCATYPE(0x2E, 0x0)	},
 	{ SMCA_PIE,	 HWID_MCATYPE(0x2E, 0x1)	},
 	{ SMCA_CS_V2,	 HWID_MCATYPE(0x2E, 0x2)	},
+	/* Software defined SMCA bank type to handle erratum 1384*/
+	{ SMCA_CS_V2_QUIRK, HWID_MCATYPE(0x0, 0x1)  },
 
 	/* Unified Memory Controller MCA type */
 	{ SMCA_UMC,	 HWID_MCATYPE(0x96, 0x0)	},
@@ -259,6 +261,17 @@ static inline void fixup_hwid(unsigned int *hwid_mcatype)
 
 	if (c->x86 == 0x19) {
 		switch (c->x86_model) {
+		/*
+		 * Per Genoa's revision guide, erratum 1384, some SMCA Extended
+		 * Error Codes and SMCA Control bits are incorrect for SMCA CS
+		 * bank type.
+		 */
+		case 0x10 ... 0x1F:
+		case 0x60 ... 0x7B:
+		case 0xA0 ... 0xAF:
+			if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2))
+				*hwid_mcatype = HWID_MCATYPE(0x0, 0x1);
+			break;
 		/*
 		 * Handle discrepancy in HWID of kernel and MCA_IPID register
 		 * for XGMI Controller SMCA bank type
@@ -270,6 +283,15 @@ static inline void fixup_hwid(unsigned int *hwid_mcatype)
 		default:
 			break;
 		}
+	} else if (c->x86 == 0x1A) {
+		switch (c->x86_model) {
+		case 0x40 ... 0x4F:
+			if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2))
+				*hwid_mcatype = HWID_MCATYPE(0x0, 0x1);
+			break;
+		default:
+			break;
+		}
 	}
 }
 
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index 869dcca5e2f4..0586356475fd 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -311,6 +311,31 @@ static const char * const smca_cs2_mce_desc[] = {
 	"Hardware Assert Error",
 };
 
+/*
+ * Per Genoa's revision guide, erratum 1384, existing bit definitions
+ * are reassigned for SMCA CS bank type.
+ */
+static const char * const smca_cs2_quirk_mce_desc[] = {
+	"Illegal Request",
+	"Address Violation",
+	"Security Violation",
+	"Illegal Response",
+	"Unexpected Response",
+	"Request or Probe Parity Error",
+	"Read Response Parity Error",
+	"Atomic Request Parity Error",
+	"SDP read response had no match in the CS queue",
+	"SDP read response had an unexpected RETRY error",
+	"Counter overflow error",
+	"Counter underflow error",
+	"Probe Filter Protocol Error",
+	"Probe Filter ECC Error",
+	"Illegal Request on the no data channel",
+	"Address Violation on the no data channel",
+	"Security Violation on the no data channel",
+	"Hardware Assert Error",
+};
+
 static const char * const smca_pie_mce_desc[] = {
 	"Hardware Assert",
 	"Register security violation",
@@ -602,6 +627,7 @@ static struct smca_mce_desc smca_mce_descs[] = {
 	[SMCA_L3_CACHE]	= { smca_l3_mce_desc,	ARRAY_SIZE(smca_l3_mce_desc)	},
 	[SMCA_CS]	= { smca_cs_mce_desc,	ARRAY_SIZE(smca_cs_mce_desc)	},
 	[SMCA_CS_V2]	= { smca_cs2_mce_desc,	ARRAY_SIZE(smca_cs2_mce_desc)	},
+	[SMCA_CS_V2_QUIRK] = { smca_cs2_quirk_mce_desc, ARRAY_SIZE(smca_cs2_quirk_mce_desc)},
 	[SMCA_PIE]	= { smca_pie_mce_desc,	ARRAY_SIZE(smca_pie_mce_desc)	},
 	[SMCA_UMC]	= { smca_umc_mce_desc,	ARRAY_SIZE(smca_umc_mce_desc)	},
 	[SMCA_UMC_V2]	= { smca_umc2_mce_desc,	ARRAY_SIZE(smca_umc2_mce_desc)	},
-- 
2.34.1

Re: [PATCH v1 3/3] x86/MCE/AMD: Handle reassigned bit definitions for CS SMCA

[Ingo Molnar]

* Avadhut Naik <avadnaik@amd.com> wrote:

> @@ -178,6 +178,8 @@ static const struct smca_hwid smca_hwid_mcatypes[] = {
>  	{ SMCA_CS,	 HWID_MCATYPE(0x2E, 0x0)	},
>  	{ SMCA_PIE,	 HWID_MCATYPE(0x2E, 0x1)	},
>  	{ SMCA_CS_V2,	 HWID_MCATYPE(0x2E, 0x2)	},
> +	/* Software defined SMCA bank type to handle erratum 1384*/
> +	{ SMCA_CS_V2_QUIRK, HWID_MCATYPE(0x0, 0x1)  },
>  
>  	/* Unified Memory Controller MCA type */
>  	{ SMCA_UMC,	 HWID_MCATYPE(0x96, 0x0)	},
> @@ -259,6 +261,17 @@ static inline void fixup_hwid(unsigned int *hwid_mcatype)
>  
>  	if (c->x86 == 0x19) {
>  		switch (c->x86_model) {
> +		/*
> +		 * Per Genoa's revision guide, erratum 1384, some SMCA Extended
> +		 * Error Codes and SMCA Control bits are incorrect for SMCA CS
> +		 * bank type.
> +		 */
> +		case 0x10 ... 0x1F:
> +		case 0x60 ... 0x7B:
> +		case 0xA0 ... 0xAF:
> +			if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2))
> +				*hwid_mcatype = HWID_MCATYPE(0x0, 0x1);

Why are we open-coding these types?

Why not use smca_hwid_mcatypes[SMCA_CS_V2], etc.?

> +			if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2))
> +				*hwid_mcatype = HWID_MCATYPE(0x0, 0x1);

Ditto.

Thanks,

	Ingo

Re: [PATCH v1 3/3] x86/MCE/AMD: Handle reassigned bit definitions for CS SMCA

[Yazen Ghannam]

On Tue, Jan 17, 2023 at 10:23:09AM +0100, Ingo Molnar wrote:
> 
> * Avadhut Naik <avadnaik@amd.com> wrote:
> 
> > @@ -178,6 +178,8 @@ static const struct smca_hwid smca_hwid_mcatypes[] = {
> >  	{ SMCA_CS,	 HWID_MCATYPE(0x2E, 0x0)	},
> >  	{ SMCA_PIE,	 HWID_MCATYPE(0x2E, 0x1)	},
> >  	{ SMCA_CS_V2,	 HWID_MCATYPE(0x2E, 0x2)	},
> > +	/* Software defined SMCA bank type to handle erratum 1384*/
> > +	{ SMCA_CS_V2_QUIRK, HWID_MCATYPE(0x0, 0x1)  },
> >  
> >  	/* Unified Memory Controller MCA type */
> >  	{ SMCA_UMC,	 HWID_MCATYPE(0x96, 0x0)	},
> > @@ -259,6 +261,17 @@ static inline void fixup_hwid(unsigned int *hwid_mcatype)
> >  
> >  	if (c->x86 == 0x19) {
> >  		switch (c->x86_model) {
> > +		/*
> > +		 * Per Genoa's revision guide, erratum 1384, some SMCA Extended
> > +		 * Error Codes and SMCA Control bits are incorrect for SMCA CS
> > +		 * bank type.
> > +		 */
> > +		case 0x10 ... 0x1F:
> > +		case 0x60 ... 0x7B:
> > +		case 0xA0 ... 0xAF:
> > +			if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2))
> > +				*hwid_mcatype = HWID_MCATYPE(0x0, 0x1);
> 
> Why are we open-coding these types?
> 
> Why not use smca_hwid_mcatypes[SMCA_CS_V2], etc.?
> 
> > +			if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2))
> > +				*hwid_mcatype = HWID_MCATYPE(0x0, 0x1);
> 
> Ditto.
>

This code runs before matching on a struct with the enums. It seems simplest
to fixup the hardware-provided value before setting things up rather than
changing things later.

Current code flow:
1) Read HWID/McaType values from hardware.
2) Loop through known types and match on the HWID_MCATYPE() tuple.
3) Continue setup based on matched type and its Linux enum.

New code flow:
1) Read HWID/McaType values from hardware.
--> Fixup HWID/McaType values due to any known hardware quirks.
2) Loop through known types and match on the HWID_MCATYPE() tuple.
3) Continue setup based on matched type and its Linux enum.

What do you think?

Also, a further update (maybe you're alluding to?) is get rid of the struct
smca_hwid and just define an enum with "NAME = HWID_MCATYPE(XXX, YYY)".

The struct smca_hwid had another field that was removed, so it seems
unnecessary at the moment.

Thanks,
Yazen

[PATCH v1 3/3] x86/MCE/AMD: Handle reassigned bit definitions for CS SMCA

[Avadhut Naik]

Hi,

On 1/17/2023 03:23, Ingo Molnar wrote:
> 
> * Avadhut Naik <avadnaik@amd.com> wrote:
> 
>> @@ -178,6 +178,8 @@ static const struct smca_hwid smca_hwid_mcatypes[] = {
>>  	{ SMCA_CS,	 HWID_MCATYPE(0x2E, 0x0)	},
>>  	{ SMCA_PIE,	 HWID_MCATYPE(0x2E, 0x1)	},
>>  	{ SMCA_CS_V2,	 HWID_MCATYPE(0x2E, 0x2)	},
>> +	/* Software defined SMCA bank type to handle erratum 1384*/
>> +	{ SMCA_CS_V2_QUIRK, HWID_MCATYPE(0x0, 0x1)  },
>>  
>>  	/* Unified Memory Controller MCA type */
>>  	{ SMCA_UMC,	 HWID_MCATYPE(0x96, 0x0)	},
>> @@ -259,6 +261,17 @@ static inline void fixup_hwid(unsigned int *hwid_mcatype)
>>  
>>  	if (c->x86 == 0x19) {
>>  		switch (c->x86_model) {
>> +		/*
>> +		 * Per Genoa's revision guide, erratum 1384, some SMCA Extended
>> +		 * Error Codes and SMCA Control bits are incorrect for SMCA CS
>> +		 * bank type.
>> +		 */
>> +		case 0x10 ... 0x1F:
>> +		case 0x60 ... 0x7B:
>> +		case 0xA0 ... 0xAF:
>> +			if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2))
>> +				*hwid_mcatype = HWID_MCATYPE(0x0, 0x1);
> 
> Why are we open-coding these types?
> 
> Why not use smca_hwid_mcatypes[SMCA_CS_V2], etc.?

If I understood correctly (And please rectify me if I didn't), did you mean using something like the below snippet instead of what I have used?

if (*hwid_mcatype == smca_hwid_mcatypes[SMCA_CS_V2].hwid_mcatype)
	*hwid_mcatype = smca_hwid_mcatypes[SMCA_CS_V2_QUIRK].hwid_mcatype;

If yes, then SMCA_CS_V2, SMCA_CS_V2_QUIRK etc. originate from the enum smca_bank_types in arch/x86/include/asm/mce.h.
As the enum stands now, it cannot be used for indexing into the smca_hwid_mcatypes array since it might result in incorrect indexing.

Please advise.

Thanks,
Avadhut Naik
> 
>> +			if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2))
>> +				*hwid_mcatype = HWID_MCATYPE(0x0, 0x1);
> 
> Ditto.
> 
> Thanks,
> 
> 	Ingo

Re: [PATCH v1 3/3] x86/MCE/AMD: Handle reassigned bit definitions for CS SMCA

[Yazen Ghannam]

On Tue, Jan 17, 2023 at 10:23:09AM +0100, Ingo Molnar wrote:
> 
> * Avadhut Naik <avadnaik@amd.com> wrote:
> 
> > @@ -178,6 +178,8 @@ static const struct smca_hwid smca_hwid_mcatypes[] = {
> >  	{ SMCA_CS,	 HWID_MCATYPE(0x2E, 0x0)	},
> >  	{ SMCA_PIE,	 HWID_MCATYPE(0x2E, 0x1)	},
> >  	{ SMCA_CS_V2,	 HWID_MCATYPE(0x2E, 0x2)	},
> > +	/* Software defined SMCA bank type to handle erratum 1384*/
> > +	{ SMCA_CS_V2_QUIRK, HWID_MCATYPE(0x0, 0x1)  },
> >  
> >  	/* Unified Memory Controller MCA type */
> >  	{ SMCA_UMC,	 HWID_MCATYPE(0x96, 0x0)	},
> > @@ -259,6 +261,17 @@ static inline void fixup_hwid(unsigned int *hwid_mcatype)
> >  
> >  	if (c->x86 == 0x19) {
> >  		switch (c->x86_model) {
> > +		/*
> > +		 * Per Genoa's revision guide, erratum 1384, some SMCA Extended
> > +		 * Error Codes and SMCA Control bits are incorrect for SMCA CS
> > +		 * bank type.
> > +		 */
> > +		case 0x10 ... 0x1F:
> > +		case 0x60 ... 0x7B:
> > +		case 0xA0 ... 0xAF:
> > +			if (*hwid_mcatype == HWID_MCATYPE(0x2E, 0x2))
> > +				*hwid_mcatype = HWID_MCATYPE(0x0, 0x1);
> 
> Why are we open-coding these types?
> 
> Why not use smca_hwid_mcatypes[SMCA_CS_V2], etc.?
>

Hi Ingo,
Is this what you mean?

	if (*hwid_mcatype == smca_hwid_mcatypes[SMCA_CS_V2].hwid_mcatype)
		*hwid_mcatype =	smca_hwid_mcatypes[SMCA_CS_V2_QUIRK].hwid_mcatype;

I think that's a good idea.

Avadhut,
Can you please make this change here and in the other patch?

Thanks,
Yazen