linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds
@ 2012-09-05 10:21 Naveen N. Rao
  2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Naveen N. Rao @ 2012-09-05 10:21 UTC (permalink / raw)
  To: tony.luck, andi, bp
  Cc: gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh,
	linux-edac

Hello,
I'm putting together my previous set of patches for MCE.

The first two patches are cleanups, as per previous discussions. The first
patch makes mce sysfs tunables available under global machinecheck sysfs
directory. We no longer remove the existing entries in the per-cpu location.
The second patch consolidates some of the mce boot options into a single
bitfield. The structure name has been changed and is now in mce-internal.h

The third patch adds a new boot option to honour bios-set CMCI thresholds.
This has been rebased on the above patches.

This patch series applies on top of -tip.


Thanks,
Naveen

---

Naveen N. Rao (3):
      x86/mce: Make sysfs tunables available globally across all cpus
      x86/mce: Pack boolean MCE flags into a structure
      x86/mce: Honour bios-set CMCI threshold


 Documentation/x86/x86_64/boot-options.txt |    5 +
 Documentation/x86/x86_64/machinecheck     |    4 +
 arch/x86/include/asm/mce.h                |    2 -
 arch/x86/kernel/cpu/mcheck/mce-internal.h |   10 +++
 arch/x86/kernel/cpu/mcheck/mce.c          |  112 ++++++++++++++++++++++-------
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |   41 ++++++++++-
 6 files changed, 140 insertions(+), 34 deletions(-)

