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@xxxxxxxxxx>
> > > ---
> > > 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
