[RFC PATCH 0/4] Teach EDAC driver about NVDIMMs

[RFC PATCH 0/4] Teach EDAC driver about NVDIMMs

[Tony Luck]

A Skylake server may have some DIMM slots filled with NVDIMMs
instead of normal DDR4 DIMMs. These are enumerated differently
by the memory controller.

Sadly there isn't an easy way to just peek at some memory controller
register to find the size of these DIMMs, so we have to rely on the
NFIT and SMBIOS tables to get that information.

This series only tackles the topology function of the EDAC
driver.  A later series of patches will fix the address translation
parts so that errors in NVDIMMs will be reported correctly.

It's marked "RFC" because it depends on the new ACPCIA version 20171110
which has only just made it to Rafael's tree.

Some of you may only care about some of the parts that touch code you
maintain, but I copied you on all four because you might like to see
the bigger picture.

Tony Luck (4):
  acpi, nfit: Add function to look up nvdimm device and provide SMBIOS
    handle
  firmware: dmi: Add function to look up a handle and return DIMM size
  edac: Add new memory type for non-volatile DIMMs
  EDAC, skx_edac: Detect non-volatile DIMMs

 drivers/acpi/nfit/core.c     | 27 +++++++++++++++++++++
 drivers/edac/Kconfig         |  2 ++
 drivers/edac/edac_mc.c       |  1 +
 drivers/edac/edac_mc_sysfs.c |  3 ++-
 drivers/edac/skx_edac.c      | 56 ++++++++++++++++++++++++++++++++++++++++----
 drivers/firmware/dmi_scan.c  | 29 +++++++++++++++++++++++
 include/acpi/nfit.h          | 19 +++++++++++++++
 include/linux/dmi.h          |  2 ++
 include/linux/edac.h         |  3 +++
 9 files changed, 136 insertions(+), 6 deletions(-)
 create mode 100644 include/acpi/nfit.h


base-commit: 3fc70f8be59950ee2deecefdddb68be19b8cddd1
-- 
2.14.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[RFC PATCH 1/4] acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle

[Tony Luck]

EDAC driver needs to look up attributes of NVDIMMs provided in SMBIOS.

Provide a function that looks up an acpi_nfit_memory_map from a device
handle (node/socket/mc/channel/dimm) and returns the SMBIOS handle.
Also pass back the "flags" so we can see if the NVDIMM is OK.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 drivers/acpi/nfit/core.c | 27 +++++++++++++++++++++++++++
 include/acpi/nfit.h      | 19 +++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 include/acpi/nfit.h

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 9c2c49b6a240..31c0dc30f88f 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/nd.h>
 #include <asm/cacheflush.h>
+#include <acpi/nfit.h>
 #include "nfit.h"
 
 /*
@@ -478,6 +479,32 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
 	return true;
 }
 
+int nfit_get_smbios_id(u32 device_handle, u16 *flags)
+{
+	struct acpi_nfit_memory_map *memdev;
+	struct acpi_nfit_desc *acpi_desc;
+	struct nfit_mem *nfit_mem;
+
+	mutex_lock(&acpi_desc_lock);
+	list_for_each_entry(acpi_desc, &acpi_descs, list) {
+		mutex_lock(&acpi_desc->init_mutex);
+		list_for_each_entry(nfit_mem, &acpi_desc->dimms, list) {
+			memdev = __to_nfit_memdev(nfit_mem);
+			if (memdev->device_handle == device_handle) {
+				mutex_unlock(&acpi_desc->init_mutex);
+				mutex_unlock(&acpi_desc_lock);
+				*flags = memdev->flags;
+				return memdev->physical_id;
+			}
+		}
+		mutex_unlock(&acpi_desc->init_mutex);
+	}
+	mutex_unlock(&acpi_desc_lock);
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(nfit_get_smbios_id);
+
 /*
  * An implementation may provide a truncated control region if no block windows
  * are defined.
diff --git a/include/acpi/nfit.h b/include/acpi/nfit.h
new file mode 100644
index 000000000000..1eee1e32e72e
--- /dev/null
+++ b/include/acpi/nfit.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright(c) 2017 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef __ACPI_NFIT_H
+#define __ACPI_NFIT_H
+
+int nfit_get_smbios_id(u32 device_handle, u16 *flags);
+
+#endif /* __ACPI_NFIT_H */
-- 
2.14.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[RFC PATCH 2/4] firmware: dmi: Add function to look up a handle and return DIMM size

[Tony Luck]

When we first scan the SMBIOS table, save the size of the DIMM.

