diff mbox

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

Message ID 1371121273-18763-4-git-send-email-andre.przywara@linaro.org
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Commit Message

Andre Przywara June 13, 2013, 11:01 a.m. UTC 
While actually switching to non-secure state is one thing, the
more important part of this process is to make sure that we still
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:
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.

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.

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

Comments

Christoffer Dall June 19, 2013, 10:27 p.m. UTC | #1 
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
Masahiro Yamada June 28, 2013, 3:09 a.m. UTC | #2 
Hi, Andre


> +.globl _nonsec_init
> +_nonsec_init:

These two lines can be written:

ENTRY(_nonsec_init)



> +	mrc	p15, 0, r0, c0, c0, 0		@ read MIDR
> +	bfc	r0, #20, #4			@ mask out variant
> +	bfc	r0, #0, #4			@ and revision

Why do you hard code bit positions
even though MIDR_PRIMARY_PART_MASK is available?

> +	movw	r1, #lo(MIDR_CORTEX_A7_R0P0 & MIDR_PRIMARY_PART_MASK)
> +	movt	r1, #hi(MIDR_CORTEX_A7_R0P0 & MIDR_PRIMARY_PART_MASK)

Why don't you use "ldr   r*, =..." statement here?

> +	orr	r1, #(MIDR_CORTEX_A15_R0P0 & 0xf0)

This code relies on  the value of MIDR_CORTEX_A15_R0P0.



So, I think you can rewrite this part more simply as follows:


	mrc	p15, 0, r0, c0, c0, 0		@ read MIDR
	ldr	r1, =MIDR_PRIMARY_PART_MASK
	and     r0, r0, r1 			@ mask out variant and revision

	ldr	r1, =MIDR_CORTEX_A7_R0P0
	cmp	r0, r1				@ check for Cortex-A7

	ldr	r1, =MIDR_CORTEX_A15_R0P0
	cmpne	r0, r1				@ check for Cortex-A15




> +	bx	lr

Add ENDPROC(_nonsec_init) when beginning with ENTRY(_nonsec_init).


> +/* defined in cpu/armv7/nonsec_virt.S */
> +void _nonsec_init(void);

I think this comment is not necessary and
mantainancability is not excellent in case you renaming
nonsec_virt.S.


BTW, tag generation tool, GNU Global can help us
for searching definition.

If you begin routines in assembly with ENTRY(...),
GNU Global picks up these symbols for tag jump.



Masahiro Yamada
diff mbox

Patch

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