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

From: Naveen N. Rao
Date: Mon Aug 27 2012 - 11:37:21 EST

Next message: HACHIMI Samir: "[PATCH v2 1/2] imx6q: pwm: Add device tree support"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH 1/3] perf tools: Fix a compiling error intrace-event-perl.c for 32 bits machine"
In reply to: Borislav Petkov: "Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure"
Next in thread: Borislav Petkov: "Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: HACHIMI Samir: "[PATCH v2 1/2] imx6q: pwm: Add device tree support"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH 1/3] perf tools: Fix a compiling error intrace-event-perl.c for 32 bits machine"
In reply to: Borislav Petkov: "Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure"
Next in thread: Borislav Petkov: "Re: [PATCH 1/2] x86/mce: Pack boolean MCE boot flags into a structure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]