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

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

On Thu, Jan 02, 2014 at 07:36:33PM +0100, Andrew Jones wrote:
> On Sat, Dec 28, 2013 at 10:32:10PM -0800, Christoffer Dall wrote:
> > On Fri, Dec 13, 2013 at 11:58:06AM +0100, Andrew Jones wrote:
> > > Add support for tests to use exception handlers.
> > 
> > You are doing a lot more than that it seems.  In fact, this is a lot of
> > code to review at one time.  I would have liked it to be split up into
> > more than one patch, for example, one introducing exception handlers,
> > and another one testing them in boot.c
> > 
> > That leads me to a general concern in boot.c.  It seems to me that this
> > is a self-test of kvm-unit-test which may be going slightly over the
> > top and it's quite a bit of code to carry around.  On the other hand, it
> > is practical while developing this tool... Hmmm.
> 
> yeah, I'll rename boot.c to selftest.c. I think it's useful for the
> development and continued sanity checking as we go forward.
> 

it actually really is, yes, while I was hacking on the more complicated
tests it was good to know that I didn't break anything fundamental
(which I did, many times).

> > 
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  arm/boot.c            | 128 ++++++++++++++++++++++++++++++++++++--
> > >  arm/cstart.S          | 168 ++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  arm/flat.lds          |   7 ++-
> > >  arm/unittests.cfg     |  19 ++++++
> > >  config/config-arm.mak |   1 +
> > >  lib/arm/processor.c   |  97 +++++++++++++++++++++++++++++
> > >  lib/arm/processor.h   |  29 +++++++++
> > >  lib/arm/sysinfo.h     |   4 ++
> > >  lib/libcflat.h        |   2 +
> > >  9 files changed, 443 insertions(+), 12 deletions(-)
> > >  create mode 100644 lib/arm/processor.c
> > >  create mode 100644 lib/arm/processor.h
> > > 
> > > diff --git a/arm/boot.c b/arm/boot.c
> > > index dc42dfc232366..fd3e3412669fe 100644
> > > --- a/arm/boot.c
> > > +++ b/arm/boot.c
> > > @@ -1,17 +1,133 @@
> > >  #include "libcflat.h"
> > >  #include "test_util.h"
> > >  #include "arm/sysinfo.h"
> > > +#include "arm/ptrace.h"
> > > +#include "arm/processor.h"
> > > +#include "arm/asm-offsets.h"
> > > +
> > > +static struct pt_regs expected_regs;
> > 
> > why are you making this a static thing and not just something allocated
> > on the stack and passed to the macro?  If you start doing this on SMP
> > you need locking and stuff or this will break...
> 
> Agreed, when going to smp we'll need to consider it, and using the
> stack sounds good to me. I just hadn't started using smp yet to
> worry about it.
> 
> > 
> > > +/* NOTE: update clobber list if passed insns needs more than r0,r1 */
> > 
> > This macro really needs a comment explaining what it does.  Also the
> > name is hard to spell out, I would much prefer test_exception.
> > 
> > > +#define test_excptn(pre_insns, excptn_insn, post_insns)		\
> > > +	asm volatile(						\
> > > +		pre_insns "\n"					\
> > > +		"mov	r0, %0\n"				\
> > > +		"stmia	r0, { r0-lr }\n"			\
> > 
> > this is weird, you're storing the pointer to the struct itself onto the
> > pt_regs struct because you want to capture the exception state exactly
> > as it is when the exception happens?  Seems a bit contrived, but ok, if
> > that's the purpose, but please state above the overall agenda.
> 
> That's the purpose. It doesn't matter what's in the registers, only
> that we get them in the pt_regs from the exception handler. I've added
> a descriptive comment above the macro.
> 
> > 
> > > +		"mrs	r1, cpsr\n"				\
> > > +		"str	r1, [r0, #" __stringify(S_PSR) "]\n"	\
> > > +		"mov	r1, #-1\n"				\
> > > +		"str	r1, [r0, #" __stringify(S_OLD_R0) "]\n"	\
> > > +		"add	r1, pc, #8\n"				\
> > 
> > Compiling in Thumb-2 is never going to happen?

but maybe be nice and define a label below and load the addres of that
label into the PC accordingly?  (Note that on Thumb the PC offsets will
be different).

> 
> Dunno. Not on my agenda anyway. Add-on patches welcome.
> 
> > 
> > > +		"str	r1, [r0, #" __stringify(S_R1) "]\n"	\
> > > +		"str	r1, [r0, #" __stringify(S_PC) "]\n"	\
> > > +		excptn_insn "\n"				\
> > > +		post_insns "\n"					\
> > > +	:: "r" (&expected_regs) : "r0", "r1")
> > > +
> > > +#define svc_mode() ((get_cpsr() & MODE_MASK) == SVC_MODE)
> > 
> > I would probably add this as a generic thing in processor.h as
> > current_cpsr() and current_mode() and then you can do:
> > 'if (current_mode() == SVCMODE)' which I think is more clear than just
> > 'svc_mode()' which could mean a bunch of things, e.g. enter SVC mode,
> > return the value for svc_mode, etc.
> 
> OK
> 
> > 
> > > +
> > > +static bool check_regs(struct pt_regs *regs)
> > > +{
> > > +	unsigned i;
> > > +
> > > +	if (!svc_mode())
> > > +		return false;
> > 
> > again, it would be nice to comment what your expectations are and why
> > you are failing in the some cases.  Here I assume you want all your
> > handlers to handle all types of exceptions in SVC mode, so you are
> > verifying this?
> 
> Correct. Added comment.
> 
> > 
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(regs->uregs); ++i)
> > > +		if (regs->uregs[i] != expected_regs.uregs[i])
> > > +			return false;
> > 
> > I'd prefer braces for the for-loop's body.
> 
> added.
> 
> > 
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static bool und_works;
> > > +static void und_handler(struct pt_regs *regs)
> > > +{
> > > +	if (check_regs(regs))
> > > +		und_works = true;
> > > +}
> > > +
> > > +static bool check_und(void)
> > > +{
> > > +	handle_exception(EXCPTN_UND, und_handler);
> > > +
> > > +	/* issue an instruction to a coprocessor we don't have */
> > > +	test_excptn("", "mcr p2, 0, r0, c0, c0", "");
> > > +
> > > +	handle_exception(EXCPTN_UND, NULL);
> > > +
> > > +	return und_works;
> > > +}
> > > +
> > > +static bool svc_works;
> > > +static void svc_handler(struct pt_regs *regs)
> > > +{
> > > +	u32 svc = *(u32 *)(regs->ARM_pc - 4) & 0xffffff;
> > > +
> > > +	if (!user_mode(regs)) {
> > 
> > if (processor_mode(regs) == SVC_MODE)  ?
> 
> yup, changed.
> 
> > 
> > > +		/*
> > > +		 * When issuing an svc from supervisor mode lr_svc will
> > > +		 * get corrupted. So before issuing the svc, callers must
> > > +		 * always push it on the stack. We pushed it to offset 4.
> > > +		 */
> > > +		regs->ARM_lr = *(unsigned long *)(regs->ARM_sp + 4);
> > > +	}
> > > +
> > > +	if (check_regs(regs) && svc == 123)
> > > +		svc_works = true;
> > > +}
> > > +
> > > +static bool check_svc(void)
> > > +{
> > > +	handle_exception(EXCPTN_SVC, svc_handler);
> > > +
> > > +	if (svc_mode()) {
> > > +		/*
> > > +		 * An svc from supervisor mode will corrupt lr_svc and
> > > +		 * spsr_svc. We need to save/restore them separately.
> > > +		 */
> > > +		test_excptn(
> > > +			"mrs	r0, spsr\n"
> > > +			"push	{ r0,lr }\n",
> > > +			"svc	#123\n",
> > > +			"pop	{ r0,lr }\n"
> > > +			"msr	spsr, r0\n"
> > 
> > You need to do "msr	spsr_cxsf, r0" otherwise it will not restore all
> > bits of the spsr.
> 
> fixed
> 
> > 
> > > +		);
> > > +	} else {
> > > +		test_excptn("", "svc #123", "");
> > > +	}
> > > +
> > > +	handle_exception(EXCPTN_SVC, NULL);
> > > +
> > > +	return svc_works;
> > > +}
> > > +
> > > +static void check_vectors(void)
> > > +{
> > > +	int ret = check_und() && check_svc() ? PASS : FAIL;
> > > +	exit(ret);
> > > +}
> > >  
> > >  int main(int argc, char **argv)
> > >  {
> > >  	int ret = FAIL;
> > >  
> > > -	if (argc >= 1) {
> > > -		--argc;
> > > -		if (!strcmp(argv[0], "mem") && enough_args(argc, 1)) {
> > > -			if (check_u32(mem32.size/1024/1024, 10, argv[1]))
> > > -				ret = PASS;
> > > -		}
> > > +	if (!enough_args(argc, 1))
> > > +		return ret;
> > > +
> > > +	if (strcmp(argv[0], "mem") == 0 && enough_args(argc, 2)) {
> > > +
> > > +		if (check_u32(mem32.size/1024/1024, 10, argv[1]))
> > > +			ret = PASS;
> > > +
> > > +	} else if (strcmp(argv[0], "vectors") == 0) {
> > > +
> > > +		check_vectors(); /* doesn't return */
> > > +
> > > +	} else if (strcmp(argv[0], "vectors_usr") == 0) {
> > > +
> > > +		start_usr(check_vectors); /* doesn't return */
> > > +
> > >  	}
> > > +
> > >  	return ret;
> > >  }
> > > diff --git a/arm/cstart.S b/arm/cstart.S
> > > index 05d4bb5becaa0..951c3c2768076 100644
> > > --- a/arm/cstart.S
> > > +++ b/arm/cstart.S
> > > @@ -1,5 +1,7 @@
> > > -
> > > -#define CR_B	(1 << 7)	/* Big endian */
> > > +#define __ASSEMBLY__
> > > +#include "arm/asm-offsets.h"
> > > +#include "arm/ptrace.h"
> > > +#include "arm/cp15.h"
> > >  
> > >  .arm
> > >  
> > > @@ -9,13 +11,23 @@
> > >  start:
> > >  	/* bootloader params are in r0-r2 */
> > >  	ldr	sp, =stacktop
> > > +	mrc	p15, 0, r8, c1, c0, 0	@ r8 := sctrl
> > >  
> > > -	mrc	p15, 0, r8, c1, c0, 0	@r8 = sctrl
> > > -	ands	r3, r8, #CR_B		@set BE, if necessary
> > > +	/* set BE, if necessary */
> > > +	tst	r8, #CR_B
> > 
> > unrelated change?
> 
> yeah
> 
> > 
> > >  	ldrne	r3, =cpu_is_be
> > >  	movne	r4, #1
> > >  	strne	r4, [r3]
> > > -	bl	setup			@complete setup
> > > +
> > > +	/* set up vector table */
> > > +	bl	exceptions_init
> > > +	bic	r8, #CR_V		@ sctrl.V := 0
> > > +	mcr	p15, 0, r8, c1, c0, 0
> > > +	ldr	r3, =vector_table	@ vbar := vector_table
> > > +	mcr	p15, 0, r3, c12, c0, 0
> > > +
> > > +	/* complete setup */
> > > +	bl	setup
> > >  
> > >  	/* start the test */
> > >  	ldr	r0, =__argc
> > > @@ -25,6 +37,25 @@ start:
> > >  	bl	exit
> > >  	b	halt
> > >  
> > > +exceptions_init:
> > 
> > this is actually not exceptions_init, but stack init, or mode_init, or
> > exception_stack_init, but ok...
> > 
> > > +	mrs	r3, cpsr
> > > +	ldr	r4, =exception_stacks
> > > +		/* first frame for svc mode */
> > 
> > First, I didn't get this comment, then I reviewed the code below and
> > understood what you mean.  I think this warrants slightly more
> > explanation, in that you are not actually allocating standard
> > down-growing stacks, but data structures for each exception mode to hold
> > the exception registers, right?
> 
> correct
> 

I reworked this quite a bit in my branch to allow each CPU to have a
separate set of exception handlers (non-Linux'y but useful for testing).
As part of that there's a verbose ASCII diagram that should help make
this more clear.

> > 
> > > +	msr	cpsr_c, #(UND_MODE | PSR_I_BIT | PSR_F_BIT)
> > > +	add	r4, #S_FRAME_SIZE
> > > +	mov	sp, r4
> > > +	msr	cpsr_c, #(ABT_MODE | PSR_I_BIT | PSR_F_BIT)
> > > +	add	r4, #S_FRAME_SIZE
> > > +	mov	sp, r4
> > > +	msr	cpsr_c, #(IRQ_MODE | PSR_I_BIT | PSR_F_BIT)
> > > +	add	r4, #S_FRAME_SIZE
> > > +	mov	sp, r4
> > > +	msr	cpsr_c, #(FIQ_MODE | PSR_I_BIT | PSR_F_BIT)
> > > +	add	r4, #S_FRAME_SIZE
> > > +	mov	sp, r4
> > > +	msr	cpsr_cxsf, r3	@ back to svc mode
> > > +	mov	pc, lr
> > > +
> > 
> > I would have loved to use the 'msr SP_<mode>, rX' and related
> > instructions for this, but QEMU doesn't seem to support this yet, so it
> > makes sense.  It's still a large block of repetitive code, so a macro
> > may be worth it, (I tested this in my tree, have a look if you want).
> > 
> > I would push {r0 - r2} immediately after boot when you have your stack,
> > and then use r0-r3 as your scratch registers in this routine to maintain
> > the standard calling convention.
> 
> OK. Well, I'll push r0-r3 (even though r3 isn't anything important) in
> order to maintain 8-byte stack alignment, because there are calls into C
> functions from 'start:' as well.
> 

Again, before you rework this too much, take a look at my branch where
I've taken a stab at it.  (I missed the AACPS 8-byte stack alignment
though, so need to fix that up).

