[kvm-unit-tests PATCH 03/32] powerpc: Fix stack backtrace termination

[Thomas Huth]

On 26/02/2024 11.11, 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 at gmail.com>
> ---
>   powerpc/cstart64.S | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)

Thanks for tackling this! ... however, not doing powerpc work since years 
anymore, I have some ignorant questions below...

> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> index e18ae9a22..14ab0c6c8 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)

0(r1) is the back chain pointer ...

> +	std	r0,16(r1)

... but what is 16(r1) ? I suppose that should be the "LR save word" ? But 
isn't that at 8(r1) instead?? (not sure whether I'm looking at the right ELF 
abi spec right now...)

Anyway, a comment in the source would be helpful here.

> +
> +	/* Create entry frame */
> +	stdu	r1,-INT_FRAME_SIZE(r1)

Since we already create an initial frame via stackptr from powerpc/flat.lds, 
do we really need to create this additional one here? Or does the one from 
flat.lds have to be completely empty, i.e. also no DTB pointer in it?

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

Isn't SAVE_GPR rather meant for the interrupt frame, not for the normal C 
calling convention frames?

Sorry for asking dumb questions ... I still have a hard time understanding 
the changes here... :-/

>   	/*
>   	 * 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 */

  Thomas