Messages in this thread Patch in this message | | | Date | Thu, 24 Nov 2022 08:47:47 +0100 | Subject | Re: objtool warning for next-20221118 | From | Juergen Gross <> |
| |
On 24.11.22 06:28, Juergen Gross wrote: > On 24.11.22 03:39, Josh Poimboeuf wrote: >> On Wed, Nov 23, 2022 at 09:03:40AM -0800, Josh Poimboeuf wrote: >>> On Wed, Nov 23, 2022 at 10:52:09AM +0000, Andrew Cooper wrote: >>>>> Well, if you return from arch_cpu_idle_dead() you're back in the idle >>>>> loop -- exactly where you would be if you were to bootstrap the whole >>>>> CPU -- provided you have it remember the whole state (easier with a >>>>> vCPU). >>> >>> play_dead() really needs sane semantics. Not only does it introduce a >>> surprise to the offlining code in do_idle(), it also skips the entire >>> hotplug state machine. Not sure if that introduces any bugs, but at the >>> very least it's subtle and surprising. >>> >>>>> But maybe I'm missing something, lets add Xen folks on. >>>> >>>> Calling VCPUOP_down on oneself always succeeds, but all it does is >>>> deschedule the vCPU. >>>> >>>> It can be undone at a later point by a different vcpu issuing VCPUOP_up >>>> against the previously-downed CPU, at which point the vCPU gets rescheduled. >>>> >>>> This is why the VCPUOP_down hypercall returns normally. All state >>>> really is intact. >>>> >>>> As for what Linux does, this is how xen_pv_cpu_up() currently behaves. >>>> If you want to make Xen behave more everything else, then bug a BUG() >>>> after VCPUOP_down, and adjust xen_pv_cpu_up() to skip its initialised >>>> check and always use VCPUOP_initialise to bring the vCPU back online. >>> >>> Or we could do what sev_es_play_dead() does and just call start_cpu0() >>> after the hypercall returns? >> >> Something like so (untested). This is only the x86 bits. >> >> I think I convinced myself that start_cpu0() isn't buggy. I'm looking >> at other cleanups, e.g. converging cpu_bringup_and_idle() with >> start_secondary(). >> >> I can pick it up again next week, post-turkey. >> >> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h >> index b4dbb20dab1a..e6d1d2810e38 100644 >> --- a/arch/x86/include/asm/smp.h >> +++ b/arch/x86/include/asm/smp.h >> @@ -93,9 +93,10 @@ 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(); >> } >> static inline void smp_send_reschedule(int cpu) >> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c >> index 26e8f57c75ad..8e2841deb1eb 100644 >> --- a/arch/x86/kernel/process.c >> +++ b/arch/x86/kernel/process.c >> @@ -700,7 +700,7 @@ EXPORT_SYMBOL(boot_option_idle_override); >> static void (*x86_idle)(void); >> #ifndef CONFIG_SMP >> -static inline void play_dead(void) >> +static inline void __noreturn play_dead(void) >> { >> BUG(); >> } >> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c >> index 55cad72715d9..d8b12ac1a7c5 100644 >> --- a/arch/x86/kernel/smpboot.c >> +++ b/arch/x86/kernel/smpboot.c >> @@ -1833,9 +1833,12 @@ void native_play_dead(void) >> play_dead_common(); >> tboot_shutdown(TB_SHUTDOWN_WFS); >> - mwait_play_dead(); /* Only returns on failure */ >> + mwait_play_dead(); /* Only returns if mwait is not supported */ >> + >> if (cpuidle_play_dead()) >> hlt_play_dead(); >> + >> + BUG(); >> } >> #else /* ... !CONFIG_HOTPLUG_CPU */ >> diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c >> index 480be82e9b7b..30dc904ca990 100644 >> --- a/arch/x86/xen/smp_pv.c >> +++ b/arch/x86/xen/smp_pv.c >> @@ -385,17 +385,9 @@ static void xen_pv_play_dead(void) /* used only with >> HOTPLUG_CPU */ >> { >> play_dead_common(); >> HYPERVISOR_vcpu_op(VCPUOP_down, xen_vcpu_nr(smp_processor_id()), NULL); >> - cpu_bringup(); >> - /* >> - * commit 4b0c0f294 (tick: Cleanup NOHZ per cpu data on cpu down) >> - * clears certain data that the cpu_idle loop (which called us >> - * and that we return from) expects. The only way to get that >> - * data back is to call: >> - */ >> - tick_nohz_idle_enter(); >> - tick_nohz_idle_stop_tick_protected(); >> - cpuhp_online_idle(CPUHP_AP_ONLINE_IDLE); >> + /* FIXME: converge cpu_bringup_and_idle() and start_secondary() */ >> + cpu_bringup_and_idle(); > > I think this will leak stack memory. Multiple cpu offline/online cycles of > the same cpu will finally exhaust the idle stack.
The attached patch seems to work fine.
The __noreturn annotation seems to trigger an objtool warning, though, in spite of the added BUG() at the end of xen_pv_play_dead():
arch/x86/xen/smp_pv.o: warning: objtool: xen_pv_play_dead() falls through to next function xen_pv_cpu_die()
Juergen
From 7ebb76ec0ed32b657ceda530b620ef563ac0f212 Mon Sep 17 00:00:00 2001 From: Juergen Gross <jgross@suse.com> Date: Thu, 24 Nov 2022 08:09:45 +0100 Subject: [PATCH] x86/xen: don't let xen_pv_play_dead() return
A function called via the paravirt play_dead() hook should not return to the caller.
xen_pv_play_dead() has a problem in this regard, as it currently will return in case an offlined cpu is brought to life again. This can be changed only by doing basically a longjmp() to cpu_bringup_and_idle(), as the hypercall for bringing down the cpu will just return when the cpu is coming up again. Just re-initializing the cpu isn't possible, as the Xen hypervisor will deny that operation.
So introduce xen_cpu_bringup_again() resetting the stack and calling cpu_bringup_and_idle(), which can be called after HYPERVISOR_vcpu_op() in xen_pv_play_dead().
Annotate xen_pv_play_dead() and xen_cpu_bringup_again() with "__noreturn".
Signed-off-by: Juergen Gross <jgross@suse.com> --- arch/x86/xen/smp.h | 2 ++ arch/x86/xen/smp_pv.c | 17 ++++------------- arch/x86/xen/xen-head.S | 7 +++++++ 3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/arch/x86/xen/smp.h b/arch/x86/xen/smp.h index bd02f9d50107..22fb982ff971 100644 --- a/arch/x86/xen/smp.h +++ b/arch/x86/xen/smp.h @@ -21,6 +21,8 @@ void xen_smp_send_reschedule(int cpu); void xen_smp_send_call_function_ipi(const struct cpumask *mask); void xen_smp_send_call_function_single_ipi(int cpu); +void __noreturn xen_cpu_bringup_again(unsigned long stack); + struct xen_common_irq { int irq; char *name; diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c index 480be82e9b7b..5801f93d567c 100644 --- a/arch/x86/xen/smp_pv.c +++ b/arch/x86/xen/smp_pv.c @@ -381,21 +381,12 @@ static void xen_pv_cpu_die(unsigned int cpu) } } -static void xen_pv_play_dead(void) /* used only with HOTPLUG_CPU */ +static void __noreturn xen_pv_play_dead(void) /* used only with HOTPLUG_CPU */ { play_dead_common(); HYPERVISOR_vcpu_op(VCPUOP_down, xen_vcpu_nr(smp_processor_id()), NULL); - cpu_bringup(); - /* - * commit 4b0c0f294 (tick: Cleanup NOHZ per cpu data on cpu down) - * clears certain data that the cpu_idle loop (which called us - * and that we return from) expects. The only way to get that - * data back is to call: - */ - tick_nohz_idle_enter(); - tick_nohz_idle_stop_tick_protected(); - - cpuhp_online_idle(CPUHP_AP_ONLINE_IDLE); + xen_cpu_bringup_again((unsigned long)task_pt_regs(current)); + BUG(); } #else /* !CONFIG_HOTPLUG_CPU */ @@ -409,7 +400,7 @@ static void xen_pv_cpu_die(unsigned int cpu) BUG(); } -static void xen_pv_play_dead(void) +static void __noreturn xen_pv_play_dead(void) { BUG(); } diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S index ffaa62167f6e..e36ea4268bd2 100644 --- a/arch/x86/xen/xen-head.S +++ b/arch/x86/xen/xen-head.S @@ -76,6 +76,13 @@ SYM_CODE_START(asm_cpu_bringup_and_idle) call cpu_bringup_and_idle SYM_CODE_END(asm_cpu_bringup_and_idle) + +SYM_CODE_START(xen_cpu_bringup_again) + UNWIND_HINT_FUNC + mov %rdi, %rsp + UNWIND_HINT_REGS + call cpu_bringup_and_idle +SYM_CODE_END(xen_cpu_bringup_again) .popsection #endif #endif -- 2.35.3 [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |