Re: [PATCH 17/17] arm: vectors support

[Christoffer Dall <christoffer.dall@linaro.org>]

On Mon, Feb 03, 2014 at 05:50:20PM +0100, Andrew Jones wrote:
> On Sat, Feb 01, 2014 at 06:29:27PM -0800, Christoffer Dall wrote:
> > On Tue, Jan 21, 2014 at 05:22:03PM +0100, Andrew Jones wrote:
> > > Add support for tests to use exception handlers.
> > > 
> > > v2 -> v3:
> > > - squashed in 'arm: Simplify exceptions_init in cstart.S' from
> > >   Christoffer Dall
> > > - suggested function name changes and comment additions [Christoffer Dall]
> > > - fix a bug with stack restore from usr mode exceptions that Christoffer
> > >   pointed out. Add a get_sp() accessor too.
> > 
> > hmmm, how did you address this, the only change I can see is using r6
> > instead of r2, which I'm not sure how changes anything.
> 
> It also adds
> 
> 	/* make sure we restore sp_svc and lr_svc on mode change */
> 	str	r6, [sp, #S_SP]
> 	str	lr, [sp, #S_LR]
> 
> lower down. Needed to switch to r6 from r2 for that
> 
> > 
> > The problem is that your ldmia is replacing the usr sp, not the svc sp,
> > so maybe you need to do ldmia sp!, [ r0 - pc ]^ (don't remember if that
> > particular combination works) or you need to do something more fancy...
> 
> I'm not sure about the magic ! + ^ stuff, but what I've done does fix
> the problem. I tested before/after the fix (actually that's why I also
> added the get_sp).
> 

Ah, I know, my original comment was wrong, what you were in fact doing
was restoring the user sp to the svc reg, the write to the CPSR from the
SPSR happens after restoring all the other regs (except the PC)
according to the ldm exception return pseudocode.

ok, I think it's correct now, except for the bit about the lr, see
below.

> > > diff --git a/arm/cstart.S b/arm/cstart.S
> > > index 4de04b62c28c2..74674fd41c1b3 100644
> > > --- a/arm/cstart.S
> > > +++ b/arm/cstart.S
> > > @@ -1,3 +1,7 @@
> > > +#define __ASSEMBLY__
> > > +#include "arm/asm-offsets.h"
> > > +#include "arm/ptrace.h"
> > > +#include "arm/cp15.h"
> > >  
> > >  .arm
> > >  
> > > @@ -10,6 +14,13 @@ start:
> > >  	 * See the kernel doc Documentation/arm/Booting
> > >  	 */
> > >  	ldr	sp, =stacktop
> > > +	push	{r0-r3}
> > > +
> > > +	/* set up vector table and mode stacks */
> > > +	bl	exceptions_init
> > > +
> > > +	/* complete setup */
> > > +	pop	{r0-r3}
> > >  	bl	setup
> > >  
> > >  	/* start the test */
> > > @@ -20,9 +31,166 @@ start:
> > >  	bl	exit
> > >  	b	halt
> > >  
> > > +.macro set_mode_stack mode, stack
> > > +	add	\stack, #S_FRAME_SIZE
> > > +	msr	cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT)
> > > +	mov	sp, \stack
> > > +.endm
> > > +
> > > +exceptions_init:
> > > +	mrc	p15, 0, r2, c1, c0, 0	@ read SCTLR
> > > +	bic	r2, #CR_V		@ SCTLR.V := 0
> > > +	mcr	p15, 0, r2, c1, c0, 0	@ write SCTLR
> > > +	ldr	r2, =vector_table
> > > +	mcr	p15, 0, r2, c12, c0, 0	@ write VBAR
> > > +
> > > +	mrs	r2, cpsr
> > > +	ldr	r1, =exception_stacks
> > > +		/* first frame for svc mode */
> > > +	set_mode_stack	UND_MODE, r1
> > > +	set_mode_stack	ABT_MODE, r1
> > > +	set_mode_stack	IRQ_MODE, r1
> > > +	set_mode_stack	FIQ_MODE, r1
> > > +	msr	cpsr_cxsf, r2	@ back to svc mode
> > > +	mov	pc, lr
> > > +
> > >  .text
> > >  
> > >  .globl halt
> > >  halt:
> > >  1:	wfi
> > >  	b	1b
> > > +
> > > +/*
> > > + * Vector stubs.
> > > + * Simplified version of the Linux kernel implementation
> > > + *   arch/arm/kernel/entry-armv.S
> > > + *
> > > + * Each mode has an S_FRAME_SIZE sized stack initialized
> > > + * in exceptions_init
> > > + */
> > > +.macro vector_stub, name, vec, mode, correction=0
> > > +.align 5
> > > +vector_\name:
> > > +.if \correction
> > > +	sub	lr, lr, #\correction
> > > +.endif
> > > +	/*
> > > +	 * Save r0, r1, lr_<exception> (parent PC)
> > > +	 * and spsr_<exception> (parent CPSR)
> > > +	 */
> > > +	str	r0, [sp, #S_R0]
> > > +	str	r1, [sp, #S_R1]
> > > +	str	lr, [sp, #S_PC]
> > > +	mrs	r0, spsr
> > > +	str	r0, [sp, #S_PSR]
> > > +
> > > +	/* Prepare for SVC32 mode. */
> > > +	mrs	r0, cpsr
> > > +	bic	r0, #MODE_MASK
> > > +	orr	r0, #SVC_MODE
> > > +	msr	spsr_cxsf, r0
> > > +
> > > +	/* Branch to handler in SVC mode */
> > > +	mov	r0, #\vec
> > > +	mov	r1, sp
> > > +	ldr	lr, =vector_common
> > > +	movs	pc, lr
> > > +.endm
> > > +
> > > +vector_stub 	rst,	0, UND_MODE
> > > +vector_stub	und,	1, UND_MODE
> > > +vector_stub	pabt,	3, ABT_MODE, 4
> > > +vector_stub	dabt,	4, ABT_MODE, 8
> > > +vector_stub	irq,	6, IRQ_MODE, 4
> > > +vector_stub	fiq,	7, FIQ_MODE, 4
> > > +
> > > +.align 5
> > > +vector_svc:
> > > +	/*
> > > +	 * Save r0, r1, lr_<exception> (parent PC)
> > > +	 * and spsr_<exception> (parent CPSR)
> > > +	 */
> > > +	push	{ r1 }
> > > +	ldr	r1, =exception_stacks
> > > +	str	r0, [r1, #S_R0]
> > > +	pop	{ r0 }
> > > +	str	r0, [r1, #S_R1]
> > > +	str	lr, [r1, #S_PC]
> > > +	mrs	r0, spsr
> > > +	str	r0, [r1, #S_PSR]
> > > +
> > > +	/*
> > > +	 * Branch to handler, still in SVC mode.
> > > +	 * r0 := 2 is the svc vector number.
> > > +	 */
> > > +	mov	r0, #2
> > > +	ldr	lr, =vector_common
> > > +	mov	pc, lr
> > > +
> > > +vector_common:
> > > +	/* make room for pt_regs */
> > > +	sub	sp, #S_FRAME_SIZE
> > > +	tst	sp, #4			@ check stack alignment
> > > +	subne	sp, #4
> > > +
> > > +	/* store registers r0-r12 */
> > > +	stmia	sp, { r0-r12 }		@ stored wrong r0 and r1, fix later
> > > +
> > > +	/* get registers saved in the stub */
> > > +	ldr	r2, [r1, #S_R0]		@ r0
> > > +	ldr	r3, [r1, #S_R1]		@ r1
> > > +	ldr	r4, [r1, #S_PC] 	@ lr_<exception> (parent PC)
> > > +	ldr	r5, [r1, #S_PSR]	@ spsr_<exception> (parent CPSR)
> > > +
> > > +	/* fix r0 and r1 */
> > > +	str	r2, [sp, #S_R0]
> > > +	str	r3, [sp, #S_R1]
> > > +
> > > +	/* store sp_svc, if we were in usr mode we'll fix this later */
> > > +	add	r6, sp, #S_FRAME_SIZE
> > > +	addne	r6, #4			@ stack wasn't aligned
> > > +	str	r6, [sp, #S_SP]
> > > +
> > > +	str	lr, [sp, #S_LR]		@ store lr_svc, fix later for usr mode
> > > +	str	r4, [sp, #S_PC]		@ store lr_<exception>
> > > +	str	r5, [sp, #S_PSR]	@ store spsr_<exception>
> > > +
> > > +	/* set ORIG_r0 */
> > > +	mov	r2, #-1
> > > +	str	r2, [sp, #S_OLD_R0]
> > > +
> > > +	/* if we were in usr mode then we need sp_usr and lr_usr instead */
> > > +	and	r1, r5, #MODE_MASK
> > > +	cmp	r1, #USR_MODE
> > > +	bne	1f
> > > +	add	r1, sp, #S_SP
> > > +	stmia	r1, { sp,lr }^
> > > +
> > > +	/* Call the handler. r0 is the vector number, r1 := pt_regs */
> > > +1:	mov	r1, sp
> > > +	bl	do_handle_exception
> > > +
> > > +	/* make sure we restore sp_svc and lr_svc on mode change */
> > > +	str	r6, [sp, #S_SP]
> > > +	str	lr, [sp, #S_LR]

I don't think store of lr is necessary, lr was just overwritten by the
bl instruction above, and it will get overwritten at exception entry to
SVC mode again.

> > > +
> > > +	/* return from exception */
> > > +	msr	spsr_cxsf, r5
> > > +	ldmia	sp, { r0-pc }^
> > > +
> > > +.align 5
> > > +vector_addrexcptn:
> > > +	b	vector_addrexcptn
> > > +
> > > +.section .text.ex
> > > +.align 5
> > > +vector_table:
> > > +	b	vector_rst
> > > +	b	vector_und
> > > +	b	vector_svc
> > > +	b	vector_pabt
> > > +	b	vector_dabt
> > > +	b	vector_addrexcptn	@ should never happen
> > > +	b	vector_irq
> > > +	b	vector_fiq

[...]

Thanks,
-Christoffer