From: Borislav Petkov <[email protected]>
Right,
so I did give that a try and it turned out to be a bit more involved
than I thought. Basically, I'm relying on the assumption that if I use a
u64 bitfield and pass a pointer to it, accessing it through that pointer
as a u64 value works. Actually, I have the u64 bitfield as the first
member of a struct and if I cast a pointer to that struct to u64 *, I'm
expecting to have the 64 bits of the same bitfield.
Therefore, I can toggle the bits in the mce code with mca_cfg.<bitname>.
When defining accessing them through the device attributes in sysfs, I
use a new macro DEVICE_BIT_ATTR which gets the corresponding bit number
of that same bit in the bitfield. This gives only one function which
operates on a bitfield instead of a single function per bit in the
bitfield.
For example,
mca_cfg {
u64 dont_log_ce : 1,
is the first bit in the bitfield and I also have a MCA_CFG_DONT_LOG_CE
define which is 0, i.e. the first bit, which I use to toggle the
corresponding bit in the u64 in device_{show,store}_bit.
So I converted mce_dont_log_ce and mce_disabled (renaming it into the
more correct mca_disabled) and it seems to work.
The asm looks sane too. One other advantage is that it makes the code
much more cleaner and compact by collecting all those bool config values
in a single struct, which was my original incentive to do that.
So please take a look and let me know whether this is sane, especially
the above assumption that I can access a u64 bitfield through a u64 *
and the bits are where they're expected to be. gcc seems to do that...
and I don't see anything in the C99 standard that would object to it but
I could be overlooking something.
Thanks.
Borislav Petkov (3):
Add DEVICE_BIT_ATTR
Change mce_dont_log_ce
Convert mce_disabled
arch/x86/include/asm/mce.h | 9 ++++++++-
arch/x86/kernel/cpu/mcheck/mce.c | 21 ++++++++++-----------
arch/x86/lguest/boot.c | 2 +-
drivers/base/core.c | 36 ++++++++++++++++++++++++++++++++++++
include/linux/device.h | 9 +++++++++
5 files changed, 64 insertions(+), 13 deletions(-)
--
1.8.0.rc0.18.gf84667d
From: Borislav Petkov <[email protected]>
Not-Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/include/asm/mce.h | 6 ++++++
arch/x86/kernel/cpu/mcheck/mce.c | 9 +++++----
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 54d73b1f00a0..18a66ac35fc5 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -207,6 +207,12 @@ extern void register_mce_write_callback(ssize_t (*)(struct file *filp,
const char __user *ubuf,
size_t usize, loff_t *off));
+struct mca_config {
+ u64 dont_log_ce : 1,
+#define MCA_CFG_DONT_LOG_CE 0
+ __resv1 : 63;
+};
+
/*
* Exception handler
*/
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 29e87d3b2843..8925bcdc5816 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -79,7 +79,6 @@ 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;
@@ -87,6 +86,8 @@ int mce_bios_cmci_threshold __read_mostly;
struct mce_bank *mce_banks __read_mostly;
+struct mca_config mca_cfg __read_mostly;
+
/* User mode helper program triggered by machine check event */
static unsigned long mce_need_notify;
static char mce_helper[128];
@@ -631,7 +632,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) && !mca_cfg.dont_log_ce)
mce_log(&m);
/*
@@ -1962,7 +1963,7 @@ static int __init mcheck_enable(char *str)
else if (!strcmp(str, "no_cmci"))
mce_cmci_disabled = 1;
else if (!strcmp(str, "dont_log_ce"))
- mce_dont_log_ce = 1;
+ mca_cfg.dont_log_ce = 1;
else if (!strcmp(str, "ignore_ce"))
mce_ignore_ce = 1;
else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
@@ -2192,7 +2193,7 @@ 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_BIT_ATTR(dont_log_ce, 0644, mca_cfg, MCA_CFG_DONT_LOG_CE);
static struct dev_ext_attribute dev_attr_check_interval = {
__ATTR(check_interval, 0644, device_show_int, store_int_with_restart),
--
1.8.0.rc0.18.gf84667d
From: Borislav Petkov <[email protected]>
Not-Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/include/asm/mce.h | 9 +++++----
arch/x86/kernel/cpu/mcheck/mce.c | 12 +++++-------
arch/x86/lguest/boot.c | 2 +-
3 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 18a66ac35fc5..e8ed5a3a0512 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -126,7 +126,6 @@ extern void mce_unregister_decode_chain(struct notifier_block *nb);
#include <linux/init.h>
#include <linux/atomic.h>
-extern int mce_disabled;
extern int mce_p5_enabled;
#ifdef CONFIG_X86_MCE
@@ -208,9 +207,11 @@ extern void register_mce_write_callback(ssize_t (*)(struct file *filp,
size_t usize, loff_t *off));
struct mca_config {
- u64 dont_log_ce : 1,
-#define MCA_CFG_DONT_LOG_CE 0
- __resv1 : 63;
+ u64 dont_log_ce : 1,
+#define MCA_CFG_DONT_LOG_CE 0
+ mca_disabled : 1,
+#define MCA_CFG_MCA_DISABLED 1
+ __resv1 : 62;
};
/*
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 8925bcdc5816..6341c1a0afdd 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -58,8 +58,6 @@ static DEFINE_MUTEX(mce_chrdev_read_mutex);
#define CREATE_TRACE_POINTS
#include <trace/events/mce.h>
-int mce_disabled __read_mostly;
-
#define SPINUNIT 100 /* 100ns */
atomic_t mce_entry;
@@ -514,7 +512,7 @@ static int mce_ring_add(unsigned long pfn)
int mce_available(struct cpuinfo_x86 *c)
{
- if (mce_disabled)
+ if (mca_cfg.mca_disabled)
return 0;
return cpu_has(c, X86_FEATURE_MCE) && cpu_has(c, X86_FEATURE_MCA);
}
@@ -1669,7 +1667,7 @@ void (*machine_check_vector)(struct pt_regs *, long error_code) =
*/
void __cpuinit mcheck_cpu_init(struct cpuinfo_x86 *c)
{
- if (mce_disabled)
+ if (mca_cfg.mca_disabled)
return;
if (__mcheck_cpu_ancient_init(c))
@@ -1679,7 +1677,7 @@ void __cpuinit mcheck_cpu_init(struct cpuinfo_x86 *c)
return;
if (__mcheck_cpu_cap_init() < 0 || __mcheck_cpu_apply_quirks(c) < 0) {
- mce_disabled = 1;
+ mca_cfg.mca_disabled = 1;
return;
}
@@ -1959,7 +1957,7 @@ static int __init mcheck_enable(char *str)
if (*str == '=')
str++;
if (!strcmp(str, "off"))
- mce_disabled = 1;
+ mca_cfg.mca_disabled = 1;
else if (!strcmp(str, "no_cmci"))
mce_cmci_disabled = 1;
else if (!strcmp(str, "dont_log_ce"))
@@ -2433,7 +2431,7 @@ device_initcall_sync(mcheck_init_device);
*/
static int __init mcheck_disable(char *str)
{
- mce_disabled = 1;
+ mca_cfg.mca_disabled = 1;
return 1;
}
__setup("nomce", mcheck_disable);
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index 642d8805bc1b..0929fbba1371 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -1412,7 +1412,7 @@ __init void lguest_init(void)
/* We don't have features. We have puppies! Puppies! */
#ifdef CONFIG_X86_MCE
- mce_disabled = 1;
+ mca_cfg.mca_disabled = 1;
#endif
#ifdef CONFIG_ACPI
acpi_disabled = 1;
--
1.8.0.rc0.18.gf84667d
struct mca_config {
- u64 dont_log_ce : 1,
-#define MCA_CFG_DONT_LOG_CE 0
- __resv1 : 63;
+ u64 dont_log_ce : 1,
+#define MCA_CFG_DONT_LOG_CE 0
+ mca_disabled : 1,
+#define MCA_CFG_MCA_DISABLED 1
+ __resv1 : 62;
};
If we do head in this direction - I don't think it is useful to change just one bit
on each commit. We should batch in larger groups.
-Tony
On Wed, Oct 10, 2012 at 03:46:39PM +0000, Luck, Tony wrote:
> struct mca_config {
> - u64 dont_log_ce : 1,
> -#define MCA_CFG_DONT_LOG_CE 0
> - __resv1 : 63;
> + u64 dont_log_ce : 1,
> +#define MCA_CFG_DONT_LOG_CE 0
> + mca_disabled : 1,
> +#define MCA_CFG_MCA_DISABLED 1
> + __resv1 : 62;
> };
>
> If we do head in this direction - I don't think it is useful to change just one bit
> on each commit. We should batch in larger groups.
Sure, this is just sanity-checking-the-approach patchset, trying to make
the intent as understandable as possible. The real thing should do a
couple of conversions in one patch, of course.
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
From: Borislav Petkov <[email protected]>
Not-Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/base/core.c | 36 ++++++++++++++++++++++++++++++++++++
include/linux/device.h | 9 +++++++++
2 files changed, 45 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index abea76c36a4b..3946a6fbe7bf 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -171,6 +171,42 @@ ssize_t device_show_int(struct device *dev,
}
EXPORT_SYMBOL_GPL(device_show_int);
+/*
+ * ->var is the bitvector and ->aux is the bit number
+ */
+ssize_t device_store_bit(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ struct dev_ext_attribute *ea = to_ext_attr(attr);
+ u64 *bvec = (u64 *)ea->var;
+ u8 val, bit = ea->aux;
+
+ if (kstrtou8(buf, 10, &val) < 0)
+ return -EINVAL;
+
+ if (val > 63)
+ return -EINVAL;
+
+ if (val)
+ *bvec |= BIT_64(bit);
+ else
+ *bvec &= ~BIT_64(bit);
+
+ return size;
+}
+EXPORT_SYMBOL_GPL(device_store_bit);
+
+ssize_t device_show_bit(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct dev_ext_attribute *ea = to_ext_attr(attr);
+ u64 *bvec = (u64 *)ea->var;
+ u8 bit = ea->aux;
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", !!(*bvec & BIT_64(bit)));
+}
+EXPORT_SYMBOL_GPL(device_show_bit);
+
/**
* device_release - free device structure.
* @kobj: device's kobject.
diff --git a/include/linux/device.h b/include/linux/device.h
index 86ef6ab553b1..78ade603875f 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -486,6 +486,7 @@ struct device_attribute {
struct dev_ext_attribute {
struct device_attribute attr;
void *var;
+ u8 aux;
};
ssize_t device_show_ulong(struct device *dev, struct device_attribute *attr,
@@ -496,6 +497,10 @@ ssize_t device_show_int(struct device *dev, struct device_attribute *attr,
char *buf);
ssize_t device_store_int(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count);
+ssize_t device_show_bit(struct device *dev, struct device_attribute *attr,
+ char *buf);
+ssize_t device_store_bit(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count);
#define DEVICE_ATTR(_name, _mode, _show, _store) \
struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, _store)
@@ -505,6 +510,10 @@ ssize_t device_store_int(struct device *dev, struct device_attribute *attr,
#define DEVICE_INT_ATTR(_name, _mode, _var) \
struct dev_ext_attribute dev_attr_##_name = \
{ __ATTR(_name, _mode, device_show_int, device_store_int), &(_var) }
+#define DEVICE_BIT_ATTR(_name, _mode, _bitvec, _bit) \
+ struct dev_ext_attribute dev_attr_##_name = \
+ { __ATTR(_name, _mode, device_show_bit, device_store_bit), \
+ &(_bitvec), (_bit) }
#define DEVICE_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
struct device_attribute dev_attr_##_name = \
__ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
--
1.8.0.rc0.18.gf84667d
> Therefore, I can toggle the bits in the mce code with mca_cfg.<bitname>.
> When defining accessing them through the device attributes in sysfs, I
> use a new macro DEVICE_BIT_ATTR which gets the corresponding bit number
> of that same bit in the bitfield. This gives only one function which
> operates on a bitfield instead of a single function per bit in the
> bitfield.
Is this true across all architectures? I know that pa-risc instructions
that operate on bitfields use "0" to operate on the high order bit
rather than the low order one. I don't recall whether this spills over
into the compiler. If it did, then you'd have to have different #defines
for the bit numbers[1]. For this specific use case it wouldn't matter because
you are just using it in x86 code. But device_store_bit() and device_show_bit()
are in generic code - so they must be able to work across all architectures.
-Tony
[1] Or fix the store/show bit functions to transform the bit numbers from
"little-bitian" to "big-bitian" on architectures that count the other way.
On Wed, Oct 10, 2012 at 03:35:56PM +0000, Luck, Tony wrote:
> > Therefore, I can toggle the bits in the mce code with mca_cfg.<bitname>.
> > When defining accessing them through the device attributes in sysfs, I
> > use a new macro DEVICE_BIT_ATTR which gets the corresponding bit number
> > of that same bit in the bitfield. This gives only one function which
> > operates on a bitfield instead of a single function per bit in the
> > bitfield.
>
> Is this true across all architectures? I know that pa-risc instructions
> that operate on bitfields use "0" to operate on the high order bit
> rather than the low order one. I don't recall whether this spills over
> into the compiler. If it did, then you'd have to have different #defines
> for the bit numbers[1]. For this specific use case it wouldn't matter because
> you are just using it in x86 code. But device_store_bit() and device_show_bit()
> are in generic code - so they must be able to work across all architectures.
>
> -Tony
>
> [1] Or fix the store/show bit functions to transform the bit numbers from
> "little-bitian" to "big-bitian" on architectures that count the other way.
Ok, the question is whether those device_{show,store}_bit functions
should be really made available in generic code. If yes, then the store case
could be made to work on any arch like this:
if (val)
*bvec |= le64_to_cpu(BIT_64(bit));
else
*bvec &= le64_to_cpu(~BIT_64(bit));
by keeping the bitnumbers little endian.
IMHO of course.
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
On 10/10/2012 07:50 PM, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> Not-Signed-off-by: Borislav Petkov <[email protected]>
> ---
> arch/x86/include/asm/mce.h | 9 +++++----
> arch/x86/kernel/cpu/mcheck/mce.c | 12 +++++-------
> arch/x86/lguest/boot.c | 2 +-
> 3 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 18a66ac35fc5..e8ed5a3a0512 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -126,7 +126,6 @@ extern void mce_unregister_decode_chain(struct notifier_block *nb);
> #include <linux/init.h>
> #include <linux/atomic.h>
>
> -extern int mce_disabled;
> extern int mce_p5_enabled;
>
> #ifdef CONFIG_X86_MCE
> @@ -208,9 +207,11 @@ extern void register_mce_write_callback(ssize_t (*)(struct file *filp,
> size_t usize, loff_t *off));
>
> struct mca_config {
> - u64 dont_log_ce : 1,
> -#define MCA_CFG_DONT_LOG_CE 0
> - __resv1 : 63;
> + u64 dont_log_ce : 1,
> +#define MCA_CFG_DONT_LOG_CE 0
> + mca_disabled : 1,
> +#define MCA_CFG_MCA_DISABLED 1
> + __resv1 : 62;
> };
Hi Boris,
Thanks for getting to this before I could!
I had a look but I still feel boolean is a better way to go. With bool,
we can get rid of the #defines above and more importantly, the aux field
in dev_ext_attribute since that is used in other places too. Further, I
suspect we'll still end up using the same or less memory since we don't
have that many boolean members within the MCA code.
Regards,
Naveen
On Fri, Oct 12, 2012 at 04:20:40PM +0530, Naveen N. Rao wrote:
> Hi Boris, Thanks for getting to this before I could!
Ah ok, I thought you wasn't interested in doing this anymore :).
> I had a look but I still feel boolean is a better way to go. With
> bool, we can get rid of the #defines above and more importantly, the
> aux field in dev_ext_attribute since that is used in other places
> too. Further, I suspect we'll still end up using the same or less
> memory since we don't have that many boolean members within the MCA
> code.
My main intention was to have all those in a single struct and use a
single store_bit/show_bit function.
Sure, you can do bools but this'll still be single variables spread
around in mce.c instead of one single struct mca_config which nicely
encapsulates all the configuration we do in the MCA code.
Or, you can modify the mca_config I have there and use bools and pass a
pointer to each actual bool member in each DEVICE_BIT_ATTR invocation
(and rename it to DEVICE_BOOL_ATTR). Yeah, that could work, unless I'm
missing something else, of course.
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
> Or, you can modify the mca_config I have there and use bools and pass a
> pointer to each actual bool member in each DEVICE_BIT_ATTR invocation
> (and rename it to DEVICE_BOOL_ATTR). Yeah, that could work, unless I'm
> missing something else, of course.
This looks like the best solution to me. Sure we use a little more memory for
a "bool" for each option instead of just a single bit. But there are only a
handful of them, not thousands. So I think we can cope with a few extra
bytes of memory consumption. I was still not completely convinced by the
if (val)
*bvec |= le64_to_cpu(BIT_64(bit));
solution - it assumes that big endian machines also assign their bit numbers
in a big->little way - but that isn't required by the C standard. bitfields are
assigned at the whim of the compiler writer (the only restrictions seem to
be on alignments of fields w.r.t. to the underlying data types).
-Tony
On Fri, Oct 12, 2012 at 05:46:16PM +0000, Luck, Tony wrote:
> > Or, you can modify the mca_config I have there and use bools and pass a
> > pointer to each actual bool member in each DEVICE_BIT_ATTR invocation
> > (and rename it to DEVICE_BOOL_ATTR). Yeah, that could work, unless I'm
> > missing something else, of course.
>
> This looks like the best solution to me. Sure we use a little more memory for
> a "bool" for each option instead of just a single bit. But there are only a
> handful of them, not thousands. So I think we can cope with a few extra
> bytes of memory consumption. I was still not completely convinced by the
>
> if (val)
> *bvec |= le64_to_cpu(BIT_64(bit));
>
> solution - it assumes that big endian machines also assign their bit numbers
> in a big->little way - but that isn't required by the C standard. bitfields are
> assigned at the whim of the compiler writer (the only restrictions seem to
> be on alignments of fields w.r.t. to the underlying data types).
Ok, it seems that would've been a can of worms if we'd opened it.
Fortunately, if we do bools and we pass a pointer to the respective bool
member of mca_config, we won't need to do that anymore. Instead simply:
*bool_ptr = !!val;
It can't get any simpler than that. I'll give it a try soon to see
whether it pans out as I'm imagining it.
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
On 10/12/2012 05:26 PM, Borislav Petkov wrote:
> On Fri, Oct 12, 2012 at 04:20:40PM +0530, Naveen N. Rao wrote:
>> Hi Boris, Thanks for getting to this before I could!
>
> Ah ok, I thought you wasn't interested in doing this anymore :).
Sorry - just got sidetracked a bit, I'm afraid :)
>
>> I had a look but I still feel boolean is a better way to go. With
>> bool, we can get rid of the #defines above and more importantly, the
>> aux field in dev_ext_attribute since that is used in other places
>> too. Further, I suspect we'll still end up using the same or less
>> memory since we don't have that many boolean members within the MCA
>> code.
>
> My main intention was to have all those in a single struct and use a
> single store_bit/show_bit function.
>
> Sure, you can do bools but this'll still be single variables spread
> around in mce.c instead of one single struct mca_config which nicely
> encapsulates all the configuration we do in the MCA code.
>
> Or, you can modify the mca_config I have there and use bools and pass a
> pointer to each actual bool member in each DEVICE_BIT_ATTR invocation
> (and rename it to DEVICE_BOOL_ATTR). Yeah, that could work, unless I'm
> missing something else, of course.
Yes, this is what I had in mind. Though your code for use of bitfield is
nicely done, I felt use of boolean will fit better in this specific case.
Thanks,
Naveen