[PATCH] Arm64: convert part of soft_restart() to assembly

Arun Chandran
2014-08-13T11:17:54Z

Hi Mark,

> On Wed, Aug 13, 2014 at 08:43:21AM +0100, Arun Chandran wrote:
>> The current soft_restart() and setup_restart implementations incorrectly
>> assume that compiler will not spill/fill values to/from stack. However
>> this assumption seems to be wrong, revealed by the disassembly of the
>> currently existing code.
>>
>> Pseudo code for disassembly looks like
>>
>> soft_restart(addr)
>> {
>> __push_to_stack(addr)
>>
>> branch to setup_mm_for_reboot()
>> branch to flush_cache_all() --> This is unnecessary
>> branch to cpu_cache_off()
>> branch to flush_cache_all() --> Not guaranteed of flushing to PoC
>>
>> __pop_from_stack(addr) --> Fails here as addr is not at PoC
>>
>> cpu_reset(addr) --> cpu_reset receives invalid reset address
>> }
>
> As I mentioned before, I think having pseudocode here is confusing.
> Either we should have a real disassembly or we should drop it. I get the
> following when I build a v3.16 arm64 defconfig with Linaro GCC
> 4.9-2014.05:
>

Hmm. I think It is better to drop it as different compilers give different
output. My compiler's output is according to the commit message, but
your's is not. I will send another one soon.

> ffffffc000085224 <soft_restart>:
> ffffffc000085224: a9be7bfd stp x29, x30, [sp,#-32]!
> ffffffc000085228: 910003fd mov x29, sp
> ffffffc00008522c: f9000fa0 str x0, [x29,#24]
> ffffffc000085230: 94003b16 bl ffffffc000093e88 <setup_mm_for_reboot>
> ffffffc000085234: 94003927 bl ffffffc0000936d0 <flush_cache_all>
> ffffffc000085238: 94003bf2 bl ffffffc000094200 <cpu_cache_off>
> ffffffc00008523c: 94003925 bl ffffffc0000936d0 <flush_cache_all>
> ffffffc000085240: b00031c1 adrp x1, ffffffc0006be000 <reset_devices>
> ffffffc000085244: f9400fa0 ldr x0, [x29,#24]
> ffffffc000085248: f941c822 ldr x2, [x1,#912]
> ffffffc00008524c: f0000061 adrp x1, ffffffc000094000 <set_mm_context+0x10>
> ffffffc000085250: 91088021 add x1, x1, #0x220
> ffffffc000085254: 8b010041 add x1, x2, x1
> ffffffc000085258: d2c00802 mov x2, #0x4000000000 // #274877906944
> ffffffc00008525c: 8b020021 add x1, x1, x2
> ffffffc000085260: d63f0020 blr x1
> ...
>
> The two ldrs correspond to the spilled addr variable and memstart_addr
> respectively.
>
>>
>> The compiler is clearly spilling here around the cache being disabled,
>> resulting in stale values being restored. As we cannot control the compiler's
>
> Nit: double spacing here doesn't match the rest of the message.
>
>> spilling behaviour we must rewrite the functions in assembly to
>> avoid use of the stack.
>>
>> Signed-off-by: Arun Chandran <achandran@mvista.com>
>> ---
>> arch/arm64/include/asm/proc-fns.h | 2 ++
>> arch/arm64/kernel/process.c | 30 ++----------------------------
>> arch/arm64/mm/proc.S | 14 ++++++++++++++
>> 3 files changed, 18 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/proc-fns.h b/arch/arm64/include/asm/proc-fns.h
>> index 0c657bb..86be4f9 100644
>> --- a/arch/arm64/include/asm/proc-fns.h
>> +++ b/arch/arm64/include/asm/proc-fns.h
>> @@ -32,6 +32,8 @@ extern void cpu_cache_off(void);
>> extern void cpu_do_idle(void);
>> extern void cpu_do_switch_mm(unsigned long pgd_phys, struct mm_struct *mm);
>> extern void cpu_reset(unsigned long addr) __attribute__((noreturn));
>> +extern void cpu_soft_restart(phys_addr_t cpu_reset,
>> + unsigned long addr) __attribute__((noreturn));
>> extern void cpu_do_suspend(struct cpu_suspend_ctx *ptr);
>> extern u64 cpu_do_resume(phys_addr_t ptr, u64 idmap_ttbr);
>>
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 1309d64..bf66922 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -57,36 +57,10 @@ unsigned long __stack_chk_guard __read_mostly;
>> EXPORT_SYMBOL(__stack_chk_guard);
>> #endif
>>
>> -static void setup_restart(void)
>> -{
>> - /*
>> - * Tell the mm system that we are going to reboot -
>> - * we may need it to insert some 1:1 mappings so that
>> - * soft boot works.
>> - */
>> - setup_mm_for_reboot();
>> -
>> - /* Clean and invalidate caches */
>> - flush_cache_all();
>> -
>> - /* Turn D-cache off */
>> - cpu_cache_off();
>> -
>> - /* Push out any further dirty data, and ensure cache is empty */
>> - flush_cache_all();
>> -}
>> -
>> void soft_restart(unsigned long addr)
>> {
>> - typedef void (*phys_reset_t)(unsigned long);
>> - phys_reset_t phys_reset;
>> -
>> - setup_restart();
>> -
>> - /* Switch to the identity mapping */
>> - phys_reset = (phys_reset_t)virt_to_phys(cpu_reset);
>> - phys_reset(addr);
>> -
>> + setup_mm_for_reboot();
>> + cpu_soft_restart(virt_to_phys(cpu_reset), addr);
>> /* Should never get here */
>> BUG();
>> }
>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> index 7736779..0eff5ee 100644
>> --- a/arch/arm64/mm/proc.S
>> +++ b/arch/arm64/mm/proc.S
>> @@ -76,6 +76,20 @@ ENTRY(cpu_reset)
>> ret x0
>> ENDPROC(cpu_reset)
>>
>> +ENTRY(cpu_soft_restart)
>> + /* Save address of cpu_reset() and reset address */
>> + mov x19, x0
>> + mov x20, x1
>> +
>> + /* Turn D-cache off */
>> + bl cpu_cache_off
>> + /* Push out all dirty data, and ensure cache is empty */
>> + bl flush_cache_all
>> +
>> + mov x0, x20
>> + ret x19
>> +ENDPROC(cpu_soft_restart)
>
> The code change looks good to me.

OK. Thanks

--Arun