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

[Marek Vasut]

On 04/26/2018 09:33 PM, Dr. Philipp Tomsich wrote:
> 
>> On 26 Apr 2018, at 20:18, Klaus Goger  
>> 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:   e1a0c00emov ip, lr
>> 40002338:   eb00df4cbl  4003a070
>> <__lowlevel_init_from_arm>
>> 4000233c:   e1a0e00cmov lr, ip
>> 40002340:   e1a0f00emov pc, lr
>> [...]
>> 4003a070 <__lowlevel_init_from_arm>:
>> 4003a070:   e59fc004ldr ip, [pc, #4]; 4003a07c
>> <__lowlevel_init_from_arm+0xc>
>> 4003a074:   e08fc00cadd ip, pc, ip
>> 4003a078:   e12fff1cbx  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 
>> Signed-off-by: Christoph Muellner 
> 
> Reviewed-by: Philipp Tomsich 

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
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Dr. Philipp Tomsich]

> On 26 Apr 2018, at 20:18, Klaus Goger  
> 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:   e1a0c00emov ip, lr
> 40002338:   eb00df4cbl  4003a070
> <__lowlevel_init_from_arm>
> 4000233c:   e1a0e00cmov lr, ip
> 40002340:   e1a0f00emov pc, lr
> [...]
> 4003a070 <__lowlevel_init_from_arm>:
> 4003a070:   e59fc004ldr ip, [pc, #4]; 4003a07c
> <__lowlevel_init_from_arm+0xc>
> 4003a074:   e08fc00cadd ip, pc, ip
> 4003a078:   e12fff1cbx  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 
> Signed-off-by: Christoph Muellner 

Reviewed-by: Philipp Tomsich 

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



signature.asc
Description: Message signed with OpenPGP
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot