x86/mce: Pack boolean MCE boot flags into a structure

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.

Changes:
- Use the mce_boot_flags structure.
- Expose bios_cmci_threshold via sysfs.

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

---

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

Changes:
- Use the mce_boot_flags structure.
- Expose bios_cmci_threshold via sysfs.

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

---

...

arch/x86/kernel/cpu/mcheck/mce_intel.c static void cmci_discover(int banks, int boot) · line 154

Quoted 10 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +6 -0

@@ -119,6 +146,12 @@ static void cmci_discover(int banks, int boot)
  	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
  	if (hdr)
  		printk(KERN_CONT "\n");
+	if (boot && mce_boot_flags.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");
+	}

All functional changes aside, why do you want to print this at all? Does it bring anything to the user?

Because if BIOS is systematically b0rked and we keep issuing this every time do do cmci_discover, then we have a lotsa users to explain to what happens.

Why not do a printk_once saying something along the lines of "BIOS hasn't setup thresholds properly, correcting..." and that's it?

Tony?

Quoted 2 lines

Why not do a printk_once saying something along the lines of "BIOS hasn't setup thresholds properly, correcting..." and that's it?

Yes, that's the intent here. I am using printk_once() and if I'm not mistaken, we print the above only once during boot.

I am open to changing the message if the above two lines aren't good.

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

Quoted 2 lines · Naveen N. Rao

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

Note that this doesn't necessarily save anything because it needs more code to access, and accesses are more common than the flag

cmpl $0,foo(%rip) 7 bytes
testl $1,foo(%rip) 10 bytes

Quoted 9 lines · Andi Kleen

Quoted 2 lines · Naveen N. Rao

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

Note that this doesn't necessarily save anything because it needs more code to access, and accesses are more common than the flag

cmpl $0,foo(%rip) 7 bytes
testl $1,foo(%rip) 10 bytes

I got 7 bytes:

12e9: f6 05 00 00 00 00 02 testb $0x2,0x0(%rip) # 12f0 <mce_start_timer.isra.18+0x30>

vs the old:

1654: 83 3d 00 00 00 00 00 cmpl $0x0,0x0(%rip) # 165b <mce_start_timer.clone.15+0x37>

also 7 bytes.

Many MCE boot 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 <[email protected]>

---
 arch/x86/include/asm/mce.h             |   11 +++-
 arch/x86/kernel/cpu/mcheck/mce.c       |   86 +++++++++++++++++++++-----------
 arch/x86/kernel/cpu/mcheck/mce_intel.c |    2 -
 3 files changed, 66 insertions(+), 33 deletions(-)

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

index a3ac52b..78caeb2 100644

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

+
 extern void mce_register_decode_chain(struct notifier_block *nb);
 extern void mce_unregister_decode_chain(struct notifier_block *nb);
 

@@ -169,8 +178,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);

arch/x86/kernel/cpu/mcheck/mce.c +7 -8

index 292d025..5a0d399 100644
@@ -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_boot_flags		mce_boot_flags		__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_boot_flags.dont_log_ce)
 			mce_log(&m);
 
 		/*

@@ -1601,7 +1600,7 @@ static void __mcheck_cpu_init_timer(void)
 
 	setup_timer(t, mce_timer_fn, smp_processor_id());
 
-	if (mce_ignore_ce)
+	if (mce_boot_flags.ignore_ce)
 		return;
 
 	__this_cpu_write(mce_next_interval, iv);

@@ -1919,11 +1918,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_boot_flags.cmci_disabled = 1;
 	else if (!strcmp(str, "dont_log_ce"))
-		mce_dont_log_ce = 1;
+		mce_boot_flags.dont_log_ce = 1;
 	else if (!strcmp(str, "ignore_ce"))
-		mce_ignore_ce = 1;
+		mce_boot_flags.ignore_ce = 1;
 	else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
 		mce_bootlog = (str[0] == 'b');
 	else if (isdigit(str[0])) {

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

@@ -2090,6 +2089,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_boot_flags.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_boot_flags.dont_log_ce = !!new;
+

arch/x86/kernel/cpu/mcheck/mce.c +20 -21

 static ssize_t set_ignore_ce(struct device *s,
 			     struct device_attribute *attr,
 			     const char *buf, size_t size)

@@ -2099,21 +2126,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_boot_flags.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_boot_flags.ignore_ce = 1;
 		} else {
 			/* enable ce features */
