[PATCH 0/4] Updates to EDAC and AMD MCE driver

[PATCH 0/4] Updates to EDAC and AMD MCE driver

[Aravind Gopalakrishnan]

This patchset mainly provides necessary EDAC bits to decode errors
occuring on Scalable MCA enabled processors and also updates AMD MCE
driver to get correct MCx_MISC register address for upcoming processors.
Patches 1 ans 2 are meant for the upcoming processors.

Patches 3 and 4 are either fixing or adding comments to help in
understanding the code and do not introduce any functional changes.

Patch 1: Updates to EDAC driver to decode the new error signatures
Patch 2: Fix logic to get correct block address
Patch 3: Fix deferred error comment
Patch 4: Add comments to mce_amd.c to describe functionality

Tested the patches for regressions on Fam15h, Fam10h systems
and found none.

Aravind Gopalakrishnan (4):
  EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors
  x86/mce/AMD: Fix logic to obtain block address
  x86/mce: Clarify comments regarding deferred error
  x86/mce/AMD: Add comments for easier understanding

 arch/x86/include/asm/mce.h           |  52 +++++-
 arch/x86/include/asm/msr-index.h     |   6 +
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 126 ++++++++++----
 drivers/edac/mce_amd.c               | 327 ++++++++++++++++++++++++++++++++++-
 4 files changed, 480 insertions(+), 31 deletions(-)

-- 
2.7.0

[PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors

[Aravind Gopalakrishnan]

For Scalable MCA enabled processors, errors are listed
per IP block. And since it is not required for an IP to
map to a particular bank, we need to use HWID and McaType
values from the MCx_IPID register to figure out which IP
a given bank represents.

We also have a new bit (TCC) in the MCx_STATUS register
to indicate Task context is corrupt.

Add logic here to decode errors from all known IP
blocks for Fam17h Model 00-0fh and to print TCC errors.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 arch/x86/include/asm/mce.h           |  50 ++++++
 arch/x86/include/asm/msr-index.h     |   2 +
 arch/x86/kernel/cpu/mcheck/mce_amd.c |  11 ++
 drivers/edac/mce_amd.c               | 327 ++++++++++++++++++++++++++++++++++-
 4 files changed, 389 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 2ea4527..2ec67ac 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -42,6 +42,17 @@
 /* AMD-specific bits */
 #define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
 #define MCI_STATUS_POISON	(1ULL<<43)  /* access poisonous data */
+#define MCI_STATUS_TCC		(1ULL<<55)  /* Task context corrupt */
+/*
+ * McaX field if set indicates a given bank supports MCA extensions:
+ *  - Deferred error interrupt type is specifiable by bank
+ *  - BlkPtr field indicates prescence of extended MISC registers.
+ *    But should not be used to determine MSR numbers
+ *  - TCC bit is present in MCx_STATUS
+ */
+#define MCI_CONFIG_MCAX		0x1
+#define MCI_IPID_MCATYPE	0xFFFF0000
+#define MCI_IPID_HWID		0xFFF
 
 /*
  * Note that the full MCACOD field of IA32_MCi_STATUS MSR is
@@ -287,4 +298,43 @@ struct cper_sec_mem_err;
 extern void apei_mce_report_mem_error(int corrected,
 				      struct cper_sec_mem_err *mem_err);
 
+/*
+ * Enumerating new IP types and HWID values
+ * in ScalableMCA enabled AMD processors
+ */
+#ifdef CONFIG_X86_MCE_AMD
+enum ip_types {
+	F17H_CORE = 0,	/* Core errors */
+	DF,		/* Data Fabric */
+	UMC,		/* Unified Memory Controller */
+	FUSE,		/* FUSE subsystem */
+	PSP,		/* Platform Security Processor */
+	SMU,		/* System Management Unit */
+	N_IP_TYPES
+};
+
+struct hwid {
+	const char *ipname;
+	unsigned int hwid_value;
+};
+
+extern struct hwid hwid_mappings[N_IP_TYPES];
+
+enum core_mcatypes {
+	LS = 0,		/* Load Store */
+	IF,		/* Instruction Fetch */
+	L2_CACHE,	/* L2 cache */
+	DE,		/* Decoder unit */
+	RES,		/* Reserved */
+	EX,		/* Execution unit */
+	FP,		/* Floating Point */
+	L3_CACHE	/* L3 cache */
+};
+
+enum df_mcatypes {
+	CS = 0,		/* Coherent Slave */
+	PIE		/* Power management, Interrupts, etc */
+};
+#endif
+
 #endif /* _ASM_X86_MCE_H */
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 5523465..93bccbc 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -266,7 +266,9 @@
 
 /* 'SMCA': AMD64 Scalable MCA */
 #define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
+#define MSR_AMD64_SMCA_MC0_IPID		0xc0002005
 #define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
+#define MSR_AMD64_SMCA_MCx_IPID(x)	(MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))
 
 #define MSR_P6_PERFCTR0			0x000000c1
 #define MSR_P6_PERFCTR1			0x000000c2
diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 88de27b..8169103 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -71,6 +71,17 @@ static const char * const th_names[] = {
 	"execution_unit",
 };
 
+/* Defining HWID to IP type mappings for Scalable MCA */
+struct hwid hwid_mappings[] = {
+	[F17H_CORE]	= { "f17h_core", 0xB0 },
+	[DF]		= { "df", 0x2E },
+	[UMC]		= { "umc", 0x96 },
+	[FUSE]		= { "fuse", 0x5 },
+	[PSP]		= { "psp", 0xFF },
+	[SMU]		= { "smu", 0x1 },
+};
+EXPORT_SYMBOL_GPL(hwid_mappings);
+
 static DEFINE_PER_CPU(struct threshold_bank **, threshold_banks);
 static DEFINE_PER_CPU(unsigned char, bank_map);	/* see which banks are on */
 
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index e3a945c..6e6b327 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -147,6 +147,136 @@ static const char * const mc6_mce_desc[] = {
 	"Status Register File",
 };
 
+/* Scalable MCA error strings */
+
+static const char * const f17h_ls_mce_desc[] = {
+	"Load queue parity",
+	"Store queue parity",
+	"Miss address buffer payload parity",
+	"L1 TLB parity",
+	"",						/* reserved */
+	"DC tag error type 6",
+	"DC tag error type 1",
+	"Internal error type 1",
+	"Internal error type 2",
+	"Sys Read data error thread 0",
+	"Sys read data error thread 1",
+	"DC tag error type 2",
+	"DC data error type 1 (poison comsumption)",
+	"DC data error type 2",
+	"DC data error type 3",
+	"DC tag error type 4",
+	"L2 TLB parity",
+	"PDC parity error",
+	"DC tag error type 3",
+	"DC tag error type 5",
+	"L2 fill data error",
+};
+
+static const char * const f17h_if_mce_desc[] = {
+	"microtag probe port parity error",
+	"IC microtag or full tag multi-hit error",
+	"IC full tag parity",
+	"IC data array parity",
+	"Decoupling queue phys addr parity error",
+	"L0 ITLB parity error",
+	"L1 ITLB parity error",
+	"L2 ITLB parity error",
+	"BPQ snoop parity on Thread 0",
+	"BPQ snoop parity on Thread 1",
+	"L1 BTB multi-match error",
+	"L2 BTB multi-match error",
+};
+
+static const char * const f17h_l2_mce_desc[] = {
+	"L2M tag multi-way-hit error",
+	"L2M tag ECC error",
+	"L2M data ECC error",
+	"HW assert",
+};
+
+static const char * const f17h_de_mce_desc[] = {
+	"uop cache tag parity error",
+	"uop cache data parity error",
+	"Insn buffer parity error",
+	"Insn dispatch queue parity error",
+	"Fetch address FIFO parity",
+	"Patch RAM data parity",
+	"Patch RAM sequencer parity",
+	"uop buffer parity"
+};
+
+static const char * const f17h_ex_mce_desc[] = {
+	"Watchdog timeout error",
+	"Phy register file parity",
+	"Flag register file parity",
+	"Immediate displacement register file parity",
+	"Address generator payload parity",
+	"EX payload parity",
+	"Checkpoint queue parity",
+	"Retire dispatch queue parity",
+};
+
+static const char * const f17h_fp_mce_desc[] = {
+	"Physical register file parity",
+	"Freelist parity error",
+	"Schedule queue parity",
+	"NSQ parity error",
+	"Retire queue parity",
+	"Status register file parity",
+};
+
+static const char * const f17h_l3_mce_desc[] = {
+	"Shadow tag macro ECC error",
+	"Shadow tag macro multi-way-hit error",
+	"L3M tag ECC error",
+	"L3M tag multi-way-hit error",
+	"L3M data ECC error",
+	"XI parity, L3 fill done channel error",
+	"L3 victim queue parity",
+	"L3 HW assert",
+};
+
+static const char * const f17h_cs_mce_desc[] = {
+	"Illegal request from transport layer",
+	"Address violation",
+	"Security violation",
+	"Illegal response from transport layer",
+	"Unexpected response",
+	"Parity error on incoming request or probe response data",
+	"Parity error on incoming read response data",
+	"Atomic request parity",
+	"ECC error on probe filter access",
+};
+
+static const char * const f17h_pie_mce_desc[] = {
+	"HW assert",
+	"Internal PIE register security violation",
+	"Error on GMI link",
+	"Poison data written to internal PIE register",
+};
+
+static const char * const f17h_umc_mce_desc[] = {
+	"DRAM ECC error",
+	"Data poison error on DRAM",
+	"SDP parity error",
+	"Advanced peripheral bus error",
+	"Command/address parity error",
+	"Write data CRC error",
+};
+
+static const char * const f17h_fuse_mce_desc[] = {
+	"FUSE RAM ECC error",
+};
+
+static const char * const f17h_psp_mce_desc[] = {
+	"PSP RAM ECC or parity error",
+};
+
+static const char * const f17h_smu_mce_desc[] = {
+	"SMU RAM ECC or parity error",
+};
+
 static bool f12h_mc0_mce(u16 ec, u8 xec)
 {
 	bool ret = false;
@@ -731,6 +861,178 @@ static bool amd_filter_mce(struct mce *m)
 	return false;
 }
 
+static void decode_f17hcore_errors(u8 xec, unsigned int mca_type)
+{
+	switch (mca_type) {
+	case LS:
+		if (xec == 0x4 || xec > (ARRAY_SIZE(f17h_ls_mce_desc) - 1))
+			goto wrong_f17hcore_error;
+
+		pr_cont("%s.\n", f17h_ls_mce_desc[xec]);
+		break;
+
+	case IF:
+		if (xec > (ARRAY_SIZE(f17h_if_mce_desc) - 1))
+			goto wrong_f17hcore_error;
+
+		pr_cont("%s.\n", f17h_if_mce_desc[xec]);
+		break;
+
+	case L2_CACHE:
+		if (xec > (ARRAY_SIZE(f17h_l2_mce_desc) - 1))
+			goto wrong_f17hcore_error;
+
+		pr_cont("%s.\n", f17h_l2_mce_desc[xec]);
+		break;
+
+	case DE:
+		if (xec > (ARRAY_SIZE(f17h_de_mce_desc) - 1))
+			goto wrong_f17hcore_error;
+
+		pr_cont("%s.\n", f17h_de_mce_desc[xec]);
+		break;
+
+	case EX:
+		if (xec > (ARRAY_SIZE(f17h_ex_mce_desc) - 1))
+			goto wrong_f17hcore_error;
+
+		pr_cont("%s.\n", f17h_ex_mce_desc[xec]);
+		break;
+
+	case FP:
+		if (xec > (ARRAY_SIZE(f17h_fp_mce_desc) - 1))
+			goto wrong_f17hcore_error;
+
+		pr_cont("%s.\n", f17h_fp_mce_desc[xec]);
+		break;
+
+	case L3_CACHE:
+		if (xec > (ARRAY_SIZE(f17h_l3_mce_desc) - 1))
+			goto wrong_f17hcore_error;
+
+		pr_cont("%s.\n", f17h_l3_mce_desc[xec]);
+		break;
+
+	default:
+		goto wrong_f17hcore_error;
+	}
+
+	return;
+
+wrong_f17hcore_error:
+	pr_cont("Unrecognized error code from %s MCA bank\n",
+		(mca_type == L3_CACHE) ? "L3 Cache" : "F17h Core");
+}
+
+static void decode_df_errors(u8 xec, unsigned int mca_type)
+{
+	switch (mca_type) {
+	case  CS:
+		if (xec > (ARRAY_SIZE(f17h_cs_mce_desc) - 1))
+			goto wrong_df_error;
+
+		pr_cont("%s.\n", f17h_cs_mce_desc[xec]);
+		break;
+
+	case PIE:
+		if (xec > (ARRAY_SIZE(f17h_pie_mce_desc) - 1))
+			goto wrong_df_error;
+
+		pr_cont("%s.\n", f17h_pie_mce_desc[xec]);
+		break;
+
+	default:
+		goto wrong_df_error;
+	}
+
+	return;
+
+wrong_df_error:
+	pr_cont("Unrecognized error code from DF MCA bank\n");
+}
+
+/* Decode errors according to Scalable MCA specification */
+static void decode_smca_errors(struct mce *m)
+{
+	u32 low, high;
+	u32 addr = MSR_AMD64_SMCA_MCx_IPID(m->bank);
+	unsigned int hwid, mca_type, i;
+	u8 xec = XEC(m->status, xec_mask);
+
+	if (rdmsr_safe(addr, &low, &high)) {
+		pr_emerg("Unable to decode errors from banks\n");
+		return;
+	}
+
+	hwid = high & MCI_IPID_HWID;
+	mca_type = (high & MCI_IPID_MCATYPE) >> 16;
+
+	pr_emerg(HW_ERR "MC%d IPID value: 0x%08x%08x\n",
+		 m->bank, high, low);
+
+	/*
+	 * Based on hwid and mca_type values,
+	 * decode errors from respective IPs.
+	 * Note: mca_type values make sense only
+	 * in the context of an hwid
+	 */
+	for (i = 0; i < ARRAY_SIZE(hwid_mappings); i++)
+		if (hwid_mappings[i].hwid_value == hwid)
+			break;
+
+	switch (i) {
+	case F17H_CORE:
+		pr_emerg(HW_ERR "%s Error: ",
+			 (mca_type == L3_CACHE) ? "L3 Cache" : "F17h Core");
+		decode_f17hcore_errors(xec, mca_type);
+		break;
+
+	case DF:
+		pr_emerg(HW_ERR "DF Error: ");
+		decode_df_errors(xec, mca_type);
+		break;
+
+	case UMC:
+		pr_emerg(HW_ERR "UMC Error: ");
+		if (xec > (ARRAY_SIZE(f17h_umc_mce_desc) - 1)) {
+			pr_cont("Unrecognized error code from UMC MCA bank\n");
+			return;
+		}
+		pr_cont("%s.\n", f17h_umc_mce_desc[xec]);
+		break;
+
+	case FUSE:
+		pr_emerg(HW_ERR "FUSE Error: ");
+		if (xec > (ARRAY_SIZE(f17h_fuse_mce_desc) - 1)) {
+			pr_cont("Unrecognized error code from FUSE MCA bank\n");
+			return;
+		}
+		pr_cont("%s.\n", f17h_fuse_mce_desc[xec]);
+		break;
+
+	case PSP:
+		pr_emerg(HW_ERR "PSP Error: ");
+		if (xec > (ARRAY_SIZE(f17h_psp_mce_desc) - 1)) {
+			pr_cont("Unrecognized error code from PSP MCA bank\n");
+			return;
+		}
+		pr_cont("%s.\n", f17h_psp_mce_desc[xec]);
+		break;
+
+	case SMU:
+		pr_emerg(HW_ERR "SMU Error: ");
+		if (xec > (ARRAY_SIZE(f17h_smu_mce_desc) - 1)) {
+			pr_cont("Unrecognized error code from SMU MCA bank\n");
+			return;
+		}
+		pr_cont("%s.\n", f17h_smu_mce_desc[xec]);
+		break;
+
+	default:
+		pr_emerg(HW_ERR "HWID:%d does not match any existing IPs\n", hwid);
+	}
+}
+
 static const char *decode_error_status(struct mce *m)
 {
 	if (m->status & MCI_STATUS_UC) {
@@ -769,11 +1071,21 @@ int amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 		((m->status & MCI_STATUS_PCC)	? "PCC"	  : "-"),
 		((m->status & MCI_STATUS_ADDRV)	? "AddrV" : "-"));
 
-	if (c->x86 == 0x15 || c->x86 == 0x16)
+	if (c->x86 >= 0x15)
 		pr_cont("|%s|%s",
 			((m->status & MCI_STATUS_DEFERRED) ? "Deferred" : "-"),
 			((m->status & MCI_STATUS_POISON)   ? "Poison"   : "-"));
 
+	if (mce_flags.smca) {
+		u32 smca_low, smca_high;
+		u32 smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(m->bank);
+
+		if (!rdmsr_safe(smca_addr, &smca_low, &smca_high) &&
+		    (smca_low & MCI_CONFIG_MCAX))
+			pr_cont("|%s",
+				((m->status & MCI_STATUS_TCC) ? "TCC" : "-"));
+	}
+
 	/* do the two bits[14:13] together */
 	ecc = (m->status >> 45) & 0x3;
 	if (ecc)
@@ -784,6 +1096,11 @@ int amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 	if (m->status & MCI_STATUS_ADDRV)
 		pr_emerg(HW_ERR "MC%d Error Address: 0x%016llx\n", m->bank, m->addr);
 
+	if (mce_flags.smca) {
+		decode_smca_errors(m);
+		goto err_code;
+	}
+
 	if (!fam_ops)
 		goto err_code;
 
@@ -888,6 +1205,14 @@ static int __init mce_amd_init(void)
 		fam_ops->mc2_mce = f16h_mc2_mce;
 		break;
 
+	case 0x17:
+		xec_mask = 0x3f;
+		if (!mce_flags.smca) {
+			printk(KERN_WARNING "Decoding supported only on Scalable MCA enabled processors\n");
+			return 0;
+		}
+		break;
+
 	default:
 		printk(KERN_WARNING "Huh? What family is it: 0x%x?!\n", c->x86);
 		kfree(fam_ops);
-- 
2.7.0

[PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address

[Aravind Gopalakrishnan]

In upcoming processors, the BLKPTR field is no longer used
to indicate the MSR number of the additional register.
Insted, it simply indicates the prescence of additional MSRs.

Fixing the logic here to gather MSR address from
MSR_AMD64_SMCA_MCx_MISC() for newer processors
and we fall back to existing logic for older processors.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 arch/x86/include/asm/msr-index.h     |  4 ++
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 94 +++++++++++++++++++++++++-----------
 2 files changed, 69 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 93bccbc..ca49e928e 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -265,10 +265,14 @@
 #define MSR_IA32_MCx_CTL2(x)		(MSR_IA32_MC0_CTL2 + (x))
 
 /* 'SMCA': AMD64 Scalable MCA */
+#define MSR_AMD64_SMCA_MC0_MISC0	0xc0002003
 #define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
 #define MSR_AMD64_SMCA_MC0_IPID		0xc0002005
+#define MSR_AMD64_SMCA_MC0_MISC1	0xc000200a
+#define MSR_AMD64_SMCA_MCx_MISC(x)	(MSR_AMD64_SMCA_MC0_MISC0 + 0x10*(x))
 #define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
 #define MSR_AMD64_SMCA_MCx_IPID(x)	(MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))
+#define MSR_AMD64_SMCA_MCx_MISCy(x, y)	((MSR_AMD64_SMCA_MC0_MISC1 + y) + (0x10*(x)))
 
 #define MSR_P6_PERFCTR0			0x000000c1
 #define MSR_P6_PERFCTR1			0x000000c2
diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 8169103..4bdc836 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -286,6 +286,58 @@ static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c)
 	wrmsr(MSR_CU_DEF_ERR, low, high);
 }
 
+static u32 get_block_address(u32 current_addr,
+			     u32 low,
+			     u32 high,
+			     unsigned int bank,
+			     unsigned int block)
+{
+	u32 addr = 0, offset = 0;
+
+	if (mce_flags.smca) {
+		if (!block) {
+			addr = MSR_AMD64_SMCA_MCx_MISC(bank);
+		} else {
+			/*
+			 * For SMCA enabled processors, BLKPTR field
+			 * of the first MISC register (MCx_MISC0) indicates
+			 * presence of additional MISC register set (MISC1-4)
+			 */
+			u32 smca_low, smca_high;
+
+			if (rdmsr_safe(MSR_AMD64_SMCA_MCx_CONFIG(bank),
+				       &smca_low, &smca_high) ||
+			    !(smca_low & MCI_CONFIG_MCAX))
+				goto nextaddr_out;
+
+			if (!rdmsr_safe(MSR_AMD64_SMCA_MCx_MISC(bank),
+					&smca_low, &smca_high) &&
+			    (smca_low & MASK_BLKPTR_LO))
+				addr = MSR_AMD64_SMCA_MCx_MISCy(bank,
+								block - 1);
+		}
+
+		goto nextaddr_out;
+	}
+
+	/* Fall back to method we used for older processors */
+	switch (block) {
+	case 0:
+		addr = MSR_IA32_MCx_MISC(bank);
+		break;
+	case 1:
+		offset = ((low & MASK_BLKPTR_LO) >> 21);
+		if (offset)
+			addr = MCG_XBLK_ADDR + offset;
+		break;
+	default:
+		addr = current_addr++;
+	}
+
+nextaddr_out:
+		return addr;
+}
+
 static int
 prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 			int offset, u32 misc_high)