-- 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus
  2012-09-05 10:21 [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds Naveen N. Rao
@ 2012-09-05 10:21 ` Naveen N. Rao
  2012-09-05 10:32   ` [PATCH] [mcelog] Start using the new sysfs tunables location Naveen N. Rao
  2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao
  2012-09-05 10:22 ` [PATCH 3/3] x86/mce: Honour bios-set CMCI threshold Naveen N. Rao
  2 siblings, 1 reply; 16+ messages in thread
From: Naveen N. Rao @ 2012-09-05 10:21 UTC (permalink / raw)
  To: tony.luck, andi, bp
  Cc: gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh,
	linux-edac

All the MCE attributes currently exported via sysfs appear under
/sys/devices/system/machinecheck/machinecheck<n>/. Pretty much all of
these are global in nature and not specific to a processor. So, make these
available under /sys/devices/system/machinecheck/ where they rightly belong.
Update documentation to also point to the new location so that user-space
tools can pick up on the new location. We would eventually want to remove
these from the per-cpu location.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 Documentation/x86/x86_64/machinecheck |    4 ++--
 arch/x86/kernel/cpu/mcheck/mce.c      |   24 +++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/x86/x86_64/machinecheck b/Documentation/x86/x86_64/machinecheck
index b1fb302..02b84a6 100644
--- a/Documentation/x86/x86_64/machinecheck
+++ b/Documentation/x86/x86_64/machinecheck
@@ -31,8 +31,8 @@ bankNctl
 	Note that BIOS maintain another mask to disable specific events
 	per bank.  This is not visible here
 
-The following entries appear for each CPU, but they are truly shared
-between all CPUs.
+The following entries are shared between all CPUs and appear under
+/sys/devices/system/machinecheck:
 
 check_interval
 	How often to poll for corrected machine check errors, in seconds
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index c311122..bf276eb 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2205,6 +2205,7 @@ static struct dev_ext_attribute dev_attr_cmci_disabled = {
 	&mce_cmci_disabled
 };
 
+/* Use this _only_ for per-cpu attributes */
 static struct device_attribute *mce_device_attrs[] = {
 	&dev_attr_tolerant.attr,
 	&dev_attr_check_interval.attr,
@@ -2216,6 +2217,27 @@ static struct device_attribute *mce_device_attrs[] = {
 	NULL
 };
 
+/* All new global attributes go here */
+static struct attribute *mce_device_global_attrs[] = {
+	&dev_attr_tolerant.attr.attr,
+	&dev_attr_check_interval.attr.attr,
+	&dev_attr_trigger.attr,
+	&dev_attr_monarch_timeout.attr.attr,
+	&dev_attr_dont_log_ce.attr.attr,
+	&dev_attr_ignore_ce.attr.attr,
+	&dev_attr_cmci_disabled.attr.attr,
+	NULL
+};
+
+static struct attribute_group mce_device_attr_group = {
+	.attrs = mce_device_global_attrs,
+};
+
+static const struct attribute_group *mce_device_attr_groups[] = {
+	&mce_device_attr_group,
+	NULL,
+};
+
 static cpumask_var_t mce_device_initialized;
 
 static void mce_device_release(struct device *dev)
@@ -2397,7 +2419,7 @@ static __init int mcheck_init_device(void)
 
 	mce_init_banks();
 
-	err = subsys_system_register(&mce_subsys, NULL);
+	err = subsys_system_register(&mce_subsys, mce_device_attr_groups);
 	if (err)
 		return err;
 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure
  2012-09-05 10:21 [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds Naveen N. Rao
  2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao
@ 2012-09-05 10:22 ` Naveen N. Rao
  2012-09-05 17:15   ` Joe Perches
  2012-09-05 18:56   ` Tony Luck
  2012-09-05 10:22 ` [PATCH 3/3] x86/mce: Honour bios-set CMCI threshold Naveen N. Rao
  2 siblings, 2 replies; 16+ messages in thread
From: Naveen N. Rao @ 2012-09-05 10:22 UTC (permalink / raw)
  To: tony.luck, andi, bp
  Cc: gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh,
	linux-edac

Many MCE flags are boolean in nature, but are declared as integers
currently. We can pack these into a bitfield to save some space.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/x86/include/asm/mce.h                |    2 -
 arch/x86/kernel/cpu/mcheck/mce-internal.h |    9 +++
 arch/x86/kernel/cpu/mcheck/mce.c          |   88 +++++++++++++++++++----------
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |    2 -
 4 files changed, 67 insertions(+), 34 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index a3ac52b..4576930 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -169,8 +169,6 @@ DECLARE_PER_CPU(struct device *, mce_device);
 #define MAX_NR_BANKS 32
 
 #ifdef CONFIG_X86_MCE_INTEL
-extern int mce_cmci_disabled;
-extern int mce_ignore_ce;
 void mce_intel_feature_init(struct cpuinfo_x86 *c);
 void cmci_clear(void);
 void cmci_reenable(void);
diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
index 6a05c1d..9a165a2 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
@@ -11,6 +11,15 @@ enum severity_level {
 	MCE_PANIC_SEVERITY,
 };
 
+struct mce_config {
+	__u32	cmci_disabled		: 1,
+		ignore_ce		: 1,
+		dont_log_ce		: 1,
+		__pad			: 29;
+};
+
+extern struct mce_config mce_cfg;
+
 #define ATTR_LEN		16
 
 /* One object for each MCE bank, shared by all CPUs */
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index bf276eb..cbdef73 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -79,11 +79,10 @@ static int			rip_msr			__read_mostly;
 static int			mce_bootlog		__read_mostly = -1;
 static int			monarch_timeout		__read_mostly = -1;
 static int			mce_panic_timeout	__read_mostly;
-static int			mce_dont_log_ce		__read_mostly;
-int				mce_cmci_disabled	__read_mostly;
-int				mce_ignore_ce		__read_mostly;
 int				mce_ser			__read_mostly;
 
+struct mce_config		mce_cfg			__read_mostly;
+
 struct mce_bank                *mce_banks		__read_mostly;
 
 /* User mode helper program triggered by machine check event */
@@ -630,7 +629,7 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		 * Don't get the IP here because it's unlikely to
 		 * have anything to do with the actual error location.
 		 */
-		if (!(flags & MCP_DONTLOG) && !mce_dont_log_ce)
+		if (!(flags & MCP_DONTLOG) && !mce_cfg.dont_log_ce)
 			mce_log(&m);
 
 		/*
@@ -1634,7 +1633,7 @@ static void mce_start_timer(unsigned int cpu, struct timer_list *t)
 
 	__this_cpu_write(mce_next_interval, iv);
 
-	if (mce_ignore_ce || !iv)
+	if (mce_cfg.ignore_ce || !iv)
 		return;
 
 	t->expires = round_jiffies(jiffies + iv);
@@ -1958,11 +1957,11 @@ static int __init mcheck_enable(char *str)
 	if (!strcmp(str, "off"))
 		mce_disabled = 1;
 	else if (!strcmp(str, "no_cmci"))
-		mce_cmci_disabled = 1;
+		mce_cfg.cmci_disabled = 1;
 	else if (!strcmp(str, "dont_log_ce"))
-		mce_dont_log_ce = 1;
+		mce_cfg.dont_log_ce = 1;
 	else if (!strcmp(str, "ignore_ce"))
-		mce_ignore_ce = 1;
+		mce_cfg.ignore_ce = 1;
 	else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
 		mce_bootlog = (str[0] == 'b');
 	else if (isdigit(str[0])) {
@@ -2129,6 +2128,34 @@ static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
 	return strlen(mce_helper) + !!p;
 }
 
+static ssize_t get_dont_log_ce(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", mce_cfg.dont_log_ce);
+}
+
+static ssize_t set_dont_log_ce(struct device *s,
+			       struct device_attribute *attr,
+			       const char *buf, size_t size)
+{
+	u64 new;
+
+	if (strict_strtoull(buf, 0, &new) < 0)
+		return -EINVAL;
+
+	mce_cfg.dont_log_ce = !!new;
+
+	return size;
+}
+
+static ssize_t get_ignore_ce(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", mce_cfg.ignore_ce);
+}
+
 static ssize_t set_ignore_ce(struct device *s,
 			     struct device_attribute *attr,
 			     const char *buf, size_t size)
@@ -2138,21 +2165,28 @@ static ssize_t set_ignore_ce(struct device *s,
 	if (strict_strtoull(buf, 0, &new) < 0)
 		return -EINVAL;
 
-	if (mce_ignore_ce ^ !!new) {
+	if (mce_cfg.ignore_ce ^ !!new) {
 		if (new) {
 			/* disable ce features */
 			mce_timer_delete_all();
 			on_each_cpu(mce_disable_cmci, NULL, 1);
-			mce_ignore_ce = 1;
+			mce_cfg.ignore_ce = 1;
 		} else {
 			/* enable ce features */
-			mce_ignore_ce = 0;
+			mce_cfg.ignore_ce = 0;
 			on_each_cpu(mce_enable_ce, (void *)1, 1);
 		}
 	}
 	return size;
 }
 
+static ssize_t get_cmci_disabled(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", mce_cfg.cmci_disabled);
+}
+
 static ssize_t set_cmci_disabled(struct device *s,
 				 struct device_attribute *attr,
 				 const char *buf, size_t size)
@@ -2162,14 +2196,14 @@ static ssize_t set_cmci_disabled(struct device *s,
 	if (strict_strtoull(buf, 0, &new) < 0)
 		return -EINVAL;
 
-	if (mce_cmci_disabled ^ !!new) {
+	if (mce_cfg.cmci_disabled ^ !!new) {
 		if (new) {
 			/* disable cmci */
 			on_each_cpu(mce_disable_cmci, NULL, 1);
-			mce_cmci_disabled = 1;
+			mce_cfg.cmci_disabled = 1;
 		} else {
 			/* enable cmci */
-			mce_cmci_disabled = 0;
+			mce_cfg.cmci_disabled = 0;
 			on_each_cpu(mce_enable_ce, NULL, 1);
 		}
 	}
@@ -2188,32 +2222,24 @@ static ssize_t store_int_with_restart(struct device *s,
 static DEVICE_ATTR(trigger, 0644, show_trigger, set_trigger);
 static DEVICE_INT_ATTR(tolerant, 0644, tolerant);
 static DEVICE_INT_ATTR(monarch_timeout, 0644, monarch_timeout);
-static DEVICE_INT_ATTR(dont_log_ce, 0644, mce_dont_log_ce);
+static DEVICE_ATTR(dont_log_ce, 0644, get_dont_log_ce, set_dont_log_ce);
+static DEVICE_ATTR(ignore_ce, 0644, get_ignore_ce, set_ignore_ce);
+static DEVICE_ATTR(cmci_disabled, 0644, get_cmci_disabled, set_cmci_disabled);
 
 static struct dev_ext_attribute dev_attr_check_interval = {
 	__ATTR(check_interval, 0644, device_show_int, store_int_with_restart),
 	&check_interval
 };
 
-static struct dev_ext_attribute dev_attr_ignore_ce = {
-	__ATTR(ignore_ce, 0644, device_show_int, set_ignore_ce),
-	&mce_ignore_ce
-};
-
-static struct dev_ext_attribute dev_attr_cmci_disabled = {
-	__ATTR(cmci_disabled, 0644, device_show_int, set_cmci_disabled),
-	&mce_cmci_disabled
-};
-
 /* Use this _only_ for per-cpu attributes */
 static struct device_attribute *mce_device_attrs[] = {
 	&dev_attr_tolerant.attr,
 	&dev_attr_check_interval.attr,
 	&dev_attr_trigger,
 	&dev_attr_monarch_timeout.attr,
-	&dev_attr_dont_log_ce.attr,
-	&dev_attr_ignore_ce.attr,
-	&dev_attr_cmci_disabled.attr,
+	&dev_attr_dont_log_ce,
+	&dev_attr_ignore_ce,
+	&dev_attr_cmci_disabled,
 	NULL
 };
 
@@ -2223,9 +2249,9 @@ static struct attribute *mce_device_global_attrs[] = {
 	&dev_attr_check_interval.attr.attr,
 	&dev_attr_trigger.attr,
 	&dev_attr_monarch_timeout.attr.attr,
-	&dev_attr_dont_log_ce.attr.attr,
-	&dev_attr_ignore_ce.attr.attr,
-	&dev_attr_cmci_disabled.attr.attr,
+	&dev_attr_dont_log_ce.attr,
+	&dev_attr_ignore_ce.attr,
+	&dev_attr_cmci_disabled.attr,
 	NULL
 };
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index 098386f..a6c028d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
@@ -53,7 +53,7 @@ static int cmci_supported(int *banks)
 {
 	u64 cap;
 
-	if (mce_cmci_disabled || mce_ignore_ce)
+	if (mce_cfg.cmci_disabled || mce_cfg.ignore_ce)
 		return 0;
 
 	/*


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/3] x86/mce: Honour bios-set CMCI threshold
  2012-09-05 10:21 [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds Naveen N. Rao
  2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao
  2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao
@ 2012-09-05 10:22 ` Naveen N. Rao
  2 siblings, 0 replies; 16+ messages in thread
From: Naveen N. Rao @ 2012-09-05 10:22 UTC (permalink / raw)
  To: tony.luck, andi, bp
  Cc: gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh,
	linux-edac

The ACPI spec doesn't provide for a way for the bios to pass down
recommended thresholds to the OS on a _per-bank_ basis. This patch adds
a new boot option, which if passed, allows bios to initialize the CMCI
threshold. In such a case, we simply skip programming any threshold
value.

As fail-safe, we initialize threshold to 1 if some banks have not been
initialized by the bios and warn the user.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 Documentation/x86/x86_64/boot-options.txt |    5 ++++
 arch/x86/kernel/cpu/mcheck/mce-internal.h |    3 +-
 arch/x86/kernel/cpu/mcheck/mce.c          |   12 +++++++++
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |   39 +++++++++++++++++++++++++++--
 4 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index c54b4f5..ec92540 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -50,6 +50,11 @@ Machine check
 		monarchtimeout:
 		Sets the time in us to wait for other CPUs on machine checks. 0
 		to disable.
+   mce=bios_cmci_threshold
+		Don't overwrite the bios-set CMCI threshold. This boot option
+		prevents Linux from overwriting the CMCI threshold set by the
+		bios. Without this option, Linux always sets the CMCI
+		threshold to 1.
 
    nomce (for compatibility with i386): same as mce=off
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
index 9a165a2..cfc2852 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
@@ -15,7 +15,8 @@ struct mce_config {
 	__u32	cmci_disabled		: 1,
 		ignore_ce		: 1,
 		dont_log_ce		: 1,
-		__pad			: 29;
+		bios_cmci_threshold	: 1,
+		__pad			: 28;
 };
 
 extern struct mce_config mce_cfg;
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index cbdef73..e0985a7 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1945,6 +1945,7 @@ static struct miscdevice mce_chrdev_device = {
  *	check, or 0 to not wait
  * mce=bootlog Log MCEs from before booting. Disabled by default on AMD.
  * mce=nobootlog Don't log MCEs from before booting.
+ * mce=bios_cmci_threshold Don't program the CMCI threshold
  */
 static int __init mcheck_enable(char *str)
 {
@@ -1964,6 +1965,8 @@ static int __init mcheck_enable(char *str)
 		mce_cfg.ignore_ce = 1;
 	else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
 		mce_bootlog = (str[0] == 'b');
+	else if (!strcmp(str, "bios_cmci_threshold"))
+		mce_cfg.bios_cmci_threshold = 1;
 	else if (isdigit(str[0])) {
 		get_option(&str, &tolerant);
 		if (*str == ',') {
@@ -2210,6 +2213,13 @@ static ssize_t set_cmci_disabled(struct device *s,
 	return size;
 }
 
+static ssize_t get_bios_cmci_threshold(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", mce_cfg.bios_cmci_threshold);
+}
+
 static ssize_t store_int_with_restart(struct device *s,
 				      struct device_attribute *attr,
 				      const char *buf, size_t size)
@@ -2225,6 +2235,7 @@ static DEVICE_INT_ATTR(monarch_timeout, 0644, monarch_timeout);
 static DEVICE_ATTR(dont_log_ce, 0644, get_dont_log_ce, set_dont_log_ce);
 static DEVICE_ATTR(ignore_ce, 0644, get_ignore_ce, set_ignore_ce);
 static DEVICE_ATTR(cmci_disabled, 0644, get_cmci_disabled, set_cmci_disabled);
+static DEVICE_ATTR(bios_cmci_threshold, 0444, get_bios_cmci_threshold, NULL);
 
 static struct dev_ext_attribute dev_attr_check_interval = {
 	__ATTR(check_interval, 0644, device_show_int, store_int_with_restart),
@@ -2252,6 +2263,7 @@ static struct attribute *mce_device_global_attrs[] = {
 	&dev_attr_dont_log_ce.attr,
 	&dev_attr_ignore_ce.attr,
 	&dev_attr_cmci_disabled.attr,
+	&dev_attr_bios_cmci_threshold.attr,
 	NULL
 };
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index a6c028d..6a2de06 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
@@ -181,10 +181,16 @@ static void cmci_discover(int banks)
 	unsigned long *owned = (void *)&__get_cpu_var(mce_banks_owned);
 	unsigned long flags;
 	int i;
+	int bios_wrong_thresh = 0;
+
+	if (mce_cfg.bios_cmci_threshold)
+		printk_once(KERN_INFO
+			"bios_cmci_threshold: Using bios-set threshold values for CMCI");
 
 	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
 	for (i = 0; i < banks; i++) {
 		u64 val;
+		int bios_zero_thresh = 0;
 
 		if (test_bit(i, owned))
 			continue;
@@ -198,8 +204,20 @@ static void cmci_discover(int banks)
 			continue;
 		}
 
-		val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
-		val |= MCI_CTL2_CMCI_EN | CMCI_THRESHOLD;
+		if (!mce_cfg.bios_cmci_threshold) {
+			val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
+			val |= CMCI_THRESHOLD;
+		} else if (!(val & MCI_CTL2_CMCI_THRESHOLD_MASK)) {
+			/*
+			 * If bios_cmci_threshold boot option was specified
+			 * but the threshold is zero, we'll try to initialize
+			 * it to 1.
+			 */
+			bios_zero_thresh = 1;
+			val |= CMCI_THRESHOLD;
+		}
+
+		val |= MCI_CTL2_CMCI_EN;
 		wrmsrl(MSR_IA32_MCx_CTL2(i), val);
 		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
 
@@ -207,11 +225,26 @@ static void cmci_discover(int banks)
 		if (val & MCI_CTL2_CMCI_EN) {
 			set_bit(i, owned);
 			__clear_bit(i, __get_cpu_var(mce_poll_banks));
+			/*
+			 * We are able to set thresholds for some banks that
+			 * had a threshold of 0. This means the BIOS has not
+			 * set the thresholds properly or does not work with
+			 * this boot option. Note down now and report later.
+			 */
+			if (mce_cfg.bios_cmci_threshold && bios_zero_thresh &&
+					(val & MCI_CTL2_CMCI_THRESHOLD_MASK))
+				bios_wrong_thresh = 1;
 		} else {
 			WARN_ON(!test_bit(i, __get_cpu_var(mce_poll_banks)));
 		}
 	}
 	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
+	if (mce_cfg.bios_cmci_threshold && bios_wrong_thresh) {
+		printk_once(KERN_INFO
+			"bios_cmci_threshold: Some banks do not have valid thresholds set");
+		printk_once(KERN_INFO
+			"bios_cmci_threshold: Make sure your BIOS supports this boot option");
+	}
 }
 
 /*
@@ -249,7 +282,7 @@ void cmci_clear(void)
 			continue;
 		/* Disable CMCI */
 		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
-		val &= ~(MCI_CTL2_CMCI_EN|MCI_CTL2_CMCI_THRESHOLD_MASK);
+		val &= ~MCI_CTL2_CMCI_EN;
 		wrmsrl(MSR_IA32_MCx_CTL2(i), val);
 		__clear_bit(i, __get_cpu_var(mce_banks_owned));
 	}


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] [mcelog] Start using the new sysfs tunables location
  2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao
@ 2012-09-05 10:32   ` Naveen N. Rao
  2012-09-05 18:47     ` Andi Kleen
  0 siblings, 1 reply; 16+ messages in thread
From: Naveen N. Rao @ 2012-09-05 10:32 UTC (permalink / raw)
  To: ak, tony.luck, andi, bp; +Cc: x86, linux-kernel, linux-edac, ananth

All the current mce tunables are now available under
/sys/devices/system/machinecheck. Start using this new location, but fall back
to the older per-cpu location so that we continue working with older kernels.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 README      |    2 +-
 mcelog.init |    5 ++++-
 tests/test  |    7 ++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/README b/README
index 08184ed..0426460 100644
--- a/README
+++ b/README
@@ -18,7 +18,7 @@ significantly (upto 10 minutes) and does not allow mcelog to keep extended state
 
 trigger is a newer method where the kernel runs mcelog on a error.
 This is configured with 
-echo /usr/sbin/mcelog > /sys/devices/system/machinecheck/machinecheck0/trigger
+echo /usr/sbin/mcelog > /sys/devices/system/machinecheck/trigger
 This is faster, but still doesn't allow mcelog to keep state,
 and has relatively high overhead for each error because a program has
 to be initialized from scratch.
diff --git a/mcelog.init b/mcelog.init
index 0abe786..5f32ba7 100755
--- a/mcelog.init
+++ b/mcelog.init
@@ -31,7 +31,10 @@ MCELOG_OPTIONS=""
 
 # private settings
 MCELOG=${MCELOG:-/usr/sbin/mcelog}
-TRIGGER=/sys/devices/system/machinecheck/machinecheck0/trigger
+TRIGGER=/sys/devices/system/machinecheck/trigger
+if [ ! -e $TRIGGER ] ; then
+	TRIGGER=/sys/devices/system/machinecheck/machinecheck0/trigger
+fi
 [ ! -x $MCELOG ] && ( echo "mcelog not found" ; exit 1 )
 [ ! -r /dev/mcelog ] && ( echo "/dev/mcelog not active" ; exit 0 )
 
diff --git a/tests/test b/tests/test
index c673eb2..52daf01 100755
--- a/tests/test
+++ b/tests/test
@@ -17,10 +17,15 @@ if [ "$(whoami)" != "root" ] ; then
 	exit 1
 fi
 
+TRIGGER=/sys/devices/system/machinecheck/trigger
+if [ ! -e $TRIGGER ] ; then
+	TRIGGER=/sys/devices/system/machinecheck/machinecheck0/trigger
+fi
+
 echo "++++++++++++ running $1 test +++++++++++++++++++"
 
 # disable trigger
-echo -n "" > /sys/devices/system/machinecheck/machinecheck0/trigger
+echo -n "" > $TRIGGER
 killall mcelog || true
 
 #killwatchdog() { 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure
  2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao
@ 2012-09-05 17:15   ` Joe Perches
  2012-09-05 18:56   ` Tony Luck
  1 sibling, 0 replies; 16+ messages in thread
From: Joe Perches @ 2012-09-05 17:15 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: tony.luck, andi, bp, gong.chen, ananth, x86, linux-kernel, mingo,
	hpa, tglx, gregkh, linux-edac

On Wed, 2012-09-05 at 15:52 +0530, Naveen N. Rao wrote:
> Many MCE flags are boolean in nature, but are declared as integers
> currently. We can pack these into a bitfield to save some space.
[]
> diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
[]
> @@ -11,6 +11,15 @@ enum severity_level {
>  	MCE_PANIC_SEVERITY,
>  };
>  
> +struct mce_config {
> +	__u32	cmci_disabled		: 1,
> +		ignore_ce		: 1,
> +		dont_log_ce		: 1,
> +		__pad			: 29;

You don't need to declare __pad
Perhaps bool:1 instead.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] [mcelog] Start using the new sysfs tunables location
  2012-09-05 10:32   ` [PATCH] [mcelog] Start using the new sysfs tunables location Naveen N. Rao
@ 2012-09-05 18:47     ` Andi Kleen
  2012-09-05 19:09       ` Tony Luck
  0 siblings, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2012-09-05 18:47 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: ak, tony.luck, andi, bp, x86, linux-kernel, linux-edac, ananth

On Wed, Sep 05, 2012 at 04:02:37PM +0530, Naveen N. Rao wrote:
> All the current mce tunables are now available under
> /sys/devices/system/machinecheck. Start using this new location, but fall back
> to the older per-cpu location so that we continue working with older kernels.

Who did that change in the kernel?

That breaks Linus rule that the kernel should not break userland.
Kernel needs to fix that.

-Andi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure
  2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao
  2012-09-05 17:15   ` Joe Perches
@ 2012-09-05 18:56   ` Tony Luck
  2012-09-06  6:48     ` Naveen N. Rao
  1 sibling, 1 reply; 16+ messages in thread
From: Tony Luck @ 2012-09-05 18:56 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: andi, bp, gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx,
	gregkh, linux-edac

On Wed, Sep 5, 2012 at 3:22 AM, Naveen N. Rao
<naveen.n.rao@linux.vnet.ibm.com> wrote:
> Many MCE flags are boolean in nature, but are declared as integers
> currently. We can pack these into a bitfield to save some space.

Before this patch:
size arch/x86/kernel/cpu/mcheck/mce.o
   text	   data	    bss	    dec	    hex	filename
  18946	   4930	    776	  24652	   604c	arch/x86/kernel/cpu/mcheck/mce.o

After:
size arch/x86/kernel/cpu/mcheck/mce.o
   text	   data	    bss	    dec	    hex	filename
  19335	   4890	    776	  25001	   61a9	arch/x86/kernel/cpu/mcheck/mce.o

So we do indeed see "data" reduced by 40 bytes. But
"text" is up by 389.  This seems to be because you have
another change, not described in the commit log, buried
in part 2 to add get_dont_log_ce(), set_dont_log_ce() etc.

Compiler version: gcc version 4.4.6 20120305 (Red Hat 4.4.6-4) (GCC)

I know I'm contradicting the feedback you got from Borislav here, but
is this code churn really worth it to save 40 bytes? I don't think so.

-Tony

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] [mcelog] Start using the new sysfs tunables location
  2012-09-05 18:47     ` Andi Kleen
@ 2012-09-05 19:09       ` Tony Luck
  2012-09-06  6:40         ` Naveen N. Rao
  2012-09-06 12:28         ` Andi Kleen
  0 siblings, 2 replies; 16+ messages in thread
From: Tony Luck @ 2012-09-05 19:09 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Naveen N. Rao, ak, bp, x86, linux-kernel, linux-edac, ananth

On Wed, Sep 5, 2012 at 11:47 AM, Andi Kleen <andi@firstfloor.org> wrote:
> On Wed, Sep 05, 2012 at 04:02:37PM +0530, Naveen N. Rao wrote:
>> All the current mce tunables are now available under
>> /sys/devices/system/machinecheck. Start using this new location, but fall back
>> to the older per-cpu location so that we continue working with older kernels.
>
> Who did that change in the kernel?
>
> That breaks Linus rule that the kernel should not break userland.
> Kernel needs to fix that.

The change is still under discussion. Stage one is to add the new global
pathnames in addition to keeping the old per-cpu ones. Also fix all utilities
(just mcelog(8) as far as we know) to prefer the new paths.

After some time[1] ... delete the old paths. This is allowable under Linus'
modified edict that you can change ABI "if nobody complains". If we wait
long enough that the new mcelog is widely deployed, then nobody should
complain.

-Tony

[1] several years - not just a kernel release or two.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] [mcelog] Start using the new sysfs tunables location
  2012-09-05 19:09       ` Tony Luck
@ 2012-09-06  6:40         ` Naveen N. Rao
  2012-09-06 12:28         ` Andi Kleen
  1 sibling, 0 replies; 16+ messages in thread
From: Naveen N. Rao @ 2012-09-06  6:40 UTC (permalink / raw)
  To: Tony Luck; +Cc: Andi Kleen, ak, bp, x86, linux-kernel, linux-edac, ananth

On 09/06/2012 12:39 AM, Tony Luck wrote:
> On Wed, Sep 5, 2012 at 11:47 AM, Andi Kleen <andi@firstfloor.org> wrote:
>> On Wed, Sep 05, 2012 at 04:02:37PM +0530, Naveen N. Rao wrote:
>>> All the current mce tunables are now available under
>>> /sys/devices/system/machinecheck. Start using this new location, but fall back
>>> to the older per-cpu location so that we continue working with older kernels.
>>
>> Who did that change in the kernel?
>>
>> That breaks Linus rule that the kernel should not break userland.
>> Kernel needs to fix that.
>
> The change is still under discussion. Stage one is to add the new global
> pathnames in addition to keeping the old per-cpu ones. Also fix all utilities
> (just mcelog(8) as far as we know) to prefer the new paths.
>
> After some time[1] ... delete the old paths. This is allowable under Linus'
> modified edict that you can change ABI "if nobody complains". If we wait
> long enough that the new mcelog is widely deployed, then nobody should
> complain.
>
> -Tony
>
> [1] several years - not just a kernel release or two.
>

Tony,
Thanks for clarifying. I should have mentioned in the patch description 
that this is indeed subject to the original patch making it into the kernel.

On a related topic. I recently noticed that we don't have an entry for 
machinecheck in Documentation/ABI/. Should we add an entry in there? We 
could perhaps add the existing entries under obsolete/ and the new 
location under testing/?


- Naveen


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure
  2012-09-05 18:56   ` Tony Luck
