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

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

[Arun Chandran]

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
}

The compiler is clearly spilling here around the cache being disabled,
resulting in stale values being restored.  As we cannot control the compiler's
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)
+
 /*
  *	cpu_do_idle()
  *
-- 
1.7.9.5

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

[Mark Rutland]

Hi Arun,

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:

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.

Cheers,
Mark.

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

[Arun Chandran]

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

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

[Mark Rutland]

On Wed, Aug 13, 2014 at 12:17:54PM +0100, Arun Chandran wrote:
> 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.

Well, either output would be fine as an example. I'd just like to see
the real asm rather than pseudocode.

Cheers,
Mark.

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

[Arun Chandran]

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 (v3.16) built with Linaro GCC 4.9-2014.05.

ffffffc000085224 <soft_restart>:
ffffffc000085224:  a9be7bfd  stp    x29, x30, [sp,#-32]!
ffffffc000085228:  910003fd  mov    x29, sp
ffffffc00008522c:  f9000fa0  str    x0, [x29,#24]
ffffffc000085230:  94003d21  bl     ffffffc0000946b4 <setup_mm_for_reboot>
ffffffc000085234:  94003b33  bl     ffffffc000093f00 <flush_cache_all>
ffffffc000085238:  94003dfa  bl     ffffffc000094a20 <cpu_cache_off>
ffffffc00008523c:  94003b31  bl     ffffffc000093f00 <flush_cache_all>
ffffffc000085240:  b0003321  adrp   x1, ffffffc0006ea000 <reset_devices>

ffffffc000085244:  f9400fa0  ldr    x0, [x29,#24] ----> spilled addr
ffffffc000085248:  f942fc22  ldr    x2, [x1,#1528] ----> spilled memstart_addr

ffffffc00008524c:  f0000061  adrp   x1, ffffffc000094000 <__inval_cache_range+0x40>
ffffffc000085250:  91290021  add    x1, x1, #0xa40
ffffffc000085254:  8b010041  add    x1, x2, x1
ffffffc000085258:  d2c00802  mov    x2, #0x4000000000           // #274877906944
ffffffc00008525c:  8b020021  add    x1, x1, x2
ffffffc000085260:  d63f0020  blr    x1
...

The compiler is clearly spilling here around the cache being disabled,
resulting in stale values being restored. As we cannot control the compiler's
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)
+
 /*
  *	cpu_do_idle()
  *
-- 
1.7.9.5

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

[Mark Rutland]

Hi Arun,

On Wed, Aug 13, 2014 at 12:54:41PM +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 (v3.16) built with Linaro GCC 4.9-2014.05.
> 
> ffffffc000085224 <soft_restart>:
> ffffffc000085224:  a9be7bfd  stp    x29, x30, [sp,#-32]!
> ffffffc000085228:  910003fd  mov    x29, sp
> ffffffc00008522c:  f9000fa0  str    x0, [x29,#24]
> ffffffc000085230:  94003d21  bl     ffffffc0000946b4 <setup_mm_for_reboot>
> ffffffc000085234:  94003b33  bl     ffffffc000093f00 <flush_cache_all>
> ffffffc000085238:  94003dfa  bl     ffffffc000094a20 <cpu_cache_off>
> ffffffc00008523c:  94003b31  bl     ffffffc000093f00 <flush_cache_all>
> ffffffc000085240:  b0003321  adrp   x1, ffffffc0006ea000 <reset_devices>
> 
> ffffffc000085244:  f9400fa0  ldr    x0, [x29,#24] ----> spilled addr
> ffffffc000085248:  f942fc22  ldr    x2, [x1,#1528] ----> spilled memstart_addr

Nit: s/spilled memstart_addr/global memstart_addr/

> ffffffc00008524c:  f0000061  adrp   x1, ffffffc000094000 <__inval_cache_range+0x40>
> ffffffc000085250:  91290021  add    x1, x1, #0xa40
> ffffffc000085254:  8b010041  add    x1, x2, x1
> ffffffc000085258:  d2c00802  mov    x2, #0x4000000000           // #274877906944
> ffffffc00008525c:  8b020021  add    x1, x1, x2
> ffffffc000085260:  d63f0020  blr    x1
> ...
> 
> The compiler is clearly spilling here around the cache being disabled,
> resulting in stale values being restored. As we cannot control the compiler's
> spilling behaviour we must rewrite the functions in assembly to
> avoid use of the stack.

Given we also seeing accesses to globals, let's change this to:

Here the compiler generates memory accesses after the cache is disabled,
loading stale values for the spilled value global variable. As we cannot
control when the compiler will access memory we must rewrite the
functions in assembly to stash values we need in registers prior to
disabling the cache, avoiding the use of memory.

> Signed-off-by: Arun Chandran <achandran@mvista.com>

Given that:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks for putting this together, and thanks for bearing with me for the
last few revisions.

Mark.

> ---
>  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)
> +
>  /*
>   *	cpu_do_idle()
>   *
> -- 
> 1.7.9.5
> 
> 

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

[Arun Chandran]

Hi Mark,
On 8/13/14, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Arun,
>
> On Wed, Aug 13, 2014 at 12:54:41PM +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 (v3.16) built with Linaro GCC 4.9-2014.05.
>>
>> ffffffc000085224 <soft_restart>:
>> ffffffc000085224:  a9be7bfd  stp    x29, x30, [sp,#-32]!
>> ffffffc000085228:  910003fd  mov    x29, sp
>> ffffffc00008522c:  f9000fa0  str    x0, [x29,#24]
>> ffffffc000085230:  94003d21  bl     ffffffc0000946b4
>> <setup_mm_for_reboot>
>> ffffffc000085234:  94003b33  bl     ffffffc000093f00 <flush_cache_all>
>> ffffffc000085238:  94003dfa  bl     ffffffc000094a20 <cpu_cache_off>
>> ffffffc00008523c:  94003b31  bl     ffffffc000093f00 <flush_cache_all>
>> ffffffc000085240:  b0003321  adrp   x1, ffffffc0006ea000 <reset_devices>
>>
>> ffffffc000085244:  f9400fa0  ldr    x0, [x29,#24] ----> spilled addr
>> ffffffc000085248:  f942fc22  ldr    x2, [x1,#1528] ----> spilled
>> memstart_addr
>
> Nit: s/spilled memstart_addr/global memstart_addr/
>
>> ffffffc00008524c:  f0000061  adrp   x1, ffffffc000094000
>> <__inval_cache_range+0x40>
>> ffffffc000085250:  91290021  add    x1, x1, #0xa40
>> ffffffc000085254:  8b010041  add    x1, x2, x1
>> ffffffc000085258:  d2c00802  mov    x2, #0x4000000000           //
>> #274877906944
>> ffffffc00008525c:  8b020021  add    x1, x1, x2
>> ffffffc000085260:  d63f0020  blr    x1
>> ...
>>
>> The compiler is clearly spilling here around the cache being disabled,
>> resulting in stale values being restored. As we cannot control the
>> compiler's
>> spilling behaviour we must rewrite the functions in assembly to
>> avoid use of the stack.
>
> Given we also seeing accesses to globals, let's change this to:
>
> Here the compiler generates memory accesses after the cache is disabled,
> loading stale values for the spilled value global variable. As we cannot
> control when the compiler will access memory we must rewrite the
> functions in assembly to stash values we need in registers prior to
> disabling the cache, avoiding the use of memory.
>

Will do that.

>> Signed-off-by: Arun Chandran <achandran@mvista.com>
>
> Given that:
>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>
> Thanks for putting this together, and thanks for bearing with me for the
> last few revisions.
>
No problem. Thanks for bearing with a new comer (me). Will post a new
one tomorrow.

--Arun

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

[Arun Chandran]

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 (v3.16) built with Linaro GCC 4.9-2014.05.

ffffffc000085224 <soft_restart>:
ffffffc000085224:  a9be7bfd  stp    x29, x30, [sp,#-32]!
ffffffc000085228:  910003fd  mov    x29, sp
ffffffc00008522c:  f9000fa0  str    x0, [x29,#24]
ffffffc000085230:  94003d21  bl     ffffffc0000946b4 <setup_mm_for_reboot>
ffffffc000085234:  94003b33  bl     ffffffc000093f00 <flush_cache_all>
ffffffc000085238:  94003dfa  bl     ffffffc000094a20 <cpu_cache_off>
ffffffc00008523c:  94003b31  bl     ffffffc000093f00 <flush_cache_all>
ffffffc000085240:  b0003321  adrp   x1, ffffffc0006ea000 <reset_devices>

ffffffc000085244:  f9400fa0  ldr    x0, [x29,#24] ----> spilled addr
ffffffc000085248:  f942fc22  ldr    x2, [x1,#1528] ----> global memstart_addr

ffffffc00008524c:  f0000061  adrp   x1, ffffffc000094000 <__inval_cache_range+0x40>
ffffffc000085250:  91290021  add    x1, x1, #0xa40
ffffffc000085254:  8b010041  add    x1, x2, x1
ffffffc000085258:  d2c00802  mov    x2, #0x4000000000           // #274877906944
ffffffc00008525c:  8b020021  add    x1, x1, x2
ffffffc000085260:  d63f0020  blr    x1
...

Here the compiler generates memory accesses after the cache is disabled,
loading stale values for the spilled value and global variable. As we cannot
control when the compiler will access memory we must rewrite the
functions in assembly to stash values we need in registers prior to
disabling the cache, avoiding the use of memory.

Signed-off-by: Arun Chandran <achandran@mvista.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.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)
+
 /*
  *	cpu_do_idle()
  *
-- 
1.7.9.5