> > 
> > >  .text
> > >  
> > >  .globl halt
> > > @@ -32,6 +63,133 @@ 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
> > 
> > magic numbers
> 
> I've already got an enum for the vector numbers in
> processor.h, but this assembly would need defines.
> I'm not sure it's worth it. I'm not even sure it's
> worth a comment, since the macro definition above
> documents what they are.
> 
> > 
> > > +
> > > +.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 */
> > > +	mov	r0, #2
> > 
> > magic number
> 
> I'll comment this one
> 
> > 
> > > +	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
> > 
> > what are you checking for here exactly?  What is the case that you are
> > catering to?
> 
> We need 8-byte alignment for stacks to conform to the APCS before
> calling the C handler function.

Ah, didn't realize that, thanks for teaching me about it.  A reference
to the AACPS section would make this code really educational, but I
don't require this from you :)

> 
> > 
> > > +
> > > +	/* 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	r2, sp, #S_FRAME_SIZE
> > > +	addne	r2, #4			@ stack wasn't aligned
> > > +	str	r2, [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
> > > +
> > > +	/* return from exception */
> > > +	msr	spsr_cxsf, r5
> > > +	ldmia	sp, { r0-pc }^
> > 
> > if you're returning to user mode, are you not leaving a portion of the
> > svc stack consumed never to be recovered again?
> 
> Ah yeah, I'll fix that. Thanks!
> 
> > 
> > > +
> > > +.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
> > > +
> > >  .data
> > >  
> > >  .globl cpu_is_be
> > > diff --git a/arm/flat.lds b/arm/flat.lds
> > > index 3e5d72e24989b..ee9fc0ab79abc 100644
> > > --- a/arm/flat.lds
> > > +++ b/arm/flat.lds
> > > @@ -3,7 +3,12 @@ SECTIONS
> > >  {
> > >      .text : { *(.init) *(.text) *(.text.*) }
> > >      . = ALIGN(4K);
> > > -    .data : { *(.data) }
> > > +    .data : {
> > > +        exception_stacks = .;
> > > +        . += 4K;
> > > +        exception_stacks_end = .;
> > > +        *(.data)
> > > +    }
> > >      . = ALIGN(16);
> > >      .rodata : { *(.rodata) }
> > >      . = ALIGN(16);
> > > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > > index c328657b7944a..3a42bbeb3cb38 100644
> > > --- a/arm/unittests.cfg
> > > +++ b/arm/unittests.cfg
> > > @@ -6,6 +6,25 @@
> > >  # arch = arm/arm64 # Only if the test case works only on one of them
> > >  # groups = group1 group2 # Used to identify test cases with run_tests -g ...
> > >  
> > > +#
> > > +# The boot group tests the initial booting of a guest, as well as
> > > +# the test framework itself.
> > > +#
> > > +
> > > +# Test bootinfo reading; configured mem-size should equal expected mem-size
> > >  [boot_info]
> > >  file = boot.flat
> > >  extra_params = -m 256 -append 'mem 256'
> > > +groups = boot
> > > +
> > > +# Test vector setup and exception handling (svc mode).
> > > +[boot_vectors]
> > > +file = boot.flat
> > > +extra_params = -append 'vectors'
> > > +groups = boot
> > > +
> > > +# Test vector setup and exception handling (usr mode).
> > > +[boot_vectors_usr]
> > > +file = boot.flat
> > > +extra_params = -append 'vectors_usr'
> > > +groups = boot
> > > diff --git a/config/config-arm.mak b/config/config-arm.mak
> > > index 173e606fbe64c..4a05519f495c5 100644
> > > --- a/config/config-arm.mak
> > > +++ b/config/config-arm.mak
> > > @@ -20,6 +20,7 @@ cflatobjs += \
> > >  	lib/virtio-testdev.o \
> > >  	lib/test_util.o \
> > >  	lib/arm/io.o \
> > > +	lib/arm/processor.o \
> > >  	lib/arm/setup.o
> > >  
> > >  libeabi := lib/arm/libeabi.a
> > > diff --git a/lib/arm/processor.c b/lib/arm/processor.c
> > > new file mode 100644
> > > index 0000000000000..d37231df19690
> > > --- /dev/null
> > > +++ b/lib/arm/processor.c
> > > @@ -0,0 +1,97 @@
> > > +#include "libcflat.h"
> > > +#include "arm/sysinfo.h"
> > > +#include "arm/ptrace.h"
> > > +#include "heap.h"
> > > +
> > > +static const char *processor_modes[] = {
> > > +	"USER_26", "FIQ_26" , "IRQ_26" , "SVC_26" ,
> > > +	"UK4_26" , "UK5_26" , "UK6_26" , "UK7_26" ,
> > > +	"UK8_26" , "UK9_26" , "UK10_26", "UK11_26",
> > > +	"UK12_26", "UK13_26", "UK14_26", "UK15_26",
> > > +	"USER_32", "FIQ_32" , "IRQ_32" , "SVC_32" ,
> > > +	"UK4_32" , "UK5_32" , "UK6_32" , "ABT_32" ,
> > > +	"UK8_32" , "UK9_32" , "UK10_32", "UND_32" ,
> > > +	"UK12_32", "UK13_32", "UK14_32", "SYS_32"
> > > +};
> > > +
> > > +static char *vector_names[] = {
> > > +	"rst", "und", "svc", "pabt", "dabt", "addrexcptn", "irq", "fiq"
> > 
> > s/rst/reset ?
> 
> I decided to go with the short names for everything, but it might
> make more sense to go with the long names for everything instead.
> 

rst is ok, because I've seen it used in the kernel too, but the excptn
thingy feels rather compressed.  Anyway, it's up to you.

> > 
> > > +};
> > > +
> > > +void show_regs(struct pt_regs *regs)
> > > +{
> > > +	unsigned long flags;
> > > +	char buf[64];
> > > +
> > > +	printf("pc : [<%08lx>]    lr : [<%08lx>]    psr: %08lx\n"
> > > +	       "sp : %08lx  ip : %08lx  fp : %08lx\n",
> > > +		regs->ARM_pc, regs->ARM_lr, regs->ARM_cpsr,
> > > +		regs->ARM_sp, regs->ARM_ip, regs->ARM_fp);
> > > +	printf("r10: %08lx  r9 : %08lx  r8 : %08lx\n",
> > > +		regs->ARM_r10, regs->ARM_r9, regs->ARM_r8);
> > > +	printf("r7 : %08lx  r6 : %08lx  r5 : %08lx  r4 : %08lx\n",
> > > +		regs->ARM_r7, regs->ARM_r6, regs->ARM_r5, regs->ARM_r4);
> > > +	printf("r3 : %08lx  r2 : %08lx  r1 : %08lx  r0 : %08lx\n",
> > > +		regs->ARM_r3, regs->ARM_r2, regs->ARM_r1, regs->ARM_r0);
> > > +
> > > +	flags = regs->ARM_cpsr;
> > > +	buf[0] = flags & PSR_N_BIT ? 'N' : 'n';
> > > +	buf[1] = flags & PSR_Z_BIT ? 'Z' : 'z';
> > > +	buf[2] = flags & PSR_C_BIT ? 'C' : 'c';
> > > +	buf[3] = flags & PSR_V_BIT ? 'V' : 'v';
> > > +	buf[4] = '\0';
> > > +
> > > +	printf("Flags: %s  IRQs o%s  FIQs o%s  Mode %s\n",
> > > +		buf, interrupts_enabled(regs) ? "n" : "ff",
> > > +		fast_interrupts_enabled(regs) ? "n" : "ff",
> > > +		processor_modes[processor_mode(regs)]);
> > > +
> > > +	if (!user_mode(regs)) {
> > > +		unsigned int ctrl, transbase, dac;
> > > +		asm volatile(
> > > +			"mrc p15, 0, %0, c1, c0\n"
> > > +			"mrc p15, 0, %1, c2, c0\n"
> > > +			"mrc p15, 0, %2, c3, c0\n"
> > 
> > aren't you omitting opc2 (it may default to 0, but it's nicer to specify
> > it).
> > 
> > > +		: "=r" (ctrl), "=r" (transbase), "=r" (dac));
> > > +		printf("Control: %08x  Table: %08x  DAC: %08x\n",
> > > +			ctrl, transbase, dac);
> > 
> > I know the kernel does it this way, but I assume that's legacy stuff
> > that we just didn't change, and I would consider:
> > 
> > s/transbase/ttbr/
> > s/Table/TTBR0/
> > s/DAC/dacr/
> > s/Control/SCTLR/
> > s/ctrl/sctlr/
> 
> yeah, so far the above is just a blind rip-off of the kernel code. I
> can clean up the naming and add whatever output is useful here.
> 

Yeah, I was back-and-forward here.  Dunno, it's up to you.

> > 
> > > +	}
> > > +}
> > > +
> > > +static void (*exception_handlers[8])(struct pt_regs *regs);
> > > +
> > > +void handle_exception(u8 v, void (*func)(struct pt_regs *regs))
> > > +{
> > > +	if (v < 8)
> > > +		exception_handlers[v] = func;
> > > +}
> > 
> > How about naming your execption enum in processor.h and let this
> > function take that as the parameter instead of v?
> 
> OK
> 
> > 
> > I would also rename the function to install_handler or
> > install_exception_handler
> 
> OK
> 
> > 
> > > +
> > > +void do_handle_exception(u8 v, struct pt_regs *regs)
> > > +{
> > > +	if (v < 8 && exception_handlers[v]) {
> > > +		exception_handlers[v](regs);
> > > +		return;
> > > +	}
> > > +
> > > +	if (v < 8)
> > > +		printf("Unhandled exception %d (%s)\n", v, vector_names[v]);
> > > +	else
> > > +		printf("%s called with vector=%d\n", __func__, v);
> > > +
> > > +	printf("Exception frame registers:\n");
> > > +	show_regs(regs);
> > > +	exit(EINTR);
> > > +}
> > > +
> > > +void start_usr(void (*func)(void))
> > > +{
> > > +	void *sp_usr = alloc_page() + PAGE_SIZE;
> > 
> > I believe you generally allocated two pages for svc mode in the linker
> > scripts, but now you're only giving user mode one page, that's sort of
> > confusing and could be hard to track down.  I would reserve 8K in the
> > linker script for two contiguous pages to use for the user stack.
> 
> hmm, I think how we prep for usr space is going to evolve quite a bit
> as we get the mmu engaged. I'd rather change the svc mode stack to
> one page, for now - if they need to be equal, than to carve out another
> section specifically for usr mode running without an mmu.
> 

I'm only thinking in some debugging scenario where you take a look and
say, oh, you have two pages, so it's not because I'm spilling over the
stack, but then you actually are, because you only had one page.

> > 
> > > +	asm volatile(
> > > +		"mrs	r0, cpsr\n"
> > > +		"bic	r0, #" __stringify(MODE_MASK) "\n"
> > > +		"orr	r0, #" __stringify(USR_MODE) "\n"
> > > +		"msr	cpsr_c, r0\n"
> > > +		"mov	sp, %0\n"
> > > +		"mov	pc, %1\n"
> > 
> > I have some vague recollection that writing the cpsr mode bits directly
> > is deprecated, but a quick scan of the ARM ARM didn't enlighten me.
> > That being said, it feels like maybe you need some isb's here, and this
> > could turn out very interesting when we start enabling the MMU.
> > 
> > I would recommend writing to the spsr and do a movs or storing the cpsr
> > and pc to memory and performing an rfe.
> 
> I think we should just start working on enabling the mmu and let this
> function get rewritten :-)
> 

I already have MMU enablement code, and it doesn't mess much with this
function actually, we can discuss more when you've seen my work.

> > 
> > > +	:: "r" (sp_usr), "r" (func) : "r0");
> > > +}
> > > diff --git a/lib/arm/processor.h b/lib/arm/processor.h
> > > new file mode 100644
> > > index 0000000000000..66cc7363a2f10
> > > --- /dev/null
> > > +++ b/lib/arm/processor.h
> > > @@ -0,0 +1,29 @@
> > > +#ifndef _ARM_PROCESSOR_H_
> > > +#define _ARM_PROCESSOR_H_
> > > +#include "libcflat.h"
> > > +#include "ptrace.h"
> > > +
> > > +enum {
> > > +	EXCPTN_RST,
> > > +	EXCPTN_UND,
> > > +	EXCPTN_SVC,
> > > +	EXCPTN_PABT,
> > > +	EXCPTN_DABT,
> > > +	EXCPTN_ADDREXCPTN,
> > 
> > ADDREXCPTRN?
> 
> the name comes for the kernel... we should never need it anyway
> 

fair enough.

> > 
> > > +	EXCPTN_IRQ,
> > > +	EXCPTN_FIQ,
> > > +};
> > > +
> > > +extern void handle_exception(u8 v, void (*func)(struct pt_regs *regs));
> > > +extern void show_regs(struct pt_regs *regs);
> > > +
> > > +extern void start_usr(void (*func)(void));
> > > +
> > > +static inline unsigned long get_cpsr(void)
> > > +{
> > > +	unsigned long cpsr;
> > > +	asm volatile("mrs %0, cpsr" : "=r" (cpsr));
> > > +	return cpsr;
> > > +}
> > > +
> > > +#endif
> > > diff --git a/lib/arm/sysinfo.h b/lib/arm/sysinfo.h
> > > index f3b076e1a34c4..91bb17dca0c86 100644
> > > --- a/lib/arm/sysinfo.h
> > > +++ b/lib/arm/sysinfo.h
> > > @@ -16,4 +16,8 @@ struct tag_mem32 {
> > >  extern u32 mach_type_id;
> > >  extern struct tag_core core;
> > >  extern struct tag_mem32 mem32;
> > > +
> > > +#ifndef PAGE_SIZE
> > > +#define PAGE_SIZE core.pagesize
> > > +#endif
> > >  #endif
> > > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > > index 8c6cf1f0735ba..951559b6d69e6 100644
> > > --- a/lib/libcflat.h
> > > +++ b/lib/libcflat.h
> > > @@ -62,6 +62,8 @@ extern long atol(const char *ptr);
> > >  		(type *)( (char *)__mptr - offsetof(type,member) );})
> > >  
> > >  #define __unused __attribute__((__unused__))
> > > +#define __stringify_1(x...)	#x
> > > +#define __stringify(x...)	__stringify_1(x)
> > >  #define NULL ((void *)0UL)
> > >  #include "errno.h"
> > >  #endif
> > > -- 
> > > 1.8.1.4
> > > 
> > 
> > Thanks,
> > -- 
> > Christoffer
> 
> Thanks!
> drew