x86/mce: Honour bios-set CMCI threshold

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.

v3: Updated messages as per Tony's inputs.
v2: Just separating out the patch. I will send a separate patch for consolidating the MCE boot flags.

Signed-off-by Naveen N. Rao <[email protected]>

---
  Documentation/x86/x86_64/boot-options.txt |    5 ++++
  arch/x86/include/asm/mce.h                |    1 +
  arch/x86/kernel/cpu/mcheck/mce.c          |   10 ++++++++
  arch/x86/kernel/cpu/mcheck/mce_intel.c    |   35 +++++++++++++++++++++++++++--
  4 files changed, 48 insertions(+), 3 deletions(-)

Documentation/x86/x86_64/boot-options.txt +5 -0

index c54b4f5..ec92540 100644
@@ -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
 

arch/x86/include/asm/mce.h +1 -0

index a3ac52b..8ad5078 100644
@@ -171,6 +171,7 @@ DECLARE_PER_CPU(struct device *, mce_device);
  #ifdef CONFIG_X86_MCE_INTEL
  extern int mce_cmci_disabled;
  extern int mce_ignore_ce;
+extern int mce_bios_cmci_threshold;
  void mce_intel_feature_init(struct cpuinfo_x86 *c);
  void cmci_clear(void);
  void cmci_reenable(void);

arch/x86/kernel/cpu/mcheck/mce.c +10 -0

index c311122..29e87d3 100644
@@ -83,6 +83,7 @@ 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;
+int				mce_bios_cmci_threshold	__read_mostly;
 
  struct mce_bank                *mce_banks		__read_mostly;
 

