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

Christoffer Dall
2013-06-19T22:27:17Z

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