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.
>
> > + 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.
>
> 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.
>
> > +
> > +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
