[PATCH v2] x86/mce: Honour bios-set CMCI threshold

[PATCH v2] x86/mce: Honour bios-set CMCI threshold

[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.

v2: Just separating out the patch. I will send a separate patch for
consolidating the MCE boot flags.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 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    |   39 +++++++++++++++++++++++++++--
 4 files changed, 52 insertions(+), 3 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/include/asm/mce.h b/arch/x86/include/asm/mce.h
index a3ac52b..8ad5078 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -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);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index c311122..29e87d3 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -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
 };
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index 098386f..d036032 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_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_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_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) {
+		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));
 	}

Re: [PATCH v2] x86/mce: Honour bios-set CMCI threshold

[Tony Luck]

On Mon, Sep 10, 2012 at 10:31 PM, Naveen N. Rao
<naveen.n.rao@linux.vnet.ibm.com> wrote:
> +       if (mce_bios_cmci_threshold)
> +               printk_once(KERN_INFO
> +                       "bios_cmci_threshold: Using bios-set threshold values for CMCI");

Do we really need this message? The user knows whether they gave
the command line option or not (and can check in /proc/cmdline if
they forgot whether they did). If it is needed, then you should add
a "\n" to it.

> +       if (mce_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");

Also need "\n"

-Tony

[PATCH v3] x86/mce: Honour bios-set CMCI threshold

[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 <naveen.n.rao@linux.vnet.ibm.com>
---
 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(-)

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/include/asm/mce.h b/arch/x86/include/asm/mce.h
index a3ac52b..8ad5078 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -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);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index c311122..29e87d3 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -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
 };
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index 098386f..5f88abf 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
@@ -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));
 	}

Re: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Naveen N. Rao]

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

Thanks,
Naveen

On 09/12/2012 05:55 PM, Naveen N. Rao wrote:
> 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 <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   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(-)
>
> 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/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index a3ac52b..8ad5078 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -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);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index c311122..29e87d3 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -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
>   };
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> index 098386f..5f88abf 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> @@ -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));
>   	}
>

Re: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Borislav Petkov]

On Fri, Sep 21, 2012 at 05:09:04PM +0530, Naveen N. Rao wrote:
> Hi Tony,
> Can you kindly take in this patch if there are no further comments?
> 
> Thanks,
> Naveen
> 
> On 09/12/2012 05:55 PM, Naveen N. Rao wrote:
> >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 <naveen.n.rao@linux.vnet.ibm.com>
> >---
> >  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(-)
> >
> >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/include/asm/mce.h b/arch/x86/include/asm/mce.h
> >index a3ac52b..8ad5078 100644
> >--- a/arch/x86/include/asm/mce.h
> >+++ b/arch/x86/include/asm/mce.h
> >@@ -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);
> >diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> >index c311122..29e87d3 100644
> >--- a/arch/x86/kernel/cpu/mcheck/mce.c
> >+++ b/arch/x86/kernel/cpu/mcheck/mce.c
> >@@ -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

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?

If there's no apparent reason, I'll remove this chunk adding the sysfs
attribute.

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Naveen N. Rao]

On 10/17/2012 04:29 PM, Borislav Petkov wrote:
>>>
>>> +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.

Regards,
Naveen

Re: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Borislav Petkov]

On Wed, Oct 17, 2012 at 04:57:30PM +0530, Naveen N. Rao wrote:
> On 10/17/2012 04:29 PM, Borislav Petkov wrote:
> >>>
> >>>+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.

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?

-- 
Regards/Gruss,
Boris.

Re: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Naveen N. Rao]

On 10/17/2012 06:39 PM, Borislav Petkov wrote:
> On Wed, Oct 17, 2012 at 04:57:30PM +0530, Naveen N. Rao wrote:
>> On 10/17/2012 04:29 PM, Borislav Petkov wrote:
>>>>>
>>>>> +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.
>
> 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?
>

This is not about the CMCI threshold being 0, but rather about the 
threshold not being 1. Linux used to program a threshold of 1 always but 
with this boot option, a firmware-set non-zero threshold is honoured.

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.


Regards,
Naveen

Re: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Borislav Petkov]

