diff mbox series

[U-Boot,v3,1/2] arm: preserve lr correctly in arm926ejs startup code

Message ID 20180426181810.15899-2-klaus.goger@theobroma-systems.com
State Superseded
Delegated to: Tom Rini
Headers show
Series Fix thumb interwork in assembly code | expand

Commit Message

Klaus Goger April 26, 2018, 6:18 p.m. UTC 
When building the mxs platform in thumb mode gcc generates code using
the intra procedure call scratch register (ip/r12) for the calling the
lowlevel_init function. This modifies the lr in flush_dcache which
causes u-boot proper to end in an endless loop.

40002334:       e1a0c00e        mov     ip, lr
40002338:       eb00df4c        bl      4003a070
<__lowlevel_init_from_arm>
4000233c:       e1a0e00c        mov     lr, ip
40002340:       e1a0f00e        mov     pc, lr
[...]
4003a070 <__lowlevel_init_from_arm>:
4003a070:       e59fc004        ldr     ip, [pc, #4]    ; 4003a07c
<__lowlevel_init_from_arm+0xc>
4003a074:       e08fc00c        add     ip, pc, ip
4003a078:       e12fff1c        bx      ip
4003a07c:       fffc86cd        .word   0xfffc86cd

Instead of using the the ip/r12 register we use sl/r10 to preserve the
link register.

According to "Procedure Call Standard for the ARM Architecture" by ARM
subroutines have to preserve the contents of register r4-r8, r10, r11
and SP. So using r12 to store the link register will fail if the called
function is using r12 and not preserving it as allowed.
This can happen in ARM and thumb mode but will definitely be triggered
by building it in thumb.
Using r10 should be safe in any case as we are startup code and not
called by any C-function and no stack is set up.

Signed-off-by: Klaus Goger <klaus.goger@theobroma-systems.com>
Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>

---

Changes in v3:
- reword commit message as it isn't thumb specific
- use r10 instead of sl alias as we don't use it as stack limit pointer
- revert return to a pc mov as it is a unnecessary change for this patch

Changes in v2:
- use bl instead of blx to call lowlevel_init
- remove mxs tag as it apply to all arm926ejs platforms

 arch/arm/cpu/arm926ejs/start.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Philipp Tomsich April 26, 2018, 7:33 p.m. UTC | #1 
> On 26 Apr 2018, at 20:18, Klaus Goger <klaus.goger@theobroma-systems.com> wrote:
> 
> When building the mxs platform in thumb mode gcc generates code using
> the intra procedure call scratch register (ip/r12) for the calling the
> lowlevel_init function. This modifies the lr in flush_dcache which
> causes u-boot proper to end in an endless loop.
> 
> 40002334:       e1a0c00e        mov     ip, lr
> 40002338:       eb00df4c        bl      4003a070
> <__lowlevel_init_from_arm>
> 4000233c:       e1a0e00c        mov     lr, ip
> 40002340:       e1a0f00e        mov     pc, lr
> [...]
> 4003a070 <__lowlevel_init_from_arm>:
> 4003a070:       e59fc004        ldr     ip, [pc, #4]    ; 4003a07c
> <__lowlevel_init_from_arm+0xc>
> 4003a074:       e08fc00c        add     ip, pc, ip
> 4003a078:       e12fff1c        bx      ip
> 4003a07c:       fffc86cd        .word   0xfffc86cd
> 
> Instead of using the the ip/r12 register we use sl/r10 to preserve the
> link register.
> 
> According to "Procedure Call Standard for the ARM Architecture" by ARM
> subroutines have to preserve the contents of register r4-r8, r10, r11
> and SP. So using r12 to store the link register will fail if the called
> function is using r12 and not preserving it as allowed.

Maybe you could rephrase this one, as the “not preserving it as allowed”
made me read this twice until I realised that it is equivalent to “does not
preserve (and is not required by the PCS to preserve)”?

> This can happen in ARM and thumb mode but will definitely be triggered
> by building it in thumb.
> Using r10 should be safe in any case as we are startup code and not
> called by any C-function and no stack is set up.
> 
> Signed-off-by: Klaus Goger <klaus.goger@theobroma-systems.com>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

> 
> ---
> 
> Changes in v3:
> - reword commit message as it isn't thumb specific
> - use r10 instead of sl alias as we don't use it as stack limit pointer
> - revert return to a pc mov as it is a unnecessary change for this patch
> 
> Changes in v2:
> - use bl instead of blx to call lowlevel_init
> - remove mxs tag as it apply to all arm926ejs platforms
> 
> arch/arm/cpu/arm926ejs/start.S | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
> index 959d1ed86d..a625b39787 100644
> --- a/arch/arm/cpu/arm926ejs/start.S
> +++ b/arch/arm/cpu/arm926ejs/start.S
> @@ -105,9 +105,9 @@ flush_dcache:
> 	/*
> 	 * Go setup Memory and board specific bits prior to relocation.
> 	 */
> -	mov	ip, lr		/* perserve link reg across call */
> +	mov	r10, lr		/* perserve link reg across call */
> 	bl	lowlevel_init	/* go setup pll,mux,memory */
> -	mov	lr, ip		/* restore link */
> +	mov	lr, r10		/* restore link */
> #endif
> 	mov	pc, lr		/* back to my caller */
> #endif /* CONFIG_SKIP_LOWLEVEL_INIT */
> --
> 2.11.0
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Marek Vasut April 26, 2018, 7:34 p.m. UTC | #2 
On 04/26/2018 09:33 PM, Dr. Philipp Tomsich wrote:
> 
>> On 26 Apr 2018, at 20:18, Klaus Goger <klaus.goger@theobroma-systems.com> wrote:
>>
>> When building the mxs platform in thumb mode gcc generates code using
>> the intra procedure call scratch register (ip/r12) for the calling the
>> lowlevel_init function. This modifies the lr in flush_dcache which
>> causes u-boot proper to end in an endless loop.
>>
>> 40002334:       e1a0c00e        mov     ip, lr
>> 40002338:       eb00df4c        bl      4003a070
>> <__lowlevel_init_from_arm>
>> 4000233c:       e1a0e00c        mov     lr, ip
>> 40002340:       e1a0f00e        mov     pc, lr
>> [...]
>> 4003a070 <__lowlevel_init_from_arm>:
>> 4003a070:       e59fc004        ldr     ip, [pc, #4]    ; 4003a07c
>> <__lowlevel_init_from_arm+0xc>
>> 4003a074:       e08fc00c        add     ip, pc, ip
>> 4003a078:       e12fff1c        bx      ip
>> 4003a07c:       fffc86cd        .word   0xfffc86cd
>>
>> Instead of using the the ip/r12 register we use sl/r10 to preserve the
>> link register.
>>
>> According to "Procedure Call Standard for the ARM Architecture" by ARM
>> subroutines have to preserve the contents of register r4-r8, r10, r11
>> and SP. So using r12 to store the link register will fail if the called
>> function is using r12 and not preserving it as allowed.
> 
> Maybe you could rephrase this one, as the “not preserving it as allowed”
> made me read this twice until I realised that it is equivalent to “does not
> preserve (and is not required by the PCS to preserve)”?
> 
>> This can happen in ARM and thumb mode but will definitely be triggered
>> by building it in thumb.
>> Using r10 should be safe in any case as we are startup code and not
>> called by any C-function and no stack is set up.
>>
>> Signed-off-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> 
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

I'd prefer to see a review from someone not from the same company :)

>> ---
>>
>> Changes in v3:
>> - reword commit message as it isn't thumb specific
>> - use r10 instead of sl alias as we don't use it as stack limit pointer
>> - revert return to a pc mov as it is a unnecessary change for this patch
>>
>> Changes in v2:
>> - use bl instead of blx to call lowlevel_init
>> - remove mxs tag as it apply to all arm926ejs platforms
>>
>> arch/arm/cpu/arm926ejs/start.S | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
>> index 959d1ed86d..a625b39787 100644
>> --- a/arch/arm/cpu/arm926ejs/start.S
>> +++ b/arch/arm/cpu/arm926ejs/start.S
>> @@ -105,9 +105,9 @@ flush_dcache:
>> 	/*
>> 	 * Go setup Memory and board specific bits prior to relocation.
>> 	 */
>> -	mov	ip, lr		/* perserve link reg across call */
>> +	mov	r10, lr		/* perserve link reg across call */
>> 	bl	lowlevel_init	/* go setup pll,mux,memory */
>> -	mov	lr, ip		/* restore link */
>> +	mov	lr, r10		/* restore link */
>> #endif
>> 	mov	pc, lr		/* back to my caller */
>> #endif /* CONFIG_SKIP_LOWLEVEL_INIT */
>> --
>> 2.11.0
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
>
diff mbox series

Patch

diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
index 959d1ed86d..a625b39787 100644
--- a/arch/arm/cpu/arm926ejs/start.S
+++ b/arch/arm/cpu/arm926ejs/start.S
@@ -105,9 +105,9 @@  flush_dcache:
 	/*
 	 * Go setup Memory and board specific bits prior to relocation.
 	 */
-	mov	ip, lr		/* perserve link reg across call */
+	mov	r10, lr		/* perserve link reg across call */
 	bl	lowlevel_init	/* go setup pll,mux,memory */
-	mov	lr, ip		/* restore link */
+	mov	lr, r10		/* restore link */
 #endif
 	mov	pc, lr		/* back to my caller */
 #endif /* CONFIG_SKIP_LOWLEVEL_INIT */