Replace the open-coded shift with BIT(x) to make the code a bit
more self-documenting, at the same time, fix some useless warnings.
Signed-off-by: Gaosheng Cui <[email protected]>
---
v2:
- Change the commit msg, remove the UBSAN warning calltrace, and
merge patch "x86/cpu: fix undefined behavior in bit shift for
intel_detect_tlb" with it. Thanks!
arch/x86/kernel/cpu/amd.c | 2 +-
arch/x86/kernel/cpu/centaur.c | 2 +-
arch/x86/kernel/cpu/hygon.c | 2 +-
arch/x86/kernel/cpu/intel.c | 4 ++--
arch/x86/kernel/cpu/proc.c | 2 +-
arch/x86/kernel/cpu/zhaoxin.c | 2 +-
6 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 860b60273df3..75d82cad323a 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -613,7 +613,7 @@ static void early_init_amd(struct cpuinfo_x86 *c)
* c->x86_power is 8000_0007 edx. Bit 8 is TSC runs at constant rate
* with P/T states and does not stop in deep C-states
*/
- if (c->x86_power & (1 << 8)) {
+ if (c->x86_power & BIT(8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
}
diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
index 345f7d905db6..9910bb1d90fd 100644
--- a/arch/x86/kernel/cpu/centaur.c
+++ b/arch/x86/kernel/cpu/centaur.c
@@ -105,7 +105,7 @@ static void early_init_centaur(struct cpuinfo_x86 *c)
#ifdef CONFIG_X86_64
set_cpu_cap(c, X86_FEATURE_SYSENTER32);
#endif
- if (c->x86_power & (1 << 8)) {
+ if (c->x86_power & BIT(8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
}
diff --git a/arch/x86/kernel/cpu/hygon.c b/arch/x86/kernel/cpu/hygon.c
index 21fd425088fe..dc473bfbf1b5 100644
--- a/arch/x86/kernel/cpu/hygon.c
x86/cpu: fix undefined behavior in bit shift for intel_detect_tlb+++ b/arch/x86/kernel/cpu/hygon.c
@@ -251,7 +251,7 @@ static void early_init_hygon(struct cpuinfo_x86 *c)
* c->x86_power is 8000_0007 edx. Bit 8 is TSC runs at constant rate
* with P/T states and does not stop in deep C-states
*/
- if (c->x86_power & (1 << 8)) {
+ if (c->x86_power & BIT(8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
}
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 2d7ea5480ec3..2bdf6d601a6f 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -286,7 +286,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
* It is also reliable across cores and sockets. (but not across
* cabinets - we turn it off in that case explicitly.)
*/
- if (c->x86_power & (1 << 8)) {
+ if (c->x86_power & BIT(8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
}
@@ -945,7 +945,7 @@ static void intel_detect_tlb(struct cpuinfo_x86 *c)
/* If bit 31 is set, this is an unknown format */
for (j = 0 ; j < 3 ; j++)
- if (regs[j] & (1 << 31))
+ if (regs[j] & BIT(31))
regs[j] = 0;
/* Byte 0 is level count, not a descriptor */
diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 099b6f0d96bd..efa1d39c4f25 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -135,7 +135,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
seq_puts(m, "power management:");
for (i = 0; i < 32; i++) {
- if (c->x86_power & (1 << i)) {
+ if (c->x86_power & BIT(i)) {
if (i < ARRAY_SIZE(x86_power_flags) &&
x86_power_flags[i])
seq_printf(m, "%s%s",
diff --git a/arch/x86/kernel/cpu/zhaoxin.c b/arch/x86/kernel/cpu/zhaoxin.c
index 05fa4ef63490..34a8a460f8f4 100644
--- a/arch/x86/kernel/cpu/zhaoxin.c
+++ b/arch/x86/kernel/cpu/zhaoxin.c
@@ -61,7 +61,7 @@ static void early_init_zhaoxin(struct cpuinfo_x86 *c)
#ifdef CONFIG_X86_64
set_cpu_cap(c, X86_FEATURE_SYSENTER32);
#endif
- if (c->x86_power & (1 << 8)) {
+ if (c->x86_power & BIT(8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
}
--
2.25.1
On Tue, Nov 01, 2022 at 07:14:18PM +0800, Gaosheng Cui wrote:
> Replace the open-coded shift with BIT(x) to make the code a bit
> more self-documenting, at the same time, fix some useless warnings.
Others might feel differently and that's fine, but I always found the
BIT() thing so much less clear than doing 1<<n, which is not only a
pattern that I recognize as builtin to my brain, but also provides a
direct description of what's happening, "shift a 1 over n times",
leaving no off-by-one ambiguity about it. If anything I'd like to see
the BIT() macro expanded throughout and then removed entirely.
Probably just me though. You can safely ignore my opinion :).
Jason
>
> Signed-off-by: Gaosheng Cui <[email protected]>
> ---
> v2:
> - Change the commit msg, remove the UBSAN warning calltrace, and
> merge patch "x86/cpu: fix undefined behavior in bit shift for
> intel_detect_tlb" with it. Thanks!
> arch/x86/kernel/cpu/amd.c | 2 +-
> arch/x86/kernel/cpu/centaur.c | 2 +-
> arch/x86/kernel/cpu/hygon.c | 2 +-
> arch/x86/kernel/cpu/intel.c | 4 ++--
> arch/x86/kernel/cpu/proc.c | 2 +-
> arch/x86/kernel/cpu/zhaoxin.c | 2 +-
> 6 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 860b60273df3..75d82cad323a 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -613,7 +613,7 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> * c->x86_power is 8000_0007 edx. Bit 8 is TSC runs at constant rate
> * with P/T states and does not stop in deep C-states
> */
> - if (c->x86_power & (1 << 8)) {
> + if (c->x86_power & BIT(8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> }
> diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
> index 345f7d905db6..9910bb1d90fd 100644
> --- a/arch/x86/kernel/cpu/centaur.c
> +++ b/arch/x86/kernel/cpu/centaur.c
> @@ -105,7 +105,7 @@ static void early_init_centaur(struct cpuinfo_x86 *c)
> #ifdef CONFIG_X86_64
> set_cpu_cap(c, X86_FEATURE_SYSENTER32);
> #endif
> - if (c->x86_power & (1 << 8)) {
> + if (c->x86_power & BIT(8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> }
> diff --git a/arch/x86/kernel/cpu/hygon.c b/arch/x86/kernel/cpu/hygon.c
> index 21fd425088fe..dc473bfbf1b5 100644
> --- a/arch/x86/kernel/cpu/hygon.c
> x86/cpu: fix undefined behavior in bit shift for intel_detect_tlb+++ b/arch/x86/kernel/cpu/hygon.c
> @@ -251,7 +251,7 @@ static void early_init_hygon(struct cpuinfo_x86 *c)
> * c->x86_power is 8000_0007 edx. Bit 8 is TSC runs at constant rate
> * with P/T states and does not stop in deep C-states
> */
> - if (c->x86_power & (1 << 8)) {
> + if (c->x86_power & BIT(8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> }
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 2d7ea5480ec3..2bdf6d601a6f 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -286,7 +286,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> * It is also reliable across cores and sockets. (but not across
> * cabinets - we turn it off in that case explicitly.)
> */
> - if (c->x86_power & (1 << 8)) {
> + if (c->x86_power & BIT(8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> }
> @@ -945,7 +945,7 @@ static void intel_detect_tlb(struct cpuinfo_x86 *c)
>
> /* If bit 31 is set, this is an unknown format */
> for (j = 0 ; j < 3 ; j++)
> - if (regs[j] & (1 << 31))
> + if (regs[j] & BIT(31))
> regs[j] = 0;
>
> /* Byte 0 is level count, not a descriptor */
> diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> index 099b6f0d96bd..efa1d39c4f25 100644
> --- a/arch/x86/kernel/cpu/proc.c
> +++ b/arch/x86/kernel/cpu/proc.c
> @@ -135,7 +135,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>
> seq_puts(m, "power management:");
> for (i = 0; i < 32; i++) {
> - if (c->x86_power & (1 << i)) {
> + if (c->x86_power & BIT(i)) {
> if (i < ARRAY_SIZE(x86_power_flags) &&
> x86_power_flags[i])
> seq_printf(m, "%s%s",
> diff --git a/arch/x86/kernel/cpu/zhaoxin.c b/arch/x86/kernel/cpu/zhaoxin.c
> index 05fa4ef63490..34a8a460f8f4 100644
> --- a/arch/x86/kernel/cpu/zhaoxin.c
> +++ b/arch/x86/kernel/cpu/zhaoxin.c
> @@ -61,7 +61,7 @@ static void early_init_zhaoxin(struct cpuinfo_x86 *c)
> #ifdef CONFIG_X86_64
> set_cpu_cap(c, X86_FEATURE_SYSENTER32);
> #endif
> - if (c->x86_power & (1 << 8)) {
> + if (c->x86_power & BIT(8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> }
> --
> 2.25.1
>
> Others might feel differently and that's fine, but I always found the
> BIT() thing so much less clear than doing 1<<n, which is not only a
> pattern that I recognize as builtin to my brain, but also provides a
> direct description of what's happening, "shift a 1 over n times",
> leaving no off-by-one ambiguity about it. If anything I'd like to see
> the BIT() macro expanded throughout and then removed entirely.
>
> Probably just me though. You can safely ignore my opinion.
Thanks for taking time to review the patch, I submit the patch to remove
the UBSAN warning, even it's not a bug, for example, when I am testing the
kernel, I get some logs as follows, maybe it's better to avoid this?
> [ 0.951719][ T0]
> ================================================================================
> 215 [ 0.953146][ T0] UBSAN: shift-out-of-bounds in mm/shmem.c:3749:18
> 216 [ 0.953863][ T0] left shift of 1 by 31 places cannot be
> represented in type 'int' 217 [ 0.955067][ T0] CPU: 0 PID: 0 Comm:
> swapper/0 Not tainted 6.1.0-rc2-00062-ga970174d7a10 #5 218 [
> 0.956400][ T0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014 219 [
> 0.958278][ T0] Call Trace: 220 [ 0.958777][ T0] <TASK> 221 [
> 0.959224][ T0] dump_stack_lvl+0x8d/0xcf 222 [ 0.959922][ T0]
> ubsan_epilogue+0xa/0x44 223 [ 0.960599][ T0]
> __ubsan_handle_shift_out_of_bounds+0x1e7/0x208 224 [ 0.961575][ T0] ?
> __kmem_cache_alloc_node+0x167/0x290 225 [ 0.962434][ T0] ?
> shmem_fill_super+0x2e/0x2e0 226 [ 0.963187][ T0] ?
> rcu_read_lock_held_common+0x9/0x40 227 [ 0.963857][ T0] ?
> shmem_alloc_hugefolio+0x110/0x110 228 [ 0.963857][ T0] ?
> shmem_fill_super+0x2cc/0x2e0 229 [ 0.963857][ T0]
> shmem_fill_super+0x2cc/0x2e0 230 [ 0.963857][ T0]
> vfs_get_super+0x78/0x160 231 [ 0.963857][ T0] vfs_get_tree+0x28/0x100
> 232 [ 0.963857][ T0] fc_mount+0x12/0x60 233 [ 0.963857][ T0]
> vfs_kern_mount.part.38+0xa5/0xc0 234 [ 0.963857][ T0]
> kern_mount+0x2e/0x60 235 [ 0.963857][ T0] shmem_init+0x63/0xef 236 [
> 0.963857][ T0] mnt_init+0x159/0x2e0 237 [ 0.963857][ T0] ?
> trace_init_perf_perm_irq_work_exit+0xe/0xe 238 [ 0.963857][ T0]
> vfs_caches_init+0xd4/0xde 239 [ 0.963857][ T0]
> start_kernel+0x837/0x8a4 240 [ 0.963857][ T0]
> secondary_startup_64_no_verify+0xce/0xdb 241 [ 0.963857][ T0] </TASK>
> 242 [ 0.963860][ T0]
> ================================================================================
> 243 [ 0.965288][ T0] Kernel panic - not syncing: panic_on_warn set ...
> 244 [ 0.966299][ T0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> 6.1.0-rc2-00062-ga970174d7a10 #5 245 [ 0.967645][ T0] Hardware name:
> QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014 246 [
> 0.969548][ T0] Call Trace: 247 [ 0.970050][ T0] <TASK> 248 [
> 0.970499][ T0] dump_stack_lvl+0x8d/0xcf 249 [ 0.971195][ T0]
> panic+0x182/0x387 250 [ 0.971797][ T0] ? ubsan_epilogue+0x33/0x44 251
> [ 0.972539][ T0] ubsan_epilogue+0x3f/0x44 252 [ 0.973237][ T0]
> __ubsan_handle_shift_out_of_bounds+0x1e7/0x208 253 [ 0.973857][ T0] ?
> __kmem_cache_alloc_node+0x167/0x290 254 [ 0.973857][ T0] ?
> shmem_fill_super+0x2e/0x2e0 255 [ 0.973857][ T0] ?
> rcu_read_lock_held_common+0x9/0x40 256 [ 0.973857][ T0] ?
> shmem_alloc_hugefolio+0x110/0x110 257 [ 0.973857][ T0] ?
> shmem_fill_super+0x2cc/0x2e0 258 [ 0.973857][ T0]
> shmem_fill_super+0x2cc/0x2e0 259 [ 0.973857][ T0]
> vfs_get_super+0x78/0x160 260 [ 0.973857][ T0] vfs_get_tree+0x28/0x100
> 261 [ 0.973857][ T0] fc_mount+0x12/0x60 262 [ 0.973857][ T0]
> vfs_kern_mount.part.38+0xa5/0xc0 263 [ 0.973857][ T0]
> kern_mount+0x2e/0x60 264 [ 0.973857][ T0] shmem_init+0x63/0xef 265 [
> 0.973857][ T0] mnt_init+0x159/0x2e0 266 [ 0.973857][ T0] ?
> trace_init_perf_perm_irq_work_exit+0xe/0xe 267 [ 0.973857][ T0]
> vfs_caches_init+0xd4/0xde 268 [ 0.973857][ T0]
> start_kernel+0x837/0x8a4 269 [ 0.973857][ T0]
> secondary_startup_64_no_verify+0xce/0xdb 270 [ 0.973857][ T0] </TASK>
> 271 [ 0.973857][ T0] Rebooting in 86400 seconds..
On 2022/11/1 19:34, Jason A. Donenfeld wrote:
> Others might feel differently and that's fine, but I always found the
> BIT() thing so much less clear than doing 1<<n, which is not only a
> pattern that I recognize as builtin to my brain, but also provides a
> direct description of what's happening, "shift a 1 over n times",
> leaving no off-by-one ambiguity about it. If anything I'd like to see
> the BIT() macro expanded throughout and then removed entirely.
>
> Probably just me though. You can safely ignore my opinion:).
On Tue, Nov 1, 2022 at 2:37 PM cuigaosheng <[email protected]> wrote:
> > 215 [ 0.953146][ T0] UBSAN: shift-out-of-bounds in mm/shmem.c:3749:18
> > 216 [ 0.953863][ T0] left shift of 1 by 31 places cannot be represented in type 'int'
Isn't this just an issue with `1 << 31` needing to be `1U << 31`?
Jason
Yes, maybe we can do the same thing with BIT(), so I add other modifications, should I remove them?
Thanks very much!
On 2022/11/1 21:43, Jason A. Donenfeld wrote:
> On Tue, Nov 1, 2022 at 2:37 PM cuigaosheng <[email protected]> wrote:
>>> 215 [ 0.953146][ T0] UBSAN: shift-out-of-bounds in mm/shmem.c:3749:18
>>> 216 [ 0.953863][ T0] left shift of 1 by 31 places cannot be represented in type 'int'
> Isn't this just an issue with `1 << 31` needing to be `1U << 31`?
>
> Jason
> .