[PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure

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

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index a3ac52b..78caeb2 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -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;
+};
+
+extern struct mce_boot_flags mce_boot_flags;
+
 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);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 292d025..5a0d399 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -79,11 +79,10 @@ static int			rip_msr			__read_mostly;
 static int			mce_bootlog		__read_mostly = -1;
 static int			monarch_timeout		__read_mostly = -1;
 static int			mce_panic_timeout	__read_mostly;
-static int			mce_dont_log_ce		__read_mostly;
-int				mce_cmci_disabled	__read_mostly;
-int				mce_ignore_ce		__read_mostly;
 int				mce_ser			__read_mostly;
 
+struct mce_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)
 }
 
 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;
 
@@ -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;
+
+	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);
+}
+
 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);
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index 38e49bc..aaf5c51 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
@@ -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;
 
 	/*

[PATCH 2/2] 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.

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

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                |    3 +-
 arch/x86/kernel/cpu/mcheck/mce.c          |   12 +++++++++
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |   39 +++++++++++++++++++++++++++--
 4 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index c54b4f5..ec92540 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -50,6 +50,11 @@ Machine check
 		monarchtimeout:
 		Sets the time in us to wait for other CPUs on machine checks. 0
 		to disable.
+   mce=bios_cmci_threshold
+		Don't overwrite the bios-set CMCI threshold. This boot option
+		prevents Linux from overwriting the CMCI threshold set by the
+		bios. Without this option, Linux always sets the CMCI
+		threshold to 1.
 
    nomce (for compatibility with i386): same as mce=off
 
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 78caeb2..7c8ad16 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -133,7 +133,8 @@ struct mce_boot_flags {
 	__u32	cmci_disabled		: 1,
 		ignore_ce		: 1,
 		dont_log_ce		: 1,
-		__pad			: 29;
+		bios_cmci_threshold	: 1,
+		__pad			: 28;
 };
 
 extern struct mce_boot_flags mce_boot_flags;
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 5a0d399..1d97e55 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1906,6 +1906,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)
 {
@@ -1925,6 +1926,8 @@ static int __init mcheck_enable(char *str)
 		mce_boot_flags.ignore_ce = 1;
 	else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
 		mce_bootlog = (str[0] == 'b');
+	else if (!strcmp(str, "bios_cmci_threshold"))
+		mce_boot_flags.bios_cmci_threshold = 1;
 	else if (isdigit(str[0])) {
 		get_option(&str, &tolerant);
 		if (*str == ',') {
@@ -2171,6 +2174,13 @@ static ssize_t set_cmci_disabled(struct device *s,
 	return size;
 }
 
+static ssize_t get_bios_cmci_threshold(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", mce_boot_flags.bios_cmci_threshold);
+}
+
 static ssize_t store_int_with_restart(struct device *s,
 				      struct device_attribute *attr,
 				      const char *buf, size_t size)
@@ -2186,6 +2196,7 @@ static DEVICE_INT_ATTR(monarch_timeout, 0644, monarch_timeout);
 static DEVICE_ATTR(dont_log_ce, 0644, get_dont_log_ce, set_dont_log_ce);
 static DEVICE_ATTR(ignore_ce, 0644, get_ignore_ce, set_ignore_ce);
 static DEVICE_ATTR(cmci_disabled, 0644, get_cmci_disabled, set_cmci_disabled);
+static DEVICE_ATTR(bios_cmci_threshold, 0444, get_bios_cmci_threshold, NULL);
 
 static struct dev_ext_attribute dev_attr_check_interval = {
 	__ATTR(check_interval, 0644, device_show_int, store_int_with_restart),
@@ -2200,6 +2211,7 @@ static struct device_attribute *mce_device_attrs[] = {
 	&dev_attr_dont_log_ce,
 	&dev_attr_ignore_ce,
 	&dev_attr_cmci_disabled,
+	&dev_attr_bios_cmci_threshold,
 	NULL
 };
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index aaf5c51..e7222e3 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
@@ -84,10 +84,16 @@ static void cmci_discover(int banks, int boot)
 	unsigned long flags;
 	int hdr = 0;
 	int i;
+	int bios_wrong_thresh = 0;
+
+	if (boot && mce_boot_flags.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;
@@ -102,8 +108,20 @@ static void cmci_discover(int banks, int boot)
 			continue;
 		}
 
-		val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
-		val |= MCI_CTL2_CMCI_EN | CMCI_THRESHOLD;
+		if (!mce_boot_flags.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);
 
@@ -112,6 +130,15 @@ static void cmci_discover(int banks, int boot)
 			if (!test_and_set_bit(i, owned) && !boot)
 				print_update("CMCI", &hdr, i);
 			__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_boot_flags.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)));
 		}
@@ -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");
+	}
 }
 
 /*
@@ -156,7 +189,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 2/2] x86/mce: Honour bios-set CMCI threshold

[Borislav Petkov]

On Mon, Aug 27, 2012 at 04:55:12PM +0530, 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.
> 
> Changes:
> - Use the mce_boot_flags structure.
> - Expose bios_cmci_threshold via sysfs.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---

...

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

-- 
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 2/2] x86/mce: Honour bios-set CMCI threshold

[Naveen N. Rao]

On 08/27/2012 08:18 PM, Borislav Petkov wrote:
> On Mon, Aug 27, 2012 at 04:55:12PM +0530, 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.
>>
>> Changes:
>> - Use the mce_boot_flags structure.
>> - Expose bios_cmci_threshold via sysfs.
>>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>
> ...
>
>> @@ -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?

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.


Thanks!
- Naveen

>
> Tony?
>

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

[Borislav Petkov]

On Mon, Aug 27, 2012 at 08:41:34PM +0530, Naveen N. Rao wrote:
> >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.

Oh yes, you are. I need to clean my glasses. 8-) Sorry.

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

Let's see what Tony has to say.

Thanks.

-- 
Regards/Gruss,
Boris.

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

Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Andi Kleen]

On Mon, Aug 27, 2012 at 04:55:03PM +0530, Naveen N. Rao wrote:
> 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
        bt    $0,foo(%rip)            8 bytes

Even worse for writes. The best you can usually do is to use chars on
x86.

-Andi

Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Borislav Petkov]

On Mon, Aug 27, 2012 at 03:58:59PM +0200, Andi Kleen wrote:
> On Mon, Aug 27, 2012 at 04:55:03PM +0530, Naveen N. Rao wrote:
> > 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.

-- 
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 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Naveen N. Rao]

On 08/27/2012 07:48 PM, Borislav Petkov wrote:
> On Mon, Aug 27, 2012 at 03:58:59PM +0200, Andi Kleen wrote:
>> On Mon, Aug 27, 2012 at 04:55:03PM +0530, Naveen N. Rao wrote:
>>> 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.
>

I'm seeing the same here. GCC v4.7 with -O2 generates testb and cmpb, 
which are both 7 bytes long.

Thanks,
Naveen

Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Borislav Petkov]

On Mon, Aug 27, 2012 at 04:55:03PM +0530, Naveen N. Rao wrote:
> 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 <naveen.n.rao@linux.vnet.ibm.com>

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.

> ---
>  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(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index a3ac52b..78caeb2 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -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.

> +
> +extern struct mce_boot_flags mce_boot_flags;

Why do we need that extern thing?

> +
>  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);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 292d025..5a0d399 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -79,11 +79,10 @@ static int			rip_msr			__read_mostly;
>  static int			mce_bootlog		__read_mostly = -1;
>  static int			monarch_timeout		__read_mostly = -1;
>  static int			mce_panic_timeout	__read_mostly;
> -static int			mce_dont_log_ce		__read_mostly;
> -int				mce_cmci_disabled	__read_mostly;
> -int				mce_ignore_ce		__read_mostly;
>  int				mce_ser			__read_mostly;
>  
> +struct mce_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)
>  }
>  
>  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.

> @@ -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;
> +
> +	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.

>  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);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> index 38e49bc..aaf5c51 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> @@ -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.

-- 
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 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Naveen N. Rao]

On 08/27/2012 08:06 PM, Borislav Petkov wrote:
> On Mon, Aug 27, 2012 at 04:55:03PM +0530, Naveen N. Rao wrote:
>> 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 <naveen.n.rao@linux.vnet.ibm.com>
>
> 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.

Sure, will do so for my next iteration.

>
>> ---
>>   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(-)
>>
>> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
>> index a3ac52b..78caeb2 100644
>> --- a/arch/x86/include/asm/mce.h
>> +++ b/arch/x86/include/asm/mce.h
>> @@ -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.

Good idea - will do.

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

>
>> +
>>   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);
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>> index 292d025..5a0d399 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> @@ -79,11 +79,10 @@ static int			rip_msr			__read_mostly;
>>   static int			mce_bootlog		__read_mostly = -1;
>>   static int			monarch_timeout		__read_mostly = -1;
>>   static int			mce_panic_timeout	__read_mostly;
>> -static int			mce_dont_log_ce		__read_mostly;
>> -int				mce_cmci_disabled	__read_mostly;
>> -int				mce_ignore_ce		__read_mostly;
>>   int				mce_ser			__read_mostly;
>>
>> +struct mce_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)
>>   }
>>
>>   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.

Ok.

>
>> @@ -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;
>> +
>> +	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.

Yes, this looks like it needs some work and I wasn't sure it would be 
helpful at this time. So, yeah, maybe later.


Thanks!
- Naveen

>
>>   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);
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
>> index 38e49bc..aaf5c51 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
>> @@ -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.
>

Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Borislav Petkov]

On Mon, Aug 27, 2012 at 09:05:46PM +0530, Naveen N. Rao wrote:
> >>+
> >>+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.

-- 
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 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Naveen N. Rao]

On 08/27/2012 09:17 PM, Borislav Petkov wrote:
> On Mon, Aug 27, 2012 at 09:05:46PM +0530, Naveen N. Rao wrote:
>>>> +
>>>> +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'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.


Thanks,
Naveen

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

Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Borislav Petkov]

On Mon, Aug 27, 2012 at 09:31:11PM +0530, Naveen N. Rao wrote:
> On 08/27/2012 09:17 PM, Borislav Petkov wrote:
> >On Mon, Aug 27, 2012 at 09:05:46PM +0530, Naveen N. Rao wrote:
> >>>>+
> >>>>+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'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?

--
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index a3ac52b29cbf..e5cfd241e508 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -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);
diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
index 6a05c1d327a9..3b25bcf452d9 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
@@ -28,6 +28,15 @@ extern int mce_ser;
 
 extern struct mce_bank *mce_banks;
 
+struct mce_cfg {
+	__u32	cmci_disabled		: 1,
+		ignore_ce		: 1,
+		dont_log_ce		: 1,
+		__pad			: 29;
+};
+
+extern struct mce_cfg cfg;
+
 #ifdef CONFIG_X86_MCE_INTEL
 unsigned long mce_intel_adjust_timer(unsigned long interval);
 void mce_intel_cmci_poll(void);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index c311122ea838..db3c5eaa16da 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -79,11 +79,10 @@ static int			rip_msr			__read_mostly;
 static int			mce_bootlog		__read_mostly = -1;
 static int			monarch_timeout		__read_mostly = -1;
 static int			mce_panic_timeout	__read_mostly;
-static int			mce_dont_log_ce		__read_mostly;
-int				mce_cmci_disabled	__read_mostly;
-int				mce_ignore_ce		__read_mostly;
 int				mce_ser			__read_mostly;
 
+struct mce_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
 };
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index 098386fed48e..f0cf857ba389 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
@@ -53,7 +53,7 @@ static int cmci_supported(int *banks)
 {
 	u64 cap;
 
-	if (mce_cmci_disabled || mce_ignore_ce)
+	if (cfg.cmci_disabled || cfg.ignore_ce)
 		return 0;
 
 	/*

-- 
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 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Naveen N. Rao]

On 08/27/2012 10:04 PM, Borislav Petkov wrote:
> On Mon, Aug 27, 2012 at 09:31:11PM +0530, Naveen N. Rao wrote:
>> On 08/27/2012 09:17 PM, Borislav Petkov wrote:
>>> On Mon, Aug 27, 2012 at 09:05:46PM +0530, Naveen N. Rao wrote:
>>>>>> +
>>>>>> +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'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?

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.

>
> --
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index a3ac52b29cbf..e5cfd241e508 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -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);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
> index 6a05c1d327a9..3b25bcf452d9 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
> +++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
> @@ -28,6 +28,15 @@ extern int mce_ser;
>
>   extern struct mce_bank *mce_banks;
>
> +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...

Thanks,
Naveen

>   #ifdef CONFIG_X86_MCE_INTEL
>   unsigned long mce_intel_adjust_timer(unsigned long interval);
>   void mce_intel_cmci_poll(void);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index c311122ea838..db3c5eaa16da 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -79,11 +79,10 @@ static int			rip_msr			__read_mostly;
>   static int			mce_bootlog		__read_mostly = -1;
>   static int			monarch_timeout		__read_mostly = -1;
>   static int			mce_panic_timeout	__read_mostly;
> -static int			mce_dont_log_ce		__read_mostly;
> -int				mce_cmci_disabled	__read_mostly;
> -int				mce_ignore_ce		__read_mostly;
>   int				mce_ser			__read_mostly;
>
> +struct mce_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
>   };
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> index 098386fed48e..f0cf857ba389 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> @@ -53,7 +53,7 @@ static int cmci_supported(int *banks)
>   {
>   	u64 cap;
>
> -	if (mce_cmci_disabled || mce_ignore_ce)
> +	if (cfg.cmci_disabled || cfg.ignore_ce)
>   		return 0;
>
>   	/*
>

Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Borislav Petkov]

On Mon, Aug 27, 2012 at 10:44:40PM +0530, Naveen N. Rao wrote:
> 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.

> >diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> >index a3ac52b29cbf..e5cfd241e508 100644
> >--- a/arch/x86/include/asm/mce.h
> >+++ b/arch/x86/include/asm/mce.h
> >@@ -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);
> >diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
> >index 6a05c1d327a9..3b25bcf452d9 100644
> >--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
> >+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
> >@@ -28,6 +28,15 @@ extern int mce_ser;
> >
> >  extern struct mce_bank *mce_banks;
> >
> >+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.

-- 
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 1/2] x86/mce: Pack boolean MCE boot flags into a structure

[Naveen N. Rao]

On 08/28/2012 01:48 AM, Borislav Petkov wrote:
> On Mon, Aug 27, 2012 at 10:44:40PM +0530, Naveen N. Rao wrote:
>> 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.

Ok.

>
>>> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
>>> index a3ac52b29cbf..e5cfd241e508 100644
>>> --- a/arch/x86/include/asm/mce.h
>>> +++ b/arch/x86/include/asm/mce.h
>>> @@ -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);
>>> diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
>>> index 6a05c1d327a9..3b25bcf452d9 100644
>>> --- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
>>> +++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
>>> @@ -28,6 +28,15 @@ extern int mce_ser;
>>>
>>>   extern struct mce_bank *mce_banks;
>>>
>>> +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.

Sure - I thought this was commonly done, but it is indeed confusing. 
I'll change it.


Thanks,
Naveen

>
> Thanks.
>