@ 2012-09-06  6:48     ` Naveen N. Rao
  2012-09-06 12:15       ` Borislav Petkov
  0 siblings, 1 reply; 16+ messages in thread
From: Naveen N. Rao @ 2012-09-06  6:48 UTC (permalink / raw)
  To: Tony Luck, bp
  Cc: andi, gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx,
	gregkh, linux-edac

On 09/06/2012 12:26 AM, Tony Luck wrote:
> On Wed, Sep 5, 2012 at 3:22 AM, Naveen N. Rao
> <naveen.n.rao@linux.vnet.ibm.com> wrote:
>> Many MCE flags are boolean in nature, but are declared as integers
>> currently. We can pack these into a bitfield to save some space.
>
> Before this patch:
> size arch/x86/kernel/cpu/mcheck/mce.o
>     text	   data	    bss	    dec	    hex	filename
>    18946	   4930	    776	  24652	   604c	arch/x86/kernel/cpu/mcheck/mce.o
>
> After:
> size arch/x86/kernel/cpu/mcheck/mce.o
>     text	   data	    bss	    dec	    hex	filename
>    19335	   4890	    776	  25001	   61a9	arch/x86/kernel/cpu/mcheck/mce.o
>
> So we do indeed see "data" reduced by 40 bytes. But
> "text" is up by 389.  This seems to be because you have
> another change, not described in the commit log, buried
> in part 2 to add get_dont_log_ce(), set_dont_log_ce() etc.
>
> Compiler version: gcc version 4.4.6 20120305 (Red Hat 4.4.6-4) (GCC)
>
> I know I'm contradicting the feedback you got from Borislav here, but
> is this code churn really worth it to save 40 bytes? I don't think so.

