Skip to contentSkip to conversation

EDAC, skx_edac: Add address translation for non-volatile DIMMs

PATCH New Borislav Petkov 1 participant 1 message Oct 16, 2018 EDAC-core

lore
Share & tools
lore

Borislav Petkov

author maintainer
#0 lore raw

PATCH v2 3/3 EDAC, skx_edac: Add address translation for non-volatile DIMMs

Quoted 100 lines · Tony Luck

From: Qiuxu Zhuo <[email protected]>

Current skx_edac driver doesn't support address translation for non-volatile DIMMs.

The ACPI ADXL DSM method support address translation for both volatile DIMMs and non-volatile DIMMs. So switch skx_edac to use the wrapped ACPI DSM methods, if they are supported and there are non-volatile DIMMs populated on the system.

---
 drivers/edac/Kconfig    |   1 +
 drivers/edac/skx_edac.c | 191 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 179 insertions(+), 13 deletions(-)
drivers/edac/Kconfig +1 -0
index 57304b2e989f..ffd349c12479 100644
@@ -234,6 +234,7 @@ config EDAC_SKX
 	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
 	depends on ACPI_NFIT || !ACPI_NFIT # if ACPI_NFIT=m, EDAC_SKX can't be y
 	select DMI
+	select ACPI_ADXL
 	help
 	  Support for error detection and correction the Intel
 	  Skylake server Integrated Memory Controllers. If your
drivers/edac/skx_edac.c +26 -2
index a710169abdbc..598564f9c4b2 100644
@@ -26,6 +26,7 @@
 #include <linux/bitmap.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
+#include <linux/adxl.h>
 #include <acpi/nfit.h>
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
@@ -35,6 +36,7 @@
 #include "edac_module.h"
 
 #define EDAC_MOD_STR    "skx_edac"
+#define MSG_SIZE	1024
 
 /*
  * Debug macros
@@ -54,6 +56,26 @@
 static LIST_HEAD(skx_edac_list);
 
 static u64 skx_tolm, skx_tohm;
+static int nvdimm_count;
+static u64 *adxl_values;
+
+enum {
+	INDEX_SOCKET,
+	INDEX_MEMCTRL,
+	INDEX_CHANNEL,
+	INDEX_DIMM,
+	INDEX_MAX
+};
+static char component_names[][32] = {
+	[INDEX_SOCKET]	= "ProcessorSocketId",
+	[INDEX_MEMCTRL]	= "MemoryControllerId",
+	[INDEX_CHANNEL]	= "ChannelId",
+	[INDEX_DIMM]	= "DimmSlotId",
+};
+
+static int component_indices[ARRAY_SIZE(component_names)];
+static int adxl_component_count;
+static const char * const *adxl_component_names;
 
 #define NUM_IMC			2	/* memory controllers per socket */
 #define NUM_CHANNELS		3	/* channels per memory controller */
