From: Borislav Petkov <[email protected]>
Needed for shifting 64-bit values on 32-bit, like MSR values, for
example.
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/include/asm/bitops.h | 2 ++
drivers/edac/mce_amd.h | 2 --
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index b97596e2b68c..a6983b277220 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -15,6 +15,8 @@
#include <linux/compiler.h>
#include <asm/alternative.h>
+#define BIT_64(n) (U64_C(1) << (n))
+
/*
* These have to be done with inline assembly: that way the bit-setting
* is guaranteed to be atomic. All bit operations return 0 if the bit
diff --git a/drivers/edac/mce_amd.h b/drivers/edac/mce_amd.h
index c6074c5cd1ef..8c87a5e87057 100644
--- a/drivers/edac/mce_amd.h
+++ b/drivers/edac/mce_amd.h
@@ -5,8 +5,6 @@
#include <asm/mce.h>
-#define BIT_64(n) (U64_C(1) << (n))
-
#define EC(x) ((x) & 0xffff)
#define XEC(x, mask) (((x) >> 16) & mask)
--
1.7.9.3.362.g71319
From: Borislav Petkov <[email protected]>
Got bitten again by the BIT() macro:
arch/x86/kernel/cpu/mcheck/mce.c: In function '__mcheck_cpu_apply_quirks':
arch/x86/kernel/cpu/mcheck/mce.c:1453:6: warning: left shift count >= width of type
arch/x86/kernel/cpu/mcheck/mce.c:1454:7: warning: left shift count >= width of type
Fix it already.
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/mcheck/mce.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 888fbf9d0adf..0456b9a08086 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1450,9 +1450,9 @@ static int __cpuinit __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
rdmsrl(msrs[i], val);
/* CntP bit set? */
- if (val & BIT(62)) {
- val &= ~BIT(62);
- wrmsrl(msrs[i], val);
+ if (val & BIT_64(62)) {
+ val &= ~BIT_64(62);
+ wrmsrl(msrs[i], val);
}
}
--
1.7.9.3.362.g71319
Commit-ID: e8f380e00840f694599e6ab42806639f7de26f11
Gitweb: http://git.kernel.org/tip/e8f380e00840f694599e6ab42806639f7de26f11
Author: Borislav Petkov <[email protected]>
AuthorDate: Tue, 22 May 2012 12:53:45 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 23 May 2012 17:16:42 +0200
x86/bitops: Move BIT_64() for a wider use
Needed for shifting 64-bit values on 32-bit, like MSR values,
for example.
Signed-off-by: Borislav Petkov <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Frank Arnold <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/bitops.h | 2 ++
drivers/edac/mce_amd.h | 2 --
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index b97596e..a6983b2 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -15,6 +15,8 @@
#include <linux/compiler.h>
#include <asm/alternative.h>
+#define BIT_64(n) (U64_C(1) << (n))
+
/*
* These have to be done with inline assembly: that way the bit-setting
* is guaranteed to be atomic. All bit operations return 0 if the bit
diff --git a/drivers/edac/mce_amd.h b/drivers/edac/mce_amd.h
index c6074c5..8c87a5e 100644
--- a/drivers/edac/mce_amd.h
+++ b/drivers/edac/mce_amd.h
@@ -5,8 +5,6 @@
#include <asm/mce.h>
-#define BIT_64(n) (U64_C(1) << (n))
-
#define EC(x) ((x) & 0xffff)
#define XEC(x, mask) (((x) >> 16) & mask)
Commit-ID: 80f033610fb968e75f5d470233d8d0260d7a72ed
Gitweb: http://git.kernel.org/tip/80f033610fb968e75f5d470233d8d0260d7a72ed
Author: Borislav Petkov <[email protected]>
AuthorDate: Tue, 22 May 2012 12:53:46 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 23 May 2012 17:16:43 +0200
x86/mce: Fix 32-bit build
Got bitten again by the BIT() macro:
arch/x86/kernel/cpu/mcheck/mce.c: In function '__mcheck_cpu_apply_quirks':
arch/x86/kernel/cpu/mcheck/mce.c:1453:6: warning: left shift
count >= width of type arch/x86/kernel/cpu/mcheck/mce.c:1454:7: warning: left shift count >= width of type
Fix it already.
Signed-off-by: Borislav Petkov <[email protected]>
Cc: Frank Arnold <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/cpu/mcheck/mce.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 888fbf9..0456b9a 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1450,9 +1450,9 @@ static int __cpuinit __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
rdmsrl(msrs[i], val);
/* CntP bit set? */
- if (val & BIT(62)) {
- val &= ~BIT(62);
- wrmsrl(msrs[i], val);
+ if (val & BIT_64(62)) {
+ val &= ~BIT_64(62);
+ wrmsrl(msrs[i], val);
}
}
On Wed, 2012-05-23 at 09:07 -0700, tip-bot for Borislav Petkov wrote:
> +#define BIT_64(n) (U64_C(1) << (n))
Because writing 1ULL << n is too much work?
On 05/23/2012 09:10 AM, Peter Zijlstra wrote:
> On Wed, 2012-05-23 at 09:07 -0700, tip-bot for Borislav Petkov wrote:
>> +#define BIT_64(n) (U64_C(1) << (n))
>
> Because writing 1ULL << n is too much work?
>
Because writing 1ULL << n is broken in anything that needs to be used in
assembly, for example.
-hpa
On Wed, 2012-05-23 at 09:08 -0700, tip-bot for Borislav Petkov wrote:
> + if (val & BIT_64(62)) {
> + val &= ~BIT_64(62);
> + wrmsrl(msrs[i], val);
> }
Wouldn't it be much better to name those things, BIT(62), BIT(18) etc.
aren't very descriptive at all.
MCE_MISC_COUNTER_PRESENT is much more descriptive than BIT(62).
On Wed, May 23, 2012 at 09:11:51AM -0700, H. Peter Anvin wrote:
> On 05/23/2012 09:10 AM, Peter Zijlstra wrote:
> > On Wed, 2012-05-23 at 09:07 -0700, tip-bot for Borislav Petkov wrote:
> >> +#define BIT_64(n) (U64_C(1) << (n))
> >
> > Because writing 1ULL << n is too much work?
> >
>
> Because writing 1ULL << n is broken in anything that needs to be used in
> assembly, for example.
Actually we need a BIT() macro that works both
on 32- and 64-bit. But that won't be that easy:
http://lkml.indiana.edu/hypermail/linux/kernel/1010.1/02335.html
And it should return UL for shift values < 32 and ULL otherwise.
--
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 Wed, May 23, 2012 at 06:18:18PM +0200, Peter Zijlstra wrote:
> On Wed, 2012-05-23 at 09:08 -0700, tip-bot for Borislav Petkov wrote:
> > + if (val & BIT_64(62)) {
> > + val &= ~BIT_64(62);
> > + wrmsrl(msrs[i], val);
> > }
>
> Wouldn't it be much better to name those things, BIT(62), BIT(18) etc.
> aren't very descriptive at all.
>
> MCE_MISC_COUNTER_PRESENT is much more descriptive than BIT(62).
I know but I have the exact bit name from the BKDG in the comment above
- CntP - and this is used only at one place - here - so no need for the
defines, IMHO.
But yeah, I see your point too.
--
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 Wed, 2012-05-23 at 18:19 +0200, Borislav Petkov wrote:
> On Wed, May 23, 2012 at 09:11:51AM -0700, H. Peter Anvin wrote:
> > On 05/23/2012 09:10 AM, Peter Zijlstra wrote:
> > > On Wed, 2012-05-23 at 09:07 -0700, tip-bot for Borislav Petkov wrote:
> > >> +#define BIT_64(n) (U64_C(1) << (n))
> > >
> > > Because writing 1ULL << n is too much work?
> > >
> >
> > Because writing 1ULL << n is broken in anything that needs to be used in
> > assembly, for example.
>
> Actually we need a BIT() macro that works both
> on 32- and 64-bit. But that won't be that easy:
> http://lkml.indiana.edu/hypermail/linux/kernel/1010.1/02335.html
>
> And it should return UL for shift values < 32 and ULL otherwise.
If I remember my type rules correctly you'll get something like that
with:
#define BIT(n) ({ typeof(n) __n = (n); (__n < 32) ? (1UL << __n) : (1ULL << __n); })
That said, having it do this might be unexpected, also hpa mentioned
something about assembly magics which will obviously not work with the
above either.
Anyway, ignore me, I just thought the BIT_64() thing looked funny, but
apparently there's good reasons for it.
On Wed, 2012-05-23 at 18:23 +0200, Borislav Petkov wrote:
> I know but I have the exact bit name from the BKDG in the comment above
> - CntP - and this is used only at one place - here - so no need for the
> defines, IMHO.
Yes, but CntP is such a good name that it actually forced me to look at
the BKDG to figure out wtf it meant ;-)
>>
>> Actually we need a BIT() macro that works both
>> on 32- and 64-bit. But that won't be that easy:
>> http://lkml.indiana.edu/hypermail/linux/kernel/1010.1/02335.html
>>
>> And it should return UL for shift values < 32 and ULL otherwise.
>
Why do you want that behavior? That seems bizarre...
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Wed, May 23, 2012 at 9:19 AM, Borislav Petkov
<[email protected]> wrote:
>
> Actually we need a BIT() macro that works both
> on 32- and 64-bit. But that won't be that easy:
We could use __builtin_choose_expr(), but that *only* works with constants.
So we could do this:
static inline unsigned long bit(unsigned int x)
{
return 1ul << x;
}
static inline u64 bit64(unsigned int x)
{
return 1ull << x;
}
#define BIT(x) \
__builtin_choose_expr((x) < 8*sizeof(unsigned long), bit(x), bit64(x))
but then you *have* to use a plain constant for the BIT() macro.
Anything else will error out in a big way. Non-constant users would
have to be modified to use bit() and bit64() instead.
And no, I tested. You apparently can't do
#define __is_longlongshift(x) \
(__builtin_constant_p(x) && (x) < 8*(sizeof(long)))
because while that is a compile-time constant expression, it's not
"constant enough" for __builtin_choose_expr().
Linus
On 05/23/2012 09:40 AM, Linus Torvalds wrote:
> On Wed, May 23, 2012 at 9:19 AM, Borislav Petkov
> <[email protected]> wrote:
>>
>> Actually we need a BIT() macro that works both
>> on 32- and 64-bit. But that won't be that easy:
>
> We could use __builtin_choose_expr(), but that *only* works with constants.
>
And it doesn't work in assembly. However, I really question the assumption.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Wed, May 23, 2012 at 9:31 AM, H. Peter Anvin <[email protected]> wrote:
>>>
>>> And it should return UL for shift values < 32 and ULL otherwise.
>>
>
> Why do you want that behavior? ?That seems bizarre...
We *have* to have that behavior.
A 64-bit value on a 32-bit architecture has fundamentally different
semantics than a 32-bit one.
It expands arithmetic, but it has other semantic differences too.
Think "printf()" etc. We don't want to force people to do 64-bit
arithmetic on x86-32 when they are working with BIT(0), for chrissake!
So if people make BIT(0) be a 64-bit value on a 32-bit architecture,
I'm going to run around naked with a chainsaw, and call people morons.
That's just not acceptable.
Linus
On Wed, May 23, 2012 at 06:31:07PM +0200, Peter Zijlstra wrote:
> On Wed, 2012-05-23 at 18:23 +0200, Borislav Petkov wrote:
> > I know but I have the exact bit name from the BKDG in the comment above
> > - CntP - and this is used only at one place - here - so no need for the
> > defines, IMHO.
>
> Yes, but CntP is such a good name that it actually forced me to look at
> the BKDG to figure out wtf it meant ;-)
Yeah, this is quirks code - you're not supposed to look at it at all :-)
FWIW, I can change it if you feel too strongly about it, I don't really
care that much.
Thanks.
--
Regards/Gruss,
Boris.
On Wed, May 23, 2012 at 9:42 AM, H. Peter Anvin <[email protected]> wrote:
> On 05/23/2012 09:40 AM, Linus Torvalds wrote:
>
> And it doesn't work in assembly. ?However, I really question the assumption.
The asm case is *trivial*.
asm cannot handle anything but constant shifts anyway, so the BIT()
constness rules would remain. But since asm doesn't have any integer
types, you simply do
#ifdef __ASSEMBLY__
#define BIT(x) (1<<(x))
#else
.. the C type-morphing one ..
#endif
As to questioning the assumption, you're simply wrong. BIT(0)
absolutely MUST NOT be a 64-bit value on a 32-bit kernel. End of
discussion. This isn't an "assumption", it's an axiom.
Linus
On Wed, 2012-05-23 at 18:44 +0200, Borislav Petkov wrote:
> FWIW, I can change it if you feel too strongly about it, I don't really
> care that much.
I don't care much at the moment, I might if I'm ever forced to actually
read the MCE code, but until that time I'll crawl back into my
corner :-)
On 05/23/2012 09:43 AM, Linus Torvalds wrote:
> On Wed, May 23, 2012 at 9:31 AM, H. Peter Anvin <[email protected]> wrote:
>>>>
>>>> And it should return UL for shift values < 32 and ULL otherwise.
>>>
>>
>> Why do you want that behavior? That seems bizarre...
>
> We *have* to have that behavior.
>
> A 64-bit value on a 32-bit architecture has fundamentally different
> semantics than a 32-bit one.
>
> It expands arithmetic, but it has other semantic differences too.
> Think "printf()" etc. We don't want to force people to do 64-bit
> arithmetic on x86-32 when they are working with BIT(0), for chrissake!
>
> So if people make BIT(0) be a 64-bit value on a 32-bit architecture,
> I'm going to run around naked with a chainsaw, and call people morons.
> That's just not acceptable.
>
BIT(0), okay. I thought we were talking about BIT_64() here...
Any reason we can't just tell people to use BIT() for a native "unsigned
long" type (32/64 bits) and BIT_64() if they really want a 64-bit result?
There are good reasons for the latter. Consider, for example:
u64 msr;
...
msr &= ~BIT_64(1);
This *better* not be an unsigned 32 bit value, or we just chopped off
the upper half. In this case and similar ones the 64-bitness of the
result really matters.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On 05/23/2012 09:47 AM, H. Peter Anvin wrote:
>
> BIT(0), okay. I thought we were talking about BIT_64() here...
>
> Any reason we can't just tell people to use BIT() for a native "unsigned
> long" type (32/64 bits) and BIT_64() if they really want a 64-bit result?
>
> There are good reasons for the latter. Consider, for example:
>
> u64 msr;
> ...
> msr &= ~BIT_64(1);
>
> This *better* not be an unsigned 32 bit value, or we just chopped off
> the upper half. In this case and similar ones the 64-bitness of the
> result really matters.
>
To better clarify my concern: my concern is that if we make BIT() be a
DWIM type, it will appear to work in most situations. As such, we'll
see things in headers like:
#define MSR_BLAH_FOO BIT(31)
#define MSR_BLAH_BAR BIT(32)
... and *almost all the time* the above will work. But if you use
MSR_BLAH_FOO inverted, then you get truncation.
-hpa
On Wed, May 23, 2012 at 09:31:17AM -0700, H. Peter Anvin wrote:
> >>
> >> Actually we need a BIT() macro that works both
> >> on 32- and 64-bit. But that won't be that easy:
> >> http://lkml.indiana.edu/hypermail/linux/kernel/1010.1/02335.html
> >>
> >> And it should return UL for shift values < 32 and ULL otherwise.
> >
>
> Why do you want that behavior? That seems bizarre...
I forgot to say "on 32-bit" above. So the sentence should've been:
"And it should return UL for shift values < 32 and ULL otherwise on
32-bit."
How about the following completely untested chunk:
#ifdef CONFIG_64BIT
#define BIT(nr) (UC_64(1) << (nr))
#else
#define BIT(nr) \
({ \
unsigned _shift = (nr); \
((_shift > 32) ? (U64_C(1) << _shift) : (U32_C(1) << _shift)); \
})
#endif
Ok? Too ugly? It still changes the return type of unsigned long to ULL
for shift values >= 32 and probably Linus doesn't want that...
Hmm.
--
Regards/Gruss,
Boris.
On Wed, May 23, 2012 at 06:46:45PM +0200, Peter Zijlstra wrote:
> On Wed, 2012-05-23 at 18:44 +0200, Borislav Petkov wrote:
>
> > FWIW, I can change it if you feel too strongly about it, I don't really
> > care that much.
>
> I don't care much at the moment, I might if I'm ever forced to actually
> read the MCE code, but until that time I'll crawl back into my
> corner :-)
Well, our job is to make it as unreadable as possible until that time
comes.
:-)
--
Regards/Gruss,
Boris.
On Wed, May 23, 2012 at 9:47 AM, H. Peter Anvin <[email protected]> wrote:
>
> Any reason we can't just tell people to use BIT() for a native "unsigned
> long" type (32/64 bits) and BIT_64() if they really want a 64-bit result?
Well, that's what we're doing now. And it seems to have resulted in
repeated bugs for architectures where most of the developers run on
64-bit machines, but the same code is actually supposed to work on
32-bit too (ie x86).
Linus
On Wed, 2012-05-23 at 18:29 +0200, Peter Zijlstra wrote:
> If I remember my type rules correctly you'll get something like that
> with:
>
> #define BIT(n) ({ typeof(n) __n = (n); (__n < 32) ? (1UL << __n) : (1ULL << __n); })
OK, that doesn't work. Memory played tricks on me. Now I'll keep
wondering what was special about ?: and how I've seen it abused.
On 05/23/2012 09:54 AM, Linus Torvalds wrote:
> On Wed, May 23, 2012 at 9:47 AM, H. Peter Anvin <[email protected]> wrote:
>>
>> Any reason we can't just tell people to use BIT() for a native "unsigned
>> long" type (32/64 bits) and BIT_64() if they really want a 64-bit result?
>
> Well, that's what we're doing now. And it seems to have resulted in
> repeated bugs for architectures where most of the developers run on
> 64-bit machines, but the same code is actually supposed to work on
> 32-bit too (ie x86).
>
Yes, but I fear that this will result in more subtle bugs which will
therefore be even harder to detect and diagnose.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Wed, May 23, 2012 at 9:53 AM, Borislav Petkov <[email protected]> wrote:
>
> How about the following completely untested chunk:
No, you can't do that. All standard C operations will return *one*
type. That very much includes the ternary ?: operator.
The *only* ways I know of to get two types are
- C preprocessor stuff, ie
#define BIT(x) __BIT_##x
and then just enumerate all the 64 cases. This is portable, but it gets old.
- using __builtin_choose_expr(), which actually allows the two
expressions to have different types, but requires a very strict
compile-time constant (ie you cannot rely on the optimizer making it a
constant - because it needs to choose the expression before the
optimizer runs)
There might be some other magic gcc extension, of course.
Linus
On Wed, May 23, 2012 at 09:57:47AM -0700, Linus Torvalds wrote:
> On Wed, May 23, 2012 at 9:53 AM, Borislav Petkov <[email protected]> wrote:
> >
> > How about the following completely untested chunk:
>
> No, you can't do that. All standard C operations will return *one*
> type. That very much includes the ternary ?: operator.
And, in addition, hpa's example won't work too:
u64 msr = ~BIT(1);
> The *only* ways I know of to get two types are
>
> - C preprocessor stuff, ie
>
> #define BIT(x) __BIT_##x
>
> and then just enumerate all the 64 cases. This is portable, but it gets old.
Nah, that's ugly.
> - using __builtin_choose_expr(), which actually allows the two
> expressions to have different types, but requires a very strict
> compile-time constant (ie you cannot rely on the optimizer making it a
> constant - because it needs to choose the expression before the
> optimizer runs)
>
> There might be some other magic gcc extension, of course.
Hmm and if not, it looks like BIT_64 is the easiest and most readable
thing we can do.
Oh well.
--
Regards/Gruss,
Boris.
On Wed, May 23, 2012 at 10:11 AM, Borislav Petkov <[email protected]> wrote:
>
> And, in addition, hpa's example won't work too:
>
> ? ? ? ?u64 msr = ~BIT(1);
Yes, that's a good example of something that can be problematic, and
is fundamentally different on 32-bit and 64-bit.
So maybe BIT() unconditionally being long, and BIT_64() being u64
simply is the right model.
Linus