-			mce_ignore_ce = 0;
+			mce_boot_flags.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_boot_flags.cmci_disabled);
+}
+
 static ssize_t set_cmci_disabled(struct device *s,
 				 struct device_attribute *attr,
 				 const char *buf, size_t size)

@@ -2123,14 +2157,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_boot_flags.cmci_disabled ^ !!new) {
 		if (new) {
 			/* disable cmci */
 			on_each_cpu(mce_disable_cmci, NULL, 1);
-			mce_cmci_disabled = 1;
+			mce_boot_flags.cmci_disabled = 1;
 		} else {
 			/* enable cmci */
-			mce_cmci_disabled = 0;
+			mce_boot_flags.cmci_disabled = 0;
 			on_each_cpu(mce_enable_ce, NULL, 1);
 		}
 	}

@@ -2149,31 +2183,23 @@ 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
-};
-
 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
 };
 

@@ -2314,7 +2340,7 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
 		break;
 	case CPU_DOWN_FAILED:
 	case CPU_DOWN_FAILED_FROZEN:
-		if (!mce_ignore_ce && check_interval) {
+		if (!mce_boot_flags.ignore_ce && check_interval) {
 			t->expires = round_jiffies(jiffies +
 					per_cpu(mce_next_interval, cpu));
 			add_timer_on(t, cpu);

arch/x86/kernel/cpu/mcheck/mce_intel.c +0 -0

index 38e49bc..aaf5c51 100644

Quoted 4 lines · Naveen N. Rao

Many MCE boot 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 <[email protected]>

Ok,

looks good.

A couple of issues though:

You need to prepare your patches against tip/master (which contains tip/x86/mce) because there are MCE changes in tip and they conflict with your changes.

Quoted 12 lines

---
  arch/x86/include/asm/mce.h             |   11 +++-
  arch/x86/kernel/cpu/mcheck/mce.c       |   86 +++++++++++++++++++++-----------
  arch/x86/kernel/cpu/mcheck/mce_intel.c |    2 -
  3 files changed, 66 insertions(+), 33 deletions(-)

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

index a3ac52b..78caeb2 100644

arch/x86/include/asm/mce.h struct mce_log { · line 137

Quoted 9 lines

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

@@ -129,6 +129,15 @@ struct mce_log {
 
  #ifdef __KERNEL__
 
+struct mce_boot_flags {
+	__u32	cmci_disabled		: 1,
+		ignore_ce		: 1,
+		dont_log_ce		: 1,
+		__pad			: 29;
+};

This looks ok but I think it would be even better if it went into mce-internal.h where all the mce-only stuff can be collected in a private struct so that we don't pollute the global MCE namespace.

Then you can call the struct simply

struct boot_flags

since it is private to mce code.

arch/x86/include/asm/mce.h struct mce_log { · line 139

Quoted 3 lines

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

+
+extern struct mce_boot_flags mce_boot_flags;

Why do we need that extern thing?

Quoted 68 lines

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

+
  extern void mce_register_decode_chain(struct notifier_block *nb);
  extern void mce_unregister_decode_chain(struct notifier_block *nb);
 

@@ -169,8 +178,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);

arch/x86/kernel/cpu/mcheck/mce.c +7 -8

index 292d025..5a0d399 100644
@@ -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_boot_flags		mce_boot_flags		__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_boot_flags.dont_log_ce)
  			mce_log(&m);
 
  		/*

@@ -1601,7 +1600,7 @@ static void __mcheck_cpu_init_timer(void)
 
  	setup_timer(t, mce_timer_fn, smp_processor_id());
 
-	if (mce_ignore_ce)
+	if (mce_boot_flags.ignore_ce)
  		return;
 
  	__this_cpu_write(mce_next_interval, iv);

@@ -1919,11 +1918,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_boot_flags.cmci_disabled = 1;
  	else if (!strcmp(str, "dont_log_ce"))
-		mce_dont_log_ce = 1;
+		mce_boot_flags.dont_log_ce = 1;
  	else if (!strcmp(str, "ignore_ce"))
-		mce_ignore_ce = 1;
+		mce_boot_flags.ignore_ce = 1;
  	else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
  		mce_bootlog = (str[0] == 'b');
  	else if (isdigit(str[0])) {

@@ -2076,7 +2075,7 @@ show_trigger(struct device *s, struct device_attribute *attr, char *buf)
  }
 

arch/x86/kernel/cpu/mcheck/mce.c show_trigger(struct device *s, struct device_attribute *attr, char *buf) · line 2081

Quoted 6 lines

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

  static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
-				const char *buf, size_t siz)
+			   const char *buf, size_t size)
  {
  	char *p;
 

This is an unrelated change, pls drop it.

Quoted 23 lines

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

@@ -2090,6 +2089,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_boot_flags.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_boot_flags.dont_log_ce = !!new;
+

arch/x86/kernel/cpu/mcheck/mce.c static ssize_t set_trigger(struct device *s, struct device_attribute *attr, · line 2119

Quoted 10 lines

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

+	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_boot_flags.ignore_ce);
+}
+

Those could be combined into a function similar to device_show/store_int pairs but working on a bitfield.

Maybe later.

Quoted 106 lines

arch/x86/kernel/cpu/mcheck/mce.c +20 -21

  static ssize_t set_ignore_ce(struct device *s,
  			     struct device_attribute *attr,
  			     const char *buf, size_t size)

@@ -2099,21 +2126,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_boot_flags.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_boot_flags.ignore_ce = 1;
  		} else {
  			/* enable ce features */
-			mce_ignore_ce = 0;
+			mce_boot_flags.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_boot_flags.cmci_disabled);
+}
+
  static ssize_t set_cmci_disabled(struct device *s,
  				 struct device_attribute *attr,
  				 const char *buf, size_t size)

@@ -2123,14 +2157,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_boot_flags.cmci_disabled ^ !!new) {
  		if (new) {
  			/* disable cmci */
  			on_each_cpu(mce_disable_cmci, NULL, 1);
-			mce_cmci_disabled = 1;
+			mce_boot_flags.cmci_disabled = 1;
  		} else {
  			/* enable cmci */
-			mce_cmci_disabled = 0;
+			mce_boot_flags.cmci_disabled = 0;
  			on_each_cpu(mce_enable_ce, NULL, 1);
  		}
  	}

