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.
> >
> > > + mrs r3, spsr
> > > + push { r0-r3 } @push r0 too for padding
> > > + add r0, sp, #4 @skip pad
> >
> > so you're embedding the exception number into the regs structure?
> > That's weird too... Why not have regs be regs at the time of calling
> > the exception and then pass that little bit of extra information here?
>
> Just to be somewhat consistent with the way x86 does it. See
> lib/x86/desc.h. I imagine there will be developers working on both
> arches, and so I want to try to keep some interfaces consistent.
>
I imagine more that ARM guys will be working on this, and x86 guys will
be working on x86, but I guess the future will tell, but it seems you
are leaning to the conclusion as well below...
> >
> > If not, could we at least call the regs structure something like
> > ex_info and have named fields on there?
>
> I should have used named fields. I'll switch to that, but I guess
> if I want to maintain my consistent naming with x86, then I can't
> rename to ex_info...
>
> > > +#define R_IP 12
> > > +#define R_SP 13
> > > +#define R_LR 14
> > > +#define R_SPSR 16
> > > +#define __ex_reg(n) \
> > > + ((n) == R_SP ? 1 : (n) == R_LR ? 16 : (n) == R_SPSR ? 2 : ((n)+3))
> > > +#define ex_reg(regs, n) ((regs)->words[__ex_reg(n)])
> > > +#define ex_vector(regs) ((regs)->words[0])
> > > +
> > > +struct ex_regs {
> > > + /* see cstart.S for the register push order */
> > > + unsigned long words[18];
> > > +};
> >
> > referring to an assembly file to understand a data structure is not
> > super user-friendly. You could also consider being inspired by ptrace.h
> > from the kernel here to use a well-known format for this data.
>
> OK, I'm inspired. Actually, inspired enough that I'll just include
> ptrace.h I'll use struct pt_regs and forget about consistency with x86.
> That means vector will be passed as a separate argument as well.
>
nice! :)
> >
> > > +
> > > +void handle_exception(u8 v, void (*func)(struct ex_regs *regs));
> > > +
> > > +#endif
> > > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > > index dce9a0f516e7e..6ebd903a27f46 100644
> > > --- a/lib/libcflat.h
> > > +++ b/lib/libcflat.h
> > > @@ -68,6 +68,8 @@ extern long atol(const char *ptr);
> > >
> > > #define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)
> > >
> > > +#define __unused __attribute__((__unused__))
> > > +
> > > #define NULL ((void *)0UL)
> > > #include "errno.h"
> > > #endif
> > > --
> > > 1.8.1.4
> > >
> >
> > Looks roughly ok as a first drop, but I would like to reuse a bit more
> > familiar names from the kernel for mode definitions etc., and possibly
> > copy in the kernel header file for the data structure.
>
> Agreed. ptrace.h will come over.
>
> >
> > I'll review the implementation in more depth for v2 when the
> > indentations are fixed.
> >
> > Thanks a lot for working on this and doing the initial legwork here.
>
> Thanks for reviewing.
>
> drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
