[U-Boot] [PATCH v2 5/5] arm: move exception handling out of start.S files

[Albert ARIBAUD]

Hi Andreas,

On Sun, 09 Mar 2014 19:21:24 +0100, Andreas Bießmann
<andreas.devel at googlemail.com> wrote:

> Dear Albert Aribaud,
> 
> On 09.03.2014 17:18, Albert ARIBAUD wrote:
> 
> <snip>
> 
> > diff --git a/arch/arm/lib/vectors.S b/arch/arm/lib/vectors.S
> > new file mode 100644
> > index 0000000..1655d49
> > --- /dev/null
> > +++ b/arch/arm/lib/vectors.S
> > @@ -0,0 +1,304 @@
> > +/*
> > + *  vectors - Generic ARM exception table code
> > + *
> > + *  Copyright (c) 1998	Dan Malek <dmalek at jlc.net>
> > + *  Copyright (c) 1999	Magnus Damm <kieraypc01.p.y.kie.era.ericsson.se>
> > + *  Copyright (c) 2000	Wolfgang Denk <wd at denx.de>
> > + *  Copyright (c) 2001	Alex Züpke <azu at sysgo.de>
> > + *  Copyright (c) 2001	Marius Gröger <mag at sysgo.de>
> > + *  Copyright (c) 2002	Alex Züpke <azu at sysgo.de>
> > + *  Copyright (c) 2002	Gary Jennejohn <garyj at denx.de>
> > + *  Copyright (c) 2002	Kyle Harris <kharris at nexus-tech.net>
> > + *
> > + * SPDX-License-Identifier:	GPL-2.0+
> > + */
> > +
> > +/*
> > + *************************************************************************
> > + *
> > + * Symbol _start is referenced elsewhere, so make it global
> > + *
> > + *************************************************************************
> > + */
> > +
> > +.globl _start
> > +
> > +/*
> > + *************************************************************************
> > + *
> > + * Vectors have their own section so linker script can map them easily
> > + *
> > + *************************************************************************
> > + */
> > +
> > +	.section ".vectors", "x"
> > +
> > +/*
> > + *************************************************************************
> > + *
> > + * Exception vectors as described in ARM reference manuals
> > + *
> > + * Uses indirect branch to allow reaching handlers anywhere in memory.
> > + *
> > + *************************************************************************
> > + */
> > +
> > +_start:
> > +
> > +#ifdef CONFIG_SYS_DV_NOR_BOOT_CFG
> > +	.word	CONFIG_SYS_DV_NOR_BOOT_CFG
> > +#endif
> > +
> > +#ifdef CONFIG_SPL_BUILD
> > +
> > +_start:
> > +	ldr	pc, _reset
> > +	ldr	pc, _hang
> > +	ldr	pc, _hang
> > +	ldr	pc, _hang
> > +	ldr	pc, _hang
> > +	ldr	pc, _hang
> > +	ldr	pc, _hang
> > +	ldr	pc, _hang
> > +
> > +#else
> > +
> > +_start:
> > +	ldr	pc, _reset
> > +	ldr	pc, _undefined_instruction
> > +	ldr	pc, _software_interrupt
> > +	ldr	pc, _prefetch_abort
> > +	ldr	pc, _data_abort
> > +	ldr	pc, _not_used
> > +	ldr	pc, _irq
> > +	ldr	pc, _fiq
> > +
> > +#endif
> > +
> > +/*
> > + *************************************************************************
> > + *
> > + * Indirect vectors table
> > + *
> > + * Symbols referenced here must be defined somewhere else
> > + *
> > + *************************************************************************
> > + */
> > +
> > +_reset:			.word reset
> > +
> > +#ifdef CONFIG_SPL_BUILD
> > +
> > +_hang:			.word do_hang
> > +
> > +#else
> > +
> > +	.globl	_undefined_instruction
> > +	.globl	_software_interrupt
> > +	.globl	_prefetch_abort
> > +	.globl	_data_abort
> > +	.globl	_not_used
> > +	.globl	_irq
> > +	.globl	_fiq
> > +
> > +_undefined_instruction:	.word undefined_instruction
> > +_software_interrupt:	.word software_interrupt
> > +_prefetch_abort:	.word prefetch_abort
> > +_data_abort:		.word data_abort
> > +_not_used:		.word not_used
> > +_irq:			.word irq
> > +_fiq:			.word fiq
> > +
> > +#endif
> > +
> > +	.balignl 16,0xdeadbeef
> > +
> > +/*
> > + *************************************************************************
> > + *
> > + * Interrupt handling
> > + *
> > + *************************************************************************
> > + */
> > +
> > +/* IRQ stack memory (calculated at run-time) + 8 bytes */
> > +.globl IRQ_STACK_START_IN
> > +IRQ_STACK_START_IN:
> > +	.word	0x0badc0de
> > +
> > +#ifndef CONFIG_SPL_BUILD
> > +
> > +#ifdef CONFIG_USE_IRQ
> > +/* IRQ stack memory (calculated at run-time) */
> > +.globl IRQ_STACK_START
> > +IRQ_STACK_START:
> > +	.word	0x0badc0de
> > +
> > +/* IRQ stack memory (calculated at run-time) */
> > +.globl FIQ_STACK_START
> > +FIQ_STACK_START:
> > +	.word 0x0badc0de
> > +#endif
> > +
> > +@
> > +@ IRQ stack frame.
> > +@
> > +#define S_FRAME_SIZE	72
> > +
> > +#define S_OLD_R0	68
> > +#define S_PSR		64
> > +#define S_PC		60
> > +#define S_LR		56
> > +#define S_SP		52
> > +
> > +#define S_IP		48
> > +#define S_FP		44
> > +#define S_R10		40
> > +#define S_R9		36
> > +#define S_R8		32
> > +#define S_R7		28
> > +#define S_R6		24
> > +#define S_R5		20
> > +#define S_R4		16
> > +#define S_R3		12
> > +#define S_R2		8
> > +#define S_R1		4
> > +#define S_R0		0
> > +
> > +#define MODE_SVC 0x13
> > +#define I_BIT	 0x80
> > +
> > +/*
> > + * use bad_save_user_regs for abort/prefetch/undef/swi ...
> > + * use irq_save_user_regs / irq_restore_user_regs for IRQ/FIQ handling
> > + */
> > +
> > +	.macro	bad_save_user_regs
> > +	@ carve out a frame on current user stack
> > +	sub	sp, sp, #S_FRAME_SIZE
> > +	stmia	sp, {r0 - r12}	@ Save user registers (now in svc mode) r0-r12
> > +	ldr	r2, IRQ_STACK_START_IN
> > +	@ get values for "aborted" pc and cpsr (into parm regs)
> > +	ldmia	r2, {r2 - r3}
> > +	add	r0, sp, #S_FRAME_SIZE		@ grab pointer to old stack
> > +	add	r5, sp, #S_SP
> > +	mov	r1, lr
> > +	stmia	r5, {r0 - r3}	@ save sp_SVC, lr_SVC, pc, cpsr
> > +	mov	r0, sp		@ save current stack into r0 (param register)
> > +	.endm
> > +
> > +	.macro	irq_save_user_regs
> > +	sub	sp, sp, #S_FRAME_SIZE
> > +	stmia	sp, {r0 - r12}			@ Calling r0-r12
> > +	@ !!!! R8 NEEDS to be saved !!!! a reserved stack spot would be good.
> > +	add	r8, sp, #S_PC
> > +	stmdb	r8, {sp, lr}^		@ Calling SP, LR
> > +	str	lr, [r8, #0]		@ Save calling PC
> > +	mrs	r6, spsr
> > +	str	r6, [r8, #4]		@ Save CPSR
> > +	str	r0, [r8, #8]		@ Save OLD_R0
> > +	mov	r0, sp
> > +	.endm
> > +
> > +	.macro	irq_restore_user_regs
> > +	ldmia	sp, {r0 - lr}^			@ Calling r0 - lr
> > +	mov	r0, r0
> > +	ldr	lr, [sp, #S_PC]			@ Get PC
> > +	add	sp, sp, #S_FRAME_SIZE
> > +	subs	pc, lr, #4		@ return & move spsr_svc into cpsr
> > +	.endm
> > +
> > +	.macro get_bad_stack
> > +	ldr	r13, IRQ_STACK_START_IN		@ setup our mode stack
> > +
> > +	str	lr, [r13]	@ save caller lr in position 0 of saved stack
> > +	mrs	lr, spsr	@ get the spsr
> > +	str	lr, [r13, #4]	@ save spsr in position 1 of saved stack
> > +	mov	r13, #MODE_SVC	@ prepare SVC-Mode
> > +	@ msr	spsr_c, r13
> > +	msr	spsr, r13	@ switch modes, make sure moves will execute
> > +	mov	lr, pc		@ capture return pc
> > +	movs	pc, lr		@ jump to next instruction & switch modes.
> > +	.endm
> > +
> > +	.macro get_irq_stack			@ setup IRQ stack
> > +	ldr	sp, IRQ_STACK_START
> > +	.endm
> > +
> > +	.macro get_fiq_stack			@ setup FIQ stack
> > +	ldr	sp, FIQ_STACK_START
> > +	.endm
> > +#endif	/* CONFIG_SPL_BUILD */
> > +
> > +/*
> > + * exception handlers
> > + */
> > +#ifdef CONFIG_SPL_BUILD
> > +	.align	5
> > +do_hang:
> > +1:
> > +	bl	1b				/* hang and never return */
> > +#else	/* !CONFIG_SPL_BUILD */
> > +	.align  5
> > +undefined_instruction:
> > +	get_bad_stack
> > +	bad_save_user_regs
> > +	bl	do_undefined_instruction
> > +
> > +	.align	5
> > +software_interrupt:
> > +	get_bad_stack
> > +	bad_save_user_regs
> > +	bl	do_software_interrupt
> > +
> > +	.align	5
> > +prefetch_abort:
> > +	get_bad_stack
> > +	bad_save_user_regs
> > +	bl	do_prefetch_abort
> > +
> > +	.align	5
> > +data_abort:
> > +	get_bad_stack
> > +	bad_save_user_regs
> > +	bl	do_data_abort
> > +
> > +	.align	5
> > +not_used:
> > +	get_bad_stack
> > +	bad_save_user_regs
> > +	bl	do_not_used
> > +
> > +#ifdef CONFIG_USE_IRQ
> > +
> > +	.align	5
> > +irq:
> > +	get_irq_stack
> > +	irq_save_user_regs
> > +	bl	do_irq
> > +	irq_restore_user_regs
> > +
> > +	.align	5
> > +fiq:
> > +	get_fiq_stack
> > +	/* someone ought to write a more effiction fiq_save_user_regs */
> > +	irq_save_user_regs
> > +	bl	do_fiq
> > +	irq_restore_user_regs
> > +
> > +#else
> > +
> > +	.align	5
> > +irq:
> > +	get_bad_stack
> > +	bad_save_user_regs
> > +	bl	do_irq
> > +
> > +	.align	5
> > +fiq:
> > +	get_bad_stack
> > +	bad_save_user_regs
> > +	bl	do_fiq
> > +
> > +#endif
> > +#endif	/* CONFIG_SPL_BUILD */
> 
> this file is completely different for SPL and non-SPL builds. They both 
> share just the header of the file. I wonder why we introduce a single 
> vectors.S then. Wouldn't it be better to have a vectors.S for non-SPL 
> builds and lets say a vectors_SPL.S for SPL builds?

Yes, handling of exceptions in SPL and non-SPL has always been very
different - this series does not change that, as many start.S files
already had this 'duality' ; the series just factorizes it.

IUUC, if we go with separate files, then the choice of either 'hanging'
or 'calling a C routine' shall be performed by selecting the right
object (or at least the right source) in the makefile rather than by
selecting the right part of code at the preprocessor stage, right? If
so, I'm not sure how one is "better" than the other -- we remove logic
from the preprocessor but add it in the makefile.

Also, from a maintenance viewpoint, I would prefer to have the vectors
table defined in a single file; on the other hand, the two types of
handlers (hanging or C-calling) could be provided by two different
files.

Further comments welcome on either point.

> Best regards
> 
> Andreas Bießmann

Thanks for your comment!

Amicalement,
-- 
Albert.