Hmm.. I think I agree. I don't see a good way to get rid of the 
individual getters and setters without adding some more code churn. I 
guess using boolean would be better. Boris?


Thanks,
Naveen


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure
  2012-09-06  6:48     ` Naveen N. Rao
@ 2012-09-06 12:15       ` Borislav Petkov
  0 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2012-09-06 12:15 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Tony Luck, andi, gong.chen, ananth, x86, linux-kernel, mingo,
	hpa, tglx, gregkh, linux-edac

On Thu, Sep 06, 2012 at 12:18:31PM +0530, Naveen N. Rao wrote:
> >I know I'm contradicting the feedback you got from Borislav here, but
> >is this code churn really worth it to save 40 bytes? I don't think
> >so.

Well, to answer Tony's question, I wanted to have all those config
booleans at the beginning of mce.c packed tightly together as a
single-bit bool values in a config-like struct so that they don't
scatter all over the place and grow out of proportion with more features
being added. So actually I'm willing to swallow the slight increase in
code size for better/more clear code.

> Hmm.. I think I agree. I don't see a good way to get rid of the
> individual getters and setters without adding some more code churn.
> I guess using boolean would be better. Boris?

Yes, it would be worth to try to add a DEVICE_BOOL_ATTR which introduce
a setter/getter flavour for bools and then use that for all.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] [mcelog] Start using the new sysfs tunables location
  2012-09-05 19:09       ` Tony Luck
  2012-09-06  6:40         ` Naveen N. Rao