@@ -348,16 +400,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 
 	for (bank = 0; bank < mca_cfg.banks; ++bank) {
 		for (block = 0; block < NR_BLOCKS; ++block) {
-			if (block == 0)
-				address = MSR_IA32_MCx_MISC(bank);
-			else if (block == 1) {
-				address = (low & MASK_BLKPTR_LO) >> 21;
-				if (!address)
-					break;
-
-				address += MCG_XBLK_ADDR;
-			} else
-				++address;
+			address = get_block_address(address, high, low,
+						    bank, block);
+			if (!address)
+				break;
 
 			if (rdmsr_safe(address, &low, &high))
 				break;
@@ -462,16 +508,10 @@ static void amd_threshold_interrupt(void)
 		if (!(per_cpu(bank_map, cpu) & (1 << bank)))
 			continue;
 		for (block = 0; block < NR_BLOCKS; ++block) {
-			if (block == 0) {
-				address = MSR_IA32_MCx_MISC(bank);
-			} else if (block == 1) {
-				address = (low & MASK_BLKPTR_LO) >> 21;
-				if (!address)
-					break;
-				address += MCG_XBLK_ADDR;
-			} else {
-				++address;
-			}
+			address = get_block_address(address, high, low,
+						    bank, block);
+			if (!address)
+				break;
 
 			if (rdmsr_safe(address, &low, &high))
 				break;
@@ -691,16 +731,12 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 	if (err)
 		goto out_free;
 recurse:
-	if (!block) {
-		address = (low & MASK_BLKPTR_LO) >> 21;
-		if (!address)
-			return 0;
-		address += MCG_XBLK_ADDR;
-	} else {
-		++address;
-	}
+	address = get_block_address(address, high, low, bank, ++block);
+
+	if (!address)
+		return 0;
 
-	err = allocate_threshold_blocks(cpu, bank, ++block, address);
+	err = allocate_threshold_blocks(cpu, bank, block, address);
 	if (err)
 		goto out_free;
 
-- 
2.7.0

[PATCH 3/4] x86/mce: Clarify comments regarding deferred error

[Aravind Gopalakrishnan]

The Deferred field indicates if we have a Deferred error.
Deferred errors indicate errors that hardware could not
fix. But it still does not cause any interruption to program
flow. So it does not generate any #MC and UC bit in MCx_STATUS
is not set.

Fixing comment here. No functional change

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 arch/x86/include/asm/mce.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 2ec67ac..476da8b 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -40,7 +40,7 @@
 #define MCI_STATUS_AR	 (1ULL<<55)  /* Action required */
 
 /* AMD-specific bits */
-#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
+#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare a deferred error */
 #define MCI_STATUS_POISON	(1ULL<<43)  /* access poisonous data */
 #define MCI_STATUS_TCC		(1ULL<<55)  /* Task context corrupt */
 /*
-- 
2.7.0

[PATCH 4/4] x86/mce/AMD: Add comments for easier understanding

[Aravind Gopalakrishnan]

In an attempt to help folks not very familiar with the code to
understand what the code is doing, adding a bit of helper
comments around some more important functions in the driver
to describe them.

No functional change is introduced.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 4bdc836..d2b6001 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -184,6 +184,11 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
 };
 
 /*
+ * Set the error_count and interrupt_enable sysfs attributes here.
+ * This function gets called during the init phase and when someone
+ * makes changes to either of the sysfs attributes.
+ * During init phase, we also program Interrupt type as 'APIC' and
+ * verify if LVT offset obtained from MCx_MISC is valid.
  * Called via smp_call_function_single(), must be called with correct
  * cpu affinity.
  */
@@ -262,6 +267,11 @@ static int setup_APIC_deferred_error(int reserved, int new)
 	return reserved;
 }
 
+/*
+ * Obtain LVT offset from MSR_CU_DEF_ERR and call
+ * setup_APIC_deferred_error() to program relevant APIC register.
+ * Also, register a deferred error interrupt handler
+ */
 static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c)
 {
 	u32 low = 0, high = 0;
@@ -338,6 +348,14 @@ nextaddr_out:
 		return addr;
 }
 
+/*
+ * struct threshold_block descriptor tracks useful info regarding the
+ * banks' MISC register. Among other things, it tracks whether interrupt
+ * is possible for the given bank, the threshold limit and the sysfs object
+ * that outputs these info. Initializing the struct here, programming
+ * LVT offset for threshold interrupts and registering a interrupt handler
+ * if we haven't already done so
+ */
 static int
 prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 			int offset, u32 misc_high)
@@ -673,6 +691,9 @@ static struct kobj_type threshold_ktype = {
 	.default_attrs		= default_attrs,
 };
 
+/*
+ * Initializing sysfs entries for each block within the MCA bank
+ */
 static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 				     unsigned int block, u32 address)
 {
-- 
2.7.0

Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address

[Aravind Gopalakrishnan]

On 2/16/2016 3:45 PM, Aravind Gopalakrishnan wrote:
> In upcoming processors, the BLKPTR field is no longer used
> to indicate the MSR number of the additional register.
> Insted, it simply indicates the prescence of additional MSRs.
>
> Fixing the logic here to gather MSR address from
> MSR_AMD64_SMCA_MCx_MISC() for newer processors
> and we fall back to existing logic for older processors.
>
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---

Caught couple of issues-

>
> +	/* Fall back to method we used for older processors */
> +	switch (block) {
> +	case 0:
> +		addr = MSR_IA32_MCx_MISC(bank);
> +		break;
> +	case 1:
> +		offset = ((low & MASK_BLKPTR_LO) >> 21);
> +		if (offset)
> +			addr = MCG_XBLK_ADDR + offset;
> +		break;
> +	default:
> +		addr = current_addr++;
> +	}
> +


This needs to be addr = ++current_addr;

>
> +			address = get_block_address(address, high, low,
> +						    bank, block);

The 'high' and 'low' variables need to be swapped.
Missed this during a rebase to latest tip, Apologies..

>
> +			address = get_block_address(address, high, low,
> +						    bank, block);

and here..

> +	address = get_block_address(address, high, low, bank, ++block);
> +

and here..

> +	if (!address)
> +		return 0;
>   
>

Apologies, these didn't show up on initial testing locally..

Fixed these on local branch and it seems to work fine.
I'll send it out as a V2 (Shall wait for further comments/reviews before 
I do that).