@@ -1946,6 +1947,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)
  {

@@ -1965,6 +1967,8 @@ static int __init mcheck_enable(char *str)
  		mce_ignore_ce = 1;
  	else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
  		mce_bootlog = (str[0] == 'b');
+	else if (!strcmp(str, "bios_cmci_threshold"))
+		mce_bios_cmci_threshold = 1;
  	else if (isdigit(str[0])) {
  		get_option(&str, &tolerant);
  		if (*str == ',') {

@@ -2205,6 +2209,11 @@ static struct dev_ext_attribute dev_attr_cmci_disabled = {
  	&mce_cmci_disabled
  };
 
+static struct dev_ext_attribute dev_attr_bios_cmci_threshold = {
+	__ATTR(bios_cmci_threshold, 0444, device_show_int, NULL),
+	&mce_bios_cmci_threshold
+};
+
  static struct device_attribute *mce_device_attrs[] = {
  	&dev_attr_tolerant.attr,
  	&dev_attr_check_interval.attr,

@@ -2213,6 +2222,7 @@ static struct device_attribute *mce_device_attrs[] = {
  	&dev_attr_dont_log_ce.attr,
  	&dev_attr_ignore_ce.attr,
  	&dev_attr_cmci_disabled.attr,
+	&dev_attr_bios_cmci_threshold.attr,
  	NULL
  };
 

arch/x86/kernel/cpu/mcheck/mce_intel.c +32 -3

index 098386f..5f88abf 100644
@@ -181,10 +181,12 @@ 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;
 
  	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 +200,20 @@ static void cmci_discover(int banks)
  			continue;
  		}
 
-		val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
-		val |= MCI_CTL2_CMCI_EN | CMCI_THRESHOLD;
+		if (!mce_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 +221,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_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_bios_cmci_threshold && bios_wrong_thresh) {
+		pr_info_once(
+			"bios_cmci_threshold: Some banks do not have valid thresholds set\n");
+		pr_info_once(
+			"bios_cmci_threshold: Make sure your BIOS supports this boot option\n");
+	}
  }
 
  /*

@@ -249,7 +278,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));
  	}

Hi Tony,
Can you kindly take in this patch if there are no further comments?

Thanks,
Naveen

Quoted 82 lines · Naveen N. Rao

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.

v3: Updated messages as per Tony's inputs.
v2: Just separating out the patch. I will send a separate patch for consolidating the MCE boot flags.

Signed-off-by Naveen N. Rao <[email protected]>

---
 Documentation/x86/x86_64/boot-options.txt |    5 ++++
 arch/x86/include/asm/mce.h                |    1 +
 arch/x86/kernel/cpu/mcheck/mce.c          |   10 ++++++++
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |   35 +++++++++++++++++++++++++++--
 4 files changed, 48 insertions(+), 3 deletions(-)

Documentation/x86/x86_64/boot-options.txt +5 -0

index c54b4f5..ec92540 100644
@@ -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
 

arch/x86/include/asm/mce.h +1 -0

index a3ac52b..8ad5078 100644
@@ -171,6 +171,7 @@ DECLARE_PER_CPU(struct device *, mce_device);
 #ifdef CONFIG_X86_MCE_INTEL
 extern int mce_cmci_disabled;
 extern int mce_ignore_ce;
+extern int mce_bios_cmci_threshold;
 void mce_intel_feature_init(struct cpuinfo_x86 *c);
 void cmci_clear(void);
 void cmci_reenable(void);

arch/x86/kernel/cpu/mcheck/mce.c +4 -0

index c311122..29e87d3 100644
@@ -83,6 +83,7 @@ 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;
+int				mce_bios_cmci_threshold	__read_mostly;
 
 struct mce_bank                *mce_banks		__read_mostly;
 

@@ -1946,6 +1947,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)
 {

@@ -1965,6 +1967,8 @@ static int __init mcheck_enable(char *str)
 		mce_ignore_ce = 1;
 	else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
 		mce_bootlog = (str[0] == 'b');
+	else if (!strcmp(str, "bios_cmci_threshold"))
+		mce_bios_cmci_threshold = 1;
 	else if (isdigit(str[0])) {
 		get_option(&str, &tolerant);
 		if (*str == ',') {

@@ -2205,6 +2209,11 @@ static struct dev_ext_attribute dev_attr_cmci_disabled = {
 	&mce_cmci_disabled
 };
 

arch/x86/kernel/cpu/mcheck/mce.c static struct dev_ext_attribute dev_attr_cmci_disabled = { · line 2214

Quoted 3 lines

arch/x86/kernel/cpu/mcheck/mce.c +3 -0

+static struct dev_ext_attribute dev_attr_bios_cmci_threshold = {
+	__ATTR(bios_cmci_threshold, 0444, device_show_int, NULL),
+	&mce_bios_cmci_threshold

Quoted 4 lines

arch/x86/kernel/cpu/mcheck/mce.c static struct dev_ext_attribute dev_attr_cmci_disabled = { · line 2214

Quoted 4 lines

arch/x86/kernel/cpu/mcheck/mce.c +3 -0

+static struct dev_ext_attribute dev_attr_bios_cmci_threshold = {
+	__ATTR(bios_cmci_threshold, 0444, device_show_int, NULL),
+	&mce_bios_cmci_threshold

Ok, I just noticed this (we must've missed it during review) but why is this read-only? If it has to be read-only, why do we have a node for this in sysfs instead of simply issuing the printk statements below and people who are interested in this, can grep dmesg?

Quoted 9 lines · Borislav Petkov

Quoted 4 lines

arch/x86/kernel/cpu/mcheck/mce.c static struct dev_ext_attribute dev_attr_cmci_disabled = { · line 2214

Quoted 4 lines

arch/x86/kernel/cpu/mcheck/mce.c +3 -0

+static struct dev_ext_attribute dev_attr_bios_cmci_threshold = {
+	__ATTR(bios_cmci_threshold, 0444, device_show_int, NULL),
+	&mce_bios_cmci_threshold

Ok, I just noticed this (we must've missed it during review) but why is this read-only? If it has to be read-only, why do we have a node for this in sysfs instead of simply issuing the printk statements below and people who are interested in this, can grep dmesg?

This was added so that user-space tools could find out if we're using thresholds for CMCI.

Quoted 11 lines · Naveen N. Rao

Quoted 9 lines · Borislav Petkov

Quoted 4 lines

arch/x86/kernel/cpu/mcheck/mce.c static struct dev_ext_attribute dev_attr_cmci_disabled = { · line 2214

Quoted 4 lines

arch/x86/kernel/cpu/mcheck/mce.c +3 -0

+static struct dev_ext_attribute dev_attr_bios_cmci_threshold = {
+	__ATTR(bios_cmci_threshold, 0444, device_show_int, NULL),
+	&mce_bios_cmci_threshold

Ok, I just noticed this (we must've missed it during review) but why is this read-only? If it has to be read-only, why do we have a node for this in sysfs instead of simply issuing the printk statements below and people who are interested in this, can grep dmesg?

Quoted 2 lines

This was added so that user-space tools could find out if we're using thresholds for CMCI.

That I figured out.

What I can't figure out is why userspace tools need to know that - the fact that some MCI_CTL2 has a 0 CMCI threshold because BIOS forgot to set it correctly? IOW, this is a rather evolved workaround for b0rked BIOS (the gazillionth BIOS f*ckup, btw if someone is counting :)) and, on top of that, we have a read-only, special sysfs node which is pretty useless to me.

Why?

Userspace tools need this sysfs attribute so they know how to react on receipt of a corrected error event: whether this is the first event or if such events have already been threshold-ed.

What's wrong with userspace tools parsing /proc/cmdline and seeing that mce_bios_cmci_threshold has been set since this is the only way to set it anyway?

Quoted 3 lines

What's wrong with userspace tools parsing /proc/cmdline and seeing that mce_bios_cmci_threshold has been set since this is the only way to set it anyway?

The argument might be on the command line, but may have been rejected because the BIOS didn't set the thresholds? So then you'd have to look at the command line, *and* check /var/log/messages to make sure we hadn't printed the message saying the BIOS was unsupportive.

BUT ... I don't think that knowing this is sufficient. A userspace tool would want to know what value had been set for each bank. So if it really wants to do something interesting, just knowing that "bios set some thresholds" doesn't sound like enough information.

BUT (squared) do you even really need to know that thresholds were set? You could look at bits {52:38} in the MCi_STATUS information for the bank to see how many corrected errors had been logged.

-Tony �{.n�+�������+%��lzwm��b�맲��r��zX���iȧ���ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z��w���?����&�)ߢf

BUT (squared) do you even really need to know that thresholds were set? You could look at bits {52:38} in the MCi_STATUS information for the bank to see how many corrected errors had been logged.

Quoted 3 lines · Tony

BUT (squared) do you even really need to know that thresholds were set? You could look at bits {52:38} in the MCi_STATUS information for the bank to see how many corrected errors had been logged.

Ah, nice. I think we should be able to use this instead of the sysfs attribute.

@Tony: I'll send it upwards soonish in case there are no objections. This way no stable backport will be needed.