@ 2012-09-06 12:28         ` Andi Kleen
  2012-09-06 12:34           ` Naveen N. Rao
  1 sibling, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2012-09-06 12:28 UTC (permalink / raw)
  To: Tony Luck
  Cc: Andi Kleen, Naveen N. Rao, bp, x86, linux-kernel, linux-edac, ananth

> The change is still under discussion. Stage one is to add the new global
> pathnames in addition to keeping the old per-cpu ones. Also fix all utilities
> (just mcelog(8) as far as we know) to prefer the new paths.

But why do you even want to change it?  Does it fix anything?
AFAIK the old setup -- while not being pretty -- works just fine.

-Andi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] [mcelog] Start using the new sysfs tunables location
  2012-09-06 12:28         ` Andi Kleen
@ 2012-09-06 12:34           ` Naveen N. Rao
  2012-09-06 12:51             ` Alan Cox
  2012-09-06 13:21             ` Andi Kleen
  0 siblings, 2 replies; 16+ messages in thread
From: Naveen N. Rao @ 2012-09-06 12:34 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Tony Luck, Andi Kleen, bp, x86, linux-kernel, linux-edac, ananth

On 09/06/2012 05:58 PM, Andi Kleen wrote:
>> The change is still under discussion. Stage one is to add the new global
>> pathnames in addition to keeping the old per-cpu ones. Also fix all utilities
>> (just mcelog(8) as far as we know) to prefer the new paths.
>
> But why do you even want to change it?  Does it fix anything?
> AFAIK the old setup -- while not being pretty -- works just fine.