Thanks,
-Aravind.

Re: [PATCH 3/4] x86/mce: Clarify comments regarding deferred error

[Borislav Petkov]

On Tue, Feb 16, 2016 at 03:45:10PM -0600, Aravind Gopalakrishnan wrote:
> The Deferred field indicates if we have a Deferred error.
> Deferred errors indicate errors that hardware could not
> fix. But it still does not cause any interruption to program
> flow. So it does not generate any #MC and UC bit in MCx_STATUS
> is not set.
> 
> Fixing comment here. No functional change
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  arch/x86/include/asm/mce.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 2ec67ac..476da8b 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -40,7 +40,7 @@
>  #define MCI_STATUS_AR	 (1ULL<<55)  /* Action required */
>  
>  /* AMD-specific bits */
> -#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
> +#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare a deferred error */

/* uncorrected error, deferred exception */

sounds better to me.

>  #define MCI_STATUS_POISON	(1ULL<<43)  /* access poisonous data */
>  #define MCI_STATUS_TCC		(1ULL<<55)  /* Task context corrupt */
>  /*
> --

For the future, such cleanups/fixes should always go first in the patch
set.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 4/4] x86/mce/AMD: Add comments for easier understanding

[Borislav Petkov]

On Tue, Feb 16, 2016 at 03:45:11PM -0600, Aravind Gopalakrishnan wrote:
> In an attempt to help folks not very familiar with the code to
> understand what the code is doing, adding a bit of helper
> comments around some more important functions in the driver
> to describe them.
> 
> No functional change is introduced.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index 4bdc836..d2b6001 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -184,6 +184,11 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
>  };
>  
>  /*
> + * Set the error_count and interrupt_enable sysfs attributes here.
> + * This function gets called during the init phase and when someone
> + * makes changes to either of the sysfs attributes.
> + * During init phase, we also program Interrupt type as 'APIC' and
> + * verify if LVT offset obtained from MCx_MISC is valid.
>   * Called via smp_call_function_single(), must be called with correct
>   * cpu affinity.
>   */

I don't think that's what threshold_restart_bank() does...

Also, that comment is too much - it shouldn't explain "what" but "why".

> @@ -262,6 +267,11 @@ static int setup_APIC_deferred_error(int reserved, int new)
>  	return reserved;
>  }
>  
> +/*
> + * Obtain LVT offset from MSR_CU_DEF_ERR and call
> + * setup_APIC_deferred_error() to program relevant APIC register.
> + * Also, register a deferred error interrupt handler
> + */

No, that's basically spelling what the code does.

>  static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c)
>  {
>  	u32 low = 0, high = 0;
> @@ -338,6 +348,14 @@ nextaddr_out:
>  		return addr;
>  }
>  
> +/*
> + * struct threshold_block descriptor tracks useful info regarding the
> + * banks' MISC register. Among other things, it tracks whether interrupt
> + * is possible for the given bank, the threshold limit and the sysfs object
> + * that outputs these info.

That should be in form of comments explaining what the members of struct
threshold_block are, where that struct is defined.

> Initializing the struct here, programming
> + * LVT offset for threshold interrupts and registering a interrupt handler
> + * if we haven't already done so

Also spelling the code.

> + */
>  static int
>  prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
>  			int offset, u32 misc_high)
-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors

[Borislav Petkov]

On Tue, Feb 16, 2016 at 03:45:08PM -0600, Aravind Gopalakrishnan wrote:
> For Scalable MCA enabled processors, errors are listed
> per IP block. And since it is not required for an IP to
> map to a particular bank, we need to use HWID and McaType
> values from the MCx_IPID register to figure out which IP
> a given bank represents.
> 
> We also have a new bit (TCC) in the MCx_STATUS register
> to indicate Task context is corrupt.
> 
> Add logic here to decode errors from all known IP
> blocks for Fam17h Model 00-0fh and to print TCC errors.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  arch/x86/include/asm/mce.h           |  50 ++++++
>  arch/x86/include/asm/msr-index.h     |   2 +
>  arch/x86/kernel/cpu/mcheck/mce_amd.c |  11 ++
>  drivers/edac/mce_amd.c               | 327 ++++++++++++++++++++++++++++++++++-
>  4 files changed, 389 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 2ea4527..2ec67ac 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -42,6 +42,17 @@
>  /* AMD-specific bits */
>  #define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
>  #define MCI_STATUS_POISON	(1ULL<<43)  /* access poisonous data */
> +#define MCI_STATUS_TCC		(1ULL<<55)  /* Task context corrupt */

\n

> +/*
> + * McaX field if set indicates a given bank supports MCA extensions:
> + *  - Deferred error interrupt type is specifiable by bank
> + *  - BlkPtr field indicates prescence of extended MISC registers.
				^^^^^^^^^

Btw, that's MCi_MISC[BlkPtr] ?

Also, please integrate a spell checker into your patch creation
workflow.


> + *    But should not be used to determine MSR numbers
> + *  - TCC bit is present in MCx_STATUS

All sentences end with a "."

> + */
> +#define MCI_CONFIG_MCAX		0x1
> +#define MCI_IPID_MCATYPE	0xFFFF0000
> +#define MCI_IPID_HWID		0xFFF
>  
>  /*
>   * Note that the full MCACOD field of IA32_MCi_STATUS MSR is
> @@ -287,4 +298,43 @@ struct cper_sec_mem_err;
>  extern void apei_mce_report_mem_error(int corrected,
>  				      struct cper_sec_mem_err *mem_err);
>  
> +/*
> + * Enumerating new IP types and HWID values

Please stop using gerund, i.e., -ing, in comments and commit messages.

"Enumerate new IP ..." is just fine.

> + * in ScalableMCA enabled AMD processors
> + */
> +#ifdef CONFIG_X86_MCE_AMD
> +enum ip_types {

AMD-specific so "amd_ip_types"

> +	F17H_CORE = 0,	/* Core errors */
> +	DF,		/* Data Fabric */
> +	UMC,		/* Unified Memory Controller */
> +	FUSE,		/* FUSE subsystem */

What's FUSE subsystem?

In any case, this could use a different name in order not to confuse
with Linux's filesystem in userspace.

> +	PSP,		/* Platform Security Processor */
> +	SMU,		/* System Management Unit */
> +	N_IP_TYPES
> +};
> +
> +struct hwid {

amd_hwid and so on. All below should have the "amd_" prefix so that it
is obvious.

> +	const char *ipname;
> +	unsigned int hwid_value;
> +};
> +
> +extern struct hwid hwid_mappings[N_IP_TYPES];


> +
> +enum core_mcatypes {
> +	LS = 0,		/* Load Store */
> +	IF,		/* Instruction Fetch */
> +	L2_CACHE,	/* L2 cache */
> +	DE,		/* Decoder unit */
> +	RES,		/* Reserved */
> +	EX,		/* Execution unit */
> +	FP,		/* Floating Point */
> +	L3_CACHE	/* L3 cache */
> +};
> +
> +enum df_mcatypes {
> +	CS = 0,		/* Coherent Slave */
> +	PIE		/* Power management, Interrupts, etc */
> +};
> +#endif

So all those are defined here but we have a header for exactly that
drivers/edac/mce_amd.h. And then you define and export hwid_mappings in
arch/x86/kernel/cpu/mcheck/mce_amd.c to not use it there.

Why isn't all this in drivers/edac/mce_amd.[ch] ?

And if it is there, then you obviously don't need the "amd_" prefix.

> +
>  #endif /* _ASM_X86_MCE_H */
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 5523465..93bccbc 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -266,7 +266,9 @@
>  
>  /* 'SMCA': AMD64 Scalable MCA */
>  #define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
> +#define MSR_AMD64_SMCA_MC0_IPID		0xc0002005
>  #define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
> +#define MSR_AMD64_SMCA_MCx_IPID(x)	(MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))

Are those MSRs used in multiple files? If not, -> mce.h.

>  #define MSR_P6_PERFCTR0			0x000000c1
>  #define MSR_P6_PERFCTR1			0x000000c2
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index 88de27b..8169103 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -71,6 +71,17 @@ static const char * const th_names[] = {
>  	"execution_unit",
>  };
>  
> +/* Defining HWID to IP type mappings for Scalable MCA */

" Define ..."