@@ -102,11 +124,11 @@ struct decoded_addr {
 	u64	addr;
 	int	socket;
 	int	imc;
-	int	channel;
+	u64	channel;
 	u64	chan_addr;
 	int	sktways;
 	int	chanways;
-	int	dimm;
+	u64	dimm;
 	int	rank;
 	int	channel_rank;
 	u64	rank_address;
@@ -393,6 +415,8 @@ static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,
 	u16 flags;
 	u64 size = 0;
 
+	nvdimm_count++;
+
 	dev_handle = ACPI_NFIT_BUILD_DEVICE_HANDLE(dimmno, chan, imc->lmc,
 						   imc->src_id, 0);
 

drivers/edac/skx_edac.c static bool skx_sad_decode(struct decoded_addr *res) · line 709

Quoted 5 lines
drivers/edac/skx_edac.c +1 -1
@@ -682,7 +706,7 @@ static bool skx_sad_decode(struct decoded_addr *res)
 	res->imc = GET_BITFIELD(d->mcroute, lchan * 3, lchan * 3 + 2);
 	res->channel = GET_BITFIELD(d->mcroute, lchan * 2 + 18, lchan * 2 + 19);
 
-	edac_dbg(2, "%llx: socket=%d imc=%d channel=%d\n",
+	edac_dbg(2, "%llx: socket=%d imc=%d channel=%llu\n",

Prepend hex formatting with "0x". Check the whole patch pls.

Quoted 25 lines
drivers/edac/skx_edac.c +10 -1
 		 res->addr, res->socket, res->imc, res->channel);
 	return true;
 }
@@ -818,7 +842,7 @@ static bool skx_rir_decode(struct decoded_addr *res)
 	res->dimm = chan_rank / 4;
 	res->rank = chan_rank % 4;
 
-	edac_dbg(2, "%llx: dimm=%d rank=%d chan_rank=%d rank_addr=%llx\n",
+	edac_dbg(2, "%llx: dimm=%llu rank=%d chan_rank=%d rank_addr=%llx\n",
 		 res->addr, res->dimm, res->rank,
 		 res->channel_rank, res->rank_address);
 	return true;
@@ -894,12 +918,48 @@ static bool skx_decode(struct decoded_addr *res)
 		skx_rir_decode(res) && skx_mad_decode(res);
 }
 
+static bool skx_dsm_decode(u64 addr, char *msg, int msglen,
+			   u64 *sock, u64 *imc,
+			   u64 *chan, u64 *dimm)
+
+{
+	int i, len = 0;
+
+	if (addr >= skx_tohm || (addr >= skx_tolm && addr < BIT_ULL(32))) {
+		edac_dbg(0, "Address %llx out of range\n", addr);

drivers/edac/skx_edac.c static bool skx_decode(struct decoded_addr *res) · line 939

Quoted 10 lines
drivers/edac/skx_edac.c +10 -0
+		return false;
+	}
+
+	if (adxl_decode(addr, adxl_values)) {
+		edac_dbg(0, "Failed to decode 0x%llx\n", addr);
+		return false;
+	}
+
+	*sock = adxl_values[component_indices[INDEX_SOCKET]];
+	*imc = adxl_values[component_indices[INDEX_MEMCTRL]];

Align all '=' vertically.

Quoted 81 lines
drivers/edac/skx_edac.c +43 -8
+	*chan = adxl_values[component_indices[INDEX_CHANNEL]];
+	*dimm = adxl_values[component_indices[INDEX_DIMM]];
+
+	for (i = 0; i < adxl_component_count; i++) {
+		if (adxl_values[i] == ~0x0ull)
+			continue;
+		len += snprintf(msg + len, msglen, " %s:0x%llx",
+				adxl_component_names[i], adxl_values[i]);
+
+		if (msglen - len <= 0)
+			break;
+	}
+
+	return true;
+}
+
 static void skx_mce_output_error(struct mem_ctl_info *mci,
 				 const struct mce *m,
-				 struct decoded_addr *res)
+				 struct decoded_addr *res,
+				 char *msg)
 {
 	enum hw_event_mc_err_type tp_event;
-	char *type, *optype, msg[256];
+	char *type, *optype;
 	bool ripv = GET_BITFIELD(m->mcgstatus, 0, 0);
 	bool overflow = GET_BITFIELD(m->status, 62, 62);
 	bool uncorrected_error = GET_BITFIELD(m->status, 61, 61);
@@ -960,13 +1020,11 @@ static void skx_mce_output_error(struct mem_ctl_info *mci,
 		}
 	}
 
-	snprintf(msg, sizeof(msg),
-		 "%s%s err_code:%04x:%04x socket:%d imc:%d rank:%d bg:%d ba:%d row:%x col:%x",
+	snprintf(msg, MSG_SIZE,
+		 "%s%s err_code:%04x:%04x %s",
 		 overflow ? " OVERFLOW" : "",
 		 (uncorrected_error && recoverable) ? " recoverable" : "",
-		 mscod, errcode,
-		 res->socket, res->imc, res->rank,
-		 res->bank_group, res->bank_address, res->row, res->column);
+		 mscod, errcode, msg + MSG_SIZE);
 
 	edac_dbg(0, "%s\n", msg);
 
@@ -977,13 +1035,33 @@ static void skx_mce_output_error(struct mem_ctl_info *mci,
 			     optype, msg);
 }
 
+static struct mem_ctl_info *get_mci(u64 src_id, u64 lmc)
+{
+	struct skx_dev *d;
+
+	if (lmc > NUM_IMC - 1) {
+		skx_printk(KERN_ERR, "Bad mc# 0x%llx\n", lmc);
+		return NULL;
+	}
+
+	list_for_each_entry(d, &skx_edac_list, list) {
+		if (d->imc[0].src_id == src_id)
+			return d->imc[lmc].mci;
+	}
+
+	skx_printk(KERN_ERR, "No mci for src_id %llx lmc %llx\n", src_id, lmc);
+
+	return NULL;
+}
+
 static int skx_mce_check_error(struct notifier_block *nb, unsigned long val,
 			       void *data)
 {
 	struct mce *mce = (struct mce *)data;
 	struct decoded_addr res;
 	struct mem_ctl_info *mci;
-	char *type;
+	u64 socket, memctrl;
+	char *type, *msg;
 
 	if (edac_get_report_status() == EDAC_REPORTING_DISABLED)
 		return NOTIFY_DONE;

drivers/edac/skx_edac.c static int skx_mce_check_error(struct notifier_block *nb, unsigned long val, · line 1073

Quoted 4 lines
drivers/edac/skx_edac.c +1 -0
@@ -992,11 +1070,35 @@ static int skx_mce_check_error(struct notifier_block *nb, unsigned long val,
 	if ((mce->status & 0xefff) >> 7 != 1 || !(mce->status & MCI_STATUS_ADDRV))
 		return NOTIFY_DONE;
 
+	msg = kzalloc(MSG_SIZE * 2, GFP_KERNEL);

You allocate this on every notifier call. Why not allocate it once on driver init?

Also, what's that magic "* 2" thing?

drivers/edac/skx_edac.c static int skx_mce_check_error(struct notifier_block *nb, unsigned long val, · line 1085

Quoted 12 lines
drivers/edac/skx_edac.c +7 -0
+	if (!msg)
+		return NOTIFY_DONE;
+
+	if (adxl_component_count)
+		goto dsm_decoding;
+
 	res.addr = mce->addr;
 	if (!skx_decode(&res))
 		return NOTIFY_DONE;
 	mci = res.dev->imc[res.imc].mci;
 
+	snprintf(msg + MSG_SIZE, MSG_SIZE,

That's jumping over the first MSG_SIZE. Why?

Quoted 20 lines
drivers/edac/skx_edac.c +16 -0
+		 "socket:%d imc:%d chan:%llu dimm:%llu rank:%d bg:%d ba:%d row:%x col:%x",
+		 res.socket, res.imc, res.channel, res.dimm, res.rank,
+		 res.bank_group, res.bank_address, res.row, res.column);
+
+	goto decoded;
+
+dsm_decoding:
+	if (!skx_dsm_decode(mce->addr, msg + MSG_SIZE, MSG_SIZE,
+			    &socket, &memctrl, &res.channel, &res.dimm))
+		goto out;
+
+	mci = get_mci(socket, memctrl);
+	if (!mci)
+		goto out;
+
+decoded:
 	if (mce->mcgstatus & MCG_STATUS_MCIP)
 		type = "Exception";
 	else

drivers/edac/skx_edac.c static int skx_mce_check_error(struct notifier_block *nb, unsigned long val, · line 1120

Quoted 5 lines
drivers/edac/skx_edac.c +1 -1
@@ -1015,8 +1117,10 @@ static int skx_mce_check_error(struct notifier_block *nb, unsigned long val,
 			  "%u APIC %x\n", mce->cpuvendor, mce->cpuid,
 			  mce->time, mce->socketid, mce->apicid);
 
-	skx_mce_output_error(mci, mce, &res);
+	skx_mce_output_error(mci, mce, &res, msg);

Ah, there it is - this function writes the first 1024 bytes. So if it is a short first errors, there'll be a bunch of '\0' in between.

Why isn't the above snprintf() simply happening in skx_mce_output_error()?

This whole code needs a reorg - like skx_mce_check_error() does a *lot* of other stuff too.

Quoted 82 lines
drivers/edac/skx_edac.c +55 -0
+out:
+	kfree(msg);
 	return NOTIFY_DONE;
 }
 
@@ -1090,6 +1194,54 @@ static void skx_remove(void)
 	}
 }
 
+static void __init skx_dsm_get(void)
+{
+	const char * const *names;
+	int i, j;
+
+	names = adxl_get_component_names();
+	if (!names) {
+		skx_printk(KERN_NOTICE, "No firmware support for address translation.");
+		skx_printk(KERN_CONT, " Only decoding DDR4 address!\n");
+		return;
+	}
+
+	for (i = 0; i < INDEX_MAX; i++) {
+		for (j = 0; names[j]; j++) {
+			if (!strcmp(component_names[i], names[j])) {
+				component_indices[i] = j;
+				break;
+			}
+		}
+
+		if (!names[j])
+			goto err;
+	}
+
+	adxl_component_names = names;
+	while (*names++)
+		adxl_component_count++;
+
+	adxl_values = kcalloc(adxl_component_count, sizeof(*adxl_values), GFP_KERNEL);
+	if (!adxl_values) {
+		adxl_component_count = 0;
+		edac_dbg(0, "No memory for adxl_decode()\n");
+	}
+
+	return;
+err:
+	skx_printk(KERN_ERR, "'%s' is not matched from DSM parameters: ",
+		   component_names[i]);
+	for (j = 0; names[j]; j++)
+		skx_printk(KERN_CONT, "%s ", names[j]);
+	skx_printk(KERN_CONT, "\n");
+}
+
+static void __exit skx_dsm_put(void)
+{
+	kfree(adxl_values);
+}
+
 /*
  * skx_init:
  *	make sure we are running on the correct cpu model
@@ -1154,6 +1306,9 @@ static int __init skx_init(void)
 		}
 	}
 
+	if (nvdimm_count)
+		skx_dsm_get();
+
 	/* Ensure that the OPSTATE is set correctly for POLL or NMI */
 	opstate_init();
 
@@ -1170,6 +1325,8 @@ static int __init skx_init(void)
 static void __exit skx_exit(void)
 {
 	edac_dbg(2, "\n");
+	if (nvdimm_count)
+		skx_dsm_put();
 	mce_unregister_decode_chain(&skx_mce_dec);
 	skx_remove();
 	teardown_skx_debug();
@@ -1180,6 +1337,14 @@ module_exit(skx_exit);
 

drivers/edac/skx_edac.c module_exit(skx_exit); · line 1347

Quoted 10 lines
drivers/edac/skx_edac.c +8 -0
 module_param(edac_op_state, int, 0444);
 MODULE_PARM_DESC(edac_op_state, "EDAC Error Reporting state: 0=Poll,1=NMI");
+module_param_string(sock, component_names[0], sizeof(component_names[0]), 0644);
+MODULE_PARM_DESC(sock, "String to get socket ID");
+module_param_string(imc, component_names[1], sizeof(component_names[1]), 0644);
+MODULE_PARM_DESC(imc, "String to get IMC ID");
+module_param_string(chan, component_names[2], sizeof(component_names[2]), 0644);
+MODULE_PARM_DESC(chan, "String to get channel ID");
+module_param_string(dimm, component_names[3], sizeof(component_names[3]), 0644);
+MODULE_PARM_DESC(dimm, "String to get DIMM ID");

So this is not really user-friendly - do you really expect people to modprobe this with 4 module params maximum when the firmware has changed?

adxl_get_component_names() already gives the component names and you control what gets written in there so why does the driver need those at all?

Worst case you can do some trivial pattern matching but letting the user figure it out is simply wrong, IMO.