Re: [PATCH 9/9] arm: add vectors support

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

On Mon, Oct 14, 2013 at 06:23:35PM +0200, Andrew Jones wrote:
> Add support for tests to manage exception handlers. Now we need kvm.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/boot.c            | 29 +++++++++++++++++++----
>  arm/cstart.S          | 46 ++++++++++++++++++++++++++++++++++++
>  arm/run               |  2 +-
>  arm/unittests.cfg     |  6 +++++
>  config/config-arm.mak |  1 +
>  lib/arm/bootinfo.c    |  2 --
>  lib/arm/processor.h   | 45 +++++++++++++++++++++++++++++++++++
>  lib/arm/vectors.c     | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/arm/vectors.h     | 37 +++++++++++++++++++++++++++++
>  lib/libcflat.h        |  2 ++
>  10 files changed, 227 insertions(+), 8 deletions(-)
>  create mode 100644 lib/arm/processor.h
>  create mode 100644 lib/arm/vectors.c
>  create mode 100644 lib/arm/vectors.h
> 
> diff --git a/arm/boot.c b/arm/boot.c
> index 375e8708a7c54..5103499b83155 100644
> --- a/arm/boot.c
> +++ b/arm/boot.c
> @@ -1,5 +1,10 @@
>  #include "libcflat.h"
>  #include "arm/bootinfo.h"
> +#include "arm/vectors.h"
> +
> +#define die(msg...) printf(msg), exit(1)
> +
> +static bool svc_works;
>  
>  static bool info_check(u32 var, char *expected)
>  {
> @@ -10,18 +15,32 @@ static bool info_check(u32 var, char *expected)
>      return !strcmp(var_str, expected);
>  }
>  
> +static void svc_check(struct ex_regs *regs __unused)
> +{
> +    svc_works = true;
> +}
> +
> +static bool vectors_check(void)
> +{
> +    handle_exception(V_SVC, svc_check);
> +    asm volatile("svc #123");
> +    return svc_works;
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int ret = 0;
>  
> -    if (argc < 3) {
> -	printf("Not enough arguments. Can't test\n");
> -	return 1;
> -    }
> +    if (argc < 1)
> +	die("boot: No test name appended to qemu command line\n");
>  
> -    if (!strcmp(argv[0], "info"))
> +    if (!strcmp(argv[0], "info")) {
> +	if (argc < 3)
> +	    die("boot info: Not enough arguments for test\n");
>  	ret = !info_check(mem32.size, argv[1])
>  		|| !info_check(core.pagesize, argv[2]);
> +    } else if (!strcmp(argv[0], "vectors"))
> +	ret = !vectors_check();
>  
>      return ret;
>  }
> diff --git a/arm/cstart.S b/arm/cstart.S
> index a65809824d4f1..0907c620bd83f 100644
> --- a/arm/cstart.S
> +++ b/arm/cstart.S
> @@ -1,5 +1,6 @@
>  
>  #define CR_B	(1 << 7)
> +#define CR_V	(1 << 13)
>  
>  .arm
>  
> @@ -12,6 +13,13 @@ start:
>  	push	{ r0-r3 }		@push r3 too for 8-byte alignment
>  
>  	mrc	p15, 0, r8, c1, c0, 0	@r8 = sctrl
> +
> +	/* set up vector table */
> +	bic	r8, #CR_V		@sctrl.V = 0
> +	mcr	p15, 0, r8, c1, c0, 0
> +	ldr	r0, =vector_table	@vbar = vector_table
> +	mcr     p15, 0, r0, c12, c0, 0
> +
>  	bl	get_endianness
>  	bl	io_init
>  
> @@ -41,6 +49,44 @@ halt:
>  1:	wfi
>  	b	1b
>  
> +vector_common:
> +	add	r2, sp, #(14 * 4)

this looks weird, what are you pointing to here?

> +	mrs	r3, spsr
> +	push	{ r0-r3 }	@push r0 too for padding
> +	add	r0, sp, #4	@skip pad

so you're embedding the exception number into the regs structure?
That's weird too...  Why not have regs be regs at the time of calling
the exception and then pass that little bit of extra information here?

If not, could we at least call the regs structure something like
ex_info and have named fields on there?

