EDAC, skx_edac: Add address translation for non-volatile DIMMs
Borislav Petkov
maintainerPATCH v2 1/1 EDAC, skx_edac: Add address translation for non-volatile DIMMs
Quoted 8 lines · Qiuxu Zhuo
Current
skx_edacdriver 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_edacto use the wrapped ACPI DSM methods, if they are supported and there are non-volatile DIMMs populated on the system.
Quoted 5 lines
(The debugfs cleanup and test for ADXL DSM decoding will be added
in later commits).Signed-off-by Qiuxu Zhuo <[email protected]>Signed-off-by Tony Luck <[email protected]>
This SOB chain is wrong. If Tony co-developed this patch, then you need to do something like this:
Quoted 52 lines
---
Pass test:Apply the patch on top of Boris' edac-for-4.20-skx-3 branch and add the test code as below in debugfs_u64_set(): struct mce m; memset(&m, 0, sizeof(m)); m.status = MCI_STATUS_ADDRV + 0x90; m.status |= BIT_ULL(MCI_STATUS_CEC_SHIFT); m.addr = val; skx_mce_check_error(NULL, 0, &m); Decoding via ADXL DSM and via original skx_edac code worked well on Skylake-2S + BIOS with ADXL DSM support.drivers/edac/Kconfig | 1 + drivers/edac/skx_edac.c | 207 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 189 insertions(+), 19 deletions(-)drivers/edac/Kconfig +1
-0@@ -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 yourdrivers/edac/skx_edac.c +2
-0@@ -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
drivers/edac/skx_edac.c line 60
Quoted 5 lines
drivers/edac/skx_edac.c +2
-0@@ -54,6 +56,29 @@static LIST_HEAD (skx_edac_list);static u64 skx_tolm, skx_tohm; +static char *skx_msg; +static int nvdimm_count;
unsigned int, or can you have negative nvdimm counts?
Quoted 25 lines
drivers/edac/skx_edac.c +21
-0+ +enum { +INDEX_SOCKET , +INDEX_MEMCTRL , +INDEX_CHANNEL , +INDEX_DIMM , +INDEX_MAX +}; + +static const char *const component_names[] = { + [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; +static u64 *adxl_values; +static char *adxl_msg;#define NUM_IMC 2/* memory controllers per socket */ #define NUM_CHANNELS 3/* channels per memory controller */
drivers/edac/skx_edac.c struct skx_pvt { · line 130
Quoted 9 lines
drivers/edac/skx_edac.c +3
-3@@ -100,13 +125,13 @@ struct skx_pvt {struct decoded_addr {struct skx_dev *dev;u64 addr; -int socket; -int imc; -int channel; +u64 socket; +u64 imc; +u64 channel;
This is nuts! Why are those three u64?
If adxl_values is a bunch of u64s that doesn't mean you should do that with the decoded address values too, does it? You need to cast them all in skx_adxl_decode().
drivers/edac/skx_edac.c struct skx_pvt { · line 134
Quoted 5 lines
drivers/edac/skx_edac.c +1
-1u64 chan_addr;int sktways;int chanways; -int dimm; +u64 dimm;
Ditto.
Quoted 13 lines
drivers/edac/skx_edac.c +2
-0int rank;int channel_rank;u64 rank_address;@@ -393,6 +418,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 712
Quoted 5 lines
drivers/edac/skx_edac.c +1
-1@@ -682,7 +709,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=%llu imc=%llu channel=%llu\n",
And then this change is not needed.
drivers/edac/skx_edac.c static bool skx_rir_decode(struct decoded_addr *res) · line 848
Quoted 9 lines
drivers/edac/skx_edac.c +1
-1res->addr, res->socket, res->imc, res->channel);return true; }@@ -818,7 +845,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",
This one too.
Quoted 3 lines
drivers/edac/skx_edac.c +0
-0res->addr, res->dimm, res->rank, res->channel_rank, res->rank_address);return true;
...
Quoted 61 lines
drivers/edac/skx_edac.c +57
-0+static void __init skx_adxl_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" ); + } + + adxl_msg =kzalloc (MSG_SIZE ,GFP_KERNEL ); +if (!adxl_msg) { + adxl_component_count =0 ; +kfree (adxl_values); +edac_dbg (0 ,"No memory for adxl_msg\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_adxl_put(void) +{ +kfree (adxl_values); +kfree (adxl_msg); +} +/* * skx_init: * make sure we are running on the correct cpu model
drivers/edac/skx_edac.c static int __init skx_init(void) · line 1319
Quoted 6 lines
drivers/edac/skx_edac.c +3
-0@@ -1158,6 +1314,16 @@ static int __init skx_init(void) } } + skx_msg =kzalloc (MSG_SIZE ,GFP_KERNEL ); +if (!skx_msg) { +edac_dbg (2 ,"No memory for skx_msg\n" );
WARNING: Possible unnecessary 'out of memory' message #358: FILE: drivers/edac/skx_edac.c:1319:
+ if (!skx_msg) {
+ edac_dbg(2, "No memory for skx_msg\n");
You have a couple more allocation error messages which you don't need either.