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.

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          |    4 +++
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |   39 +++++++++++++++++++++++++++--
 4 files changed, 46 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 +0 -0

index 292d025..401359d 100644

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

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          |    4 +++
  arch/x86/kernel/cpu/mcheck/mce_intel.c    |   39 +++++++++++++++++++++++++++--
  4 files changed, 46 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 +0 -0

index 292d025..401359d 100644

arch/x86/kernel/cpu/mcheck/mce.c static int mce_panic_timeout __read_mostly; · line 86

Quoted 5 lines

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

@@ -82,6 +82,7 @@ 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_bios_cmci_threshold	__read_mostly;
  int				mce_ser			__read_mostly;

AFAICT, this is actually a single-bit flag but we're using a whole integer for it and from looking at the other boot options a couple of them are used as flags too.

Care to define a

struct boot_flags {
	__u64 	mce_bios_cmci_threshold : 1,
		__reserved		: 63;
};

and use

boot_flags.mce_bios_cmci_threshold

in the conditionals below instead?

I'll try to convert the rest of them to that struct and thus save some more space...

Thanks.

Sure - that sounds like a good idea. Further, a #define can be used to avoid the need to change other references, though I'm not sure it's considered good practice:

#define mce_bios_cmci_threshold boot_flags.mce_bios_cmci_threshold

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

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          |    4 +++
  arch/x86/kernel/cpu/mcheck/mce_intel.c    |   39 +++++++++++++++++++++++++++--
  4 files changed, 46 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 +0 -0

index 292d025..401359d 100644

arch/x86/kernel/cpu/mcheck/mce.c static int mce_panic_timeout __read_mostly; · line 86

Quoted 5 lines

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

@@ -82,6 +82,7 @@ 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_bios_cmci_threshold	__read_mostly;
  int				mce_ser			__read_mostly;

AFAICT, this is actually a single-bit flag but we're using a whole integer for it and from looking at the other boot options a couple of them are used as flags too.

Care to define a

struct boot_flags {
	__u64 	mce_bios_cmci_threshold : 1,
		__reserved		: 63;
};

and use

boot_flags.mce_bios_cmci_threshold

in the conditionals below instead?

I'll try to convert the rest of them to that struct and thus save some more space...

Thanks.

Sure - sounds like a good idea. Further, a #define could eliminate the need to change other references, but I'm not sure that's GENERALLacceptable

#define mce_bios_cmci_threshold boot_flags.mce_bios_cmci_threshold

could eliminate the need to change other references, but I'm not sure that's acceptable

But, I just had a quick look and it seems to me that these were defined as integers since they are exposed via sysfs. For instance:

static struct dev_ext_attribute dev_attr_cmci_disabled = {
        __ATTR(cmci_disabled, 0644, device_show_int, set_cmci_disabled),
        &mce_cmci_disabled
};

Converting mce_cmci_disabled to a bit-field doesn't work since we take its address above. We could ignore and not set the second field at all (dev_ext_attribute->var) and define our own callbacks, but that'll be more work and I'm not sure if we work fine without

Quoted 4 lines · Naveen N. Rao

Sure - sounds like a good idea. Further, a #define could eliminate the need to change other references, but I'm not sure that's GENERALLacceptable

Quoted 4 lines

#define mce_bios_cmci_threshold boot_flags.mce_bios_cmci_threshold

could eliminate the need to change other references, but I'm not sure that's acceptable

Yeah, that's kinda obfuscating it for no reason. As I said before, we can always add it later if it makes sense.

Quoted 9 lines

But, I just had a quick look and it seems to me that these were defined as integers since they are exposed via sysfs. For instance:

static struct dev_ext_attribute dev_attr_cmci_disabled = {
         __ATTR(cmci_disabled, 0644, device_show_int, set_cmci_disabled),
         &mce_cmci_disabled
};

Quoted 4 lines

Converting mce_cmci_disabled to a bit-field doesn't work since we take its address above. We could ignore and not set the second field at all (dev_ext_attribute->var) and define our own callbacks, but that'll be more work and I'm not sure if we work fine without

Right,

but take a look at set_cmci_disabled(): it converts the newly read value to bool anyway:

         if (mce_cmci_disabled ^ !!new) {

so we can do later

	flags.mce_cmci_disabled = true;

or

	flags.mce_cmci_disabled = false;

instead of assigning 0 or 1 to it.

And, about showing it with device_show_int, a simple test works:

---
#include <stdio.h>
#include <stdbool.h>

int main()

{
         bool a = true;
         printf("%d\n", a);
         return 0;
}

--

but even if there are troubles with that, we can change device_show_int to a locally defined function.

But, anyway, long story short: I wasn't suggesting you go and change all of them - simply start by adding your flag mce_bios_cmci_threshold to a struct <something>_flags and I'll take care of the rest.

Unless you really want to do it, of course :-)

Oh, and the more important thing is, Tony would need to review your Intel-specific changes so pls keep him CCed on your next iteration too.

Thanks.