Provide a function for other code (EDAC driver) to look up the size
of a DIMM from its SMBIOS handle.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 drivers/firmware/dmi_scan.c | 29 +++++++++++++++++++++++++++++
 include/linux/dmi.h         |  2 ++
 2 files changed, 31 insertions(+)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 783041964439..946e86fb1ec6 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -37,6 +37,7 @@ static char dmi_ids_string[128] __initdata;
 static struct dmi_memdev_info {
 	const char *device;
 	const char *bank;
+	u64 size;
 	u16 handle;
 } *dmi_memdev;
 static int dmi_memdev_nr;
@@ -395,6 +396,8 @@ static void __init save_mem_devices(const struct dmi_header *dm, void *v)
 {
 	const char *d = (const char *)dm;
 	static int nr;
+	u64 bytes;
+	u16 size;
 
 	if (dm->type != DMI_ENTRY_MEM_DEVICE || dm->length < 0x12)
 		return;
@@ -405,6 +408,18 @@ static void __init save_mem_devices(const struct dmi_header *dm, void *v)
 	dmi_memdev[nr].handle = get_unaligned(&dm->handle);
 	dmi_memdev[nr].device = dmi_string(dm, d[0x10]);
 	dmi_memdev[nr].bank = dmi_string(dm, d[0x11]);
+	size = get_unaligned((u16 *)&d[0xC]);
+	if (size == 0)
+		bytes = 0;
+	else if (size == 0xffff)
+		bytes = ~0ul;
+	else if (size & 0x8000)
+		bytes = (u64)(size & 0x7fff) << 10;
+	else if (size != 0x7fff)
+		bytes = (u64)size << 20;
+	else
+		bytes = (u64)get_unaligned((u32 *)&d[0x1C]) << 20;
+	dmi_memdev[nr].size = bytes;
 	nr++;
 }
 
@@ -1073,3 +1088,17 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
 	}
 }
 EXPORT_SYMBOL_GPL(dmi_memdev_name);
+
+u64 dmi_memdev_size(u16 handle)
+{
+	int n;
+
+	if (dmi_memdev) {
+		for (n = 0; n < dmi_memdev_nr; n++) {
+			if (handle == dmi_memdev[n].handle)
+				return dmi_memdev[n].size;
+		}
+	}
+	return ~0ul;
+}
+EXPORT_SYMBOL_GPL(dmi_memdev_size);
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 46e151172d95..7f5929123b69 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -113,6 +113,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	void *private_data);
 extern bool dmi_match(enum dmi_field f, const char *str);
 extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
+extern u64 dmi_memdev_size(u16 handle);
 
 #else
 
@@ -142,6 +143,7 @@ static inline bool dmi_match(enum dmi_field f, const char *str)
 	{ return false; }
 static inline void dmi_memdev_name(u16 handle, const char **bank,
 		const char **device) { }
+static inline u64 dmi_memdev_size(u16 handle) { return ~0ul; }
 static inline const struct dmi_system_id *
 	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
 
-- 
2.14.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[RFC PATCH 3/4] edac: Add new memory type for non-volatile DIMMs

[Tony Luck]

