Re: [kvm-unit-tests PATCH 03/32] powerpc: Fix stack backtrace termination
From: Thomas Huth <hidden>
Date: 2024-02-27 08:51:14
Also in:
kvm
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@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...
quoted hunk ↗ jump to hunk
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... :-/
quoted hunk ↗ jump to hunk
/* * 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