[U-Boot] [PATCH v2 3/7] ARM: add assembly routine to switch to non-secure state

Thu Jun 20 00:27:17 CEST 2013

On Thu, Jun 13, 2013 at 01:01:09PM +0200, Andre Przywara wrote:
> While actually switching to non-secure state is one thing, the
> more important part of this process is to make sure that we still

super nit: not sure it's more important - it's just another thing we
need to do.

> have full access to the interrupt controller (GIC).
> The GIC is fully aware of secure vs. non-secure state, some
> registers are banked, others may be configured to be accessible from
> secure state only.
> To be as generic as possible, we get the GIC memory mapped address
> based on the PERIPHBASE value in the CBAR register. Since this
> register is not architecturally defined, we check the MIDR before to
> be from an A15 or A7.
> For CPUs not having the CBAR or boards with wrong information herein
> we allow providing the base address as a configuration variable.
> 
> With the GIC accessible, we:

"With the GIC accessible" ?

> a) allow private interrupts to be delivered to the core
>    (GICD_IGROUPR0 = 0xFFFFFFFF)
> b) enable the CPU interface (GICC_CTLR[0] = 1)
> c) set the priority filter to allow non-secure interrupts
>    (GICC_PMR = 0xFF)
> 
> After having switched to non-secure state, we also enable the
> non-secure GIC CPU interface, since this register is banked.
> 
> Also we allow access to all coprocessor interfaces from non-secure
> state by writing the appropriate bits in the NSACR register.

super nit 2: move this last paragraph above the non-secure stuff, so
there's no confusion that this is done from secure mode.

> 
> Since we need to call this routine also directly from the smp_pen
> later (where we don't have any stack), we can only use caller saved
> registers r0-r3 and r12 to not disturb the compiler.

the compiler certainly does seem to get cranky when we disturb it ;)

> 
> Signed-off-by: Andre Przywara <andre.przywara at linaro.org>
> ---
>  arch/arm/cpu/armv7/nonsec_virt.S | 66 ++++++++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/armv7.h     | 16 ++++++++++
>  arch/arm/include/asm/gic.h       | 17 +++++++++++
>  3 files changed, 99 insertions(+)
>  create mode 100644 arch/arm/include/asm/gic.h
> 
> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
> index f5572f5..656d99b 100644
> --- a/arch/arm/cpu/armv7/nonsec_virt.S
> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
> @@ -23,6 +23,8 @@
>   */
>  
>  #include <config.h>
> +#include <asm/gic.h>
> +#include <asm/armv7.h>
>  
>  /* the vector table for secure state */
>  _secure_vectors:
> @@ -52,3 +54,67 @@ _secure_monitor:
>  
>  	movs	pc, lr				@ return to non-secure SVC
>  
> +#define lo(x) ((x) & 0xFFFF)
> +#define hi(x) ((x) >> 16)
> +
> +/*
> + * Routine to switch a core to non-secure state.
> + * Initializes the GIC per-core interface, allows coprocessor access in
> + * non-secure modes and uses smc #0 to do the non-secure transition.
> + * To be called by smp_pen for secondary cores and directly for the BSP.
> + * For those two cases to work we must not use any stack and thus are
> + * limited to the caller saved registers r0-r3.

you also use r12 (ip) ?

Also, I think you can rewrite this comment to make it a little nicer.
May I propose something along the lines of:

/*
 * Switch a core to non-secure state.
 *
 *  1. initialize the GIC per-core interface
 *  2. allow coprocessor access in non-secure modes
 *  3. switch the cpu mode (by calling "smc #0")
 *
 * Called from smp_pen by non-primary cores and directly by the BSP.
 * Do not assume that the stack is available and only use registers
 * r0-r3.
 *
 * PERIPHBASE is used to get the GIC address. This could be 40 bits long,
 * though, but we check this in C before calling this function.
 */

(I only propose this to match the high standard of these patches)

