diff mbox series

[kvm-unit-tests,v5,18/29] powerpc: Fix stack backtrace termination

Message ID 20231216134257.1743345-19-npiggin@gmail.com (mailing list archive)
State New, archived
Headers show
Series powerpc: updates, P10, PNV support | expand

Commit Message

Nicholas Piggin Dec. 16, 2023, 1:42 p.m. UTC
The backtrace handler terminates when it sees a NULL caller address,
but the powerpc stack setup does not keep such a NULL caller frame
at the start of the stack.

This happens to work on pseries because the memory at 0 is mapped and
it contains 0 at the location of the return address pointer if it
were a stack frame. But this is fragile, and does not work with powernv
where address 0 contains firmware instructions.

Use the existing dummy frame on stack as the NULL caller, and create a
new frame on stack for the entry code.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 powerpc/cstart64.S | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Thomas Huth Dec. 19, 2023, 12:22 p.m. UTC | #1
On 16/12/2023 14.42, Nicholas Piggin wrote:
> The backtrace handler terminates when it sees a NULL caller address,
> but the powerpc stack setup does not keep such a NULL caller frame
> at the start of the stack.
> 
> This happens to work on pseries because the memory at 0 is mapped and
> it contains 0 at the location of the return address pointer if it
> were a stack frame. But this is fragile, and does not work with powernv
> where address 0 contains firmware instructions.
> 
> Use the existing dummy frame on stack as the NULL caller, and create a
> new frame on stack for the entry code.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   powerpc/cstart64.S | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> index e18ae9a2..14ab0c6c 100644
> --- a/powerpc/cstart64.S
> +++ b/powerpc/cstart64.S
> @@ -46,8 +46,16 @@ start:
>   	add	r1, r1, r31
>   	add	r2, r2, r31
>   
> +	/* Zero backpointers in initial stack frame so backtrace() stops */
> +	li	r0,0
> +	std	r0,0(r1)
> +	std	r0,16(r1)
> +
> +	/* Create entry frame */
> +	stdu	r1,-INT_FRAME_SIZE(r1)

Shouldn't that rather be STACK_FRAME_OVERHEAD instead of INT_FRAME_SIZE...

>   	/* save DTB pointer */
> -	std	r3, 56(r1)
> +	SAVE_GPR(3,r1)

... since SAVE_GPR uses STACK_FRAME_OVERHEAD (via GPR0), too?

  Thomas
Nicholas Piggin Dec. 22, 2023, 9:55 a.m. UTC | #2
On Tue Dec 19, 2023 at 10:22 PM AEST, Thomas Huth wrote:
> On 16/12/2023 14.42, Nicholas Piggin wrote:
> > The backtrace handler terminates when it sees a NULL caller address,
> > but the powerpc stack setup does not keep such a NULL caller frame
> > at the start of the stack.
> > 
> > This happens to work on pseries because the memory at 0 is mapped and
> > it contains 0 at the location of the return address pointer if it
> > were a stack frame. But this is fragile, and does not work with powernv
> > where address 0 contains firmware instructions.
> > 
> > Use the existing dummy frame on stack as the NULL caller, and create a
> > new frame on stack for the entry code.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   powerpc/cstart64.S | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> > index e18ae9a2..14ab0c6c 100644
> > --- a/powerpc/cstart64.S
> > +++ b/powerpc/cstart64.S
> > @@ -46,8 +46,16 @@ start:
> >   	add	r1, r1, r31
> >   	add	r2, r2, r31
> >   
> > +	/* Zero backpointers in initial stack frame so backtrace() stops */
> > +	li	r0,0
> > +	std	r0,0(r1)
> > +	std	r0,16(r1)
> > +
> > +	/* Create entry frame */
> > +	stdu	r1,-INT_FRAME_SIZE(r1)
>
> Shouldn't that rather be STACK_FRAME_OVERHEAD instead of INT_FRAME_SIZE...
>
> >   	/* save DTB pointer */
> > -	std	r3, 56(r1)
> > +	SAVE_GPR(3,r1)
>
> ... since SAVE_GPR uses STACK_FRAME_OVERHEAD (via GPR0), too?

No I think it's correct. INT_FRAME_SIZE has STACK_FRAME_OVERHEAD and
struct pt_regs. The STACK_FRAME_OVERHEAD in GPR offsets is just to skip
that and get to pt_regs.gpr[].

Thanks,
Nick
diff mbox series

Patch

diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
index e18ae9a2..14ab0c6c 100644
--- a/powerpc/cstart64.S
+++ b/powerpc/cstart64.S
@@ -46,8 +46,16 @@  start:
 	add	r1, r1, r31
 	add	r2, r2, r31
 
+	/* Zero backpointers in initial stack frame so backtrace() stops */
+	li	r0,0
+	std	r0,0(r1)
+	std	r0,16(r1)
+
+	/* Create entry frame */
+	stdu	r1,-INT_FRAME_SIZE(r1)
+
 	/* save DTB pointer */
-	std	r3, 56(r1)
+	SAVE_GPR(3,r1)
 
 	/*
 	 * Call relocate. relocate is C code, but careful to not use
@@ -101,7 +109,7 @@  start:
 	stw	r4, 0(r3)
 
 	/* complete setup */
-1:	ld	r3, 56(r1)
+1:	REST_GPR(3, r1)
 	bl	setup
 
 	/* run the test */