There are now non-volatile versions of DIMMs. Add a new entry to
"enum mem_type" and update places that use it with new strings.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/edac_mc.c       | 1 +
 drivers/edac/edac_mc_sysfs.c | 3 ++-
 include/linux/edac.h         | 3 +++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 480072139b7a..8178e74decbf 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -215,6 +215,7 @@ const char * const edac_mem_types[] = {
 	[MEM_LRDDR3]	= "Load-Reduced DDR3 RAM",
 	[MEM_DDR4]	= "Unbuffered DDR4 RAM",
 	[MEM_RDDR4]	= "Registered DDR4 RAM",
+	[MEM_NVDIMM]	= "Non-volatile RAM",
 };
 EXPORT_SYMBOL_GPL(edac_mem_types);
 
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index e4fcfa84fbd3..53cbb3518efc 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -110,7 +110,8 @@ static const char * const mem_types[] = {
 	[MEM_DDR3] = "Unbuffered-DDR3",
 	[MEM_RDDR3] = "Registered-DDR3",
 	[MEM_DDR4] = "Unbuffered-DDR4",
-	[MEM_RDDR4] = "Registered-DDR4"
+	[MEM_RDDR4] = "Registered-DDR4",
+	[MEM_NVDIMM] = "Non-volatile RAM",
 };
 
 static const char * const dev_types[] = {
diff --git a/include/linux/edac.h b/include/linux/edac.h
index cd75c173fd00..bffb97828ed6 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -186,6 +186,7 @@ static inline char *mc_event_error_type(const unsigned int err_type)
  * @MEM_RDDR4:		Registered DDR4 RAM
  *			This is a variant of the DDR4 memories.
  * @MEM_LRDDR4:		Load-Reduced DDR4 memory.
+ * @MEM_NVDIMM:		Non-volatile RAM
  */
 enum mem_type {
 	MEM_EMPTY = 0,
@@ -209,6 +210,7 @@ enum mem_type {
 	MEM_DDR4,
 	MEM_RDDR4,
 	MEM_LRDDR4,
+	MEM_NVDIMM,
 };
 
 #define MEM_FLAG_EMPTY		BIT(MEM_EMPTY)
@@ -231,6 +233,7 @@ enum mem_type {
 #define MEM_FLAG_DDR4           BIT(MEM_DDR4)
 #define MEM_FLAG_RDDR4          BIT(MEM_RDDR4)
 #define MEM_FLAG_LRDDR4         BIT(MEM_LRDDR4)
+#define MEM_FLAG_NVDIMM         BIT(MEM_NVDIMM)
 
 /**
  * enum edac-type - Error Detection and Correction capabilities and mode
-- 
2.14.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[RFC PATCH 4/4] EDAC, skx_edac: Detect non-volatile DIMMs

[Tony Luck]

This just covers the topology function of the EDAC driver.
We locate which DIMM slots are populated with NVDIMMs and
query the NFIT and SMBIOS tables to get the size.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/Kconfig    |  2 ++
 drivers/edac/skx_edac.c | 56 ++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 96afb2aeed18..5c0c4a358f67 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -232,6 +232,8 @@ config EDAC_SBRIDGE
 config EDAC_SKX
 	tristate "Intel Skylake server Integrated MC"
 	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
+	select DMI
+	select ACPI_NFIT
 	help
 	  Support for error detection and correction the Intel
 	  Skylake server Integrated Memory Controllers.
diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c
index 16dea97568a1..814a5245029c 100644
--- a/drivers/edac/skx_edac.c
+++ b/drivers/edac/skx_edac.c
@@ -14,6 +14,8 @@
 
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/pci.h>
 #include <linux/pci_ids.h>
 #include <linux/slab.h>
@@ -24,6 +26,7 @@
 #include <linux/bitmap.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
+#include <acpi/nfit.h>
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/processor.h>
@@ -298,6 +301,7 @@ static int get_dimm_attr(u32 reg, int lobit, int hibit, int add, int minval,
 }
 
 #define IS_DIMM_PRESENT(mtr)		GET_BITFIELD((mtr), 15, 15)
+#define IS_NVDIMM_PRESENT(mcddrtcfg, i)	GET_BITFIELD((mcddrtcfg), (i), (i))
 
 #define numrank(reg) get_dimm_attr((reg), 12, 13, 0, 1, 2, "ranks")
 #define numrow(reg) get_dimm_attr((reg), 2, 4, 12, 1, 6, "rows")
@@ -346,8 +350,6 @@ static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm,
 	int  banks = 16, ranks, rows, cols, npages;
 	u64 size;
 
-	if (!IS_DIMM_PRESENT(mtr))
-		return 0;
 	ranks = numrank(mtr);
 	rows = numrow(mtr);
 	cols = numcol(mtr);
@@ -379,6 +381,46 @@ static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm,
 	return 1;
 }
 
+static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,
+			   int chan, int dimmno)
+{
+	int smbios_handle;
+	u32 dev_handle;
+	u16 flags;
+	u64 size;
+
+	dev_handle = ACPI_NFIT_BUILD_DEVICE_HANDLE(dimmno, chan, imc->lmc,
+						   imc->src_id, 0);
+
+	smbios_handle = nfit_get_smbios_id(dev_handle, &flags);
+	if (smbios_handle < 0) {
+		skx_printk(KERN_ERR, "Can't find handle for NVDIMM ADR=%x\n", dev_handle);
+		return 0;
+	}
+	if (flags & ACPI_NFIT_MEM_MAP_FAILED) {
+		skx_printk(KERN_ERR, "NVDIMM ADR=%x is not mapped\n", dev_handle);
+		return 0;
+	}
+	size = dmi_memdev_size(smbios_handle);
+	if (size == ~0ul) {
+		skx_printk(KERN_ERR, "Can't find size for NVDIMM ADR=%x/SMBIOS=%x\n",
+			   dev_handle, smbios_handle);
+		return 0;
+	}
+	edac_dbg(0, "mc#%d: channel %d, dimm %d, %lld Mb (%lld pages)\n",
+		 imc->mc, chan, dimmno, size >> 20, size >> PAGE_SHIFT);
+
+	dimm->nr_pages = size >> PAGE_SHIFT;
+	dimm->grain = 32;
+	dimm->dtype = DEV_UNKNOWN;
+	dimm->mtype = MEM_NVDIMM;
+	dimm->edac_mode = EDAC_SECDED; /* likely better than this */
+	snprintf(dimm->label, sizeof(dimm->label), "CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u",
+		 imc->src_id, imc->lmc, chan, dimmno);
+
+	return 1;
+}
+
 #define SKX_GET_MTMTR(dev, reg) \
 	pci_read_config_dword((dev), 0x87c, &reg)
 
@@ -395,20 +437,24 @@ static int skx_get_dimm_config(struct mem_ctl_info *mci)
 {
 	struct skx_pvt *pvt = mci->pvt_info;
 	struct skx_imc *imc = pvt->imc;
+	u32 mtr, amap, mcddrtcfg;
 	struct dimm_info *dimm;
 	int i, j;
-	u32 mtr, amap;
 	int ndimms;
 
 	for (i = 0; i < NUM_CHANNELS; i++) {
 		ndimms = 0;
 		pci_read_config_dword(imc->chan[i].cdev, 0x8C, &amap);
+		pci_read_config_dword(imc->chan[i].cdev, 0x400, &mcddrtcfg);
 		for (j = 0; j < NUM_DIMMS; j++) {
 			dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
 					     mci->n_layers, i, j, 0);
 			pci_read_config_dword(imc->chan[i].cdev,
 					0x80 + 4*j, &mtr);
-			ndimms += get_dimm_info(mtr, amap, dimm, imc, i, j);
+			if (IS_DIMM_PRESENT(mtr))
+				ndimms += get_dimm_info(mtr, amap, dimm, imc, i, j);
+			else if (IS_NVDIMM_PRESENT(mcddrtcfg, j))
+				ndimms += get_nvdimm_info(dimm, imc, i, j);
 		}
 		if (ndimms && !skx_check_ecc(imc->chan[0].cdev)) {
 			skx_printk(KERN_ERR, "ECC is disabled on imc %d\n", imc->mc);
@@ -466,7 +512,7 @@ static int skx_register_mci(struct skx_imc *imc)
 
 	mci->ctl_name = kasprintf(GFP_KERNEL, "Skylake Socket#%d IMC#%d",
 				  imc->node_id, imc->lmc);
-	mci->mtype_cap = MEM_FLAG_DDR4;
+	mci->mtype_cap = MEM_FLAG_DDR4 | MEM_FLAG_NVDIMM;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE;
 	mci->edac_cap = EDAC_FLAG_NONE;
 	mci->mod_name = "skx_edac.c";
-- 
2.14.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 2/4] firmware: dmi: Add function to look up a handle and return DIMM size

[Borislav Petkov]

On Thu, Nov 30, 2017 at 12:40:40PM -0800, Tony Luck wrote:
> When we first scan the SMBIOS table, save the size of the DIMM.
> 
> Provide a function for other code (EDAC driver) to look up the size
> of a DIMM from its SMBIOS handle.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/firmware/dmi_scan.c | 29 +++++++++++++++++++++++++++++
>  include/linux/dmi.h         |  2 ++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 783041964439..946e86fb1ec6 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -37,6 +37,7 @@ static char dmi_ids_string[128] __initdata;
>  static struct dmi_memdev_info {
>  	const char *device;
>  	const char *bank;
> +	u64 size;
>  	u16 handle;
>  } *dmi_memdev;
>  static int dmi_memdev_nr;
> @@ -395,6 +396,8 @@ static void __init save_mem_devices(const struct dmi_header *dm, void *v)
>  {
>  	const char *d = (const char *)dm;
>  	static int nr;
> +	u64 bytes;
> +	u16 size;
>  
>  	if (dm->type != DMI_ENTRY_MEM_DEVICE || dm->length < 0x12)
>  		return;
> @@ -405,6 +408,18 @@ static void __init save_mem_devices(const struct dmi_header *dm, void *v)
>  	dmi_memdev[nr].handle = get_unaligned(&dm->handle);
>  	dmi_memdev[nr].device = dmi_string(dm, d[0x10]);
>  	dmi_memdev[nr].bank = dmi_string(dm, d[0x11]);

<---- newline here.

> +	size = get_unaligned((u16 *)&d[0xC]);
> +	if (size == 0)
> +		bytes = 0;
> +	else if (size == 0xffff)
> +		bytes = ~0ul;
> +	else if (size & 0x8000)
> +		bytes = (u64)(size & 0x7fff) << 10;
> +	else if (size != 0x7fff)
> +		bytes = (u64)size << 20;
> +	else
> +		bytes = (u64)get_unaligned((u32 *)&d[0x1C]) << 20;

<---- newline here.

> +	dmi_memdev[nr].size = bytes;
>  	nr++;
>  }
>  
> @@ -1073,3 +1088,17 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
>  	}
>  }
>  EXPORT_SYMBOL_GPL(dmi_memdev_name);
> +
> +u64 dmi_memdev_size(u16 handle)
> +{
> +	int n;
> +
> +	if (dmi_memdev) {
> +		for (n = 0; n < dmi_memdev_nr; n++) {
> +			if (handle == dmi_memdev[n].handle)
> +				return dmi_memdev[n].size;
> +		}
> +	}
> +	return ~0ul;
> +}

So dmi_memdev_name() also loops over dmi_memdev and returns bank and
device. This new function returns size.

If code is going to be calling those one after the other, you could do a

	dmi_memdev_desc(u16 handle, struct memdev_desc *desc)

which fills up a descriptor with all fields a caller would need in one
go so that you don't have to iterate multiple times. Looking at struct
dmi_memdev_info, there are no more fields so maybe this is probably
silly though or you can simply return struct dmi_memdev_info directly...

Meh.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 2/4] firmware: dmi: Add function to look up a handle and return DIMM size

[Luck, Tony]

On Mon, Dec 04, 2017 at 10:38:14PM +0100, Borislav Petkov wrote:
> On Thu, Nov 30, 2017 at 12:40:40PM -0800, Tony Luck wrote:
> >  	dmi_memdev[nr].bank = dmi_string(dm, d[0x11]);
> 
> <---- newline here.
> 
> > +	size = get_unaligned((u16 *)&d[0xC]);
> > +	if (size == 0)
> > +		bytes = 0;
> > +	else if (size == 0xffff)
> > +		bytes = ~0ul;
> > +	else if (size & 0x8000)
> > +		bytes = (u64)(size & 0x7fff) << 10;
> > +	else if (size != 0x7fff)
> > +		bytes = (u64)size << 20;
> > +	else
> > +		bytes = (u64)get_unaligned((u32 *)&d[0x1C]) << 20;
> 
> <---- newline here.

Ok ... it does read better with those breaks. Will add.

> So dmi_memdev_name() also loops over dmi_memdev and returns bank and
> device. This new function returns size.
> 
> If code is going to be calling those one after the other, you could do a
> 
> 	dmi_memdev_desc(u16 handle, struct memdev_desc *desc)

Current callers are from different code paths (dmi_memdev_name() from
ghes and EFI, dmi_memdev_size() just from the Skylake EDAC driver).
If we later find a place that calls both this would be a good cleanup.

