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.
>
> >
> > 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?
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
>
> > + 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.
>
> > .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.
>
> > +
> > + /* 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.
>
> > +};
> > +
> > +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.
>
> > + }
> > +}
> > +
> > +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.
>
> > + 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 :-)
>
> > + :: "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
>
> > + 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
