[PATCH v2 0/3] kvm-unit-tests/arm: add vectors support

[PATCH v2 0/3] kvm-unit-tests/arm: add vectors support

[Andrew Jones]

This series adds exception handling support to kvm-unit-tests/arm, and
is based on v2 of the arm initial drop series. A vector support patch
was also posted with the v1 of the arm initial drop series, but not
with the v2. This series is its v2.

v2:
 - use pt_regs as the kernel does [Christoffer Dall]

Andrew Jones (3):
  printf: support field padding
  arm: add useful headers from the linux kernel
  arm: vectors support

 arm/boot.c                | 128 +++++++++++++++++++++++++++++++++--
 arm/cstart.S              | 168 ++++++++++++++++++++++++++++++++++++++++++++--
 arm/flat.lds              |   7 +-
 arm/unittests.cfg         |  19 ++++++
 config/config-arm.mak     |  18 +++--
 lib/arm/asm-offsets.h     |  27 ++++++++
 lib/arm/cp15.h            |  36 ++++++++++
 lib/arm/processor.c       |  97 ++++++++++++++++++++++++++
 lib/arm/processor.h       |  29 ++++++++
 lib/arm/ptrace.h          | 100 +++++++++++++++++++++++++++
 lib/arm/sysinfo.h         |   4 ++
 lib/libcflat.h            |   2 +
 lib/printf.c              |  84 +++++++++++++++++++----
 scripts/arm/asm-offsets.c |  40 +++++++++++
 14 files changed, 729 insertions(+), 30 deletions(-)
 create mode 100644 lib/arm/asm-offsets.h
 create mode 100644 lib/arm/cp15.h
 create mode 100644 lib/arm/processor.c
 create mode 100644 lib/arm/processor.h
 create mode 100644 lib/arm/ptrace.h
 create mode 100644 scripts/arm/asm-offsets.c

-- 
1.8.1.4

[PATCH 1/3] printf: support field padding

[Andrew Jones]

Support format flags for field padding, such as "%08x", allowing
register dumps to be easier on the eyes.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/printf.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 14 deletions(-)

diff --git a/lib/printf.c b/lib/printf.c
index 867eb1926f742..dd20f755c01a0 100644
--- a/lib/printf.c
+++ b/lib/printf.c
@@ -6,6 +6,11 @@ typedef struct pstream {
     int added;
 } pstream_t;
 
