These are some minor changes to enable the __noreturn attribute for
arch_cpu_idle_dead(). (If there are no objections, I can merge the
entire set through the tip tree.)
Until recently [1], in Xen, when a previously offlined CPU was brought
back online, it unexpectedly resumed execution where it left off in the
middle of the idle loop by returning from play_dead() and its caller
arch_cpu_idle_dead().
There were some clever hacks to make that work, but the behavior was
surprising as do_idle() doesn't expect an offlined CPU to return from
the dead in the middle of the function.
Now that Xen has been fixed, make sure arch_cpu_idle_dead() never
returns by marking it __noreturn. This causes the compiler to complain
if an arch-specific implementation might return. It also improves code
generation for both caller and callee.
Patches 1-20 update the arch-specific implementations of the function so
they don't actually return (even in error cases), and make that visible
to the compiler. That's typically done by ending the function with a
call to another noreturn function, or with a BUG().
Patch 21 fixes the weak implementation.
Patch 22 actually adds the __noreturn attribute.
[1] 076cbf5d2163 ("x86/xen: don't let xen_pv_play_dead() return")
Josh Poimboeuf (22):
alpha/cpu: Expose arch_cpu_idle_dead()'s prototype declaration
alpha/cpu: Make sure arch_cpu_idle_dead() doesn't return
arm/cpu: Make sure arch_cpu_idle_dead() doesn't return
arm64/cpu: Mark cpu_die() __noreturn
csky/cpu: Make sure arch_cpu_idle_dead() doesn't return
ia64/cpu: Mark play_dead() __noreturn
loongarch/cpu: Make sure play_dead() doesn't return
loongarch/cpu: Mark play_dead() __noreturn
mips/cpu: Expose play_dead()'s prototype definition
mips/cpu: Make sure play_dead() doesn't return
mips/cpu: Mark play_dead() __noreturn
powerpc/cpu: Mark start_secondary_resume() __noreturn
sh/cpu: Make sure play_dead() doesn't return
sh/cpu: Mark play_dead() __noreturn
sh/cpu: Expose arch_cpu_idle_dead()'s prototype definition
sparc/cpu: Mark cpu_play_dead() __noreturn
x86/cpu: Make sure play_dead() doesn't return
x86/cpu: Mark play_dead() __noreturn
xtensa/cpu: Make sure cpu_die() doesn't return
xtensa/cpu: Mark cpu_die() __noreturn
sched/idle: Make sure weak version of arch_cpu_idle_dead() doesn't
return
sched/idle: Mark arch_cpu_idle_dead() __noreturn
arch/alpha/kernel/process.c | 2 ++
arch/arm/kernel/smp.c | 2 ++
arch/arm64/include/asm/smp.h | 2 +-
arch/csky/kernel/smp.c | 2 ++
arch/ia64/kernel/process.c | 4 ++--
arch/loongarch/include/asm/smp.h | 2 +-
arch/loongarch/kernel/smp.c | 2 +-
arch/mips/include/asm/smp.h | 2 +-
arch/mips/kernel/smp-bmips.c | 3 +++
arch/mips/loongson64/smp.c | 1 +
arch/powerpc/include/asm/smp.h | 2 +-
arch/sh/include/asm/smp-ops.h | 5 +++--
arch/sh/kernel/idle.c | 1 +
arch/sparc/include/asm/smp_64.h | 2 +-
arch/x86/include/asm/smp.h | 3 ++-
arch/x86/kernel/process.c | 2 +-
arch/xtensa/include/asm/smp.h | 2 +-
arch/xtensa/kernel/smp.c | 2 ++
include/linux/cpu.h | 2 +-
kernel/sched/idle.c | 2 +-
tools/objtool/check.c | 1 +
21 files changed, 31 insertions(+), 15 deletions(-)
--
2.39.0
Include <linux/cpu.h> to make sure arch_cpu_idle_dead() matches its
prototype going forward.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/alpha/kernel/process.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/alpha/kernel/process.c b/arch/alpha/kernel/process.c
index 0eddd22c6212..4813172547b5 100644
--- a/arch/alpha/kernel/process.c
+++ b/arch/alpha/kernel/process.c
@@ -9,6 +9,7 @@
* This file handles the architecture-dependent parts of process handling.
*/
+#include <linux/cpu.h>
#include <linux/errno.h>
#include <linux/module.h>
#include <linux/sched.h>
--
2.39.0
arch_cpu_idle_dead() doesn't return. Make that more explicit with a
BUG().
BUG() is preferable to unreachable() because BUG() is a more explicit
failure mode and avoids undefined behavior like falling off the edge of
the function into whatever code happens to be next.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/arm/kernel/smp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 0b8c25763adc..adcd417c526b 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -382,6 +382,8 @@ void arch_cpu_idle_dead(void)
: "r" (task_stack_page(current) + THREAD_SIZE - 8),
"r" (current)
: "r0");
+
+ BUG();
}
#endif /* CONFIG_HOTPLUG_CPU */
--
2.39.0
arch_cpu_idle_dead() doesn't return. Make that more explicit with a
BUG().
BUG() is preferable to unreachable() because BUG() is a more explicit
failure mode and avoids undefined behavior like falling off the edge of
the function into whatever code happens to be next.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/alpha/kernel/process.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/alpha/kernel/process.c b/arch/alpha/kernel/process.c
index 4813172547b5..94938f856545 100644
--- a/arch/alpha/kernel/process.c
+++ b/arch/alpha/kernel/process.c
@@ -63,6 +63,7 @@ void arch_cpu_idle(void)
void arch_cpu_idle_dead(void)
{
wtint(INT_MAX);
+ BUG();
}
#endif /* ALPHA_WTINT */
--
2.39.0
cpu_die() doesn't return. Annotate it as such. By extension this also
makes arch_cpu_idle_dead() noreturn.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/arm64/include/asm/smp.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index fc55f5a57a06..5733a31bab08 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -100,7 +100,7 @@ static inline void arch_send_wakeup_ipi_mask(const struct cpumask *mask)
extern int __cpu_disable(void);
extern void __cpu_die(unsigned int cpu);
-extern void cpu_die(void);
+extern void __noreturn cpu_die(void);
extern void cpu_die_early(void);
static inline void cpu_park_loop(void)
--
2.39.0
arch_cpu_idle_dead() doesn't return. Make that more explicit with a
BUG().
BUG() is preferable to unreachable() because BUG() is a more explicit
failure mode and avoids undefined behavior like falling off the edge of
the function into whatever code happens to be next.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/csky/kernel/smp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
index b45d1073307f..0ec20efaf5fd 100644
--- a/arch/csky/kernel/smp.c
+++ b/arch/csky/kernel/smp.c
@@ -317,5 +317,7 @@ void arch_cpu_idle_dead(void)
"jmpi csky_start_secondary"
:
: "r" (secondary_stack));
+
+ BUG();
}
#endif
--
2.39.0
play_dead() doesn't return. Annotate it as such. By extension this
also makes arch_cpu_idle_dead() noreturn.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/ia64/kernel/process.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index f6195a0a00ae..78f5794b2dde 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -201,7 +201,7 @@ __setup("nohalt", nohalt_setup);
#ifdef CONFIG_HOTPLUG_CPU
/* We don't actually take CPU down, just spin without interrupts. */
-static inline void play_dead(void)
+static inline void __noreturn play_dead(void)
{
unsigned int this_cpu = smp_processor_id();
@@ -219,7 +219,7 @@ static inline void play_dead(void)
BUG();
}
#else
-static inline void play_dead(void)
+static inline void __noreturn play_dead(void)
{
BUG();
}
--
2.39.0
play_dead() doesn't return. Annotate it as such. By extension this
also makes arch_cpu_idle_dead() noreturn.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/loongarch/include/asm/smp.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/loongarch/include/asm/smp.h b/arch/loongarch/include/asm/smp.h
index d82687390b4a..416b653bccb4 100644
--- a/arch/loongarch/include/asm/smp.h
+++ b/arch/loongarch/include/asm/smp.h
@@ -99,7 +99,7 @@ static inline void __cpu_die(unsigned int cpu)
loongson_cpu_die(cpu);
}
-extern void play_dead(void);
+extern void __noreturn play_dead(void);
#endif
#endif /* __ASM_SMP_H */
--
2.39.0
play_dead() doesn't return. Make that more explicit with a BUG().
BUG() is preferable to unreachable() because BUG() is a more explicit
failure mode and avoids undefined behavior like falling off the edge of
the function into whatever code happens to be next.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/loongarch/kernel/smp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
index 8c6e227cb29d..51f328169a7b 100644
--- a/arch/loongarch/kernel/smp.c
+++ b/arch/loongarch/kernel/smp.c
@@ -336,7 +336,7 @@ void play_dead(void)
iocsr_write32(0xffffffff, LOONGARCH_IOCSR_IPI_CLEAR);
init_fn();
- unreachable();
+ BUG();
}
#endif
--
2.39.0
play_dead() doesn't return. Make that more explicit with a BUG().
BUG() is preferable to unreachable() because BUG() is a more explicit
failure mode and avoids undefined behavior like falling off the edge of
the function into whatever code happens to be next.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/mips/kernel/smp-bmips.c | 2 ++
arch/mips/loongson64/smp.c | 1 +
2 files changed, 3 insertions(+)
diff --git a/arch/mips/kernel/smp-bmips.c b/arch/mips/kernel/smp-bmips.c
index df9158e8329d..be85fa075830 100644
--- a/arch/mips/kernel/smp-bmips.c
+++ b/arch/mips/kernel/smp-bmips.c
@@ -414,6 +414,8 @@ void __ref play_dead(void)
" wait\n"
" j bmips_secondary_reentry\n"
: : : "memory");
+
+ BUG();
}
#endif /* CONFIG_HOTPLUG_CPU */
diff --git a/arch/mips/loongson64/smp.c b/arch/mips/loongson64/smp.c
index 660e1de4412a..c81c2bd07c62 100644
--- a/arch/mips/loongson64/smp.c
+++ b/arch/mips/loongson64/smp.c
@@ -822,6 +822,7 @@ void play_dead(void)
state_addr = &per_cpu(cpu_state, cpu);
mb();
play_dead_at_ckseg1(state_addr);
+ BUG();
}
static int loongson3_disable_clock(unsigned int cpu)
--
2.39.0
Include <asm/smp.h> to make sure play_dead() matches its prototype going
forward.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/mips/kernel/smp-bmips.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/mips/kernel/smp-bmips.c b/arch/mips/kernel/smp-bmips.c
index f5d7bfa3472a..df9158e8329d 100644
--- a/arch/mips/kernel/smp-bmips.c
+++ b/arch/mips/kernel/smp-bmips.c
@@ -38,6 +38,7 @@
#include <asm/traps.h>
#include <asm/barrier.h>
#include <asm/cpu-features.h>
+#include <asm/smp.h>
static int __maybe_unused max_cpus = 1;
--
2.39.0
play_dead() doesn't return. Annotate it as such. By extension this
also makes arch_cpu_idle_dead() noreturn.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/mips/include/asm/smp.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/include/asm/smp.h b/arch/mips/include/asm/smp.h
index 5d9ff61004ca..4eee29b7845c 100644
--- a/arch/mips/include/asm/smp.h
+++ b/arch/mips/include/asm/smp.h
@@ -88,7 +88,7 @@ static inline void __cpu_die(unsigned int cpu)
mp_ops->cpu_die(cpu);
}
-extern void play_dead(void);
+extern void __noreturn play_dead(void);
#endif
#ifdef CONFIG_KEXEC
--
2.39.0
start_secondary_resume() doesn't return. Annotate it as such. By
extension this also makes arch_cpu_idle_dead() noreturn.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/powerpc/include/asm/smp.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index f63505d74932..cfd42ca8765c 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -66,7 +66,7 @@ void start_secondary(void *unused);
extern int smp_send_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us);
extern int smp_send_safe_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us);
extern void smp_send_debugger_break(void);
-extern void start_secondary_resume(void);
+extern void __noreturn start_secondary_resume(void);
extern void smp_generic_give_timebase(void);
extern void smp_generic_take_timebase(void);
--
2.39.0
play_dead() doesn't return. Make that more explicit with a BUG().
BUG() is preferable to unreachable() because BUG() is a more explicit
failure mode and avoids undefined behavior like falling off the edge of
the function into whatever code happens to be next.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/sh/include/asm/smp-ops.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/sh/include/asm/smp-ops.h b/arch/sh/include/asm/smp-ops.h
index e27702130eb6..63866b1595a0 100644
--- a/arch/sh/include/asm/smp-ops.h
+++ b/arch/sh/include/asm/smp-ops.h
@@ -27,6 +27,7 @@ static inline void plat_smp_setup(void)
static inline void play_dead(void)
{
mp_ops->play_dead();
+ BUG();
}
extern void register_smp_ops(struct plat_smp_ops *ops);
--
2.39.0
Include <linux/cpu.h> to make sure arch_cpu_idle_dead() matches its
prototype going forward.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/sh/kernel/idle.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/sh/kernel/idle.c b/arch/sh/kernel/idle.c
index 3418c40f0099..114f0c4abeac 100644
--- a/arch/sh/kernel/idle.c
+++ b/arch/sh/kernel/idle.c
@@ -4,6 +4,7 @@
*
* Copyright (C) 2002 - 2009 Paul Mundt
*/
+#include <linux/cpu.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/mm.h>
--
2.39.0
play_dead() doesn't return. Annotate it as such. By extension this
also makes arch_cpu_idle_dead() noreturn.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/sh/include/asm/smp-ops.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/sh/include/asm/smp-ops.h b/arch/sh/include/asm/smp-ops.h
index 63866b1595a0..97331fcb7b85 100644
--- a/arch/sh/include/asm/smp-ops.h
+++ b/arch/sh/include/asm/smp-ops.h
@@ -24,7 +24,7 @@ static inline void plat_smp_setup(void)
mp_ops->smp_setup();
}
-static inline void play_dead(void)
+static inline void __noreturn play_dead(void)
{
mp_ops->play_dead();
BUG();
@@ -43,7 +43,7 @@ static inline void register_smp_ops(struct plat_smp_ops *ops)
{
}
-static inline void play_dead(void)
+static inline void __noreturn play_dead(void)
{
BUG();
}
--
2.39.0
cpu_play_dead() doesn't return. Annotate it as such. By extension this
also makes arch_cpu_idle_dead() noreturn.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/sparc/include/asm/smp_64.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/sparc/include/asm/smp_64.h b/arch/sparc/include/asm/smp_64.h
index e75783b6abc4..505b6700805d 100644
--- a/arch/sparc/include/asm/smp_64.h
+++ b/arch/sparc/include/asm/smp_64.h
@@ -49,7 +49,7 @@ int hard_smp_processor_id(void);
void smp_fill_in_cpu_possible_map(void);
void smp_fill_in_sib_core_maps(void);
-void cpu_play_dead(void);
+void __noreturn cpu_play_dead(void);
void smp_fetch_global_regs(void);
void smp_fetch_global_pmu(void);
--
2.39.0
After commit 076cbf5d2163 ("x86/xen: don't let xen_pv_play_dead()
return"), play_dead() never returns. Make that more explicit with a
BUG().
BUG() is preferable to unreachable() because BUG() is a more explicit
failure mode and avoids undefined behavior like falling off the edge of
the function into whatever code happens to be next.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/include/asm/smp.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index b4dbb20dab1a..8f628e08b25a 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -96,6 +96,7 @@ static inline void __cpu_die(unsigned int cpu)
static inline void play_dead(void)
{
smp_ops.play_dead();
+ BUG();
}
static inline void smp_send_reschedule(int cpu)
--
2.39.0
cpu_die() doesn't return. Make that more explicit with a BUG().
BUG() is preferable to unreachable() because BUG() is a more explicit
failure mode and avoids undefined behavior like falling off the edge of
the function into whatever code happens to be next.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/xtensa/kernel/smp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/xtensa/kernel/smp.c b/arch/xtensa/kernel/smp.c
index 4dc109dd6214..7bad78495536 100644
--- a/arch/xtensa/kernel/smp.c
+++ b/arch/xtensa/kernel/smp.c
@@ -341,6 +341,8 @@ void __ref cpu_die(void)
__asm__ __volatile__(
" movi a2, cpu_restart\n"
" jx a2\n");
+
+ BUG();
}
#endif /* CONFIG_HOTPLUG_CPU */
--
2.39.0
cpu_die() doesn't return. Annotate it as such. By extension this also
makes arch_cpu_idle_dead() noreturn.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/xtensa/include/asm/smp.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/xtensa/include/asm/smp.h b/arch/xtensa/include/asm/smp.h
index 4e43f5643891..5dc5bf8cdd77 100644
--- a/arch/xtensa/include/asm/smp.h
+++ b/arch/xtensa/include/asm/smp.h
@@ -33,7 +33,7 @@ void show_ipi_list(struct seq_file *p, int prec);
void __cpu_die(unsigned int cpu);
int __cpu_disable(void);
-void cpu_die(void);
+void __noreturn cpu_die(void);
void cpu_restart(void);
#endif /* CONFIG_HOTPLUG_CPU */
--
2.39.0
arch_cpu_idle_dead() should never return. Make it so.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
kernel/sched/idle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index e9ef66be2870..56e152f06d0f 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -75,7 +75,7 @@ static noinline int __cpuidle cpu_idle_poll(void)
void __weak arch_cpu_idle_prepare(void) { }
void __weak arch_cpu_idle_enter(void) { }
void __weak arch_cpu_idle_exit(void) { }
-void __weak arch_cpu_idle_dead(void) { }
+void __weak arch_cpu_idle_dead(void) { while (1); }
void __weak arch_cpu_idle(void)
{
cpu_idle_force_poll = 1;
--
2.39.0
play_dead() doesn't return. Annotate it as such. By extension this
also makes arch_cpu_idle_dead() noreturn.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/include/asm/smp.h | 2 +-
arch/x86/kernel/process.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 8f628e08b25a..e6d1d2810e38 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -93,7 +93,7 @@ static inline void __cpu_die(unsigned int cpu)
smp_ops.cpu_die(cpu);
}
-static inline void play_dead(void)
+static inline void __noreturn play_dead(void)
{
smp_ops.play_dead();
BUG();
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index b650cde3f64d..f1ec36caf1d8 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -715,7 +715,7 @@ static bool x86_idle_set(void)
}
#ifndef CONFIG_SMP
-static inline void play_dead(void)
+static inline void __noreturn play_dead(void)
{
BUG();
}
--
2.39.0
Before commit 076cbf5d2163 ("x86/xen: don't let xen_pv_play_dead()
return"), in Xen, when a previously offlined CPU was brought back
online, it unexpectedly resumed execution where it left off in the
middle of the idle loop.
There were some hacks to make that work, but the behavior was surprising
as do_idle() doesn't expect an offlined CPU to return from the dead (in
arch_cpu_idle_dead()).
Now that Xen has been fixed, and the arch-specific implementations of
arch_cpu_idle_dead() also don't return, give it a __noreturn attribute.
This will cause the compiler to complain if an arch-specific
implementation might return. It also improves code generation for both
caller and callee.
Also fixes the following warning:
vmlinux.o: warning: objtool: do_idle+0x25f: unreachable instruction
Reported-by: Paul E. McKenney <[email protected]>
Tested-by: Paul E. McKenney <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
include/linux/cpu.h | 2 +-
tools/objtool/check.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index f83e4519c5f0..8582a7142623 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -182,7 +182,7 @@ void arch_cpu_idle(void);
void arch_cpu_idle_prepare(void);
void arch_cpu_idle_enter(void);
void arch_cpu_idle_exit(void);
-void arch_cpu_idle_dead(void);
+void __noreturn arch_cpu_idle_dead(void);
int cpu_report_state(int cpu);
int cpu_check_up_prepare(int cpu);
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0f67c6a8bc98..e3fa2279d612 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -167,6 +167,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
"__reiserfs_panic",
"__stack_chk_fail",
"__ubsan_handle_builtin_unreachable",
+ "arch_cpu_idle_dead",
"cpu_bringup_and_idle",
"cpu_startup_entry",
"do_exit",
--
2.39.0
On Sat, Feb 4, 2023 at 6:05 AM Josh Poimboeuf <[email protected]> wrote:
>
> arch_cpu_idle_dead() doesn't return. Make that more explicit with a
> BUG().
>
> BUG() is preferable to unreachable() because BUG() is a more explicit
> failure mode and avoids undefined behavior like falling off the edge of
> the function into whatever code happens to be next.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> arch/csky/kernel/smp.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
> index b45d1073307f..0ec20efaf5fd 100644
> --- a/arch/csky/kernel/smp.c
> +++ b/arch/csky/kernel/smp.c
> @@ -317,5 +317,7 @@ void arch_cpu_idle_dead(void)
> "jmpi csky_start_secondary"
> :
> : "r" (secondary_stack));
> +
> + BUG();
Why not:
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index f26ab2675f7d..1d3bf903add2 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -285,6 +285,7 @@ static void do_idle(void)
tick_nohz_idle_stop_tick();
cpuhp_report_idle_dead();
arch_cpu_idle_dead();
+ BUG();
}
arch_cpu_idle_enter();
> }
> #endif
> --
> 2.39.0
>
--
Best Regards
Guo Ren
On Sat, Feb 04, 2023 at 09:12:31AM +0800, Guo Ren wrote:
> On Sat, Feb 4, 2023 at 6:05 AM Josh Poimboeuf <[email protected]> wrote:
> >
> > arch_cpu_idle_dead() doesn't return. Make that more explicit with a
> > BUG().
> >
> > BUG() is preferable to unreachable() because BUG() is a more explicit
> > failure mode and avoids undefined behavior like falling off the edge of
> > the function into whatever code happens to be next.
> >
> > Signed-off-by: Josh Poimboeuf <[email protected]>
> > ---
> > arch/csky/kernel/smp.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
> > index b45d1073307f..0ec20efaf5fd 100644
> > --- a/arch/csky/kernel/smp.c
> > +++ b/arch/csky/kernel/smp.c
> > @@ -317,5 +317,7 @@ void arch_cpu_idle_dead(void)
> > "jmpi csky_start_secondary"
> > :
> > : "r" (secondary_stack));
> > +
> > + BUG();
> Why not:
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index f26ab2675f7d..1d3bf903add2 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -285,6 +285,7 @@ static void do_idle(void)
> tick_nohz_idle_stop_tick();
> cpuhp_report_idle_dead();
> arch_cpu_idle_dead();
> + BUG();
Without the BUG() in csky arch_cpu_idle_dead(), the compiler will warn
about arch_cpu_idle_dead() returning, because it's marked __noreturn but
doesn't clearly return (as far as the compiler knows).
And we want it marked __noreturn so we'll be more likely to catch such
bugs at build time.
And as a bonus we get better code generation and clearer code semantics
which helps both humans and tooling understand the intent of the code.
--
Josh
On Sat, Feb 4, 2023 at 10:29 AM Josh Poimboeuf <[email protected]> wrote:
>
> On Sat, Feb 04, 2023 at 09:12:31AM +0800, Guo Ren wrote:
> > On Sat, Feb 4, 2023 at 6:05 AM Josh Poimboeuf <[email protected]> wrote:
> > >
> > > arch_cpu_idle_dead() doesn't return. Make that more explicit with a
> > > BUG().
> > >
> > > BUG() is preferable to unreachable() because BUG() is a more explicit
> > > failure mode and avoids undefined behavior like falling off the edge of
> > > the function into whatever code happens to be next.
> > >
> > > Signed-off-by: Josh Poimboeuf <[email protected]>
> > > ---
> > > arch/csky/kernel/smp.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
> > > index b45d1073307f..0ec20efaf5fd 100644
> > > --- a/arch/csky/kernel/smp.c
> > > +++ b/arch/csky/kernel/smp.c
> > > @@ -317,5 +317,7 @@ void arch_cpu_idle_dead(void)
> > > "jmpi csky_start_secondary"
> > > :
> > > : "r" (secondary_stack));
> > > +
> > > + BUG();
> > Why not:
> > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> > index f26ab2675f7d..1d3bf903add2 100644
> > --- a/kernel/sched/idle.c
> > +++ b/kernel/sched/idle.c
> > @@ -285,6 +285,7 @@ static void do_idle(void)
> > tick_nohz_idle_stop_tick();
> > cpuhp_report_idle_dead();
> > arch_cpu_idle_dead();
> > + BUG();
>
> Without the BUG() in csky arch_cpu_idle_dead(), the compiler will warn
> about arch_cpu_idle_dead() returning, because it's marked __noreturn but
> doesn't clearly return (as far as the compiler knows).
>
> And we want it marked __noreturn so we'll be more likely to catch such
> bugs at build time.
>
> And as a bonus we get better code generation and clearer code semantics
> which helps both humans and tooling understand the intent of the code.
Thx for the clarification.
Acked-by: Guo Ren <[email protected]>
>
> --
> Josh
--
Best Regards
Guo Ren
Josh Poimboeuf <[email protected]> writes:
> start_secondary_resume() doesn't return. Annotate it as such. By
> extension this also makes arch_cpu_idle_dead() noreturn.
Can we also mark arch_cpu_idle_dead() (the C function) __noreturn ?
Seems like it would be good documentation, even if it's not required
once the generic prototype is __noreturn.
But not a show-stopper.
Acked-by: Michael Ellerman <[email protected]> (powerpc)
cheers
> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
> index f63505d74932..cfd42ca8765c 100644
> --- a/arch/powerpc/include/asm/smp.h
> +++ b/arch/powerpc/include/asm/smp.h
> @@ -66,7 +66,7 @@ void start_secondary(void *unused);
> extern int smp_send_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us);
> extern int smp_send_safe_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us);
> extern void smp_send_debugger_break(void);
> -extern void start_secondary_resume(void);
> +extern void __noreturn start_secondary_resume(void);
> extern void smp_generic_give_timebase(void);
> extern void smp_generic_take_timebase(void);
>
> --
> 2.39.0
On Mon, Feb 06, 2023 at 10:10:22PM +1100, Michael Ellerman wrote:
> Josh Poimboeuf <[email protected]> writes:
> > start_secondary_resume() doesn't return. Annotate it as such. By
> > extension this also makes arch_cpu_idle_dead() noreturn.
>
> Can we also mark arch_cpu_idle_dead() (the C function) __noreturn ?
>
> Seems like it would be good documentation, even if it's not required
> once the generic prototype is __noreturn.
Yeah, agreed. Maybe I'll do that (for all the implementations) with
patch 22.
> But not a show-stopper.
>
> Acked-by: Michael Ellerman <[email protected]> (powerpc)
Thanks!
--
Josh
On 2/3/23 14:05, Josh Poimboeuf wrote:
> Include <asm/smp.h> to make sure play_dead() matches its prototype going
> forward.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
--
Florian
On 2/3/23 14:05, Josh Poimboeuf wrote:
> play_dead() doesn't return. Make that more explicit with a BUG().
>
> BUG() is preferable to unreachable() because BUG() is a more explicit
> failure mode and avoids undefined behavior like falling off the edge of
> the function into whatever code happens to be next.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
--
Florian
On 2/3/23 14:05, Josh Poimboeuf wrote:
> play_dead() doesn't return. Annotate it as such. By extension this
> also makes arch_cpu_idle_dead() noreturn.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
--
Florian
Before commit 076cbf5d2163 ("x86/xen: don't let xen_pv_play_dead()
return"), in Xen, when a previously offlined CPU was brought back
online, it unexpectedly resumed execution where it left off in the
middle of the idle loop.
There were some hacks to make that work, but the behavior was surprising
as do_idle() doesn't expect an offlined CPU to return from the dead (in
arch_cpu_idle_dead()).
Now that Xen has been fixed, and the arch-specific implementations of
arch_cpu_idle_dead() also don't return, give it a __noreturn attribute.
This will cause the compiler to complain if an arch-specific
implementation might return. It also improves code generation for both
caller and callee.
Also fixes the following warning:
vmlinux.o: warning: objtool: do_idle+0x25f: unreachable instruction
Reported-by: Paul E. McKenney <[email protected]>
Tested-by: Paul E. McKenney <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
v1.1:
- add __noreturn to the implementations (in addition to just the
prototype)
arch/alpha/kernel/process.c | 2 +-
arch/arm/kernel/smp.c | 2 +-
arch/arm64/kernel/process.c | 2 +-
arch/csky/kernel/smp.c | 2 +-
arch/ia64/kernel/process.c | 2 +-
arch/loongarch/kernel/process.c | 2 +-
arch/mips/kernel/process.c | 2 +-
arch/parisc/kernel/process.c | 2 +-
arch/powerpc/kernel/smp.c | 2 +-
arch/riscv/kernel/cpu-hotplug.c | 2 +-
arch/s390/kernel/idle.c | 2 +-
arch/sh/kernel/idle.c | 2 +-
arch/sparc/kernel/process_64.c | 2 +-
arch/x86/kernel/process.c | 2 +-
arch/xtensa/kernel/smp.c | 2 +-
include/linux/cpu.h | 2 +-
tools/objtool/check.c | 1 +
17 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/arch/alpha/kernel/process.c b/arch/alpha/kernel/process.c
index 94938f856545..c9a5ed23c5a6 100644
--- a/arch/alpha/kernel/process.c
+++ b/arch/alpha/kernel/process.c
@@ -60,7 +60,7 @@ void arch_cpu_idle(void)
wtint(0);
}
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
wtint(INT_MAX);
BUG();
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index adcd417c526b..c2daa0f2f784 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -320,7 +320,7 @@ void __cpu_die(unsigned int cpu)
* of the other hotplug-cpu capable cores, so presumably coming
* out of idle fixes this.
*/
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
unsigned int cpu = smp_processor_id();
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 71d59b5abede..089ced6d6bd6 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -69,7 +69,7 @@ void (*pm_power_off)(void);
EXPORT_SYMBOL_GPL(pm_power_off);
#ifdef CONFIG_HOTPLUG_CPU
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
cpu_die();
}
diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
index 0ec20efaf5fd..9c7a20b73ac6 100644
--- a/arch/csky/kernel/smp.c
+++ b/arch/csky/kernel/smp.c
@@ -300,7 +300,7 @@ void __cpu_die(unsigned int cpu)
pr_notice("CPU%u: shutdown\n", cpu);
}
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
idle_task_exit();
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 78f5794b2dde..9a5cd9fad3a9 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -225,7 +225,7 @@ static inline void __noreturn play_dead(void)
}
#endif /* CONFIG_HOTPLUG_CPU */
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
play_dead();
}
diff --git a/arch/loongarch/kernel/process.c b/arch/loongarch/kernel/process.c
index edfd220a3737..ba70e94eb996 100644
--- a/arch/loongarch/kernel/process.c
+++ b/arch/loongarch/kernel/process.c
@@ -61,7 +61,7 @@ unsigned long boot_option_idle_override = IDLE_NO_OVERRIDE;
EXPORT_SYMBOL(boot_option_idle_override);
#ifdef CONFIG_HOTPLUG_CPU
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
play_dead();
}
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 093dbbd6b843..a3225912c862 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -40,7 +40,7 @@
#include <asm/stacktrace.h>
#ifdef CONFIG_HOTPLUG_CPU
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
play_dead();
}
diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
index c064719b49b0..97c6f875bd0e 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c
@@ -159,7 +159,7 @@ EXPORT_SYMBOL(running_on_qemu);
/*
* Called from the idle thread for the CPU which has been shutdown.
*/
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
#ifdef CONFIG_HOTPLUG_CPU
idle_task_exit();
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 6b90f10a6c81..f62e5e651bcd 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1752,7 +1752,7 @@ void __cpu_die(unsigned int cpu)
smp_ops->cpu_die(cpu);
}
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
/*
* Disable on the down path. This will be re-enabled by
diff --git a/arch/riscv/kernel/cpu-hotplug.c b/arch/riscv/kernel/cpu-hotplug.c
index f7a832e3a1d1..59b80211c25f 100644
--- a/arch/riscv/kernel/cpu-hotplug.c
+++ b/arch/riscv/kernel/cpu-hotplug.c
@@ -71,7 +71,7 @@ void __cpu_die(unsigned int cpu)
/*
* Called from the idle thread for the CPU which has been shutdown.
*/
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
idle_task_exit();
diff --git a/arch/s390/kernel/idle.c b/arch/s390/kernel/idle.c
index cb653c87018f..481ca32e628e 100644
--- a/arch/s390/kernel/idle.c
+++ b/arch/s390/kernel/idle.c
@@ -143,7 +143,7 @@ void arch_cpu_idle_exit(void)
{
}
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
cpu_die();
}
diff --git a/arch/sh/kernel/idle.c b/arch/sh/kernel/idle.c
index 114f0c4abeac..d662503b0665 100644
--- a/arch/sh/kernel/idle.c
+++ b/arch/sh/kernel/idle.c
@@ -30,7 +30,7 @@ void default_idle(void)
clear_bl_bit();
}
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
play_dead();
}
diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c
index 91c2b8124527..b51d8fb0ecdc 100644
--- a/arch/sparc/kernel/process_64.c
+++ b/arch/sparc/kernel/process_64.c
@@ -95,7 +95,7 @@ void arch_cpu_idle(void)
}
#ifdef CONFIG_HOTPLUG_CPU
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
sched_preempt_enable_no_resched();
cpu_play_dead();
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index f1ec36caf1d8..3e30147a537e 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -727,7 +727,7 @@ void arch_cpu_idle_enter(void)
local_touch_nmi();
}
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
play_dead();
}
diff --git a/arch/xtensa/kernel/smp.c b/arch/xtensa/kernel/smp.c
index 7bad78495536..054bd64eab19 100644
--- a/arch/xtensa/kernel/smp.c
+++ b/arch/xtensa/kernel/smp.c
@@ -322,7 +322,7 @@ void __cpu_die(unsigned int cpu)
pr_err("CPU%u: unable to kill\n", cpu);
}
-void arch_cpu_idle_dead(void)
+void __noreturn arch_cpu_idle_dead(void)
{
cpu_die();
}
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index f83e4519c5f0..8582a7142623 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -182,7 +182,7 @@ void arch_cpu_idle(void);
void arch_cpu_idle_prepare(void);
void arch_cpu_idle_enter(void);
void arch_cpu_idle_exit(void);
-void arch_cpu_idle_dead(void);
+void __noreturn arch_cpu_idle_dead(void);
int cpu_report_state(int cpu);
int cpu_check_up_prepare(int cpu);
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0f67c6a8bc98..e3fa2279d612 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -167,6 +167,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
"__reiserfs_panic",
"__stack_chk_fail",
"__ubsan_handle_builtin_unreachable",
+ "arch_cpu_idle_dead",
"cpu_bringup_and_idle",
"cpu_startup_entry",
"do_exit",
--
2.39.0