[U-Boot] [RFC] powerpc/lib: unsafe register handling in wait_ticks
Hi,
If watchdog is enabled, the arch/powerpc/lib/ticks.S::wait_ticks() function calls the function specified by the WATCHDOG_RESET macro. The wait_ticks function depends on the registers r0, r6 and r7 being preserved however that is not guaranteed if the reset function is a C function.
The following patch changes to using r14+r15 instead of r6+r7 and saves all necessary registers on the stack.
As I'm quite fresh to PowerPC assembly language I would appreciate any feedback on the implementation.
On a side note, one could wonder why this function is not written in C language to start with.
Best regards, Mats Kärrman
--- a/arch/powerpc/lib/ticks.S +++ b/arch/powerpc/lib/ticks.S @@ -50,19 +50,24 @@ wait_ticks: stwu r1, -16(r1) mflr r0 /* save link register */ stw r0, 20(r1) /* Use r0 or GDB will be unhappy */ - mr r7, r3 /* save tick count */ + stw r14, 12(r1) /* save used registers */ + stw r15, 8(r1) + mr r14, r3 /* save tick count */ bl get_ticks /* Get start time */
/* Calculate end time */ - addc r7, r4, r7 /* Compute end time lower */ - addze r6, r3 /* and end time upper */ + addc r14, r4, r14 /* Compute end time lower */ + addze r15, r3 /* and end time upper */
WATCHDOG_RESET /* Trigger watchdog, if needed */ 1: bl get_ticks /* Get current time */ - subfc r4, r4, r7 /* Subtract current time from end time */ - subfe. r3, r3, r6 + subfc r4, r4, r14 /* Subtract current time from end time */ + subfe. r3, r3, r15 bge 1b /* Loop until time expired */
- mtlr r0 /* restore link register */ + lwz r15, 8(r1) /* restore saved registers */ + lwz r14, 12(r1) + lwz r0, 20(r1) addi r1,r1,16 + mtlr r0 blr
Hi,
Hi Mats
I would appreciate if you CC me directly on stuff I have been involved in. I don't read every mail on u-boot list(to many of them). It was just plain luck I saw this one.
If watchdog is enabled, the arch/powerpc/lib/ticks.S::wait_ticks()
function
calls the function specified by the WATCHDOG_RESET macro. The wait_ticks function depends on the registers r0, r6 and r7 being preserved however that is not guaranteed if the reset function is a C function.
The following patch changes to using r14+r15 instead of r6+r7 and saves all necessary registers on the stack.
This adds some extra churn to the code that my patch didn't do. On the other hand your patch makes the function look more like how gcc would have done so I am fine with that. However, I am not sure r14 is a good fit, I cannot remember if there is some special purpose for r14. Hopefully somebody on the list knows.
As I'm quite fresh to PowerPC assembly language I would appreciate any feedback on the implementation.
On a side note, one could wonder why this function is not written in C language to start with.
History I guess, once upon a time this function didn't need(or could not use) the stack. Now it would be quite possible to change it into C.
Best regards, Mats Kärrman
--- a/arch/powerpc/lib/ticks.S +++ b/arch/powerpc/lib/ticks.S @@ -50,19 +50,24 @@ wait_ticks: stwu r1, -16(r1) mflr r0 /* save link register */ stw r0, 20(r1) /* Use r0 or GDB will be unhappy */
- mr r7, r3 /* save tick count */
stw r14, 12(r1) /* save used registers */
stw r15, 8(r1)
mr r14, r3 /* save tick count */ bl get_ticks /* Get start time */
/* Calculate end time */
- addc r7, r4, r7 /* Compute end time lower */
- addze r6, r3 /* and end time upper */
addc r14, r4, r14 /* Compute end time lower */
addze r15, r3 /* and end time upper */
WATCHDOG_RESET /* Trigger watchdog, if needed */
1: bl get_ticks /* Get current time */
- subfc r4, r4, r7 /* Subtract current time from end time */
- subfe. r3, r3, r6
- subfc r4, r4, r14 /* Subtract current time from end time */
- subfe. r3, r3, r15 bge 1b /* Loop until time expired */
- mtlr r0 /* restore link register */
- lwz r15, 8(r1) /* restore saved registers */
- lwz r14, 12(r1)
- lwz r0, 20(r1) addi r1,r1,16
- mtlr r0 blr
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Dear Joakim Tjernlund,
In message OF52C94A3D.C3BD2E6F-ONC1257AFD.005BAFE0-C1257AFD.006736C5@transmode.se you wrote:
This adds some extra churn to the code that my patch didn't do. On the other hand your patch makes the function look more like how gcc would have done so I am fine with that. However, I am not sure r14 is a good fit, I cannot remember if there is some special purpose for r14. Hopefully somebody on the list knows.
This is documented in the README:
| For PowerPC, the following registers have specific use: | R1: stack pointer | R2: reserved for system use | R3-R4: parameter passing and return values | R5-R10: parameter passing | R13: small data area pointer | R30: GOT pointer | R31: frame pointer | | (U-Boot also uses R12 as internal GOT pointer. r12 | is a volatile register so r12 needs to be reset when | going back and forth between asm and C) | | ==> U-Boot will use R2 to hold a pointer to the global data |
Best regards,
Wolfgang Denk
Wolfgang Denk wd@denx.de wrote on 2013/01/24 20:27:26:
Dear Joakim Tjernlund,
In message <OF52C94A3D.C3BD2E6F-ONC1257AFD.005BAFE0-C1257AFD. 006736C5@transmode.se> you wrote:
This adds some extra churn to the code that my patch didn't do. On the other hand your patch makes the function look more like how gcc would have done so I am fine with that. However, I am not sure r14 is a good fit, I cannot remember if there
is
some special purpose for r14. Hopefully somebody on the list knows.
This is documented in the README:
| For PowerPC, the following registers have specific use: | R1: stack pointer | R2: reserved for system use | R3-R4: parameter passing and return values | R5-R10: parameter passing | R13: small data area pointer | R30: GOT pointer | R31: frame pointer | | (U-Boot also uses R12 as internal GOT pointer. r12 | is a volatile register so r12 needs to be reset when | going back and forth between asm and C) | | ==> U-Boot will use R2 to hold a pointer to the global data |
Right, then I think the patch is good:
Acked-by: Joakim Tjernlund joakim.tjernlund@transmode.se
participants (3)
-
Joakim Tjernlund -
Mats Kärrman -
Wolfgang Denk