> +struct hwid hwid_mappings[] = {
> +	[F17H_CORE]	= { "f17h_core", 0xB0 },
> +	[DF]		= { "df", 0x2E },
> +	[UMC]		= { "umc", 0x96 },
> +	[FUSE]		= { "fuse", 0x5 },
> +	[PSP]		= { "psp", 0xFF },
> +	[SMU]		= { "smu", 0x1 },
> +};
> +EXPORT_SYMBOL_GPL(hwid_mappings);
> +
>  static DEFINE_PER_CPU(struct threshold_bank **, threshold_banks);
>  static DEFINE_PER_CPU(unsigned char, bank_map);	/* see which banks are on */
>  
> diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
> index e3a945c..6e6b327 100644
> --- a/drivers/edac/mce_amd.c
> +++ b/drivers/edac/mce_amd.c
> @@ -147,6 +147,136 @@ static const char * const mc6_mce_desc[] = {
>  	"Status Register File",

...

> +static void decode_f17hcore_errors(u8 xec, unsigned int mca_type)
> +{
> +	switch (mca_type) {
> +	case LS:
> +		if (xec == 0x4 || xec > (ARRAY_SIZE(f17h_ls_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_ls_mce_desc[xec]);
> +		break;
> +
> +	case IF:
> +		if (xec > (ARRAY_SIZE(f17h_if_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_if_mce_desc[xec]);
> +		break;
> +
> +	case L2_CACHE:
> +		if (xec > (ARRAY_SIZE(f17h_l2_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_l2_mce_desc[xec]);
> +		break;
> +
> +	case DE:
> +		if (xec > (ARRAY_SIZE(f17h_de_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_de_mce_desc[xec]);
> +		break;
> +
> +	case EX:
> +		if (xec > (ARRAY_SIZE(f17h_ex_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_ex_mce_desc[xec]);
> +		break;
> +
> +	case FP:
> +		if (xec > (ARRAY_SIZE(f17h_fp_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_fp_mce_desc[xec]);
> +		break;
> +
> +	case L3_CACHE:
> +		if (xec > (ARRAY_SIZE(f17h_l3_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_l3_mce_desc[xec]);
> +		break;
> +
> +	default:
> +		goto wrong_f17hcore_error;

That's a lot of repeated code. You can assign the desc array to a temp
variable depending on mca_type and do the if and pr_cont only once using
that temp variable.

> +	}
> +
> +	return;
> +
> +wrong_f17hcore_error:
> +	pr_cont("Unrecognized error code from %s MCA bank\n",
> +		(mca_type == L3_CACHE) ? "L3 Cache" : "F17h Core");
> +}
> +
> +static void decode_df_errors(u8 xec, unsigned int mca_type)
> +{
> +	switch (mca_type) {
> +	case  CS:
> +		if (xec > (ARRAY_SIZE(f17h_cs_mce_desc) - 1))
> +			goto wrong_df_error;
> +
> +		pr_cont("%s.\n", f17h_cs_mce_desc[xec]);
> +		break;
> +
> +	case PIE:
> +		if (xec > (ARRAY_SIZE(f17h_pie_mce_desc) - 1))
> +			goto wrong_df_error;
> +
> +		pr_cont("%s.\n", f17h_pie_mce_desc[xec]);
> +		break;

Ditto.

> +
> +	default:
> +		goto wrong_df_error;
> +	}
> +
> +	return;
> +
> +wrong_df_error:
> +	pr_cont("Unrecognized error code from DF MCA bank\n");
> +}
> +
> +/* Decode errors according to Scalable MCA specification */
> +static void decode_smca_errors(struct mce *m)
> +{
> +	u32 low, high;
> +	u32 addr = MSR_AMD64_SMCA_MCx_IPID(m->bank);
> +	unsigned int hwid, mca_type, i;
> +	u8 xec = XEC(m->status, xec_mask);
> +
> +	if (rdmsr_safe(addr, &low, &high)) {
> +		pr_emerg("Unable to decode errors from banks\n");

That statement is not very useful in its current form.

> +		return;
> +	}
> +
> +	hwid = high & MCI_IPID_HWID;
> +	mca_type = (high & MCI_IPID_MCATYPE) >> 16;
> +
> +	pr_emerg(HW_ERR "MC%d IPID value: 0x%08x%08x\n",
> +		 m->bank, high, low);
> +
> +	/*
> +	 * Based on hwid and mca_type values,
> +	 * decode errors from respective IPs.
> +	 * Note: mca_type values make sense only
> +	 * in the context of an hwid
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(hwid_mappings); i++)

So you use those hwid_mappings here. Why aren't they defined here too?

> +		if (hwid_mappings[i].hwid_value == hwid)
> +			break;
> +
> +	switch (i) {
> +	case F17H_CORE:
> +		pr_emerg(HW_ERR "%s Error: ",
> +			 (mca_type == L3_CACHE) ? "L3 Cache" : "F17h Core");
> +		decode_f17hcore_errors(xec, mca_type);
> +		break;
> +
> +	case DF:
> +		pr_emerg(HW_ERR "DF Error: ");
> +		decode_df_errors(xec, mca_type);
> +		break;
> +
> +	case UMC:
> +		pr_emerg(HW_ERR "UMC Error: ");
> +		if (xec > (ARRAY_SIZE(f17h_umc_mce_desc) - 1)) {
> +			pr_cont("Unrecognized error code from UMC MCA bank\n");
> +			return;
> +		}
> +		pr_cont("%s.\n", f17h_umc_mce_desc[xec]);
> +		break;
> +
> +	case FUSE:
> +		pr_emerg(HW_ERR "FUSE Error: ");
> +		if (xec > (ARRAY_SIZE(f17h_fuse_mce_desc) - 1)) {
> +			pr_cont("Unrecognized error code from FUSE MCA bank\n");
> +			return;
> +		}
> +		pr_cont("%s.\n", f17h_fuse_mce_desc[xec]);
> +		break;
> +
> +	case PSP:
> +		pr_emerg(HW_ERR "PSP Error: ");
> +		if (xec > (ARRAY_SIZE(f17h_psp_mce_desc) - 1)) {
> +			pr_cont("Unrecognized error code from PSP MCA bank\n");
> +			return;
> +		}
> +		pr_cont("%s.\n", f17h_psp_mce_desc[xec]);
> +		break;
> +
> +	case SMU:
> +		pr_emerg(HW_ERR "SMU Error: ");
> +		if (xec > (ARRAY_SIZE(f17h_smu_mce_desc) - 1)) {
> +			pr_cont("Unrecognized error code from SMU MCA bank\n");
> +			return;
> +		}
> +		pr_cont("%s.\n", f17h_smu_mce_desc[xec]);
> +		break;

Also a lot of repeated code which could be simplified.

> +
> +	default:
> +		pr_emerg(HW_ERR "HWID:%d does not match any existing IPs\n", hwid);
> +	}
> +}
> +
>  static const char *decode_error_status(struct mce *m)
>  {
>  	if (m->status & MCI_STATUS_UC) {
> @@ -769,11 +1071,21 @@ int amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
>  		((m->status & MCI_STATUS_PCC)	? "PCC"	  : "-"),
>  		((m->status & MCI_STATUS_ADDRV)	? "AddrV" : "-"));
>  
> -	if (c->x86 == 0x15 || c->x86 == 0x16)
> +	if (c->x86 >= 0x15)
>  		pr_cont("|%s|%s",
>  			((m->status & MCI_STATUS_DEFERRED) ? "Deferred" : "-"),
>  			((m->status & MCI_STATUS_POISON)   ? "Poison"   : "-"));
>  
> +	if (mce_flags.smca) {

ERROR: "mce_flags" [drivers/edac/edac_mce_amd.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....

Just read CPUID again here instead of exporting mce_flags.

> +		u32 smca_low, smca_high;


> +		u32 smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(m->bank);

s/smca_//

Also, all those MSRs should be defined in drivers/edac/mce_amd.h if not
used outside.

> +
> +		if (!rdmsr_safe(smca_addr, &smca_low, &smca_high) &&
> +		    (smca_low & MCI_CONFIG_MCAX))
> +			pr_cont("|%s",

No need for that break here.

> +				((m->status & MCI_STATUS_TCC) ? "TCC" : "-"));
> +	}
> +
>  	/* do the two bits[14:13] together */
>  	ecc = (m->status >> 45) & 0x3;
>  	if (ecc)
> @@ -784,6 +1096,11 @@ int amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
>  	if (m->status & MCI_STATUS_ADDRV)
>  		pr_emerg(HW_ERR "MC%d Error Address: 0x%016llx\n", m->bank, m->addr);
>  
> +	if (mce_flags.smca) {
> +		decode_smca_errors(m);
> +		goto err_code;
> +	}
> +
>  	if (!fam_ops)
>  		goto err_code;
>  
> @@ -888,6 +1205,14 @@ static int __init mce_amd_init(void)
>  		fam_ops->mc2_mce = f16h_mc2_mce;
>  		break;
>  
> +	case 0x17:
> +		xec_mask = 0x3f;
> +		if (!mce_flags.smca) {
> +			printk(KERN_WARNING "Decoding supported only on Scalable MCA enabled processors\n");

What is that supposed to do? I thought all new ones will have SMCA...

> +			return 0;
> +		}
> +		break;
> +
>  	default:
>  		printk(KERN_WARNING "Huh? What family is it: 0x%x?!\n", c->x86);
>  		kfree(fam_ops);
> -- 
> 2.7.0
> 
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address

[Borislav Petkov]

On Tue, Feb 16, 2016 at 03:45:09PM -0600, Aravind Gopalakrishnan wrote:
> In upcoming processors, the BLKPTR field is no longer used
> to indicate the MSR number of the additional register.
> Insted, it simply indicates the prescence of additional MSRs.
> 
> Fixing the logic here to gather MSR address from
> MSR_AMD64_SMCA_MCx_MISC() for newer processors
> and we fall back to existing logic for older processors.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  arch/x86/include/asm/msr-index.h     |  4 ++
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 94 +++++++++++++++++++++++++-----------
>  2 files changed, 69 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 93bccbc..ca49e928e 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -265,10 +265,14 @@
>  #define MSR_IA32_MCx_CTL2(x)		(MSR_IA32_MC0_CTL2 + (x))
>  
>  /* 'SMCA': AMD64 Scalable MCA */
> +#define MSR_AMD64_SMCA_MC0_MISC0	0xc0002003
>  #define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
>  #define MSR_AMD64_SMCA_MC0_IPID		0xc0002005
> +#define MSR_AMD64_SMCA_MC0_MISC1	0xc000200a
> +#define MSR_AMD64_SMCA_MCx_MISC(x)	(MSR_AMD64_SMCA_MC0_MISC0 + 0x10*(x))
>  #define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
>  #define MSR_AMD64_SMCA_MCx_IPID(x)	(MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))
> +#define MSR_AMD64_SMCA_MCx_MISCy(x, y)	((MSR_AMD64_SMCA_MC0_MISC1 + y) + (0x10*(x)))

Are those MSRs going to be used in multiple files? If not, they should
all go to mce.h.

>  #define MSR_P6_PERFCTR0			0x000000c1
>  #define MSR_P6_PERFCTR1			0x000000c2
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index 8169103..4bdc836 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -286,6 +286,58 @@ static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c)
>  	wrmsr(MSR_CU_DEF_ERR, low, high);
>  }
>  
> +static u32 get_block_address(u32 current_addr,
> +			     u32 low,
> +			     u32 high,
> +			     unsigned int bank,
> +			     unsigned int block)

Use arg formatting like the rest of functions in the file please.

> +{
> +	u32 addr = 0, offset = 0;
> +
> +	if (mce_flags.smca) {
> +		if (!block) {
> +			addr = MSR_AMD64_SMCA_MCx_MISC(bank);
> +		} else {
> +			/*
> +			 * For SMCA enabled processors, BLKPTR field
> +			 * of the first MISC register (MCx_MISC0) indicates
> +			 * presence of additional MISC register set (MISC1-4)
> +			 */
> +			u32 smca_low, smca_high;

s/smca_//

> +
> +			if (rdmsr_safe(MSR_AMD64_SMCA_MCx_CONFIG(bank),
> +				       &smca_low, &smca_high) ||
> +			    !(smca_low & MCI_CONFIG_MCAX))
> +				goto nextaddr_out;
> +
> +			if (!rdmsr_safe(MSR_AMD64_SMCA_MCx_MISC(bank),
> +					&smca_low, &smca_high) &&
> +			    (smca_low & MASK_BLKPTR_LO))
> +				addr = MSR_AMD64_SMCA_MCx_MISCy(bank,
> +								block - 1);

unnecessary line break.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors

[Aravind Gopalakrishnan]

On 2/23/16 6:37 AM, Borislav Petkov wrote:
> On Tue, Feb 16, 2016 at 03:45:08PM -0600, Aravind Gopalakrishnan wrote:
>>   /* AMD-specific bits */
>>   #define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
>>   #define MCI_STATUS_POISON	(1ULL<<43)  /* access poisonous data */
>> +#define MCI_STATUS_TCC		(1ULL<<55)  /* Task context corrupt */
> \n
>

Ack.

>> +/*
>> + * McaX field if set indicates a given bank supports MCA extensions:
>> + *  - Deferred error interrupt type is specifiable by bank
>> + *  - BlkPtr field indicates prescence of extended MISC registers.
> 				^^^^^^^^^
>
> Btw, that's MCi_MISC[BlkPtr] ?

MCi_MISC0[BlkPtr] specifically. Will update the comments about this.

> Also, please integrate a spell checker into your patch creation
> workflow.

Sorry about that. Looks like this pair is not defined in spelling.txt. 
So, might be worth adding there as well?

>> + *    But should not be used to determine MSR numbers
>> + *  - TCC bit is present in MCx_STATUS
> All sentences end with a "."

Will fix.

>
>> +/*
>> + * Enumerating new IP types and HWID values
> Please stop using gerund, i.e., -ing, in comments and commit messages.
>
> "Enumerate new IP ..." is just fine.

Ack.

>
>> + * in ScalableMCA enabled AMD processors
>> + */
>> +#ifdef CONFIG_X86_MCE_AMD
>> +enum ip_types {
> AMD-specific so "amd_ip_types"

Ok, will fix.

>
>> +	F17H_CORE = 0,	/* Core errors */
>> +	DF,		/* Data Fabric */
>> +	UMC,		/* Unified Memory Controller */
>> +	FUSE,		/* FUSE subsystem */
> What's FUSE subsystem?

It's the block for programming FUSE registers.

>
> In any case, this could use a different name in order not to confuse
> with Linux's filesystem in userspace.

Ok, will ask internally as well as to what name suits here.

>> +
>> +struct hwid {
> amd_hwid and so on. All below should have the "amd_" prefix so that it
> is obvious.

Will fix.

>
>> +	const char *ipname;
>> +	unsigned int hwid_value;
>> +};
>> +
>> +extern struct hwid hwid_mappings[N_IP_TYPES];
>> +
>> +enum core_mcatypes {
>> +	LS = 0,		/* Load Store */
>> +	IF,		/* Instruction Fetch */
>> +	L2_CACHE,	/* L2 cache */
>> +	DE,		/* Decoder unit */
>> +	RES,		/* Reserved */
>> +	EX,		/* Execution unit */
>> +	FP,		/* Floating Point */
>> +	L3_CACHE	/* L3 cache */
>> +};
>> +
>> +enum df_mcatypes {
>> +	CS = 0,		/* Coherent Slave */
>> +	PIE		/* Power management, Interrupts, etc */
>> +};
>> +#endif
> So all those are defined here but we have a header for exactly that
> drivers/edac/mce_amd.h. And then you define and export hwid_mappings in
> arch/x86/kernel/cpu/mcheck/mce_amd.c to not use it there.
>
> Why isn't all this in drivers/edac/mce_amd.[ch] ?
>
> And if it is there, then you obviously don't need the "amd_" prefix.

I have a patch that uses these enums here. But I didn't send it out 
along with this patchset as I was testing the patch.
So yes, I need it here and in the EDAC driver.

>
>> +
>>   #endif /* _ASM_X86_MCE_H */
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 5523465..93bccbc 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -266,7 +266,9 @@
>>   
>>   /* 'SMCA': AMD64 Scalable MCA */
>>   #define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
>> +#define MSR_AMD64_SMCA_MC0_IPID		0xc0002005
>>   #define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
>> +#define MSR_AMD64_SMCA_MCx_IPID(x)	(MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))
> Are those MSRs used in multiple files? If not, -> mce.h.

Yes, I'll need them in arch/x86/.../mce_amd.c as well.
A later patch will be using it there.

>>   
>>   
>> +/* Defining HWID to IP type mappings for Scalable MCA */
> " Define ..."

Ack

>
>> +	case L3_CACHE:
>> +		if (xec > (ARRAY_SIZE(f17h_l3_mce_desc) - 1))
>> +			goto wrong_f17hcore_error;
>> +
>> +		pr_cont("%s.\n", f17h_l3_mce_desc[xec]);
>> +		break;
>> +
>> +	default:
>> +		goto wrong_f17hcore_error;
> That's a lot of repeated code. You can assign the desc array to a temp
> variable depending on mca_type and do the if and pr_cont only once using
> that temp variable.

Ok, will simplify.

>
>> +
>> +	case PIE:
>> +		if (xec > (ARRAY_SIZE(f17h_pie_mce_desc) - 1))
>> +			goto wrong_df_error;
>> +
>> +		pr_cont("%s.\n", f17h_pie_mce_desc[xec]);
>> +		break;
> Ditto.
>

Will fix.

>> +
>> +/* Decode errors according to Scalable MCA specification */
>> +static void decode_smca_errors(struct mce *m)
>> +{
>> +	u32 low, high;
>> +	u32 addr = MSR_AMD64_SMCA_MCx_IPID(m->bank);
>> +	unsigned int hwid, mca_type, i;
>> +	u8 xec = XEC(m->status, xec_mask);
>> +
>> +	if (rdmsr_safe(addr, &low, &high)) {
>> +		pr_emerg("Unable to decode errors from banks\n");
> That statement is not very useful in its current form.

How about "Unable to gather IP block that threw the error. Therefore 
cannot decode errors further.\n"

Or should I just remove the pr_emerg()?

>
>> +	for (i = 0; i < ARRAY_SIZE(hwid_mappings); i++)
> So you use those hwid_mappings here. Why aren't they defined here too?

Same reason as the enums-
In a subsequent patch, I'll need it in arch/x86/../mce_amd.c
(I should have probably indicated this in the cover letter so you were 
aware of it. Sorry about that)

>
>> +	case SMU:
>> +		pr_emerg(HW_ERR "SMU Error: ");
>> +		if (xec > (ARRAY_SIZE(f17h_smu_mce_desc) - 1)) {
>> +			pr_cont("Unrecognized error code from SMU MCA bank\n");
>> +			return;
>> +		}
>> +		pr_cont("%s.\n", f17h_smu_mce_desc[xec]);
>> +		break;
> Also a lot of repeated code which could be simplified.

Hmm, will try to simplify it in V2.

>
>>   
>> +	if (mce_flags.smca) {
> ERROR: "mce_flags" [drivers/edac/edac_mce_amd.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> make: *** Waiting for unfinished jobs....
>
> Just read CPUID again here instead of exporting mce_flags.

Right. Will fix this.

>
>> +		u32 smca_low, smca_high;
>> +		u32 smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(m->bank);
> s/smca_//

Will fix.

>
> Also, all those MSRs should be defined in drivers/edac/mce_amd.h if not
> used outside.

It will be used in arch/x86/../mce_amd.c

>
>> +
>> +		if (!rdmsr_safe(smca_addr, &smca_low, &smca_high) &&
>> +		    (smca_low & MCI_CONFIG_MCAX))
>> +			pr_cont("|%s",
> No need for that break here.

Ok, Will fix.

>
>>   
>> +	case 0x17:
>> +		xec_mask = 0x3f;
>> +		if (!mce_flags.smca) {
>> +			printk(KERN_WARNING "Decoding supported only on Scalable MCA enabled processors\n");
> What is that supposed to do? I thought all new ones will have SMCA...
>
>
If for some reason the CPUID bit is not set, then we should not assume 
the processor supports the features right?

Thanks,
-Aravind.

Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address

[Aravind Gopalakrishnan]

On 2/23/16 6:39 AM, Borislav Petkov wrote:
> On Tue, Feb 16, 2016 at 03:45:09PM -0600, Aravind Gopalakrishnan wrote:
>>   
>>   /* 'SMCA': AMD64 Scalable MCA */
>> +#define MSR_AMD64_SMCA_MC0_MISC0	0xc0002003
>>   #define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
>>   #define MSR_AMD64_SMCA_MC0_IPID		0xc0002005
>> +#define MSR_AMD64_SMCA_MC0_MISC1	0xc000200a
>> +#define MSR_AMD64_SMCA_MCx_MISC(x)	(MSR_AMD64_SMCA_MC0_MISC0 + 0x10*(x))
>>   #define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
>>   #define MSR_AMD64_SMCA_MCx_IPID(x)	(MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))
>> +#define MSR_AMD64_SMCA_MCx_MISCy(x, y)	((MSR_AMD64_SMCA_MC0_MISC1 + y) + (0x10*(x)))
> Are those MSRs going to be used in multiple files? If not, they should
> all go to mce.h.

I think MSR_AMD64_SMCA_MC0_MISC0 would be required in mce.c as well.
So might be better to retain it here.

MSR_AMD64_SMCA_MC0_MISC1 might be required only in mce_amd.c, So, I'll 
move it to mce.h

>
>>   
>>   
>> +static u32 get_block_address(u32 current_addr,
>> +			     u32 low,
>> +			     u32 high,
>> +			     unsigned int bank,
>> +			     unsigned int block)
> Use arg formatting like the rest of functions in the file please.

Will fix.

>> +			u32 smca_low, smca_high;
> s/smca_//

Will fix.

>
>> +
>> +			if (rdmsr_safe(MSR_AMD64_SMCA_MCx_CONFIG(bank),
>> +				       &smca_low, &smca_high) ||
>> +			    !(smca_low & MCI_CONFIG_MCAX))
>> +				goto nextaddr_out;
>> +
>> +			if (!rdmsr_safe(MSR_AMD64_SMCA_MCx_MISC(bank),
>> +					&smca_low, &smca_high) &&
>> +			    (smca_low & MASK_BLKPTR_LO))
>> +				addr = MSR_AMD64_SMCA_MCx_MISCy(bank,
>> +								block - 1);
> unnecessary line break.
>

Will fix it like so-
addr = MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);

(It comes up to 81 chars, but will ignore checkpatch in this case..)

Thanks,
-Aravind.

Re: [PATCH 3/4] x86/mce: Clarify comments regarding deferred error

[Aravind Gopalakrishnan]

On 2/23/16 6:11 AM, Borislav Petkov wrote:
> On Tue, Feb 16, 2016 at 03:45:10PM -0600, Aravind Gopalakrishnan wrote:
>>   /* AMD-specific bits */
>> -#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
>> +#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare a deferred error */
> /* uncorrected error, deferred exception */
>
> sounds better to me.

Hmm. Well, Deferred error is a separate class of error by itself.
It's neither Corrected in HW nor is it Uncorrected like a MCE.

If you feel "Uncorrected error, deferred error exception" won;t be 
confusing, that's OK with me.


>
> For the future, such cleanups/fixes should always go first in the patch
> set.
>

Ok, I'll retain the order this time for V2 patchset as well.
But noted for future.

Thanks,
-Aravind.

Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors

[Borislav Petkov]

On Tue, Feb 23, 2016 at 04:50:37PM -0600, Aravind Gopalakrishnan wrote:
> Sorry about that. Looks like this pair is not defined in spelling.txt. So,
> might be worth adding there as well?

Oh geez, we have a spelling.txt! I think we can declare the kernel as
done and go do something else with our lives...

> It's the block for programming FUSE registers.

Oh, that's what it is.

So maybe "fuses block" or "fuses" or ... just the capitalized "FUSE" is
kinda misleading.

> How about "Unable to gather IP block that threw the error. Therefore cannot
> decode errors further.\n"

Or simply "Invalid IP block specified, error information is unreliable."
and still continue decoding. It might still be recognizable from the
signature, methinks.

> If for some reason the CPUID bit is not set, then we should not assume the
> processor supports the features right?

Is that even remotely possible? If yes, then we should keep the warning,
otherwise it is useless.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address

[Borislav Petkov]

On Tue, Feb 23, 2016 at 04:56:38PM -0600, Aravind Gopalakrishnan wrote:
> I think MSR_AMD64_SMCA_MC0_MISC0 would be required in mce.c as well.
> So might be better to retain it here.

Actually, I'm thinking, these all are - even if used in multiple files
- all MCE-specific and therefore used in MCE/RAS-specific code. So they
all should go into mce.h. Everything RAS includes that header so they're
perfectly fine there...

> (It comes up to 81 chars, but will ignore checkpatch in this case..)

The 80-cols rule is not a hard one. Here's some food for thought:

https://lkml.kernel.org/r/20160219095132.GA9681@gmail.com

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 3/4] x86/mce: Clarify comments regarding deferred error

[Borislav Petkov]

On Tue, Feb 23, 2016 at 05:02:40PM -0600, Aravind Gopalakrishnan wrote:
> On 2/23/16 6:11 AM, Borislav Petkov wrote:
> >On Tue, Feb 16, 2016 at 03:45:10PM -0600, Aravind Gopalakrishnan wrote:
> >>  /* AMD-specific bits */
> >>-#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
> >>+#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare a deferred error */
> >/* uncorrected error, deferred exception */
> >
> >sounds better to me.
> 
> Hmm. Well, Deferred error is a separate class of error by itself.
> It's neither Corrected in HW nor is it Uncorrected like a MCE.

Let's consult the BKDG:

"Deferred: deferred error.

Read-write; Updated-by-hardware. Cold reset:0.
1=A deferred error was created. A deferred error is the result of an
uncorrectable data error which did not immediately cause a processor
exception; the data is poisoned and an exception is deferred until
the data is loaded by a core. See 2.16.1.10 [Deferred Errors and Data
Poisoning]."

So it is an uncorrected error for which the raising of the error
exception was deferred until consumption.

> If you feel "Uncorrected error, deferred error exception" won;t be
> confusing, that's OK with me.

Why would it be confusing? It is describing exactly what a deferred
error is, albeit a bit too laconic but people can find the longer
description.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors

[Aravind Gopalakrishnan]

On 2/24/2016 5:28 AM, Borislav Petkov wrote:
> On Tue, Feb 23, 2016 at 04:50:37PM -0600, Aravind Gopalakrishnan wrote:
>> Sorry about that. Looks like this pair is not defined in spelling.txt. So,
>> might be worth adding there as well?
> Oh geez, we have a spelling.txt! I think we can declare the kernel as
> done and go do something else with our lives...

Haha:)

>> It's the block for programming FUSE registers.
> Oh, that's what it is.
>
> So maybe "fuses block" or "fuses" or ... just the capitalized "FUSE" is
> kinda misleading.

Asked about this internally.
Looks like it might be renamed to "Parameter block". So, I'll use that.

>> How about "Unable to gather IP block that threw the error. Therefore cannot
>> decode errors further.\n"
> Or simply "Invalid IP block specified, error information is unreliable."
> and still continue decoding. It might still be recognizable from the
> signature, methinks.

Hmm. We might be able to decode other bits of MCi_STATUS. Not the XEC 
which is what we do in this function.
So better to return early if we can't figure out which IP block to indict.

>> If for some reason the CPUID bit is not set, then we should not assume the
>> processor supports the features right?
> Is that even remotely possible? If yes, then we should keep the warning,
> otherwise it is useless.
>

Yes, it is possible.

Thanks,
-Aravind.

Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address

[Aravind Gopalakrishnan]

On 2/24/2016 5:33 AM, Borislav Petkov wrote:
> On Tue, Feb 23, 2016 at 04:56:38PM -0600, Aravind Gopalakrishnan wrote:
>> I think MSR_AMD64_SMCA_MC0_MISC0 would be required in mce.c as well.
>> So might be better to retain it here.
> Actually, I'm thinking, these all are - even if used in multiple files
> - all MCE-specific and therefore used in MCE/RAS-specific code. So they
> all should go into mce.h. Everything RAS includes that header so they're
> perfectly fine there...

Hmm. We introduced MSR_AMD64_SMCA_MCx_CONFIG in this patch-
https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?id=e6c8f1873be8a14c7e44202df1f7e6ea61bf3352

Should I change that as well and move it to mce.h?

>> (It comes up to 81 chars, but will ignore checkpatch in this case..)
> The 80-cols rule is not a hard one. Here's some food for thought:
>
> https://lkml.kernel.org/r/20160219095132.GA9681@gmail.com
>

Got it.

Thanks,
-Aravind.

Re: [PATCH 3/4] x86/mce: Clarify comments regarding deferred error

[Aravind Gopalakrishnan]

On 2/24/2016 5:37 AM, Borislav Petkov wrote:
> On Tue, Feb 23, 2016 at 05:02:40PM -0600, Aravind Gopalakrishnan wrote:
>> On 2/23/16 6:11 AM, Borislav Petkov wrote:
>>> On Tue, Feb 16, 2016 at 03:45:10PM -0600, Aravind Gopalakrishnan wrote:
>>>>   /* AMD-specific bits */
>>>> -#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
>>>> +#define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare a deferred error */
>>> /* uncorrected error, deferred exception */
>>>
>>> sounds better to me.
>> Hmm. Well, Deferred error is a separate class of error by itself.
>> It's neither Corrected in HW nor is it Uncorrected like a MCE.
> Let's consult the BKDG:
>
> "Deferred: deferred error.
>
>
> So it is an uncorrected error for which the raising of the error
> exception was deferred until consumption.

Yep. Okay, I'll fix as you suggested.

>> If you feel "Uncorrected error, deferred error exception" won;t be
>> confusing, that's OK with me.
> Why would it be confusing? It is describing exactly what a deferred
> error is, albeit a bit too laconic but people can find the longer
> description.
>

That's precisely it-
I thought I wasn't descriptive enough. But yeah, I guess I can include a 
reference to BKDG as well if anyone wants a detailed description.

Thanks,
-Aravind.

Re: [PATCH 4/4] x86/mce/AMD: Add comments for easier understanding

[Aravind Gopalakrishnan]

On 2/23/2016 6:35 AM, Borislav Petkov wrote:
> On Tue, Feb 16, 2016 at 03:45:11PM -0600, Aravind Gopalakrishnan wrote:
>>   
>>   /*
>> + * Set the error_count and interrupt_enable sysfs attributes here.
>> + * This function gets called during the init phase and when someone
>> + * makes changes to either of the sysfs attributes.
>> + * During init phase, we also program Interrupt type as 'APIC' and
>> + * verify if LVT offset obtained from MCx_MISC is valid.
>>    * Called via smp_call_function_single(), must be called with correct
>>    * cpu affinity.
>>    */
> I don't think that's what threshold_restart_bank() does...

Hmm, we call this from mce_threshold_block_init() with set_lvt_off = 1 
to write LVT offset value to MCi_MISC.
And we call this from store_interrupt_enable() to program APIC INT TYPE-
         if (tr->b->interrupt_enable)
                 hi |= INT_TYPE_APIC;

and from store_threshold_limit() to re-set the "error count"-
             hi = (hi & ~MASK_ERR_COUNT_HI) |
                     (new_count & THRESHOLD_MAX);

So I thought it fit the description as to "what" it does..

> Also, that comment is too much - it shouldn't explain "what" but "why".

How about-

"This function provides user with capabilities to re-program the 
'thresold_limit' and 'interrupt_enable' sysfs attributes"


>> @@ -262,6 +267,11 @@ static int setup_APIC_deferred_error(int reserved, int new)
>>   	return reserved;
>>   }
>>   
>> +/*
>> + * Obtain LVT offset from MSR_CU_DEF_ERR and call
>> + * setup_APIC_deferred_error() to program relevant APIC register.
>> + * Also, register a deferred error interrupt handler
>> + */
> No, that's basically spelling what the code does.

Ok, I'll remove this.

>>   static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c)
>>   {
>>   	u32 low = 0, high = 0;
>> @@ -338,6 +348,14 @@ nextaddr_out:
>>   		return addr;
>>   }
>>   
>> +/*
>> + * struct threshold_block descriptor tracks useful info regarding the
>> + * banks' MISC register. Among other things, it tracks whether interrupt
>> + * is possible for the given bank, the threshold limit and the sysfs object
>> + * that outputs these info.
> That should be in form of comments explaining what the members of struct
> threshold_block are, where that struct is defined.

Ok, I'll remove comments here and add it to arch/x86/include/asm/amd_nb.h

>> Initializing the struct here, programming
>> + * LVT offset for threshold interrupts and registering a interrupt handler
>> + * if we haven't already done so
> Also spelling the code.

Will remove this

Thanks,
-Aravind.

Re: [PATCH 3/4] x86/mce: Clarify comments regarding deferred error

[Boris Petkov]

Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
>
>
>That's precisely it-
>I thought I wasn't descriptive enough. But yeah, I guess I can include
>a 
>reference to BKDG as well if anyone wants a detailed description.

No need for that. There are these things called search engines, you know...

;)


-- 
Sent from a small device: formatting sux and brevity is inevitable. 

Re: [PATCH 2/4] x86/mce/AMD: Fix logic to obtain block address

[Boris Petkov]

Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:

>Should I change that as well and move it to mce.h?

Yes please. 


-- 
Sent from a small device: formatting sux and brevity is inevitable. 

Re: [PATCH 4/4] x86/mce/AMD: Add comments for easier understanding

[Borislav Petkov]

On Wed, Feb 24, 2016 at 12:26:44PM -0600, Aravind Gopalakrishnan wrote:
> Hmm, we call this from mce_threshold_block_init() with set_lvt_off = 1 to
> write LVT offset value to MCi_MISC.
> And we call this from store_interrupt_enable() to program APIC INT TYPE-
>         if (tr->b->interrupt_enable)
>                 hi |= INT_TYPE_APIC;
> 
> and from store_threshold_limit() to re-set the "error count"-
>             hi = (hi & ~MASK_ERR_COUNT_HI) |
>                     (new_count & THRESHOLD_MAX);
> 
> So I thought it fit the description as to "what" it does..

threshold_restart_bank() reprograms the MISC MSR after sanity-checking
the fields supplied for that MSR. store_threshold_limit() sets the error
count, store_interrupt_enable() enables/disables the interrupt and both
call threshold_restart_bank() to do that.

But this is basically spelling the code now - I don't think we need to
comment in that detail.

/*
 * Called via smp_call_function_single(), must be called with correct
 * cpu affinity.
 */

is also useless.

> "This function provides user with capabilities to re-program the
> 'thresold_limit' and 'interrupt_enable' sysfs attributes"

No sorry, I don't want to be explaining every line. Just say: "Reprogram
the MISC MSR behind this threshold bank."

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 4/4] x86/mce/AMD: Add comments for easier understanding

[Aravind Gopalakrishnan]

On 2/26/2016 11:44 AM, Borislav Petkov wrote:
>
> threshold_restart_bank() reprograms the MISC MSR after sanity-checking
> the fields supplied for that MSR. store_threshold_limit() sets the error
> count, store_interrupt_enable() enables/disables the interrupt and both
> call threshold_restart_bank() to do that.
>
> But this is basically spelling the code now - I don't think we need to
> comment in that detail.

Ok, Have dropped this for V2.

> /*
>   * Called via smp_call_function_single(), must be called with correct
>   * cpu affinity.
>   */
>
> is also useless.

Will remove these as well.

>> "This function provides user with capabilities to re-program the
>> 'thresold_limit' and 'interrupt_enable' sysfs attributes"
> No sorry, I don't want to be explaining every line. Just say: "Reprogram
> the MISC MSR behind this threshold bank."
>

Ok, Will do that.

Btw, included comments around struct threshold_block to describethe members.
Do let me know if this seems OK-

  struct threshold_block {
-       unsigned int            block;
-       unsigned int            bank;
-       unsigned int            cpu;
-       u32                     address;
-       u16                     interrupt_enable;
-       bool                    interrupt_capable;
-       u16                     threshold_limit;
-       struct kobject          kobj;
-       struct list_head        miscj;
+       unsigned int            block;                  /* Threshold 
block number within bank */
+       unsigned int            bank;                   /* MCA bank the 
block belongs to */
+       unsigned int            cpu;                    /* CPU which 
controls the MCA bank */
+       u32                     address;                /* MSR address 
for the block */
+       u16                     interrupt_enable;       /* Enable/ 
Disable APIC interrupt upon threshold error */
+       bool                    interrupt_capable;      /* Specifies if 
interrupt is possible from the block */
+       u16                     threshold_limit;        /* Value upon 
which threshold interrupt is generated */
+       struct kobject          kobj;                   /* sysfs object */
+       struct list_head        miscj;                  /* Add multiple 
threshold blocks within a bank to the list */
  };

Thanks,
-Aravind.