[PATCH] Arm64: convert soft_restart() to assembly code

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

soft_restart() will fail on arm64 systems that does not
quarantee the flushing of cache to PoC with flush_cache_all().

soft_restart(addr)
{
	push_to_stack(addr);

	Do mm setup for restart;
	Flush&turnoff D-cache;

	pop_from_stack(addr); --> fails here as addr is not at PoC
	cpu_reset(addr) --> Jumps to invalid address
}

So convert the whole code to assembly to make sure that addr
is not pushed to stack.

Signed-off-by: Arun Chandran <achandran@mvista.com>
---
 arch/arm64/include/asm/proc-fns.h    |    1 +
 arch/arm64/include/asm/system_misc.h |    1 -
 arch/arm64/kernel/process.c          |   38 ++--------------------------------
 arch/arm64/mm/proc.S                 |   34 ++++++++++++++++++++++++++++++
 4 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/arch/arm64/include/asm/proc-fns.h b/arch/arm64/include/asm/proc-fns.h
index 0c657bb..e18d5d0 100644
--- a/arch/arm64/include/asm/proc-fns.h
+++ b/arch/arm64/include/asm/proc-fns.h
@@ -32,6 +32,7 @@ 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(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/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
index 7a18fab..659fbf5 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -41,7 +41,6 @@ struct mm_struct;
 extern void show_pte(struct mm_struct *mm, unsigned long addr);
 extern void __show_regs(struct pt_regs *);
 
-void soft_restart(unsigned long);
 extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 
 #define UDBG_UNDEFINED	(1 << 0)
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 1309d64..3ca1ade 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -57,40 +57,6 @@ 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);
-
-	/* Should never get here */
-	BUG();
-}
-
 /*
  * Function pointers to optional machine specific functions
  */
@@ -162,8 +128,8 @@ void machine_power_off(void)
 
 /*
  * Restart requires that the secondary CPUs stop performing any activity
- * while the primary CPU resets the system. Systems with a single CPU can
- * use soft_restart() as their machine descriptor's .restart hook, since that
+ * while the primary CPU resets the system. Systems with a single CPU can use
+ * cpu_soft_restart() as their machine descriptor's .restart hook, since that
  * will cause the only available CPU to reset. Systems with multiple CPUs must
  * provide a HW restart implementation, to ensure that all CPUs reset@once.
  * This is required so that any code running after reset on the primary CPU
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 7736779..a7c3fce 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -76,6 +76,40 @@ ENTRY(cpu_reset)
 	ret	x0
 ENDPROC(cpu_reset)
 
+	.align 3
+1:	.quad	memstart_addr
+
+ENTRY(cpu_soft_restart)
+	adr	x1, cpu_reset
+	adr	x2, 1b
+
+	/* virt_to_phys(cpu_reset) */
+	ldr	x3, [x2]
+	ldr	x3, [x3]
+	mov	x4, #1
+	lsl	x4, x4, #(VA_BITS - 1)
+	add	x1, x1, x4
+	add	x1, x1, x3
+
+	/* Save it; We can't use stack as it is going to run with caches OFF */
+	mov	x19, x0
+	mov	x20, x1
+
+	bl	setup_mm_for_reboot
+
+	bl	flush_cache_all
+	/* Turn D-cache off */
+	bl	cpu_cache_off
+	/* Push out any further dirty data, and ensure cache is empty */
+	bl	flush_cache_all
+
+	mov	x0, x19
+
+	br	x20
+	/* It should never come back */
+	bl	panic
+ENDPROC(cpu_soft_restart)
+
 /*
  *	cpu_do_idle()
  *
-- 
1.7.9.5

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

Hi Arun,

On Tue, Aug 12, 2014 at 01:42:45PM +0100, Arun Chandran wrote:
> soft_restart() will fail on arm64 systems that does not
> quarantee the flushing of cache to PoC with flush_cache_all().

The flush_cache_all function is never guaranteed to flush data to the
PoC, so I wouldn't say that this is only broken on some systems. We just
happen to have been lucky so far.

> soft_restart(addr)
> {
> 	push_to_stack(addr);
> 
> 	Do mm setup for restart;
> 	Flush&turnoff D-cache;
> 
> 	pop_from_stack(addr); --> fails here as addr is not at PoC
> 	cpu_reset(addr) --> Jumps to invalid address
> }

This doesn't match this organisation on soft_restart, which makes it a
little confusing.

>
> So convert the whole code to assembly to make sure that addr
> is not pushed to stack.

It's possible that the compiler would previously have spilled other
values too, we only observed addr being spilled.

How about we reword this something like:

The current soft_restart and setup_restart implementations incorrectly
assume that the compiler will not spill/fill values to/from the stack,
but the compiler has been observed to do so 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    |    1 +
>  arch/arm64/include/asm/system_misc.h |    1 -
>  arch/arm64/kernel/process.c          |   38 ++--------------------------------
>  arch/arm64/mm/proc.S                 |   34 ++++++++++++++++++++++++++++++
>  4 files changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/proc-fns.h b/arch/arm64/include/asm/proc-fns.h
> index 0c657bb..e18d5d0 100644
> --- a/arch/arm64/include/asm/proc-fns.h
> +++ b/arch/arm64/include/asm/proc-fns.h
> @@ -32,6 +32,7 @@ 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(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/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
> index 7a18fab..659fbf5 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -41,7 +41,6 @@ struct mm_struct;
>  extern void show_pte(struct mm_struct *mm, unsigned long addr);
>  extern void __show_regs(struct pt_regs *);
>  
> -void soft_restart(unsigned long);
>  extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>  
>  #define UDBG_UNDEFINED	(1 << 0)
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 1309d64..3ca1ade 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -57,40 +57,6 @@ 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);
> -
> -	/* Should never get here */
> -	BUG();
> -}
> -
>  /*
>   * Function pointers to optional machine specific functions
>   */
> @@ -162,8 +128,8 @@ void machine_power_off(void)
>  
>  /*
>   * Restart requires that the secondary CPUs stop performing any activity
> - * while the primary CPU resets the system. Systems with a single CPU can
> - * use soft_restart() as their machine descriptor's .restart hook, since that
> + * while the primary CPU resets the system. Systems with a single CPU can use
> + * cpu_soft_restart() as their machine descriptor's .restart hook, since that
>   * will cause the only available CPU to reset. Systems with multiple CPUs must
>   * provide a HW restart implementation, to ensure that all CPUs reset at once.
>   * This is required so that any code running after reset on the primary CPU
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 7736779..a7c3fce 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -76,6 +76,40 @@ ENTRY(cpu_reset)
>  	ret	x0
>  ENDPROC(cpu_reset)
>  
> +	.align 3
> +1:	.quad	memstart_addr
> +
> +ENTRY(cpu_soft_restart)
> +	adr	x1, cpu_reset
> +	adr	x2, 1b
> +
> +	/* virt_to_phys(cpu_reset) */
> +	ldr	x3, [x2]
> +	ldr	x3, [x3]
> +	mov	x4, #1
> +	lsl	x4, x4, #(VA_BITS - 1)
> +	add	x1, x1, x4
> +	add	x1, x1, x3

Rather than having a custom virt_to_phys here, I think it would be
better to leave that address translation and table setup in c, and have
the asm be a smaller function which disables and cleans the cache, then
branches to the provided address.

So we'd still have a soft_restart function in c that looked something
like:

void soft_restart(unsigned long addr)
{
	setup_mm_for_reboot();
	cpu_soft_restart(virt_to_phys(cpu_reset), addr);
}

> +
> +	/* Save it; We can't use stack as it is going to run with caches OFF */
> +	mov	x19, x0
> +	mov	x20, x1
> +
> +	bl	setup_mm_for_reboot
> +
> +	bl	flush_cache_all

This first cache flush can be dropped entirely.

> +	/* Turn D-cache off */
> +	bl	cpu_cache_off
> +	/* Push out any further dirty data, and ensure cache is empty */
> +	bl	flush_cache_all
> +
> +	mov	x0, x19
> +
> +	br	x20
> +	/* It should never come back */
> +	bl	panic

Given we didn't use blr x20, won't we get stuck in an infinite loop here
if we returned from the address in x20?

I don't think panic will work here given we disabled the MMU and caches.
A branch to self is probably the best we can do if it's worth doing
anything.

Thanks,
Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

Hi Mark,

Thank you for reviewing the code.

On Tue, Aug 12, 2014 at 7:35 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Arun,
>
> On Tue, Aug 12, 2014 at 01:42:45PM +0100, Arun Chandran wrote:
>> soft_restart() will fail on arm64 systems that does not
>> quarantee the flushing of cache to PoC with flush_cache_all().
>
> The flush_cache_all function is never guaranteed to flush data to the
> PoC, so I wouldn't say that this is only broken on some systems. We just
> happen to have been lucky so far.
>
>> soft_restart(addr)
>> {
>>       push_to_stack(addr);
>>
>>       Do mm setup for restart;
>>       Flush&turnoff D-cache;
>>
>>       pop_from_stack(addr); --> fails here as addr is not at PoC
>>       cpu_reset(addr) --> Jumps to invalid address
>> }
>
> This doesn't match this organisation on soft_restart, which makes it a
> little confusing.
>
>>
>> So convert the whole code to assembly to make sure that addr
>> is not pushed to stack.
>
> It's possible that the compiler would previously have spilled other
> values too, we only observed addr being spilled.
>
> How about we reword this something like:
>
> The current soft_restart and setup_restart implementations incorrectly
> assume that the compiler will not spill/fill values to/from the stack,
> but the compiler has been observed to do so around the cache being
> disabled, resulting in stale values being restored.
>
That is better. I will send an updated patch soon.

> 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    |    1 +
>>  arch/arm64/include/asm/system_misc.h |    1 -
>>  arch/arm64/kernel/process.c          |   38 ++--------------------------------
>>  arch/arm64/mm/proc.S                 |   34 ++++++++++++++++++++++++++++++
>>  4 files changed, 37 insertions(+), 37 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/proc-fns.h b/arch/arm64/include/asm/proc-fns.h
>> index 0c657bb..e18d5d0 100644
>> --- a/arch/arm64/include/asm/proc-fns.h
>> +++ b/arch/arm64/include/asm/proc-fns.h
>> @@ -32,6 +32,7 @@ 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(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/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
>> index 7a18fab..659fbf5 100644
>> --- a/arch/arm64/include/asm/system_misc.h
>> +++ b/arch/arm64/include/asm/system_misc.h
>> @@ -41,7 +41,6 @@ struct mm_struct;
>>  extern void show_pte(struct mm_struct *mm, unsigned long addr);
>>  extern void __show_regs(struct pt_regs *);
>>
>> -void soft_restart(unsigned long);
>>  extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>>
>>  #define UDBG_UNDEFINED       (1 << 0)
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 1309d64..3ca1ade 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -57,40 +57,6 @@ 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);
>> -
>> -     /* Should never get here */
>> -     BUG();
>> -}
>> -
>>  /*
>>   * Function pointers to optional machine specific functions
>>   */
>> @@ -162,8 +128,8 @@ void machine_power_off(void)
>>
>>  /*
>>   * Restart requires that the secondary CPUs stop performing any activity
>> - * while the primary CPU resets the system. Systems with a single CPU can
>> - * use soft_restart() as their machine descriptor's .restart hook, since that
>> + * while the primary CPU resets the system. Systems with a single CPU can use
>> + * cpu_soft_restart() as their machine descriptor's .restart hook, since that
>>   * will cause the only available CPU to reset. Systems with multiple CPUs must
>>   * provide a HW restart implementation, to ensure that all CPUs reset at once.
>>   * This is required so that any code running after reset on the primary CPU
>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> index 7736779..a7c3fce 100644
>> --- a/arch/arm64/mm/proc.S
>> +++ b/arch/arm64/mm/proc.S
>> @@ -76,6 +76,40 @@ ENTRY(cpu_reset)
>>       ret     x0
>>  ENDPROC(cpu_reset)
>>
>> +     .align 3
>> +1:   .quad   memstart_addr
>> +
>> +ENTRY(cpu_soft_restart)
>> +     adr     x1, cpu_reset
>> +     adr     x2, 1b
>> +
>> +     /* virt_to_phys(cpu_reset) */
>> +     ldr     x3, [x2]
>> +     ldr     x3, [x3]
>> +     mov     x4, #1
>> +     lsl     x4, x4, #(VA_BITS - 1)
>> +     add     x1, x1, x4
>> +     add     x1, x1, x3
>
> Rather than having a custom virt_to_phys here, I think it would be
> better to leave that address translation and table setup in c, and have
> the asm be a smaller function which disables and cleans the cache, then
> branches to the provided address.
>
> So we'd still have a soft_restart function in c that looked something
> like:
>
> void soft_restart(unsigned long addr)
> {
>         setup_mm_for_reboot();
>         cpu_soft_restart(virt_to_phys(cpu_reset), addr);
> }
>

Ok. I also thought of it but my mind was stuck at converting soft_restart() to
assembly.

>> +
>> +     /* Save it; We can't use stack as it is going to run with caches OFF */
>> +     mov     x19, x0
>> +     mov     x20, x1
>> +
>> +     bl      setup_mm_for_reboot
>> +
>> +     bl      flush_cache_all
>
> This first cache flush can be dropped entirely.
>

Ok.
>> +     /* Turn D-cache off */
>> +     bl      cpu_cache_off
>> +     /* Push out any further dirty data, and ensure cache is empty */
>> +     bl      flush_cache_all
>> +
>> +     mov     x0, x19
>> +
>> +     br      x20
>> +     /* It should never come back */
>> +     bl      panic
>
> Given we didn't use blr x20, won't we get stuck in an infinite loop here
> if we returned from the address in x20?
>
> I don't think panic will work here given we disabled the MMU and caches.
> A branch to self is probably the best we can do if it's worth doing
> anything.
>

Ok.

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

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 soft_restart() to assembly code

[Geoff Levand]

Hi Arun,

On Tue, 2014-08-12 at 18:12 +0530, Arun Chandran wrote:
> soft_restart() will fail on arm64 systems that does not
> quarantee the flushing of cache to PoC with flush_cache_all().
> 
> soft_restart(addr)
> {
> 	push_to_stack(addr);
> 
> 	Do mm setup for restart;
> 	Flush&turnoff D-cache;
> 
> 	pop_from_stack(addr); --> fails here as addr is not at PoC
> 	cpu_reset(addr) --> Jumps to invalid address
> }

For the cpu-ops shutdown I'm working on I need a call to move the
secondary processors to an identity mapped spin loop after the identity
map is enabled.  I want to do this in C code, so it needs to happen
after the identity map is enabled, and before the dcache is disabled.

I think to do this we can keep the existing soft_restart(addr) routine
with something like this:

void soft_restart(unsigned long addr)
{
	setup_mm_for_reboot();

#if defined(CONFIG_SMP)
	smp_secondary_shutdown();
#endif

	cpu_soft_restart(addr);

	/* Should never get here */
	BUG();
}


> 
> So convert the whole code to assembly to make sure that addr
> is not pushed to stack.
> 
> Signed-off-by: Arun Chandran <achandran@mvista.com>
> ---
>  arch/arm64/include/asm/proc-fns.h    |    1 +
>  arch/arm64/include/asm/system_misc.h |    1 -
>  arch/arm64/kernel/process.c          |   38 ++--------------------------------
>  arch/arm64/mm/proc.S                 |   34 ++++++++++++++++++++++++++++++
>  4 files changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/proc-fns.h b/arch/arm64/include/asm/proc-fns.h
> index 0c657bb..e18d5d0 100644
> --- a/arch/arm64/include/asm/proc-fns.h
> +++ b/arch/arm64/include/asm/proc-fns.h
> @@ -32,6 +32,7 @@ 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(unsigned long addr) __attribute__((noreturn));

Function prototypes are never definitions, so remove this 'extern'
keyword.  checkpatch should have warned about this.  If it did not,
report it to the checkpatch maintainers.

>  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/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
> index 7a18fab..659fbf5 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -41,7 +41,6 @@ struct mm_struct;
>  extern void show_pte(struct mm_struct *mm, unsigned long addr);
>  extern void __show_regs(struct pt_regs *);
>  
> -void soft_restart(unsigned long);
>  extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>  
>  #define UDBG_UNDEFINED	(1 << 0)
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 1309d64..3ca1ade 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -57,40 +57,6 @@ 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);
> -
> -	/* Should never get here */
> -	BUG();
> -}
> -
>  /*
>   * Function pointers to optional machine specific functions
>   */
> @@ -162,8 +128,8 @@ void machine_power_off(void)
>  
>  /*
>   * Restart requires that the secondary CPUs stop performing any activity
> - * while the primary CPU resets the system. Systems with a single CPU can
> - * use soft_restart() as their machine descriptor's .restart hook, since that
> + * while the primary CPU resets the system. Systems with a single CPU can use
> + * cpu_soft_restart() as their machine descriptor's .restart hook, since that
>   * will cause the only available CPU to reset. Systems with multiple CPUs must
>   * provide a HW restart implementation, to ensure that all CPUs reset at once.
>   * This is required so that any code running after reset on the primary CPU
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 7736779..a7c3fce 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -76,6 +76,40 @@ ENTRY(cpu_reset)
>  	ret	x0
>  ENDPROC(cpu_reset)
>  
> +	.align 3
> +1:	.quad	memstart_addr
> +
> +ENTRY(cpu_soft_restart)
> +	adr	x1, cpu_reset
> +	adr	x2, 1b
> +
> +	/* virt_to_phys(cpu_reset) */
> +	ldr	x3, [x2]
> +	ldr	x3, [x3]
> +	mov	x4, #1
> +	lsl	x4, x4, #(VA_BITS - 1)
> +	add	x1, x1, x4
> +	add	x1, x1, x3
> +
> +	/* Save it; We can't use stack as it is going to run with caches OFF */
> +	mov	x19, x0
> +	mov	x20, x1
> +
> +	bl	setup_mm_for_reboot
> +
> +	bl	flush_cache_all
> +	/* Turn D-cache off */
> +	bl	cpu_cache_off
> +	/* Push out any further dirty data, and ensure cache is empty */
> +	bl	flush_cache_all

It would be nice to have some blank lines above the comments.  Same
below.