@@ -2149,31 +2183,23 @@ 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
-};
-
  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
  };
 

@@ -2314,7 +2340,7 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
  		break;
  	case CPU_DOWN_FAILED:
  	case CPU_DOWN_FAILED_FROZEN:
-		if (!mce_ignore_ce && check_interval) {
+		if (!mce_boot_flags.ignore_ce && check_interval) {
  			t->expires = round_jiffies(jiffies +
  					per_cpu(mce_next_interval, cpu));
  			add_timer_on(t, cpu);

arch/x86/kernel/cpu/mcheck/mce_intel.c +0 -0

index 38e49bc..aaf5c51 100644

arch/x86/kernel/cpu/mcheck/mce_intel.c static int cmci_supported(int *banks) · line 40

Quoted 6 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -1

@@ -36,7 +36,7 @@ static int cmci_supported(int *banks)
  {
  	u64 cap;
 
-	if (mce_cmci_disabled || mce_ignore_ce)
+	if (mce_boot_flags.cmci_disabled || mce_boot_flags.ignore_ce)
  		return 0;

Thanks.

Quoted 4 lines

arch/x86/include/asm/mce.h struct mce_log { · line 139

Quoted 2 lines

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

+
+extern struct mce_boot_flags mce_boot_flags;

Why do we need that extern thing?

So that this is visible across mce.c and mce_intel.c?

Quoted 6 lines · Naveen N. Rao

Quoted 4 lines

arch/x86/include/asm/mce.h struct mce_log { · line 139

Quoted 2 lines

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

+
+extern struct mce_boot_flags mce_boot_flags;

Why do we need that extern thing?

So that this is visible across mce.c and mce_intel.c?

Ok. But if you move the struct to mce-internal.h and since both .c files include it, we shouldn't need that extern, right?

I think that'll be the most optimal placement for now.

Thanks.

Quoted 6 lines · Borislav Petkov

Quoted 5 lines · Naveen N. Rao

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+extern struct mce_boot_flags mce_boot_flags;

Why do we need that extern thing?

Quoted 4 lines

Quoted 1 line

So that this is visible across mce.c and mce_intel.c?

Ok. But if you move the struct to mce-internal.h and since both .c files include it, we shouldn't need that extern, right?

I'm not sure I understand. I think we still need it. This is not about the structure, but the variable itself. extern allows mce_boot_flags _variable_ accessible from mce_intel.c.

Quoted 7 lines · Naveen N. Rao

Quoted 6 lines · Borislav Petkov

Quoted 5 lines · Naveen N. Rao

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+extern struct mce_boot_flags mce_boot_flags;

Why do we need that extern thing?

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

So that this is visible across mce.c and mce_intel.c?

Ok. But if you move the struct to mce-internal.h and since both .c files include it, we shouldn't need that extern, right?

I'm not sure I understand. I think we still need it. This is not about the structure, but the variable itself. extern allows mce_boot_flags _variable_ accessible from mce_intel.c.

Ok, you're right. I actually was thinking of having athe extern at the place it is used, i.e. mce_intel.c but looking at mce-internal, it has a bunch of other externs so let keep it that way.

But, I went and did change your patch a bit because I think mce_boot_flags is not such a fitting name if we want to move more of those items at the beginning of mce.c to it. Basically, I'd like to put all those MCE vendor-specific config settings into one struct which encapsulates them all together in a clean way instead of having a bunch of single variables all over the place.

IOW, how about the following?

--

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

index a3ac52b29cbf..e5cfd241e508 100644
@@ -126,7 +126,6 @@ struct mce_log {
  #define K8_MCE_THRESHOLD_BANK_5    (MCE_THRESHOLD_BASE + 5 * 9)
  #define K8_MCE_THRESHOLD_DRAM_ECC  (MCE_THRESHOLD_BANK_4 + 0)
 
-
  #ifdef __KERNEL__
 
  extern void mce_register_decode_chain(struct notifier_block *nb);

@@ -169,8 +168,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);

arch/x86/kernel/cpu/mcheck/mce-internal.h +0 -0

index 6a05c1d327a9..3b25bcf452d9 100644
@@ -28,6 +28,15 @@ extern int mce_ser;
 
  extern struct mce_bank *mce_banks;

arch/x86/kernel/cpu/mcheck/mce-internal.h +0 -0

  #ifdef CONFIG_X86_MCE_INTEL
  unsigned long mce_intel_adjust_timer(unsigned long interval);
  void mce_intel_cmci_poll(void);

arch/x86/kernel/cpu/mcheck/mce.c +55 -29

index c311122ea838..db3c5eaa16da 100644
@@ -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_cfg			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) && !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 (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;
+		cfg.cmci_disabled = 1;
  	else if (!strcmp(str, "dont_log_ce"))
-		mce_dont_log_ce = 1;
+		cfg.dont_log_ce = 1;
  	else if (!strcmp(str, "ignore_ce"))
-		mce_ignore_ce = 1;
+		cfg.ignore_ce = 1;
  	else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
  		mce_bootlog = (str[0] == 'b');
  	else if (isdigit(str[0])) {

@@ -2115,7 +2114,7 @@ show_trigger(struct device *s, struct device_attribute *attr, char *buf)
  }
 
  static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
-				const char *buf, size_t siz)
+			   const char *buf, size_t size)
  {
  	char *p;
 

@@ -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", 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;
+
+	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", 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 (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;
+			cfg.ignore_ce = 1;
  		} else {
  			/* enable ce features */
-			mce_ignore_ce = 0;
+			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", 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 (cfg.cmci_disabled ^ !!new) {
  		if (new) {
  			/* disable cmci */
  			on_each_cpu(mce_disable_cmci, NULL, 1);
-			mce_cmci_disabled = 1;
+			cfg.cmci_disabled = 1;
  		} else {
  			/* enable cmci */
-			mce_cmci_disabled = 0;
+			cfg.cmci_disabled = 0;
  			on_each_cpu(mce_enable_ce, NULL, 1);
  		}
  	}

@@ -2188,31 +2222,23 @@ 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
-};
-
  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
  };
 

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -1

index 098386fed48e..f0cf857ba389 100644
@@ -53,7 +53,7 @@ static int cmci_supported(int *banks)
  {
  	u64 cap;
 
-	if (mce_cmci_disabled || mce_ignore_ce)
+	if (cfg.cmci_disabled || cfg.ignore_ce)
  		return 0;
 
  	/*

Looks good. Infact, I had actually added mce_ser and mce_disabled into the bitfield, but backed off not wanting to overdo.

We could pull in all the other configuration parameters into this structure as long as everyone is ok with this.

Quoted 29 lines

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

index a3ac52b29cbf..e5cfd241e508 100644
@@ -126,7 +126,6 @@ struct mce_log {
 #define K8_MCE_THRESHOLD_BANK_5    (MCE_THRESHOLD_BASE + 5 * 9)
 #define K8_MCE_THRESHOLD_DRAM_ECC  (MCE_THRESHOLD_BANK_4 + 0)
 
-
 #ifdef __KERNEL__
 
 extern void mce_register_decode_chain(struct notifier_block *nb);

@@ -169,8 +168,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);

arch/x86/kernel/cpu/mcheck/mce-internal.h +0 -0

index 6a05c1d327a9..3b25bcf452d9 100644
@@ -28,6 +28,15 @@ extern int mce_ser;
 
 extern struct mce_bank *mce_banks;
 

arch/x86/kernel/cpu/mcheck/mce-internal.h extern int mce_ser; · line 39

Quoted 9 lines

arch/x86/kernel/cpu/mcheck/mce-internal.h +9 -0

+struct mce_cfg {
+	__u32	cmci_disabled		: 1,
+		ignore_ce		: 1,
+		dont_log_ce		: 1,
+		__pad			: 29;
+};
+
+extern struct mce_cfg cfg;
+

I'd prefer mce_cfg, rather than just cfg. I think it looks clearer to say, for instance, mce_ser.ignore_ce rather than just cfg.ignore_ce where the latter looks more like a global thing. But, of course, the former is more concise...

Quoted 5 lines · Naveen N. Rao

Looks good. Infact, I had actually added mce_ser and mce_disabled into the bitfield, but backed off not wanting to overdo.

We could pull in all the other configuration parameters into this structure as long as everyone is ok with this.

Well, if you'd like, you can make one change per patch so that they can be easily reviewable.

Quoted 30 lines

Quoted 30 lines

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

index a3ac52b29cbf..e5cfd241e508 100644
@@ -126,7 +126,6 @@ struct mce_log {
  #define K8_MCE_THRESHOLD_BANK_5    (MCE_THRESHOLD_BASE + 5 * 9)
  #define K8_MCE_THRESHOLD_DRAM_ECC  (MCE_THRESHOLD_BANK_4 + 0)
 
-
  #ifdef __KERNEL__
 
  extern void mce_register_decode_chain(struct notifier_block *nb);

@@ -169,8 +168,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);

arch/x86/kernel/cpu/mcheck/mce-internal.h +0 -0

index 6a05c1d327a9..3b25bcf452d9 100644
@@ -28,6 +28,15 @@ extern int mce_ser;
 
  extern struct mce_bank *mce_banks;
 

Quoted 14 lines

arch/x86/kernel/cpu/mcheck/mce-internal.h extern int mce_ser; · line 39

Quoted 9 lines

arch/x86/kernel/cpu/mcheck/mce-internal.h +9 -0

+struct mce_cfg {
+	__u32	cmci_disabled		: 1,
+		ignore_ce		: 1,
+		dont_log_ce		: 1,
+		__pad			: 29;
+};
+
+extern struct mce_cfg cfg;
+

I'd prefer mce_cfg, rather than just cfg. I think it looks clearer to say, for instance, mce_ser.ignore_ce rather than just cfg.ignore_ce where the latter looks more like a global thing. But, of course, the former is more concise...

Yes,

* it is more consise
* it is private to mce so no ambiguity
* having identical struct name and variable names is very confusing (at least to me)

so you can do

extern struct mce_cfg m_cfg;

or

extern struct mce_config mcfg;

or similar but please keep struct name and variable name different.

Thanks.