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.
However, I did not come up with a nice solution for secondary CPUs idle
tasks. The patch just shows the idea what should be done but it is an
ugly hack. Ideas are more than welcome.
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 | 14 +++++++++++++-
2 files changed, 15 insertions(+), 2 deletions(-)
--
2.25.1
The unwinder reports the boot CPU idle task's stack on XEN PV as
unreliable, which affects at least live patching. There are two reasons
for this. First, the task does not follow the x86 convention that its
stack starts at the offset right below saved pt_regs. It allows the
unwinder to easily detect the end of the stack and verify it. Second,
startup_xen() function does not store the return address before jumping
to xen_start_kernel() which confuses the unwinder.
Amend both issues by moving the starting point of initial stack in
startup_xen() and storing the return address before the jump.
Signed-off-by: Miroslav Benes <[email protected]>
---
arch/x86/xen/xen-head.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 1d0cee3163e4..642f346bfe02 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -35,7 +35,7 @@ SYM_CODE_START(startup_xen)
rep __ASM_SIZE(stos)
mov %_ASM_SI, xen_start_info
- mov $init_thread_union+THREAD_SIZE, %_ASM_SP
+ mov $init_thread_union+THREAD_SIZE-SIZEOF_PTREGS, %_ASM_SP
#ifdef CONFIG_X86_64
/* Set up %gs.
@@ -51,7 +51,9 @@ SYM_CODE_START(startup_xen)
wrmsr
#endif
+ push $1f
jmp xen_start_kernel
+1:
SYM_CODE_END(startup_xen)
__FINIT
#endif
--
2.25.1
On 12/03/2020 14:20, Miroslav Benes wrote:
> The unwinder reports the boot CPU idle task's stack on XEN PV as
> unreliable, which affects at least live patching. There are two reasons
> for this. First, the task does not follow the x86 convention that its
> stack starts at the offset right below saved pt_regs. It allows the
> unwinder to easily detect the end of the stack and verify it. Second,
> startup_xen() function does not store the return address before jumping
> to xen_start_kernel() which confuses the unwinder.
>
> Amend both issues by moving the starting point of initial stack in
> startup_xen() and storing the return address before the jump.
>
> Signed-off-by: Miroslav Benes <[email protected]>
> ---
> arch/x86/xen/xen-head.S | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> index 1d0cee3163e4..642f346bfe02 100644
> --- a/arch/x86/xen/xen-head.S
> +++ b/arch/x86/xen/xen-head.S
> @@ -35,7 +35,7 @@ SYM_CODE_START(startup_xen)
> rep __ASM_SIZE(stos)
>
> mov %_ASM_SI, xen_start_info
> - mov $init_thread_union+THREAD_SIZE, %_ASM_SP
> + mov $init_thread_union+THREAD_SIZE-SIZEOF_PTREGS, %_ASM_SP
>
> #ifdef CONFIG_X86_64
> /* Set up %gs.
> @@ -51,7 +51,9 @@ SYM_CODE_START(startup_xen)
> wrmsr
> #endif
>
> + push $1f
> jmp xen_start_kernel
> +1:
Hang on. Isn't this just a `call` instruction written in longhand?
~Andrew
On Thu, 12 Mar 2020, Andrew Cooper wrote:
> On 12/03/2020 14:20, Miroslav Benes wrote:
> > The unwinder reports the boot CPU idle task's stack on XEN PV as
> > unreliable, which affects at least live patching. There are two reasons
> > for this. First, the task does not follow the x86 convention that its
> > stack starts at the offset right below saved pt_regs. It allows the
> > unwinder to easily detect the end of the stack and verify it. Second,
> > startup_xen() function does not store the return address before jumping
> > to xen_start_kernel() which confuses the unwinder.
> >
> > Amend both issues by moving the starting point of initial stack in
> > startup_xen() and storing the return address before the jump.
> >
> > Signed-off-by: Miroslav Benes <[email protected]>
> > ---
> > arch/x86/xen/xen-head.S | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> > index 1d0cee3163e4..642f346bfe02 100644
> > --- a/arch/x86/xen/xen-head.S
> > +++ b/arch/x86/xen/xen-head.S
> > @@ -35,7 +35,7 @@ SYM_CODE_START(startup_xen)
> > rep __ASM_SIZE(stos)
> >
> > mov %_ASM_SI, xen_start_info
> > - mov $init_thread_union+THREAD_SIZE, %_ASM_SP
> > + mov $init_thread_union+THREAD_SIZE-SIZEOF_PTREGS, %_ASM_SP
> >
> > #ifdef CONFIG_X86_64
> > /* Set up %gs.
> > @@ -51,7 +51,9 @@ SYM_CODE_START(startup_xen)
> > wrmsr
> > #endif
> >
> > + push $1f
> > jmp xen_start_kernel
> > +1:
>
> Hang on. Isn't this just a `call` instruction written in longhand?
It is (as far as I know). I wanted to keep it opencoded for a reason I
don't remember now. I'll change it. Thanks.
Miroslav
On 3/12/20 10:20 AM, Miroslav Benes wrote:
> --- a/arch/x86/xen/xen-head.S
> +++ b/arch/x86/xen/xen-head.S
> @@ -35,7 +35,7 @@ SYM_CODE_START(startup_xen)
> rep __ASM_SIZE(stos)
>
> mov %_ASM_SI, xen_start_info
> - mov $init_thread_union+THREAD_SIZE, %_ASM_SP
> + mov $init_thread_union+THREAD_SIZE-SIZEOF_PTREGS, %_ASM_SP
This is initial_stack, isn't it?
-boris
On Mon, 16 Mar 2020, Boris Ostrovsky wrote:
>
>
> On 3/12/20 10:20 AM, Miroslav Benes wrote:
> > --- a/arch/x86/xen/xen-head.S
> > +++ b/arch/x86/xen/xen-head.S
> > @@ -35,7 +35,7 @@ SYM_CODE_START(startup_xen)
> > rep __ASM_SIZE(stos)
> >
> > mov %_ASM_SI, xen_start_info
> > - mov $init_thread_union+THREAD_SIZE, %_ASM_SP
> > + mov $init_thread_union+THREAD_SIZE-SIZEOF_PTREGS, %_ASM_SP
>
> This is initial_stack, isn't it?
It is. I'll change it.
Thanks
Miroslav