ARM: fix v7-M signal return

Commit Message

Rabin Vincent May 4, 2014, 3:36 p.m. UTC

On Sat, May 03, 2014 at 08:45:12PM +0200, Uwe Kleine-König wrote:
> On Sat, May 03, 2014 at 07:39:03PM +0200, Rabin Vincent wrote:
> > I'm running this on QEMU.  Here is an old qemu-devel thread on this
> > topic if you are interested:
> > http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg00158.html
> I'm interested in your setup and (if applicable) additional kernel
> patches.

No kernel patches are needed other than the ones I already posted.  My
QEMU patches for EFM32 support are needed.  I put together a small
README here: https://github.com/rabinv/qemu-efm32-tools

> > +	@ ensure bit 0 is cleared in the PC
> Maybe add: , otherwise behaviour is unpredictable
> 
> > +	bic	r4, r4, #1
> I just notice that the coding style is inconsitent here, the
> instructions above don't repeat the dest register. Can you please make
> this "bic r4, #1", too?
> 
> With these two changes you can have my ack.

v3 below.

> What happens on qemu without this fix? If it crashes I'd suggest to get
> this patch into 3.15 with a stable annotation. If not I think 3.16-rc1
> is enough.

User space processes crash because qemu's emulation of the CPU does not
discard the zero bit.

8<--------------------
From 577b0c0e15057f58f86996da7f363c8a608c389f Mon Sep 17 00:00:00 2001
From: Rabin Vincent <[e-mail]>
Date: Sat, 3 May 2014 19:27:09 +0200
Subject: [PATCHv3] ARM: fix v7-M signal return

According to the ARM ARM, the behaviour is UNDPREDICTABLE if the PC read
from the exception return stack is not half word aligned.  See the
pseudo code for ExceptionReturn() and PopStack().

The signal handler's address has the bit 0 set, and setup_return()
directly writes this to regs->ARM_pc.  Mask out bit 0 before the
exception return to get predictable behaviour.

Signed-off-by: Rabin Vincent <[e-mail]>
---
 arch/arm/kernel/entry-header.S | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Uwe Kleine-König May 5, 2014, 1:28 p.m. UTC | #1

Hello Rabin,

On Sun, May 04, 2014 at 05:36:18PM +0200, Rabin Vincent wrote:
> On Sat, May 03, 2014 at 08:45:12PM +0200, Uwe Kleine-König wrote:
> > On Sat, May 03, 2014 at 07:39:03PM +0200, Rabin Vincent wrote:
> > > I'm running this on QEMU.  Here is an old qemu-devel thread on this
> > > topic if you are interested:
> > > http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg00158.html
> > I'm interested in your setup and (if applicable) additional kernel
> > patches.
> 
> No kernel patches are needed other than the ones I already posted.  My
> QEMU patches for EFM32 support are needed.  I put together a small
> README here: https://github.com/rabinv/qemu-efm32-tools
> 
> > > +	@ ensure bit 0 is cleared in the PC
> > Maybe add: , otherwise behaviour is unpredictable
> > 
> > > +	bic	r4, r4, #1
> > I just notice that the coding style is inconsitent here, the
> > instructions above don't repeat the dest register. Can you please make
> > this "bic r4, #1", too?
> > 
> > With these two changes you can have my ack.
> 
> v3 below.
> 
> > What happens on qemu without this fix? If it crashes I'd suggest to get
> > this patch into 3.15 with a stable annotation. If not I think 3.16-rc1
> > is enough.
> 
> User space processes crash because qemu's emulation of the CPU does not
> discard the zero bit.
> 
> 8<--------------------
> From 577b0c0e15057f58f86996da7f363c8a608c389f Mon Sep 17 00:00:00 2001
> From: Rabin Vincent <[e-mail]>
> Date: Sat, 3 May 2014 19:27:09 +0200
> Subject: [PATCHv3] ARM: fix v7-M signal return
> 
> According to the ARM ARM, the behaviour is UNDPREDICTABLE if the PC read
s/UNDPR/UNPR/

> from the exception return stack is not half word aligned.  See the
> pseudo code for ExceptionReturn() and PopStack().
> 
> The signal handler's address has the bit 0 set, and setup_return()
> directly writes this to regs->ARM_pc.  Mask out bit 0 before the
> exception return to get predictable behaviour.

Actual machines seem to behave just fine (i.e. discard this bit) but
qemu's emulation doesn't and makes processes crash.

Acked-by: Uwe Kleine-König <[e-mail]>
Fixes: 19c4d593f0b4 ("ARM: ARMv7-M: Add support for exception handling")
Cc: [e-mail]

----
Can you please put this in Russell's patch tracker.

Maybe pc in the saved struct regs should get bit 0 set during exception
entry for consistency?! (That would be a separate patch of course.)

Thanks
Uwe

Patch

diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S
index 1420725..efb208d 100644
--- a/arch/arm/kernel/entry-header.S
+++ b/arch/arm/kernel/entry-header.S
@@ -132,6 +132,10 @@ 
 	orrne	r5, V7M_xPSR_FRAMEPTRALIGN
 	biceq	r5, V7M_xPSR_FRAMEPTRALIGN
 
+	@ ensure bit 0 is cleared in the PC, otherwise behaviour is
+	@ unpredictable
+	bic	r4, #1
+
 	@ write basic exception frame
 	stmdb	r2!, {r1, r3-r5}
 	ldmia	sp, {r1, r3-r5}