[PATCH 03/11] Thumb-2: Implementation of the unified start-up and exceptions code

[PATCH 03/11] Thumb-2: Implementation of the unified start-up and exceptions code

[Anders Grafström]

Catalin Marinas wrote:
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index b55cb03..9341b38 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -33,14 +33,7 @@ ret_fast_syscall:
>  	/* perform architecture specific actions before user return */
>  	arch_ret_to_user r1, lr
>  
> -	@ fast_restore_user_regs
> -	ldr	r1, [sp, #S_OFF + S_PSR]	@ get calling cpsr
> -	ldr	lr, [sp, #S_OFF + S_PC]!	@ get pc
> -	msr	spsr_cxsf, r1			@ save in spsr_svc
> -	ldmdb	sp, {r1 - lr}^			@ get calling r1 - lr
> -	mov	r0, r0
> -	add	sp, sp, #S_FRAME_SIZE - S_PC
> -	movs	pc, lr				@ return & move spsr_svc into cpsr
> +	restore_user_regs fast = 1, offset = S_OFF
>   UNWIND(.fnend		)
>  
>  /*
> @@ -73,14 +66,7 @@ no_work_pending:
>  	/* perform architecture specific actions before user return */
>  	arch_ret_to_user r1, lr
>  
> -	@ slow_restore_user_regs
> -	ldr	r1, [sp, #S_PSR]		@ get calling cpsr
> -	ldr	lr, [sp, #S_PC]!		@ get pc
> -	msr	spsr_cxsf, r1			@ save in spsr_svc
> -	ldmdb	sp, {r0 - lr}^			@ get calling r0 - lr
> -	mov	r0, r0
> -	add	sp, sp, #S_FRAME_SIZE - S_PC
> -	movs	pc, lr				@ return & move spsr_svc into cpsr
> +	restore_user_regs fast = 0, offset = 0
>  ENDPROC(ret_to_user)
>  
>  /*

> diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S
> +
> +	.macro	restore_user_regs, fast = 0, offset = 0
> +	ldr	r1, [sp, #\offset + S_PSR]	@ get calling cpsr
> +	ldr	lr, [sp, #\offset + S_PC]!	@ get pc
> +	msr	spsr_cxsf, r1			@ save in spsr_svc
> +	.if	\fast
> +	ldmdb	sp, {r1 - lr}^			@ get calling r1 - lr
> +	.else
> +	ldmdb	sp, {r0 - lr}^			@ get calling r0 - lr
> +	.endif
> +	add	sp, sp, #S_FRAME_SIZE - S_PC
> +	movs	pc, lr				@ return & move spsr_svc into cpsr
> +	.endm

This commit seems to have broken things for ARM720T and it looks like
the removal of the "mov r0, r0" instruction in restore_user_regs is what caused it.

The patch below makes it work again but why?

diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S
index 7e9ed1e..516d14e 100644
--- a/arch/arm/kernel/entry-header.S
+++ b/arch/arm/kernel/entry-header.S
@@ -102,6 +102,7 @@
 	.else
 	ldmdb	sp, {r0 - lr}^			@ get calling r0 - lr
 	.endif
+	mov	r0, r0
 	add	sp, sp, #S_FRAME_SIZE - S_PC
 	movs	pc, lr				@ return & move spsr_svc into cpsr
 	.endm

[PATCH 03/11] Thumb-2: Implementation of the unified start-up and exceptions code

[Russell King - ARM Linux]

On Fri, Mar 12, 2010 at 06:30:36PM +0100, Anders Grafstr?m wrote:
> This commit seems to have broken things for ARM720T and it looks like
> the removal of the "mov r0, r0" instruction in restore_user_regs is
> what caused it.
> 
> The patch below makes it work again but why?

Because the 'add' instruction will end up touching the _userspace_
stack pointer, not the SVC stack pointer.

It's a question of timing in the core - the ldmdb instruction accesses
the user sp+lr, which is still in progress when the following instruction
is executed.  So, referencing 'sp' in the following instruction ends up
hitting the user mode stack pointer.

I'm surprised this hasn't caused more problems - there is a stipulated
requirement that banked registers are _NOT_ accessed by the instruction
following a ldm { }^ instruction - this requirement goes all the way up
to V5T as defined by DDI0100I and below:

  In ARM architecture versions earlier than ARMv6, this form of LDM must
  not be followed by an instruction that accesses banked registers. A
  following NOP is a good way to ensure this.

So yes, your patch is absolutely required - all ARMv5 and below are
facing a hazard as the kernel stands.  It might explain some of the
weird behaviours people have been reporting as well.

Please cleanup the description and submit to the patch system.

[PATCH 03/11] Thumb-2: Implementation of the unified start-up and exceptions code

[Nicolas Pitre]

On Fri, 12 Mar 2010, Russell King - ARM Linux wrote:

> So yes, your patch is absolutely required - all ARMv5 and below are
> facing a hazard as the kernel stands.  It might explain some of the
> weird behaviours people have been reporting as well.
> 
> Please cleanup the description and submit to the patch system.

And please add a small comment along side the otherwise seemingly 
useless nop.


Nicolas

[PATCH 03/11] Thumb-2: Implementation of the unified start-up and exceptions code

[Anders Grafström]

Nicolas Pitre wrote:
> On Fri, 12 Mar 2010, Russell King - ARM Linux wrote:
> 
>> So yes, your patch is absolutely required - all ARMv5 and below are
>> facing a hazard as the kernel stands.  It might explain some of the
>> weird behaviours people have been reporting as well.
>>
>> Please cleanup the description and submit to the patch system.
> 
> And please add a small comment along side the otherwise seemingly 
> useless nop.

Done.

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=5991/1

[PATCH 03/11] Thumb-2: Implementation of the unified start-up and exceptions code

[Catalin Marinas]

On Mon, 2010-03-15 at 15:20 +0000, Anders Grafstr?m wrote:
> Nicolas Pitre wrote:
> > On Fri, 12 Mar 2010, Russell King - ARM Linux wrote:
> >
> >> So yes, your patch is absolutely required - all ARMv5 and below are
> >> facing a hazard as the kernel stands.  It might explain some of the
> >> weird behaviours people have been reporting as well.
> >>
> >> Please cleanup the description and submit to the patch system.
> >
> > And please add a small comment along side the otherwise seemingly
> > useless nop.
> 
> Done.
> 
> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=5991/1
> 
BTW, should we have an #ifdef for __LINUX_ARM_ARCH__ >= 6? Not that it
would make a noticeable difference in speed.

-- 
Catalin