> which fills up a descriptor with all fields a caller would need in one
> go so that you don't have to iterate multiple times. Looking at struct
> dmi_memdev_info, there are no more fields so maybe this is probably
> silly though or you can simply return struct dmi_memdev_info directly...
> 
> Meh.

I take it that you talked yourself out of asking for the cleanup now?

-Tony
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 2/4] firmware: dmi: Add function to look up a handle and return DIMM size

[Borislav Petkov]

On Mon, Dec 04, 2017 at 02:03:16PM -0800, Luck, Tony wrote:
> I take it that you talked yourself out of asking for the cleanup now?

:-)

Nah, it was just a thinking-out-loud thing. Doesn't make sense to do the
cleanup, it seems.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 3/4] edac: Add new memory type for non-volatile DIMMs

[Borislav Petkov]

On Thu, Nov 30, 2017 at 12:40:41PM -0800, Tony Luck wrote:
> There are now non-volatile versions of DIMMs. Add a new entry to
> "enum mem_type" and update places that use it with new strings.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/edac/edac_mc.c       | 1 +
>  drivers/edac/edac_mc_sysfs.c | 3 ++-
>  include/linux/edac.h         | 3 +++
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 480072139b7a..8178e74decbf 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -215,6 +215,7 @@ const char * const edac_mem_types[] = {
>  	[MEM_LRDDR3]	= "Load-Reduced DDR3 RAM",
>  	[MEM_DDR4]	= "Unbuffered DDR4 RAM",
>  	[MEM_RDDR4]	= "Registered DDR4 RAM",
> +	[MEM_NVDIMM]	= "Non-volatile RAM",
>  };
>  EXPORT_SYMBOL_GPL(edac_mem_types);
>  
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index e4fcfa84fbd3..53cbb3518efc 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -110,7 +110,8 @@ static const char * const mem_types[] = {
>  	[MEM_DDR3] = "Unbuffered-DDR3",
>  	[MEM_RDDR3] = "Registered-DDR3",
>  	[MEM_DDR4] = "Unbuffered-DDR4",
> -	[MEM_RDDR4] = "Registered-DDR4"
> +	[MEM_RDDR4] = "Registered-DDR4",
> +	[MEM_NVDIMM] = "Non-volatile RAM",
>  };

WTF, there are *two* string arrays with memory types?! How did that
happen?

Can you please remove that above in a prepatch and switch to
edac_mem_types? Or should we switch to this mem_types array instead and
kill edac_mem_types since former is visible in sysfs?

:-( Grrr.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 3/4] edac: Add new memory type for non-volatile DIMMs

[Luck, Tony]