> +	bl	do_handle_exception
> +	pop	{ r0-r3 }
> +	msr	spsr_fsxc, r3
> +	ldm	sp!, { r0-r12,pc }^
> +
> +.macro m_vector, v
> +	push	{ r0-r12,lr }
> +	mov	r1, \v
> +	b	vector_common
> +.endm
> +
> +reset:		m_vector #0
> +undef:		m_vector #1
> +svc:		m_vector #2
> +prefetch:	m_vector #3
> +data:		m_vector #4
> +impossible:	m_vector #5
> +irq:		m_vector #6
> +fiq:		m_vector #7
> +
> +.section .text.ex
> +.align 5
> +
> +vector_table:
> +	b	reset
> +	b	undef
> +	b	svc
> +	b	prefetch
> +	b	data
> +	b	impossible
> +	b	irq
> +	b	fiq
> +
>  .data
>  
>  .globl cpu_is_be
> diff --git a/arm/run b/arm/run
> index 64446e8907564..b512880e28cb7 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -10,7 +10,7 @@ fi
>  
>  command="$qemu -device $testdev -display none -serial stdio "
>  command+="-M virt -cpu cortex-a15 "
> -#command+="-enable-kvm "
> +command+="-enable-kvm "
>  command+="-kernel"
>  echo $command "$@"
>  $command "$@"
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index fb78cd906839a..9c4faa600e9c5 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -9,3 +9,9 @@
>  [boot_info]
>  file = boot.flat
>  extra_params = -m 256 -append 'info 0x10000000 0x1000'
> +groups = boot
> +
> +[boot_vectors]
> +file = boot.flat
> +extra_params = -append 'vectors'
> +groups = boot
> diff --git a/config/config-arm.mak b/config/config-arm.mak
> index 066b1f725c5b3..87d61872ab16a 100644
> --- a/config/config-arm.mak
> +++ b/config/config-arm.mak
> @@ -15,6 +15,7 @@ cflatobjs += \
>  	lib/iomaps.o \
>  	lib/virtio-testdev.o \
>  	lib/arm/io.o \
> +	lib/arm/vectors.o \
>  	lib/arm/bootinfo.o
>  
>  $(libcflat): LDFLAGS += -nostdlib
> diff --git a/lib/arm/bootinfo.c b/lib/arm/bootinfo.c
> index c362064f661d9..fd74fac2c5a2f 100644
> --- a/lib/arm/bootinfo.c
> +++ b/lib/arm/bootinfo.c
> @@ -49,8 +49,6 @@ static void read_atags(u32 id, u32 *info)
>  	}
>  }
>  
> -#define __unused __attribute__((__unused__))
> -
>  void read_bootinfo(u32 arg __unused, u32 id, u32 *info)
>  {
>      u32 *atags = NULL;
> diff --git a/lib/arm/processor.h b/lib/arm/processor.h
> new file mode 100644
> index 0000000000000..32e1778477be2
> --- /dev/null
> +++ b/lib/arm/processor.h
> @@ -0,0 +1,45 @@
> +#ifndef _PROCESSOR_H_
> +#define _PROCESSOR_H_
> +#include "libcflat.h"
> +
> +#define __stringify_1(x...)	#x
> +#define __stringify(x...)	__stringify_1(x)
> +
> +#undef __regfn
> +#define __regfn(reg)						\
> +static inline u32 read_##reg(void)				\
> +{								\
> +    u32 val;							\
> +    asm volatile("mov %0, " __stringify(reg) : "=r" (val));	\
> +    return val;							\
> +}
> +__regfn(lr)
> +
> +#undef __regfn
> +#define __regfn(reg)						\
> +static inline void write_##reg(u32 val)				\
> +{								\
> +    asm volatile("mov " __stringify(reg) ", %0" :: "r" (val));	\
> +}
> +__regfn(lr)
> +
> +#undef __regfn
> +#define __regfn(reg)						\
> +static inline u32 read_##reg(void)				\
> +{								\
> +    u32 val;							\
> +    asm volatile("mrs %0, " __stringify(reg) : "=r" (val));	\
> +    return val;							\
> +}
> +__regfn(cpsr)
> +
> +#undef __regfn
> +#define __regfn(reg)						\
> +static inline void write_##reg(u32 val)				\
> +{								\
> +    asm volatile("msr " __stringify(reg) ", %0" :: "r" (val));	\
> +}
> +__regfn(cpsr)
> +
> +#undef __regfn
> +#endif
> diff --git a/lib/arm/vectors.c b/lib/arm/vectors.c
> new file mode 100644
> index 0000000000000..c1e2dea519e56
> --- /dev/null
> +++ b/lib/arm/vectors.c
> @@ -0,0 +1,65 @@
> +#include "libcflat.h"
> +#include "arm/processor.h"
> +#include "arm/vectors.h"
> +
> +#define abort(msg...) \
> +	printf(msg), dump_ex_regs(regs), exit(EINTR)
> +
> +const char *mode_names[] = {
> +    "usr", "fiq", "irq", "svc", NULL, NULL, NULL,
> +    "abt", NULL, NULL, NULL, "und",
> +};
> +
> +const char *vector_names[] = {
> +    "Reset", "Undefined", "SVC", "Prefetch abort", "Data abort",
> +    "Impossible vector 0x14", "IRQ", "FIQ",
> +};
> +
> +void dump_ex_regs(struct ex_regs *regs)
> +{
> +    u32 cpsr = read_cpsr();
> +    u8 mode, smode;
> +
> +    mode = cpsr & MODE_MASK;
> +    smode = ex_reg(regs, R_SPSR) & MODE_MASK;
> +
> +    printf("vector = %s\n", vector_name(ex_vector(regs)));
> +    printf("mode %s to %s\n", mode_name(smode), mode_name(mode));
> +    printf("cpsr=%x\n", cpsr);
> +    printf(
> +	"r0=%x\n" "r1=%x\n" "r2=%x\n" "r3=%x\n"
> +	"r4=%x\n" "r5=%x\n" "r6=%x\n" "r7=%x\n"
> +	"r8=%x\n" "r9=%x\n" "r10=%x\n" "r11=%x\n"
> +	"ip=%x\n" "sp_%s=%x\n" "lr_%s=%x\n" "cpsr_%s=%x\n",
> +	ex_reg(regs,0), ex_reg(regs,1), ex_reg(regs,2), ex_reg(regs,3),
> +	ex_reg(regs,4), ex_reg(regs,5), ex_reg(regs,6), ex_reg(regs,7),
> +	ex_reg(regs,8), ex_reg(regs,9), ex_reg(regs,10), ex_reg(regs,11),
> +	ex_reg(regs, R_IP),
> +	mode_name(mode), ex_reg(regs, R_SP),
> +	mode_name(mode), ex_reg(regs, R_LR),
> +	mode_name(mode), ex_reg(regs, R_SPSR)
> +    );
> +}
> +
> +static void (*exception_handlers[8])(struct ex_regs *regs);
> +
> +void handle_exception(u8 v, void (*func)(struct ex_regs *regs))
> +{
> +    if (v < 8)
> +	exception_handlers[v] = func;
> +}
> +
> +void do_handle_exception(struct ex_regs *regs)
> +{
> +    u8 v;
> +
> +    if ((v = ex_vector(regs)) > 7)
> +	abort("%s called with vector=%d\n", __func__, v);
> +
> +    if (exception_handlers[v]) {
> +	exception_handlers[v](regs);
> +	return;
> +    }
> +
> +    abort("Exception %s\n", vector_name(v));
> +}
> diff --git a/lib/arm/vectors.h b/lib/arm/vectors.h
> new file mode 100644
> index 0000000000000..ca074aab674a3
> --- /dev/null
> +++ b/lib/arm/vectors.h
> @@ -0,0 +1,37 @@
> +#ifndef _VECTORS_H_
> +#define _VECTORS_H_
> +
> +/* modes */
> +enum {
> +    M_USR = 0x10, M_FIQ = 0x11, M_IRQ = 0x12, M_SVC = 0x13,
> +    M_ABT = 0x17, M_UND = 0x1b,
> +};
> +#define MODE_MASK 0x1f
> +extern const char *mode_names[];
> +#define mode_name(m) mode_names[(m) - 0x10]
> +
> +/* vectors */
> +enum {
> +    V_RESET, V_UNDEF, V_SVC, V_PREFETCH_ABT, V_DATA_ABT,
> +    V_IMPOSSIBLE, V_IRQ, V_FIQ,
> +};
> +extern const char *vector_names[];
> +#define vector_name(v) vector_names[v]
> +
> +#define R_IP	12
> +#define R_SP	13
> +#define R_LR	14
> +#define R_SPSR	16
> +#define __ex_reg(n) \
> +    ((n) == R_SP ? 1 : (n) == R_LR ? 16 : (n) == R_SPSR ? 2 : ((n)+3))
> +#define ex_reg(regs, n) ((regs)->words[__ex_reg(n)])
> +#define ex_vector(regs) ((regs)->words[0])
> +
> +struct ex_regs {
> +    /* see cstart.S for the register push order */
> +    unsigned long words[18];
> +};

referring to an assembly file to understand a data structure is not
super user-friendly.  You could also consider being inspired by ptrace.h
from the kernel here to use a well-known format for this data.

> +
> +void handle_exception(u8 v, void (*func)(struct ex_regs *regs));
> +
> +#endif
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index dce9a0f516e7e..6ebd903a27f46 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -68,6 +68,8 @@ extern long atol(const char *ptr);
>  
>  #define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)
>  
> +#define __unused __attribute__((__unused__))
> +
>  #define NULL ((void *)0UL)
>  #include "errno.h"
>  #endif
> -- 
> 1.8.1.4
> 

Looks roughly ok as a first drop, but I would like to reuse a bit more
familiar names from the kernel for mode definitions etc., and possibly
copy in the kernel header file for the data structure.

I'll review the implementation in more depth for v2 when the
indentations are fixed.

Thanks a lot for working on this and doing the initial legwork here.

-Christoffer