The reason for this was explained in this thread:
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg298302.html

Even if we decide not to remove these tunables from under their current 
per-cpu location, I still think it is much cleaner to have them 
available under /sys/devices/system/machinecheck.


- Naveen


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] [mcelog] Start using the new sysfs tunables location
  2012-09-06 12:34           ` Naveen N. Rao
@ 2012-09-06 12:51             ` Alan Cox
  2012-09-06 13:21             ` Andi Kleen
  1 sibling, 0 replies; 16+ messages in thread
From: Alan Cox @ 2012-09-06 12:51 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Andi Kleen, Tony Luck, Andi Kleen, bp, x86, linux-kernel,
	linux-edac, ananth

On Thu, 06 Sep 2012 18:04:27 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 09/06/2012 05:58 PM, Andi Kleen wrote:
> >> The change is still under discussion. Stage one is to add the new global
> >> pathnames in addition to keeping the old per-cpu ones. Also fix all utilities
> >> (just mcelog(8) as far as we know) to prefer the new paths.
> >
> > But why do you even want to change it?  Does it fix anything?
> > AFAIK the old setup -- while not being pretty -- works just fine.
> 
> The reason for this was explained in this thread:
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg298302.html
> 
> Even if we decide not to remove these tunables from under their current 
> per-cpu location, I still think it is much cleaner to have them 
> available under /sys/devices/system/machinecheck.

That to me seems a ridiculous proposal. What are you going to do if in
future they ceased to be system wide ? Move them back ?

The threshold for playing musical chairs with sysfs nodes is a lot higher
than "I think it's much cleaner"

The current approach is a lot more futureproof even if a spot more ugly.

Alan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] [mcelog] Start using the new sysfs tunables location
  2012-09-06 12:34           ` Naveen N. Rao
  2012-09-06 12:51             ` Alan Cox
@ 2012-09-06 13:21             ` Andi Kleen
  1 sibling, 0 replies; 16+ messages in thread