On Mon, Dec 04, 2017 at 11:37:11PM +0100, Borislav Petkov wrote:
> On Thu, Nov 30, 2017 at 12:40:41PM -0800, Tony Luck wrote:
> WTF, there are *two* string arrays with memory types?! How did that
> happen?
> 
> Can you please remove that above in a prepatch and switch to
> edac_mem_types? Or should we switch to this mem_types array instead and
> kill edac_mem_types since former is visible in sysfs?
> 
> :-( Grrr.

Oops. I didn't stare hard enough as I walked the "grep" output
to just blindly add a new case everywhere for the new type.

Need to resolve the differences:
1) MEM_LRDDR3 only appears in one of the tables
2) edac_mem_types[] is exported from edac_mc.c and used
   by two drivers
3) mem_types[] is static in edac_mc_sysfs.c, but values
   are visible to user via sysfs
4) Strings are different between the two :-(

Since the drivers that use edac_mem_types[] only do so in edac_dbg()
output, I think it's safe to declare those strings as the non-canon
ones.

So the plan: copy the strings from mem_types[] to edac_mem_types[]
and then make edac_mc_sysfs.c pick up the exported list from edac_mc.c
instead of having its own static version.

Both files have the same build scope:

edac_core-y     := edac_mc.o edac_device.o edac_mc_sysfs.o

so there shouldn't be any weird dependency problems.

-Tony
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 4/4] EDAC, skx_edac: Detect non-volatile DIMMs

[Borislav Petkov]

On Thu, Nov 30, 2017 at 12:40:42PM -0800, Tony Luck wrote:
> This just covers the topology function of the EDAC driver.
> We locate which DIMM slots are populated with NVDIMMs and
> query the NFIT and SMBIOS tables to get the size.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/edac/Kconfig    |  2 ++
>  drivers/edac/skx_edac.c | 56 ++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 96afb2aeed18..5c0c4a358f67 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -232,6 +232,8 @@ config EDAC_SBRIDGE
>  config EDAC_SKX
>  	tristate "Intel Skylake server Integrated MC"
>  	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
> +	select DMI
> +	select ACPI_NFIT

Hmm, that ACPI_NFIT depends on a bunch of stuff and enabling EDAC_SKX
would pull in all that. Should we make this a user choice instead?

I mean, there could be boxes which don't have nvdimms so all that code
would be dead weight there...

>  	help
>  	  Support for error detection and correction the Intel
>  	  Skylake server Integrated Memory Controllers.
> diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c
> index 16dea97568a1..814a5245029c 100644
> --- a/drivers/edac/skx_edac.c
> +++ b/drivers/edac/skx_edac.c
> @@ -14,6 +14,8 @@
>  
>  #include <linux/module.h>
>  #include <linux/init.h>
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
>  #include <linux/pci.h>
>  #include <linux/pci_ids.h>
>  #include <linux/slab.h>
> @@ -24,6 +26,7 @@
>  #include <linux/bitmap.h>
>  #include <linux/math64.h>
>  #include <linux/mod_devicetable.h>
> +#include <acpi/nfit.h>
>  #include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
>  #include <asm/processor.h>
> @@ -298,6 +301,7 @@ static int get_dimm_attr(u32 reg, int lobit, int hibit, int add, int minval,
>  }
>  
>  #define IS_DIMM_PRESENT(mtr)		GET_BITFIELD((mtr), 15, 15)
> +#define IS_NVDIMM_PRESENT(mcddrtcfg, i)	GET_BITFIELD((mcddrtcfg), (i), (i))
>  
>  #define numrank(reg) get_dimm_attr((reg), 12, 13, 0, 1, 2, "ranks")
>  #define numrow(reg) get_dimm_attr((reg), 2, 4, 12, 1, 6, "rows")
> @@ -346,8 +350,6 @@ static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm,
>  	int  banks = 16, ranks, rows, cols, npages;
>  	u64 size;
>  
> -	if (!IS_DIMM_PRESENT(mtr))
> -		return 0;
>  	ranks = numrank(mtr);
>  	rows = numrow(mtr);
>  	cols = numcol(mtr);
> @@ -379,6 +381,46 @@ static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm,
>  	return 1;
>  }
>  
> +static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,
> +			   int chan, int dimmno)
> +{
> +	int smbios_handle;
> +	u32 dev_handle;
> +	u16 flags;
> +	u64 size;
> +
> +	dev_handle = ACPI_NFIT_BUILD_DEVICE_HANDLE(dimmno, chan, imc->lmc,
> +						   imc->src_id, 0);
> +
> +	smbios_handle = nfit_get_smbios_id(dev_handle, &flags);
> +	if (smbios_handle < 0) {
> +		skx_printk(KERN_ERR, "Can't find handle for NVDIMM ADR=%x\n", dev_handle);
> +		return 0;
> +	}

<---- newline here.

> +	if (flags & ACPI_NFIT_MEM_MAP_FAILED) {
> +		skx_printk(KERN_ERR, "NVDIMM ADR=%x is not mapped\n", dev_handle);
> +		return 0;
> +	}

<---- newline here.

> +	size = dmi_memdev_size(smbios_handle);
> +	if (size == ~0ul) {
> +		skx_printk(KERN_ERR, "Can't find size for NVDIMM ADR=%x/SMBIOS=%x\n",
> +			   dev_handle, smbios_handle);
> +		return 0;
> +	}

Ditto.

> +	edac_dbg(0, "mc#%d: channel %d, dimm %d, %lld Mb (%lld pages)\n",
> +		 imc->mc, chan, dimmno, size >> 20, size >> PAGE_SHIFT);
> +
> +	dimm->nr_pages = size >> PAGE_SHIFT;
> +	dimm->grain = 32;
> +	dimm->dtype = DEV_UNKNOWN;
> +	dimm->mtype = MEM_NVDIMM;
> +	dimm->edac_mode = EDAC_SECDED; /* likely better than this */

Ditto.

/me hands aegl a bunch of newlines - seems he's all out of \n's

> +	snprintf(dimm->label, sizeof(dimm->label), "CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u",
> +		 imc->src_id, imc->lmc, chan, dimmno);
> +
> +	return 1;
> +}
> +
>  #define SKX_GET_MTMTR(dev, reg) \
>  	pci_read_config_dword((dev), 0x87c, &reg)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 4/4] EDAC, skx_edac: Detect non-volatile DIMMs

