* [PATCH -next] edac/mce_amd: fix shift warning
@ 2010-10-13 20:10 Randy Dunlap
2010-10-13 22:39 ` Doug Thompson
2010-10-14 4:24 ` Borislav Petkov
0 siblings, 2 replies; 9+ messages in thread
From: Randy Dunlap @ 2010-10-13 20:10 UTC (permalink / raw)
To: lkml; +Cc: Doug Thompson, Borislav Petkov, akpm
From: Randy Dunlap <randy.dunlap@oracle.com>
The BIT() macro works on unsigned longs, but this warning happens
on i386 (X86_32), where UL is 32 bits and this value needs to be
64 bits, so use open-coded ULL.
drivers/edac/mce_amd.c:262: warning: left shift count >= width of type
Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
Cc: Doug Thompson <dougthompson@xmission.com>
Cc: Borislav Petkov <borislav.petkov@amd.com>
---
drivers/edac/mce_amd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-next-20101012.orig/drivers/edac/mce_amd.c
+++ linux-next-20101012/drivers/edac/mce_amd.c
@@ -259,7 +259,8 @@ static void amd_decode_ic_mce(struct mce
pr_cont("%s TLB %s.\n", LL_MSG(ec),
(xec ? "multimatch" : "parity error"));
else if (BUS_ERROR(ec)) {
- bool k8 = (boot_cpu_data.x86 == 0xf && (m->status & BIT(58)));
+ bool k8 = (boot_cpu_data.x86 == 0xf &&
+ (m->status & (1ULL << 58)));
pr_cont("during %s.\n", (k8 ? "system linefill" : "NB data read"));
} else if (fam_ops->ic_mce(ec))
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next] edac/mce_amd: fix shift warning
2010-10-13 20:10 [PATCH -next] edac/mce_amd: fix shift warning Randy Dunlap
@ 2010-10-13 22:39 ` Doug Thompson
2010-10-14 4:24 ` Borislav Petkov
1 sibling, 0 replies; 9+ messages in thread
From: Doug Thompson @ 2010-10-13 22:39 UTC (permalink / raw)
To: lkml, Randy Dunlap; +Cc: Doug Thompson, Borislav Petkov, akpm
--- On Wed, 10/13/10, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> From: Randy Dunlap <randy.dunlap@oracle.com>
> Subject: [PATCH -next] edac/mce_amd: fix shift warning
> To: "lkml" <linux-kernel@vger.kernel.org>
> Cc: "Doug Thompson" <dougthompson@xmission.com>, "Borislav Petkov" <borislav.petkov@amd.com>, "akpm" <akpm@linux-foundation.org>
> Date: Wednesday, October 13, 2010, 2:10 PM
> From: Randy Dunlap <randy.dunlap@oracle.com>
>
> The BIT() macro works on unsigned longs, but this warning
> happens
> on i386 (X86_32), where UL is 32 bits and this value needs
> to be
> 64 bits, so use open-coded ULL.
good find
>
> drivers/edac/mce_amd.c:262: warning: left shift count >=
> width of type
>
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> Cc: Doug Thompson <dougthompson@xmission.com>
> Cc: Borislav Petkov <borislav.petkov@amd.com>
> ---
> drivers/edac/mce_amd.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- linux-next-20101012.orig/drivers/edac/mce_amd.c
> +++ linux-next-20101012/drivers/edac/mce_amd.c
> @@ -259,7 +259,8 @@ static void amd_decode_ic_mce(struct
> mce
> pr_cont("%s TLB
> %s.\n", LL_MSG(ec),
>
> (xec ? "multimatch" : "parity error"));
> else if (BUS_ERROR(ec)) {
> - bool k8 =
> (boot_cpu_data.x86 == 0xf && (m->status &
> BIT(58)));
> + bool k8 =
> (boot_cpu_data.x86 == 0xf &&
> +
> (m->status & (1ULL <<
> 58)));
>
> pr_cont("during
> %s.\n", (k8 ? "system linefill" : "NB data read"));
> } else if (fam_ops->ic_mce(ec))
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next] edac/mce_amd: fix shift warning
2010-10-13 20:10 [PATCH -next] edac/mce_amd: fix shift warning Randy Dunlap
2010-10-13 22:39 ` Doug Thompson
@ 2010-10-14 4:24 ` Borislav Petkov
2010-10-14 10:58 ` [PATCH] bitops.h: Widen BIT macro to support 64-bit types Borislav Petkov
1 sibling, 1 reply; 9+ messages in thread
From: Borislav Petkov @ 2010-10-14 4:24 UTC (permalink / raw)
To: Randy Dunlap; +Cc: lkml, Doug Thompson, akpm
From: Randy Dunlap <randy.dunlap@oracle.com>
Date: Wed, Oct 13, 2010 at 04:10:57PM -0400
> The BIT() macro works on unsigned longs, but this warning happens
> on i386 (X86_32), where UL is 32 bits and this value needs to be
> 64 bits, so use open-coded ULL.
>
> drivers/edac/mce_amd.c:262: warning: left shift count >= width of type
Ok, so BIT() should be fixed to work with the largest type available,
IMHO. Let me cook up something.
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> Cc: Doug Thompson <dougthompson@xmission.com>
> Cc: Borislav Petkov <borislav.petkov@amd.com>
In the meantime, I'll push your fix to Linus, thanks for testing this on
32-bit :).
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] bitops.h: Widen BIT macro to support 64-bit types
2010-10-14 4:24 ` Borislav Petkov
@ 2010-10-14 10:58 ` Borislav Petkov
2010-10-14 11:12 ` Matthew Wilcox
2010-10-14 15:03 ` Linus Torvalds
0 siblings, 2 replies; 9+ messages in thread
From: Borislav Petkov @ 2010-10-14 10:58 UTC (permalink / raw)
To: Randy Dunlap, Linus Torvalds; +Cc: lkml, Doug Thompson, akpm, linux-arch
> From: Randy Dunlap <randy.dunlap@oracle.com>
> Date: Wed, Oct 13, 2010 at 04:10:57PM -0400
>
> > The BIT() macro works on unsigned longs, but this warning happens
> > on i386 (X86_32), where UL is 32 bits and this value needs to be
> > 64 bits, so use open-coded ULL.
> >
> > drivers/edac/mce_amd.c:262: warning: left shift count >= width of type
>
> Ok, so BIT() should be fixed to work with the largest type available,
> IMHO. Let me cook up something.
Maybe something like the following. Build-tested with the crosstool
(http://www.kernel.org/pub/tools/crosstool) on the following arches:
alpha blackfin cris hppa64 ia64 mips64 sparc.
Any objections?
--
From: Borislav Petkov <Borislav.Petkov@amd.com>
Date: Thu, 14 Oct 2010 12:00:17 +0200
Subject: [PATCH] bitops.h: Widen BIT macro to support 64-bit types
Nowadays, every arch defines an unsigned 64-bit type using either one
of the include/asm-generic/int-l{,l}64.h headers. Thus, make the BIT()
macro return that 64-bit type by default. This makes sense on x86 when
manipulating MSR values and prevents overflow errors on 32-bit arches,
for example.
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
include/linux/bitops.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index fc68053..7b9170b 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -3,7 +3,7 @@
#include <asm/types.h>
#ifdef __KERNEL__
-#define BIT(nr) (1UL << (nr))
+#define BIT(nr) (U64_C(1) << (nr))
#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
#define BITS_PER_BYTE 8
--
1.7.3.1.50.g1e633
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types
2010-10-14 10:58 ` [PATCH] bitops.h: Widen BIT macro to support 64-bit types Borislav Petkov
@ 2010-10-14 11:12 ` Matthew Wilcox
2010-10-14 12:12 ` Borislav Petkov
2010-10-14 15:03 ` Linus Torvalds
1 sibling, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2010-10-14 11:12 UTC (permalink / raw)
To: Borislav Petkov
Cc: Randy Dunlap, Linus Torvalds, lkml, Doug Thompson, akpm, linux-arch
On Thu, Oct 14, 2010 at 12:58:55PM +0200, Borislav Petkov wrote:
> #ifdef __KERNEL__
> -#define BIT(nr) (1UL << (nr))
> +#define BIT(nr) (U64_C(1) << (nr))
Why not just use 1ULL instead?
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types
2010-10-14 11:12 ` Matthew Wilcox
@ 2010-10-14 12:12 ` Borislav Petkov
2010-10-14 14:27 ` Matthew Wilcox
0 siblings, 1 reply; 9+ messages in thread
From: Borislav Petkov @ 2010-10-14 12:12 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Randy Dunlap, Linus Torvalds, lkml, Doug Thompson, akpm, linux-arch
From: Matthew Wilcox <matthew@wil.cx>
Date: Thu, Oct 14, 2010 at 07:12:13AM -0400
> On Thu, Oct 14, 2010 at 12:58:55PM +0200, Borislav Petkov wrote:
> > #ifdef __KERNEL__
> > -#define BIT(nr) (1UL << (nr))
> > +#define BIT(nr) (U64_C(1) << (nr))
>
> Why not just use 1ULL instead?
Wanted to be __ASSEMBLY__ safe.
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types
2010-10-14 12:12 ` Borislav Petkov
@ 2010-10-14 14:27 ` Matthew Wilcox
0 siblings, 0 replies; 9+ messages in thread
From: Matthew Wilcox @ 2010-10-14 14:27 UTC (permalink / raw)
To: Borislav Petkov
Cc: Randy Dunlap, Linus Torvalds, lkml, Doug Thompson, akpm, linux-arch
On Thu, Oct 14, 2010 at 02:12:30PM +0200, Borislav Petkov wrote:
> From: Matthew Wilcox <matthew@wil.cx>
> Date: Thu, Oct 14, 2010 at 07:12:13AM -0400
>
> > On Thu, Oct 14, 2010 at 12:58:55PM +0200, Borislav Petkov wrote:
> > > #ifdef __KERNEL__
> > > -#define BIT(nr) (1UL << (nr))
> > > +#define BIT(nr) (U64_C(1) << (nr))
> >
> > Why not just use 1ULL instead?
>
> Wanted to be __ASSEMBLY__ safe.
Admirable, but the entire file is __ASSEMBLY__ unsafe at this point,
so I don't see the point.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types
2010-10-14 10:58 ` [PATCH] bitops.h: Widen BIT macro to support 64-bit types Borislav Petkov
2010-10-14 11:12 ` Matthew Wilcox
@ 2010-10-14 15:03 ` Linus Torvalds
2010-10-14 15:36 ` Borislav Petkov
1 sibling, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2010-10-14 15:03 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Randy Dunlap, lkml, Doug Thompson, akpm, linux-arch
On Thu, Oct 14, 2010 at 3:58 AM, Borislav Petkov <bp@amd64.org> wrote:
>>
>> Ok, so BIT() should be fixed to work with the largest type available,
>> IMHO. Let me cook up something.
>
> Maybe something like the following. Build-tested with the crosstool
> (http://www.kernel.org/pub/tools/crosstool) on the following arches:
> alpha blackfin cris hppa64 ia64 mips64 sparc.
>
> Any objections?
Yeah. I object. I have no idea what this will change for everything
else that expects bitops to work on unsigned long values.
I really think that the bug is not in the BIT() definition, but in the
use. If somebody wants a non-unsigned-long bit field, they had better
not use bitops.h.
And no, just changing the BIT() macro to return a 64-bit value is
_not_ trivially safe. Due to C type rules, now all arithmetic using
BIT() will suddenly be 64-bit, which is often *much* slower, and can
introduce real bugs.
On many architectures, a 64-bit non-constant shift will even end up
being a function call. And if the thing is used in a varargs function,
the argument layout will be totally different. We've also had several
issues with 64-bit types and switch() statements, for example. And a
quick grep for '\<BIT(' shows that non-constant cases are not unheard
of, and there's a lot of random use where it is not at all obvious
that it's safe (because it's used for defining other defines).
So no. I do not think BIT() should be 64-bit. It's "unsigned long".
Look at all the other things around it, and look at all the historical
uses.
Linus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types
2010-10-14 15:03 ` Linus Torvalds
@ 2010-10-14 15:36 ` Borislav Petkov
0 siblings, 0 replies; 9+ messages in thread
From: Borislav Petkov @ 2010-10-14 15:36 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Randy Dunlap, lkml, Doug Thompson, akpm, linux-arch
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, Oct 14, 2010 at 08:03:17AM -0700
> On Thu, Oct 14, 2010 at 3:58 AM, Borislav Petkov <bp@amd64.org> wrote:
> >>
> >> Ok, so BIT() should be fixed to work with the largest type available,
> >> IMHO. Let me cook up something.
> >
> > Maybe something like the following. Build-tested with the crosstool
> > (http://www.kernel.org/pub/tools/crosstool) on the following arches:
> > alpha blackfin cris hppa64 ia64 mips64 sparc.
> >
> > Any objections?
>
> Yeah. I object. I have no idea what this will change for everything
> else that expects bitops to work on unsigned long values.
>
> I really think that the bug is not in the BIT() definition, but in the
> use. If somebody wants a non-unsigned-long bit field, they had better
> not use bitops.h.
>
> And no, just changing the BIT() macro to return a 64-bit value is
> _not_ trivially safe. Due to C type rules, now all arithmetic using
> BIT() will suddenly be 64-bit, which is often *much* slower, and can
> introduce real bugs.
>
> On many architectures, a 64-bit non-constant shift will even end up
> being a function call. And if the thing is used in a varargs function,
> the argument layout will be totally different. We've also had several
> issues with 64-bit types and switch() statements, for example. And a
> quick grep for '\<BIT(' shows that non-constant cases are not unheard
> of, and there's a lot of random use where it is not at all obvious
> that it's safe (because it's used for defining other defines).
Concerning safety, I actually had a version which did check the bit
number supplied as an arg for overflowing but this failed when using
BIT() in struct initializers:
.struct_member = { BIT(bla) }
But thanks for the detailed explanation! This makes perfect sense; it
was too much wishful thinking on my part to assume that a ULL BIT()
would be fine after checking that all arches support the unsigned 64-bit
type.
I'm much better off with a local BIT_64() or similar, definition.
Thanks.
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-10-14 15:36 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-13 20:10 [PATCH -next] edac/mce_amd: fix shift warning Randy Dunlap
2010-10-13 22:39 ` Doug Thompson
2010-10-14 4:24 ` Borislav Petkov
2010-10-14 10:58 ` [PATCH] bitops.h: Widen BIT macro to support 64-bit types Borislav Petkov
2010-10-14 11:12 ` Matthew Wilcox
2010-10-14 12:12 ` Borislav Petkov
2010-10-14 14:27 ` Matthew Wilcox
2010-10-14 15:03 ` Linus Torvalds
2010-10-14 15:36 ` Borislav Petkov
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.