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. > > 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... > +/* 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. > + "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? > + "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. > + > +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? > + > + 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. > + > + 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) ? > + /* > + * 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. > + ); > + } 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? > 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? > + 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. > .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 > + > +.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 > + 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? > + > + /* 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? > + > +.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 ? > +}; > + > +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/ > + } > +} > + > +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? I would also rename the function to install_handler or install_exception_handler > + > +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. > + 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. > + :: "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? > + 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 -- 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