* [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds @ 2012-09-05 10:21 Naveen N. Rao 2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Naveen N. Rao @ 2012-09-05 10:21 UTC (permalink / raw) To: tony.luck, andi, bp Cc: gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh, linux-edac Hello, I'm putting together my previous set of patches for MCE. The first two patches are cleanups, as per previous discussions. The first patch makes mce sysfs tunables available under global machinecheck sysfs directory. We no longer remove the existing entries in the per-cpu location. The second patch consolidates some of the mce boot options into a single bitfield. The structure name has been changed and is now in mce-internal.h The third patch adds a new boot option to honour bios-set CMCI thresholds. This has been rebased on the above patches. This patch series applies on top of -tip. Thanks, Naveen --- Naveen N. Rao (3): x86/mce: Make sysfs tunables available globally across all cpus x86/mce: Pack boolean MCE flags into a structure x86/mce: Honour bios-set CMCI threshold Documentation/x86/x86_64/boot-options.txt | 5 + Documentation/x86/x86_64/machinecheck | 4 + arch/x86/include/asm/mce.h | 2 - arch/x86/kernel/cpu/mcheck/mce-internal.h | 10 +++ arch/x86/kernel/cpu/mcheck/mce.c | 112 ++++++++++++++++++++++------- arch/x86/kernel/cpu/mcheck/mce_intel.c | 41 ++++++++++- 6 files changed, 140 insertions(+), 34 deletions(-) -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus 2012-09-05 10:21 [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds Naveen N. Rao @ 2012-09-05 10:21 ` Naveen N. Rao 2012-09-05 10:32 ` [PATCH] [mcelog] Start using the new sysfs tunables location Naveen N. Rao 2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao 2012-09-05 10:22 ` [PATCH 3/3] x86/mce: Honour bios-set CMCI threshold Naveen N. Rao 2 siblings, 1 reply; 16+ messages in thread From: Naveen N. Rao @ 2012-09-05 10:21 UTC (permalink / raw) To: tony.luck, andi, bp Cc: gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh, linux-edac All the MCE attributes currently exported via sysfs appear under /sys/devices/system/machinecheck/machinecheck<n>/. Pretty much all of these are global in nature and not specific to a processor. So, make these available under /sys/devices/system/machinecheck/ where they rightly belong. Update documentation to also point to the new location so that user-space tools can pick up on the new location. We would eventually want to remove these from the per-cpu location. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- Documentation/x86/x86_64/machinecheck | 4 ++-- arch/x86/kernel/cpu/mcheck/mce.c | 24 +++++++++++++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/Documentation/x86/x86_64/machinecheck b/Documentation/x86/x86_64/machinecheck index b1fb302..02b84a6 100644 --- a/Documentation/x86/x86_64/machinecheck +++ b/Documentation/x86/x86_64/machinecheck @@ -31,8 +31,8 @@ bankNctl Note that BIOS maintain another mask to disable specific events per bank. This is not visible here -The following entries appear for each CPU, but they are truly shared -between all CPUs. +The following entries are shared between all CPUs and appear under +/sys/devices/system/machinecheck: check_interval How often to poll for corrected machine check errors, in seconds diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c index c311122..bf276eb 100644 --- a/arch/x86/kernel/cpu/mcheck/mce.c +++ b/arch/x86/kernel/cpu/mcheck/mce.c @@ -2205,6 +2205,7 @@ static struct dev_ext_attribute dev_attr_cmci_disabled = { &mce_cmci_disabled }; +/* Use this _only_ for per-cpu attributes */ static struct device_attribute *mce_device_attrs[] = { &dev_attr_tolerant.attr, &dev_attr_check_interval.attr, @@ -2216,6 +2217,27 @@ static struct device_attribute *mce_device_attrs[] = { NULL }; +/* All new global attributes go here */ +static struct attribute *mce_device_global_attrs[] = { + &dev_attr_tolerant.attr.attr, + &dev_attr_check_interval.attr.attr, + &dev_attr_trigger.attr, + &dev_attr_monarch_timeout.attr.attr, + &dev_attr_dont_log_ce.attr.attr, + &dev_attr_ignore_ce.attr.attr, + &dev_attr_cmci_disabled.attr.attr, + NULL +}; + +static struct attribute_group mce_device_attr_group = { + .attrs = mce_device_global_attrs, +}; + +static const struct attribute_group *mce_device_attr_groups[] = { + &mce_device_attr_group, + NULL, +}; + static cpumask_var_t mce_device_initialized; static void mce_device_release(struct device *dev) @@ -2397,7 +2419,7 @@ static __init int mcheck_init_device(void) mce_init_banks(); - err = subsys_system_register(&mce_subsys, NULL); + err = subsys_system_register(&mce_subsys, mce_device_attr_groups); if (err) return err; ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] [mcelog] Start using the new sysfs tunables location 2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao @ 2012-09-05 10:32 ` Naveen N. Rao 2012-09-05 18:47 ` Andi Kleen 0 siblings, 1 reply; 16+ messages in thread From: Naveen N. Rao @ 2012-09-05 10:32 UTC (permalink / raw) To: ak, tony.luck, andi, bp; +Cc: x86, linux-kernel, linux-edac, ananth All the current mce tunables are now available under /sys/devices/system/machinecheck. Start using this new location, but fall back to the older per-cpu location so that we continue working with older kernels. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- README | 2 +- mcelog.init | 5 ++++- tests/test | 7 ++++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/README b/README index 08184ed..0426460 100644 --- a/README +++ b/README @@ -18,7 +18,7 @@ significantly (upto 10 minutes) and does not allow mcelog to keep extended state trigger is a newer method where the kernel runs mcelog on a error. This is configured with -echo /usr/sbin/mcelog > /sys/devices/system/machinecheck/machinecheck0/trigger +echo /usr/sbin/mcelog > /sys/devices/system/machinecheck/trigger This is faster, but still doesn't allow mcelog to keep state, and has relatively high overhead for each error because a program has to be initialized from scratch. diff --git a/mcelog.init b/mcelog.init index 0abe786..5f32ba7 100755 --- a/mcelog.init +++ b/mcelog.init @@ -31,7 +31,10 @@ MCELOG_OPTIONS="" # private settings MCELOG=${MCELOG:-/usr/sbin/mcelog} -TRIGGER=/sys/devices/system/machinecheck/machinecheck0/trigger +TRIGGER=/sys/devices/system/machinecheck/trigger +if [ ! -e $TRIGGER ] ; then + TRIGGER=/sys/devices/system/machinecheck/machinecheck0/trigger +fi [ ! -x $MCELOG ] && ( echo "mcelog not found" ; exit 1 ) [ ! -r /dev/mcelog ] && ( echo "/dev/mcelog not active" ; exit 0 ) diff --git a/tests/test b/tests/test index c673eb2..52daf01 100755 --- a/tests/test +++ b/tests/test @@ -17,10 +17,15 @@ if [ "$(whoami)" != "root" ] ; then exit 1 fi +TRIGGER=/sys/devices/system/machinecheck/trigger +if [ ! -e $TRIGGER ] ; then + TRIGGER=/sys/devices/system/machinecheck/machinecheck0/trigger +fi + echo "++++++++++++ running $1 test +++++++++++++++++++" # disable trigger -echo -n "" > /sys/devices/system/machinecheck/machinecheck0/trigger +echo -n "" > $TRIGGER killall mcelog || true #killwatchdog() { ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] [mcelog] Start using the new sysfs tunables location 2012-09-05 10:32 ` [PATCH] [mcelog] Start using the new sysfs tunables location Naveen N. Rao @ 2012-09-05 18:47 ` Andi Kleen 2012-09-05 19:09 ` Tony Luck 0 siblings, 1 reply; 16+ messages in thread From: Andi Kleen @ 2012-09-05 18:47 UTC (permalink / raw) To: Naveen N. Rao Cc: ak, tony.luck, andi, bp, x86, linux-kernel, linux-edac, ananth On Wed, Sep 05, 2012 at 04:02:37PM +0530, Naveen N. Rao wrote: > All the current mce tunables are now available under > /sys/devices/system/machinecheck. Start using this new location, but fall back > to the older per-cpu location so that we continue working with older kernels. Who did that change in the kernel? That breaks Linus rule that the kernel should not break userland. Kernel needs to fix that. -Andi ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [mcelog] Start using the new sysfs tunables location 2012-09-05 18:47 ` Andi Kleen @ 2012-09-05 19:09 ` Tony Luck 2012-09-06 6:40 ` Naveen N. Rao 2012-09-06 12:28 ` Andi Kleen 0 siblings, 2 replies; 16+ messages in thread From: Tony Luck @ 2012-09-05 19:09 UTC (permalink / raw) To: Andi Kleen; +Cc: Naveen N. Rao, ak, bp, x86, linux-kernel, linux-edac, ananth On Wed, Sep 5, 2012 at 11:47 AM, Andi Kleen <andi@firstfloor.org> wrote: > On Wed, Sep 05, 2012 at 04:02:37PM +0530, Naveen N. Rao wrote: >> All the current mce tunables are now available under >> /sys/devices/system/machinecheck. Start using this new location, but fall back >> to the older per-cpu location so that we continue working with older kernels. > > Who did that change in the kernel? > > That breaks Linus rule that the kernel should not break userland. > Kernel needs to fix that. The change is still under discussion. Stage one is to add the new global pathnames in addition to keeping the old per-cpu ones. Also fix all utilities (just mcelog(8) as far as we know) to prefer the new paths. After some time[1] ... delete the old paths. This is allowable under Linus' modified edict that you can change ABI "if nobody complains". If we wait long enough that the new mcelog is widely deployed, then nobody should complain. -Tony [1] several years - not just a kernel release or two. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [mcelog] Start using the new sysfs tunables location 2012-09-05 19:09 ` Tony Luck @ 2012-09-06 6:40 ` Naveen N. Rao 2012-09-06 12:28 ` Andi Kleen 1 sibling, 0 replies; 16+ messages in thread From: Naveen N. Rao @ 2012-09-06 6:40 UTC (permalink / raw) To: Tony Luck; +Cc: Andi Kleen, ak, bp, x86, linux-kernel, linux-edac, ananth On 09/06/2012 12:39 AM, Tony Luck wrote: > On Wed, Sep 5, 2012 at 11:47 AM, Andi Kleen <andi@firstfloor.org> wrote: >> On Wed, Sep 05, 2012 at 04:02:37PM +0530, Naveen N. Rao wrote: >>> All the current mce tunables are now available under >>> /sys/devices/system/machinecheck. Start using this new location, but fall back >>> to the older per-cpu location so that we continue working with older kernels. >> >> Who did that change in the kernel? >> >> That breaks Linus rule that the kernel should not break userland. >> Kernel needs to fix that. > > The change is still under discussion. Stage one is to add the new global > pathnames in addition to keeping the old per-cpu ones. Also fix all utilities > (just mcelog(8) as far as we know) to prefer the new paths. > > After some time[1] ... delete the old paths. This is allowable under Linus' > modified edict that you can change ABI "if nobody complains". If we wait > long enough that the new mcelog is widely deployed, then nobody should > complain. > > -Tony > > [1] several years - not just a kernel release or two. > Tony, Thanks for clarifying. I should have mentioned in the patch description that this is indeed subject to the original patch making it into the kernel. On a related topic. I recently noticed that we don't have an entry for machinecheck in Documentation/ABI/. Should we add an entry in there? We could perhaps add the existing entries under obsolete/ and the new location under testing/? - Naveen ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [mcelog] Start using the new sysfs tunables location 2012-09-05 19:09 ` Tony Luck 2012-09-06 6:40 ` Naveen N. Rao @ 2012-09-06 12:28 ` Andi Kleen 2012-09-06 12:34 ` Naveen N. Rao 1 sibling, 1 reply; 16+ messages in thread From: Andi Kleen @ 2012-09-06 12:28 UTC (permalink / raw) To: Tony Luck Cc: Andi Kleen, Naveen N. Rao, bp, x86, linux-kernel, linux-edac, ananth > The change is still under discussion. Stage one is to add the new global > pathnames in addition to keeping the old per-cpu ones. Also fix all utilities > (just mcelog(8) as far as we know) to prefer the new paths. But why do you even want to change it? Does it fix anything? AFAIK the old setup -- while not being pretty -- works just fine. -Andi ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [mcelog] Start using the new sysfs tunables location 2012-09-06 12:28 ` Andi Kleen @ 2012-09-06 12:34 ` Naveen N. Rao 2012-09-06 12:51 ` Alan Cox 2012-09-06 13:21 ` Andi Kleen 0 siblings, 2 replies; 16+ messages in thread From: Naveen N. Rao @ 2012-09-06 12:34 UTC (permalink / raw) To: Andi Kleen Cc: Tony Luck, Andi Kleen, bp, x86, linux-kernel, linux-edac, ananth On 09/06/2012 05:58 PM, Andi Kleen wrote: >> The change is still under discussion. Stage one is to add the new global >> pathnames in addition to keeping the old per-cpu ones. Also fix all utilities >> (just mcelog(8) as far as we know) to prefer the new paths. > > But why do you even want to change it? Does it fix anything? > AFAIK the old setup -- while not being pretty -- works just fine. The reason for this was explained in this thread: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg298302.html Even if we decide not to remove these tunables from under their current per-cpu location, I still think it is much cleaner to have them available under /sys/devices/system/machinecheck. - Naveen ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [mcelog] Start using the new sysfs tunables location 2012-09-06 12:34 ` Naveen N. Rao @ 2012-09-06 12:51 ` Alan Cox 2012-09-06 13:21 ` Andi Kleen 1 sibling, 0 replies; 16+ messages in thread From: Alan Cox @ 2012-09-06 12:51 UTC (permalink / raw) To: Naveen N. Rao Cc: Andi Kleen, Tony Luck, Andi Kleen, bp, x86, linux-kernel, linux-edac, ananth On Thu, 06 Sep 2012 18:04:27 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > On 09/06/2012 05:58 PM, Andi Kleen wrote: > >> The change is still under discussion. Stage one is to add the new global > >> pathnames in addition to keeping the old per-cpu ones. Also fix all utilities > >> (just mcelog(8) as far as we know) to prefer the new paths. > > > > But why do you even want to change it? Does it fix anything? > > AFAIK the old setup -- while not being pretty -- works just fine. > > The reason for this was explained in this thread: > http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg298302.html > > Even if we decide not to remove these tunables from under their current > per-cpu location, I still think it is much cleaner to have them > available under /sys/devices/system/machinecheck. That to me seems a ridiculous proposal. What are you going to do if in future they ceased to be system wide ? Move them back ? The threshold for playing musical chairs with sysfs nodes is a lot higher than "I think it's much cleaner" The current approach is a lot more futureproof even if a spot more ugly. Alan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [mcelog] Start using the new sysfs tunables location 2012-09-06 12:34 ` Naveen N. Rao 2012-09-06 12:51 ` Alan Cox @ 2012-09-06 13:21 ` Andi Kleen 1 sibling, 0 replies; 16+ messages in thread From: Andi Kleen @ 2012-09-06 13:21 UTC (permalink / raw) To: Naveen N. Rao Cc: Andi Kleen, Tony Luck, Andi Kleen, bp, x86, linux-kernel, linux-edac, ananth > Even if we decide not to remove these tunables from under their current > per-cpu location, I still think it is much cleaner to have them > available under /sys/devices/system/machinecheck. "much cleaner" is not sufficient justification to break an ABI. -Andi -- ak@linux.intel.com -- Speaking for myself only. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure 2012-09-05 10:21 [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds Naveen N. Rao 2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao @ 2012-09-05 10:22 ` Naveen N. Rao 2012-09-05 17:15 ` Joe Perches 2012-09-05 18:56 ` Tony Luck 2012-09-05 10:22 ` [PATCH 3/3] x86/mce: Honour bios-set CMCI threshold Naveen N. Rao 2 siblings, 2 replies; 16+ messages in thread From: Naveen N. Rao @ 2012-09-05 10:22 UTC (permalink / raw) To: tony.luck, andi, bp Cc: gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh, linux-edac Many MCE 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 | 2 - arch/x86/kernel/cpu/mcheck/mce-internal.h | 9 +++ arch/x86/kernel/cpu/mcheck/mce.c | 88 +++++++++++++++++++---------- arch/x86/kernel/cpu/mcheck/mce_intel.c | 2 - 4 files changed, 67 insertions(+), 34 deletions(-) diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h index a3ac52b..4576930 100644 --- a/arch/x86/include/asm/mce.h +++ b/arch/x86/include/asm/mce.h @@ -169,8 +169,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 6a05c1d..9a165a2 100644 --- a/arch/x86/kernel/cpu/mcheck/mce-internal.h +++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h @@ -11,6 +11,15 @@ enum severity_level { MCE_PANIC_SEVERITY, }; +struct mce_config { + __u32 cmci_disabled : 1, + ignore_ce : 1, + dont_log_ce : 1, + __pad : 29; +}; + +extern struct mce_config mce_cfg; + #define ATTR_LEN 16 /* One object for each MCE bank, shared by all CPUs */ diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c index bf276eb..cbdef73 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_config mce_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) && !mce_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 (mce_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; + mce_cfg.cmci_disabled = 1; else if (!strcmp(str, "dont_log_ce")) - mce_dont_log_ce = 1; + mce_cfg.dont_log_ce = 1; else if (!strcmp(str, "ignore_ce")) - mce_ignore_ce = 1; + mce_cfg.ignore_ce = 1; else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog")) mce_bootlog = (str[0] == 'b'); else if (isdigit(str[0])) { @@ -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", mce_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; + + mce_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", mce_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 (mce_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; + mce_cfg.ignore_ce = 1; } else { /* enable ce features */ - mce_ignore_ce = 0; + mce_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", mce_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 (mce_cfg.cmci_disabled ^ !!new) { if (new) { /* disable cmci */ on_each_cpu(mce_disable_cmci, NULL, 1); - mce_cmci_disabled = 1; + mce_cfg.cmci_disabled = 1; } else { /* enable cmci */ - mce_cmci_disabled = 0; + mce_cfg.cmci_disabled = 0; on_each_cpu(mce_enable_ce, NULL, 1); } } @@ -2188,32 +2222,24 @@ 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 -}; - /* Use this _only_ for per-cpu attributes */ 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 }; @@ -2223,9 +2249,9 @@ static struct attribute *mce_device_global_attrs[] = { &dev_attr_check_interval.attr.attr, &dev_attr_trigger.attr, &dev_attr_monarch_timeout.attr.attr, - &dev_attr_dont_log_ce.attr.attr, - &dev_attr_ignore_ce.attr.attr, - &dev_attr_cmci_disabled.attr.attr, + &dev_attr_dont_log_ce.attr, + &dev_attr_ignore_ce.attr, + &dev_attr_cmci_disabled.attr, NULL }; diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c index 098386f..a6c028d 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 (mce_cfg.cmci_disabled || mce_cfg.ignore_ce) return 0; /* ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure 2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao @ 2012-09-05 17:15 ` Joe Perches 2012-09-05 18:56 ` Tony Luck 1 sibling, 0 replies; 16+ messages in thread From: Joe Perches @ 2012-09-05 17:15 UTC (permalink / raw) To: Naveen N. Rao Cc: tony.luck, andi, bp, gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh, linux-edac On Wed, 2012-09-05 at 15:52 +0530, Naveen N. Rao wrote: > Many MCE flags are boolean in nature, but are declared as integers > currently. We can pack these into a bitfield to save some space. [] > diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h [] > @@ -11,6 +11,15 @@ enum severity_level { > MCE_PANIC_SEVERITY, > }; > > +struct mce_config { > + __u32 cmci_disabled : 1, > + ignore_ce : 1, > + dont_log_ce : 1, > + __pad : 29; You don't need to declare __pad Perhaps bool:1 instead. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure 2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao 2012-09-05 17:15 ` Joe Perches @ 2012-09-05 18:56 ` Tony Luck 2012-09-06 6:48 ` Naveen N. Rao 1 sibling, 1 reply; 16+ messages in thread From: Tony Luck @ 2012-09-05 18:56 UTC (permalink / raw) To: Naveen N. Rao Cc: andi, bp, gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh, linux-edac On Wed, Sep 5, 2012 at 3:22 AM, Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote: > Many MCE flags are boolean in nature, but are declared as integers > currently. We can pack these into a bitfield to save some space. Before this patch: size arch/x86/kernel/cpu/mcheck/mce.o text data bss dec hex filename 18946 4930 776 24652 604c arch/x86/kernel/cpu/mcheck/mce.o After: size arch/x86/kernel/cpu/mcheck/mce.o text data bss dec hex filename 19335 4890 776 25001 61a9 arch/x86/kernel/cpu/mcheck/mce.o So we do indeed see "data" reduced by 40 bytes. But "text" is up by 389. This seems to be because you have another change, not described in the commit log, buried in part 2 to add get_dont_log_ce(), set_dont_log_ce() etc. Compiler version: gcc version 4.4.6 20120305 (Red Hat 4.4.6-4) (GCC) I know I'm contradicting the feedback you got from Borislav here, but is this code churn really worth it to save 40 bytes? I don't think so. -Tony ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure 2012-09-05 18:56 ` Tony Luck @ 2012-09-06 6:48 ` Naveen N. Rao 2012-09-06 12:15 ` Borislav Petkov 0 siblings, 1 reply; 16+ messages in thread From: Naveen N. Rao @ 2012-09-06 6:48 UTC (permalink / raw) To: Tony Luck, bp Cc: andi, gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh, linux-edac On 09/06/2012 12:26 AM, Tony Luck wrote: > On Wed, Sep 5, 2012 at 3:22 AM, Naveen N. Rao > <naveen.n.rao@linux.vnet.ibm.com> wrote: >> Many MCE flags are boolean in nature, but are declared as integers >> currently. We can pack these into a bitfield to save some space. > > Before this patch: > size arch/x86/kernel/cpu/mcheck/mce.o > text data bss dec hex filename > 18946 4930 776 24652 604c arch/x86/kernel/cpu/mcheck/mce.o > > After: > size arch/x86/kernel/cpu/mcheck/mce.o > text data bss dec hex filename > 19335 4890 776 25001 61a9 arch/x86/kernel/cpu/mcheck/mce.o > > So we do indeed see "data" reduced by 40 bytes. But > "text" is up by 389. This seems to be because you have > another change, not described in the commit log, buried > in part 2 to add get_dont_log_ce(), set_dont_log_ce() etc. > > Compiler version: gcc version 4.4.6 20120305 (Red Hat 4.4.6-4) (GCC) > > I know I'm contradicting the feedback you got from Borislav here, but > is this code churn really worth it to save 40 bytes? I don't think so. Hmm.. I think I agree. I don't see a good way to get rid of the individual getters and setters without adding some more code churn. I guess using boolean would be better. Boris? Thanks, Naveen ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure 2012-09-06 6:48 ` Naveen N. Rao @ 2012-09-06 12:15 ` Borislav Petkov 0 siblings, 0 replies; 16+ messages in thread From: Borislav Petkov @ 2012-09-06 12:15 UTC (permalink / raw) To: Naveen N. Rao Cc: Tony Luck, andi, gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh, linux-edac On Thu, Sep 06, 2012 at 12:18:31PM +0530, Naveen N. Rao wrote: > >I know I'm contradicting the feedback you got from Borislav here, but > >is this code churn really worth it to save 40 bytes? I don't think > >so. Well, to answer Tony's question, I wanted to have all those config booleans at the beginning of mce.c packed tightly together as a single-bit bool values in a config-like struct so that they don't scatter all over the place and grow out of proportion with more features being added. So actually I'm willing to swallow the slight increase in code size for better/more clear code. > Hmm.. I think I agree. I don't see a good way to get rid of the > individual getters and setters without adding some more code churn. > I guess using boolean would be better. Boris? Yes, it would be worth to try to add a DEVICE_BOOL_ATTR which introduce a setter/getter flavour for bools and then use that for all. 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] x86/mce: Honour bios-set CMCI threshold 2012-09-05 10:21 [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds Naveen N. Rao 2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao 2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao @ 2012-09-05 10:22 ` Naveen N. Rao 2 siblings, 0 replies; 16+ messages in thread From: Naveen N. Rao @ 2012-09-05 10:22 UTC (permalink / raw) To: tony.luck, andi, bp Cc: gong.chen, ananth, x86, linux-kernel, mingo, hpa, tglx, gregkh, linux-edac The ACPI spec doesn't provide for a way for the bios to pass down recommended thresholds to the OS on a _per-bank_ basis. This patch adds a new boot option, which if passed, allows bios to initialize the CMCI threshold. In such a case, we simply skip programming any threshold value. As fail-safe, we initialize threshold to 1 if some banks have not been initialized by the bios and warn the user. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- Documentation/x86/x86_64/boot-options.txt | 5 ++++ arch/x86/kernel/cpu/mcheck/mce-internal.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/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h index 9a165a2..cfc2852 100644 --- a/arch/x86/kernel/cpu/mcheck/mce-internal.h +++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h @@ -15,7 +15,8 @@ struct mce_config { __u32 cmci_disabled : 1, ignore_ce : 1, dont_log_ce : 1, - __pad : 29; + bios_cmci_threshold : 1, + __pad : 28; }; extern struct mce_config mce_cfg; diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c index cbdef73..e0985a7 100644 --- a/arch/x86/kernel/cpu/mcheck/mce.c +++ b/arch/x86/kernel/cpu/mcheck/mce.c @@ -1945,6 +1945,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) { @@ -1964,6 +1965,8 @@ static int __init mcheck_enable(char *str) mce_cfg.ignore_ce = 1; else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog")) mce_bootlog = (str[0] == 'b'); + else if (!strcmp(str, "bios_cmci_threshold")) + mce_cfg.bios_cmci_threshold = 1; else if (isdigit(str[0])) { get_option(&str, &tolerant); if (*str == ',') { @@ -2210,6 +2213,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_cfg.bios_cmci_threshold); +} + static ssize_t store_int_with_restart(struct device *s, struct device_attribute *attr, const char *buf, size_t size) @@ -2225,6 +2235,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), @@ -2252,6 +2263,7 @@ static struct attribute *mce_device_global_attrs[] = { &dev_attr_dont_log_ce.attr, &dev_attr_ignore_ce.attr, &dev_attr_cmci_disabled.attr, + &dev_attr_bios_cmci_threshold.attr, NULL }; diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c index a6c028d..6a2de06 100644 --- a/arch/x86/kernel/cpu/mcheck/mce_intel.c +++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c @@ -181,10 +181,16 @@ static void cmci_discover(int banks) unsigned long *owned = (void *)&__get_cpu_var(mce_banks_owned); unsigned long flags; int i; + int bios_wrong_thresh = 0; + + if (mce_cfg.bios_cmci_threshold) + printk_once(KERN_INFO + "bios_cmci_threshold: Using bios-set threshold values for CMCI"); raw_spin_lock_irqsave(&cmci_discover_lock, flags); for (i = 0; i < banks; i++) { u64 val; + int bios_zero_thresh = 0; if (test_bit(i, owned)) continue; @@ -198,8 +204,20 @@ static void cmci_discover(int banks) continue; } - val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK; - val |= MCI_CTL2_CMCI_EN | CMCI_THRESHOLD; + if (!mce_cfg.bios_cmci_threshold) { + val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK; + val |= CMCI_THRESHOLD; + } else if (!(val & MCI_CTL2_CMCI_THRESHOLD_MASK)) { + /* + * If bios_cmci_threshold boot option was specified + * but the threshold is zero, we'll try to initialize + * it to 1. + */ + bios_zero_thresh = 1; + val |= CMCI_THRESHOLD; + } + + val |= MCI_CTL2_CMCI_EN; wrmsrl(MSR_IA32_MCx_CTL2(i), val); rdmsrl(MSR_IA32_MCx_CTL2(i), val); @@ -207,11 +225,26 @@ static void cmci_discover(int banks) if (val & MCI_CTL2_CMCI_EN) { set_bit(i, owned); __clear_bit(i, __get_cpu_var(mce_poll_banks)); + /* + * We are able to set thresholds for some banks that + * had a threshold of 0. This means the BIOS has not + * set the thresholds properly or does not work with + * this boot option. Note down now and report later. + */ + if (mce_cfg.bios_cmci_threshold && bios_zero_thresh && + (val & MCI_CTL2_CMCI_THRESHOLD_MASK)) + bios_wrong_thresh = 1; } else { WARN_ON(!test_bit(i, __get_cpu_var(mce_poll_banks))); } } raw_spin_unlock_irqrestore(&cmci_discover_lock, flags); + if (mce_cfg.bios_cmci_threshold && bios_wrong_thresh) { + printk_once(KERN_INFO + "bios_cmci_threshold: Some banks do not have valid thresholds set"); + printk_once(KERN_INFO + "bios_cmci_threshold: Make sure your BIOS supports this boot option"); + } } /* @@ -249,7 +282,7 @@ void cmci_clear(void) continue; /* Disable CMCI */ rdmsrl(MSR_IA32_MCx_CTL2(i), val); - val &= ~(MCI_CTL2_CMCI_EN|MCI_CTL2_CMCI_THRESHOLD_MASK); + val &= ~MCI_CTL2_CMCI_EN; wrmsrl(MSR_IA32_MCx_CTL2(i), val); __clear_bit(i, __get_cpu_var(mce_banks_owned)); } ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-09-06 13:21 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-05 10:21 [PATCH 0/3] x86:mce: Some cleanups and bios-set CMCI thresholds Naveen N. Rao 2012-09-05 10:21 ` [PATCH 1/3] x86/mce: Make sysfs tunables available globally across all cpus Naveen N. Rao 2012-09-05 10:32 ` [PATCH] [mcelog] Start using the new sysfs tunables location Naveen N. Rao 2012-09-05 18:47 ` Andi Kleen 2012-09-05 19:09 ` Tony Luck 2012-09-06 6:40 ` Naveen N. Rao 2012-09-06 12:28 ` Andi Kleen 2012-09-06 12:34 ` Naveen N. Rao 2012-09-06 12:51 ` Alan Cox 2012-09-06 13:21 ` Andi Kleen 2012-09-05 10:22 ` [PATCH 2/3] x86/mce: Pack boolean MCE flags into a structure Naveen N. Rao 2012-09-05 17:15 ` Joe Perches 2012-09-05 18:56 ` Tony Luck 2012-09-06 6:48 ` Naveen N. Rao 2012-09-06 12:15 ` Borislav Petkov 2012-09-05 10:22 ` [PATCH 3/3] x86/mce: Honour bios-set CMCI threshold Naveen N. Rao
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.