From: Andi Kleen @ 2012-09-06 13:21 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Andi Kleen, Tony Luck, Andi Kleen, bp, x86, linux-kernel,
	linux-edac, ananth

> Even if we decide not to remove these tunables from under their current 
> per-cpu location, I still think it is much cleaner to have them 
> available under /sys/devices/system/machinecheck.

"much cleaner" is not sufficient justification to break an ABI.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2012-09-06 13:21 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-05 10:21 [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds Naveen N. Rao
2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao
2012-09-05 10:32   ` [PATCH] [mcelog] Start using the new sysfs tunables location Naveen N. Rao
2012-09-05 18:47     ` Andi Kleen
2012-09-05 19:09       ` Tony Luck
2012-09-06  6:40         ` Naveen N. Rao
2012-09-06 12:28         ` Andi Kleen
2012-09-06 12:34           ` Naveen N. Rao
2012-09-06 12:51             ` Alan Cox
2012-09-06 13:21             ` Andi Kleen
2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao
2012-09-05 17:15   ` Joe Perches
2012-09-05 18:56   ` Tony Luck
2012-09-06  6:48     ` Naveen N. Rao
2012-09-06 12:15       ` Borislav Petkov
2012-09-05 10:22 ` [PATCH 3/3] x86/mce: Honour bios-set CMCI threshold Naveen N. Rao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).