On Wed, Oct 17, 2012 at 09:17:39PM +0530, Naveen N. Rao wrote:
> 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?

I still don't see any reason for that read-only sysfs attribute.

Thanks.

-- 
Regards/Gruss,
Boris.

RE: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1060 bytes --]

> 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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Borislav Petkov]

On Wed, Oct 17, 2012 at 05:28:11PM +0000, Luck, Tony wrote:
> > 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.

Right, but the sysfs read-only thing doesn't solve that either - it only
dumps the mce_bios_cmci_threshold value so it doesn't help.

The stuff you're talking about is not reflected in that variable but
saved in some other like bios_wrong_thresh and bios_zero_thresh.

> 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.

Yes. And I still haven't gotten a real use case of what userspace tool
will do with this 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.

Right, a real use case which shows why we need that information should
be good.

Thanks.

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

What happened here? It seems your mail client wanted to say something
too but it has been censored.

:-)

-- 
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

Re: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Naveen N. Rao]

On 10/17/2012 10:58 PM, Luck, Tony wrote:
> 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.

Thanks,
Naveen

Re: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Borislav Petkov]

On Thu, Oct 18, 2012 at 11:13:14AM +0530, Naveen N. Rao wrote:
> On 10/17/2012 10:58 PM, Luck, Tony wrote:
> >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.

Ok, patch below removes it.

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

Thanks.

--
From: Borislav Petkov <borislav.petkov@amd.com>
Date: Thu, 18 Oct 2012 15:10:56 +0200
Subject: [PATCH] x86, MCE: Remove bios_cmci_threshold sysfs attribute

450cc201038f3 ("x86/mce: Provide boot argument to honour bios-set CMCI
threshold") added the bios_cmci_threshold attribute which was supposed
to communicate to userspace tools that BIOS CMCI threshold has been
honoured.

However, this info is not of any importance to userspace - rather it
can get the actual error count it has been thresholded already from
MCi_STATUS[38:52].

So drop this before it becomes a used interface (good thing is we caught
this early in 3.7-rc1, right after the merge window closed).

Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Tony Luck <tony.luck@intel.com>
Link: http://lkml.kernel.org/r/20121017105940.GA14590@x1.osrc.amd.com
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 29e87d3b2843..46cbf8689692 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2209,11 +2209,6 @@ 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,
@@ -2222,7 +2217,6 @@ 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
 };
 
-- 
1.8.0.rc2.4.g42e55a5

-- 
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

RE: [PATCH v3] x86/mce: Honour bios-set CMCI threshold

[Luck, Tony]

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

Acked-by: Tony Luck <tony.luck@intel.com>

[tip:x86/urgent] x86, MCE: Remove bios_cmci_threshold sysfs attribute

[tip-bot for Borislav Petkov]

Commit-ID:  5bc66170dc486556a1e36fd384463536573f4b82
Gitweb:     http://git.kernel.org/tip/5bc66170dc486556a1e36fd384463536573f4b82
Author:     Borislav Petkov <borislav.petkov@amd.com>
AuthorDate: Thu, 18 Oct 2012 15:10:56 +0200
Committer:  Borislav Petkov <borislav.petkov@amd.com>
CommitDate: Fri, 19 Oct 2012 15:22:29 +0200

x86, MCE: Remove bios_cmci_threshold sysfs attribute

450cc201038f3 ("x86/mce: Provide boot argument to honour bios-set CMCI
threshold") added the bios_cmci_threshold sysfs attribute which was
supposed to communicate to userspace tools that BIOS CMCI threshold has
been honoured.

However, this info is not of any importance to userspace - it should
rather get the actual error count it has been thresholded already from
MCi_STATUS[38:52].

So drop this before it becomes a used interface (good thing we caught
this early in 3.7-rc1, right after the merge window closed).

Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Acked-by: Tony Luck <tony.luck@intel.com>
Link: http://lkml.kernel.org/r/20121017105940.GA14590@x1.osrc.amd.com
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 29e87d3..46cbf86 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2209,11 +2209,6 @@ 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,
@@ -2222,7 +2217,6 @@ 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
 };