> +	mov	x0, x19
> +
> +	br	x20
> +	/* It should never come back */
> +	bl	panic
> +ENDPROC(cpu_soft_restart)
> +
>  /*
>   *	cpu_do_idle()
>   *

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

Hi Geoff,

On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
> Hi Arun,
> 
> On Tue, 2014-08-12 at 18:12 +0530, Arun Chandran wrote:
> > soft_restart() will fail on arm64 systems that does not
> > quarantee the flushing of cache to PoC with flush_cache_all().
> > 
> > soft_restart(addr)
> > {
> > 	push_to_stack(addr);
> > 
> > 	Do mm setup for restart;
> > 	Flush&turnoff D-cache;
> > 
> > 	pop_from_stack(addr); --> fails here as addr is not at PoC
> > 	cpu_reset(addr) --> Jumps to invalid address
> > }
> 
> For the cpu-ops shutdown I'm working on I need a call to move the
> secondary processors to an identity mapped spin loop after the identity
> map is enabled.  I want to do this in C code, so it needs to happen
> after the identity map is enabled, and before the dcache is disabled.
> 
> I think to do this we can keep the existing soft_restart(addr) routine
> with something like this:
> 
> void soft_restart(unsigned long addr)
> {
> 	setup_mm_for_reboot();
> 
> #if defined(CONFIG_SMP)
> 	smp_secondary_shutdown();
> #endif
> 
> 	cpu_soft_restart(addr);
> 
> 	/* Should never get here */
> 	BUG();
> }
> 

I don't follow why you need a hook in the middle of soft_restart. That
sounds like a layering violation to me.

I assume this is for implementing the spin-table cpu-return-addr idea?

If so, what's wrong with something like:

#define ADDR_INVALID ((unsigned long)-1)
static DEFINE_PER_CPU(unsigned long, return_addr) = ADDR_INVALID;

int spin_table_cpu_disable(unsigned int cpu)
{
	if (per_cpu(return_addr, cpu) != ADDR_INVALID)
		return 0;
	
	return -EOPNOTSUPP;
}

void spin_table_cpu_die(unsigned int cpu)
{
	unsigned long release_addr = per_cpu(return_addr, cpu);
	
	/*
	 * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
	 * something similar as these are all context synchronising and
	 * therefore expensive.
	 */
	local_dbg_disable();
	local_async_disable();
	local_fiq_disable();
	arch_local_irq_disable();

	soft_restart(release_addr);
}

[...]

> > diff --git a/arch/arm64/include/asm/proc-fns.h b/arch/arm64/include/asm/proc-fns.h
> > index 0c657bb..e18d5d0 100644
> > --- a/arch/arm64/include/asm/proc-fns.h
> > +++ b/arch/arm64/include/asm/proc-fns.h
> > @@ -32,6 +32,7 @@ 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(unsigned long addr) __attribute__((noreturn));
> 
> Function prototypes are never definitions, so remove this 'extern'
> keyword.  checkpatch should have warned about this.  If it did not,
> report it to the checkpatch maintainers.

Good point.

Arun, could you fix up the latest version [1] of your patch to not use
extern for the function declaration?

If you'd be willing to spin a preparatory patch removing the other
externs on function declarations in asm/proc-fns.h, that would be
appreciated. Remember to add a Reported-by for Geoff.

Also, please remember to use a version number in the patch subject (e.g.
"[PATCHv2] arm64: convert part of soft_restart() to assembly"), as that
will make it easier to find the latest version in future.

[...]

> > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> > index 7736779..a7c3fce 100644
> > --- a/arch/arm64/mm/proc.S
> > +++ b/arch/arm64/mm/proc.S
> > @@ -76,6 +76,40 @@ ENTRY(cpu_reset)
> >  	ret	x0
> >  ENDPROC(cpu_reset)
> >  
> > +	.align 3
> > +1:	.quad	memstart_addr
> > +
> > +ENTRY(cpu_soft_restart)
> > +	adr	x1, cpu_reset
> > +	adr	x2, 1b
> > +
> > +	/* virt_to_phys(cpu_reset) */
> > +	ldr	x3, [x2]
> > +	ldr	x3, [x3]
> > +	mov	x4, #1
> > +	lsl	x4, x4, #(VA_BITS - 1)
> > +	add	x1, x1, x4
> > +	add	x1, x1, x3
> > +
> > +	/* Save it; We can't use stack as it is going to run with caches OFF */
> > +	mov	x19, x0
> > +	mov	x20, x1
> > +
> > +	bl	setup_mm_for_reboot
> > +
> > +	bl	flush_cache_all
> > +	/* Turn D-cache off */
> > +	bl	cpu_cache_off
> > +	/* Push out any further dirty data, and ensure cache is empty */
> > +	bl	flush_cache_all
> 
> It would be nice to have some blank lines above the comments.  Same
> below.

Makes sense to me. For the latest version [1], that should only mean a
line after the call to cpu_cache_off in cpu_soft_restart.

Cheers,
Mark.

[1] lists.infradead.org/pipermail/linux-arm-kernel/2014-August/279390.html

[PATCH] Arm64: convert soft_restart() to assembly code

[Geoff Levand]

Hi Mark,

On Fri, 2014-08-15 at 19:21 +0100, Mark Rutland wrote:
> On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
> > For the cpu-ops shutdown I'm working on I need a call to move the
> > secondary processors to an identity mapped spin loop after the identity
> > map is enabled.  I want to do this in C code, so it needs to happen
> > after the identity map is enabled, and before the dcache is disabled.
> > 
> > I think to do this we can keep the existing soft_restart(addr) routine
> > with something like this:
> > 
> > void soft_restart(unsigned long addr)
> > {
> > 	setup_mm_for_reboot();
> > 
> > #if defined(CONFIG_SMP)
> > 	smp_secondary_shutdown();
> > #endif
> > 
> > 	cpu_soft_restart(addr);
> > 
> > 	/* Should never get here */
> > 	BUG();
> > }
> > 
> 
> I don't follow why you need a hook in the middle of soft_restart. That
> sounds like a layering violation to me.
> 
> I assume this is for implementing the spin-table cpu-return-addr idea?

Yes.

> If so, what's wrong with something like:

> void spin_table_cpu_die(unsigned int cpu)
> {
> 	unsigned long release_addr = per_cpu(return_addr, cpu);
> 	
> 	/*
> 	 * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
> 	 * something similar as these are all context synchronising and
> 	 * therefore expensive.
> 	 */
> 	local_dbg_disable();
> 	local_async_disable();
> 	local_fiq_disable();
> 	arch_local_irq_disable();
> 
> 	soft_restart(release_addr);
> }

OK, this is a much simpler way than what I was thinking, which
was to have the secondaries spin in the kernel until the main
cpu shutdown.  I'll switch over to this, thanks.

-Geoff

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

Hi Geoff,

On Fri, Aug 15, 2014 at 07:53:19PM +0100, Geoff Levand wrote:
> Hi Mark,
> 
> On Fri, 2014-08-15 at 19:21 +0100, Mark Rutland wrote:
> > On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
> > > For the cpu-ops shutdown I'm working on I need a call to move the
> > > secondary processors to an identity mapped spin loop after the identity
> > > map is enabled.  I want to do this in C code, so it needs to happen
> > > after the identity map is enabled, and before the dcache is disabled.
> > > 
> > > I think to do this we can keep the existing soft_restart(addr) routine
> > > with something like this:
> > > 
> > > void soft_restart(unsigned long addr)
> > > {
> > > 	setup_mm_for_reboot();
> > > 
> > > #if defined(CONFIG_SMP)
> > > 	smp_secondary_shutdown();
> > > #endif
> > > 
> > > 	cpu_soft_restart(addr);
> > > 
> > > 	/* Should never get here */
> > > 	BUG();
> > > }
> > > 
> > 
> > I don't follow why you need a hook in the middle of soft_restart. That
> > sounds like a layering violation to me.
> > 
> > I assume this is for implementing the spin-table cpu-return-addr idea?
> 
> Yes.
> 
> > If so, what's wrong with something like:
> 
> > void spin_table_cpu_die(unsigned int cpu)
> > {
> > 	unsigned long release_addr = per_cpu(return_addr, cpu);
> > 	
> > 	/*
> > 	 * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
> > 	 * something similar as these are all context synchronising and
> > 	 * therefore expensive.
> > 	 */
> > 	local_dbg_disable();
> > 	local_async_disable();
> > 	local_fiq_disable();
> > 	arch_local_irq_disable();
> > 
> > 	soft_restart(release_addr);
> > }
> 
> OK, this is a much simpler way than what I was thinking, which
> was to have the secondaries spin in the kernel until the main
> cpu shutdown.  I'll switch over to this, thanks.

I just realised that this is still missing the jump to EL2 that I
mentioned a while back.

I think what we need to do is:

* Have KVM (if present) tears itself down prior to cpu_die, restoring
  the __hyp_stub_vectors in VBAR_EL2 and disabling the MMU, and caches.
  
* Add a mechanism to __hyp_stub_vectors to allow a hypercall to
  call a function at EL2. We should be able to replace the current
  hyp_stub el1_sync handler with that, and rework KVM to call a function
  at EL2 to setup VBAR_EL2 appropriately at init time.

* Depending on whether EL2 is available, go via soft_restart or the
  hypercall to cpu_soft_restart (or something very close to it).

How does that sound?

Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Christoffer Dall]

On Mon, Aug 18, 2014 at 6:02 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Geoff,
>
> On Fri, Aug 15, 2014 at 07:53:19PM +0100, Geoff Levand wrote:
>> Hi Mark,
>>
>> On Fri, 2014-08-15 at 19:21 +0100, Mark Rutland wrote:
>> > On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> > > For the cpu-ops shutdown I'm working on I need a call to move the
>> > > secondary processors to an identity mapped spin loop after the identity
>> > > map is enabled.  I want to do this in C code, so it needs to happen
>> > > after the identity map is enabled, and before the dcache is disabled.
>> > >
>> > > I think to do this we can keep the existing soft_restart(addr) routine
>> > > with something like this:
>> > >
>> > > void soft_restart(unsigned long addr)
>> > > {
>> > >   setup_mm_for_reboot();
>> > >
>> > > #if defined(CONFIG_SMP)
>> > >   smp_secondary_shutdown();
>> > > #endif
>> > >
>> > >   cpu_soft_restart(addr);
>> > >
>> > >   /* Should never get here */
>> > >   BUG();
>> > > }
>> > >
>> >
>> > I don't follow why you need a hook in the middle of soft_restart. That
>> > sounds like a layering violation to me.
>> >
>> > I assume this is for implementing the spin-table cpu-return-addr idea?
>>
>> Yes.
>>
>> > If so, what's wrong with something like:
>>
>> > void spin_table_cpu_die(unsigned int cpu)
>> > {
>> >     unsigned long release_addr = per_cpu(return_addr, cpu);
>> >
>> >     /*
>> >      * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
>> >      * something similar as these are all context synchronising and
>> >      * therefore expensive.
>> >      */
>> >     local_dbg_disable();
>> >     local_async_disable();
>> >     local_fiq_disable();
>> >     arch_local_irq_disable();
>> >
>> >     soft_restart(release_addr);
>> > }
>>
>> OK, this is a much simpler way than what I was thinking, which
>> was to have the secondaries spin in the kernel until the main
>> cpu shutdown.  I'll switch over to this, thanks.
>
> I just realised that this is still missing the jump to EL2 that I
> mentioned a while back.
>
> I think what we need to do is:
>
> * Have KVM (if present) tears itself down prior to cpu_die, restoring
>   the __hyp_stub_vectors in VBAR_EL2 and disabling the MMU, and caches.
>
> * Add a mechanism to __hyp_stub_vectors to allow a hypercall to
>   call a function at EL2. We should be able to replace the current
>   hyp_stub el1_sync handler with that, and rework KVM to call a function
>   at EL2 to setup VBAR_EL2 appropriately at init time.
>
Why do you need to change the current mechanism?  Is this due to the
CPU being in a different state when restarted with the MMU enabled in
EL2 or something like that?

-Christoffer

[PATCH] Arm64: convert soft_restart() to assembly code

[Geoff Levand]

Hi Christoffer,

On Mon, 2014-08-18 at 19:33 +0200, Christoffer Dall wrote:
> On Mon, Aug 18, 2014 at 6:02 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > I just realised that this is still missing the jump to EL2 that I
> > mentioned a while back.
> >
> > I think what we need to do is:
> >
> > * Have KVM (if present) tears itself down prior to cpu_die, restoring
> >   the __hyp_stub_vectors in VBAR_EL2 and disabling the MMU, and caches.
> >
> > * Add a mechanism to __hyp_stub_vectors to allow a hypercall to
> >   call a function at EL2. We should be able to replace the current
> >   hyp_stub el1_sync handler with that, and rework KVM to call a function
> >   at EL2 to setup VBAR_EL2 appropriately at init time.
> >
> Why do you need to change the current mechanism?  Is this due to the
> CPU being in a different state when restarted with the MMU enabled in
> EL2 or something like that?

I haven't looked into the details of how to do it too closely yet,
but we need to handle the case where a 1st stage kernel that is not
configured with visualization support, needs to kexec re-boot to a
2nd stage kernel that will support visualization.

Also, the current arm64 bootwrapper program expects to be entered in
EL2 or higher, so a CPU running at EL1 needs to get back to EL2
before re-entering the bootwrapper program on a CPU hot-unplug.

-Geoff

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

On Mon, Aug 18, 2014 at 06:33:36PM +0100, Christoffer Dall wrote:
> On Mon, Aug 18, 2014 at 6:02 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi Geoff,
> >
> > On Fri, Aug 15, 2014 at 07:53:19PM +0100, Geoff Levand wrote:
> >> Hi Mark,
> >>
> >> On Fri, 2014-08-15 at 19:21 +0100, Mark Rutland wrote:
> >> > On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
> >> > > For the cpu-ops shutdown I'm working on I need a call to move the
> >> > > secondary processors to an identity mapped spin loop after the identity
> >> > > map is enabled.  I want to do this in C code, so it needs to happen
> >> > > after the identity map is enabled, and before the dcache is disabled.
> >> > >
> >> > > I think to do this we can keep the existing soft_restart(addr) routine
> >> > > with something like this:
> >> > >
> >> > > void soft_restart(unsigned long addr)
> >> > > {
> >> > >   setup_mm_for_reboot();
> >> > >
> >> > > #if defined(CONFIG_SMP)
> >> > >   smp_secondary_shutdown();
> >> > > #endif
> >> > >
> >> > >   cpu_soft_restart(addr);
> >> > >
> >> > >   /* Should never get here */
> >> > >   BUG();
> >> > > }
> >> > >
> >> >
> >> > I don't follow why you need a hook in the middle of soft_restart. That
> >> > sounds like a layering violation to me.
> >> >
> >> > I assume this is for implementing the spin-table cpu-return-addr idea?
> >>
> >> Yes.
> >>
> >> > If so, what's wrong with something like:
> >>
> >> > void spin_table_cpu_die(unsigned int cpu)
> >> > {
> >> >     unsigned long release_addr = per_cpu(return_addr, cpu);
> >> >
> >> >     /*
> >> >      * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
> >> >      * something similar as these are all context synchronising and
> >> >      * therefore expensive.
> >> >      */
> >> >     local_dbg_disable();
> >> >     local_async_disable();
> >> >     local_fiq_disable();
> >> >     arch_local_irq_disable();
> >> >
> >> >     soft_restart(release_addr);
> >> > }
> >>
> >> OK, this is a much simpler way than what I was thinking, which
> >> was to have the secondaries spin in the kernel until the main
> >> cpu shutdown.  I'll switch over to this, thanks.
> >
> > I just realised that this is still missing the jump to EL2 that I
> > mentioned a while back.
> >
> > I think what we need to do is:
> >
> > * Have KVM (if present) tears itself down prior to cpu_die, restoring
> >   the __hyp_stub_vectors in VBAR_EL2 and disabling the MMU, and caches.
> >
> > * Add a mechanism to __hyp_stub_vectors to allow a hypercall to
> >   call a function at EL2. We should be able to replace the current
> >   hyp_stub el1_sync handler with that, and rework KVM to call a function
> >   at EL2 to setup VBAR_EL2 appropriately at init time.
> >
> Why do you need to change the current mechanism?  Is this due to the
> CPU being in a different state when restarted with the MMU enabled in
> EL2 or something like that?

Something like that, yes.

For hotplug with spin-table we need to return CPUs to the spin-table in
the mode they entered to prevent mismatched modes when we throw those
CPUs back into the kernel. For kexec we need to move the final CPU up to
the mode it started in before we branch to the new kernel. If we don't
do that then we either get mismatched modes or lose the use of EL2.

Whatever mechanism we use for this needs to be independent of KVM. 
Ideally this would be in the hyp_stub vectors and we'd have KVM tear
itself down at EL2 and restore the hyp_stub before we offline a CPU.

I'd rather not have a custom set of EL2 vectors that the spin-table code
has to install via the curent mechanism, so IMO reworking the hyp stub
to implement a simple function call hypercall would be preferable.  KVM
can use that to set up its vectors and the spin-table and kexec code
could use to leave the kernel at EL2.

Cheers,
Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Christoffer Dall]

On Wed, Aug 20, 2014 at 12:48 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Aug 18, 2014 at 06:33:36PM +0100, Christoffer Dall wrote:
>> On Mon, Aug 18, 2014 at 6:02 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > Hi Geoff,
>> >
>> > On Fri, Aug 15, 2014 at 07:53:19PM +0100, Geoff Levand wrote:
>> >> Hi Mark,
>> >>
>> >> On Fri, 2014-08-15 at 19:21 +0100, Mark Rutland wrote:
>> >> > On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> >> > > For the cpu-ops shutdown I'm working on I need a call to move the
>> >> > > secondary processors to an identity mapped spin loop after the identity
>> >> > > map is enabled.  I want to do this in C code, so it needs to happen
>> >> > > after the identity map is enabled, and before the dcache is disabled.
>> >> > >
>> >> > > I think to do this we can keep the existing soft_restart(addr) routine
>> >> > > with something like this:
>> >> > >
>> >> > > void soft_restart(unsigned long addr)
>> >> > > {
>> >> > >   setup_mm_for_reboot();
>> >> > >
>> >> > > #if defined(CONFIG_SMP)
>> >> > >   smp_secondary_shutdown();
>> >> > > #endif
>> >> > >
>> >> > >   cpu_soft_restart(addr);
>> >> > >
>> >> > >   /* Should never get here */
>> >> > >   BUG();
>> >> > > }
>> >> > >
>> >> >
>> >> > I don't follow why you need a hook in the middle of soft_restart. That
>> >> > sounds like a layering violation to me.
>> >> >
>> >> > I assume this is for implementing the spin-table cpu-return-addr idea?
>> >>
>> >> Yes.
>> >>
>> >> > If so, what's wrong with something like:
>> >>
>> >> > void spin_table_cpu_die(unsigned int cpu)
>> >> > {
>> >> >     unsigned long release_addr = per_cpu(return_addr, cpu);
>> >> >
>> >> >     /*
>> >> >      * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
>> >> >      * something similar as these are all context synchronising and
>> >> >      * therefore expensive.
>> >> >      */
>> >> >     local_dbg_disable();
>> >> >     local_async_disable();
>> >> >     local_fiq_disable();
>> >> >     arch_local_irq_disable();
>> >> >
>> >> >     soft_restart(release_addr);
>> >> > }
>> >>
>> >> OK, this is a much simpler way than what I was thinking, which
>> >> was to have the secondaries spin in the kernel until the main
>> >> cpu shutdown.  I'll switch over to this, thanks.
>> >
>> > I just realised that this is still missing the jump to EL2 that I
>> > mentioned a while back.
>> >
>> > I think what we need to do is:
>> >
>> > * Have KVM (if present) tears itself down prior to cpu_die, restoring
>> >   the __hyp_stub_vectors in VBAR_EL2 and disabling the MMU, and caches.
>> >
>> > * Add a mechanism to __hyp_stub_vectors to allow a hypercall to
>> >   call a function at EL2. We should be able to replace the current
>> >   hyp_stub el1_sync handler with that, and rework KVM to call a function
>> >   at EL2 to setup VBAR_EL2 appropriately at init time.
>> >
>> Why do you need to change the current mechanism?  Is this due to the
>> CPU being in a different state when restarted with the MMU enabled in
>> EL2 or something like that?
>
> Something like that, yes.
>
> For hotplug with spin-table we need to return CPUs to the spin-table in
> the mode they entered to prevent mismatched modes when we throw those
> CPUs back into the kernel. For kexec we need to move the final CPU up to
> the mode it started in before we branch to the new kernel. If we don't
> do that then we either get mismatched modes or lose the use of EL2.
>
> Whatever mechanism we use for this needs to be independent of KVM.
> Ideally this would be in the hyp_stub vectors and we'd have KVM tear
> itself down at EL2 and restore the hyp_stub before we offline a CPU.
>
> I'd rather not have a custom set of EL2 vectors that the spin-table code
> has to install via the curent mechanism, so IMO reworking the hyp stub
> to implement a simple function call hypercall would be preferable.  KVM
> can use that to set up its vectors and the spin-table and kexec code
> could use to leave the kernel at EL2.
>
So you'd still always assume the hyp-stub mechanism has the MMU turned
off at EL2, but just make it easier for callers to deal with,
essentially. As far as I can tell, there shouldn't be any problems
converting the hyp-stub API to specify a function to call in EL2
rather than the current method of replacing the vectors.

Letting KVM tear itself down and re-establish the hyp-stub API as it
was at boot time seems completely reasonable to me.

-Christoffer

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

[...]

> >> > I just realised that this is still missing the jump to EL2 that I
> >> > mentioned a while back.
> >> >
> >> > I think what we need to do is:
> >> >
> >> > * Have KVM (if present) tears itself down prior to cpu_die, restoring
> >> >   the __hyp_stub_vectors in VBAR_EL2 and disabling the MMU, and caches.
> >> >
> >> > * Add a mechanism to __hyp_stub_vectors to allow a hypercall to
> >> >   call a function at EL2. We should be able to replace the current
> >> >   hyp_stub el1_sync handler with that, and rework KVM to call a function
> >> >   at EL2 to setup VBAR_EL2 appropriately at init time.
> >> >
> >> Why do you need to change the current mechanism?  Is this due to the
> >> CPU being in a different state when restarted with the MMU enabled in
> >> EL2 or something like that?
> >
> > Something like that, yes.
> >
> > For hotplug with spin-table we need to return CPUs to the spin-table in
> > the mode they entered to prevent mismatched modes when we throw those
> > CPUs back into the kernel. For kexec we need to move the final CPU up to
> > the mode it started in before we branch to the new kernel. If we don't
> > do that then we either get mismatched modes or lose the use of EL2.
> >
> > Whatever mechanism we use for this needs to be independent of KVM.
> > Ideally this would be in the hyp_stub vectors and we'd have KVM tear
> > itself down at EL2 and restore the hyp_stub before we offline a CPU.
> >
> > I'd rather not have a custom set of EL2 vectors that the spin-table code
> > has to install via the curent mechanism, so IMO reworking the hyp stub
> > to implement a simple function call hypercall would be preferable.  KVM
> > can use that to set up its vectors and the spin-table and kexec code
> > could use to leave the kernel at EL2.
> >
> So you'd still always assume the hyp-stub mechanism has the MMU turned
> off at EL2, but just make it easier for callers to deal with,
> essentially.

Yes. I'd only expect this would be used by a few assembly functions that
would assume a very bare EL2 (only expecting what we initialize in
el2_setup). Having a function call hypercall just makes it easier for
callers and prevents a proliferation of temporary EL2 vectors.

> As far as I can tell, there shouldn't be any problems
> converting the hyp-stub API to specify a function to call in EL2
> rather than the current method of replacing the vectors.
> 
> Letting KVM tear itself down and re-establish the hyp-stub API as it
> was at boot time seems completely reasonable to me.

That's exactly what I was hoping to hear. :)

Now the only thing to figure out is what that tear-down hangs off of.

I thought that would go in kvm_arch_hardware_disable, but it doesn't
look like we use either of kvm_arch_hardware_{enable,disable}. I guess
that would fall in the hyp_init_cpu_notify notifier call.

Is there a reason we have our own notifier rather than using the
kvm_arch_hardware_{enable,disable} calls?

Cheers,
Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

Hi Mark

On Mon, Aug 18, 2014 at 9:32 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Geoff,
>
> On Fri, Aug 15, 2014 at 07:53:19PM +0100, Geoff Levand wrote:
>> Hi Mark,
>>
>> On Fri, 2014-08-15 at 19:21 +0100, Mark Rutland wrote:
>> > On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> > > For the cpu-ops shutdown I'm working on I need a call to move the
>> > > secondary processors to an identity mapped spin loop after the identity
>> > > map is enabled.  I want to do this in C code, so it needs to happen
>> > > after the identity map is enabled, and before the dcache is disabled.
>> > >
>> > > I think to do this we can keep the existing soft_restart(addr) routine
>> > > with something like this:
>> > >
>> > > void soft_restart(unsigned long addr)
>> > > {
>> > >   setup_mm_for_reboot();
>> > >
>> > > #if defined(CONFIG_SMP)
>> > >   smp_secondary_shutdown();
>> > > #endif
>> > >
>> > >   cpu_soft_restart(addr);
>> > >
>> > >   /* Should never get here */
>> > >   BUG();
>> > > }
>> > >
>> >
>> > I don't follow why you need a hook in the middle of soft_restart. That
>> > sounds like a layering violation to me.
>> >
>> > I assume this is for implementing the spin-table cpu-return-addr idea?
>>
>> Yes.
>>
>> > If so, what's wrong with something like:
>>
>> > void spin_table_cpu_die(unsigned int cpu)
>> > {
>> >     unsigned long release_addr = per_cpu(return_addr, cpu);
>> >
>> >     /*
>> >      * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
>> >      * something similar as these are all context synchronising and
>> >      * therefore expensive.
>> >      */
>> >     local_dbg_disable();
>> >     local_async_disable();
>> >     local_fiq_disable();
>> >     arch_local_irq_disable();
>> >
>> >     soft_restart(release_addr);
>> > }
>>
>> OK, this is a much simpler way than what I was thinking, which
>> was to have the secondaries spin in the kernel until the main
>> cpu shutdown.  I'll switch over to this, thanks.
>
> I just realised that this is still missing the jump to EL2 that I
> mentioned a while back.
>
> I think what we need to do is:
>
> * Have KVM (if present) tears itself down prior to cpu_die, restoring
>   the __hyp_stub_vectors in VBAR_EL2 and disabling the MMU, and caches.
>
> * Add a mechanism to __hyp_stub_vectors to allow a hypercall to
>   call a function at EL2. We should be able to replace the current
>   hyp_stub el1_sync handler with that, and rework KVM to call a function
>   at EL2 to setup VBAR_EL2 appropriately at init time.
>
> * Depending on whether EL2 is available, go via soft_restart or the
>   hypercall to cpu_soft_restart (or something very close to it).
>
> How does that sound?

Hi Mark,

What about the implementation below?

##############
diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
index 4371f45..799ca8a 100644
--- a/arch/arm/include/asm/virt.h
+++ b/arch/arm/include/asm/virt.h
@@ -52,7 +52,7 @@ static inline void sync_boot_mode(void)
        sync_cache_r(&__boot_cpu_mode);
 }

-void __hyp_set_vectors(unsigned long phys_vector_base);
+void __hyp_func_call(unsigned long addr);
 unsigned long __hyp_get_vectors(void);
 #else
 #define __boot_cpu_mode        (SVC_MODE)
diff --git a/arch/arm64/include/asm/proc-fns.h
b/arch/arm64/include/asm/proc-fns.h
index ddbc3f5..40d3360 100644
--- a/arch/arm64/include/asm/proc-fns.h
+++ b/arch/arm64/include/asm/proc-fns.h
@@ -31,7 +31,7 @@ struct cpu_suspend_ctx;
 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_reset(unsigned long addr, unsigned long boot_mode)
__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/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
index a272f33..4dd86b4 100644
--- a/arch/arm64/kernel/hyp-stub.S
+++ b/arch/arm64/kernel/hyp-stub.S
@@ -54,13 +54,20 @@ ENDPROC(__hyp_stub_vectors)

 el1_sync:
        mrs     x1, esr_el2
-       lsr     x1, x1, #26
-       cmp     x1, #0x16
+       lsr     x2, x1, #26
+       cmp     x2, #0x16
        b.ne    2f                              // Not an HVC trap
-       cbz     x0, 1f
-       msr     vbar_el2, x0                    // Set vbar_el2
+
+       and     x1, x1, #1                      // x1=1 -> Func call;
x1=0 -> get_vectors
+       cbnz    x1, 1f
+       mrs     x0, vbar_el2                    // Return vbar_el2
        b       2f
-1:     mrs     x0, vbar_el2                    // Return vbar_el2
+
+       /* Fluch I-cache before calling function @x0 */
+1:     ic      ialluis
+       dsb     sy
+       isb
+       blr     x0
 2:     eret
 ENDPROC(el1_sync)

@@ -101,10 +108,12 @@ ENDPROC(\label)
  */

 ENTRY(__hyp_get_vectors)
-       mov     x0, xzr
-       // fall through
-ENTRY(__hyp_set_vectors)
        hvc     #0
        ret
 ENDPROC(__hyp_get_vectors)
-ENDPROC(__hyp_set_vectors)
+
+/* Call a function @x0 */
+ENTRY(__hyp_func_call)
+       hvc     #1
+       ret
+ENDPROC(__hyp_func_call)
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 64733d4..77298c2 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -65,6 +65,14 @@ void soft_restart(unsigned long addr)
 //     smp_secondary_shutdown();
 #endif

+       /* Delay primary cpu; allow enough time for
+        * secondaries to enter spin loop
+        */
+#if defined(CONFIG_SMP)
+       if (smp_processor_id() == 0)
+               mdelay(1000);
+#endif
+
        cpu_soft_restart(virt_to_phys(cpu_reset), addr);

        /* Should never get here */

diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 3cb6dec..74e11c2 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -25,6 +25,7 @@
 #include <asm/hwcap.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/pgtable.h>
+#include <asm/virt.h>

 #include "proc-macros.S"

@@ -69,19 +70,25 @@ ENDPROC(cpu_cache_off)
  */
        .align  5
 ENTRY(cpu_reset)
-       mrs     x1, sctlr_el1
-       bic     x1, x1, #1
-       msr     sctlr_el1, x1                   // disable the MMU
+       mrs     x2, sctlr_el1
+       bic     x2, x2, #1
+       msr     sctlr_el1, x2                   // disable the MMU
        isb
 #if defined(CONFIG_SMP)
 /*     bl      secondary_shutdown */
 #endif
+       sub     w1, w1, #BOOT_CPU_MODE_EL2
+       cbz     w1, __hyp_func_call
        ret     x0
 ENDPROC(cpu_reset)

 ENTRY(cpu_soft_restart)
+       ldr     x3, =__boot_cpu_mode
+       ldr     w2, [x3]
+
        mov     x19, x0
        mov     x20, x1
+       mov     w21, w2

        /* Turn D-cache off */
        bl      cpu_cache_off
@@ -89,6 +96,7 @@ ENTRY(cpu_soft_restart)
        bl      flush_cache_all

        mov     x0, x20
+       mov     w1, w21
        ret     x19
 ENDPROC(cpu_soft_restart)

###################

Am I anywhere close to your idea?

--Arun

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

On Mon, Aug 18, 2014 at 9:32 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Geoff,
>
> On Fri, Aug 15, 2014 at 07:53:19PM +0100, Geoff Levand wrote:
>> Hi Mark,
>>
>> On Fri, 2014-08-15 at 19:21 +0100, Mark Rutland wrote:
>> > On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> > > For the cpu-ops shutdown I'm working on I need a call to move the
>> > > secondary processors to an identity mapped spin loop after the identity
>> > > map is enabled.  I want to do this in C code, so it needs to happen
>> > > after the identity map is enabled, and before the dcache is disabled.
>> > >
>> > > I think to do this we can keep the existing soft_restart(addr) routine
>> > > with something like this:
>> > >
>> > > void soft_restart(unsigned long addr)
>> > > {
>> > >   setup_mm_for_reboot();
>> > >
>> > > #if defined(CONFIG_SMP)
>> > >   smp_secondary_shutdown();
>> > > #endif
>> > >
>> > >   cpu_soft_restart(addr);
>> > >
>> > >   /* Should never get here */
>> > >   BUG();
>> > > }
>> > >
>> >
>> > I don't follow why you need a hook in the middle of soft_restart. That
>> > sounds like a layering violation to me.
>> >
>> > I assume this is for implementing the spin-table cpu-return-addr idea?
>>
>> Yes.
>>
>> > If so, what's wrong with something like:
>>
>> > void spin_table_cpu_die(unsigned int cpu)
>> > {
>> >     unsigned long release_addr = per_cpu(return_addr, cpu);
>> >
>> >     /*
>> >      * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
>> >      * something similar as these are all context synchronising and
>> >      * therefore expensive.
>> >      */
>> >     local_dbg_disable();
>> >     local_async_disable();
>> >     local_fiq_disable();
>> >     arch_local_irq_disable();
>> >
>> >     soft_restart(release_addr);
>> > }
>>
>> OK, this is a much simpler way than what I was thinking, which
>> was to have the secondaries spin in the kernel until the main
>> cpu shutdown.  I'll switch over to this, thanks.
>
> I just realised that this is still missing the jump to EL2 that I
> mentioned a while back.
>
> I think what we need to do is:
>
> * Have KVM (if present) tears itself down prior to cpu_die, restoring
>   the __hyp_stub_vectors in VBAR_EL2 and disabling the MMU, and caches.
>
> * Add a mechanism to __hyp_stub_vectors to allow a hypercall to
>   call a function at EL2. We should be able to replace the current
>   hyp_stub el1_sync handler with that, and rework KVM to call a function
>   at EL2 to setup VBAR_EL2 appropriately at init time.
>
> * Depending on whether EL2 is available, go via soft_restart or the
>   hypercall to cpu_soft_restart (or something very close to it).
>
> How does that sound?

Hi Mark,

Please Ignore my previous mail. I think this version of sample
implementation is simple and better. Please give your comments.

###########
diff --git a/arch/arm64/include/asm/proc-fns.h
b/arch/arm64/include/asm/proc-fns.h
index ddbc3f5..40d3360 100644
--- a/arch/arm64/include/asm/proc-fns.h
+++ b/arch/arm64/include/asm/proc-fns.h
@@ -31,7 +31,7 @@ struct cpu_suspend_ctx;
 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_reset(unsigned long addr, unsigned long boot_mode)
__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/include/asm/virt.h b/arch/arm64/include/asm/virt.h
index 215ad46..3976737 100644
--- a/arch/arm64/include/asm/virt.h
+++ b/arch/arm64/include/asm/virt.h
@@ -34,6 +34,7 @@
  */
 extern u32 __boot_cpu_mode[2];

+void __hyp_func_call(phys_addr_t func, ...);
 void __hyp_set_vectors(phys_addr_t phys_vector_base);
 phys_addr_t __hyp_get_vectors(void);

diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
index a272f33..82c2b0d 100644
--- a/arch/arm64/kernel/hyp-stub.S
+++ b/arch/arm64/kernel/hyp-stub.S
@@ -53,14 +53,12 @@ ENDPROC(__hyp_stub_vectors)
        .align 11

 el1_sync:
-       mrs     x1, esr_el2
-       lsr     x1, x1, #26
-       cmp     x1, #0x16
+       mrs     x19, esr_el2
+       lsr     x19, x19, #26
+       cmp     x19, #0x16
        b.ne    2f                              // Not an HVC trap
-       cbz     x0, 1f
-       msr     vbar_el2, x0                    // Set vbar_el2
-       b       2f
-1:     mrs     x0, vbar_el2                    // Return vbar_el2
+
+1:     blr     x0                              // Jump to the function
 2:     eret
 ENDPROC(el1_sync)

@@ -101,10 +99,17 @@ ENDPROC(\label)
  */

 ENTRY(__hyp_get_vectors)
-       mov     x0, xzr
-       // fall through
-ENTRY(__hyp_set_vectors)
-       hvc     #0
+       mrs     x0, vbar_el2                    // Return vbar_el2
        ret
 ENDPROC(__hyp_get_vectors)
+
+ENTRY(__hyp_set_vectors)
+       msr     vbar_el2, x1
+       ret
 ENDPROC(__hyp_set_vectors)
+
+/* Call a function @x0 */
+ENTRY(__hyp_func_call)
+       hvc     #0
+       ret
+ENDPROC(__hyp_func_call)
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 3cb6dec..e68f42e 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -25,6 +25,7 @@
 #include <asm/hwcap.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/pgtable.h>
+#include <asm/virt.h>

 #include "proc-macros.S"

@@ -69,19 +70,26 @@ ENDPROC(cpu_cache_off)
  */
        .align  5
 ENTRY(cpu_reset)
-       mrs     x1, sctlr_el1
-       bic     x1, x1, #1
-       msr     sctlr_el1, x1                   // disable the MMU
+       mrs     x2, sctlr_el1
+       bic     x2, x2, #1
+       msr     sctlr_el1, x2                   // disable the MMU
        isb
 #if defined(CONFIG_SMP)
 /*     bl      secondary_shutdown */
 #endif
+       sub     w1, w1, #BOOT_CPU_MODE_EL2
+       cbz     w1, __hyp_func_call
        ret     x0
 ENDPROC(cpu_reset)

 ENTRY(cpu_soft_restart)
+       ldr     x3, =__boot_cpu_mode
+       add     x3, x3, #4
+       ldr     w2, [x3]
+
        mov     x19, x0
        mov     x20, x1
+       mov     w21, w2

        /* Turn D-cache off */
        bl      cpu_cache_off
@@ -89,6 +97,7 @@ ENTRY(cpu_soft_restart)
        bl      flush_cache_all

        mov     x0, x20
+       mov     w1, w21
        ret     x19
 ENDPROC(cpu_soft_restart)
#########

I have implemented __hyp_func_call; other userscan
make use of it to call __hyp_set_vectors/__hyp_get_vectors

--Arun

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

> Hi Mark,

Hi Arun,

> Please Ignore my previous mail. I think this version of sample
> implementation is simple and better. Please give your comments.

If you wish to have this reviewed in any depth, please:

 - Base your patches on mainline, or at the very least provide a pointer
   to the branch the patch is based upon.

 - Test that this functions with and without CONFIG_KVM.

 - Post this as an RFC PATCH, complete with a rationale (as previously
   discussed).

 - For later revisions, please label the patches with a version and give
   adequate delay between postings to allow for review.

I've given some comments below, but please post proper patches in
future.

> ###########
> diff --git a/arch/arm64/include/asm/proc-fns.h
> b/arch/arm64/include/asm/proc-fns.h
> index ddbc3f5..40d3360 100644
> --- a/arch/arm64/include/asm/proc-fns.h
> +++ b/arch/arm64/include/asm/proc-fns.h
> @@ -31,7 +31,7 @@ struct cpu_suspend_ctx;
>  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_reset(unsigned long addr, unsigned long boot_mode)
> __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/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> index 215ad46..3976737 100644
> --- a/arch/arm64/include/asm/virt.h
> +++ b/arch/arm64/include/asm/virt.h
> @@ -34,6 +34,7 @@
>   */
>  extern u32 __boot_cpu_mode[2];
> 
> +void __hyp_func_call(phys_addr_t func, ...);
>  void __hyp_set_vectors(phys_addr_t phys_vector_base);
>  phys_addr_t __hyp_get_vectors(void);
> 
> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
> index a272f33..82c2b0d 100644
> --- a/arch/arm64/kernel/hyp-stub.S
> +++ b/arch/arm64/kernel/hyp-stub.S
> @@ -53,14 +53,12 @@ ENDPROC(__hyp_stub_vectors)
>         .align 11
> 
>  el1_sync:
> -       mrs     x1, esr_el2
> -       lsr     x1, x1, #26
> -       cmp     x1, #0x16
> +       mrs     x19, esr_el2
> +       lsr     x19, x19, #26
> +       cmp     x19, #0x16
>         b.ne    2f                              // Not an HVC trap
> -       cbz     x0, 1f
> -       msr     vbar_el2, x0                    // Set vbar_el2
> -       b       2f
> -1:     mrs     x0, vbar_el2                    // Return vbar_el2
> +
> +1:     blr     x0                              // Jump to the function

You need to stash the orignal lr, or we can't return from
__hyp_call_func.

>  2:     eret
>  ENDPROC(el1_sync)
> 
> @@ -101,10 +99,17 @@ ENDPROC(\label)
>   */
> 
>  ENTRY(__hyp_get_vectors)
> -       mov     x0, xzr
> -       // fall through
> -ENTRY(__hyp_set_vectors)
> -       hvc     #0
> +       mrs     x0, vbar_el2                    // Return vbar_el2
>         ret
>  ENDPROC(__hyp_get_vectors)
> +
> +ENTRY(__hyp_set_vectors)
> +       msr     vbar_el2, x1
> +       ret
>  ENDPROC(__hyp_set_vectors)
> +
> +/* Call a function @x0 */
> +ENTRY(__hyp_func_call)
> +       hvc     #0
> +       ret
> +ENDPROC(__hyp_func_call)

These will be called at EL1, so this breaks KVM.

> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 3cb6dec..e68f42e 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -25,6 +25,7 @@
>  #include <asm/hwcap.h>
>  #include <asm/pgtable-hwdef.h>
>  #include <asm/pgtable.h>
> +#include <asm/virt.h>
> 
>  #include "proc-macros.S"
> 
> @@ -69,19 +70,26 @@ ENDPROC(cpu_cache_off)
>   */
>         .align  5
>  ENTRY(cpu_reset)
> -       mrs     x1, sctlr_el1
> -       bic     x1, x1, #1
> -       msr     sctlr_el1, x1                   // disable the MMU
> +       mrs     x2, sctlr_el1
> +       bic     x2, x2, #1
> +       msr     sctlr_el1, x2                   // disable the MMU
>         isb
>  #if defined(CONFIG_SMP)
>  /*     bl      secondary_shutdown */
>  #endif
> +       sub     w1, w1, #BOOT_CPU_MODE_EL2
> +       cbz     w1, __hyp_func_call
>         ret     x0
>  ENDPROC(cpu_reset)

Please base your patches on mainline (or if you need the fixed-up
soft_restart, the arm64 devel branch).

I thought we'd shot down the idea of the secondary_shutdown call.

>  ENTRY(cpu_soft_restart)
> +       ldr     x3, =__boot_cpu_mode
> +       add     x3, x3, #4
> +       ldr     w2, [x3]
> +
>         mov     x19, x0
>         mov     x20, x1
> +       mov     w21, w2
> 
>         /* Turn D-cache off */
>         bl      cpu_cache_off
> @@ -89,6 +97,7 @@ ENTRY(cpu_soft_restart)
>         bl      flush_cache_all
> 
>         mov     x0, x20
> +       mov     w1, w21
>         ret     x19
>  ENDPROC(cpu_soft_restart)
> #########
> 
> I have implemented __hyp_func_call; other userscan
> make use of it to call __hyp_set_vectors/__hyp_get_vectors

The callers of those functions _must_ be updated in this patch to use
the new hypercall mechanism.

Thanks,
Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

Hi Mark,

On Tue, Aug 26, 2014 at 8:52 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi Mark,
>
> Hi Arun,
>
>> Please Ignore my previous mail. I think this version of sample
>> implementation is simple and better. Please give your comments.
>
> If you wish to have this reviewed in any depth, please:
>
>  - Base your patches on mainline, or at the very least provide a pointer
>    to the branch the patch is based upon.
>
>  - Test that this functions with and without CONFIG_KVM.
>
>  - Post this as an RFC PATCH, complete with a rationale (as previously
>    discussed).
>
>  - For later revisions, please label the patches with a version and give
>    adequate delay between postings to allow for review.
>

Yes I will put a RFC patch soon.

> I've given some comments below, but please post proper patches in
> future.
>

My Intention was to do the same(RFC patch); but I did not wanted
to drift a lot from your idea that is why I sent it like below.

>> ###########
>> diff --git a/arch/arm64/include/asm/proc-fns.h
>> b/arch/arm64/include/asm/proc-fns.h
>> index ddbc3f5..40d3360 100644
>> --- a/arch/arm64/include/asm/proc-fns.h
>> +++ b/arch/arm64/include/asm/proc-fns.h
>> @@ -31,7 +31,7 @@ struct cpu_suspend_ctx;
>>  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_reset(unsigned long addr, unsigned long boot_mode)
>> __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/include/asm/virt.h b/arch/arm64/include/asm/virt.h
>> index 215ad46..3976737 100644
>> --- a/arch/arm64/include/asm/virt.h
>> +++ b/arch/arm64/include/asm/virt.h
>> @@ -34,6 +34,7 @@
>>   */
>>  extern u32 __boot_cpu_mode[2];
>>
>> +void __hyp_func_call(phys_addr_t func, ...);
>>  void __hyp_set_vectors(phys_addr_t phys_vector_base);
>>  phys_addr_t __hyp_get_vectors(void);
>>
>> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
>> index a272f33..82c2b0d 100644
>> --- a/arch/arm64/kernel/hyp-stub.S
>> +++ b/arch/arm64/kernel/hyp-stub.S
>> @@ -53,14 +53,12 @@ ENDPROC(__hyp_stub_vectors)
>>         .align 11
>>
>>  el1_sync:
>> -       mrs     x1, esr_el2
>> -       lsr     x1, x1, #26
>> -       cmp     x1, #0x16
>> +       mrs     x19, esr_el2
>> +       lsr     x19, x19, #26
>> +       cmp     x19, #0x16
>>         b.ne    2f                              // Not an HVC trap
>> -       cbz     x0, 1f
>> -       msr     vbar_el2, x0                    // Set vbar_el2
>> -       b       2f
>> -1:     mrs     x0, vbar_el2                    // Return vbar_el2
>> +
>> +1:     blr     x0                              // Jump to the function
>
> You need to stash the orignal lr, or we can't return from
> __hyp_call_func.

Ok.
>
>>  2:     eret
>>  ENDPROC(el1_sync)
>>
>> @@ -101,10 +99,17 @@ ENDPROC(\label)
>>   */
>>
>>  ENTRY(__hyp_get_vectors)
>> -       mov     x0, xzr
>> -       // fall through
>> -ENTRY(__hyp_set_vectors)
>> -       hvc     #0
>> +       mrs     x0, vbar_el2                    // Return vbar_el2
>>         ret
>>  ENDPROC(__hyp_get_vectors)
>> +
>> +ENTRY(__hyp_set_vectors)
>> +       msr     vbar_el2, x1
>> +       ret
>>  ENDPROC(__hyp_set_vectors)
>> +
>> +/* Call a function @x0 */
>> +ENTRY(__hyp_func_call)
>> +       hvc     #0
>> +       ret
>> +ENDPROC(__hyp_func_call)
>
> These will be called at EL1, so this breaks KVM.
>
>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> index 3cb6dec..e68f42e 100644
>> --- a/arch/arm64/mm/proc.S
>> +++ b/arch/arm64/mm/proc.S
>> @@ -25,6 +25,7 @@
>>  #include <asm/hwcap.h>
>>  #include <asm/pgtable-hwdef.h>
>>  #include <asm/pgtable.h>
>> +#include <asm/virt.h>
>>
>>  #include "proc-macros.S"
>>
>> @@ -69,19 +70,26 @@ ENDPROC(cpu_cache_off)
>>   */
>>         .align  5
>>  ENTRY(cpu_reset)
>> -       mrs     x1, sctlr_el1
>> -       bic     x1, x1, #1
>> -       msr     sctlr_el1, x1                   // disable the MMU
>> +       mrs     x2, sctlr_el1
>> +       bic     x2, x2, #1
>> +       msr     sctlr_el1, x2                   // disable the MMU
>>         isb
>>  #if defined(CONFIG_SMP)
>>  /*     bl      secondary_shutdown */
>>  #endif
>> +       sub     w1, w1, #BOOT_CPU_MODE_EL2
>> +       cbz     w1, __hyp_func_call
>>         ret     x0
>>  ENDPROC(cpu_reset)
>
> Please base your patches on mainline (or if you need the fixed-up
> soft_restart, the arm64 devel branch).
>
> I thought we'd shot down the idea of the secondary_shutdown call.
>
Yes. Please see it is commented.

>>  ENTRY(cpu_soft_restart)
>> +       ldr     x3, =__boot_cpu_mode
>> +       add     x3, x3, #4
>> +       ldr     w2, [x3]
>> +
>>         mov     x19, x0
>>         mov     x20, x1
>> +       mov     w21, w2
>>
>>         /* Turn D-cache off */
>>         bl      cpu_cache_off
>> @@ -89,6 +97,7 @@ ENTRY(cpu_soft_restart)
>>         bl      flush_cache_all
>>
>>         mov     x0, x20
>> +       mov     w1, w21
>>         ret     x19
>>  ENDPROC(cpu_soft_restart)
>> #########
>>
>> I have implemented __hyp_func_call; other userscan
>> make use of it to call __hyp_set_vectors/__hyp_get_vectors
>
> The callers of those functions _must_ be updated in this patch to use
> the new hypercall mechanism.

Ok.

--Arun

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

Hi Geoff, Mark,

On Fri, Aug 15, 2014 at 11:51 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Geoff,
>
> On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> Hi Arun,
>>
>> On Tue, 2014-08-12 at 18:12 +0530, Arun Chandran wrote:
>> > soft_restart() will fail on arm64 systems that does not
>> > quarantee the flushing of cache to PoC with flush_cache_all().
>> >
>> > soft_restart(addr)
>> > {
>> >     push_to_stack(addr);
>> >
>> >     Do mm setup for restart;
>> >     Flush&turnoff D-cache;
>> >
>> >     pop_from_stack(addr); --> fails here as addr is not at PoC
>> >     cpu_reset(addr) --> Jumps to invalid address
>> > }
>>
>> For the cpu-ops shutdown I'm working on I need a call to move the
>> secondary processors to an identity mapped spin loop after the identity
>> map is enabled.  I want to do this in C code, so it needs to happen
>> after the identity map is enabled, and before the dcache is disabled.
>>
>> I think to do this we can keep the existing soft_restart(addr) routine
>> with something like this:
>>
>> void soft_restart(unsigned long addr)
>> {
>>       setup_mm_for_reboot();
>>
>> #if defined(CONFIG_SMP)
>>       smp_secondary_shutdown();
>> #endif
>>
>>       cpu_soft_restart(addr);
>>
>>       /* Should never get here */
>>       BUG();
>> }
>>
>
> I don't follow why you need a hook in the middle of soft_restart. That
> sounds like a layering violation to me.
>
> I assume this is for implementing the spin-table cpu-return-addr idea?
>
> If so, what's wrong with something like:
>
> #define ADDR_INVALID ((unsigned long)-1)
> static DEFINE_PER_CPU(unsigned long, return_addr) = ADDR_INVALID;
>
> int spin_table_cpu_disable(unsigned int cpu)
> {
>         if (per_cpu(return_addr, cpu) != ADDR_INVALID)
>                 return 0;
>
>         return -EOPNOTSUPP;
> }
>
> void spin_table_cpu_die(unsigned int cpu)
> {
>         unsigned long release_addr = per_cpu(return_addr, cpu);
>
>         /*
>          * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
>          * something similar as these are all context synchronising and
>          * therefore expensive.
>          */
>         local_dbg_disable();
>         local_async_disable();
>         local_fiq_disable();
>         arch_local_irq_disable();
>
>         soft_restart(release_addr);
> }
>
> [...]
>
>> > diff --git a/arch/arm64/include/asm/proc-fns.h b/arch/arm64/include/asm/proc-fns.h
>> > index 0c657bb..e18d5d0 100644
>> > --- a/arch/arm64/include/asm/proc-fns.h
>> > +++ b/arch/arm64/include/asm/proc-fns.h
>> > @@ -32,6 +32,7 @@ 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(unsigned long addr) __attribute__((noreturn));
>>
>> Function prototypes are never definitions, so remove this 'extern'
>> keyword.  checkpatch should have warned about this.  If it did not,
>> report it to the checkpatch maintainers.
>
> Good point.
>
> Arun, could you fix up the latest version [1] of your patch to not use
> extern for the function declaration?

Yes I will fix and name it as V1 and send.
>
> If you'd be willing to spin a preparatory patch removing the other
> externs on function declarations in asm/proc-fns.h, that would be
> appreciated. Remember to add a Reported-by for Geoff.
>
Yes I will be happy to do that :)

> Also, please remember to use a version number in the patch subject (e.g.
> "[PATCHv2] arm64: convert part of soft_restart() to assembly"), as that
> will make it easier to find the latest version in future.
>
OK. Done.

> [...]
>
>> > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> > index 7736779..a7c3fce 100644
>> > --- a/arch/arm64/mm/proc.S
>> > +++ b/arch/arm64/mm/proc.S
>> > @@ -76,6 +76,40 @@ ENTRY(cpu_reset)
>> >     ret     x0
>> >  ENDPROC(cpu_reset)
>> >
>> > +   .align 3
>> > +1: .quad   memstart_addr
>> > +
>> > +ENTRY(cpu_soft_restart)
>> > +   adr     x1, cpu_reset
>> > +   adr     x2, 1b
>> > +
>> > +   /* virt_to_phys(cpu_reset) */
>> > +   ldr     x3, [x2]
>> > +   ldr     x3, [x3]
>> > +   mov     x4, #1
>> > +   lsl     x4, x4, #(VA_BITS - 1)
>> > +   add     x1, x1, x4
>> > +   add     x1, x1, x3
>> > +
>> > +   /* Save it; We can't use stack as it is going to run with caches OFF */
>> > +   mov     x19, x0
>> > +   mov     x20, x1
>> > +
>> > +   bl      setup_mm_for_reboot
>> > +
>> > +   bl      flush_cache_all
>> > +   /* Turn D-cache off */
>> > +   bl      cpu_cache_off
>> > +   /* Push out any further dirty data, and ensure cache is empty */
>> > +   bl      flush_cache_all
>>
>> It would be nice to have some blank lines above the comments.  Same
>> below.
>
> Makes sense to me. For the latest version [1], that should only mean a
> line after the call to cpu_cache_off in cpu_soft_restart.
>

Ok.
--Arun

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

On Fri, Aug 15, 2014 at 11:51 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Geoff,
>
> On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> Hi Arun,
>>
>> On Tue, 2014-08-12 at 18:12 +0530, Arun Chandran wrote:
>> > soft_restart() will fail on arm64 systems that does not
>> > quarantee the flushing of cache to PoC with flush_cache_all().
>> >
>> > soft_restart(addr)
>> > {
>> >     push_to_stack(addr);
>> >
>> >     Do mm setup for restart;
>> >     Flush&turnoff D-cache;
>> >
>> >     pop_from_stack(addr); --> fails here as addr is not at PoC
>> >     cpu_reset(addr) --> Jumps to invalid address
>> > }
>>
>> For the cpu-ops shutdown I'm working on I need a call to move the
>> secondary processors to an identity mapped spin loop after the identity
>> map is enabled.  I want to do this in C code, so it needs to happen
>> after the identity map is enabled, and before the dcache is disabled.
>>
>> I think to do this we can keep the existing soft_restart(addr) routine
>> with something like this:
>>
>> void soft_restart(unsigned long addr)
>> {
>>       setup_mm_for_reboot();
>>
>> #if defined(CONFIG_SMP)
>>       smp_secondary_shutdown();
>> #endif
>>
>>       cpu_soft_restart(addr);
>>
>>       /* Should never get here */
>>       BUG();
>> }
>>
>
> I don't follow why you need a hook in the middle of soft_restart. That
> sounds like a layering violation to me.
>
> I assume this is for implementing the spin-table cpu-return-addr idea?
>
> If so, what's wrong with something like:
>
> #define ADDR_INVALID ((unsigned long)-1)
> static DEFINE_PER_CPU(unsigned long, return_addr) = ADDR_INVALID;
>
> int spin_table_cpu_disable(unsigned int cpu)
> {
>         if (per_cpu(return_addr, cpu) != ADDR_INVALID)
>                 return 0;
>
>         return -EOPNOTSUPP;
> }
>
> void spin_table_cpu_die(unsigned int cpu)
> {
>         unsigned long release_addr = per_cpu(return_addr, cpu);
>
>         /*
>          * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
>          * something similar as these are all context synchronising and
>          * therefore expensive.
>          */
>         local_dbg_disable();
>         local_async_disable();
>         local_fiq_disable();
>         arch_local_irq_disable();
>
>         soft_restart(release_addr);
> }
>
> [...]
>
>> > diff --git a/arch/arm64/include/asm/proc-fns.h b/arch/arm64/include/asm/proc-fns.h
>> > index 0c657bb..e18d5d0 100644
>> > --- a/arch/arm64/include/asm/proc-fns.h
>> > +++ b/arch/arm64/include/asm/proc-fns.h
>> > @@ -32,6 +32,7 @@ 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(unsigned long addr) __attribute__((noreturn));
>>
>> Function prototypes are never definitions, so remove this 'extern'
>> keyword.  checkpatch should have warned about this.  If it did not,
>> report it to the checkpatch maintainers.
>
> Good point.
>
> Arun, could you fix up the latest version [1] of your patch to not use
> extern for the function declaration?
>
> If you'd be willing to spin a preparatory patch removing the other
> externs on function declarations in asm/proc-fns.h, that would be
> appreciated. Remember to add a Reported-by for Geoff.
>

Today I did "find arch/arm64 -name "*.h" | xargs grep -l "^extern"" in v3.16
and got

arch/arm64/mm/mm.h
arch/arm64/include/asm/exec.h
arch/arm64/include/asm/cpu_ops.h
arch/arm64/include/asm/suspend.h
arch/arm64/include/asm/tlbflush.h
arch/arm64/include/asm/stackprotector.h
arch/arm64/include/asm/stacktrace.h
arch/arm64/include/asm/mmu_context.h
arch/arm64/include/asm/cachetype.h
arch/arm64/include/asm/cputable.h
arch/arm64/include/asm/virt.h
arch/arm64/include/asm/efi.h
arch/arm64/include/asm/elf.h
arch/arm64/include/asm/pgtable.h
arch/arm64/include/asm/bitops.h
arch/arm64/include/asm/kgdb.h
arch/arm64/include/asm/fixmap.h
arch/arm64/include/asm/uaccess.h
arch/arm64/include/asm/page.h
arch/arm64/include/asm/smp_plat.h
arch/arm64/include/asm/memblock.h
arch/arm64/include/asm/perf_event.h
arch/arm64/include/asm/processor.h
arch/arm64/include/asm/ptrace.h
arch/arm64/include/asm/smp.h
arch/arm64/include/asm/hw_breakpoint.h
arch/arm64/include/asm/string.h
arch/arm64/include/asm/dma-mapping.h
arch/arm64/include/asm/fpsimd.h
arch/arm64/include/asm/mmu.h
arch/arm64/include/asm/memory.h
arch/arm64/include/asm/hwcap.h
arch/arm64/include/asm/system_misc.h
arch/arm64/include/asm/signal32.h
arch/arm64/include/asm/hardirq.h
arch/arm64/include/asm/ftrace.h
arch/arm64/include/asm/io.h
arch/arm64/include/asm/cacheflush.h
arch/arm64/include/asm/irq.h
arch/arm64/include/asm/pgalloc.h
arch/arm64/include/asm/syscall.h
arch/arm64/include/asm/topology.h
arch/arm64/include/asm/kvm_asm.h

No Idea what to do with the above ones.
Anyway I have sent a patch to remove it from asm/proc-fns.h.

--Arun

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

Hi Mark,

On Fri, Aug 15, 2014 at 11:51 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Geoff,
>
> On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> Hi Arun,
>>
>> On Tue, 2014-08-12 at 18:12 +0530, Arun Chandran wrote:
>> > soft_restart() will fail on arm64 systems that does not
>> > quarantee the flushing of cache to PoC with flush_cache_all().
>> >
>> > soft_restart(addr)
>> > {
>> >     push_to_stack(addr);
>> >
>> >     Do mm setup for restart;
>> >     Flush&turnoff D-cache;
>> >
>> >     pop_from_stack(addr); --> fails here as addr is not at PoC
>> >     cpu_reset(addr) --> Jumps to invalid address
>> > }
>>
>> For the cpu-ops shutdown I'm working on I need a call to move the
>> secondary processors to an identity mapped spin loop after the identity
>> map is enabled.  I want to do this in C code, so it needs to happen
>> after the identity map is enabled, and before the dcache is disabled.
>>
>> I think to do this we can keep the existing soft_restart(addr) routine
>> with something like this:
>>
>> void soft_restart(unsigned long addr)
>> {
>>       setup_mm_for_reboot();
>>
>> #if defined(CONFIG_SMP)
>>       smp_secondary_shutdown();
>> #endif
>>
>>       cpu_soft_restart(addr);
>>
>>       /* Should never get here */
>>       BUG();
>> }
>>
>
> I don't follow why you need a hook in the middle of soft_restart. That
> sounds like a layering violation to me.
>
> I assume this is for implementing the spin-table cpu-return-addr idea?
>
> If so, what's wrong with something like:
>
> #define ADDR_INVALID ((unsigned long)-1)
> static DEFINE_PER_CPU(unsigned long, return_addr) = ADDR_INVALID;
>
> int spin_table_cpu_disable(unsigned int cpu)
> {
>         if (per_cpu(return_addr, cpu) != ADDR_INVALID)
>                 return 0;
>
>         return -EOPNOTSUPP;
> }
>
> void spin_table_cpu_die(unsigned int cpu)
> {
>         unsigned long release_addr = per_cpu(return_addr, cpu);
>
>         /*
>          * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
>          * something similar as these are all context synchronising and
>          * therefore expensive.
>          */
>         local_dbg_disable();
>         local_async_disable();
>         local_fiq_disable();
>         arch_local_irq_disable();
>
>         soft_restart(release_addr);
> }
>

I am trying the above method for kexec.

As of now I have hardcoded cpu-return-addr , which is 0x0 in my
case.

##################
diff --git a/arch/arm64/kernel/smp_spin_table.c
b/arch/arm64/kernel/smp_spin_table.c
index e54ceed..e7275b3 100644
--- a/arch/arm64/kernel/smp_spin_table.c
+++ b/arch/arm64/kernel/smp_spin_table.c
@@ -27,6 +27,7 @@
 #include <asm/cpu_ops.h>
 #include <asm/cputype.h>
 #include <asm/smp_plat.h>
+#include <asm/system_misc.h>

 #include "cpu-properties.h"

@@ -97,7 +98,7 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu)
         * boot-loader's endianess before jumping. This is mandated by
         * the boot protocol.
         */
-       writeq_relaxed(__pa(secondary_holding_entry), release_addr);
+       writeq_relaxed(__pa(secondary_holding_pen), release_addr);
        __flush_dcache_area((__force void *)release_addr,
                            sizeof(*release_addr));

@@ -135,18 +136,22 @@ static void smp_spin_table_cpu_die(unsigned int cpu)
 {
        u64 *release_addr = phys_to_virt(cpu_release_addr[cpu]);

-       pr_devel("%s:%d: id: %u, holding count: %lu\n", __func__, __LINE__,
-               smp_processor_id(), secondary_holding_pen_count);
+       pr_devel("%s:%d: id: %u dying\n", __func__, __LINE__,
+               smp_processor_id());

        /* Send cpu back to secondary_holding_pen. */

+       local_dbg_disable();    // FIXME: need this???
+       local_async_disable();  // FIXME: need this???
        local_fiq_disable();    // FIXME: need this???
+       arch_local_irq_disable();

        *release_addr = 0;
+       __flush_dcache_area(release_addr, 8);

        mb();

-       secondary_holding_pen();
+       soft_restart(0);

        BUG();
 }
###########################

I can see that my secondary cpu's are not going to the exact same
state as they were in when I boot a SMP kernel from u-boot.

[[They are not waiting for addr(secondary_holding_pen()) at
 release_addr]]

Like geoff mentioned in another mail do we need to
to get back to EL2 before re-entering the bootwrapper program?

--Arun

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

On Wed, Aug 20, 2014 at 11:28:52AM +0100, Arun Chandran wrote:
> Hi Mark,

Hi Arun,

> On Fri, Aug 15, 2014 at 11:51 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi Geoff,
> >
> > On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
> >> Hi Arun,
> >>
> >> On Tue, 2014-08-12 at 18:12 +0530, Arun Chandran wrote:
> >> > soft_restart() will fail on arm64 systems that does not
> >> > quarantee the flushing of cache to PoC with flush_cache_all().
> >> >
> >> > soft_restart(addr)
> >> > {
> >> >     push_to_stack(addr);
> >> >
> >> >     Do mm setup for restart;
> >> >     Flush&turnoff D-cache;
> >> >
> >> >     pop_from_stack(addr); --> fails here as addr is not at PoC
> >> >     cpu_reset(addr) --> Jumps to invalid address
> >> > }
> >>
> >> For the cpu-ops shutdown I'm working on I need a call to move the
> >> secondary processors to an identity mapped spin loop after the identity
> >> map is enabled.  I want to do this in C code, so it needs to happen
> >> after the identity map is enabled, and before the dcache is disabled.
> >>
> >> I think to do this we can keep the existing soft_restart(addr) routine
> >> with something like this:
> >>
> >> void soft_restart(unsigned long addr)
> >> {
> >>       setup_mm_for_reboot();
> >>
> >> #if defined(CONFIG_SMP)
> >>       smp_secondary_shutdown();
> >> #endif
> >>
> >>       cpu_soft_restart(addr);
> >>
> >>       /* Should never get here */
> >>       BUG();
> >> }
> >>
> >
> > I don't follow why you need a hook in the middle of soft_restart. That
> > sounds like a layering violation to me.
> >
> > I assume this is for implementing the spin-table cpu-return-addr idea?
> >
> > If so, what's wrong with something like:
> >
> > #define ADDR_INVALID ((unsigned long)-1)
> > static DEFINE_PER_CPU(unsigned long, return_addr) = ADDR_INVALID;
> >
> > int spin_table_cpu_disable(unsigned int cpu)
> > {
> >         if (per_cpu(return_addr, cpu) != ADDR_INVALID)
> >                 return 0;
> >
> >         return -EOPNOTSUPP;
> > }
> >
> > void spin_table_cpu_die(unsigned int cpu)
> > {
> >         unsigned long release_addr = per_cpu(return_addr, cpu);
> >
> >         /*
> >          * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
> >          * something similar as these are all context synchronising and
> >          * therefore expensive.
> >          */
> >         local_dbg_disable();
> >         local_async_disable();
> >         local_fiq_disable();
> >         arch_local_irq_disable();
> >
> >         soft_restart(release_addr);
> > }
> >
> 
> I am trying the above method for kexec.
> 
> As of now I have hardcoded cpu-return-addr , which is 0x0 in my
> case.
> 
> ##################
> diff --git a/arch/arm64/kernel/smp_spin_table.c
> b/arch/arm64/kernel/smp_spin_table.c
> index e54ceed..e7275b3 100644
> --- a/arch/arm64/kernel/smp_spin_table.c
> +++ b/arch/arm64/kernel/smp_spin_table.c
> @@ -27,6 +27,7 @@
>  #include <asm/cpu_ops.h>
>  #include <asm/cputype.h>
>  #include <asm/smp_plat.h>
> +#include <asm/system_misc.h>
> 
>  #include "cpu-properties.h"
> 
> @@ -97,7 +98,7 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu)
>          * boot-loader's endianess before jumping. This is mandated by
>          * the boot protocol.
>          */
> -       writeq_relaxed(__pa(secondary_holding_entry), release_addr);
> +       writeq_relaxed(__pa(secondary_holding_pen), release_addr);
>         __flush_dcache_area((__force void *)release_addr,
>                             sizeof(*release_addr));
> 
> @@ -135,18 +136,22 @@ static void smp_spin_table_cpu_die(unsigned int cpu)
>  {
>         u64 *release_addr = phys_to_virt(cpu_release_addr[cpu]);
> 
> -       pr_devel("%s:%d: id: %u, holding count: %lu\n", __func__, __LINE__,
> -               smp_processor_id(), secondary_holding_pen_count);
> +       pr_devel("%s:%d: id: %u dying\n", __func__, __LINE__,
> +               smp_processor_id());
> 
>         /* Send cpu back to secondary_holding_pen. */
> 
> +       local_dbg_disable();    // FIXME: need this???
> +       local_async_disable();  // FIXME: need this???
>         local_fiq_disable();    // FIXME: need this???
> +       arch_local_irq_disable();
> 
>         *release_addr = 0;
> +       __flush_dcache_area(release_addr, 8);
> 
>         mb();
> 
> -       secondary_holding_pen();
> +       soft_restart(0);
> 
>         BUG();
>  }
> ###########################
> 
> I can see that my secondary cpu's are not going to the exact same
> state as they were in when I boot a SMP kernel from u-boot.
> 
> [[They are not waiting for addr(secondary_holding_pen()) at
>  release_addr]]
> 
> Like geoff mentioned in another mail do we need to
> to get back to EL2 before re-entering the bootwrapper program?

As I mentioned we do need to ensure that the CPUs are in the mode they
started in, though I'm not sure I follow what you mean by "not waiting".
This could be an orthogonal issue.

What exactly do you see, do the CPUs leave the spin-table, are they
taking exceptions, are they getting stuck in the spin-table, etc?

Are the cpu-release-addr entries getting restored to zero before each
CPU begins polling them?

Cheers,
Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

On Wed, Aug 20, 2014 at 4:24 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Aug 20, 2014 at 11:28:52AM +0100, Arun Chandran wrote:
>> Hi Mark,
>
> Hi Arun,
>
>> On Fri, Aug 15, 2014 at 11:51 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > Hi Geoff,
>> >
>> > On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> >> Hi Arun,
>> >>
>> >> On Tue, 2014-08-12 at 18:12 +0530, Arun Chandran wrote:
>> >> > soft_restart() will fail on arm64 systems that does not
>> >> > quarantee the flushing of cache to PoC with flush_cache_all().
>> >> >
>> >> > soft_restart(addr)
>> >> > {
>> >> >     push_to_stack(addr);
>> >> >
>> >> >     Do mm setup for restart;
>> >> >     Flush&turnoff D-cache;
>> >> >
>> >> >     pop_from_stack(addr); --> fails here as addr is not at PoC
>> >> >     cpu_reset(addr) --> Jumps to invalid address
>> >> > }
>> >>
>> >> For the cpu-ops shutdown I'm working on I need a call to move the
>> >> secondary processors to an identity mapped spin loop after the identity
>> >> map is enabled.  I want to do this in C code, so it needs to happen
>> >> after the identity map is enabled, and before the dcache is disabled.
>> >>
>> >> I think to do this we can keep the existing soft_restart(addr) routine
>> >> with something like this:
>> >>
>> >> void soft_restart(unsigned long addr)
>> >> {
>> >>       setup_mm_for_reboot();
>> >>
>> >> #if defined(CONFIG_SMP)
>> >>       smp_secondary_shutdown();
>> >> #endif
>> >>
>> >>       cpu_soft_restart(addr);
>> >>
>> >>       /* Should never get here */
>> >>       BUG();
>> >> }
>> >>
>> >
>> > I don't follow why you need a hook in the middle of soft_restart. That
>> > sounds like a layering violation to me.
>> >
>> > I assume this is for implementing the spin-table cpu-return-addr idea?
>> >
>> > If so, what's wrong with something like:
>> >
>> > #define ADDR_INVALID ((unsigned long)-1)
>> > static DEFINE_PER_CPU(unsigned long, return_addr) = ADDR_INVALID;
>> >
>> > int spin_table_cpu_disable(unsigned int cpu)
>> > {
>> >         if (per_cpu(return_addr, cpu) != ADDR_INVALID)
>> >                 return 0;
>> >
>> >         return -EOPNOTSUPP;
>> > }
>> >
>> > void spin_table_cpu_die(unsigned int cpu)
>> > {
>> >         unsigned long release_addr = per_cpu(return_addr, cpu);
>> >
>> >         /*
>> >          * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
>> >          * something similar as these are all context synchronising and
>> >          * therefore expensive.
>> >          */
>> >         local_dbg_disable();
>> >         local_async_disable();
>> >         local_fiq_disable();
>> >         arch_local_irq_disable();
>> >
>> >         soft_restart(release_addr);
>> > }
>> >
>>
>> I am trying the above method for kexec.
>>
>> As of now I have hardcoded cpu-return-addr , which is 0x0 in my
>> case.
>>
>> ##################
>> diff --git a/arch/arm64/kernel/smp_spin_table.c
>> b/arch/arm64/kernel/smp_spin_table.c
>> index e54ceed..e7275b3 100644
>> --- a/arch/arm64/kernel/smp_spin_table.c
>> +++ b/arch/arm64/kernel/smp_spin_table.c
>> @@ -27,6 +27,7 @@
>>  #include <asm/cpu_ops.h>
>>  #include <asm/cputype.h>
>>  #include <asm/smp_plat.h>
>> +#include <asm/system_misc.h>
>>
>>  #include "cpu-properties.h"
>>
>> @@ -97,7 +98,7 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu)
>>          * boot-loader's endianess before jumping. This is mandated by
>>          * the boot protocol.
>>          */
>> -       writeq_relaxed(__pa(secondary_holding_entry), release_addr);
>> +       writeq_relaxed(__pa(secondary_holding_pen), release_addr);
>>         __flush_dcache_area((__force void *)release_addr,
>>                             sizeof(*release_addr));
>>
>> @@ -135,18 +136,22 @@ static void smp_spin_table_cpu_die(unsigned int cpu)
>>  {
>>         u64 *release_addr = phys_to_virt(cpu_release_addr[cpu]);
>>
>> -       pr_devel("%s:%d: id: %u, holding count: %lu\n", __func__, __LINE__,
>> -               smp_processor_id(), secondary_holding_pen_count);
>> +       pr_devel("%s:%d: id: %u dying\n", __func__, __LINE__,
>> +               smp_processor_id());
>>
>>         /* Send cpu back to secondary_holding_pen. */
>>
>> +       local_dbg_disable();    // FIXME: need this???
>> +       local_async_disable();  // FIXME: need this???
>>         local_fiq_disable();    // FIXME: need this???
>> +       arch_local_irq_disable();
>>
>>         *release_addr = 0;
>> +       __flush_dcache_area(release_addr, 8);
>>
>>         mb();
>>
>> -       secondary_holding_pen();
>> +       soft_restart(0);
>>
>>         BUG();
>>  }
>> ###########################
>>
>> I can see that my secondary cpu's are not going to the exact same
>> state as they were in when I boot a SMP kernel from u-boot.
>>
>> [[They are not waiting for addr(secondary_holding_pen()) at
>>  release_addr]]
>>
>> Like geoff mentioned in another mail do we need to
>> to get back to EL2 before re-entering the bootwrapper program?
>

I am trying to kexec a UP-LE kernel from and SMP-LE kernel.

> As I mentioned we do need to ensure that the CPUs are in the mode they
> started in, though I'm not sure I follow what you mean by "not waiting".
> This could be an orthogonal issue.

If I verify the secondary CPUs from u-boot I can see that
they are all looping at

    Core number       : 1
    Core state        : debug (AArch64 EL2)
    Debug entry cause : External Debug Request
    Current PC        : 0x0000000000000238
    Current CPSR      : 0x200003c9 (EL2h)

But after the kexec calls soft_restar(0) for all secondary CPUs
they are looping at

    Core number       : 1
    Core state        : debug (AArch64 EL1)
    Debug entry cause : External Debug Request
    Current PC        : 0xffffffc000083200
    Current CPSR      : 0x600003c5 (EL1h)

This is what I mean by they are not waiting for
the secondary start-up address to jump.

>
> What exactly do you see, do the CPUs leave the spin-table, are they
> taking exceptions, are they getting stuck in the spin-table, etc?
>
They all are clearly resetting to address "0"(Put a breakpoint and
verified) but somehow they end up @0xffffffc000083200.
I still don't know why.

########
ffffffc00008319c:       d503201f        nop
        ...
ffffffc000083200:       14000260        b       ffffffc000083b80 <el1_sync>
ffffffc000083204:       d503201f        nop
ffffffc000083208:       d503201f        nop
########

> Are the cpu-release-addr entries getting restored to zero before each
> CPU begins polling them?

Yes I can see

CPU#1>md 0x400000fff8
40_0000fff8 : 00000000


--Arun

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

[...]

> I am trying to kexec a UP-LE kernel from and SMP-LE kernel.
> 
> > As I mentioned we do need to ensure that the CPUs are in the mode they
> > started in, though I'm not sure I follow what you mean by "not waiting".
> > This could be an orthogonal issue.
> 
> If I verify the secondary CPUs from u-boot I can see that
> they are all looping at
> 
>     Core number       : 1
>     Core state        : debug (AArch64 EL2)
>     Debug entry cause : External Debug Request
>     Current PC        : 0x0000000000000238
>     Current CPSR      : 0x200003c9 (EL2h)
> 
> But after the kexec calls soft_restar(0) for all secondary CPUs
> they are looping at
> 
>     Core number       : 1
>     Core state        : debug (AArch64 EL1)
>     Debug entry cause : External Debug Request
>     Current PC        : 0xffffffc000083200
>     Current CPSR      : 0x600003c5 (EL1h)
> 
> This is what I mean by they are not waiting for
> the secondary start-up address to jump.

Ok.

> >
> > What exactly do you see, do the CPUs leave the spin-table, are they
> > taking exceptions, are they getting stuck in the spin-table, etc?
> >
> They all are clearly resetting to address "0"(Put a breakpoint and
> verified) but somehow they end up @0xffffffc000083200.
> I still don't know why.
> 
> ########
> ffffffc00008319c:       d503201f        nop
>         ...
> ffffffc000083200:       14000260        b       ffffffc000083b80 <el1_sync>
> ffffffc000083204:       d503201f        nop
> ffffffc000083208:       d503201f        nop
> ########

That's the EL1 exception vector table.

What looks to be happening is that something causes the CPUs to take an
exception (at EL1). Because translation isn't enabled and the vector
address doesn't map to anything, they'll take some sort of exception.
Because the vectors aren't mapped that will go recursive.

Your spin-table implementation might be poking something that's not
accessible at EL1, in which case you require the jump to EL2 for
correctness. I can't say for certain either way.

Thanks,
Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

Hi Mark,

On Wed, Aug 20, 2014 at 7:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> [...]
>
>> I am trying to kexec a UP-LE kernel from and SMP-LE kernel.
>>
>> > As I mentioned we do need to ensure that the CPUs are in the mode they
>> > started in, though I'm not sure I follow what you mean by "not waiting".
>> > This could be an orthogonal issue.
>>
>> If I verify the secondary CPUs from u-boot I can see that
>> they are all looping at
>>
>>     Core number       : 1
>>     Core state        : debug (AArch64 EL2)
>>     Debug entry cause : External Debug Request
>>     Current PC        : 0x0000000000000238
>>     Current CPSR      : 0x200003c9 (EL2h)
>>
>> But after the kexec calls soft_restar(0) for all secondary CPUs
>> they are looping at
>>
>>     Core number       : 1
>>     Core state        : debug (AArch64 EL1)
>>     Debug entry cause : External Debug Request
>>     Current PC        : 0xffffffc000083200
>>     Current CPSR      : 0x600003c5 (EL1h)
>>
>> This is what I mean by they are not waiting for
>> the secondary start-up address to jump.
>
> Ok.
>
>> >
>> > What exactly do you see, do the CPUs leave the spin-table, are they
>> > taking exceptions, are they getting stuck in the spin-table, etc?
>> >
>> They all are clearly resetting to address "0"(Put a breakpoint and
>> verified) but somehow they end up @0xffffffc000083200.
>> I still don't know why.
>>
>> ########
>> ffffffc00008319c:       d503201f        nop
>>         ...
>> ffffffc000083200:       14000260        b       ffffffc000083b80 <el1_sync>
>> ffffffc000083204:       d503201f        nop
>> ffffffc000083208:       d503201f        nop
>> ########
>
> That's the EL1 exception vector table.
>
> What looks to be happening is that something causes the CPUs to take an
> exception (at EL1). Because translation isn't enabled and the vector
> address doesn't map to anything, they'll take some sort of exception.
> Because the vectors aren't mapped that will go recursive.
>
> Your spin-table implementation might be poking something that's not
> accessible at EL1, in which case you require the jump to EL2 for
> correctness. I can't say for certain either way.
>

Yes you are right I needed a jump to EL2 for putting the
secondary CPUs at correct spinning loop.

I made some progress with the below changes.

###########
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 607d4bb..8969922 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -343,6 +343,11 @@ CPU_LE(    movk    x0, #0x30d0, lsl #16    )
 // Clear EE and E0E on LE systems
                      PSR_MODE_EL1h)
        msr     spsr_el2, x0
        msr     elr_el2, lr
+       mov     x0, #0x33ff
+       movk    x0, #0x801f, lsl #16
+       msr     cptr_el2, x0                    // Enable traps to el2
when CPACR or CPACR_EL1 is accessed
+       mov     x0, #3 << 20
+       msr     cpacr_el1, x0                   // Enable FP/ASIMD
        mov     w20, #BOOT_CPU_MODE_EL2         // This CPU booted in EL2
        eret
 ENDPROC(el2_setup)
diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
index a272f33..d8e806c 100644
--- a/arch/arm64/kernel/hyp-stub.S
+++ b/arch/arm64/kernel/hyp-stub.S
@@ -66,6 +66,14 @@ ENDPROC(el1_sync)

 .macro invalid_vector  label
 \label:
+       mov     x1, #0x33ff
+       msr     cptr_el2, x1                    // Disable copro. traps to EL2
+
+       ic      ialluis
+       isb
+       dsb     sy
+
+       br  x0
        b \label
 ENDPROC(\label)
 .endm
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 64733d4..8ca929c 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -65,6 +65,12 @@ void soft_restart(unsigned long addr)
 //     smp_secondary_shutdown();
 #endif

+       /* Delay primary cpu; allow enough time for
+        * secondaries to enter spin loop
+        */
+       if (smp_processor_id() == 0)
+               mdelay(1000);
+
        cpu_soft_restart(virt_to_phys(cpu_reset), addr);

        /* Should never get here */
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 3cb6dec..f6ab468 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -76,6 +76,10 @@ ENTRY(cpu_reset)
 #if defined(CONFIG_SMP)
 /*     bl      secondary_shutdown */
 #endif
+
+       /* Trap to EL2 */
+       mov     x1, #3 << 20
+       msr     cpacr_el1, x1                   // Enable FP/ASIMD
        ret     x0
 ENDPROC(cpu_reset)

@@ -200,8 +204,6 @@ ENTRY(__cpu_setup)
        tlbi    vmalle1is                       // invalidate I + D TLBs
        dsb     ish

-       mov     x0, #3 << 20
-       msr     cpacr_el1, x0                   // Enable FP/ASIMD
        msr     mdscr_el1, xzr                  // Reset mdscr_el1
        /*
         * Memory region attributes for LPAE:
#########

With the above code I was able to manually kexec
an SMP kernel (With the help of debugger).

Basically the branch instruction (br x0) is not working
in the el2 vector.
------------------
CPU#0>h
    Core number       : 0
    Core state        : debug (AArch64 EL2)
    Debug entry cause : External Debug Request
    Current PC        : 0x0000004000089800
    Current CPSR      : 0x600003c9 (EL2h)
CPU#0>rd
GPR00: 00000043eb891000 0000000000000018 0000000000000006 0000000000000004
GPR04: 000000000000001f 000000000000001b 0000000000000000 ffffffffffffffff
GPR08: ffffffc3ffe80614 ffffffffffffffff 0000000000000000 0000000000000002
GPR12: ffffffc0000968f0 00000040008c0000 00000043eb949002 ffffffffffffffff
GPR16: ffffffc0000c1244 0000000000435260 0000007ff9c5d490 00000040000968c0
GPR20: 00000043eb891000 ffffffc3eb891000 ffffffc3eb973c00 0000000000000110
GPR24: ffffffc000094000 ffffffc000094cd8 000000000000008e ffffffc00082f000
GPR28: ffffffc3e9888000 ffffffc3e988bd00 ffffffc000095d88 00000043efb24430
CPU#0>go 0x00000043eb891000
--------------------

I had to use the debugger  to make cpu0 jump to kexec-boot code.
Similarly I had to manually put other CPUs to spinning code
using debugger. Could you please throw some light here?

--Arun











> Thanks,
> Mark.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

On Thu, Aug 21, 2014 at 02:34:46PM +0100, Arun Chandran wrote:
> Hi Mark,
> 
> On Wed, Aug 20, 2014 at 7:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > [...]
> >
> >> I am trying to kexec a UP-LE kernel from and SMP-LE kernel.
> >>
> >> > As I mentioned we do need to ensure that the CPUs are in the mode they
> >> > started in, though I'm not sure I follow what you mean by "not waiting".
> >> > This could be an orthogonal issue.
> >>
> >> If I verify the secondary CPUs from u-boot I can see that
> >> they are all looping at
> >>
> >>     Core number       : 1
> >>     Core state        : debug (AArch64 EL2)
> >>     Debug entry cause : External Debug Request
> >>     Current PC        : 0x0000000000000238
> >>     Current CPSR      : 0x200003c9 (EL2h)
> >>
> >> But after the kexec calls soft_restar(0) for all secondary CPUs
> >> they are looping at
> >>
> >>     Core number       : 1
> >>     Core state        : debug (AArch64 EL1)
> >>     Debug entry cause : External Debug Request
> >>     Current PC        : 0xffffffc000083200
> >>     Current CPSR      : 0x600003c5 (EL1h)
> >>
> >> This is what I mean by they are not waiting for
> >> the secondary start-up address to jump.
> >
> > Ok.
> >
> >> >
> >> > What exactly do you see, do the CPUs leave the spin-table, are they
> >> > taking exceptions, are they getting stuck in the spin-table, etc?
> >> >
> >> They all are clearly resetting to address "0"(Put a breakpoint and
> >> verified) but somehow they end up @0xffffffc000083200.
> >> I still don't know why.
> >>
> >> ########
> >> ffffffc00008319c:       d503201f        nop
> >>         ...
> >> ffffffc000083200:       14000260        b       ffffffc000083b80 <el1_sync>
> >> ffffffc000083204:       d503201f        nop
> >> ffffffc000083208:       d503201f        nop
> >> ########
> >
> > That's the EL1 exception vector table.
> >
> > What looks to be happening is that something causes the CPUs to take an
> > exception (at EL1). Because translation isn't enabled and the vector
> > address doesn't map to anything, they'll take some sort of exception.
> > Because the vectors aren't mapped that will go recursive.
> >
> > Your spin-table implementation might be poking something that's not
> > accessible at EL1, in which case you require the jump to EL2 for
> > correctness. I can't say for certain either way.
> >
> 
> Yes you are right I needed a jump to EL2 for putting the
> secondary CPUs at correct spinning loop.

Ok. So we need to do what I have suggested elsewhere w.r.t. jumping back
up to EL2. As you point out below we need to synchronise with the CPUs
on their way out too we can add a spin-table cpu_kill with a slightly
dodgy delay to ensure that, given we have no other mechanism for
synchronising.

> I made some progress with the below changes.
> 
> ###########
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 607d4bb..8969922 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -343,6 +343,11 @@ CPU_LE(    movk    x0, #0x30d0, lsl #16    )
>  // Clear EE and E0E on LE systems
>                       PSR_MODE_EL1h)
>         msr     spsr_el2, x0
>         msr     elr_el2, lr
> +       mov     x0, #0x33ff
> +       movk    x0, #0x801f, lsl #16
> +       msr     cptr_el2, x0                    // Enable traps to el2
> when CPACR or CPACR_EL1 is accessed
> +       mov     x0, #3 << 20
> +       msr     cpacr_el1, x0                   // Enable FP/ASIMD
>         mov     w20, #BOOT_CPU_MODE_EL2         // This CPU booted in EL2
>         eret
>  ENDPROC(el2_setup)
> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
> index a272f33..d8e806c 100644
> --- a/arch/arm64/kernel/hyp-stub.S
> +++ b/arch/arm64/kernel/hyp-stub.S
> @@ -66,6 +66,14 @@ ENDPROC(el1_sync)
> 
>  .macro invalid_vector  label
>  \label:
> +       mov     x1, #0x33ff
> +       msr     cptr_el2, x1                    // Disable copro. traps to EL2
> +
> +       ic      ialluis
> +       isb
> +       dsb     sy

As far as I am aware an "isb; dsb" sequence never makes sense. It should
be "dsb; isb". You want the I-side maintenance to complete (for which
you have the DSB) _then_ you want to synchronise the execution context
with the newly-visible instructions (for which you have the ISB).

> +
> +       br  x0
>         b \label
>  ENDPROC(\label)
>  .endm
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 64733d4..8ca929c 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -65,6 +65,12 @@ void soft_restart(unsigned long addr)
>  //     smp_secondary_shutdown();
>  #endif
> 
> +       /* Delay primary cpu; allow enough time for
> +        * secondaries to enter spin loop
> +        */
> +       if (smp_processor_id() == 0)
> +               mdelay(1000);
> +
>         cpu_soft_restart(virt_to_phys(cpu_reset), addr);
> 
>         /* Should never get here */
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 3cb6dec..f6ab468 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -76,6 +76,10 @@ ENTRY(cpu_reset)
>  #if defined(CONFIG_SMP)
>  /*     bl      secondary_shutdown */
>  #endif
> +
> +       /* Trap to EL2 */
> +       mov     x1, #3 << 20
> +       msr     cpacr_el1, x1                   // Enable FP/ASIMD
>         ret     x0
>  ENDPROC(cpu_reset)
> 
> @@ -200,8 +204,6 @@ ENTRY(__cpu_setup)
>         tlbi    vmalle1is                       // invalidate I + D TLBs
>         dsb     ish
> 
> -       mov     x0, #3 << 20
> -       msr     cpacr_el1, x0                   // Enable FP/ASIMD
>         msr     mdscr_el1, xzr                  // Reset mdscr_el1
>         /*
>          * Memory region attributes for LPAE:
> #########
> 
> With the above code I was able to manually kexec
> an SMP kernel (With the help of debugger).
> 
> Basically the branch instruction (br x0) is not working
> in the el2 vector.
> ------------------
> CPU#0>h
>     Core number       : 0
>     Core state        : debug (AArch64 EL2)
>     Debug entry cause : External Debug Request
>     Current PC        : 0x0000004000089800

Does this address look similar to the VBAR_EL2, perhaps?

>     Current CPSR      : 0x600003c9 (EL2h)
> CPU#0>rd
> GPR00: 00000043eb891000 0000000000000018 0000000000000006 0000000000000004
> GPR04: 000000000000001f 000000000000001b 0000000000000000 ffffffffffffffff
> GPR08: ffffffc3ffe80614 ffffffffffffffff 0000000000000000 0000000000000002
> GPR12: ffffffc0000968f0 00000040008c0000 00000043eb949002 ffffffffffffffff
> GPR16: ffffffc0000c1244 0000000000435260 0000007ff9c5d490 00000040000968c0
> GPR20: 00000043eb891000 ffffffc3eb891000 ffffffc3eb973c00 0000000000000110
> GPR24: ffffffc000094000 ffffffc000094cd8 000000000000008e ffffffc00082f000
> GPR28: ffffffc3e9888000 ffffffc3e988bd00 ffffffc000095d88 00000043efb24430
> CPU#0>go 0x00000043eb891000
> --------------------
> 
> I had to use the debugger  to make cpu0 jump to kexec-boot code.
> Similarly I had to manually put other CPUs to spinning code
> using debugger. Could you please throw some light here?

I really can't say. I know nothing of your platform or spin-table
implementation, and this is nothing like what I'd expect the jump to EL2
code to look like.

Thanks,
Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

Hi Mark,

On Thu, Aug 21, 2014 at 8:01 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Aug 21, 2014 at 02:34:46PM +0100, Arun Chandran wrote:
>> Hi Mark,
>>
>> On Wed, Aug 20, 2014 at 7:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > [...]
>> >
>> >> I am trying to kexec a UP-LE kernel from and SMP-LE kernel.
>> >>
>> >> > As I mentioned we do need to ensure that the CPUs are in the mode they
>> >> > started in, though I'm not sure I follow what you mean by "not waiting".
>> >> > This could be an orthogonal issue.
>> >>
>> >> If I verify the secondary CPUs from u-boot I can see that
>> >> they are all looping at
>> >>
>> >>     Core number       : 1
>> >>     Core state        : debug (AArch64 EL2)
>> >>     Debug entry cause : External Debug Request
>> >>     Current PC        : 0x0000000000000238
>> >>     Current CPSR      : 0x200003c9 (EL2h)
>> >>
>> >> But after the kexec calls soft_restar(0) for all secondary CPUs
>> >> they are looping at
>> >>
>> >>     Core number       : 1
>> >>     Core state        : debug (AArch64 EL1)
>> >>     Debug entry cause : External Debug Request
>> >>     Current PC        : 0xffffffc000083200
>> >>     Current CPSR      : 0x600003c5 (EL1h)
>> >>
>> >> This is what I mean by they are not waiting for
>> >> the secondary start-up address to jump.
>> >
>> > Ok.
>> >
>> >> >
>> >> > What exactly do you see, do the CPUs leave the spin-table, are they
>> >> > taking exceptions, are they getting stuck in the spin-table, etc?
>> >> >
>> >> They all are clearly resetting to address "0"(Put a breakpoint and
>> >> verified) but somehow they end up @0xffffffc000083200.
>> >> I still don't know why.
>> >>
>> >> ########
>> >> ffffffc00008319c:       d503201f        nop
>> >>         ...
>> >> ffffffc000083200:       14000260        b       ffffffc000083b80 <el1_sync>
>> >> ffffffc000083204:       d503201f        nop
>> >> ffffffc000083208:       d503201f        nop
>> >> ########
>> >
>> > That's the EL1 exception vector table.
>> >
>> > What looks to be happening is that something causes the CPUs to take an
>> > exception (at EL1). Because translation isn't enabled and the vector
>> > address doesn't map to anything, they'll take some sort of exception.
>> > Because the vectors aren't mapped that will go recursive.
>> >
>> > Your spin-table implementation might be poking something that's not
>> > accessible at EL1, in which case you require the jump to EL2 for
>> > correctness. I can't say for certain either way.
>> >
>>
>> Yes you are right I needed a jump to EL2 for putting the
>> secondary CPUs at correct spinning loop.
>
> Ok. So we need to do what I have suggested elsewhere w.r.t. jumping back
> up to EL2. As you point out below we need to synchronise with the CPUs
> on their way out too we can add a spin-table cpu_kill with a slightly
> dodgy delay to ensure that, given we have no other mechanism for
> synchronising.
>
Ok

>
> As far as I am aware an "isb; dsb" sequence never makes sense. It should
> be "dsb; isb". You want the I-side maintenance to complete (for which
> you have the DSB) _then_ you want to synchronise the execution context
> with the newly-visible instructions (for which you have the ISB).
>
Thanks for the nice explanation.

Now I am able to kexec an SMP kernel with the below change.

##########
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 607d4bb..8969922 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -343,6 +343,11 @@ CPU_LE(    movk    x0, #0x30d0, lsl #16    )
 // Clear EE and E0E on LE systems
                      PSR_MODE_EL1h)
        msr     spsr_el2, x0
        msr     elr_el2, lr
+       mov     x0, #0x33ff
+       movk    x0, #0x801f, lsl #16
+       msr     cptr_el2, x0                    // Enable traps to el2
when CPACR or CPACR_EL1 is accessed
+       mov     x0, #3 << 20
+       msr     cpacr_el1, x0                   // Enable FP/ASIMD
        mov     w20, #BOOT_CPU_MODE_EL2         // This CPU booted in EL2
        eret
 ENDPROC(el2_setup)
diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
index a272f33..1fb4c38 100644
--- a/arch/arm64/kernel/hyp-stub.S
+++ b/arch/arm64/kernel/hyp-stub.S
@@ -56,12 +56,25 @@ el1_sync:
        mrs     x1, esr_el2
        lsr     x1, x1, #26
        cmp     x1, #0x16
-       b.ne    2f                              // Not an HVC trap
+       b.ne    3f                              // Not an HVC trap
        cbz     x0, 1f
        msr     vbar_el2, x0                    // Set vbar_el2
        b       2f
 1:     mrs     x0, vbar_el2                    // Return vbar_el2
 2:     eret
+3:     cmp     x1, #0x18       // Traps to EL2 for CPACR access have this code
+       b.ne    2b
+
+       mov     x1, #0x33ff
+       msr     cptr_el2, x1                    // Disable copro. traps to EL2
+
+       msr     cntp_ctl_el0, xzr
+       msr     cntfrq_el0, xzr
+       ic      ialluis
+       dsb     sy
+       isb
+
+       br  x0
 ENDPROC(el1_sync)

 .macro invalid_vector  label
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 64733d4..77298c2 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -65,6 +65,14 @@ void soft_restart(unsigned long addr)
 //     smp_secondary_shutdown();
 #endif

+       /* Delay primary cpu; allow enough time for
+        * secondaries to enter spin loop
+        */
+#if defined(CONFIG_SMP)
+       if (smp_processor_id() == 0)
+               mdelay(1000);
+#endif
+
        cpu_soft_restart(virt_to_phys(cpu_reset), addr);

        /* Should never get here */
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 3cb6dec..f6ab468 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -76,6 +76,10 @@ ENTRY(cpu_reset)
 #if defined(CONFIG_SMP)
 /*     bl      secondary_shutdown */
 #endif