[Luck, Tony]

On Tue, Dec 05, 2017 at 11:54:51AM +0100, Borislav Petkov wrote:
> On Thu, Nov 30, 2017 at 12:40:42PM -0800, Tony Luck wrote:
> >  config EDAC_SKX
> >  	tristate "Intel Skylake server Integrated MC"
> >  	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
> > +	select DMI
> > +	select ACPI_NFIT
> 
> Hmm, that ACPI_NFIT depends on a bunch of stuff and enabling EDAC_SKX
> would pull in all that. Should we make this a user choice instead?
> 
> I mean, there could be boxes which don't have nvdimms so all that code
> would be dead weight there...

I could. But what happens when someone ends up on a system with
an edac driver configured without ACPI_NFIT that does have NVDIMMs?

I can make a stub version of nfit_get_smbios_id() that returns some
error code ... and have the EDAC driver report size==0.

Would that be OK?


> <---- newline here.
> 
> > +	size = dmi_memdev_size(smbios_handle);
> > +	if (size == ~0ul) {
> > +		skx_printk(KERN_ERR, "Can't find size for NVDIMM ADR=%x/SMBIOS=%x\n",
> > +			   dev_handle, smbios_handle);
> > +		return 0;
> > +	}
> 
> Ditto.
> 
> > +	edac_dbg(0, "mc#%d: channel %d, dimm %d, %lld Mb (%lld pages)\n",
> > +		 imc->mc, chan, dimmno, size >> 20, size >> PAGE_SHIFT);
> > +
> > +	dimm->nr_pages = size >> PAGE_SHIFT;
> > +	dimm->grain = 32;
> > +	dimm->dtype = DEV_UNKNOWN;
> > +	dimm->mtype = MEM_NVDIMM;
> > +	dimm->edac_mode = EDAC_SECDED; /* likely better than this */
> 
> Ditto.
> 
> /me hands aegl a bunch of newlines - seems he's all out of \n's

/me finds that the "Enter" key on his keyboard will put in a newline.
Hurrah! now I don't have to copy/paste them from Boris.

-Tony
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 4/4] EDAC, skx_edac: Detect non-volatile DIMMs

[Borislav Petkov]

On Tue, Dec 05, 2017 at 12:03:37PM -0800, Luck, Tony wrote:
> I could. But what happens when someone ends up on a system with
> an edac driver configured without ACPI_NFIT that does have NVDIMMs?

Same thing when you land on a system with a kernel where the driver for
a piece of hw is not enabled. I mean, this won't be an issue on distros
as there *everything* is enabled but for tailored configs, where people
want skx_edac but don't need the nvdimm part.

> I can make a stub version of nfit_get_smbios_id() that returns some
> error code ... and have the EDAC driver report size==0.
> 
> Would that be OK?

Sure, thanks!

> /me finds that the "Enter" key on his keyboard will put in a newline.
> Hurrah! now I don't have to copy/paste them from Boris.

Cheers! :-)))

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 4/4] EDAC, skx_edac: Detect non-volatile DIMMs

[Luck, Tony]

On Tue, Dec 05, 2017 at 10:44:41PM +0100, Borislav Petkov wrote:
> On Tue, Dec 05, 2017 at 12:03:37PM -0800, Luck, Tony wrote:
> > I could. But what happens when someone ends up on a system with
> > an edac driver configured without ACPI_NFIT that does have NVDIMMs?
> 
> Same thing when you land on a system with a kernel where the driver for
> a piece of hw is not enabled. I mean, this won't be an issue on distros
> as there *everything* is enabled but for tailored configs, where people
> want skx_edac but don't need the nvdimm part.
> 
> > I can make a stub version of nfit_get_smbios_id() that returns some
> > error code ... and have the EDAC driver report size==0.
> > 
> > Would that be OK?
> 
> Sure, thanks!