+typedef struct strprops {
+    char pad;
+    int npad;
+} strprops_t;
+
 static void addchar(pstream_t *p, char c)
 {
     if (p->remain) {
@@ -15,15 +20,37 @@ static void addchar(pstream_t *p, char c)
     ++p->added;
 }
 
-void print_str(pstream_t *p, const char *s)
+void print_str(pstream_t *p, const char *s, strprops_t props)
 {
+    const char *s_orig = s;
+    int npad = props.npad;
+
+    if (npad > 0) {
+	npad -= strlen(s_orig);
+	while (npad > 0) {
+	    addchar(p, props.pad);
+	    --npad;
+	}
+    }
+
     while (*s)
 	addchar(p, *s++);
+
+    if (npad < 0) {
+	char pad = props.pad;
+	if (pad == '0') /* ignore '0' flag with '-' flag */
+	    pad = ' ';
+	npad += strlen(s_orig);
+	while (npad < 0) {
+	    addchar(p, pad);
+	    ++npad;
+	}
+    }
 }
 
 static char digits[16] = "0123456789abcdef";
 
-void print_int(pstream_t *ps, long long n, int base)
+void print_int(pstream_t *ps, long long n, int base, strprops_t props)
 {
     char buf[sizeof(long) * 3 + 2], *p = buf;
     int s = 0, i;
@@ -54,10 +81,11 @@ void print_int(pstream_t *ps, long long n, int base)
 
     *p = 0;
 
-    print_str(ps, buf);
+    print_str(ps, buf, props);
 }
 
-void print_unsigned(pstream_t *ps, unsigned long long n, int base)
+void print_unsigned(pstream_t *ps, unsigned long long n, int base,
+		    strprops_t props)
 {
     char buf[sizeof(long) * 3 + 1], *p = buf;
     int i;
@@ -80,7 +108,23 @@ void print_unsigned(pstream_t *ps, unsigned long long n, int base)
 
     *p = 0;
 
-    print_str(ps, buf);
+    print_str(ps, buf, props);
+}
+
+static int fmtnum(const char **fmt)
+{
+    const char *f = *fmt;
+    int len = 0, num;
+
+    if (*f == '-')
+	++f, ++len;
+
+    while (*f >= '0' && *f <= '9')
+	++f, ++len;
+
+    num = atol(*fmt);
+    *fmt += len;
+    return num;
 }
 
 int vsnprintf(char *buf, int size, const char *fmt, va_list va)
@@ -93,6 +137,9 @@ int vsnprintf(char *buf, int size, const char *fmt, va_list va)
     while (*fmt) {
 	char f = *fmt++;
 	int nlong = 0;
+	strprops_t props;
+	memset(&props, 0, sizeof(props));
+	props.pad = ' ';
 
 	if (f != '%') {
 	    addchar(&s, f);
@@ -110,41 +157,50 @@ int vsnprintf(char *buf, int size, const char *fmt, va_list va)
 	case '\0':
 	    --fmt;
 	    break;
+	case '0':
+	    props.pad = '0';
+	    ++fmt;
+	    /* fall through */
+	case '1'...'9':
+	case '-':
+	    --fmt;
+	    props.npad = fmtnum(&fmt);
+	    goto morefmt;
 	case 'l':
 	    ++nlong;
 	    goto morefmt;
 	case 'd':
 	    switch (nlong) {
 	    case 0:
-		print_int(&s, va_arg(va, int), 10);
+		print_int(&s, va_arg(va, int), 10, props);
 		break;
 	    case 1:
-		print_int(&s, va_arg(va, long), 10);
+		print_int(&s, va_arg(va, long), 10, props);
 		break;
 	    default:
-		print_int(&s, va_arg(va, long long), 10);
+		print_int(&s, va_arg(va, long long), 10, props);
 		break;
 	    }
 	    break;
 	case 'x':
 	    switch (nlong) {
 	    case 0:
-		print_unsigned(&s, va_arg(va, unsigned), 16);
+		print_unsigned(&s, va_arg(va, unsigned), 16, props);
 		break;
 	    case 1:
-		print_unsigned(&s, va_arg(va, unsigned long), 16);
+		print_unsigned(&s, va_arg(va, unsigned long), 16, props);
 		break;
 	    default:
-		print_unsigned(&s, va_arg(va, unsigned long long), 16);
+		print_unsigned(&s, va_arg(va, unsigned long long), 16, props);
 		break;
 	    }
 	    break;
 	case 'p':
-	    print_str(&s, "0x");
-	    print_unsigned(&s, (unsigned long)va_arg(va, void *), 16);
+	    print_str(&s, "0x", props);
+	    print_unsigned(&s, (unsigned long)va_arg(va, void *), 16, props);
 	    break;
 	case 's':
-	    print_str(&s, va_arg(va, const char *));
+	    print_str(&s, va_arg(va, const char *), props);
 	    break;
 	default:
 	    addchar(&s, f);
-- 
1.8.1.4

[PATCH 2/3] arm: add useful headers from the linux kernel

[Andrew Jones]

We're going to need PSR bit defines and pt_regs. We'll also need
pt_regs offsets in assembly code. This patch adapts the linux
kernel's ptrace.h and asm-offsets to this framework. Even though
lib/arm/asm-offsets.h is a generated file, we still commit it,
as it's unlikely to change. Also adapt cp15.h from the kernel,
since we'll need bit defines from there too.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 config/config-arm.mak     |  17 ++++++--
 lib/arm/asm-offsets.h     |  27 +++++++++++++
 lib/arm/cp15.h            |  36 +++++++++++++++++
 lib/arm/ptrace.h          | 100 ++++++++++++++++++++++++++++++++++++++++++++++
 scripts/arm/asm-offsets.c |  40 +++++++++++++++++++
 5 files changed, 216 insertions(+), 4 deletions(-)
 create mode 100644 lib/arm/asm-offsets.h
 create mode 100644 lib/arm/cp15.h
 create mode 100644 lib/arm/ptrace.h
 create mode 100644 scripts/arm/asm-offsets.c

diff --git a/config/config-arm.mak b/config/config-arm.mak
index d0814186b279c..173e606fbe64c 100644
--- a/config/config-arm.mak
+++ b/config/config-arm.mak
@@ -1,3 +1,4 @@
+PROCESSOR = cortex-a15
 mach = mach-virt
 iodevs = pl011 virtio_mmio
 phys_base = 0x40000000
@@ -30,8 +31,7 @@ $(libcflat) $(libeabi): CFLAGS += -ffreestanding -I lib
 
 CFLAGS += -Wextra
 CFLAGS += -marm
-#CFLAGS += -mcpu=$(PROCESSOR)
-CFLAGS += -mcpu=cortex-a15
+CFLAGS += -mcpu=$(PROCESSOR)
 CFLAGS += -O2
 
 libgcc := $(shell $(CC) -m$(ARCH) --print-libgcc-file-name)
@@ -55,7 +55,7 @@ tests_and_config = $(TEST_DIR)/*.flat $(TEST_DIR)/unittests.cfg
 
 test_cases: $(tests-common) $(tests)
 
-$(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I lib
+$(TEST_DIR)/%.o scripts/arm/%.o: CFLAGS += -std=gnu99 -ffreestanding -I lib
 
 $(TEST_DIR)/boot.elf: $(cstart.o) $(TEST_DIR)/boot.o
 
@@ -67,7 +67,16 @@ lib/$(TEST_DIR)/mach-virt.dts:
 	$(QEMU_BIN) -kernel /dev/null -M virt -machine dumpdtb=$(dtb)
 	fdtdump $(dtb) > $@
 
+.PHONY: asm-offsets
+
+asm-offsets: scripts/arm/asm-offsets.flat
+	$(QEMU_BIN) -device virtio-testdev -display none -serial stdio \
+		-M virt -cpu $(PROCESSOR) \
+		-kernel $^ > lib/arm/asm-offsets.h || true
+scripts/arm/asm-offsets.elf: $(cstart.o) scripts/arm/asm-offsets.o
+
 arch_clean:
 	$(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
 	$(libeabi) $(eabiobjs) $(TEST_DIR)/.*.d lib/arm/.*.d \
-	lib/$(TEST_DIR)/iomaps.gen.c lib/$(TEST_DIR)/mach-virt.*
+	lib/$(TEST_DIR)/iomaps.gen.c lib/$(TEST_DIR)/mach-virt.* \
+	scripts/arm/.*.d scripts/arm/*.o scripts/arm/*.flat scripts/arm/*.elf
diff --git a/lib/arm/asm-offsets.h b/lib/arm/asm-offsets.h
new file mode 100644
index 0000000000000..b43be20ef8377
--- /dev/null
+++ b/lib/arm/asm-offsets.h
@@ -0,0 +1,27 @@
+#ifndef _ARM_ASM_OFFSETS_H_
+#define _ARM_ASM_OFFSETS_H_
+/*
+ * Generated file. Regenerate with 'make asm-offsets'
+ */
+
+#define S_R0	0x0
+#define S_R1	0x4
+#define S_R2	0x8
+#define S_R3	0xc
+#define S_R4	0x10
+#define S_R5	0x14
+#define S_R6	0x18
+#define S_R7	0x1c
+#define S_R8	0x20
+#define S_R9	0x24
+#define S_R10	0x28
+#define S_FP	0x2c
+#define S_IP	0x30
+#define S_SP	0x34
+#define S_LR	0x38
+#define S_PC	0x3c
+#define S_PSR	0x40
+#define S_OLD_R0	0x44
+#define S_FRAME_SIZE	0x48
+
+#endif /* _ARM_ASM_OFFSETS_H_ */
diff --git a/lib/arm/cp15.h b/lib/arm/cp15.h
new file mode 100644
index 0000000000000..6a8a29aadb008
--- /dev/null
+++ b/lib/arm/cp15.h
@@ -0,0 +1,36 @@
+#ifndef _ARM_CP15_H_
+#define _ARM_CP15_H_
+/*
+ * From Linux kernel arch/arm/include/asm/cp15.h
+ *
+ * CR1 bits (CP#15 CR1)
+ */
+#define CR_M	(1 << 0)	/* MMU enable				*/
+#define CR_A	(1 << 1)	/* Alignment abort enable		*/
+#define CR_C	(1 << 2)	/* Dcache enable			*/
+#define CR_W	(1 << 3)	/* Write buffer enable			*/
+#define CR_P	(1 << 4)	/* 32-bit exception handler		*/
+#define CR_D	(1 << 5)	/* 32-bit data address range		*/
+#define CR_L	(1 << 6)	/* Implementation defined		*/
+#define CR_B	(1 << 7)	/* Big endian				*/
+#define CR_S	(1 << 8)	/* System MMU protection		*/
+#define CR_R	(1 << 9)	/* ROM MMU protection			*/
+#define CR_F	(1 << 10)	/* Implementation defined		*/
+#define CR_Z	(1 << 11)	/* Implementation defined		*/
+#define CR_I	(1 << 12)	/* Icache enable			*/
+#define CR_V	(1 << 13)	/* Vectors relocated to 0xffff0000	*/
+#define CR_RR	(1 << 14)	/* Round Robin cache replacement	*/
+#define CR_L4	(1 << 15)	/* LDR pc can set T bit			*/
+#define CR_DT	(1 << 16)
+#define CR_HA	(1 << 17)	/* Hardware management of Access Flag	*/
+#define CR_IT	(1 << 18)
+#define CR_ST	(1 << 19)
+#define CR_FI	(1 << 21)	/* Fast interrupt (lower latency mode)	*/
+#define CR_U	(1 << 22)	/* Unaligned access operation		*/
+#define CR_XP	(1 << 23)	/* Extended page tables			*/
+#define CR_VE	(1 << 24)	/* Vectored interrupts			*/
+#define CR_EE	(1 << 25)	/* Exception (Big) Endian		*/
+#define CR_TRE	(1 << 28)	/* TEX remap enable			*/
+#define CR_AFE	(1 << 29)	/* Access flag enable			*/
+#define CR_TE	(1 << 30)	/* Thumb exception enable		*/
+#endif
diff --git a/lib/arm/ptrace.h b/lib/arm/ptrace.h
new file mode 100644
index 0000000000000..3c8781508a72e
--- /dev/null
+++ b/lib/arm/ptrace.h
@@ -0,0 +1,100 @@
+#ifndef _ARM_PTRACE_H_
+#define _ARM_PTRACE_H_
+/*
+ * Adapted from Linux kernel headers
+ *   arch/arm/include/asm/ptrace.h
+ *   arch/arm/include/uapi/asm/ptrace.h
+ */
+
+/*
+ * PSR bits
+ */
+#define USR_MODE	0x00000010
+#define SVC_MODE	0x00000013
+#define FIQ_MODE	0x00000011
+#define IRQ_MODE	0x00000012
+#define ABT_MODE	0x00000017
+#define HYP_MODE	0x0000001a
+#define UND_MODE	0x0000001b
+#define SYSTEM_MODE	0x0000001f
+#define MODE32_BIT	0x00000010
+#define MODE_MASK	0x0000001f
+
+#define PSR_T_BIT	0x00000020	/* >= V4T, but not V7M */
+#define PSR_F_BIT	0x00000040	/* >= V4, but not V7M */
+#define PSR_I_BIT	0x00000080	/* >= V4, but not V7M */
+#define PSR_A_BIT	0x00000100	/* >= V6, but not V7M */
+#define PSR_E_BIT	0x00000200	/* >= V6, but not V7M */
+#define PSR_J_BIT	0x01000000	/* >= V5J, but not V7M */
+#define PSR_Q_BIT	0x08000000	/* >= V5E, including V7M */
+#define PSR_V_BIT	0x10000000
+#define PSR_C_BIT	0x20000000
+#define PSR_Z_BIT	0x40000000
+#define PSR_N_BIT	0x80000000
+
+/*
+ * Groups of PSR bits
+ */
+#define PSR_f		0xff000000	/* Flags                */
+#define PSR_s		0x00ff0000	/* Status               */
+#define PSR_x		0x0000ff00	/* Extension            */
+#define PSR_c		0x000000ff	/* Control              */
+
+/*
+ * ARMv7 groups of PSR bits
+ */
+#define APSR_MASK	0xf80f0000	/* N, Z, C, V, Q and GE flags */
+#define PSR_ISET_MASK	0x01000010	/* ISA state (J, T) mask */
+#define PSR_IT_MASK	0x0600fc00	/* If-Then execution state mask */
+#define PSR_ENDIAN_MASK	0x00000200	/* Endianness state mask */
+
+#ifndef __ASSEMBLY__
+#include "libcflat.h"
+
+struct pt_regs {
+	unsigned long uregs[18];
+};
+
+#define ARM_cpsr	uregs[16]
+#define ARM_pc		uregs[15]
+#define ARM_lr		uregs[14]
+#define ARM_sp		uregs[13]
+#define ARM_ip		uregs[12]
+#define ARM_fp		uregs[11]
+#define ARM_r10		uregs[10]
+#define ARM_r9		uregs[9]
+#define ARM_r8		uregs[8]
+#define ARM_r7		uregs[7]
+#define ARM_r6		uregs[6]
+#define ARM_r5		uregs[5]
+#define ARM_r4		uregs[4]
+#define ARM_r3		uregs[3]
+#define ARM_r2		uregs[2]
+#define ARM_r1		uregs[1]
+#define ARM_r0		uregs[0]
+#define ARM_ORIG_r0	uregs[17]
+
+#define user_mode(regs) \
+	(((regs)->ARM_cpsr & 0xf) == 0)
+
+#define processor_mode(regs) \
+	((regs)->ARM_cpsr & MODE_MASK)
+
+#define interrupts_enabled(regs) \
+	(!((regs)->ARM_cpsr & PSR_I_BIT))
+
+#define fast_interrupts_enabled(regs) \
+	(!((regs)->ARM_cpsr & PSR_F_BIT))
+
+#define MAX_REG_OFFSET (offsetof(struct pt_regs, ARM_ORIG_r0))
+
+static inline unsigned long regs_get_register(struct pt_regs *regs,
+					      unsigned int offset)
+{
+	if (offset > MAX_REG_OFFSET)
+		return 0;
+	return *(unsigned long *)((unsigned long)regs + offset);
+}
+
+#endif /* __ASSEMBLY__ */
+#endif /* _ARM_PTRACE_H_ */
diff --git a/scripts/arm/asm-offsets.c b/scripts/arm/asm-offsets.c
new file mode 100644
index 0000000000000..03b22da907d58
--- /dev/null
+++ b/scripts/arm/asm-offsets.c
@@ -0,0 +1,40 @@
+/*
+ * Adapted from arch/arm/kernel/asm-offsets.c
+ */
+#include "libcflat.h"
+#include "arm/ptrace.h"
+
+#define P(sym, val) \
+	printf("#define " #sym "\t0x%x\n", val)
+
+int main(void)
+{
+	printf("#ifndef _ARM_ASM_OFFSETS_H_\n");
+	printf("#define _ARM_ASM_OFFSETS_H_\n");
+	printf("/*\n");
+	printf(" * Generated file. Regenerate with 'make asm-offsets'\n");
+	printf(" */\n");
+	printf("\n");
+	P(S_R0, offsetof(struct pt_regs, ARM_r0));
+	P(S_R1, offsetof(struct pt_regs, ARM_r1));
+	P(S_R2, offsetof(struct pt_regs, ARM_r2));
+	P(S_R3, offsetof(struct pt_regs, ARM_r3));
+	P(S_R4, offsetof(struct pt_regs, ARM_r4));
+	P(S_R5, offsetof(struct pt_regs, ARM_r5));
+	P(S_R6, offsetof(struct pt_regs, ARM_r6));
+	P(S_R7, offsetof(struct pt_regs, ARM_r7));
+	P(S_R8, offsetof(struct pt_regs, ARM_r8));
+	P(S_R9, offsetof(struct pt_regs, ARM_r9));
+	P(S_R10, offsetof(struct pt_regs, ARM_r10));
+	P(S_FP, offsetof(struct pt_regs, ARM_fp));
+	P(S_IP, offsetof(struct pt_regs, ARM_ip));
+	P(S_SP, offsetof(struct pt_regs, ARM_sp));
+	P(S_LR, offsetof(struct pt_regs, ARM_lr));
+	P(S_PC, offsetof(struct pt_regs, ARM_pc));
+	P(S_PSR, offsetof(struct pt_regs, ARM_cpsr));
+	P(S_OLD_R0, offsetof(struct pt_regs, ARM_ORIG_r0));
+	P(S_FRAME_SIZE, sizeof(struct pt_regs));
+	printf("\n");
+	printf("#endif /* _ARM_ASM_OFFSETS_H_ */\n");
+	return 0;
+}
-- 
1.8.1.4

[PATCH 3/3] arm: vectors support

[Andrew Jones]

Add support for tests to use exception handlers.

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;
+/* NOTE: update clobber list if passed insns needs more than r0,r1 */
+#define test_excptn(pre_insns, excptn_insn, post_insns)		\
+	asm volatile(						\
+		pre_insns "\n"					\
+		"mov	r0, %0\n"				\
+		"stmia	r0, { r0-lr }\n"			\
+		"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"				\
+		"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)
+
+static bool check_regs(struct pt_regs *regs)
+{
+	unsigned i;
+
+	if (!svc_mode())
+		return false;
+
+	for (i = 0; i < ARRAY_SIZE(regs->uregs); ++i)
+		if (regs->uregs[i] != expected_regs.uregs[i])
+			return false;
+
+	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)) {
+		/*
+		 * 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"
+		);
+	} 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
 	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:
+	mrs	r3, cpsr
+	ldr	r4, =exception_stacks
+		/* first frame for svc mode */
+	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
+
 .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
+
+.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
+	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
+
+	/* 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 }^
+
+.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"
+};
+
+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"
+		: "=r" (ctrl), "=r" (transbase), "=r" (dac));
+		printf("Control: %08x  Table: %08x  DAC: %08x\n",
+			ctrl, transbase, dac);
+	}
+}
+
+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;
+}
+
+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;
+	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"
+	:: "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,
+	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

Re: [PATCH 1/3] printf: support field padding

[Christoffer Dall]

On Fri, Dec 13, 2013 at 11:58:04AM +0100, Andrew Jones wrote:
> Support format flags for field padding, such as "%08x", allowing
> register dumps to be easier on the eyes.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/printf.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 70 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/printf.c b/lib/printf.c
> index 867eb1926f742..dd20f755c01a0 100644
> --- a/lib/printf.c
> +++ b/lib/printf.c
> @@ -6,6 +6,11 @@ typedef struct pstream {
>      int added;
>  } pstream_t;
>  
> +typedef struct strprops {
> +    char pad;
> +    int npad;
> +} strprops_t;
> +
>  static void addchar(pstream_t *p, char c)
>  {
>      if (p->remain) {
> @@ -15,15 +20,37 @@ static void addchar(pstream_t *p, char c)
>      ++p->added;
>  }
>  
> -void print_str(pstream_t *p, const char *s)
> +void print_str(pstream_t *p, const char *s, strprops_t props)
>  {
> +    const char *s_orig = s;
> +    int npad = props.npad;
> +
> +    if (npad > 0) {
> +	npad -= strlen(s_orig);
> +	while (npad > 0) {
> +	    addchar(p, props.pad);
> +	    --npad;
> +	}
> +    }
> +
>      while (*s)
>  	addchar(p, *s++);
> +
> +    if (npad < 0) {
> +	char pad = props.pad;
> +	if (pad == '0') /* ignore '0' flag with '-' flag */
> +	    pad = ' ';

there are only the two options, so you can drop the check if
you like.

> +	npad += strlen(s_orig);
> +	while (npad < 0) {
> +	    addchar(p, pad);
> +	    ++npad;
> +	}
> +    }
>  }
>  
>  static char digits[16] = "0123456789abcdef";
>  
> -void print_int(pstream_t *ps, long long n, int base)
> +void print_int(pstream_t *ps, long long n, int base, strprops_t props)
>  {
>      char buf[sizeof(long) * 3 + 2], *p = buf;
>      int s = 0, i;
> @@ -54,10 +81,11 @@ void print_int(pstream_t *ps, long long n, int base)
>  
>      *p = 0;
>  
> -    print_str(ps, buf);
> +    print_str(ps, buf, props);
>  }
>  
> -void print_unsigned(pstream_t *ps, unsigned long long n, int base)
> +void print_unsigned(pstream_t *ps, unsigned long long n, int base,
> +		    strprops_t props)
>  {
>      char buf[sizeof(long) * 3 + 1], *p = buf;
>      int i;
> @@ -80,7 +108,23 @@ void print_unsigned(pstream_t *ps, unsigned long long n, int base)
>  
>      *p = 0;
>  
> -    print_str(ps, buf);
> +    print_str(ps, buf, props);
> +}
> +
> +static int fmtnum(const char **fmt)
> +{
> +    const char *f = *fmt;
> +    int len = 0, num;
> +
> +    if (*f == '-')
> +	++f, ++len;

oh wow, this deserves a small comment saying that negative values are
used to add trailing padding instead of leading.

> +
> +    while (*f >= '0' && *f <= '9')
> +	++f, ++len;
> +
> +    num = atol(*fmt);
> +    *fmt += len;
> +    return num;
>  }

some funny indentation is back here...  Better check your entire patch
for that.

>  
>  int vsnprintf(char *buf, int size, const char *fmt, va_list va)
> @@ -93,6 +137,9 @@ int vsnprintf(char *buf, int size, const char *fmt, va_list va)
>      while (*fmt) {
>  	char f = *fmt++;
>  	int nlong = 0;
> +	strprops_t props;
> +	memset(&props, 0, sizeof(props));
> +	props.pad = ' ';
>  
>  	if (f != '%') {
>  	    addchar(&s, f);
> @@ -110,41 +157,50 @@ int vsnprintf(char *buf, int size, const char *fmt, va_list va)
>  	case '\0':
>  	    --fmt;
>  	    break;
> +	case '0':
> +	    props.pad = '0';
> +	    ++fmt;
> +	    /* fall through */
> +	case '1'...'9':
> +	case '-':
> +	    --fmt;
> +	    props.npad = fmtnum(&fmt);
> +	    goto morefmt;
>  	case 'l':
>  	    ++nlong;
>  	    goto morefmt;
>  	case 'd':
>  	    switch (nlong) {
>  	    case 0:
> -		print_int(&s, va_arg(va, int), 10);
> +		print_int(&s, va_arg(va, int), 10, props);
>  		break;
>  	    case 1:
> -		print_int(&s, va_arg(va, long), 10);
> +		print_int(&s, va_arg(va, long), 10, props);
>  		break;
>  	    default:
> -		print_int(&s, va_arg(va, long long), 10);
> +		print_int(&s, va_arg(va, long long), 10, props);
>  		break;
>  	    }
>  	    break;
>  	case 'x':
>  	    switch (nlong) {
>  	    case 0:
> -		print_unsigned(&s, va_arg(va, unsigned), 16);
> +		print_unsigned(&s, va_arg(va, unsigned), 16, props);
>  		break;
>  	    case 1:
> -		print_unsigned(&s, va_arg(va, unsigned long), 16);
> +		print_unsigned(&s, va_arg(va, unsigned long), 16, props);
>  		break;
>  	    default:
> -		print_unsigned(&s, va_arg(va, unsigned long long), 16);
> +		print_unsigned(&s, va_arg(va, unsigned long long), 16, props);
>  		break;
>  	    }
>  	    break;
>  	case 'p':
> -	    print_str(&s, "0x");
> -	    print_unsigned(&s, (unsigned long)va_arg(va, void *), 16);
> +	    print_str(&s, "0x", props);
> +	    print_unsigned(&s, (unsigned long)va_arg(va, void *), 16, props);
>  	    break;
>  	case 's':
> -	    print_str(&s, va_arg(va, const char *));
> +	    print_str(&s, va_arg(va, const char *), props);
>  	    break;
>  	default:
>  	    addchar(&s, f);
> -- 
> 1.8.1.4
> 

Besides the formatting stuff:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

Re: [PATCH 2/3] arm: add useful headers from the linux kernel

[Christoffer Dall]

On Fri, Dec 13, 2013 at 11:58:05AM +0100, Andrew Jones wrote:
> We're going to need PSR bit defines and pt_regs. We'll also need
> pt_regs offsets in assembly code. This patch adapts the linux
> kernel's ptrace.h and asm-offsets to this framework. Even though
> lib/arm/asm-offsets.h is a generated file, we still commit it,
> as it's unlikely to change. Also adapt cp15.h from the kernel,
> since we'll need bit defines from there too.

Why commit the autogenerated header?  That will just create noise in the
git log...  It seems like it's auto-building it every time anyway, so it
would only be to avoid that step...

> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  config/config-arm.mak     |  17 ++++++--
>  lib/arm/asm-offsets.h     |  27 +++++++++++++
>  lib/arm/cp15.h            |  36 +++++++++++++++++
>  lib/arm/ptrace.h          | 100 ++++++++++++++++++++++++++++++++++++++++++++++
>  scripts/arm/asm-offsets.c |  40 +++++++++++++++++++
>  5 files changed, 216 insertions(+), 4 deletions(-)
>  create mode 100644 lib/arm/asm-offsets.h
>  create mode 100644 lib/arm/cp15.h
>  create mode 100644 lib/arm/ptrace.h
>  create mode 100644 scripts/arm/asm-offsets.c
> 
> diff --git a/config/config-arm.mak b/config/config-arm.mak
> index d0814186b279c..173e606fbe64c 100644
> --- a/config/config-arm.mak
> +++ b/config/config-arm.mak
> @@ -1,3 +1,4 @@
> +PROCESSOR = cortex-a15
>  mach = mach-virt
>  iodevs = pl011 virtio_mmio
>  phys_base = 0x40000000
> @@ -30,8 +31,7 @@ $(libcflat) $(libeabi): CFLAGS += -ffreestanding -I lib
>  
>  CFLAGS += -Wextra
>  CFLAGS += -marm
> -#CFLAGS += -mcpu=$(PROCESSOR)
> -CFLAGS += -mcpu=cortex-a15
> +CFLAGS += -mcpu=$(PROCESSOR)
>  CFLAGS += -O2

unrelated changes?

>  
>  libgcc := $(shell $(CC) -m$(ARCH) --print-libgcc-file-name)
> @@ -55,7 +55,7 @@ tests_and_config = $(TEST_DIR)/*.flat $(TEST_DIR)/unittests.cfg
>  
>  test_cases: $(tests-common) $(tests)
>  
> -$(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I lib
> +$(TEST_DIR)/%.o scripts/arm/%.o: CFLAGS += -std=gnu99 -ffreestanding -I lib
>  
>  $(TEST_DIR)/boot.elf: $(cstart.o) $(TEST_DIR)/boot.o
>  
> @@ -67,7 +67,16 @@ lib/$(TEST_DIR)/mach-virt.dts:
>  	$(QEMU_BIN) -kernel /dev/null -M virt -machine dumpdtb=$(dtb)
>  	fdtdump $(dtb) > $@
>  
> +.PHONY: asm-offsets
> +
> +asm-offsets: scripts/arm/asm-offsets.flat
> +	$(QEMU_BIN) -device virtio-testdev -display none -serial stdio \
> +		-M virt -cpu $(PROCESSOR) \
> +		-kernel $^ > lib/arm/asm-offsets.h || true

this is a shame, you're depending on a full working setup with a correct
QEMU to just build this thing.  Did you consider replicating the
kernel's Kbuild approach?

In fact, when I started playing around with this, it was quite the
hassle, because I run with a cross-compiled QEMU for my ARM devices
which doesn't compile on my build-machine, so now I need a different
x86-compiled QEMU with mach-virt support compiled somewhere, and I can
only build unit-tests when I remember to set the QEMU variable in my
shell.  So if we really stick with this approach, why not make it part
of the ./configure options?  That being said, I really want the build
here only to depend on having an ARM compiler :((

> +scripts/arm/asm-offsets.elf: $(cstart.o) scripts/arm/asm-offsets.o
> +
>  arch_clean:
>  	$(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
>  	$(libeabi) $(eabiobjs) $(TEST_DIR)/.*.d lib/arm/.*.d \
> -	lib/$(TEST_DIR)/iomaps.gen.c lib/$(TEST_DIR)/mach-virt.*
> +	lib/$(TEST_DIR)/iomaps.gen.c lib/$(TEST_DIR)/mach-virt.* \
> +	scripts/arm/.*.d scripts/arm/*.o scripts/arm/*.flat scripts/arm/*.elf
> diff --git a/lib/arm/asm-offsets.h b/lib/arm/asm-offsets.h
> new file mode 100644
> index 0000000000000..b43be20ef8377
> --- /dev/null
> +++ b/lib/arm/asm-offsets.h
> @@ -0,0 +1,27 @@
> +#ifndef _ARM_ASM_OFFSETS_H_
> +#define _ARM_ASM_OFFSETS_H_
> +/*
> + * Generated file. Regenerate with 'make asm-offsets'
> + */
> +
> +#define S_R0	0x0
> +#define S_R1	0x4
> +#define S_R2	0x8
> +#define S_R3	0xc
> +#define S_R4	0x10
> +#define S_R5	0x14
> +#define S_R6	0x18
> +#define S_R7	0x1c
> +#define S_R8	0x20
> +#define S_R9	0x24
> +#define S_R10	0x28
> +#define S_FP	0x2c
> +#define S_IP	0x30
> +#define S_SP	0x34
> +#define S_LR	0x38
> +#define S_PC	0x3c
> +#define S_PSR	0x40
> +#define S_OLD_R0	0x44
> +#define S_FRAME_SIZE	0x48
> +
> +#endif /* _ARM_ASM_OFFSETS_H_ */
> diff --git a/lib/arm/cp15.h b/lib/arm/cp15.h
> new file mode 100644
> index 0000000000000..6a8a29aadb008
> --- /dev/null
> +++ b/lib/arm/cp15.h
> @@ -0,0 +1,36 @@
> +#ifndef _ARM_CP15_H_
> +#define _ARM_CP15_H_
> +/*
> + * From Linux kernel arch/arm/include/asm/cp15.h
> + *
> + * CR1 bits (CP#15 CR1)
> + */
> +#define CR_M	(1 << 0)	/* MMU enable				*/
> +#define CR_A	(1 << 1)	/* Alignment abort enable		*/
> +#define CR_C	(1 << 2)	/* Dcache enable			*/
> +#define CR_W	(1 << 3)	/* Write buffer enable			*/
> +#define CR_P	(1 << 4)	/* 32-bit exception handler		*/
> +#define CR_D	(1 << 5)	/* 32-bit data address range		*/
> +#define CR_L	(1 << 6)	/* Implementation defined		*/
> +#define CR_B	(1 << 7)	/* Big endian				*/
> +#define CR_S	(1 << 8)	/* System MMU protection		*/
> +#define CR_R	(1 << 9)	/* ROM MMU protection			*/
> +#define CR_F	(1 << 10)	/* Implementation defined		*/
> +#define CR_Z	(1 << 11)	/* Implementation defined		*/
> +#define CR_I	(1 << 12)	/* Icache enable			*/
> +#define CR_V	(1 << 13)	/* Vectors relocated to 0xffff0000	*/
> +#define CR_RR	(1 << 14)	/* Round Robin cache replacement	*/
> +#define CR_L4	(1 << 15)	/* LDR pc can set T bit			*/
> +#define CR_DT	(1 << 16)
> +#define CR_HA	(1 << 17)	/* Hardware management of Access Flag	*/
> +#define CR_IT	(1 << 18)
> +#define CR_ST	(1 << 19)
> +#define CR_FI	(1 << 21)	/* Fast interrupt (lower latency mode)	*/
> +#define CR_U	(1 << 22)	/* Unaligned access operation		*/
> +#define CR_XP	(1 << 23)	/* Extended page tables			*/
> +#define CR_VE	(1 << 24)	/* Vectored interrupts			*/
> +#define CR_EE	(1 << 25)	/* Exception (Big) Endian		*/
> +#define CR_TRE	(1 << 28)	/* TEX remap enable			*/
> +#define CR_AFE	(1 << 29)	/* Access flag enable			*/
> +#define CR_TE	(1 << 30)	/* Thumb exception enable		*/
> +#endif
> diff --git a/lib/arm/ptrace.h b/lib/arm/ptrace.h
> new file mode 100644
> index 0000000000000..3c8781508a72e
> --- /dev/null
> +++ b/lib/arm/ptrace.h
> @@ -0,0 +1,100 @@
> +#ifndef _ARM_PTRACE_H_
> +#define _ARM_PTRACE_H_
> +/*
> + * Adapted from Linux kernel headers
> + *   arch/arm/include/asm/ptrace.h
> + *   arch/arm/include/uapi/asm/ptrace.h
> + */
> +
> +/*
> + * PSR bits
> + */
> +#define USR_MODE	0x00000010
> +#define SVC_MODE	0x00000013
> +#define FIQ_MODE	0x00000011
> +#define IRQ_MODE	0x00000012
> +#define ABT_MODE	0x00000017
> +#define HYP_MODE	0x0000001a
> +#define UND_MODE	0x0000001b
> +#define SYSTEM_MODE	0x0000001f
> +#define MODE32_BIT	0x00000010
> +#define MODE_MASK	0x0000001f
> +
> +#define PSR_T_BIT	0x00000020	/* >= V4T, but not V7M */
> +#define PSR_F_BIT	0x00000040	/* >= V4, but not V7M */
> +#define PSR_I_BIT	0x00000080	/* >= V4, but not V7M */
> +#define PSR_A_BIT	0x00000100	/* >= V6, but not V7M */
> +#define PSR_E_BIT	0x00000200	/* >= V6, but not V7M */
> +#define PSR_J_BIT	0x01000000	/* >= V5J, but not V7M */
> +#define PSR_Q_BIT	0x08000000	/* >= V5E, including V7M */
> +#define PSR_V_BIT	0x10000000
> +#define PSR_C_BIT	0x20000000
> +#define PSR_Z_BIT	0x40000000
> +#define PSR_N_BIT	0x80000000
> +
> +/*
> + * Groups of PSR bits
> + */
> +#define PSR_f		0xff000000	/* Flags                */
> +#define PSR_s		0x00ff0000	/* Status               */
> +#define PSR_x		0x0000ff00	/* Extension            */
> +#define PSR_c		0x000000ff	/* Control              */
> +
> +/*
> + * ARMv7 groups of PSR bits
> + */
> +#define APSR_MASK	0xf80f0000	/* N, Z, C, V, Q and GE flags */
> +#define PSR_ISET_MASK	0x01000010	/* ISA state (J, T) mask */
> +#define PSR_IT_MASK	0x0600fc00	/* If-Then execution state mask */
> +#define PSR_ENDIAN_MASK	0x00000200	/* Endianness state mask */
> +
> +#ifndef __ASSEMBLY__
> +#include "libcflat.h"
> +
> +struct pt_regs {
> +	unsigned long uregs[18];
> +};
> +
> +#define ARM_cpsr	uregs[16]
> +#define ARM_pc		uregs[15]
> +#define ARM_lr		uregs[14]
> +#define ARM_sp		uregs[13]
> +#define ARM_ip		uregs[12]
> +#define ARM_fp		uregs[11]
> +#define ARM_r10		uregs[10]
> +#define ARM_r9		uregs[9]
> +#define ARM_r8		uregs[8]
> +#define ARM_r7		uregs[7]
> +#define ARM_r6		uregs[6]
> +#define ARM_r5		uregs[5]
> +#define ARM_r4		uregs[4]
> +#define ARM_r3		uregs[3]
> +#define ARM_r2		uregs[2]
> +#define ARM_r1		uregs[1]
> +#define ARM_r0		uregs[0]
> +#define ARM_ORIG_r0	uregs[17]
> +
> +#define user_mode(regs) \
> +	(((regs)->ARM_cpsr & 0xf) == 0)
> +
> +#define processor_mode(regs) \
> +	((regs)->ARM_cpsr & MODE_MASK)
> +
> +#define interrupts_enabled(regs) \
> +	(!((regs)->ARM_cpsr & PSR_I_BIT))
> +
> +#define fast_interrupts_enabled(regs) \
> +	(!((regs)->ARM_cpsr & PSR_F_BIT))
> +
> +#define MAX_REG_OFFSET (offsetof(struct pt_regs, ARM_ORIG_r0))
> +
> +static inline unsigned long regs_get_register(struct pt_regs *regs,
> +					      unsigned int offset)
> +{
> +	if (offset > MAX_REG_OFFSET)
> +		return 0;
> +	return *(unsigned long *)((unsigned long)regs + offset);
> +}
> +
> +#endif /* __ASSEMBLY__ */
> +#endif /* _ARM_PTRACE_H_ */
> diff --git a/scripts/arm/asm-offsets.c b/scripts/arm/asm-offsets.c
> new file mode 100644
> index 0000000000000..03b22da907d58
> --- /dev/null
> +++ b/scripts/arm/asm-offsets.c
> @@ -0,0 +1,40 @@
> +/*
> + * Adapted from arch/arm/kernel/asm-offsets.c
> + */
> +#include "libcflat.h"
> +#include "arm/ptrace.h"
> +
> +#define P(sym, val) \
> +	printf("#define " #sym "\t0x%x\n", val)
> +
> +int main(void)
> +{
> +	printf("#ifndef _ARM_ASM_OFFSETS_H_\n");
> +	printf("#define _ARM_ASM_OFFSETS_H_\n");
> +	printf("/*\n");
> +	printf(" * Generated file. Regenerate with 'make asm-offsets'\n");
> +	printf(" */\n");
> +	printf("\n");
> +	P(S_R0, offsetof(struct pt_regs, ARM_r0));
> +	P(S_R1, offsetof(struct pt_regs, ARM_r1));
> +	P(S_R2, offsetof(struct pt_regs, ARM_r2));
> +	P(S_R3, offsetof(struct pt_regs, ARM_r3));
> +	P(S_R4, offsetof(struct pt_regs, ARM_r4));
> +	P(S_R5, offsetof(struct pt_regs, ARM_r5));
> +	P(S_R6, offsetof(struct pt_regs, ARM_r6));
> +	P(S_R7, offsetof(struct pt_regs, ARM_r7));
> +	P(S_R8, offsetof(struct pt_regs, ARM_r8));
> +	P(S_R9, offsetof(struct pt_regs, ARM_r9));
> +	P(S_R10, offsetof(struct pt_regs, ARM_r10));
> +	P(S_FP, offsetof(struct pt_regs, ARM_fp));
> +	P(S_IP, offsetof(struct pt_regs, ARM_ip));
> +	P(S_SP, offsetof(struct pt_regs, ARM_sp));
> +	P(S_LR, offsetof(struct pt_regs, ARM_lr));
> +	P(S_PC, offsetof(struct pt_regs, ARM_pc));
> +	P(S_PSR, offsetof(struct pt_regs, ARM_cpsr));
> +	P(S_OLD_R0, offsetof(struct pt_regs, ARM_ORIG_r0));
> +	P(S_FRAME_SIZE, sizeof(struct pt_regs));
> +	printf("\n");
> +	printf("#endif /* _ARM_ASM_OFFSETS_H_ */\n");
> +	return 0;
> +}
> -- 
> 1.8.1.4
> 

-- 
Christoffer

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

[Christoffer Dall]

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@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...

> +/* 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

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

[Peter Maydell]

On 29 December 2013 06:32, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Fri, Dec 13, 2013 at 11:58:06AM +0100, Andrew Jones wrote:

>> +     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.

QEMU doesn't implement those because they are present
in ARMv7VE (ie v7 + virtualization extensions) only, and
we don't emulate the virt extensions.

thanks
-- PMM

Re: [PATCH 1/3] printf: support field padding

[Andrew Jones]

On Sat, Dec 28, 2013 at 10:31:44PM -0800, Christoffer Dall wrote:
> > +
> > +    if (npad < 0) {
> > +	char pad = props.pad;
> > +	if (pad == '0') /* ignore '0' flag with '-' flag */
> > +	    pad = ' ';
> 
> there are only the two options, so you can drop the check if
> you like.

true. removed.

> > +static int fmtnum(const char **fmt)
> > +{
> > +    const char *f = *fmt;
> > +    int len = 0, num;
> > +
> > +    if (*f == '-')
> > +	++f, ++len;
> 
> oh wow, this deserves a small comment saying that negative values are
> used to add trailing padding instead of leading.

You mean something beyond "man 3 printf; /flag"? :-)

> 
> > +
> > +    while (*f >= '0' && *f <= '9')
> > +	++f, ++len;
> > +
> > +    num = atol(*fmt);
> > +    *fmt += len;
> > +    return num;
> >  }
> 
> some funny indentation is back here...  Better check your entire patch
> for that.

The whole file has the funny indentation, I just followed suit. The
alternative is to add a patch that "fixes" all the pre-existing lib/*
files first, but for this patch I didn't think it was worth it.

drew

Re: [PATCH 2/3] arm: add useful headers from the linux kernel

[Andrew Jones]

On Sat, Dec 28, 2013 at 10:31:58PM -0800, Christoffer Dall wrote:
> On Fri, Dec 13, 2013 at 11:58:05AM +0100, Andrew Jones wrote:
> > We're going to need PSR bit defines and pt_regs. We'll also need
> > pt_regs offsets in assembly code. This patch adapts the linux
> > kernel's ptrace.h and asm-offsets to this framework. Even though
> > lib/arm/asm-offsets.h is a generated file, we still commit it,
> > as it's unlikely to change. Also adapt cp15.h from the kernel,
> > since we'll need bit defines from there too.
> 
> Why commit the autogenerated header?  That will just create noise in the
> git log...  It seems like it's auto-building it every time anyway, so it
> would only be to avoid that step...

Hmm, it doesn't for me. For me you need to do 'make asm-offsets' to
regenerate it, which will likely only be necessary when we add more
structs. Possibly we'll be adding structs frequently at first, but
eventually we'll have what we need, and then the churn will be low.
Anyway, whatever, it just didn't seem necessary to me, and also nice
to be able to git-diff the asm-offsets.h file, rather than just the
generator.

> > diff --git a/config/config-arm.mak b/config/config-arm.mak
> > index d0814186b279c..173e606fbe64c 100644
> > --- a/config/config-arm.mak
> > +++ b/config/config-arm.mak
> > @@ -1,3 +1,4 @@
> > +PROCESSOR = cortex-a15
> >  mach = mach-virt
> >  iodevs = pl011 virtio_mmio
> >  phys_base = 0x40000000
> > @@ -30,8 +31,7 @@ $(libcflat) $(libeabi): CFLAGS += -ffreestanding -I lib
> >  
> >  CFLAGS += -Wextra
> >  CFLAGS += -marm
> > -#CFLAGS += -mcpu=$(PROCESSOR)
> > -CFLAGS += -mcpu=cortex-a15
> > +CFLAGS += -mcpu=$(PROCESSOR)
> >  CFLAGS += -O2
> 
> unrelated changes?

yeah...

> 
> >  
> >  libgcc := $(shell $(CC) -m$(ARCH) --print-libgcc-file-name)
> > @@ -55,7 +55,7 @@ tests_and_config = $(TEST_DIR)/*.flat $(TEST_DIR)/unittests.cfg
> >  
> >  test_cases: $(tests-common) $(tests)
> >  
> > -$(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I lib
> > +$(TEST_DIR)/%.o scripts/arm/%.o: CFLAGS += -std=gnu99 -ffreestanding -I lib
> >  
> >  $(TEST_DIR)/boot.elf: $(cstart.o) $(TEST_DIR)/boot.o
> >  
> > @@ -67,7 +67,16 @@ lib/$(TEST_DIR)/mach-virt.dts:
> >  	$(QEMU_BIN) -kernel /dev/null -M virt -machine dumpdtb=$(dtb)
> >  	fdtdump $(dtb) > $@
> >  
> > +.PHONY: asm-offsets
> > +
> > +asm-offsets: scripts/arm/asm-offsets.flat
> > +	$(QEMU_BIN) -device virtio-testdev -display none -serial stdio \
> > +		-M virt -cpu $(PROCESSOR) \
> > +		-kernel $^ > lib/arm/asm-offsets.h || true
> 
> this is a shame, you're depending on a full working setup with a correct
> QEMU to just build this thing.  Did you consider replicating the
> kernel's Kbuild approach?

Actually, I'm not, because we commit the asm-offsets.h file :-) So unless
you're changing it, you don't need the above target. I'm not sure what
part of Kbuild you're referring to. I certainly don't want to over-engineer
the build process of these unit tests though.

> 
> In fact, when I started playing around with this, it was quite the
> hassle, because I run with a cross-compiled QEMU for my ARM devices
> which doesn't compile on my build-machine, so now I need a different
> x86-compiled QEMU with mach-virt support compiled somewhere, and I can
> only build unit-tests when I remember to set the QEMU variable in my
> shell.  So if we really stick with this approach, why not make it part
> of the ./configure options?  That being said, I really want the build
> here only to depend on having an ARM compiler :((

Ahh, maybe we should commit the dts that mach-virt generates too then.

drew

Re: [PATCH 1/3] printf: support field padding

[Christoffer Dall]

On Thu, Jan 02, 2014 at 06:09:48PM +0100, Andrew Jones wrote:
> On Sat, Dec 28, 2013 at 10:31:44PM -0800, Christoffer Dall wrote:
> > > +
> > > +    if (npad < 0) {
> > > +	char pad = props.pad;
> > > +	if (pad == '0') /* ignore '0' flag with '-' flag */
> > > +	    pad = ' ';
> > 
> > there are only the two options, so you can drop the check if
> > you like.
> 
> true. removed.
> 
> > > +static int fmtnum(const char **fmt)
> > > +{
> > > +    const char *f = *fmt;
> > > +    int len = 0, num;
> > > +
> > > +    if (*f == '-')
> > > +	++f, ++len;
> > 
> > oh wow, this deserves a small comment saying that negative values are
> > used to add trailing padding instead of leading.
> 
> You mean something beyond "man 3 printf; /flag"? :-)
> 

yes, that's a functional description, not helping the reader of the
implememtation.  But ok, once this works, it's not likely to pass many
eyes again.

> > 
> > > +
> > > +    while (*f >= '0' && *f <= '9')
> > > +	++f, ++len;
> > > +
> > > +    num = atol(*fmt);
> > > +    *fmt += len;
> > > +    return num;
> > >  }
> > 
> > some funny indentation is back here...  Better check your entire patch
> > for that.
> 
> The whole file has the funny indentation, I just followed suit. The
> alternative is to add a patch that "fixes" all the pre-existing lib/*
> files first, but for this patch I didn't think it was worth it.
> 
fair enough, but it really hurts when reading patches so we should fix
this some time...

-Christoffer

Re: [PATCH 2/3] arm: add useful headers from the linux kernel

[Christoffer Dall]

On Thu, Jan 02, 2014 at 06:24:10PM +0100, Andrew Jones wrote:
> On Sat, Dec 28, 2013 at 10:31:58PM -0800, Christoffer Dall wrote:
> > On Fri, Dec 13, 2013 at 11:58:05AM +0100, Andrew Jones wrote:
> > > We're going to need PSR bit defines and pt_regs. We'll also need
> > > pt_regs offsets in assembly code. This patch adapts the linux
> > > kernel's ptrace.h and asm-offsets to this framework. Even though
> > > lib/arm/asm-offsets.h is a generated file, we still commit it,
> > > as it's unlikely to change. Also adapt cp15.h from the kernel,
> > > since we'll need bit defines from there too.
> > 
> > Why commit the autogenerated header?  That will just create noise in the
> > git log...  It seems like it's auto-building it every time anyway, so it
> > would only be to avoid that step...
> 
> Hmm, it doesn't for me. For me you need to do 'make asm-offsets' to
> regenerate it, which will likely only be necessary when we add more
> structs. Possibly we'll be adding structs frequently at first, but
> eventually we'll have what we need, and then the churn will be low.
> Anyway, whatever, it just didn't seem necessary to me, and also nice
> to be able to git-diff the asm-offsets.h file, rather than just the
> generator.

So, it actually doesn't work for me to do 'make asm-offsets' if I do 'rm
lib/arm/asm-offsets.h'.  It complains about the file missing from other
tools that it tries to build first before producing the header file.  If
I do a 'touch lib/arm/asm-offsets.h', I get warnings about the missing
defines from building cstart.o and also if I do a make clean the 'make
asm-offsets' doesn't work for me anymore, because it depends on the
iomaps being generated, which doesn't work on my machine, so I manually
have to copy the iomaps.gen.c (which I generated by hand for now) first,
but still I don't get anywhere.

All this to say that with this appraoch, the 'make asm-offsets' relies
on a lot of logic to be in place, and it doesn't seem particularly easy
to make the compilation portable...

> 
> > > diff --git a/config/config-arm.mak b/config/config-arm.mak
> > > index d0814186b279c..173e606fbe64c 100644
> > > --- a/config/config-arm.mak
> > > +++ b/config/config-arm.mak
> > > @@ -1,3 +1,4 @@
> > > +PROCESSOR = cortex-a15
> > >  mach = mach-virt
> > >  iodevs = pl011 virtio_mmio
> > >  phys_base = 0x40000000
> > > @@ -30,8 +31,7 @@ $(libcflat) $(libeabi): CFLAGS += -ffreestanding -I lib
> > >  
> > >  CFLAGS += -Wextra
> > >  CFLAGS += -marm
> > > -#CFLAGS += -mcpu=$(PROCESSOR)
> > > -CFLAGS += -mcpu=cortex-a15
> > > +CFLAGS += -mcpu=$(PROCESSOR)
> > >  CFLAGS += -O2
> > 
> > unrelated changes?
> 
> yeah...
> 
> > 
> > >  
> > >  libgcc := $(shell $(CC) -m$(ARCH) --print-libgcc-file-name)
> > > @@ -55,7 +55,7 @@ tests_and_config = $(TEST_DIR)/*.flat $(TEST_DIR)/unittests.cfg
> > >  
> > >  test_cases: $(tests-common) $(tests)
> > >  
> > > -$(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I lib
> > > +$(TEST_DIR)/%.o scripts/arm/%.o: CFLAGS += -std=gnu99 -ffreestanding -I lib
> > >  
> > >  $(TEST_DIR)/boot.elf: $(cstart.o) $(TEST_DIR)/boot.o
> > >  
> > > @@ -67,7 +67,16 @@ lib/$(TEST_DIR)/mach-virt.dts:
> > >  	$(QEMU_BIN) -kernel /dev/null -M virt -machine dumpdtb=$(dtb)
> > >  	fdtdump $(dtb) > $@
> > >  
> > > +.PHONY: asm-offsets
> > > +
> > > +asm-offsets: scripts/arm/asm-offsets.flat
> > > +	$(QEMU_BIN) -device virtio-testdev -display none -serial stdio \
> > > +		-M virt -cpu $(PROCESSOR) \
> > > +		-kernel $^ > lib/arm/asm-offsets.h || true
> > 
> > this is a shame, you're depending on a full working setup with a correct
> > QEMU to just build this thing.  Did you consider replicating the
> > kernel's Kbuild approach?
> 
> Actually, I'm not, because we commit the asm-offsets.h file :-) So unless
> you're changing it, you don't need the above target. I'm not sure what
> part of Kbuild you're referring to. I certainly don't want to over-engineer
> the build process of these unit tests though.
> 
I don't think constructing a mechanism (which you can copy from the
kernel) that makes it possible to compile this tool without relying on a
another set of tools (that needs to be of a quite recent revision) is
over-engineering a build-process.  If you are going this approach, you
really need to check for the available tools as part of configure and
give nice user messages saying "you need a version of qemu newer than
1.7 compiled for your host architecture with target-arm support on your
build system) or something like that.

But again, since we can, I think this tool should really just build
without relying on that.

> > 
> > In fact, when I started playing around with this, it was quite the
> > hassle, because I run with a cross-compiled QEMU for my ARM devices
> > which doesn't compile on my build-machine, so now I need a different
> > x86-compiled QEMU with mach-virt support compiled somewhere, and I can
> > only build unit-tests when I remember to set the QEMU variable in my
> > shell.  So if we really stick with this approach, why not make it part
> > of the ./configure options?  That being said, I really want the build
> > here only to depend on having an ARM compiler :((
> 
> Ahh, maybe we should commit the dts that mach-virt generates too then.
> 

I would advise very strongly against that.  We want this tool to be
something that is broadly applied and used by KVM developers, and since
we really don't want another place where the whole
keep-the-kernel-and-device-tree-in-sync problem can be an issue.
Ideally QEMU just gives you a DTB and the test tool consumes this.

-Christoffer

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

[Andrew Jones]

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@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?

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

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

[Christoffer Dall]

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