> + * PERIPHBASE is used to get the GIC address. This could be 40 bits long,
> + * though, but we check this in C before calling this function.
> + */
> +.globl _nonsec_init
> +_nonsec_init:
> +#ifdef CONFIG_ARM_GIC_BASE_ADDRESS
> +	ldr	r2, =CONFIG_ARM_GIC_BASE_ADDRESS
> +#else
> +	mrc	p15, 4, r2, c15, c0, 0		@ read CBAR
> +#endif
> +	add	r3, r2, #GIC_DIST_OFFSET	@ GIC dist i/f offset
> +	mvn	r1, #0				@ all bits to 1
> +	str	r1, [r3, #GICD_IGROUPRn]	@ allow private interrupts
> +
> +	mrc	p15, 0, r0, c0, c0, 0		@ read MIDR
> +	bfc	r0, #20, #4			@ mask out variant
> +	bfc	r0, #0, #4			@ and revision
> +
> +	movw	r1, #lo(MIDR_CORTEX_A7_R0P0 & MIDR_PRIMARY_PART_MASK)

in the git repo branch you pointed me to in the cover e-mail, this refers to
MIDR_CORTEX_A6_R0P0 ?

Forgot to push the last revision?

> +	movt	r1, #hi(MIDR_CORTEX_A7_R0P0 & MIDR_PRIMARY_PART_MASK)
> +	cmp	r0, r1				@ check for Cortex-A7
> +
> +	orr	r1, #(MIDR_CORTEX_A15_R0P0 & 0xf0)
> +	cmpne	r0, r1				@ check for Cortex-A15
> +
> +	movne	r1, #GIC_CPU_OFFSET_A9		@ GIC CPU offset for A9
> +	moveq	r1, #GIC_CPU_OFFSET_A15		@ GIC CPU offset for A15/A7
> +	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
> +
> +	mov	r1, #1				@ set GICC_CTLR[enable]
> +	str	r1, [r3, #GICC_CTLR]		@ and clear all other bits
> +	mov	r1, #0xff
> +	str	r1, [r3, #GICC_PMR]		@ set priority mask register
> +
> +	movw	r1, #0x3fff
> +	movt	r1, #0x0006
> +	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
> +
> +	adr	r1, _secure_vectors
> +	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR to secure vectors
> +
> +	mrc	p15, 0, ip, c12, c0, 0		@ save secure copy of VBAR
> +
> +	isb
> +	smc	#0				@ call into MONITOR mode
> +
> +	mcr	p15, 0, ip, c12, c0, 0		@ write non-secure copy of VBAR
> +
> +	mov	r1, #1
> +	str	r1, [r3, #GICC_CTLR]		@ enable non-secure CPU i/f
> +	add	r2, r2, #GIC_DIST_OFFSET
> +	str	r1, [r2]			@ allow private interrupts

For those who don't remember which register is at offset zero, Could you
do:
	str	r1, [r2, #GICD_CTLR]

> +
> +	bx	lr
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index 20caa7c..989bb72 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -34,6 +34,17 @@
>  #define MIDR_CORTEX_A15_R0P0	0x410FC0F0
>  #define MIDR_CORTEX_A15_R2P2	0x412FC0F2
>  
> +/* Cortex-A7 revisions */
> +#define MIDR_CORTEX_A7_R0P0	0x410FC070
> +
> +#define MIDR_PRIMARY_PART_MASK	0xFF0FFFF0
> +
> +/* ID_PFR1 feature fields */
> +#define CPUID_ARM_VIRT_SHIFT	12
> +#define CPUID_ARM_VIRT_MASK	(0xF << CPUID_ARM_VIRT_SHIFT)
> +#define CPUID_ARM_TIMER_SHIFT	16
> +#define CPUID_ARM_TIMER_MASK	(0xF << CPUID_ARM_TIMER_SHIFT)
> +
>  /* CCSIDR */
>  #define CCSIDR_LINE_SIZE_OFFSET		0
>  #define CCSIDR_LINE_SIZE_MASK		0x7
> @@ -76,6 +87,11 @@ void v7_outer_cache_inval_all(void);
>  void v7_outer_cache_flush_range(u32 start, u32 end);
>  void v7_outer_cache_inval_range(u32 start, u32 end);
>  
> +#ifdef CONFIG_ARMV7_VIRT
> +/* defined in cpu/armv7/nonsec_virt.S */
> +void _nonsec_init(void);
> +#endif /* CONFIG_ARMV7_VIRT */
> +
>  #endif /* ! __ASSEMBLY__ */
>  
>  #endif
> diff --git a/arch/arm/include/asm/gic.h b/arch/arm/include/asm/gic.h
> new file mode 100644
> index 0000000..c2b1e28
> --- /dev/null
> +++ b/arch/arm/include/asm/gic.h
> @@ -0,0 +1,17 @@
> +#ifndef __GIC_V2_H__
> +#define __GIC_V2_H__
> +
> +/* register offsets for the ARM generic interrupt controller (GIC) */
> +
> +#define GIC_DIST_OFFSET		0x1000
> +#define GICD_CTLR		0x0000
> +#define GICD_TYPER		0x0004
> +#define GICD_IGROUPRn		0x0080
> +#define GICD_SGIR		0x0F00
> +
> +#define GIC_CPU_OFFSET_A9	0x0100
> +#define GIC_CPU_OFFSET_A15	0x2000
> +#define GICC_CTLR		0x0000
> +#define GICC_PMR		0x0004
> +
> +#endif
> -- 

Looks great otherwise.

I cannot build this with the Ubuntu Raring arm-linux-gnueabihf- cross
compiler without adding ".arch_extension sec" into this file.

Perhaps you need to play with a few compilers to be sure this builds
properly.  You may also want to look in arch/arm/kvm/Makefile in the
kernel to see how we use the as-instr to make sure the proper directives
are used in the source file or option given to the assembler.  You
should be able to easily port the as-instr into U-boot if needed (TI
does this in their U-boot for for example).

-Christoffer