The unwinder reports idle tasks' stack on XEN PV as unreliable which
complicates things for at least live patching. The two patches in the
series try to amend that by using similar approach as non-XEN x86 does.
v1->v2:
- call instruction used instead of push+jmp
- initial_stack used directly
There is a thing which makes me slightly uncomfortable. s/jmp/call/
means that, theoretically, the called function could return. GCC then
generates not so nice code and there is
asm_cpu_bringup_and_idle+0x5/0x1000 symbol last on the stack due to
alignment in asm/x86/xen/xen-head.S which could be confusing.
Practically it is all fine, because neither xen_start_kernel(), nor
cpu_bringup_and_idle() return (there is unbounded loop in
cpu_startup_entry() around do_idle()). __noreturn annotation of these
functions did not help.
So I don't think it is really a problem, but one may wonder.
Miroslav Benes (2):
x86/xen: Make the boot CPU idle task reliable
x86/xen: Make the secondary CPU idle tasks reliable
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 16 ++++++++++++++--
2 files changed, 16 insertions(+), 3 deletions(-)
--
2.25.1
The unwinder reports the secondary CPU idle tasks' stack on XEN PV as
unreliable, which affects at least live patching.
cpu_initialize_context() sets up the context of the CPU through
VCPUOP_initialise hypercall. After it is woken up, the idle task starts
in cpu_bringup_and_idle() function and its stack starts at the offset
right below pt_regs. The unwinder correctly detects the end of stack
there but it is confused by NULL return address in the last frame.
Introduce a wrapper in assembly, which just calls
cpu_bringup_and_idle(). The return address is thus pushed on the stack
and the wrapper contains the annotation hint for the unwinder regarding
the stack state.
Signed-off-by: Miroslav Benes <[email protected]>
---
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 8 ++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 802ee5bba66c..6b88cdcbef8f 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -53,6 +53,7 @@ static DEFINE_PER_CPU(struct xen_common_irq, xen_irq_work) = { .irq = -1 };
static DEFINE_PER_CPU(struct xen_common_irq, xen_pmu_irq) = { .irq = -1 };
static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id);
+extern unsigned char asm_cpu_bringup_and_idle[];
static void cpu_bringup(void)
{
@@ -309,7 +310,7 @@ cpu_initialize_context(unsigned int cpu, struct task_struct *idle)
* pointing just below where pt_regs would be if it were a normal
* kernel entry.
*/
- ctxt->user_regs.eip = (unsigned long)cpu_bringup_and_idle;
+ ctxt->user_regs.eip = (unsigned long)asm_cpu_bringup_and_idle;
ctxt->flags = VGCF_IN_KERNEL;
ctxt->user_regs.eflags = 0x1000; /* IOPL_RING1 */
ctxt->user_regs.ds = __USER_DS;
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index edc776af0e0a..9dc6f9a420a8 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -58,6 +58,14 @@ SYM_CODE_START(startup_xen)
call xen_start_kernel
SYM_CODE_END(startup_xen)
__FINIT
+
+.pushsection .text
+SYM_CODE_START(asm_cpu_bringup_and_idle)
+ UNWIND_HINT_EMPTY
+
+ call cpu_bringup_and_idle
+SYM_CODE_END(asm_cpu_bringup_and_idle)
+.popsection
#endif
.pushsection .text
--
2.25.1
On 19.03.2020 10:56, Miroslav Benes wrote:
> --- a/arch/x86/xen/smp_pv.c
> +++ b/arch/x86/xen/smp_pv.c
> @@ -53,6 +53,7 @@ static DEFINE_PER_CPU(struct xen_common_irq, xen_irq_work) = { .irq = -1 };
> static DEFINE_PER_CPU(struct xen_common_irq, xen_pmu_irq) = { .irq = -1 };
>
> static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id);
> +extern unsigned char asm_cpu_bringup_and_idle[];
Imo this would better reflect the actual type, i.e. be a function
decl. If left as an array one, I guess you may want to add const.
Jan
On Thu, 19 Mar 2020, Jan Beulich wrote:
> On 19.03.2020 10:56, Miroslav Benes wrote:
> > --- a/arch/x86/xen/smp_pv.c
> > +++ b/arch/x86/xen/smp_pv.c
> > @@ -53,6 +53,7 @@ static DEFINE_PER_CPU(struct xen_common_irq, xen_irq_work) = { .irq = -1 };
> > static DEFINE_PER_CPU(struct xen_common_irq, xen_pmu_irq) = { .irq = -1 };
> >
> > static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id);
> > +extern unsigned char asm_cpu_bringup_and_idle[];
>
> Imo this would better reflect the actual type, i.e. be a function
> decl. If left as an array one, I guess you may want to add const.
I sticked to what x86 has for secondary_startup_64. I can make it
void asm_cpu_bringup_and_idle(void);
Miroslav