* [RFC PATCH 1/3] Add DEVICE_BIT_ATTR
2012-10-10 14:19 [RFC PATCH 0/3] mca_config stuff Borislav Petkov
@ 2012-10-10 14:19 ` Borislav Petkov
2012-10-10 14:20 ` [RFC PATCH 2/3] Change mce_dont_log_ce Borislav Petkov
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Borislav Petkov @ 2012-10-10 14:19 UTC (permalink / raw)
To: Tony Luck; +Cc: LKML, Borislav Petkov
From: Borislav Petkov <borislav.petkov@amd.com>
Not-Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFC PATCH 2/3] Change mce_dont_log_ce
2012-10-10 14:19 [RFC PATCH 0/3] mca_config stuff Borislav Petkov
2012-10-10 14:19 ` [RFC PATCH 1/3] Add DEVICE_BIT_ATTR Borislav Petkov
@ 2012-10-10 14:20 ` Borislav Petkov
2012-10-10 14:20 ` [RFC PATCH 3/3] Convert mce_disabled Borislav Petkov
2012-10-10 15:35 ` [RFC PATCH 0/3] mca_config stuff Luck, Tony
3 siblings, 0 replies; 13+ messages in thread
From: Borislav Petkov @ 2012-10-10 14:20 UTC (permalink / raw)
To: Tony Luck; +Cc: LKML, Borislav Petkov
From: Borislav Petkov <borislav.petkov@amd.com>
Not-Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFC PATCH 3/3] Convert mce_disabled
2012-10-10 14:19 [RFC PATCH 0/3] mca_config stuff Borislav Petkov
2012-10-10 14:19 ` [RFC PATCH 1/3] Add DEVICE_BIT_ATTR Borislav Petkov
2012-10-10 14:20 ` [RFC PATCH 2/3] Change mce_dont_log_ce Borislav Petkov
@ 2012-10-10 14:20 ` Borislav Petkov
2012-10-10 15:46 ` Luck, Tony
2012-10-12 10:50 ` Naveen N. Rao
2012-10-10 15:35 ` [RFC PATCH 0/3] mca_config stuff Luck, Tony
3 siblings, 2 replies; 13+ messages in thread
From: Borislav Petkov @ 2012-10-10 14:20 UTC (permalink / raw)
To: Tony Luck; +Cc: LKML, Borislav Petkov
From: Borislav Petkov <borislav.petkov@amd.com>
Not-Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: [RFC PATCH 3/3] Convert mce_disabled
2012-10-10 14:20 ` [RFC PATCH 3/3] Convert mce_disabled Borislav Petkov
@ 2012-10-10 15:46 ` Luck, Tony
2012-10-10 15:53 ` Borislav Petkov
2012-10-12 10:50 ` Naveen N. Rao
1 sibling, 1 reply; 13+ messages in thread
From: Luck, Tony @ 2012-10-10 15:46 UTC (permalink / raw)
To: Borislav Petkov; +Cc: LKML, Borislav Petkov
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 3/3] Convert mce_disabled
2012-10-10 15:46 ` Luck, Tony
@ 2012-10-10 15:53 ` Borislav Petkov
0 siblings, 0 replies; 13+ messages in thread
From: Borislav Petkov @ 2012-10-10 15:53 UTC (permalink / raw)
To: Luck, Tony; +Cc: LKML
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 3/3] Convert mce_disabled
2012-10-10 14:20 ` [RFC PATCH 3/3] Convert mce_disabled Borislav Petkov
2012-10-10 15:46 ` Luck, Tony
@ 2012-10-12 10:50 ` Naveen N. Rao
2012-10-12 11:56 ` Borislav Petkov
1 sibling, 1 reply; 13+ messages in thread
From: Naveen N. Rao @ 2012-10-12 10:50 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Tony Luck, LKML, Borislav Petkov
On 10/10/2012 07:50 PM, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
>
> Not-Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 3/3] Convert mce_disabled
2012-10-12 10:50 ` Naveen N. Rao
@ 2012-10-12 11:56 ` Borislav Petkov
2012-10-12 17:46 ` Luck, Tony
2012-10-15 5:53 ` Naveen N. Rao
0 siblings, 2 replies; 13+ messages in thread
From: Borislav Petkov @ 2012-10-12 11:56 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: Tony Luck, LKML, Borislav Petkov
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [RFC PATCH 3/3] Convert mce_disabled
2012-10-12 11:56 ` Borislav Petkov
@ 2012-10-12 17:46 ` Luck, Tony
2012-10-12 21:58 ` Borislav Petkov
2012-10-15 5:53 ` Naveen N. Rao
1 sibling, 1 reply; 13+ messages in thread
From: Luck, Tony @ 2012-10-12 17:46 UTC (permalink / raw)
To: Borislav Petkov, Naveen N. Rao; +Cc: LKML, Borislav Petkov
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 3/3] Convert mce_disabled
2012-10-12 17:46 ` Luck, Tony
@ 2012-10-12 21:58 ` Borislav Petkov
0 siblings, 0 replies; 13+ messages in thread
From: Borislav Petkov @ 2012-10-12 21:58 UTC (permalink / raw)
To: Luck, Tony; +Cc: Naveen N. Rao, LKML, Borislav Petkov
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 3/3] Convert mce_disabled
2012-10-12 11:56 ` Borislav Petkov
2012-10-12 17:46 ` Luck, Tony
@ 2012-10-15 5:53 ` Naveen N. Rao
1 sibling, 0 replies; 13+ messages in thread
From: Naveen N. Rao @ 2012-10-15 5:53 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Tony Luck, LKML, Borislav Petkov
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [RFC PATCH 0/3] mca_config stuff
2012-10-10 14:19 [RFC PATCH 0/3] mca_config stuff Borislav Petkov
` (2 preceding siblings ...)
2012-10-10 14:20 ` [RFC PATCH 3/3] Convert mce_disabled Borislav Petkov
@ 2012-10-10 15:35 ` Luck, Tony
2012-10-10 19:53 ` Borislav Petkov
3 siblings, 1 reply; 13+ messages in thread
From: Luck, Tony @ 2012-10-10 15:35 UTC (permalink / raw)
To: Borislav Petkov; +Cc: LKML, Borislav Petkov, jejb, deller
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 0/3] mca_config stuff
2012-10-10 15:35 ` [RFC PATCH 0/3] mca_config stuff Luck, Tony
@ 2012-10-10 19:53 ` Borislav Petkov
0 siblings, 0 replies; 13+ messages in thread
From: Borislav Petkov @ 2012-10-10 19:53 UTC (permalink / raw)
To: Luck, Tony; +Cc: LKML, Borislav Petkov, jejb, deller
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
^ permalink raw reply [flat|nested] 13+ messages in thread