powerpc: Stack Pointer must be 16 aligned
PATCH 1/2 powerpc: Stack Pointer must be 16 aligned
The PowerPC ABI mandates the SP to be 16 bytes aligned, make is so.
--- arch/powerpc/lib/board.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
powerpc/lib/board.c +2 -3
@@ -521,9 +521,8 @@ void board_init_f(ulong bootflag) addr_sp -=16 ; addr_sp &= ~0xF ; s = (ulong *) addr_sp; - *s-- =0 ; - *s-- =0 ; - addr_sp = (ulong ) s; + *s =0 ;/* NULL Back Chain */ + *--s =0 ;/* NULL LR */ debug ("Stack Pointer at: %08lx\n" , addr_sp);/*
Signature · 1 line
1.7.3.4
PATCH 2/2 mpc85xx: Initial SP alignment is wrong.
PowerPC mandates SP to be 16 bytes aligned.
Furthermore, a stack frame is added, pointing to the reset vector which is just get in the way when gdb is walking the stack because the reset vector may not accessible depending on emulator settings. Also use a temp register so gdb doesn't pick up intermediate values.
--- arch/powerpc/cpu/mpc85xx/start.S | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)
powerpc/cpu/mpc85xx/start.S +5 -11
@@ -848,18 +848,12 @@ version_string: .globl _start_cont _start_cont: /* Setup the stack in initial RAM,could be L2-as-SRAM or L1 dcache*/ - lis r1,CONFIG_SYS_INIT_RAM_ADDR at h - ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET at l - + lis r2,(CONFIG_SYS_INIT_RAM_ADDR)@h + ori r2,r2,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 - stwu r0,-4(r1) - stwu r0,-4(r1) /* Terminate call chain */ - - stwu r1,-8(r1) /* Save back chain and move SP */ - lis r0,RESET_VECTOR at h /* Address of reset vector */ - ori r0,r0,RESET_VECTOR at l - stwu r1,-8(r1) /* Save back chain and move SP */ - stw r0,+12(r1) /* Save return addr (underflow vect) */ + stw r0,-4(r2) /* Store a NULL LR */ + stw r0,0(r2) /* Store a NULL Back Chain */ + mr r1,r2 /* Transfer to SP(r1) */ GET_GOT bl cpu_init_early_f
Signature · 1 line
1.7.3.4
PATCH 2/2 mpc85xx: Initial SP alignment is wrong.
Quoted 16 lines · Joakim Tjernlund
PowerPC mandates SP to be 16 bytes aligned.
Furthermore, a stack frame is added, pointing to the reset vector which is just get in the way when gdb is walking the stack because the reset vector may not accessible depending on emulator settings. Also use a temp register so gdb doesn't pick up intermediate values.Signed-off-by Joakim Tjernlund <[email protected]>--- arch/powerpc/cpu/mpc85xx/start.S | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)powerpc/cpu/mpc85xx/start.S +0
-0
powerpc/cpu/mpc85xx/start.S version_string: · line 852
Quoted 8 lines
powerpc/cpu/mpc85xx/start.S +2
-3@@ -848,18 +848,12 @@ version_string: .globl _start_cont _start_cont: /* Setup the stack in initial RAM,could be L2-as-SRAM or L1 dcache*/ - lis r1,CONFIG_SYS_INIT_RAM_ADDR at h - ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET at l - + lis r2,(CONFIG_SYS_INIT_RAM_ADDR)@h + ori r2,r2,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
Please don't use r2 as a general purpose register -- even if it's early enough that we haven't started using it for its fixed purpose yet.
powerpc/cpu/mpc85xx/start.S version_string: · line 856
Quoted 3 lines
powerpc/cpu/mpc85xx/start.S +3
-0+ stw r0,-4(r2) /* Store a NULL LR */ + stw r0,0(r2) /* Store a NULL Back Chain */ + mr r1,r2 /* Transfer to SP(r1) */
Why are you storing anything below the stack pointer? LR goes above the backchain, not below -- and it's not valid anyway for the current stack frame. If you want to have a terminating stack frame with a NULL LR, you need to create a second stack frame, like the code currently does.
-Scott
PATCH 2/2 mpc85xx: Initial SP alignment is wrong.
Scott Wood <[email protected]> wrote on 2012/07/20 23:11:33:
Quoted 13 lines · Joakim Tjernlund
Quoted 11 lines
PowerPC mandates SP to be 16 bytes aligned.
Furthermore, a stack frame is added, pointing to the reset vector which is just get in the way when gdb is walking the stack because the reset vector may not accessible depending on emulator settings. Also use a temp register so gdb doesn't pick up intermediate values.Signed-off-by Joakim Tjernlund <[email protected]>--- arch/powerpc/cpu/mpc85xx/start.S | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)
Quoted 16 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 852
Quoted 13 lines
powerpc/cpu/mpc85xx/start.S +2
-3@@ -848,18 +848,12 @@ version_string: .globl _start_cont _start_cont: /* Setup the stack in initial RAM,could be L2-as-SRAM or L1 dcache*/ - lis r1,CONFIG_SYS_INIT_RAM_ADDR at h - ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET at l - + lis r2,(CONFIG_SYS_INIT_RAM_ADDR)@h + ori r2,r2,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */Please don't use r2 as a general purpose register -- even if it's early enough that we haven't started using it for its fixed purpose yet.
I guess that would be a tiny improvement, but it is nothing compared with the r1 abuse in this file :)
Quoted 9 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 856
Quoted 4 lines
powerpc/cpu/mpc85xx/start.S +3
-0+ stw r0,-4(r2) /* Store a NULL LR */ + stw r0,0(r2) /* Store a NULL Back Chain */ + mr r1,r2 /* Transfer to SP(r1) */Why are you storing anything below the stack pointer? LR goes above the backchain, not below -- and it's not valid anyway for the current stack frame. If you want to have a terminating stack frame with a NULL LR, you need to create a second stack frame, like the code currently does.
Right, it should be +4 where the NULL LR should be. As you said, it doesn't matter because it is not valid for this stack frame and could be left out.
I do not want to create a frame as the current code does, it might create an memory access to 0xfffffffc which I don't want. How about this then:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 stw r0,0(r3) /* Terminate Back Chain */ stw r0,+4(r3) /* NULL LR, to be nice. */ mr r1,r3 /* Transfer to SP(r1) */
or if that reset address is wanted:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 stw r0,0(r3) /* Terminate Back Chain */ stw r0,+4(r3) /* NULL LR, to be nice. */ stwu r3,-16(r3) /* save back chain and move SP */ lis r0,RESET_VECTOR at h ori r0,RESET_VECTOR at l stw r0,+20(r3) /* save return address */ mr r1,r3 /* Transfer to SP(r1) */
I would have to test with the emulator if both are OK or if there will be an access to RESET_VECTOR(I don't have that connected ATM).
Jocke
PATCH 2/2 mpc85xx: Initial SP alignment is wrong.
Quoted 67 lines
Scott Wood <[email protected]> wrote on 2012/07/20 23:11:33:
Quoted 13 lines · Joakim Tjernlund
Quoted 11 lines
PowerPC mandates SP to be 16 bytes aligned.
Furthermore, a stack frame is added, pointing to the reset vector which is just get in the way when gdb is walking the stack because the reset vector may not accessible depending on emulator settings. Also use a temp register so gdb doesn't pick up intermediate values.Signed-off-by Joakim Tjernlund <[email protected]>--- arch/powerpc/cpu/mpc85xx/start.S | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)Quoted 16 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 852
Quoted 13 lines
powerpc/cpu/mpc85xx/start.S +2
-3@@ -848,18 +848,12 @@ version_string: .globl _start_cont _start_cont: /* Setup the stack in initial RAM,could be L2-as-SRAM or L1 dcache*/ - lis r1,CONFIG_SYS_INIT_RAM_ADDR at h - ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET at l - + lis r2,(CONFIG_SYS_INIT_RAM_ADDR)@h + ori r2,r2,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */Please don't use r2 as a general purpose register -- even if it's early enough that we haven't started using it for its fixed purpose yet.
I guess that would be a tiny improvement, but it is nothing compared with the r1 abuse in this file :)
Quoted 9 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 856
Quoted 4 lines
powerpc/cpu/mpc85xx/start.S +3
-0+ stw r0,-4(r2) /* Store a NULL LR */ + stw r0,0(r2) /* Store a NULL Back Chain */ + mr r1,r2 /* Transfer to SP(r1) */Why are you storing anything below the stack pointer? LR goes above the backchain, not below -- and it's not valid anyway for the current stack frame. If you want to have a terminating stack frame with a NULL LR, you need to create a second stack frame, like the code currently does.
Right, it should be +4 where the NULL LR should be. As you said, it doesn't matter because it is not valid for this stack frame and could be left out.
I do not want to create a frame as the current code does, it might create an memory access to
0xfffffffcwhich I don't want. How about this then:lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h
ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
li r0,0
stw r0,0(r3) /* Terminate Back Chain */
stw r0,+4(r3) /* NULL LR, to be nice. */
mr r1,r3 /* Transfer to SP(r1) */or if that reset address is wanted:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h
ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
li r0,0
stw r0,0(r3) /* Terminate Back Chain */
stw r0,+4(r3) /* NULL LR, to be nice. */
stwu r3,-16(r3) /* save back chain and move SP */
Quoted 7 lines
lis r0,RESET_VECTOR at h ori r0,RESET_VECTOR at l stw r0,+20(r3) /* save return address */mr r1,r3 /* Transfer to SP(r1) */
I would have to test with the emulator if both are OK or if there will be an access to
RESET_VECTOR(I don't have that connected ATM).
So I got the BDI connected and did som experiments. Having RESET_VECTOR in the initial stack chain makes gdb print this:
(gdb) bt
#0 cpu_init_early_f () at cpu_init_early.c:116
#1 0xeff800a4 in _start_cont () at start.S:877
Backtrace stopped: frame did not save the PC
(gdb)
So I don't think there is any benefit trying to squeeze RESET_VECTOR into the stack chain.
Jocke
PATCH 2/2 mpc85xx: Initial SP alignment is wrong.
In reply to Joakim TjernlundQuoted 32 lines · Joakim Tjernlund
Scott Wood <[email protected]> wrote on 2012/07/20 23:11:33:
Quoted 13 lines · Joakim Tjernlund
Quoted 11 lines
PowerPC mandates SP to be 16 bytes aligned.
Furthermore, a stack frame is added, pointing to the reset vector which is just get in the way when gdb is walking the stack because the reset vector may not accessible depending on emulator settings. Also use a temp register so gdb doesn't pick up intermediate values.Signed-off-by Joakim Tjernlund <[email protected]>--- arch/powerpc/cpu/mpc85xx/start.S | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)Quoted 16 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 852
Quoted 13 lines
powerpc/cpu/mpc85xx/start.S +2
-3@@ -848,18 +848,12 @@ version_string: .globl _start_cont _start_cont: /* Setup the stack in initial RAM,could be L2-as-SRAM or L1 dcache*/ - lis r1,CONFIG_SYS_INIT_RAM_ADDR at h - ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET at l - + lis r2,(CONFIG_SYS_INIT_RAM_ADDR)@h + ori r2,r2,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */Please don't use r2 as a general purpose register -- even if it's early enough that we haven't started using it for its fixed purpose yet.
Quoted 2 lines
I guess that would be a tiny improvement, but it is nothing compared with the r1 abuse in this file :)
Yeah, I just wanted to discourage making it worse.
Quoted 9 lines
Quoted 8 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 856
Quoted 3 lines
powerpc/cpu/mpc85xx/start.S +3
-0+ stw r0,-4(r2) /* Store a NULL LR */ + stw r0,0(r2) /* Store a NULL Back Chain */ + mr r1,r2 /* Transfer to SP(r1) */Why are you storing anything below the stack pointer? LR goes above the backchain, not below -- and it's not valid anyway for the current stack frame. If you want to have a terminating stack frame with a NULL LR, you need to create a second stack frame, like the code currently does.
Quoted 5 lines
Right, it should be +4 where the NULL LR should be. As you said, it doesn't matter because it is not valid for this stack frame and could be left out.
I do not want to create a frame as the current code does, it might create an memory access to
0xfffffffcwhich I don't want.
How would that happen?
Quoted 21 lines
How about this then:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 stw r0,0(r3) /* Terminate Back Chain */ stw r0,+4(r3) /* NULL LR, to be nice. */ mr r1,r3 /* Transfer to SP(r1) */or if that reset address is wanted:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 stw r0,0(r3) /* Terminate Back Chain */ stw r0,+4(r3) /* NULL LR, to be nice. */ stwu r3,-16(r3) /* save back chain and move SP */ lis r0,RESET_VECTOR at h ori r0,RESET_VECTOR at l stw r0,+20(r3) /* save return address */ mr r1,r3 /* Transfer to SP(r1) */
Quoted 2 lines
I would have to test with the emulator if both are OK or if there will be an access to
RESET_VECTOR(I don't have that connected ATM).
How about:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h
ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
li r0,0
stw r0,0(r3) /* Terminate Back Chain */
stw r0,4(r3) /* NULL LR, to be nice. */
stwu r3,-16(r3) /* Create new stack frame * so NULL LR is valid */
mr r1,r3 /* Transfer to SP(r1) */
-Scott
PATCH 2/2 mpc85xx: Initial SP alignment is wrong.
Scott Wood <[email protected]> wrote on 2012/07/23 18:52:28:
Quoted 92 lines
From: Scott Wood <[email protected]>
To: Joakim Tjernlund <[email protected]>,
Cc: <[email protected]>
Date: 2012/07/23 18:52
Subject: Re: [U-Boot] [PATCH 2/2] mpc85xx: Initial SP alignment is wrong.Quoted 32 lines · Joakim Tjernlund
Scott Wood <[email protected]> wrote on 2012/07/20 23:11:33:
Quoted 13 lines · Joakim Tjernlund
Quoted 11 lines
PowerPC mandates SP to be 16 bytes aligned.
Furthermore, a stack frame is added, pointing to the reset vector which is just get in the way when gdb is walking the stack because the reset vector may not accessible depending on emulator settings. Also use a temp register so gdb doesn't pick up intermediate values.Signed-off-by Joakim Tjernlund <[email protected]>--- arch/powerpc/cpu/mpc85xx/start.S | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)Quoted 16 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 852
Quoted 13 lines
powerpc/cpu/mpc85xx/start.S +2
-3@@ -848,18 +848,12 @@ version_string: .globl _start_cont _start_cont: /* Setup the stack in initial RAM,could be L2-as-SRAM or L1 dcache*/ - lis r1,CONFIG_SYS_INIT_RAM_ADDR at h - ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET at l - + lis r2,(CONFIG_SYS_INIT_RAM_ADDR)@h + ori r2,r2,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */Please don't use r2 as a general purpose register -- even if it's early enough that we haven't started using it for its fixed purpose yet.
Quoted 2 lines
I guess that would be a tiny improvement, but it is nothing compared with the r1 abuse in this file :)
Yeah, I just wanted to discourage making it worse.
Quoted 9 lines
Quoted 8 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 856
Quoted 3 lines
powerpc/cpu/mpc85xx/start.S +3
-0+ stw r0,-4(r2) /* Store a NULL LR */ + stw r0,0(r2) /* Store a NULL Back Chain */ + mr r1,r2 /* Transfer to SP(r1) */Why are you storing anything below the stack pointer? LR goes above the backchain, not below -- and it's not valid anyway for the current stack frame. If you want to have a terminating stack frame with a NULL LR, you need to create a second stack frame, like the code currently does.
Quoted 5 lines
Right, it should be +4 where the NULL LR should be. As you said, it doesn't matter because it is not valid for this stack frame and could be left out.
I do not want to create a frame as the current code does, it might create an memory access to
0xfffffffcwhich I don't want.How would that happen?
Quoted 16 lines
How about this then:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h
ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
li r0,0
stw r0,0(r3) /* Terminate Back Chain */
stw r0,+4(r3) /* NULL LR, to be nice. */
mr r1,r3 /* Transfer to SP(r1) */or if that reset address is wanted:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h
ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
li r0,0
stw r0,0(r3) /* Terminate Back Chain */
stw r0,+4(r3) /* NULL LR, to be nice. */
stwu r3,-16(r3) /* save back chain and move SP */Quoted 7 lines
lis r0,RESET_VECTOR at h ori r0,RESET_VECTOR at l stw r0,+20(r3) /* save return address */mr r1,r3 /* Transfer to SP(r1) */
I would have to test with the emulator if both are OK or if there will be an access to
RESET_VECTOR(I don't have that connected ATM).How about:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 stw r0,0(r3) /* Terminate Back Chain */ stw r0,4(r3) /* NULL LR, to be nice. */
Quoted 4 lines
stwu r3,-16(r3) /* Create new stack frame
* so NULL LR is valid
*/
mr r1,r3 /* Transfer to SP(r1) */
This also confuses gdb
(gdb) bt
#0 cpu_init_early_f () at cpu_init_early.c:96
#1 0xeff80098 in _start_cont () at start.S:863
Backtrace stopped: frame did not save the PC
that extra stwu creates an improper call chain. We have the initial stack frame already without the stwu
Jocke
PATCH 2/2 mpc85xx: Initial SP alignment is wrong.
Quoted 101 lines · Joakim Tjernlund
Scott Wood <[email protected]> wrote on 2012/07/23 18:52:28:
Quoted 92 lines
From: Scott Wood <[email protected]>
To: Joakim Tjernlund <[email protected]>,
Cc: <[email protected]>
Date: 2012/07/23 18:52
Subject: Re: [U-Boot] [PATCH 2/2] mpc85xx: Initial SP alignment is wrong.Quoted 32 lines · Joakim Tjernlund
Scott Wood <[email protected]> wrote on 2012/07/20 23:11:33:
Quoted 13 lines · Joakim Tjernlund
Quoted 11 lines
PowerPC mandates SP to be 16 bytes aligned.
Furthermore, a stack frame is added, pointing to the reset vector which is just get in the way when gdb is walking the stack because the reset vector may not accessible depending on emulator settings. Also use a temp register so gdb doesn't pick up intermediate values.Signed-off-by Joakim Tjernlund <[email protected]>--- arch/powerpc/cpu/mpc85xx/start.S | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)Quoted 16 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 852
Quoted 13 lines
powerpc/cpu/mpc85xx/start.S +2
-3@@ -848,18 +848,12 @@ version_string: .globl _start_cont _start_cont: /* Setup the stack in initial RAM,could be L2-as-SRAM or L1 dcache*/ - lis r1,CONFIG_SYS_INIT_RAM_ADDR at h - ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET at l - + lis r2,(CONFIG_SYS_INIT_RAM_ADDR)@h + ori r2,r2,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */Please don't use r2 as a general purpose register -- even if it's early enough that we haven't started using it for its fixed purpose yet.
Quoted 2 lines
I guess that would be a tiny improvement, but it is nothing compared with the r1 abuse in this file :)
Yeah, I just wanted to discourage making it worse.
Quoted 9 lines
Quoted 8 lines
powerpc/cpu/mpc85xx/start.S version_string: · line 856
Quoted 3 lines
powerpc/cpu/mpc85xx/start.S +3
-0+ stw r0,-4(r2) /* Store a NULL LR */ + stw r0,0(r2) /* Store a NULL Back Chain */ + mr r1,r2 /* Transfer to SP(r1) */Why are you storing anything below the stack pointer? LR goes above the backchain, not below -- and it's not valid anyway for the current stack frame. If you want to have a terminating stack frame with a NULL LR, you need to create a second stack frame, like the code currently does.
Quoted 5 lines
Right, it should be +4 where the NULL LR should be. As you said, it doesn't matter because it is not valid for this stack frame and could be left out.
I do not want to create a frame as the current code does, it might create an memory access to
0xfffffffcwhich I don't want.How would that happen?
Quoted 16 lines
How about this then:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h
ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
li r0,0
stw r0,0(r3) /* Terminate Back Chain */
stw r0,+4(r3) /* NULL LR, to be nice. */
mr r1,r3 /* Transfer to SP(r1) */or if that reset address is wanted:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h
ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
li r0,0
stw r0,0(r3) /* Terminate Back Chain */
stw r0,+4(r3) /* NULL LR, to be nice. */
stwu r3,-16(r3) /* save back chain and move SP */Quoted 7 lines
lis r0,RESET_VECTOR at h ori r0,RESET_VECTOR at l stw r0,+20(r3) /* save return address */mr r1,r3 /* Transfer to SP(r1) */
I would have to test with the emulator if both are OK or if there will be an access to
RESET_VECTOR(I don't have that connected ATM).How about:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 stw r0,0(r3) /* Terminate Back Chain */ stw r0,4(r3) /* NULL LR, to be nice. */Quoted 4 lines
stwu r3,-16(r3) /* Create new stack frame
* so NULL LR is valid
*/
mr r1,r3 /* Transfer to SP(r1) */
Quoted 8 lines
This also confuses gdb
(gdb) bt
#0cpu_init_early_f() at cpu_init_early.c:96
#10xeff80098in_start_cont() at start.S:863
Backtrace stopped: frame did not save the PCthat extra stwu creates an improper call chain. We have the initial stack frame already without the stwu
Well, we do want it to stop the backtrace at that point. :-)
...but I was a bit confused when I thought it would help terminate things. The NULL LR only helps prevent finding something worse, if something happens to do a backtrace immediately after the first stwu but before a real LR is saved.
-Scott
PATCH 2/2 mpc85xx: Initial SP alignment is wrong.
Scott Wood <[email protected]> wrote on 2012/07/23 19:28:20:
Quoted 10 lines · Joakim Tjernlund
Quoted 2 lines
...but I was a bit confused when I thought it would help terminate things. The NULL LR only helps prevent finding something worse, if something happens to do a backtrace immediately after the first stwu but before a real LR is saved.
So I just sent v2, now I am offline for a few days/a week.
Jocke
PATCH 1/2 powerpc: Stack Pointer must be 16 aligned
In reply to Joakim TjernlundQuoted 12 lines · Joakim Tjernlund
The PowerPC ABI mandates the SP to be 16 bytes aligned, make is so.
Signed-off-by Joakim Tjernlund <[email protected]>--- arch/powerpc/lib/board.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)powerpc/lib/board.c +0
-0
powerpc/lib/board.c void board_init_f(ulong bootflag) · line 525
Quoted 8 lines
powerpc/lib/board.c +2
-3@@ -521,9 +521,8 @@ void board_init_f(ulong bootflag) addr_sp -=16 ; addr_sp &= ~0xF ; s = (ulong *) addr_sp; - *s-- =0 ; - *s-- =0 ; - addr_sp = (ulong ) s; + *s =0 ;/* NULL Back Chain */ + *--s =0 ;/* NULL LR */
Same problems as in patch 2/2.
-Scott
PATCH 1/2 powerpc: Stack Pointer must be 16 aligned
Scott Wood <[email protected]> wrote on 2012/07/20 23:12:49:
Quoted 14 lines
From: Scott Wood <[email protected]>
To: Joakim Tjernlund <[email protected]>,
Cc: <[email protected]>
Date: 2012/07/20 23:12
Subject: Re: [U-Boot] [PATCH 1/2] powerpc: Stack Pointer must be 16 alignedQuoted 7 lines · Joakim Tjernlund
The PowerPC ABI mandates the SP to be 16 bytes aligned, make is so.
Signed-off-by Joakim Tjernlund <[email protected]>--- arch/powerpc/lib/board.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
Quoted 15 lines
powerpc/lib/board.c void board_init_f(ulong bootflag) · line 525
Quoted 13 lines
powerpc/lib/board.c +2
-3@@ -521,9 +521,8 @@ void board_init_f(ulong bootflag) addr_sp -=16 ; addr_sp &= ~0xF ; s = (ulong *) addr_sp; - *s-- =0 ; - *s-- =0 ; - addr_sp = (ulong ) s; + *s =0 ;/* NULL Back Chain */ + *--s =0 ;/* NULL LR */ Same problems as in patch 2/2.
Right, that should be
*s = 0; /* NULL Back Chain */
*++s = 0; /* NULL LR */Jocke