So this is what that would look like (on top of existing patches,
but would be folded into them for next version):

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 5c0c4a358f67..7f0bc4cd5086 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -233,10 +233,11 @@ config EDAC_SKX
 	tristate "Intel Skylake server Integrated MC"
 	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
 	select DMI
-	select ACPI_NFIT
 	help
 	  Support for error detection and correction the Intel
-	  Skylake server Integrated Memory Controllers.
+	  Skylake server Integrated Memory Controllers. If your
+	  has non-volatile DIMMs you should also manually select
+	  CONFIG_ACPI_NFIT
 
 config EDAC_PND2
 	tristate "Intel Pondicherry2"
diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c
index f42e382f82b1..8374deb83246 100644
--- a/drivers/edac/skx_edac.c
+++ b/drivers/edac/skx_edac.c
@@ -387,12 +387,16 @@ static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,
 	int smbios_handle;
 	u32 dev_handle;
 	u16 flags;
-	u64 size;
+	u64 size = 0;
 
 	dev_handle = ACPI_NFIT_BUILD_DEVICE_HANDLE(dimmno, chan, imc->lmc,
 						   imc->src_id, 0);
 
 	smbios_handle = nfit_get_smbios_id(dev_handle, &flags);
+	if (smbios_handle == -EOPNOTSUPP) {
+		pr_warn_once("skx_edac: can't find size of NVDIMM\n");
+		goto unknown_size;
+	}
 	if (smbios_handle < 0) {
 		skx_printk(KERN_ERR, "Can't find handle for NVDIMM ADR=%x\n", dev_handle);
 		return 0;
@@ -410,6 +414,7 @@ static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,
 		return 0;
 	}
 
+unknown_size:
 	edac_dbg(0, "mc#%d: channel %d, dimm %d, %lld Mb (%lld pages)\n",
 		 imc->mc, chan, dimmno, size >> 20, size >> PAGE_SHIFT);
 
diff --git a/include/acpi/nfit.h b/include/acpi/nfit.h
index 1eee1e32e72e..f58e9eee6e6a 100644
--- a/include/acpi/nfit.h
+++ b/include/acpi/nfit.h
@@ -14,6 +14,13 @@
 #ifndef __ACPI_NFIT_H
 #define __ACPI_NFIT_H
 
+#if defined(CONFIG_ACPI_NFIT) || defined(CONFIG_ACPI_NFIT_MODULE)
 int nfit_get_smbios_id(u32 device_handle, u16 *flags);
+#else
+static inline int nfit_get_smbios_id(u32 device_handle, u16 *flags)
+{
+	return -EOPNOTSUPP;
+}
+#endif
 
 #endif /* __ACPI_NFIT_H */
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH 4/4] EDAC, skx_edac: Detect non-volatile DIMMs

[Borislav Petkov]

On Tue, Dec 05, 2017 at 02:24:29PM -0800, Luck, Tony wrote:
> So this is what that would look like (on top of existing patches,
> but would be folded into them for next version):
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 5c0c4a358f67..7f0bc4cd5086 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -233,10 +233,11 @@ config EDAC_SKX
>  	tristate "Intel Skylake server Integrated MC"
>  	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
>  	select DMI
> -	select ACPI_NFIT
>  	help
>  	  Support for error detection and correction the Intel
> -	  Skylake server Integrated Memory Controllers.
> +	  Skylake server Integrated Memory Controllers. If your
> +	  has non-volatile DIMMs you should also manually select
> +	  CONFIG_ACPI_NFIT
>  
>  config EDAC_PND2
>  	tristate "Intel Pondicherry2"
> diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c
> index f42e382f82b1..8374deb83246 100644
> --- a/drivers/edac/skx_edac.c
> +++ b/drivers/edac/skx_edac.c
> @@ -387,12 +387,16 @@ static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,
>  	int smbios_handle;
>  	u32 dev_handle;
>  	u16 flags;
> -	u64 size;
> +	u64 size = 0;
>  
>  	dev_handle = ACPI_NFIT_BUILD_DEVICE_HANDLE(dimmno, chan, imc->lmc,
>  						   imc->src_id, 0);
>  
>  	smbios_handle = nfit_get_smbios_id(dev_handle, &flags);
> +	if (smbios_handle == -EOPNOTSUPP) {
> +		pr_warn_once("skx_edac: can't find size of NVDIMM\n");

	" ... find size of NVDIMM. Try enabling CONFIG_ACPI_NFIT.\n"

to make it a bit more user-friendly.

Looks ok, otherwise.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm