Re: [PATCH 9/9] arm: add vectors support

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

On Sun, Oct 20, 2013 at 06:35:34PM +0200, Andrew Jones wrote:
> On Thu, Oct 17, 2013 at 11:58:43AM -0700, Christoffer Dall wrote:
> > On Thu, Oct 17, 2013 at 12:38:59PM +0200, Andrew Jones wrote:
> > > On Wed, Oct 16, 2013 at 06:06:42PM -0700, Christoffer Dall wrote:
> > > > > +++ b/arm/cstart.S
> > > > > @@ -1,5 +1,6 @@
> > > > >  
> > > > >  #define CR_B	(1 << 7)
> > > > > +#define CR_V	(1 << 13)
> > > > >  
> > > > >  .arm
> > > > >  
> > > > > @@ -12,6 +13,13 @@ start:
> > > > >  	push	{ r0-r3 }		@push r3 too for 8-byte alignment
> > > > >  
> > > > >  	mrc	p15, 0, r8, c1, c0, 0	@r8 = sctrl
> > > > > +
> > > > > +	/* set up vector table */
> > > > > +	bic	r8, #CR_V		@sctrl.V = 0
> > > > > +	mcr	p15, 0, r8, c1, c0, 0
> > > > > +	ldr	r0, =vector_table	@vbar = vector_table
> > > > > +	mcr     p15, 0, r0, c12, c0, 0
> > > > > +
> > > > >  	bl	get_endianness
> > > > >  	bl	io_init
> > > > >  
> > > > > @@ -41,6 +49,44 @@ halt:
> > > > >  1:	wfi
> > > > >  	b	1b
> > > > >  
> > > > > +vector_common:
> > > > > +	add	r2, sp, #(14 * 4)
> > > > 
> > > > this looks weird, what are you pointing to here?
> > > 
> > > What sp was at the time of exception. So if you look at ex_regs->sp,
> > > then you'll see what sp was when the exception occurred, not that plus
> > > what we're pushing on now for the handler.
> > > 
> > 
> > Hmmm, so you're assuming that all exceptions will be taken from SVC
> > mode?  I assume we will run tests in more than SVC mode, no?
> > 
> > Also note that the lr you're pushing here is not the lr at the time the
> > exception occurs, but the return address from the exception.  If the SVC
> > instruction is executed from SVC mode, the original lr is lost iirc, and
> > the caller needs to save it.  If you're from user mode, something like
> > 
> >     stm sp, {r0-lr}^
> > 
> > will take care of this for you, and if you're from svc
> > mode, you may want consider doing something like
> > 
> >     push {sp, lr}
> >     push {r0-r12}
> > 
> > instead (assuming this is only ever compiled in ARM mode, not Thumb2, in
> > which case the whole thing gets more complicated.
> 
> I think the lr pushing should be ok. That part is done in the macro that
> all vectors start with.  It got snipped from this mail, so here it is
> 
> .macro m_vector, v
> 	push    { r0-r12,lr }
> 	mov     r1, \v
> 	b       vector_common
> .endm

You push this lr after taking the exception, so the lr will point to the
instruction following the trap instruction itself; if your convention is
that the regs struct is the lr as it was when the trap was generated,
well then, things don't match.  If the exception is taken from SVC mode,
I'm not sure there's a proper way to get this information though, so it
really depends on how you imagine this to be used...

> 
> I realize it may still not be correct, just as the calculated sp
> may not be correct, depending on which mode,vector combo is used,
> but I was expecting to have different paths in the C code for
> fixing it up. Although that said, I haven't thought about it a
> bunch yet, so maybe it won't work that way...
> 

I think it may be worth taking an extra look at how the kernel handles
all this and somewhat replicate some of what we're doing, but again, it
depends on what the goal here is.

The SP calculation is probably correct, but it's error-prone and bound
to confuse people IMHO, but it's up to you at this point.

-Christoffer