+
+       /* Trap to EL2 */
+       mov     x1, #3 << 20
+       msr     cpacr_el1, x1                   // Enable FP/ASIMD
        ret     x0
 ENDPROC(cpu_reset)

@@ -200,8 +204,6 @@ ENTRY(__cpu_setup)
        tlbi    vmalle1is                       // invalidate I + D TLBs
        dsb     ish

-       mov     x0, #3 << 20
-       msr     cpacr_el1, x0                   // Enable FP/ASIMD
        msr     mdscr_el1, xzr                  // Reset mdscr_el1
        /*
         * Memory region attributes for LPAE:
##########

The two lines
+       msr     cntp_ctl_el0, xzr
+       msr     cntfrq_el0, xzr

were added for
##########
SANITY CHECK: Unexpected variation in cntfrq. Boot CPU:
0x00000002faf080, CPU1: 0x00000000000000
------------[ cut here ]------------
WARNING: CPU: 1 PID: 0@arch/arm64/kernel/cpuinfo.c:146
cpuinfo_store_cpu+0x2e8/0x14f4()
Unsupported CPU feature variation.
Modules linked in:
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.16.0-rc4+ #196
Call trace:
[<ffffffc000087fd0>] dump_backtrace+0x0/0x130
[<ffffffc000088110>] show_stack+0x10/0x1c
[<ffffffc0004c03e0>] dump_stack+0x74/0xb8
[<ffffffc00009dfac>] warn_slowpath_common+0x8c/0xb4
[<ffffffc00009e078>] warn_slowpath_fmt_taint+0x4c/0x58
[<ffffffc00008adf0>] cpuinfo_store_cpu+0x2e4/0x14f4
[<ffffffc00008f7e0>] secondary_start_kernel+0xd4/0x120
---[ end trace 62c0cf48de286b1c ]---
CPU2: Booted secondary processor
Detected PIPT I-cache on CPU2
SANITY CHECK: Unexpected variation in cntfrq. Boot CPU:
0x00000002faf080, CPU2: 0x00000000000000
CPU3: Booted secondary processor
##########

does this mean we also need to stop/handle timers before
jumping to new kernel??

> I really can't say. I know nothing of your platform or spin-table
> implementation, and this is nothing like what I'd expect the jump to EL2
> code to look like.

Yes It was just for trying things out. Traping to EL2 and then jumping
to the kexec/SMP-spin code is not sane. And by the way
I did not find anyway to do mode change to el2 from el1 other than
trapping.

Could you please elaborate more about the way you expect
it to be working/implemented??.

--Arun

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

[...]

> >> > Your spin-table implementation might be poking something that's not
> >> > accessible at EL1, in which case you require the jump to EL2 for
> >> > correctness. I can't say for certain either way.
> >> >
> >>
> >> Yes you are right I needed a jump to EL2 for putting the
> >> secondary CPUs at correct spinning loop.
> >
> > Ok. So we need to do what I have suggested elsewhere w.r.t. jumping back
> > up to EL2. As you point out below we need to synchronise with the CPUs
> > on their way out too we can add a spin-table cpu_kill with a slightly
> > dodgy delay to ensure that, given we have no other mechanism for
> > synchronising.
> >
> Ok
> 
> >
> > As far as I am aware an "isb; dsb" sequence never makes sense. It should
> > be "dsb; isb". You want the I-side maintenance to complete (for which
> > you have the DSB) _then_ you want to synchronise the execution context
> > with the newly-visible instructions (for which you have the ISB).
> >
> Thanks for the nice explanation.
> 
> Now I am able to kexec an SMP kernel with the below change.
> 
> ##########
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 607d4bb..8969922 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -343,6 +343,11 @@ CPU_LE(    movk    x0, #0x30d0, lsl #16    )
>  // Clear EE and E0E on LE systems
>                       PSR_MODE_EL1h)
>         msr     spsr_el2, x0
>         msr     elr_el2, lr
> +       mov     x0, #0x33ff
> +       movk    x0, #0x801f, lsl #16
> +       msr     cptr_el2, x0                    // Enable traps to el2
> when CPACR or CPACR_EL1 is accessed
> +       mov     x0, #3 << 20
> +       msr     cpacr_el1, x0                   // Enable FP/ASIMD
>         mov     w20, #BOOT_CPU_MODE_EL2         // This CPU booted in EL2
>         eret
>  ENDPROC(el2_setup)
> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
> index a272f33..1fb4c38 100644
> --- a/arch/arm64/kernel/hyp-stub.S
> +++ b/arch/arm64/kernel/hyp-stub.S
> @@ -56,12 +56,25 @@ el1_sync:
>         mrs     x1, esr_el2
>         lsr     x1, x1, #26
>         cmp     x1, #0x16
> -       b.ne    2f                              // Not an HVC trap
> +       b.ne    3f                              // Not an HVC trap
>         cbz     x0, 1f
>         msr     vbar_el2, x0                    // Set vbar_el2
>         b       2f
>  1:     mrs     x0, vbar_el2                    // Return vbar_el2
>  2:     eret
> +3:     cmp     x1, #0x18       // Traps to EL2 for CPACR access have this code
> +       b.ne    2b
> +
> +       mov     x1, #0x33ff
> +       msr     cptr_el2, x1                    // Disable copro. traps to EL2
> +
> +       msr     cntp_ctl_el0, xzr
> +       msr     cntfrq_el0, xzr
> +       ic      ialluis
> +       dsb     sy
> +       isb
> +
> +       br  x0
>  ENDPROC(el1_sync)
> 
>  .macro invalid_vector  label
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 64733d4..77298c2 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -65,6 +65,14 @@ void soft_restart(unsigned long addr)
>  //     smp_secondary_shutdown();
>  #endif
> 
> +       /* Delay primary cpu; allow enough time for
> +        * secondaries to enter spin loop
> +        */
> +#if defined(CONFIG_SMP)
> +       if (smp_processor_id() == 0)
> +               mdelay(1000);
> +#endif
> +
>         cpu_soft_restart(virt_to_phys(cpu_reset), addr);
> 
>         /* Should never get here */
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 3cb6dec..f6ab468 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -76,6 +76,10 @@ ENTRY(cpu_reset)
>  #if defined(CONFIG_SMP)
>  /*     bl      secondary_shutdown */
>  #endif
> +
> +       /* Trap to EL2 */
> +       mov     x1, #3 << 20
> +       msr     cpacr_el1, x1                   // Enable FP/ASIMD
>         ret     x0
>  ENDPROC(cpu_reset)
> 
> @@ -200,8 +204,6 @@ ENTRY(__cpu_setup)
>         tlbi    vmalle1is                       // invalidate I + D TLBs
>         dsb     ish
> 
> -       mov     x0, #3 << 20
> -       msr     cpacr_el1, x0                   // Enable FP/ASIMD
>         msr     mdscr_el1, xzr                  // Reset mdscr_el1
>         /*
>          * Memory region attributes for LPAE:
> ##########
> 
> The two lines
> +       msr     cntp_ctl_el0, xzr
> +       msr     cntfrq_el0, xzr

The kernel should _NEVER_ write to CNTFRQ. It is not guaranteed to be
accessible from the non-secure side. The firmware should have configured
this appropriately.

If the firmware does not configure CNTFRQ correctly then it is buggy and
must be fixed.

Overriding the CNTFRQ to zero is completely broken.

> were added for
> ##########
> SANITY CHECK: Unexpected variation in cntfrq. Boot CPU:
> 0x00000002faf080, CPU1: 0x00000000000000
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 0 at arch/arm64/kernel/cpuinfo.c:146
> cpuinfo_store_cpu+0x2e8/0x14f4()
> Unsupported CPU feature variation.
> Modules linked in:
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.16.0-rc4+ #196
> Call trace:
> [<ffffffc000087fd0>] dump_backtrace+0x0/0x130
> [<ffffffc000088110>] show_stack+0x10/0x1c
> [<ffffffc0004c03e0>] dump_stack+0x74/0xb8
> [<ffffffc00009dfac>] warn_slowpath_common+0x8c/0xb4
> [<ffffffc00009e078>] warn_slowpath_fmt_taint+0x4c/0x58
> [<ffffffc00008adf0>] cpuinfo_store_cpu+0x2e4/0x14f4
> [<ffffffc00008f7e0>] secondary_start_kernel+0xd4/0x120
> ---[ end trace 62c0cf48de286b1c ]---
> CPU2: Booted secondary processor
> Detected PIPT I-cache on CPU2
> SANITY CHECK: Unexpected variation in cntfrq. Boot CPU:
> 0x00000002faf080, CPU2: 0x00000000000000
> CPU3: Booted secondary processor
> ##########
> 
> does this mean we also need to stop/handle timers before
> jumping to new kernel??

The value of CNTFRQ has nothing to do with whether the timers are
enabled. If we don't already disabling the timers I guess we could as a
courtesy to the next kernel, but interrupts should be masked and the
next kernel should be able to handle it anyway. That would all live in
the timer driver anyhow.

Do you see the sanity check failure on a cold boot (with the same
kernel)?

 - If so, your firmware is not configuring CNTFRQ appropriately as it
   MUST do. Virtualization IS broken on your system. This is a bug in
   the firmware.

 - If not, then something in the spin-table's cpu return path is
   resulting in CPUs being reconfigured, or reset and not configured as
   with cold boot. This is a bug in the firmware.

The firmware MUST configure CNTFRQ. It must do so regardless of
cold/warm boot. A clock-frequency property in the DT is insufficient;
it's a half-baked workaround that's especially problematic if you boot
at EL2.

> > I really can't say. I know nothing of your platform or spin-table
> > implementation, and this is nothing like what I'd expect the jump to EL2
> > code to look like.
> 
> Yes It was just for trying things out. Traping to EL2 and then jumping
> to the kexec/SMP-spin code is not sane. And by the way
> I did not find anyway to do mode change to el2 from el1 other than
> trapping.
> 
> Could you please elaborate more about the way you expect
> it to be working/implemented??.

Elsewhere in this thread I gave an outline of how I would expect this to
be implemented with modifications to KVM and the hyp stub.

Obviously some trap will be taken to EL2, but that should be as a result
of an explicit HVC rather than an access to the CPACR, and it should
only occur when the kernel was booted at EL2.

Thanks,
Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

Hi Mark,

On Fri, Aug 22, 2014 at 6:45 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> [...]
>
>> >> > Your spin-table implementation might be poking something that's not
>> >> > accessible at EL1, in which case you require the jump to EL2 for
>> >> > correctness. I can't say for certain either way.
>> >> >
>> >>
>> >> Yes you are right I needed a jump to EL2 for putting the
>> >> secondary CPUs at correct spinning loop.
>> >
>> > Ok. So we need to do what I have suggested elsewhere w.r.t. jumping back
>> > up to EL2. As you point out below we need to synchronise with the CPUs
>> > on their way out too we can add a spin-table cpu_kill with a slightly
>> > dodgy delay to ensure that, given we have no other mechanism for
>> > synchronising.
>> >
>> Ok
>>
>> >
>> > As far as I am aware an "isb; dsb" sequence never makes sense. It should
>> > be "dsb; isb". You want the I-side maintenance to complete (for which
>> > you have the DSB) _then_ you want to synchronise the execution context
>> > with the newly-visible instructions (for which you have the ISB).
>> >
>> Thanks for the nice explanation.
>>
>> Now I am able to kexec an SMP kernel with the below change.
>>
>> ##########
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 607d4bb..8969922 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -343,6 +343,11 @@ CPU_LE(    movk    x0, #0x30d0, lsl #16    )
>>  // Clear EE and E0E on LE systems
>>                       PSR_MODE_EL1h)
>>         msr     spsr_el2, x0
>>         msr     elr_el2, lr
>> +       mov     x0, #0x33ff
>> +       movk    x0, #0x801f, lsl #16
>> +       msr     cptr_el2, x0                    // Enable traps to el2
>> when CPACR or CPACR_EL1 is accessed
>> +       mov     x0, #3 << 20
>> +       msr     cpacr_el1, x0                   // Enable FP/ASIMD
>>         mov     w20, #BOOT_CPU_MODE_EL2         // This CPU booted in EL2
>>         eret
>>  ENDPROC(el2_setup)
>> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
>> index a272f33..1fb4c38 100644
>> --- a/arch/arm64/kernel/hyp-stub.S
>> +++ b/arch/arm64/kernel/hyp-stub.S
>> @@ -56,12 +56,25 @@ el1_sync:
>>         mrs     x1, esr_el2
>>         lsr     x1, x1, #26
>>         cmp     x1, #0x16
>> -       b.ne    2f                              // Not an HVC trap
>> +       b.ne    3f                              // Not an HVC trap
>>         cbz     x0, 1f
>>         msr     vbar_el2, x0                    // Set vbar_el2
>>         b       2f
>>  1:     mrs     x0, vbar_el2                    // Return vbar_el2
>>  2:     eret
>> +3:     cmp     x1, #0x18       // Traps to EL2 for CPACR access have this code
>> +       b.ne    2b
>> +
>> +       mov     x1, #0x33ff
>> +       msr     cptr_el2, x1                    // Disable copro. traps to EL2
>> +
>> +       msr     cntp_ctl_el0, xzr
>> +       msr     cntfrq_el0, xzr
>> +       ic      ialluis
>> +       dsb     sy
>> +       isb
>> +
>> +       br  x0
>>  ENDPROC(el1_sync)
>>
>>  .macro invalid_vector  label
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 64733d4..77298c2 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -65,6 +65,14 @@ void soft_restart(unsigned long addr)
>>  //     smp_secondary_shutdown();
>>  #endif
>>
>> +       /* Delay primary cpu; allow enough time for
>> +        * secondaries to enter spin loop
>> +        */
>> +#if defined(CONFIG_SMP)
>> +       if (smp_processor_id() == 0)
>> +               mdelay(1000);
>> +#endif
>> +
>>         cpu_soft_restart(virt_to_phys(cpu_reset), addr);
>>
>>         /* Should never get here */
>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> index 3cb6dec..f6ab468 100644
>> --- a/arch/arm64/mm/proc.S
>> +++ b/arch/arm64/mm/proc.S
>> @@ -76,6 +76,10 @@ ENTRY(cpu_reset)
>>  #if defined(CONFIG_SMP)
>>  /*     bl      secondary_shutdown */
>>  #endif
>> +
>> +       /* Trap to EL2 */
>> +       mov     x1, #3 << 20
>> +       msr     cpacr_el1, x1                   // Enable FP/ASIMD
>>         ret     x0
>>  ENDPROC(cpu_reset)
>>
>> @@ -200,8 +204,6 @@ ENTRY(__cpu_setup)
>>         tlbi    vmalle1is                       // invalidate I + D TLBs
>>         dsb     ish
>>
>> -       mov     x0, #3 << 20
>> -       msr     cpacr_el1, x0                   // Enable FP/ASIMD
>>         msr     mdscr_el1, xzr                  // Reset mdscr_el1
>>         /*
>>          * Memory region attributes for LPAE:
>> ##########
>>
>> The two lines
>> +       msr     cntp_ctl_el0, xzr
>> +       msr     cntfrq_el0, xzr
>
> The kernel should _NEVER_ write to CNTFRQ. It is not guaranteed to be
> accessible from the non-secure side. The firmware should have configured
> this appropriately.
>
> If the firmware does not configure CNTFRQ correctly then it is buggy and
> must be fixed.
>
> Overriding the CNTFRQ to zero is completely broken.
>
>> were added for
>> ##########
>> SANITY CHECK: Unexpected variation in cntfrq. Boot CPU:
>> 0x00000002faf080, CPU1: 0x00000000000000
>> ------------[ cut here ]------------
>> WARNING: CPU: 1 PID: 0 at arch/arm64/kernel/cpuinfo.c:146
>> cpuinfo_store_cpu+0x2e8/0x14f4()
>> Unsupported CPU feature variation.
>> Modules linked in:
>> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.16.0-rc4+ #196
>> Call trace:
>> [<ffffffc000087fd0>] dump_backtrace+0x0/0x130
>> [<ffffffc000088110>] show_stack+0x10/0x1c
>> [<ffffffc0004c03e0>] dump_stack+0x74/0xb8
>> [<ffffffc00009dfac>] warn_slowpath_common+0x8c/0xb4
>> [<ffffffc00009e078>] warn_slowpath_fmt_taint+0x4c/0x58
>> [<ffffffc00008adf0>] cpuinfo_store_cpu+0x2e4/0x14f4
>> [<ffffffc00008f7e0>] secondary_start_kernel+0xd4/0x120
>> ---[ end trace 62c0cf48de286b1c ]---
>> CPU2: Booted secondary processor
>> Detected PIPT I-cache on CPU2
>> SANITY CHECK: Unexpected variation in cntfrq. Boot CPU:
>> 0x00000002faf080, CPU2: 0x00000000000000
>> CPU3: Booted secondary processor
>> ##########
>>
>> does this mean we also need to stop/handle timers before
>> jumping to new kernel??
>
> The value of CNTFRQ has nothing to do with whether the timers are
> enabled. If we don't already disabling the timers I guess we could as a
> courtesy to the next kernel, but interrupts should be masked and the
> next kernel should be able to handle it anyway. That would all live in
> the timer driver anyhow.
>
> Do you see the sanity check failure on a cold boot (with the same
> kernel)?
>

No I don't see the failure on cold boot.

>  - If so, your firmware is not configuring CNTFRQ appropriately as it
>    MUST do. Virtualization IS broken on your system. This is a bug in
>    the firmware.
>
>  - If not, then something in the spin-table's cpu return path is
>    resulting in CPUs being reconfigured, or reset and not configured as
>    with cold boot. This is a bug in the firmware.

I will report this to my hardware guys.
>
> The firmware MUST configure CNTFRQ. It must do so regardless of
> cold/warm boot. A clock-frequency property in the DT is insufficient;
> it's a half-baked workaround that's especially problematic if you boot
> at EL2.
>
>> > I really can't say. I know nothing of your platform or spin-table
>> > implementation, and this is nothing like what I'd expect the jump to EL2
>> > code to look like.
>>
>> Yes It was just for trying things out. Traping to EL2 and then jumping
>> to the kexec/SMP-spin code is not sane. And by the way
>> I did not find anyway to do mode change to el2 from el1 other than
>> trapping.
>>
>> Could you please elaborate more about the way you expect
>> it to be working/implemented??.
>
> Elsewhere in this thread I gave an outline of how I would expect this to
> be implemented with modifications to KVM and the hyp stub.
>
OK. I will give it a try.

> Obviously some trap will be taken to EL2, but that should be as a result
> of an explicit HVC rather than an access to the CPACR, and it should
> only occur when the kernel was booted at EL2.
>
Ok. Thanks for the nice explanation.

--Arun

[PATCH] Arm64: convert soft_restart() to assembly code

[Arun Chandran]

On Thu, Aug 21, 2014 at 8:01 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Aug 21, 2014 at 02:34:46PM +0100, Arun Chandran wrote:
>> Hi Mark,
>>
>> On Wed, Aug 20, 2014 at 7:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > [...]
>> >
>> >> I am trying to kexec a UP-LE kernel from and SMP-LE kernel.
>> >>
>> >> > As I mentioned we do need to ensure that the CPUs are in the mode they
>> >> > started in, though I'm not sure I follow what you mean by "not waiting".
>> >> > This could be an orthogonal issue.
>> >>
>> >> If I verify the secondary CPUs from u-boot I can see that
>> >> they are all looping at
>> >>
>> >>     Core number       : 1
>> >>     Core state        : debug (AArch64 EL2)
>> >>     Debug entry cause : External Debug Request
>> >>     Current PC        : 0x0000000000000238
>> >>     Current CPSR      : 0x200003c9 (EL2h)
>> >>
>> >> But after the kexec calls soft_restar(0) for all secondary CPUs
>> >> they are looping at
>> >>
>> >>     Core number       : 1
>> >>     Core state        : debug (AArch64 EL1)
>> >>     Debug entry cause : External Debug Request
>> >>     Current PC        : 0xffffffc000083200
>> >>     Current CPSR      : 0x600003c5 (EL1h)
>> >>
>> >> This is what I mean by they are not waiting for
>> >> the secondary start-up address to jump.
>> >
>> > Ok.
>> >
>> >> >
>> >> > What exactly do you see, do the CPUs leave the spin-table, are they
>> >> > taking exceptions, are they getting stuck in the spin-table, etc?
>> >> >
>> >> They all are clearly resetting to address "0"(Put a breakpoint and
>> >> verified) but somehow they end up @0xffffffc000083200.
>> >> I still don't know why.
>> >>
>> >> ########
>> >> ffffffc00008319c:       d503201f        nop
>> >>         ...
>> >> ffffffc000083200:       14000260        b       ffffffc000083b80 <el1_sync>
>> >> ffffffc000083204:       d503201f        nop
>> >> ffffffc000083208:       d503201f        nop
>> >> ########
>> >
>> > That's the EL1 exception vector table.
>> >
>> > What looks to be happening is that something causes the CPUs to take an
>> > exception (at EL1). Because translation isn't enabled and the vector
>> > address doesn't map to anything, they'll take some sort of exception.
>> > Because the vectors aren't mapped that will go recursive.
>> >
>> > Your spin-table implementation might be poking something that's not
>> > accessible at EL1, in which case you require the jump to EL2 for
>> > correctness. I can't say for certain either way.
>> >
>>
>> Yes you are right I needed a jump to EL2 for putting the
>> secondary CPUs at correct spinning loop.
>
> Ok. So we need to do what I have suggested elsewhere w.r.t. jumping back
> up to EL2. As you point out below we need to synchronise with the CPUs
> on their way out too we can add a spin-table cpu_kill with a slightly
> dodgy delay to ensure that, given we have no other mechanism for
> synchronising.
>

I was able to remove the delay by pushing "set_cpu_online(cpu, false);"
further down.

############
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 3fb64cb..200e49e 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -543,8 +543,6 @@ static void ipi_cpu_stop(unsigned int cpu)
                raw_spin_unlock(&stop_lock);
        }

-       set_cpu_online(cpu, false);
-
        local_irq_disable();

        /* If we have the cpu ops use them. */
@@ -554,6 +552,8 @@ static void ipi_cpu_stop(unsigned int cpu)
                && !cpu_ops[cpu]->cpu_disable(cpu))
                cpu_ops[cpu]->cpu_die(cpu);

+
+       set_cpu_online(cpu, false);
        /* Otherwise spin here. */

        while (1)
diff --git a/arch/arm64/kernel/smp_spin_table.c
b/arch/arm64/kernel/smp_spin_table.c
index e7275b3..8dcca88 100644
--- a/arch/arm64/kernel/smp_spin_table.c
+++ b/arch/arm64/kernel/smp_spin_table.c
@@ -149,6 +149,7 @@ static void smp_spin_table_cpu_die(unsigned int cpu)
        *release_addr = 0;
        __flush_dcache_area(release_addr, 8);

+       set_cpu_online(cpu, false);
        mb();

        soft_restart(0);
##############

This will

a) Make the waiting loop inside smp_send_stop() more meaningful

b) Make sure that at least cpu-release-addr is invalidated.

--Arun


>> I made some progress with the below changes.
>>
>> ###########
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 607d4bb..8969922 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -343,6 +343,11 @@ CPU_LE(    movk    x0, #0x30d0, lsl #16    )
>>  // Clear EE and E0E on LE systems
>>                       PSR_MODE_EL1h)
>>         msr     spsr_el2, x0
>>         msr     elr_el2, lr
>> +       mov     x0, #0x33ff
>> +       movk    x0, #0x801f, lsl #16
>> +       msr     cptr_el2, x0                    // Enable traps to el2
>> when CPACR or CPACR_EL1 is accessed
>> +       mov     x0, #3 << 20
>> +       msr     cpacr_el1, x0                   // Enable FP/ASIMD
>>         mov     w20, #BOOT_CPU_MODE_EL2         // This CPU booted in EL2
>>         eret
>>  ENDPROC(el2_setup)
>> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
>> index a272f33..d8e806c 100644
>> --- a/arch/arm64/kernel/hyp-stub.S
>> +++ b/arch/arm64/kernel/hyp-stub.S
>> @@ -66,6 +66,14 @@ ENDPROC(el1_sync)
>>
>>  .macro invalid_vector  label
>>  \label:
>> +       mov     x1, #0x33ff
>> +       msr     cptr_el2, x1                    // Disable copro. traps to EL2
>> +
>> +       ic      ialluis
>> +       isb
>> +       dsb     sy
>
> As far as I am aware an "isb; dsb" sequence never makes sense. It should
> be "dsb; isb". You want the I-side maintenance to complete (for which
> you have the DSB) _then_ you want to synchronise the execution context
> with the newly-visible instructions (for which you have the ISB).
>
>> +
>> +       br  x0
>>         b \label
>>  ENDPROC(\label)
>>  .endm
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 64733d4..8ca929c 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -65,6 +65,12 @@ void soft_restart(unsigned long addr)
>>  //     smp_secondary_shutdown();
>>  #endif
>>
>> +       /* Delay primary cpu; allow enough time for
>> +        * secondaries to enter spin loop
>> +        */
>> +       if (smp_processor_id() == 0)
>> +               mdelay(1000);
>> +
>>         cpu_soft_restart(virt_to_phys(cpu_reset), addr);
>>
>>         /* Should never get here */
>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> index 3cb6dec..f6ab468 100644
>> --- a/arch/arm64/mm/proc.S
>> +++ b/arch/arm64/mm/proc.S
>> @@ -76,6 +76,10 @@ ENTRY(cpu_reset)
>>  #if defined(CONFIG_SMP)
>>  /*     bl      secondary_shutdown */
>>  #endif
>> +
>> +       /* Trap to EL2 */
>> +       mov     x1, #3 << 20
>> +       msr     cpacr_el1, x1                   // Enable FP/ASIMD
>>         ret     x0
>>  ENDPROC(cpu_reset)
>>
>> @@ -200,8 +204,6 @@ ENTRY(__cpu_setup)
>>         tlbi    vmalle1is                       // invalidate I + D TLBs
>>         dsb     ish
>>
>> -       mov     x0, #3 << 20
>> -       msr     cpacr_el1, x0                   // Enable FP/ASIMD
>>         msr     mdscr_el1, xzr                  // Reset mdscr_el1
>>         /*
>>          * Memory region attributes for LPAE:
>> #########
>>
>> With the above code I was able to manually kexec
>> an SMP kernel (With the help of debugger).
>>
>> Basically the branch instruction (br x0) is not working
>> in the el2 vector.
>> ------------------
>> CPU#0>h
>>     Core number       : 0
>>     Core state        : debug (AArch64 EL2)
>>     Debug entry cause : External Debug Request
>>     Current PC        : 0x0000004000089800
>
> Does this address look similar to the VBAR_EL2, perhaps?
>
>>     Current CPSR      : 0x600003c9 (EL2h)
>> CPU#0>rd
>> GPR00: 00000043eb891000 0000000000000018 0000000000000006 0000000000000004
>> GPR04: 000000000000001f 000000000000001b 0000000000000000 ffffffffffffffff
>> GPR08: ffffffc3ffe80614 ffffffffffffffff 0000000000000000 0000000000000002
>> GPR12: ffffffc0000968f0 00000040008c0000 00000043eb949002 ffffffffffffffff
>> GPR16: ffffffc0000c1244 0000000000435260 0000007ff9c5d490 00000040000968c0
>> GPR20: 00000043eb891000 ffffffc3eb891000 ffffffc3eb973c00 0000000000000110
>> GPR24: ffffffc000094000 ffffffc000094cd8 000000000000008e ffffffc00082f000
>> GPR28: ffffffc3e9888000 ffffffc3e988bd00 ffffffc000095d88 00000043efb24430
>> CPU#0>go 0x00000043eb891000
>> --------------------
>>
>> I had to use the debugger  to make cpu0 jump to kexec-boot code.
>> Similarly I had to manually put other CPUs to spinning code
>> using debugger. Could you please throw some light here?
>
> I really can't say. I know nothing of your platform or spin-table
> implementation, and this is nothing like what I'd expect the jump to EL2
> code to look like.
>
> Thanks,
> Mark.

[PATCH] Arm64: convert soft_restart() to assembly code

[Mark Rutland]

Hi Arun,

Please start a new thread if you have new things to say; this thread has
drifted far from its original purpose (the titular patch is now in the
arm64 devel branch [1]).

[...]

> > Ok. So we need to do what I have suggested elsewhere w.r.t. jumping back
> > up to EL2. As you point out below we need to synchronise with the CPUs
> > on their way out too we can add a spin-table cpu_kill with a slightly
> > dodgy delay to ensure that, given we have no other mechanism for
> > synchronising.
> >
> 
> I was able to remove the delay by pushing "set_cpu_online(cpu, false);"
> further down.
> 
> ############
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 3fb64cb..200e49e 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -543,8 +543,6 @@ static void ipi_cpu_stop(unsigned int cpu)
>                 raw_spin_unlock(&stop_lock);
>         }
> 
> -       set_cpu_online(cpu, false);
> -
>         local_irq_disable();
> 

If we don't set the cpu offline here, we'll call
clear_tasks_mm_cpumask(cpu) and migrate IRQs while the CPU is marked
online, wait a bit, then mark the CPU offline. 

I suspect this is broken, though I could be wrong.

>         /* If we have the cpu ops use them. */
> @@ -554,6 +552,8 @@ static void ipi_cpu_stop(unsigned int cpu)
>                 && !cpu_ops[cpu]->cpu_disable(cpu))
>                 cpu_ops[cpu]->cpu_die(cpu);
> 
> +
> +       set_cpu_online(cpu, false);
>         /* Otherwise spin here. */
> 
>         while (1)
> diff --git a/arch/arm64/kernel/smp_spin_table.c
> b/arch/arm64/kernel/smp_spin_table.c
> index e7275b3..8dcca88 100644
> --- a/arch/arm64/kernel/smp_spin_table.c
> +++ b/arch/arm64/kernel/smp_spin_table.c
> @@ -149,6 +149,7 @@ static void smp_spin_table_cpu_die(unsigned int cpu)
>         *release_addr = 0;
>         __flush_dcache_area(release_addr, 8);
> 
> +       set_cpu_online(cpu, false);
>         mb();
> 
>         soft_restart(0);
> ##############
> 
> This will
> 
> a) Make the waiting loop inside smp_send_stop() more meaningful

I don't follow how this is any more meaningful. We still have no
guarantee that the CPU is actually dead.

> b) Make sure that at least cpu-release-addr is invalidated.

There is still a period between the call to set_cpu_online(cpu, false)
and the CPU jumping to the return-address where it is still in the
kernel, so all this does is shorten the window for the race.

For PSCI 0.2+ we can poll to determine when the CPU is in the firmware.
For PSCI prior to 0.2 we can't, but the window is very short (as the
firmware performs the cache maintenance) and we seem to have gotten away
with it so far.

For spin-table we might have a large race window because the kernel must
flush the caches at EL2, incurring a relatively large delay. If we are
encountering a race there I'd rather this were fixed with a cpu_kill
callback for spin-table.

Cheers,
Mark.

[1] https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/commit/?h=devel&id=6c80fe35fe9edf9147e3db9c8ff1a7761c49c4cc

[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