[U-Boot] [PATCH 0/6] ARMv7: Add HYP mode switching support

[U-Boot] [PATCH 0/6] ARMv7: Add HYP mode switching support

[Andre Przywara]

(for GIT URL and Changelog see below)

ARM CPUs with the virtualization extension have a new mode called
HYP mode, which allows hypervisors to safely control and monitor
guests. The current hypervisor (KVM and Xen) implementations
require the kernel to be entered in that HYP mode.

This patch series introduces a configuration variable
CONFIG_ARMV7_VIRT which enables code to switch all cores into HYP
mode. This is done automatically during execution of the bootm
command (but could also be done earlier - U-Boot runs fine in HYP
mode without MMU usage).

The process of switching into HYP mode requires the CPU to be in
non-secure state, which requires the GIC to be programmed properly
first. Explanations about the details are in the commit messages
of the respective patches.

The code aims to be as generic as possible, though currently it has
only been tested on the Versatile Express TC-2 board. The last patch
thus enables the feature for that board and relies on the Versatile
Express updates patches sent out lately[1].

For convenience there is now a GIT tree which you can pull these
and the Versatile Express patches from ("hypmode" branch):
git://git.linaro.org/people/aprzywara/u-boot.git

Changes from the RFC version I sent out before:
* not a dedicated command anymore, code run by bootm & friends
* protecting code with #ifdefs to avoid unnecessary inclusion and
  accidental crashing (when accessing restricted registers)
* moving prototypes to header file to meet checkpatch recommendation
* adding comment as proposed by Christoffer

Please review and comment!
Contributions and comments to support other boards are welcome.

Andre.

[1] http://lists.denx.de/pipermail/u-boot/2013-April/151366.html

Andre Przywara (6):
  ARM: add secure monitor handler to switch to non-secure state
  ARM: add assembly routine to switch to non-secure state
  ARM: switch to non-secure state during bootm execution
  ARM: add SMP support for non-secure switch
  ARM: extend non-secure switch to also go into HYP mode
  ARM: VExpress: enable ARMv7 virt support for VExpress A15

 arch/arm/cpu/armv7/start.S          | 129 +++++++++++++++++++++++++++++++++---
 arch/arm/include/asm/armv7.h        |   8 +++
 arch/arm/lib/Makefile               |   2 +
 arch/arm/lib/bootm.c                |  26 ++++++++
 arch/arm/lib/virt-v7.c              | 128 +++++++++++++++++++++++++++++++++++
 include/configs/vexpress_ca15_tc2.h |   3 +
 6 files changed, 286 insertions(+), 10 deletions(-)
 create mode 100644 arch/arm/lib/virt-v7.c

-- 
1.7.12.1

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Andre Przywara]

A prerequisite for using virtualization is to be in HYP mode, which
requires the CPU to be in non-secure state.
Introduce a monitor handler routine which switches the CPU to
non-secure state by setting the NS and associated bits.
According to the ARM ARM this should not be done in SVC mode, so we
have to setup a SMC handler for this. We reuse the current vector
table for this and make sure that we only access the MVBAR register
if the CPU supports the security extension and only if we
configured the board to use it, since boards entering u-boot already
in non-secure mode would crash on accessing MVBAR otherwise.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index e9e57e6..da48b36 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -155,6 +155,13 @@ reset:
 	/* Set vector address in CP15 VBAR register */
 	ldr	r0, =_start
 	mcr	p15, 0, r0, c12, c0, 0	@Set VBAR
+
+#ifdef CONFIG_ARMV7_VIRT
+	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
+	ands	r1, r1, #0x30
+	mcrne	p15, 0, r0, c12, c0, 1	@ Set secure monitor MVBAR
+#endif
+
 #endif
 
 	/* the mask ROM code should have PLL and others stable */
@@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup)
 	ldr     r0, =_start
 	mcr     p15, 0, r0, c12, c0, 0  @Set VBAR
 
+#ifdef CONFIG_ARMV7_VIRT
+	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
+	ands	r1, r1, #0x30
+	mcrne	p15, 0, r0, c12, c0, 1  @ Set secure monitor MVBAR
+#endif
+
 	bx	lr
 
 ENDPROC(c_runtime_cpu_setup)
@@ -490,11 +503,23 @@ undefined_instruction:
 	bad_save_user_regs
 	bl	do_undefined_instruction
 
+/*
+ * software interrupt aka. secure monitor handler
+ * This is executed on a "smc" instruction, we use a "smc #0" to switch
+ * to non-secure state
+ */
 	.align	5
 software_interrupt:
-	get_bad_stack_swi
-	bad_save_user_regs
-	bl	do_software_interrupt
+	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
+	bic	r1, r1, #0x07f
+	orr	r1, r1, #0x31			@ enable NS, AW, FW
+
+	mrc	p15, 0, r0, c12, c0, 0		@ save secure copy of VBAR
+	mcr	p15, 0, r1, c1, c1, 0		@ write SCR, switch to non-sec
+	isb
+	mcr	p15, 0, r0, c12, c0, 0		@ write non-secure copy of VBAR
+
+	movs	pc, lr
 
 	.align	5
 prefetch_abort:
-- 
1.7.12.1

[U-Boot] [PATCH 2/6] ARM: add assembly routine to switch to non-secure state

[Andre Przywara]

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 register. We check explicitly for
ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those
cores we know the offsets for the GIC CPU interface from the
PERIPHBASE content. Other cores could be added as needed.

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 = 0x80)

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.

For reasons obvious later we only use caller saved registers r0-r3.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/cpu/armv7/start.S | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index da48b36..e63e892 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -572,3 +572,50 @@ fiq:
 
 #endif /* CONFIG_USE_IRQ */
 #endif /* CONFIG_SPL_BUILD */
+
+#ifdef CONFIG_ARMV7_VIRT
+/* Routine to initialize GIC CPU interface and switch to nonsecure state.
+ */
+.globl _nonsec_gic_switch
+_nonsec_gic_switch:
+	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
+	add	r3, r2, #0x1000			@ GIC dist i/f offset
+	mvn	r1, #0
+	str	r1, [r3, #0x80]			@ allow private interrupts
+
+	mrc	p15, 0, r0, c0, c0, 0		@ MIDR
+	bfc	r0, #16, #8			@ mask out variant, arch
+	bfc	r0, #0, #4			@ and revision
+	movw	r1, #0xc070
+	movt	r1, #0x4100
+	cmp	r0, r1				@ check for Cortex-A7
+	orr	r1, #0xf0
+	cmpne	r0, r1				@ check for Cortex-A15
+	movne	r1, #0x100			@ GIC CPU offset for A9
+	moveq	r1, #0x2000			@ GIC CPU offset for A15/A7
+	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
+
+	mov	r1, #1
+	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
+	mov	r1, #0x80
+	str	r1, [r3, #4]			@ set GICC_PIMR[7]
+
+	movw	r1, #0x3fff
+	movt	r1, #0x0006
+	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
+
+	ldr	r1, =_start
+	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
+	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR
+
+	isb
+	smc	#0				@ call into MONITOR mode
+	isb					@ clobbers r0 and r1
+
+	mov	r1, #1
+	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
+	add	r2, r2, #0x1000			@ GIC dist i/f offset
+	str	r1, [r2]			@ allow private interrupts
+
+	mov	pc, lr
+#endif /* CONFIG_ARMV7_VIRT */
-- 
1.7.12.1

[U-Boot] [PATCH 3/6] ARM: switch to non-secure state during bootm execution

[Andre Przywara]

To actually trigger the non-secure switch we just implemented, call
the switching routine from within the bootm command implementation.
This way we automatically enable this feature without further user
intervention.

Some part of the work is done in the assembly routine in start.S,
introduced with the previous patch, but for the full glory we need
to setup the GIC distributor interface once for the whole system,
which is done in C here.
The routine is placed in arch/arm/lib to allow easy access from
different boards or CPUs.

First we check for the availability of the security extensions.

The generic timer base frequency register is only accessible from
secure state, so we have to program it now. Actually this should be
done from primary firmware before, but some boards seems to omit
this, so if needed we do this here with a board specific value.

Since we need a safe way to access the GIC, we use the PERIPHBASE
registers on Cortex-A15 and A7 CPUs and do some sanity checks.

Then we actually do the GIC enablement:
a) enable the GIC distributor, both for non-secure and secure state
   (GICD_CTLR[1:0] = 11b)
b) allow all interrupts to be handled from non-secure state
   (GICD_IGROUPRn = 0xFFFFFFFF)
The core specific GIC setup is then done in the assembly routine.

The actual bootm trigger is pretty small: calling the routine and
doing some error reporting. A return value of 1 will be added later.

To enable the whole code we introduce the CONFIG_ARMV7_VIRT variable.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/include/asm/armv7.h |   7 +++
 arch/arm/lib/Makefile        |   2 +
 arch/arm/lib/bootm.c         |  20 ++++++++
 arch/arm/lib/virt-v7.c       | 113 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 142 insertions(+)
 create mode 100644 arch/arm/lib/virt-v7.c

diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index a73630b..25afffe 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -74,4 +74,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
+int armv7_switch_nonsec(void);
+
+/* defined in cpu/armv7/start.S */
+void _nonsec_gic_switch(void);
+#endif /* CONFIG_ARMV7_VIRT */
+
 #endif
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 6ae161a..37a0e71 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -58,6 +58,8 @@ COBJS-y	+= reset.o
 COBJS-y	+= cache.o
 COBJS-y	+= cache-cp15.o
 
+COBJS-$(CONFIG_ARMV7_VIRT) += virt-v7.o
+
 SRCS	:= $(GLSOBJS:.o=.S) $(GLCOBJS:.o=.c) \
 	   $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index f3b30c5..a3d3aae 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -35,6 +35,10 @@
 #include <asm/bootm.h>
 #include <linux/compiler.h>
 
+#ifdef CONFIG_ARMV7_VIRT
+#include <asm/armv7.h>
+#endif
+
 DECLARE_GLOBAL_DATA_PTR;
 
 #if defined(CONFIG_SETUP_MEMORY_TAGS) || \
@@ -319,6 +323,22 @@ static void boot_prep_linux(bootm_headers_t *images)
 		hang();
 #endif /* all tags */
 	}
+#ifdef CONFIG_ARMV7_VIRT
+	switch (armv7_switch_nonsec()) {
+	case 0:
+		debug("entered non-secure state\n");
+		break;
+	case 2:
+		printf("HYP mode: Security extensions not implemented.\n");
+		break;
+	case 3:
+		printf("HYP mode: CPU not supported (must be Cortex-A15 or A7).\n");
+		break;
+	case 4:
+		printf("HYP mode: PERIPHBASE is above 4 GB, cannot access this.\n");
+		break;
+	}
+#endif
 }
 
 /* Subcommand: GO */
diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
new file mode 100644
index 0000000..3a48aee
--- /dev/null
+++ b/arch/arm/lib/virt-v7.c
@@ -0,0 +1,113 @@
+/*
+ * (C) Copyright 2013
+ * Andre Przywara, Linaro
+ *
+ * routines to push ARMv7 processors from secure into non-secure state
+ * needed to enable ARMv7 virtualization for current hypervisors
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <asm/armv7.h>
+
+#define GICD_CTLR	0x000
+#define GICD_TYPER	0x004
+#define GICD_IGROUPR0	0x080
+#define GICD_SGIR	0xf00
+
+#define CPU_ARM_CORTEX_A15	0x4100c0f0
+#define CPU_ARM_CORTEX_A7	0x4100c070
+
+static inline unsigned int read_cpsr(void)
+{
+	unsigned int reg;
+
+	asm volatile ("mrs %0, cpsr\n" : "=r" (reg));
+	return reg;
+}
+
+int armv7_switch_nonsec(void)
+{
+	unsigned int reg;
+	volatile unsigned int *gicdptr;
+	unsigned itlinesnr, i;
+
+	/* check whether the CPU supports the security extensions */
+	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
+	if ((reg & 0xF0) == 0)
+		return 2;
+
+	/* the timer frequency for the generic timer needs to be
+	 * programmed still in secure state, should be done by firmware.
+	 * check whether we have the generic timer first
+	 */
+#ifdef CONFIG_SYS_CLK_FREQ
+	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
+	if ((reg & 0xF0000) == 0x10000)
+		asm("mcr p15, 0, %0, c14, c0, 0\n"
+		: : "r"(CONFIG_SYS_CLK_FREQ));
+#endif
+
+	/* the SCR register will be set directly in the monitor mode handler,
+	 * according to the spec one should not tinker with it in secure state
+	 * in SVC mode. Do not try to read it once in non-secure state,
+	 * any access to it will trap.
+	 */
+
+	/* check whether we are an Cortex-A15 or A7.
+	 * The actual non-secure switch should work with all CPUs supporting
+	 * the security extension, but we need the GIC address,
+	 * which we know only for sure for those two CPUs.
+	 */
+	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(reg));
+	if (((reg & 0xFF00FFF0) != 0x4100C0F0) &&
+	    ((reg & 0xFF00FFF0) != 0x4100C070))
+		return 3;
+
+	/* get the GIC base address from the A15 PERIPHBASE register */
+	asm("mrc p15, 4, %0, c15, c0, 0\n" : "=r" (reg));
+
+	/* the PERIPHBASE can be mapped above 4 GB (lower 8 bits used to
+	 * encode this). Bail out here since we cannot access this without
+	 * enabling paging.
+	 */
+	if ((reg & 0xff) != 0)
+		return 4;
+
+	/* GIC distributor registers start at offset 0x1000 */
+	gicdptr = (unsigned *)(reg + 0x1000);
+
+	/* enable the GIC distributor */
+	gicdptr[GICD_CTLR / 4] |= 0x03;
+
+	/* TYPER[4:0] contains an encoded number of all interrupts */
+	itlinesnr = gicdptr[GICD_TYPER / 4] & 0x1f;
+
+	/* set all bits in the GIC group registers to one to allow access
+	 * from non-secure state
+	 */
+	for (i = 0; i <= itlinesnr; i++)
+		gicdptr[GICD_IGROUPR0 / 4 + i] = (unsigned)-1;
+
+	/* call the non-sec switching code on this CPU */
+	_nonsec_gic_switch();
+
+	return 0;
+}
-- 
1.7.12.1

[U-Boot] [PATCH 4/6] ARM: add SMP support for non-secure switch

[Andre Przywara]

Currently the non-secure switch is only done for the boot processor.
To later allow full SMP support, we have to switch all secondary
cores into non-secure state also.

So we add an entry point for secondary CPUs coming out of low-power
state and make sure we put them into WFI again after having switched
to non-secure state.
For this we acknowledge and EOI the wake-up IPI, then go into WFI.
Once being kicked out of it later, we sanity check that the start
address has actually been changed (since another attempt to switch
to non-secure would block the core) and jump to the new address.

The actual CPU kick is done by sending an inter-processor interrupt
via the GIC to all CPU interfaces except the requesting processor.
The secondary cores will then setup their respective GIC CPU
interface.

The address secondary cores jump to is board specific, we provide
the value here for the Versatile Express board.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/cpu/armv7/start.S          | 27 ++++++++++++++++++++++++++-
 arch/arm/include/asm/armv7.h        |  1 +
 arch/arm/lib/virt-v7.c              |  9 ++++++++-
 include/configs/vexpress_ca15_tc2.h |  1 +
 4 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index e63e892..02234c7 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -575,8 +575,19 @@ fiq:
 
 #ifdef CONFIG_ARMV7_VIRT
 /* Routine to initialize GIC CPU interface and switch to nonsecure state.
+ * Will be executed directly by secondary CPUs after coming out of
+ * WFI, or can be called directly by C code for CPU 0.
+ * Those two paths mandate to not use any stack and to only use registers
+ * r0-r3 to comply with both the C ABI and the requirement of SMP startup
+ * code.
  */
 .globl _nonsec_gic_switch
+.globl _smp_pen
+_smp_pen:
+	mrs	r0, cpsr
+	orr	r0, r0, #0xc0
+	msr	cpsr, r0			@ disable interrupts
+	mov	lr, #0				@ clear LR to mark secondary
 _nonsec_gic_switch:
 	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
 	add	r3, r2, #0x1000			@ GIC dist i/f offset
@@ -617,5 +628,19 @@ _nonsec_gic_switch:
 	add	r2, r2, #0x1000			@ GIC dist i/f offset
 	str	r1, [r2]			@ allow private interrupts
 
-	mov	pc, lr
+	cmp	lr, #0
+	movne	pc, lr				@ CPU 0 to return
+						@ all others: go to sleep
+_ack_int:
+	ldr	r1, [r3, #0x0c]			@ read GICD acknowledge
+	str	r1, [r3, #0x10]			@ write GICD EOI
+
+	adr	r1, _smp_pen
+waitloop:
+	wfi
+	ldr	r0, =CONFIG_SYSFLAGS_ADDR	@ load start address
+	ldr	r0, [r0]
+	cmp	r0, r1			@ make sure we dont execute this code
+	beq	waitloop		@ again (due to a spurious wakeup)
+	mov	pc, r0
 #endif /* CONFIG_ARMV7_VIRT */
diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index 25afffe..296dc92 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -78,6 +78,7 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
 int armv7_switch_nonsec(void);
 
 /* defined in cpu/armv7/start.S */
+void _smp_pen(void);
 void _nonsec_gic_switch(void);
 #endif /* CONFIG_ARMV7_VIRT */
 
diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
index 3a48aee..0248010 100644
--- a/arch/arm/lib/virt-v7.c
+++ b/arch/arm/lib/virt-v7.c
@@ -48,6 +48,7 @@ int armv7_switch_nonsec(void)
 	unsigned int reg;
 	volatile unsigned int *gicdptr;
 	unsigned itlinesnr, i;
+	unsigned int *sysflags;
 
 	/* check whether the CPU supports the security extensions */
 	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
@@ -106,7 +107,13 @@ int armv7_switch_nonsec(void)
 	for (i = 0; i <= itlinesnr; i++)
 		gicdptr[GICD_IGROUPR0 / 4 + i] = (unsigned)-1;
 
-	/* call the non-sec switching code on this CPU */
+	/* now kick all CPUs (expect this one) by writing to GICD_SIGR */
+	sysflags = (void *)CONFIG_SYSFLAGS_ADDR;
+	sysflags[1] = (unsigned)-1;
+	sysflags[0] = (uintptr_t)_smp_pen;
+	gicdptr[GICD_SGIR / 4] = 1U << 24;
+
+	/* call the non-sec switching code on this CPU also */
 	_nonsec_gic_switch();
 
 	return 0;
diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
index 9e230ad..210a27c 100644
--- a/include/configs/vexpress_ca15_tc2.h
+++ b/include/configs/vexpress_ca15_tc2.h
@@ -32,5 +32,6 @@
 #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
 
 #define CONFIG_SYS_CLK_FREQ 24000000
+#define CONFIG_SYSFLAGS_ADDR 0x1c010030
 
 #endif
-- 
1.7.12.1

[U-Boot] [PATCH 5/6] ARM: extend non-secure switch to also go into HYP mode

[Andre Przywara]

For the KVM and XEN hypervisors to be usable, we need to enter the
kernel in HYP mode. Now that we already are in non-secure state,
HYP mode switching is within short reach.

While doing the non-secure switch, we have to enable the HVC
instruction and setup the HYP mode HVBAR (while still secure).

The actual switch is done by dropping back from a HYP mode handler
without actually leaving HYP mode, so we introduce a new handler
routine in the exception vector table.

In the assembly switching routine - which we rename to hyp_gic_switch
on the way - we save and restore the banked LR and SP registers
around the hypercall to do the actual HYP mode switch.

The C routine first checks whether we are in HYP mode already and
also whether the virtualization extensions are available. It also
checks whether the HYP mode switch was finally successful.
The bootm command part only adds and adjusts some error reporting.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/cpu/armv7/start.S   | 34 +++++++++++++++++++++++-----------
 arch/arm/include/asm/armv7.h |  4 ++--
 arch/arm/lib/bootm.c         | 12 +++++++++---
 arch/arm/lib/virt-v7.c       | 22 +++++++++++++++-------
 4 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 02234c7..921e9d9 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -41,7 +41,7 @@ _start: b	reset
 	ldr	pc, _software_interrupt
 	ldr	pc, _prefetch_abort
 	ldr	pc, _data_abort
-	ldr	pc, _not_used
+	ldr	pc, _hyp_trap
 	ldr	pc, _irq
 	ldr	pc, _fiq
 #ifdef CONFIG_SPL_BUILD
@@ -49,7 +49,7 @@ _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
+_hyp_trap:		.word _hyp_trap
 _irq:			.word _irq
 _fiq:			.word _fiq
 _pad:			.word 0x12345678 /* now 16*4=64 */
@@ -58,7 +58,7 @@ _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
+_hyp_trap:		.word hyp_trap
 _irq:			.word irq
 _fiq:			.word fiq
 _pad:			.word 0x12345678 /* now 16*4=64 */
@@ -513,12 +513,18 @@ software_interrupt:
 	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
 	bic	r1, r1, #0x07f
 	orr	r1, r1, #0x31			@ enable NS, AW, FW
+	mrc	p15, 0, r0, c0, c1, 1		@ check for Virt ext
+	and	r0, r0, #0xf000
+	cmp	r0, #0x1000
+	orreq	r1, r1, #0x100			@ allow HVC instruction
 
 	mrc	p15, 0, r0, c12, c0, 0		@ save secure copy of VBAR
 	mcr	p15, 0, r1, c1, c1, 0		@ write SCR, switch to non-sec
 	isb
 	mcr	p15, 0, r0, c12, c0, 0		@ write non-secure copy of VBAR
 
+	mcreq	p15, 4, r0, c12, c0, 0		@ write HYP mode HVBAR
+
 	movs	pc, lr
 
 	.align	5
@@ -534,10 +540,9 @@ data_abort:
 	bl	do_data_abort
 
 	.align	5
-not_used:
-	get_bad_stack
-	bad_save_user_regs
-	bl	do_not_used
+hyp_trap:
+	.byte 0x00, 0xe3, 0x0e, 0xe1		@ mrs lr, elr_hyp
+	mov pc, lr
 
 #ifdef CONFIG_USE_IRQ
 
@@ -574,21 +579,21 @@ fiq:
 #endif /* CONFIG_SPL_BUILD */
 
 #ifdef CONFIG_ARMV7_VIRT
-/* Routine to initialize GIC CPU interface and switch to nonsecure state.
- * Will be executed directly by secondary CPUs after coming out of
+/* Routine to initialize GIC CPU interface, switch to nonsecure and to HYP
+ * mode. Will be executed directly by secondary CPUs after coming out of
  * WFI, or can be called directly by C code for CPU 0.
  * Those two paths mandate to not use any stack and to only use registers
  * r0-r3 to comply with both the C ABI and the requirement of SMP startup
  * code.
  */
-.globl _nonsec_gic_switch
+.globl _hyp_gic_switch
 .globl _smp_pen
 _smp_pen:
 	mrs	r0, cpsr
 	orr	r0, r0, #0xc0
 	msr	cpsr, r0			@ disable interrupts
 	mov	lr, #0				@ clear LR to mark secondary
-_nonsec_gic_switch:
+_hyp_gic_switch:
 	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
 	add	r3, r2, #0x1000			@ GIC dist i/f offset
 	mvn	r1, #0
@@ -628,6 +633,13 @@ _nonsec_gic_switch:
 	add	r2, r2, #0x1000			@ GIC dist i/f offset
 	str	r1, [r2]			@ allow private interrupts
 
+	mov	r2, lr
+	mov	r1, sp
+	.byte 0x70, 0x00, 0x40, 0xe1		@ hvc #0
+	isb
+	mov	sp, r1
+	mov	lr, r2
+
 	cmp	lr, #0
 	movne	pc, lr				@ CPU 0 to return
 						@ all others: go to sleep
diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index 296dc92..17bb497 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -75,11 +75,11 @@ void v7_outer_cache_flush_range(u32 start, u32 end);
 void v7_outer_cache_inval_range(u32 start, u32 end);
 
 #ifdef CONFIG_ARMV7_VIRT
-int armv7_switch_nonsec(void);
+int armv7_switch_hyp(void);
 
 /* defined in cpu/armv7/start.S */
 void _smp_pen(void);
-void _nonsec_gic_switch(void);
+void _hyp_gic_switch(void);
 #endif /* CONFIG_ARMV7_VIRT */
 
 #endif
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index a3d3aae..552ba59 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -324,12 +324,15 @@ static void boot_prep_linux(bootm_headers_t *images)
 #endif /* all tags */
 	}
 #ifdef CONFIG_ARMV7_VIRT
-	switch (armv7_switch_nonsec()) {
+	switch (armv7_switch_hyp()) {
 	case 0:
-		debug("entered non-secure state\n");
+		debug("entered HYP mode\n");
+		break;
+	case 1:
+		debug("CPU already in HYP mode\n");
 		break;
 	case 2:
-		printf("HYP mode: Security extensions not implemented.\n");
+		printf("HYP mode: Virtualization extensions not implemented.\n");
 		break;
 	case 3:
 		printf("HYP mode: CPU not supported (must be Cortex-A15 or A7).\n");
@@ -337,6 +340,9 @@ static void boot_prep_linux(bootm_headers_t *images)
 	case 4:
 		printf("HYP mode: PERIPHBASE is above 4 GB, cannot access this.\n");
 		break;
+	case 5:
+		printf("HYP mode: switch not successful.\n");
+		break;
 	}
 #endif
 }
diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
index 0248010..3883463 100644
--- a/arch/arm/lib/virt-v7.c
+++ b/arch/arm/lib/virt-v7.c
@@ -3,6 +3,7 @@
  * Andre Przywara, Linaro
  *
  * routines to push ARMv7 processors from secure into non-secure state
+ * and from non-secure SVC into HYP mode
  * needed to enable ARMv7 virtualization for current hypervisors
  *
  * See file CREDITS for list of people who contributed to this
@@ -43,16 +44,20 @@ static inline unsigned int read_cpsr(void)
 	return reg;
 }
 
-int armv7_switch_nonsec(void)
+int armv7_switch_hyp(void)
 {
 	unsigned int reg;
 	volatile unsigned int *gicdptr;
 	unsigned itlinesnr, i;
 	unsigned int *sysflags;
 
-	/* check whether the CPU supports the security extensions */
+	/* check whether we are in HYP mode already */
+	if ((read_cpsr() & 0x1F) == 0x1a)
+		return 1;
+
+	/* check whether the CPU supports the virtualization extensions */
 	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
-	if ((reg & 0xF0) == 0)
+	if ((reg & 0xF000) != 0x1000)
 		return 2;
 
 	/* the timer frequency for the generic timer needs to be
@@ -73,8 +78,8 @@ int armv7_switch_nonsec(void)
 	 */
 
 	/* check whether we are an Cortex-A15 or A7.
-	 * The actual non-secure switch should work with all CPUs supporting
-	 * the security extension, but we need the GIC address,
+	 * The actual HYP switch should work with all CPUs supporting
+	 * the virtualization extension, but we need the GIC address,
 	 * which we know only for sure for those two CPUs.
 	 */
 	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(reg));
@@ -113,8 +118,11 @@ int armv7_switch_nonsec(void)
 	sysflags[0] = (uintptr_t)_smp_pen;
 	gicdptr[GICD_SGIR / 4] = 1U << 24;
 
-	/* call the non-sec switching code on this CPU also */
-	_nonsec_gic_switch();
+	/* call the HYP switching code on this CPU also */
+	_hyp_gic_switch();
+
+	if ((read_cpsr() & 0x1F) != 0x1a)
+		return 5;
 
 	return 0;
 }
-- 
1.7.12.1

[U-Boot] [PATCH 6/6] ARM: VExpress: enable ARMv7 virt support for VExpress A15

[Andre Przywara]

Switch to HYP mode during bootm execution for the Versatile Express
TC2 board with the virtualization capable Cortex-A15 CPU to allow
booting Xen or a KVM-enabled Linux kernel in HYP mode.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 include/configs/vexpress_ca15_tc2.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
index 210a27c..4ac8009 100644
--- a/include/configs/vexpress_ca15_tc2.h
+++ b/include/configs/vexpress_ca15_tc2.h
@@ -31,6 +31,8 @@
 #include "vexpress_common.h"
 #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
 
+#define CONFIG_ARMV7_VIRT
+
 #define CONFIG_SYS_CLK_FREQ 24000000
 #define CONFIG_SYSFLAGS_ADDR 0x1c010030
 
-- 
1.7.12.1

[U-Boot] [PATCH 5/6] ARM: extend non-secure switch to also go into HYP mode

[Tom Rini]

On Mon, May 06, 2013 at 03:17:49PM +0200, Andre Przywara wrote:

[snip]
> -		printf("HYP mode: Security extensions not implemented.\n");
> +		printf("HYP mode: Virtualization extensions not implemented.\n");

When we don't need printf-modifiers, using puts is preferred.

And I leave review of the implementation to others that know the area,
but nothing jumps out at me.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130509/e7de5d2f/attachment.pgp>

[U-Boot] [PATCH 0/6] ARMv7: Add HYP mode switching support

[Albert ARIBAUD]

Hi Andre,

On Mon,  6 May 2013 15:17:44 +0200, Andre Przywara
<andre.przywara@linaro.org> wrote:

> (for GIT URL and Changelog see below)
> 
> ARM CPUs with the virtualization extension have a new mode called
> HYP mode, which allows hypervisors to safely control and monitor
> guests. The current hypervisor (KVM and Xen) implementations
> require the kernel to be entered in that HYP mode.
> 
> This patch series introduces a configuration variable
> CONFIG_ARMV7_VIRT which enables code to switch all cores into HYP
> mode. This is done automatically during execution of the bootm
> command (but could also be done earlier - U-Boot runs fine in HYP
> mode without MMU usage).
> 
> The process of switching into HYP mode requires the CPU to be in
> non-secure state, which requires the GIC to be programmed properly
> first. Explanations about the details are in the commit messages
> of the respective patches.
> 
> The code aims to be as generic as possible, though currently it has
> only been tested on the Versatile Express TC-2 board. The last patch
> thus enables the feature for that board and relies on the Versatile
> Express updates patches sent out lately[1].
> 
> For convenience there is now a GIT tree which you can pull these
> and the Versatile Express patches from ("hypmode" branch):
> git://git.linaro.org/people/aprzywara/u-boot.git
> 
> Changes from the RFC version I sent out before:
> * not a dedicated command anymore, code run by bootm & friends
> * protecting code with #ifdefs to avoid unnecessary inclusion and
>   accidental crashing (when accessing restricted registers)
> * moving prototypes to header file to meet checkpatch recommendation
> * adding comment as proposed by Christoffer
> 
> Please review and comment!
> Contributions and comments to support other boards are welcome.

I know... virtually... nothing in virtualization, so please excuse any
silly questions below:

- what happens if a target with these patches actually starts with HYP
  already enabled by some secure ROM boot code?

- I thought that once HYP is enabled, then all secure functionalities
  are performed by some monitor code invoked through a sw exception. Is
  the swi handler this monitor code? If it is, then is this monitor
  crippled, and what will happen if the bootm code needs to use some
  secure functionality?

> Andre.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Albert ARIBAUD]

Hi Andre,

On Mon,  6 May 2013 15:17:45 +0200, Andre Przywara
<andre.przywara@linaro.org> wrote:

> A prerequisite for using virtualization is to be in HYP mode, which
> requires the CPU to be in non-secure state.
> Introduce a monitor handler routine which switches the CPU to
> non-secure state by setting the NS and associated bits.
> According to the ARM ARM this should not be done in SVC mode, so we

ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is
a more precise reference, please provide it.

> have to setup a SMC handler for this. We reuse the current vector
> table for this and make sure that we only access the MVBAR register
> if the CPU supports the security extension and only if we
> configured the board to use it, since boards entering u-boot already
> in non-secure mode would crash on accessing MVBAR otherwise.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index e9e57e6..da48b36 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -155,6 +155,13 @@ reset:
>  	/* Set vector address in CP15 VBAR register */
>  	ldr	r0, =_start
>  	mcr	p15, 0, r0, c12, c0, 0	@Set VBAR
> +
> +#ifdef CONFIG_ARMV7_VIRT
> +	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
> +	ands	r1, r1, #0x30
> +	mcrne	p15, 0, r0, c12, c0, 1	@ Set secure monitor MVBAR
> +#endif
> +
>  #endif
>  
>  	/* the mask ROM code should have PLL and others stable */
> @@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup)
>  	ldr     r0, =_start
>  	mcr     p15, 0, r0, c12, c0, 0  @Set VBAR
>  
> +#ifdef CONFIG_ARMV7_VIRT
> +	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
> +	ands	r1, r1, #0x30
> +	mcrne	p15, 0, r0, c12, c0, 1  @ Set secure monitor MVBAR
> +#endif
> +
>  	bx	lr
>  
>  ENDPROC(c_runtime_cpu_setup)
> @@ -490,11 +503,23 @@ undefined_instruction:
>  	bad_save_user_regs
>  	bl	do_undefined_instruction
>  
> +/*
> + * software interrupt aka. secure monitor handler
> + * This is executed on a "smc" instruction, we use a "smc #0" to switch
> + * to non-secure state
> + */
>  	.align	5
>  software_interrupt:
> -	get_bad_stack_swi
> -	bad_save_user_regs
> -	bl	do_software_interrupt
> +	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
> +	bic	r1, r1, #0x07f
> +	orr	r1, r1, #0x31			@ enable NS, AW, FW
> +
> +	mrc	p15, 0, r0, c12, c0, 0		@ save secure copy of VBAR
> +	mcr	p15, 0, r1, c1, c1, 0		@ write SCR, switch to non-sec
> +	isb
> +	mcr	p15, 0, r0, c12, c0, 0		@ write non-secure copy of VBAR
> +
> +	movs	pc, lr
>  
>  	.align	5
>  prefetch_abort:


Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Marc Zyngier]

On 23/05/13 11:52, Albert ARIBAUD wrote:
> Hi Andre,
> 
> On Mon,  6 May 2013 15:17:45 +0200, Andre Przywara
> <andre.przywara@linaro.org> wrote:
> 
>> A prerequisite for using virtualization is to be in HYP mode, which
>> requires the CPU to be in non-secure state.
>> Introduce a monitor handler routine which switches the CPU to
>> non-secure state by setting the NS and associated bits.
>> According to the ARM ARM this should not be done in SVC mode, so we
> 
> ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is
> a more precise reference, please provide it.

I believe the ARM ARM (as in ARM Architecture Reference Manual) is the
correct document here.

http://infocenter.arm.com/help/topic/com.arm.doc.ddi0406c/index.html

Cheers,

	M.
-- 
Jazz is not dead. It just smells funny...

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Albert ARIBAUD]

Hi Marc,

On Thu, 23 May 2013 13:14:01 +0100, Marc Zyngier <marc.zyngier@arm.com>
wrote:

> On 23/05/13 11:52, Albert ARIBAUD wrote:
> > Hi Andre,
> > 
> > On Mon,  6 May 2013 15:17:45 +0200, Andre Przywara
> > <andre.przywara@linaro.org> wrote:
> > 
> >> A prerequisite for using virtualization is to be in HYP mode, which
> >> requires the CPU to be in non-secure state.
> >> Introduce a monitor handler routine which switches the CPU to
> >> non-secure state by setting the NS and associated bits.
> >> According to the ARM ARM this should not be done in SVC mode, so we
> > 
> > ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is
> > a more precise reference, please provide it.
> 
> I believe the ARM ARM (as in ARM Architecture Reference Manual) is the
> correct document here.
> 
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0406c/index.html

Well -- if you form the acronym, you do end with 'A R M' indeed, but
this is quite unfortunate, as 'ARM ARM' is redundant (as the acronym's
A already has ARM in it), confusion (as 'ARM' already bears a quite
established meaning) and ambiguous (as there are actually several
documents with the title of 'ARM<vXXX> Reference Manual' and which
would end up with the same acronym).

So if you don't want to use 'TRM' (which I can understand), then
at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'.
Stating the DDI* reference is not a must, unless you want to specify
a given revision (but then I suggest adding it after 'Manual' too).

> Cheers,
> 
> 	M.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Albert ARIBAUD]

On Thu, 23 May 2013 14:34:38 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Hi Marc,

> So if you don't want to use 'TRM' (which I can understand), then
> at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'.
> Stating the DDI* reference is not a must, unless you want to specify
> a given revision (but then I suggest adding it after 'Manual' too).

My bad: this last paragraph was actually directed at Andrew, not Marc.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Albert ARIBAUD]

On Thu, 23 May 2013 14:40:09 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> My bad: this last paragraph was actually directed at Andrew

Make that Andre.

Apologies,
-- 
Albert.

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Peter Maydell]

On 23 May 2013 13:34, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> Well -- if you form the acronym, you do end with 'A R M' indeed, but
> this is quite unfortunate, as 'ARM ARM' is redundant (as the acronym's
> A already has ARM in it), confusion (as 'ARM' already bears a quite
> established meaning) and ambiguous (as there are actually several
> documents with the title of 'ARM<vXXX> Reference Manual' and which
> would end up with the same acronym).

"ARM ARM" is the standard abbreviated way of referring to the
ARM Architecture Reference Manual (and as you can see it's
not a redundant acronym, since the A doesn't stand for ARM).
A TRM (Technical Reference Manual) is a completely different
document type, describing a specific processor. Andre is correct
that the restriction in question is architectural (and thus
described in the ARM ARM), not implementation specific (which
would be what you'd find in a TRM).

> So if you don't want to use 'TRM' (which I can understand), then
> at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'.
> Stating the DDI* reference is not a must, unless you want to specify
> a given revision (but then I suggest adding it after 'Manual' too).

"ARMv7-AR Reference Manual" is confusing, because you've dropped
the "Architecture" bit.

Since this is only a git comment, I'd suggest "ARM architecture
reference manual" as both clear for non-ARM people and sufficiently
unambiguous, or "ARMv7-AR Architecture Reference Manual" if you
want to be a bit more formal about it.

thanks
-- PMM

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Albert ARIBAUD]

Hi Peter,

(sorry for the duplicate; first reply sent was missing recipients, and
I had a fix to do anyway)

On Thu, 23 May 2013 14:00:17 +0100, Peter Maydell
<peter.maydell@linaro.org> wrote:

> On 23 May 2013 13:34, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> > Well -- if you form the acronym, you do end with 'A R M' indeed, but
> > this is quite unfortunate, as 'ARM ARM' is redundant (as the acronym's
> > A already has ARM in it), confusion (as 'ARM' already bears a quite
> > established meaning) and ambiguous (as there are actually several
> > documents with the title of 'ARM<vXXX> Reference Manual' and which
> > would end up with the same acronym).
> 
> "ARM ARM" is the standard abbreviated way of referring to the
> ARM Architecture Reference Manual (and as you can see it's
> not a redundant acronym, since the A doesn't stand for ARM).

Before answering about the double 'ARM', I did google for possible
meanings of the acronym 'ARM' and did not find any indication of it
meaning 'Architecture Reference Manual' except on www.all-acronyms.com,
which is completely foreign to the embedded world and where the
definition is not backed by any substantial source.

OTOH, the ARM Information Center does not use the acronym ARM to mean
'Architecture Reference Manual', nor does the Glossary section of ARM
documents I've read so far - including "the" ARM Glossary (AEG0014E),
which lists quite a lot of 'ARM something' but no 'ARM' alone, except
in the preamble phrase: 'Where the term ARM is used it means ?ARM
or any of its subsidiaries as appropriate?'.

Of course, I might have missed it, so any actual pointer to the
definition is heartily welcome.

> A TRM (Technical Reference Manual) is a completely different
> document type, describing a specific processor. Andre is correct
> that the restriction in question is architectural (and thus
> described in the ARM ARM), not implementation specific (which
> would be what you'd find in a TRM).

You are correct that here the document is not a TRM.

> > So if you don't want to use 'TRM' (which I can understand), then
> > at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'.
> > Stating the DDI* reference is not a must, unless you want to specify
> > a given revision (but then I suggest adding it after 'Manual' too).
> 
> "ARMv7-AR Reference Manual" is confusing, because you've dropped
> the "Architecture" bit.

That drop was involuntary.

> Since this is only a git comment, I'd suggest "ARM architecture
> reference manual" as both clear for non-ARM people and sufficiently
> unambiguous, or "ARMv7-AR Architecture Reference Manual" if you
> want to be a bit more formal about it.

Since a git comment is there for a reason, which includes helping its
readers understand the commit, I consider "ARMv7-AR Reference Manual"
to help them much more than 'ARM ARM', as it points them unambiguously
to the document by stating the exact title under which it is listed in
the ARM information center, but I am ok with 'ARMv7-AR Architecture
Reference Manual' as this how its title goes.

> thanks
> -- PMM

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Albert ARIBAUD]

On Thu, 23 May 2013 16:08:35 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Since a git comment is there for a reason, which includes helping its
> readers understand the commit, I consider "ARMv7-AR Reference Manual"
> to help them much more than 'ARM ARM', as it points them unambiguously
> to the document by stating the exact title under which it is listed in
> the ARM information center, but I am ok with 'ARMv7-AR Architecture
> Reference Manual' as this how its title goes.

Just to clarify: yes, the document is listed (on the left, in the docs
tree as "ARMv7-AR Reference Manual", and yes, its title is 'ARMv7-AR
Architecture Reference Manual'. That is not a mistake apparently, or it
was a duly repeated one for all ARM architecture reference manuals on
the Information Center.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Andre Przywara]

On 05/23/2013 12:52 PM, Albert ARIBAUD wrote:
> Hi Andre,
>
> On Mon,  6 May 2013 15:17:45 +0200, Andre Przywara
> <andre.przywara@linaro.org> wrote:
>
>> A prerequisite for using virtualization is to be in HYP mode, which
>> requires the CPU to be in non-secure state.
>> Introduce a monitor handler routine which switches the CPU to
>> non-secure state by setting the NS and associated bits.
>> According to the ARM ARM this should not be done in SVC mode, so we
>
> ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is
> a more precise reference, please provide it.

Albert,

my apologies for the confusion. As Peter already pointed out, the 
reference is really in the architectural manual. I just picked up that 
"ARM ARM" phrase lately and assumed that this is common knowledge. I 
will change it to something more precise in the next revision.

Thanks,
Andre.

[U-Boot] [PATCH 0/6] ARMv7: Add HYP mode switching support

[Andre Przywara]

On 05/23/2013 12:52 PM, Albert ARIBAUD wrote:
> Hi Andre,
>
> On Mon,  6 May 2013 15:17:44 +0200, Andre Przywara
> <andre.przywara@linaro.org> wrote:
>
>> (for GIT URL and Changelog see below)
>>
>> ARM CPUs with the virtualization extension have a new mode called
>> HYP mode, which allows hypervisors to safely control and monitor
>> guests. The current hypervisor (KVM and Xen) implementations
>> require the kernel to be entered in that HYP mode.
>>
>> This patch series introduces a configuration variable
>> CONFIG_ARMV7_VIRT which enables code to switch all cores into HYP
>> mode. This is done automatically during execution of the bootm
>> command (but could also be done earlier - U-Boot runs fine in HYP
>> mode without MMU usage).
>>
>> The process of switching into HYP mode requires the CPU to be in
>> non-secure state, which requires the GIC to be programmed properly
>> first. Explanations about the details are in the commit messages
>> of the respective patches.
>>
>> The code aims to be as generic as possible, though currently it has
>> only been tested on the Versatile Express TC-2 board. The last patch
>> thus enables the feature for that board and relies on the Versatile
>> Express updates patches sent out lately[1].
>>
>> For convenience there is now a GIT tree which you can pull these
>> and the Versatile Express patches from ("hypmode" branch):
>> git://git.linaro.org/people/aprzywara/u-boot.git
>>
>> Changes from the RFC version I sent out before:
>> * not a dedicated command anymore, code run by bootm & friends
>> * protecting code with #ifdefs to avoid unnecessary inclusion and
>>    accidental crashing (when accessing restricted registers)
>> * moving prototypes to header file to meet checkpatch recommendation
>> * adding comment as proposed by Christoffer
>>
>> Please review and comment!
>> Contributions and comments to support other boards are welcome.
>
> I know... virtually... nothing in virtualization, so please excuse any
> silly questions below:

Not silly at all, keep asking!

> - what happens if a target with these patches actually starts with HYP
>    already enabled by some secure ROM boot code?

If HYP mode is already enabled, then armv7_switch_hyp() returns early 
(the 3rd last hunk in patch 5), so that actually no code from that 
series is ever executed.
But thinking about it again I see that I write MVBAR before the check, I 
think it is not necessary to do it that early. Will check this again and 
send out a fixed version.

Also the idea was that the CONFIG_ option is only set on boards for 
which one knows that they enter u-boot in secure state. If ROM code 
already enabled HYP, then this code is not necessary and doesn't need to 
be configured.
Originally I wanted to have truly universal code, but since we need to 
configure u-boot for a specific board anyway and also there is no easy 
way to determine whether we already are non-secure I gave up on this for 
the time being. Not sure if using something like catching the UNDEF 
exception and returning to the code with an error condition (like 
wrmsr_safe in Linux/x86) is worth being added.

> - I thought that once HYP is enabled, then all secure functionalities
>    are performed by some monitor code invoked through a sw exception. Is
>    the swi handler this monitor code? If it is, then is this monitor
>    crippled, and what will happen if the bootm code needs to use some
>    secure functionality?

If board firmware offers mandatory secure functionalities (like the PSCI 
interface), then this firmware should better go to HYP itself, otherwise 
there is no way to do this without being secure first (we need to set 
HVBAR, this only works from monitor or HYP mode). Another option would 
be to use a secure service routine offered by the secure ROM code to set 
this HVBAR register. This would be board specific, I guess. If there is 
such a board, I'd be happy to hear about it and add support for it.

Regards,
Andre.

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Christoffer Dall]

On Mon, May 06, 2013 at 03:17:45PM +0200, Andre Przywara wrote:
> A prerequisite for using virtualization is to be in HYP mode, which
> requires the CPU to be in non-secure state.
> Introduce a monitor handler routine which switches the CPU to
> non-secure state by setting the NS and associated bits.
> According to the ARM ARM this should not be done in SVC mode, so we
> have to setup a SMC handler for this. We reuse the current vector
> table for this and make sure that we only access the MVBAR register
> if the CPU supports the security extension and only if we
> configured the board to use it, since boards entering u-boot already
> in non-secure mode would crash on accessing MVBAR otherwise.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index e9e57e6..da48b36 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -155,6 +155,13 @@ reset:
>  	/* Set vector address in CP15 VBAR register */
>  	ldr	r0, =_start
>  	mcr	p15, 0, r0, c12, c0, 0	@Set VBAR
> +
> +#ifdef CONFIG_ARMV7_VIRT
> +	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
> +	ands	r1, r1, #0x30
> +	mcrne	p15, 0, r0, c12, c0, 1	@ Set secure monitor MVBAR

Hmm, this smells a bit simplified to me.

Support for ARMv7_VIRT should easy to integrate into u-boot even for
platforms that do not boot U-boot directly into secure mode (OMAP5 GP
platforms are such an example).  In this case you cannot assume that you
can write the secure monitor mvbar.

> +#endif
> +
>  #endif
>  
>  	/* the mask ROM code should have PLL and others stable */
> @@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup)
>  	ldr     r0, =_start
>  	mcr     p15, 0, r0, c12, c0, 0  @Set VBAR
>  
> +#ifdef CONFIG_ARMV7_VIRT
> +	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
> +	ands	r1, r1, #0x30
> +	mcrne	p15, 0, r0, c12, c0, 1  @ Set secure monitor MVBAR
> +#endif
> +
>  	bx	lr
>  
>  ENDPROC(c_runtime_cpu_setup)
> @@ -490,11 +503,23 @@ undefined_instruction:
>  	bad_save_user_regs
>  	bl	do_undefined_instruction
>  
> +/*
> + * software interrupt aka. secure monitor handler
> + * This is executed on a "smc" instruction, we use a "smc #0" to switch
> + * to non-secure state
> + */
>  	.align	5
>  software_interrupt:
> -	get_bad_stack_swi
> -	bad_save_user_regs
> -	bl	do_software_interrupt

Why is the following block not conditional on CONFIG_ARMV7_VIRT?

Again, it feels a bit funny to modify this generic mechanism to contain
this code for boards that boot in NS mode but have a way to enter Hyp
mode using an HVC or SMC instruction.

> +	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
> +	bic	r1, r1, #0x07f
> +	orr	r1, r1, #0x31			@ enable NS, AW, FW

Are you sure you want to always route FIQ to non-secure here?

Don't you need to set the HCE bit?  The whole register resets to
0register resets to zero.

> +
> +	mrc	p15, 0, r0, c12, c0, 0		@ save secure copy of VBAR
> +	mcr	p15, 0, r1, c1, c1, 0		@ write SCR, switch to non-sec

Not quite a "swith to non-sec"; you're still in monitor mode.

> +	isb
> +	mcr	p15, 0, r0, c12, c0, 0		@ write non-secure copy of VBAR

I don't actually think that you are, I think you're writing the secure
copy here.

In that case, I'm also wondering if the isb is superflous, because we
perform an exception return below, but we of course want to make damn
sure that the write of the NS bit is set before the exception return,
maybe some ARM guys have the right expertise here.

> +
> +	movs	pc, lr

This movs is pretty drastic, because it changes from secure to
non-secure world, and yes, you can tell by looking at the orr
instruction above, but I would prefer a (potentially big fat) comment
here as well.

>  
>  	.align	5
>  prefetch_abort:
> -- 
> 1.7.12.1
> 

[U-Boot] [PATCH 2/6] ARM: add assembly routine to switch to non-secure state

[Christoffer Dall]

On Mon, May 06, 2013 at 03:17:46PM +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
> 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 register. We check explicitly for
> ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those
> cores we know the offsets for the GIC CPU interface from the
> PERIPHBASE content. Other cores could be added as needed.
> 
> 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 = 0x80)
> 
> 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.
> 
> For reasons obvious later we only use caller saved registers r0-r3.

You probably want to put that in a comment in the code, and it would
also be super helpful to explain the obvious part here, because most
readers don't look "forward in time" to understand this patch...

> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  arch/arm/cpu/armv7/start.S | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index da48b36..e63e892 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -572,3 +572,50 @@ fiq:
>  
>  #endif /* CONFIG_USE_IRQ */
>  #endif /* CONFIG_SPL_BUILD */
> +
> +#ifdef CONFIG_ARMV7_VIRT
> +/* Routine to initialize GIC CPU interface and switch to nonsecure state.
> + */
> +.globl _nonsec_gic_switch
> +_nonsec_gic_switch:
> +	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE

You should probably check if bits [7:0] == 0x00 here, otherwise you may
end up doing some very strange stuff - if any of those bits are set you
can just error out at this point, but it should be gracefully handled.

Also since it's core specific, you'd probably want to check that before
using it.

> +	add	r3, r2, #0x1000			@ GIC dist i/f offset

Since this is core specific, could the offset please go in an
appropriate header file?  It will also eliminate the need for the
comment if you just have a proper define (i.e. GIC_DIST_OFFSET ...)

> +	mvn	r1, #0
> +	str	r1, [r3, #0x80]			@ allow private interrupts

Aren't you makeing an assumption about the number of available
interrupts here?  You should read the ITLinesNumber field from the
GICD_TYPER if I'm not mistaking.

I also think it would be much cleaner if you again used a define for the
0x80 offset.

Also, don't you need to set some enable fields on the GICD_CTLR here to
enable group 1 interrupts?

> +
> +	mrc	p15, 0, r0, c0, c0, 0		@ MIDR
> +	bfc	r0, #16, #8			@ mask out variant, arch
> +	bfc	r0, #0, #4			@ and revision
> +	movw	r1, #0xc070
> +	movt	r1, #0x4100
> +	cmp	r0, r1				@ check for Cortex-A7
> +	orr	r1, #0xf0

wow, nice bit fiddling.  It may be quite a bit easier to read if you
simply had defines for the bitmasks and real values and just did a load
and placed a literal section accordingly.

> +	cmpne	r0, r1				@ check for Cortex-A15
> +	movne	r1, #0x100			@ GIC CPU offset for A9

So I read the ARMV7_VIRT config flag as something you can only enable on
a board that actually supports the virtulization extensions, which the
A9 doesn't so this should probably error out instead (or do an endless
loop, or ignore the case in the code, ...).

> +	moveq	r1, #0x2000			@ GIC CPU offset for A15/A7

Again, I think defines for these are appropriate.

> +	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
> +
> +	mov	r1, #1
> +	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
> +	mov	r1, #0x80

Why are you not setting this to #0xff ?

> +	str	r1, [r3, #4]			@ set GICC_PIMR[7]
> +

here it seems we're moving into non-gic related stuff in a function
called _nonsec_gic_switch

> +	movw	r1, #0x3fff
> +	movt	r1, #0x0006
> +	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
> +
> +	ldr	r1, =_start
> +	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
> +	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR

I thought you already took care of the MVBAR during init?
> +
> +	isb
> +	smc	#0				@ call into MONITOR mode
> +	isb					@ clobbers r0 and r1

This isb shouldn't be necessary if you just did an exception return?

> +
> +	mov	r1, #1
> +	str	r1, [r3, #0]			@ set GICC_CTLR[enable]

you're doing more than setting the enable bit: you're setting the EOImodeNS,
IRQBypDisGrp1, and FIQBypDisGrp1, as you should.

> +	add	r2, r2, #0x1000			@ GIC dist i/f offset
> +	str	r1, [r2]			@ allow private interrupts

It seems a bit brittle to rely on the smc handler to not clobber r2 and
r3, and it may an annoying thing to track down.  You could just
re-generate the the gic base address from the cp15 register.  Your
choice.

> +
> +	mov	pc, lr
> +#endif /* CONFIG_ARMV7_VIRT */
> -- 
> 1.7.12.1
> 

[U-Boot] [PATCH 3/6] ARM: switch to non-secure state during bootm execution

[Christoffer Dall]

On Mon, May 06, 2013 at 03:17:47PM +0200, Andre Przywara wrote:
> To actually trigger the non-secure switch we just implemented, call
> the switching routine from within the bootm command implementation.
> This way we automatically enable this feature without further user
> intervention.
> 
> Some part of the work is done in the assembly routine in start.S,
> introduced with the previous patch, but for the full glory we need
> to setup the GIC distributor interface once for the whole system,
> which is done in C here.
> The routine is placed in arch/arm/lib to allow easy access from
> different boards or CPUs.

I'm beginning to loose track of exactly which parts is in assembly and
which parts are in C, and why.  We are fiddling with some gic dist.
settings in assembly, and some of them in C as well.

I think it's just the ordering or naming of the patches that is a little
confusing.

> 
> First we check for the availability of the security extensions.
> 
> The generic timer base frequency register is only accessible from
> secure state, so we have to program it now. Actually this should be
> done from primary firmware before, but some boards seems to omit
> this, so if needed we do this here with a board specific value.
> 
> Since we need a safe way to access the GIC, we use the PERIPHBASE
> registers on Cortex-A15 and A7 CPUs and do some sanity checks.
> 
> Then we actually do the GIC enablement:
> a) enable the GIC distributor, both for non-secure and secure state
>    (GICD_CTLR[1:0] = 11b)
> b) allow all interrupts to be handled from non-secure state
>    (GICD_IGROUPRn = 0xFFFFFFFF)
> The core specific GIC setup is then done in the assembly routine.
> 
> The actual bootm trigger is pretty small: calling the routine and
> doing some error reporting. A return value of 1 will be added later.
> 
> To enable the whole code we introduce the CONFIG_ARMV7_VIRT variable.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  arch/arm/include/asm/armv7.h |   7 +++
>  arch/arm/lib/Makefile        |   2 +
>  arch/arm/lib/bootm.c         |  20 ++++++++
>  arch/arm/lib/virt-v7.c       | 113 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 142 insertions(+)
>  create mode 100644 arch/arm/lib/virt-v7.c
> 
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index a73630b..25afffe 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -74,4 +74,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
> +int armv7_switch_nonsec(void);
> +
> +/* defined in cpu/armv7/start.S */
> +void _nonsec_gic_switch(void);
> +#endif /* CONFIG_ARMV7_VIRT */
> +
>  #endif
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index 6ae161a..37a0e71 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -58,6 +58,8 @@ COBJS-y	+= reset.o
>  COBJS-y	+= cache.o
>  COBJS-y	+= cache-cp15.o
>  
> +COBJS-$(CONFIG_ARMV7_VIRT) += virt-v7.o
> +
>  SRCS	:= $(GLSOBJS:.o=.S) $(GLCOBJS:.o=.c) \
>  	   $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
>  OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index f3b30c5..a3d3aae 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -35,6 +35,10 @@
>  #include <asm/bootm.h>
>  #include <linux/compiler.h>
>  
> +#ifdef CONFIG_ARMV7_VIRT
> +#include <asm/armv7.h>
> +#endif
> +
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  #if defined(CONFIG_SETUP_MEMORY_TAGS) || \
> @@ -319,6 +323,22 @@ static void boot_prep_linux(bootm_headers_t *images)
>  		hang();
>  #endif /* all tags */
>  	}
> +#ifdef CONFIG_ARMV7_VIRT
> +	switch (armv7_switch_nonsec()) {
> +	case 0:
> +		debug("entered non-secure state\n");
> +		break;
> +	case 2:
> +		printf("HYP mode: Security extensions not implemented.\n");
> +		break;
> +	case 3:
> +		printf("HYP mode: CPU not supported (must be Cortex-A15 or A7).\n");

I would drop the specifics of what's supported here.

> +		break;
> +	case 4:
> +		printf("HYP mode: PERIPHBASE is above 4 GB, cannot access this.\n");
> +		break;
> +	}

I think these hard-coded numbers are a bit ugly, either define an enum
or some defines somewhere, or just make the armv7_switch_nonsec return a
boolean value and put the prints in there.

That will also make it easier to read the other function and not go
"return 2" hmmm, I wonder what that means ;)

> +#endif
>  }
>  
>  /* Subcommand: GO */
> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
> new file mode 100644
> index 0000000..3a48aee
> --- /dev/null
> +++ b/arch/arm/lib/virt-v7.c
> @@ -0,0 +1,113 @@
> +/*
> + * (C) Copyright 2013
> + * Andre Przywara, Linaro
> + *
> + * routines to push ARMv7 processors from secure into non-secure state
> + * needed to enable ARMv7 virtualization for current hypervisors

Nits: Routines should be capitalized.  Also not completely sure about
the wording about pushing between secure and non-secure state, changing,
transitioning, seems like more commonly used terms, but I dont' feel
strongly about any of this.

Again, I really think this is the wrong way to go about it.
Transitioning from secure to non-secure is really a feature of its own
which is a useful feature in U-boot.  Orthogonal to that is the need to
boot kernels in Hyp-mode, and being booted in secure more and
controlling all of the non-secure worlds is just one scenario for that.

> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <asm/armv7.h>
> +
> +#define GICD_CTLR	0x000
> +#define GICD_TYPER	0x004
> +#define GICD_IGROUPR0	0x080
> +#define GICD_SGIR	0xf00
> +
> +#define CPU_ARM_CORTEX_A15	0x4100c0f0
> +#define CPU_ARM_CORTEX_A7	0x4100c070
> +
> +static inline unsigned int read_cpsr(void)

inline is typically not used in C-files - at least not in the kernel.
GCC is pretty smart about this on its own.

> +{
> +	unsigned int reg;
> +
> +	asm volatile ("mrs %0, cpsr\n" : "=r" (reg));
> +	return reg;
> +}
> +
> +int armv7_switch_nonsec(void)
> +{
> +	unsigned int reg;
> +	volatile unsigned int *gicdptr;
> +	unsigned itlinesnr, i;
> +
> +	/* check whether the CPU supports the security extensions */
> +	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));

My brain hasn't managed to learn all the coprocessor register by heart
yet, so if you could provide the name for these registers it would be
cool.  Alternatively you could just create nice static little functions
just like you do with the cpsr.

> +	if ((reg & 0xF0) == 0)
> +		return 2;
> +
> +	/* the timer frequency for the generic timer needs to be
> +	 * programmed still in secure state, should be done by firmware.

nit: drop 'still'
nit: the 'should be done by firmware' is not very descriptive, consider
stating clearly that this is a work-around for broken bootloaders.

> +	 * check whether we have the generic timer first
> +	 */
> +#ifdef CONFIG_SYS_CLK_FREQ
> +	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> +	if ((reg & 0xF0000) == 0x10000)
> +		asm("mcr p15, 0, %0, c14, c0, 0\n"
> +		: : "r"(CONFIG_SYS_CLK_FREQ));
> +#endif
> +
> +	/* the SCR register will be set directly in the monitor mode handler,
> +	 * according to the spec one should not tinker with it in secure state
> +	 * in SVC mode. Do not try to read it once in non-secure state,
> +	 * any access to it will trap.
> +	 */
> +
> +	/* check whether we are an Cortex-A15 or A7.

nit: s/whether/if/

> +	 * The actual non-secure switch should work with all CPUs supporting
> +	 * the security extension, but we need the GIC address,
> +	 * which we know only for sure for those two CPUs.
> +	 */
> +	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(reg));
> +	if (((reg & 0xFF00FFF0) != 0x4100C0F0) &&
> +	    ((reg & 0xFF00FFF0) != 0x4100C070))

Are there really no defines for this in U-boot already?

It seems to me that a static inline in a header file somewhere that
gives you a processor type back would be useful for other things as
well, but I don't know U-boot enough to properly say...

> +		return 3;
> +
> +	/* get the GIC base address from the A15 PERIPHBASE register */
> +	asm("mrc p15, 4, %0, c15, c0, 0\n" : "=r" (reg));
> +
> +	/* the PERIPHBASE can be mapped above 4 GB (lower 8 bits used to
> +	 * encode this). Bail out here since we cannot access this without
> +	 * enabling paging.
> +	 */

ah, here you check for it...

> +	if ((reg & 0xff) != 0)
> +		return 4;

if you're getting the PERIPHBASE here anyway, why not just pass it as a
parameter to the non-secure gic init routine?

> +
> +	/* GIC distributor registers start at offset 0x1000 */
> +	gicdptr = (unsigned *)(reg + 0x1000);
> +
> +	/* enable the GIC distributor */
> +	gicdptr[GICD_CTLR / 4] |= 0x03;

so this is I/O accesses, so you could consider using readl for
consistency, which would also get rid of the division in the array
element accessors.

> +
> +	/* TYPER[4:0] contains an encoded number of all interrupts */
> +	itlinesnr = gicdptr[GICD_TYPER / 4] & 0x1f;
> +
> +	/* set all bits in the GIC group registers to one to allow access
> +	 * from non-secure state
> +	 */
> +	for (i = 0; i <= itlinesnr; i++)
> +		gicdptr[GICD_IGROUPR0 / 4 + i] = (unsigned)-1;

didn't you also do this in the assembly routine for IGROUP0 only?

oh wait, it's dawning on me, the _nonsec_gic_switch is actually per-cpu
non-secure init, right?

> +
> +	/* call the non-sec switching code on this CPU */
> +	_nonsec_gic_switch();
> +
> +	return 0;
> +}
> -- 
> 1.7.12.1
> 

[U-Boot] [PATCH 4/6] ARM: add SMP support for non-secure switch

[Christoffer Dall]

On Mon, May 06, 2013 at 03:17:48PM +0200, Andre Przywara wrote:
> Currently the non-secure switch is only done for the boot processor.
> To later allow full SMP support, we have to switch all secondary
> cores into non-secure state also.
> 
> So we add an entry point for secondary CPUs coming out of low-power
> state and make sure we put them into WFI again after having switched
> to non-secure state.
> For this we acknowledge and EOI the wake-up IPI, then go into WFI.
> Once being kicked out of it later, we sanity check that the start
> address has actually been changed (since another attempt to switch
> to non-secure would block the core) and jump to the new address.
> 
> The actual CPU kick is done by sending an inter-processor interrupt
> via the GIC to all CPU interfaces except the requesting processor.
> The secondary cores will then setup their respective GIC CPU
> interface.
> 
> The address secondary cores jump to is board specific, we provide
> the value here for the Versatile Express board.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  arch/arm/cpu/armv7/start.S          | 27 ++++++++++++++++++++++++++-
>  arch/arm/include/asm/armv7.h        |  1 +
>  arch/arm/lib/virt-v7.c              |  9 ++++++++-
>  include/configs/vexpress_ca15_tc2.h |  1 +
>  4 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index e63e892..02234c7 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -575,8 +575,19 @@ fiq:
>  
>  #ifdef CONFIG_ARMV7_VIRT
>  /* Routine to initialize GIC CPU interface and switch to nonsecure state.
> + * Will be executed directly by secondary CPUs after coming out of
> + * WFI, or can be called directly by C code for CPU 0.
> + * Those two paths mandate to not use any stack and to only use registers
> + * r0-r3 to comply with both the C ABI and the requirement of SMP startup
> + * code.
>   */
>  .globl _nonsec_gic_switch
> +.globl _smp_pen
> +_smp_pen:
> +	mrs	r0, cpsr
> +	orr	r0, r0, #0xc0
> +	msr	cpsr, r0			@ disable interrupts
> +	mov	lr, #0				@ clear LR to mark secondary

instead of this subtle abuse of lr, why not make this routine simply
take a parameter?

I also slightly object against wrapping the _smp_pen around the
_nonsec_gic_switch, I really think these are separate routines, where
one can just call the other...?

>  _nonsec_gic_switch:
>  	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
>  	add	r3, r2, #0x1000			@ GIC dist i/f offset
> @@ -617,5 +628,19 @@ _nonsec_gic_switch:
>  	add	r2, r2, #0x1000			@ GIC dist i/f offset
>  	str	r1, [r2]			@ allow private interrupts
>  
> -	mov	pc, lr
> +	cmp	lr, #0
> +	movne	pc, lr				@ CPU 0 to return
> +						@ all others: go to sleep
> +_ack_int:
> +	ldr	r1, [r3, #0x0c]			@ read GICD acknowledge
> +	str	r1, [r3, #0x10]			@ write GICD EOI
> +
> +	adr	r1, _smp_pen
> +waitloop:
> +	wfi
> +	ldr	r0, =CONFIG_SYSFLAGS_ADDR	@ load start address
> +	ldr	r0, [r0]
> +	cmp	r0, r1			@ make sure we dont execute this code

I think I raised this issue previously, but this code is in a core
u-boot file, but I could imagine a board with a different crazy boot
protocol that required you to check two different fields and jump
through other hoops to wake up from the smp pen, so I really think the
whole smp pen belongs in a board specific place.

Also, the boot-wrapper code used wfe instead, not sure if there are any
users that just send an event and doesn't send an IPI?

> +	beq	waitloop		@ again (due to a spurious wakeup)
> +	mov	pc, r0
>  #endif /* CONFIG_ARMV7_VIRT */
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index 25afffe..296dc92 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -78,6 +78,7 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
>  int armv7_switch_nonsec(void);
>  
>  /* defined in cpu/armv7/start.S */
> +void _smp_pen(void);
>  void _nonsec_gic_switch(void);
>  #endif /* CONFIG_ARMV7_VIRT */
>  
> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
> index 3a48aee..0248010 100644
> --- a/arch/arm/lib/virt-v7.c
> +++ b/arch/arm/lib/virt-v7.c
> @@ -48,6 +48,7 @@ int armv7_switch_nonsec(void)
>  	unsigned int reg;
>  	volatile unsigned int *gicdptr;
>  	unsigned itlinesnr, i;
> +	unsigned int *sysflags;
>  
>  	/* check whether the CPU supports the security extensions */
>  	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> @@ -106,7 +107,13 @@ int armv7_switch_nonsec(void)
>  	for (i = 0; i <= itlinesnr; i++)
>  		gicdptr[GICD_IGROUPR0 / 4 + i] = (unsigned)-1;
>  
> -	/* call the non-sec switching code on this CPU */
> +	/* now kick all CPUs (expect this one) by writing to GICD_SIGR */
> +	sysflags = (void *)CONFIG_SYSFLAGS_ADDR;
> +	sysflags[1] = (unsigned)-1;
> +	sysflags[0] = (uintptr_t)_smp_pen;
> +	gicdptr[GICD_SGIR / 4] = 1U << 24;

here you definitely want a barrier to make sure you don't kick the cpus
before the sysflags addresses have been written.  What does the
(unsigned)-1 write to sysflags[1] do?

> +
> +	/* call the non-sec switching code on this CPU also */
>  	_nonsec_gic_switch();
>  
>  	return 0;
> diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
> index 9e230ad..210a27c 100644
> --- a/include/configs/vexpress_ca15_tc2.h
> +++ b/include/configs/vexpress_ca15_tc2.h
> @@ -32,5 +32,6 @@
>  #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
>  
>  #define CONFIG_SYS_CLK_FREQ 24000000
> +#define CONFIG_SYSFLAGS_ADDR 0x1c010030
>  
>  #endif
> -- 
> 1.7.12.1
> 

[U-Boot] [PATCH 5/6] ARM: extend non-secure switch to also go into HYP mode

[Christoffer Dall]

On Mon, May 06, 2013 at 03:17:49PM +0200, Andre Przywara wrote:
> For the KVM and XEN hypervisors to be usable, we need to enter the
> kernel in HYP mode. Now that we already are in non-secure state,
> HYP mode switching is within short reach.
> 
> While doing the non-secure switch, we have to enable the HVC
> instruction and setup the HYP mode HVBAR (while still secure).
> 
> The actual switch is done by dropping back from a HYP mode handler
> without actually leaving HYP mode, so we introduce a new handler
> routine in the exception vector table.
> 
> In the assembly switching routine - which we rename to hyp_gic_switch
> on the way - we save and restore the banked LR and SP registers
> around the hypercall to do the actual HYP mode switch.
> 
> The C routine first checks whether we are in HYP mode already and
> also whether the virtualization extensions are available. It also
> checks whether the HYP mode switch was finally successful.
> The bootm command part only adds and adjusts some error reporting.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  arch/arm/cpu/armv7/start.S   | 34 +++++++++++++++++++++++-----------
>  arch/arm/include/asm/armv7.h |  4 ++--
>  arch/arm/lib/bootm.c         | 12 +++++++++---
>  arch/arm/lib/virt-v7.c       | 22 +++++++++++++++-------
>  4 files changed, 49 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index 02234c7..921e9d9 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -41,7 +41,7 @@ _start: b	reset
>  	ldr	pc, _software_interrupt
>  	ldr	pc, _prefetch_abort
>  	ldr	pc, _data_abort
> -	ldr	pc, _not_used
> +	ldr	pc, _hyp_trap
>  	ldr	pc, _irq
>  	ldr	pc, _fiq
>  #ifdef CONFIG_SPL_BUILD
> @@ -49,7 +49,7 @@ _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
> +_hyp_trap:		.word _hyp_trap
>  _irq:			.word _irq
>  _fiq:			.word _fiq
>  _pad:			.word 0x12345678 /* now 16*4=64 */
> @@ -58,7 +58,7 @@ _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
> +_hyp_trap:		.word hyp_trap
>  _irq:			.word irq
>  _fiq:			.word fiq
>  _pad:			.word 0x12345678 /* now 16*4=64 */
> @@ -513,12 +513,18 @@ software_interrupt:
>  	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
>  	bic	r1, r1, #0x07f
>  	orr	r1, r1, #0x31			@ enable NS, AW, FW
> +	mrc	p15, 0, r0, c0, c1, 1		@ check for Virt ext
> +	and	r0, r0, #0xf000
> +	cmp	r0, #0x1000

you can just do ands r0, r0 ,#0xf000 and change the orreq below to orrne

> +	orreq	r1, r1, #0x100			@ allow HVC instruction
>  
>  	mrc	p15, 0, r0, c12, c0, 0		@ save secure copy of VBAR
>  	mcr	p15, 0, r1, c1, c1, 0		@ write SCR, switch to non-sec
>  	isb
>  	mcr	p15, 0, r0, c12, c0, 0		@ write non-secure copy of VBAR
>  
> +	mcreq	p15, 4, r0, c12, c0, 0		@ write HYP mode HVBAR
> +

nit: s/HYP mode//

>  	movs	pc, lr
>  
>  	.align	5
> @@ -534,10 +540,9 @@ data_abort:
>  	bl	do_data_abort
>  
>  	.align	5
> -not_used:
> -	get_bad_stack
> -	bad_save_user_regs
> -	bl	do_not_used
> +hyp_trap:
> +	.byte 0x00, 0xe3, 0x0e, 0xe1		@ mrs lr, elr_hyp

do we really need to support this on assemblers that old?

> +	mov pc, lr
>  
>  #ifdef CONFIG_USE_IRQ
>  
> @@ -574,21 +579,21 @@ fiq:
>  #endif /* CONFIG_SPL_BUILD */
>  
>  #ifdef CONFIG_ARMV7_VIRT
> -/* Routine to initialize GIC CPU interface and switch to nonsecure state.
> - * Will be executed directly by secondary CPUs after coming out of
> +/* Routine to initialize GIC CPU interface, switch to nonsecure and to HYP
> + * mode. Will be executed directly by secondary CPUs after coming out of

So now this routine does three different things in different context at
once, why?

>   * WFI, or can be called directly by C code for CPU 0.
>   * Those two paths mandate to not use any stack and to only use registers
>   * r0-r3 to comply with both the C ABI and the requirement of SMP startup
>   * code.
>   */
> -.globl _nonsec_gic_switch
> +.globl _hyp_gic_switch
>  .globl _smp_pen
>  _smp_pen:
>  	mrs	r0, cpsr
>  	orr	r0, r0, #0xc0
>  	msr	cpsr, r0			@ disable interrupts
>  	mov	lr, #0				@ clear LR to mark secondary
> -_nonsec_gic_switch:
> +_hyp_gic_switch:
>  	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
>  	add	r3, r2, #0x1000			@ GIC dist i/f offset
>  	mvn	r1, #0
> @@ -628,6 +633,13 @@ _nonsec_gic_switch:
>  	add	r2, r2, #0x1000			@ GIC dist i/f offset
>  	str	r1, [r2]			@ allow private interrupts
>  
> +	mov	r2, lr
> +	mov	r1, sp
> +	.byte 0x70, 0x00, 0x40, 0xe1		@ hvc #0
> +	isb

again, I'm doubtful this isb is necessary when you just did an exception
return.

> +	mov	sp, r1
> +	mov	lr, r2
> +
>  	cmp	lr, #0
>  	movne	pc, lr				@ CPU 0 to return
>  						@ all others: go to sleep
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index 296dc92..17bb497 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -75,11 +75,11 @@ void v7_outer_cache_flush_range(u32 start, u32 end);
>  void v7_outer_cache_inval_range(u32 start, u32 end);
>  
>  #ifdef CONFIG_ARMV7_VIRT
> -int armv7_switch_nonsec(void);
> +int armv7_switch_hyp(void);
>  
>  /* defined in cpu/armv7/start.S */
>  void _smp_pen(void);
> -void _nonsec_gic_switch(void);
> +void _hyp_gic_switch(void);
>  #endif /* CONFIG_ARMV7_VIRT */
>  
>  #endif
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index a3d3aae..552ba59 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -324,12 +324,15 @@ static void boot_prep_linux(bootm_headers_t *images)
>  #endif /* all tags */
>  	}
>  #ifdef CONFIG_ARMV7_VIRT
> -	switch (armv7_switch_nonsec()) {
> +	switch (armv7_switch_hyp()) {
>  	case 0:
> -		debug("entered non-secure state\n");
> +		debug("entered HYP mode\n");
> +		break;
> +	case 1:
> +		debug("CPU already in HYP mode\n");
>  		break;
>  	case 2:
> -		printf("HYP mode: Security extensions not implemented.\n");
> +		printf("HYP mode: Virtualization extensions not implemented.\n");
>  		break;
>  	case 3:
>  		printf("HYP mode: CPU not supported (must be Cortex-A15 or A7).\n");
> @@ -337,6 +340,9 @@ static void boot_prep_linux(bootm_headers_t *images)
>  	case 4:
>  		printf("HYP mode: PERIPHBASE is above 4 GB, cannot access this.\n");
>  		break;
> +	case 5:
> +		printf("HYP mode: switch not successful.\n");
> +		break;
>  	}
>  #endif
>  }
> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
> index 0248010..3883463 100644
> --- a/arch/arm/lib/virt-v7.c
> +++ b/arch/arm/lib/virt-v7.c
> @@ -3,6 +3,7 @@
>   * Andre Przywara, Linaro
>   *
>   * routines to push ARMv7 processors from secure into non-secure state
> + * and from non-secure SVC into HYP mode
>   * needed to enable ARMv7 virtualization for current hypervisors
>   *
>   * See file CREDITS for list of people who contributed to this
> @@ -43,16 +44,20 @@ static inline unsigned int read_cpsr(void)
>  	return reg;
>  }
>  
> -int armv7_switch_nonsec(void)
> +int armv7_switch_hyp(void)
>  {
>  	unsigned int reg;
>  	volatile unsigned int *gicdptr;
>  	unsigned itlinesnr, i;
>  	unsigned int *sysflags;
>  
> -	/* check whether the CPU supports the security extensions */
> +	/* check whether we are in HYP mode already */
> +	if ((read_cpsr() & 0x1F) == 0x1a)
> +		return 1;
> +
> +	/* check whether the CPU supports the virtualization extensions */
>  	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> -	if ((reg & 0xF0) == 0)
> +	if ((reg & 0xF000) != 0x1000)
>  		return 2;
>  
>  	/* the timer frequency for the generic timer needs to be
> @@ -73,8 +78,8 @@ int armv7_switch_nonsec(void)
>  	 */
>  
>  	/* check whether we are an Cortex-A15 or A7.
> -	 * The actual non-secure switch should work with all CPUs supporting
> -	 * the security extension, but we need the GIC address,
> +	 * The actual HYP switch should work with all CPUs supporting
> +	 * the virtualization extension, but we need the GIC address,
>  	 * which we know only for sure for those two CPUs.
>  	 */
>  	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(reg));
> @@ -113,8 +118,11 @@ int armv7_switch_nonsec(void)
>  	sysflags[0] = (uintptr_t)_smp_pen;
>  	gicdptr[GICD_SGIR / 4] = 1U << 24;
>  
> -	/* call the non-sec switching code on this CPU also */
> -	_nonsec_gic_switch();
> +	/* call the HYP switching code on this CPU also */
> +	_hyp_gic_switch();
> +
> +	if ((read_cpsr() & 0x1F) != 0x1a)
> +		return 5;

this is really a fatal crash right? We probably don't want to try and
proceed with boot at this point.

>  
>  	return 0;
>  }
> -- 
> 1.7.12.1
> 

[U-Boot] [PATCH 0/6] ARMv7: Add HYP mode switching support

[Christoffer Dall]

On Mon, May 06, 2013 at 03:17:44PM +0200, Andre Przywara wrote:
> (for GIT URL and Changelog see below)
>
> ARM CPUs with the virtualization extension have a new mode called
> HYP mode, which allows hypervisors to safely control and monitor
> guests. The current hypervisor (KVM and Xen) implementations
> require the kernel to be entered in that HYP mode.
>
> This patch series introduces a configuration variable
> CONFIG_ARMV7_VIRT which enables code to switch all cores into HYP
> mode. This is done automatically during execution of the bootm
> command (but could also be done earlier - U-Boot runs fine in HYP
> mode without MMU usage).

I forget the u-boot specifics here, but if you boot over networking, for
example, does that eventually end up calling bootm or would Hyp mode not
be entered in this case?

I'm only raising this issue because it's important to minimize the hoops
and efforts for booting in Hyp mode, since it's harmless to do so
regardless of the linux kernel we're booting.

[...]

-Christoffer

[U-Boot] [PATCH 0/6] ARMv7: Add HYP mode switching support

[Andre Przywara]

On 05/31/2013 08:11 AM, Christoffer Dall wrote:
> On Mon, May 06, 2013 at 03:17:44PM +0200, Andre Przywara wrote:
>> (for GIT URL and Changelog see below)
>>
>> ARM CPUs with the virtualization extension have a new mode called
>> HYP mode, which allows hypervisors to safely control and monitor
>> guests. The current hypervisor (KVM and Xen) implementations
>> require the kernel to be entered in that HYP mode.
>>
>> This patch series introduces a configuration variable
>> CONFIG_ARMV7_VIRT which enables code to switch all cores into HYP
>> mode. This is done automatically during execution of the bootm
>> command (but could also be done earlier - U-Boot runs fine in HYP
>> mode without MMU usage).
>
> I forget the u-boot specifics here, but if you boot over networking, for
> example, does that eventually end up calling bootm or would Hyp mode not
> be entered in this case?

Despite the naming "bootm" is eventually always called when booting 
Linux, even bootz is a wrapper around this. Same with network boot. So 
unless you show me a case where this isn't, I think this is fine.

Regards,
Andre.

> I'm only raising this issue because it's important to minimize the hoops
> and efforts for booting in Hyp mode, since it's harmless to do so
> regardless of the linux kernel we're booting.
>
> [...]
>
> -Christoffer
>

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Andre Przywara]

On 05/31/2013 03:02 AM, Christoffer Dall wrote:

Christoffer,

thanks a lot for the thorough review. Comments inline.

> On Mon, May 06, 2013 at 03:17:45PM +0200, Andre Przywara wrote:
>> A prerequisite for using virtualization is to be in HYP mode, which
>> requires the CPU to be in non-secure state.
>> Introduce a monitor handler routine which switches the CPU to
>> non-secure state by setting the NS and associated bits.
>> According to the ARM ARM this should not be done in SVC mode, so we
>> have to setup a SMC handler for this. We reuse the current vector
>> table for this and make sure that we only access the MVBAR register
>> if the CPU supports the security extension and only if we
>> configured the board to use it, since boards entering u-boot already
>> in non-secure mode would crash on accessing MVBAR otherwise.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++---
>>   1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>> index e9e57e6..da48b36 100644
>> --- a/arch/arm/cpu/armv7/start.S
>> +++ b/arch/arm/cpu/armv7/start.S
>> @@ -155,6 +155,13 @@ reset:
>>   	/* Set vector address in CP15 VBAR register */
>>   	ldr	r0, =_start
>>   	mcr	p15, 0, r0, c12, c0, 0	@Set VBAR
>> +
>> +#ifdef CONFIG_ARMV7_VIRT
>> +	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
>> +	ands	r1, r1, #0x30
>> +	mcrne	p15, 0, r0, c12, c0, 1	@ Set secure monitor MVBAR
>
> Hmm, this smells a bit simplified to me.
>
> Support for ARMv7_VIRT should easy to integrate into u-boot even for
> platforms that do not boot U-boot directly into secure mode (OMAP5 GP
> platforms are such an example).  In this case you cannot assume that you
> can write the secure monitor mvbar.

Right, Albert kind of hinted on this already. I already fixed this in my 
private tree, totally removing these MVBAR writes here and instead 
copying the values from VBAR later just before we do the smc.
Will send out a fixed version.

>> +#endif
>> +
>>   #endif
>>
>>   	/* the mask ROM code should have PLL and others stable */
>> @@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup)
>>   	ldr     r0, =_start
>>   	mcr     p15, 0, r0, c12, c0, 0  @Set VBAR
>>
>> +#ifdef CONFIG_ARMV7_VIRT
>> +	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
>> +	ands	r1, r1, #0x30
>> +	mcrne	p15, 0, r0, c12, c0, 1  @ Set secure monitor MVBAR
>> +#endif
>> +
>>   	bx	lr
>>
>>   ENDPROC(c_runtime_cpu_setup)
>> @@ -490,11 +503,23 @@ undefined_instruction:
>>   	bad_save_user_regs
>>   	bl	do_undefined_instruction
>>
>> +/*
>> + * software interrupt aka. secure monitor handler
>> + * This is executed on a "smc" instruction, we use a "smc #0" to switch
>> + * to non-secure state
>> + */
>>   	.align	5
>>   software_interrupt:
>> -	get_bad_stack_swi
>> -	bad_save_user_regs
>> -	bl	do_software_interrupt
>
> Why is the following block not conditional on CONFIG_ARMV7_VIRT?
>
> Again, it feels a bit funny to modify this generic mechanism to contain
> this code for boards that boot in NS mode but have a way to enter Hyp
> mode using an HVC or SMC instruction.

software_interrupt is currently a panic routine. So it is not actually 
used by u-boot, it's just there to dump some state and eventually call 
reset_cpu().
So I feel that since I am now the only user of software_interrupt I 
don't need any special precautions and consider this routine now 
actually part of the HYP mode switcher. But of course I can retain the 
original "functionality" if CONFIG_ARMV7_VIRT is not defined.

>> +	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
>> +	bic	r1, r1, #0x07f
>> +	orr	r1, r1, #0x31			@ enable NS, AW, FW
>
> Are you sure you want to always route FIQ to non-secure here?

Since we actually don't install any secure software and just use the 
secure state to do the HYP switch I don't feel like we should restrict 
FIQ. Since we don't use it for our own purposes, no one would be able to 
use it then, right? So my idea was to allow as much as possible.

> Don't you need to set the HCE bit?  The whole register resets to
> 0register resets to zero.

This is added later in 5/6. For reviewing purposes I split the patches 
up to do the non-secure switch only first. Later I add the bits to 
actually go to HYP mode.

>> +
>> +	mrc	p15, 0, r0, c12, c0, 0		@ save secure copy of VBAR
>> +	mcr	p15, 0, r1, c1, c1, 0		@ write SCR, switch to non-sec
>
> Not quite a "swith to non-sec"; you're still in monitor mode.

Right, _non-secure_ monitor mode. If I got this thing correctly, then 
secure/non-secure is a state, not a mode. Switching from secure to 
non-secure can only be done in monitor mode. And the state totally 
depends on the NS bit in SCR, so by setting this we enter non-secure 
world immediately.

>> +	isb
>> +	mcr	p15, 0, r0, c12, c0, 0		@ write non-secure copy of VBAR
>
> I don't actually think that you are, I think you're writing the secure
> copy here.

With the mcr above I set the NS bit, so I am non-secure from that point 
on (hence the isb). Writing the VBAR with the NS bit set should set the 
non-secure copy. Not doing this was a problem I chased down for some 
days, so I believe this is correct.

> In that case, I'm also wondering if the isb is superflous, because we
> perform an exception return below, but we of course want to make damn
> sure that the write of the NS bit is set before the exception return,
> maybe some ARM guys have the right expertise here.
>
>> +
>> +	movs	pc, lr
>
> This movs is pretty drastic, because it changes from secure to
> non-secure world, and yes, you can tell by looking at the orr
> instruction above, but I would prefer a (potentially big fat) comment
> here as well.

OK.

Regards,
Andre.

>
>>
>>   	.align	5
>>   prefetch_abort:
>> --
>> 1.7.12.1
>>

[U-Boot] [PATCH 2/6] ARM: add assembly routine to switch to non-secure state

[Andre Przywara]

On 05/31/2013 05:04 AM, Christoffer Dall wrote:
> On Mon, May 06, 2013 at 03:17:46PM +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
>> 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 register. We check explicitly for
>> ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those
>> cores we know the offsets for the GIC CPU interface from the
>> PERIPHBASE content. Other cores could be added as needed.
>>
>> 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 = 0x80)
>>
>> 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.
>>
>> For reasons obvious later we only use caller saved registers r0-r3.
>
> You probably want to put that in a comment in the code, and it would
> also be super helpful to explain the obvious part here, because most
> readers don't look "forward in time" to understand this patch...

Agreed.

>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   arch/arm/cpu/armv7/start.S | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>> index da48b36..e63e892 100644
>> --- a/arch/arm/cpu/armv7/start.S
>> +++ b/arch/arm/cpu/armv7/start.S
>> @@ -572,3 +572,50 @@ fiq:
>>
>>   #endif /* CONFIG_USE_IRQ */
>>   #endif /* CONFIG_SPL_BUILD */
>> +
>> +#ifdef CONFIG_ARMV7_VIRT
>> +/* Routine to initialize GIC CPU interface and switch to nonsecure state.
>> + */
>> +.globl _nonsec_gic_switch
>> +_nonsec_gic_switch:
>> +	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
>
> You should probably check if bits [7:0] == 0x00 here, otherwise you may
> end up doing some very strange stuff - if any of those bits are set you
> can just error out at this point, but it should be gracefully handled.
>
> Also since it's core specific, you'd probably want to check that before
> using it.

As you found out later, I am doing this before calling this routine. But 
I will add a comment here to avoid confusion.

>> +	add	r3, r2, #0x1000			@ GIC dist i/f offset
>
> Since this is core specific, could the offset please go in an
> appropriate header file?  It will also eliminate the need for the
> comment if you just have a proper define (i.e. GIC_DIST_OFFSET ...)
>
>> +	mvn	r1, #0
>> +	str	r1, [r3, #0x80]			@ allow private interrupts
>
> Aren't you makeing an assumption about the number of available
> interrupts here?  You should read the ITLinesNumber field from the
> GICD_TYPER if I'm not mistaking.

This is the per core private interrupts. All bits should be implemented.
 From the GIC spec, chapter 4.3.4:
"In a multiprocessor implementation, GICD_IGROUPR0 is banked for each 
connected processor. This register holds the group status bits for 
interrupts 0-31."

> I also think it would be much cleaner if you again used a define for the
> 0x80 offset.
>
> Also, don't you need to set some enable fields on the GICD_CTLR here to
> enable group 1 interrupts?

Since this a non-banked per-system register, I do this later in the C part.

>> +
>> +	mrc	p15, 0, r0, c0, c0, 0		@ MIDR
>> +	bfc	r0, #16, #8			@ mask out variant, arch
>> +	bfc	r0, #0, #4			@ and revision
>> +	movw	r1, #0xc070
>> +	movt	r1, #0x4100
>> +	cmp	r0, r1				@ check for Cortex-A7
>> +	orr	r1, #0xf0
>
> wow, nice bit fiddling.  It may be quite a bit easier to read if you
> simply had defines for the bitmasks and real values and just did a load
> and placed a literal section accordingly.

The sequence is necessary since we are short on registers. I agree it is 
a bit obfuscated, will make it more readable.

>> +	cmpne	r0, r1				@ check for Cortex-A15
>> +	movne	r1, #0x100			@ GIC CPU offset for A9
>
> So I read the ARMV7_VIRT config flag as something you can only enable on
> a board that actually supports the virtulization extensions, which the
> A9 doesn't so this should probably error out instead (or do an endless
> loop, or ignore the case in the code, ...).

Yeah, originally I had the idea to support a non-sec switch only on 
non-virt capable CPUs. My first version had a separate non-sec switch 
command. This here is kind of a leftover of that early version. But 
until patch 5/6 is applied this actually works (and we had a use-case 
internally here), so I decided to leave this in.

>> +	moveq	r1, #0x2000			@ GIC CPU offset for A15/A7
>
> Again, I think defines for these are appropriate.

Good point. Will fix this.

>> +	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
>> +
>> +	mov	r1, #1
>> +	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
>> +	mov	r1, #0x80
>
> Why are you not setting this to #0xff ?

Because a certain Christoffer Dall did this the same way in his Arndale 
specific patch ;-)
+       /* Set GIC priority mask bit [7] = 1 */
+       addr = EXYNOS5_GIC_CPU_BASE;
+       writel(0x80, addr + ARM_GICV2_ICCPMR);
(and I remember having read this recommendation somewhere)

>> +	str	r1, [r3, #4]			@ set GICC_PIMR[7]
>> +
>
> here it seems we're moving into non-gic related stuff in a function
> called _nonsec_gic_switch

Right, but this is per CPU and needs to be done in secure monitor mode, 
so it belongs here. Shall I rename the function to be called 
non_secure_init or the like?

>
>> +	movw	r1, #0x3fff
>> +	movt	r1, #0x0006
>> +	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
>> +
>> +	ldr	r1, =_start
>> +	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
>> +	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR
>
> I thought you already took care of the MVBAR during init?

Right, as mentioned earlier I have fixed this already.

>> +
>> +	isb
>> +	smc	#0				@ call into MONITOR mode
>> +	isb					@ clobbers r0 and r1
>
> This isb shouldn't be necessary if you just did an exception return?

Seems to be a paranoid leftover of one debug session...

>> +
>> +	mov	r1, #1
>> +	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
>
> you're doing more than setting the enable bit: you're setting the EOImodeNS,
> IRQBypDisGrp1, and FIQBypDisGrp1, as you should.

But 0x01 is the correct value? And the reset value is 0, right?
I will extend the comment.

>> +	add	r2, r2, #0x1000			@ GIC dist i/f offset
>> +	str	r1, [r2]			@ allow private interrupts
>
> It seems a bit brittle to rely on the smc handler to not clobber r2 and
> r3, and it may an annoying thing to track down.  You could just
> re-generate the the gic base address from the cp15 register.  Your
> choice.

So shall I put comments here and at the smc handler?

Thanks again for the comments!
Andre.

>> +
>> +	mov	pc, lr
>> +#endif /* CONFIG_ARMV7_VIRT */
>> --
>> 1.7.12.1
>>

[U-Boot] [PATCH 3/6] ARM: switch to non-secure state during bootm execution

[Andre Przywara]

On 05/31/2013 07:10 AM, Christoffer Dall wrote:
> On Mon, May 06, 2013 at 03:17:47PM +0200, Andre Przywara wrote:
>> To actually trigger the non-secure switch we just implemented, call
>> the switching routine from within the bootm command implementation.
>> This way we automatically enable this feature without further user
>> intervention.
>>
>> Some part of the work is done in the assembly routine in start.S,
>> introduced with the previous patch, but for the full glory we need
>> to setup the GIC distributor interface once for the whole system,
>> which is done in C here.
>> The routine is placed in arch/arm/lib to allow easy access from
>> different boards or CPUs.
>
> I'm beginning to loose track of exactly which parts is in assembly and
> which parts are in C, and why.  We are fiddling with some gic dist.
> settings in assembly, and some of them in C as well.

I fear I dropped the explanation some time during a patch split earlier.
So the assembly code is the per core part - including GICD_IGROUPR0, 
which is banked per core. The reason this is in assembly is to make it 
easily run right out of the SMP pen.

In C I do anything that needs to be only done once for the whole system.

More comments inline...

> I think it's just the ordering or naming of the patches that is a little
> confusing.
>
>>
>> First we check for the availability of the security extensions.
>>
>> The generic timer base frequency register is only accessible from
>> secure state, so we have to program it now. Actually this should be
>> done from primary firmware before, but some boards seems to omit
>> this, so if needed we do this here with a board specific value.
>>
>> Since we need a safe way to access the GIC, we use the PERIPHBASE
>> registers on Cortex-A15 and A7 CPUs and do some sanity checks.
>>
>> Then we actually do the GIC enablement:
>> a) enable the GIC distributor, both for non-secure and secure state
>>     (GICD_CTLR[1:0] = 11b)
>> b) allow all interrupts to be handled from non-secure state
>>     (GICD_IGROUPRn = 0xFFFFFFFF)
>> The core specific GIC setup is then done in the assembly routine.
>>
>> The actual bootm trigger is pretty small: calling the routine and
>> doing some error reporting. A return value of 1 will be added later.
>>
>> To enable the whole code we introduce the CONFIG_ARMV7_VIRT variable.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   arch/arm/include/asm/armv7.h |   7 +++
>>   arch/arm/lib/Makefile        |   2 +
>>   arch/arm/lib/bootm.c         |  20 ++++++++
>>   arch/arm/lib/virt-v7.c       | 113 +++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 142 insertions(+)
>>   create mode 100644 arch/arm/lib/virt-v7.c
>>
>> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
>> index a73630b..25afffe 100644
>> --- a/arch/arm/include/asm/armv7.h
>> +++ b/arch/arm/include/asm/armv7.h
>> @@ -74,4 +74,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
>> +int armv7_switch_nonsec(void);
>> +
>> +/* defined in cpu/armv7/start.S */
>> +void _nonsec_gic_switch(void);
>> +#endif /* CONFIG_ARMV7_VIRT */
>> +
>>   #endif
>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
>> index 6ae161a..37a0e71 100644
>> --- a/arch/arm/lib/Makefile
>> +++ b/arch/arm/lib/Makefile
>> @@ -58,6 +58,8 @@ COBJS-y	+= reset.o
>>   COBJS-y	+= cache.o
>>   COBJS-y	+= cache-cp15.o
>>
>> +COBJS-$(CONFIG_ARMV7_VIRT) += virt-v7.o
>> +
>>   SRCS	:= $(GLSOBJS:.o=.S) $(GLCOBJS:.o=.c) \
>>   	   $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
>>   OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
>> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
>> index f3b30c5..a3d3aae 100644
>> --- a/arch/arm/lib/bootm.c
>> +++ b/arch/arm/lib/bootm.c
>> @@ -35,6 +35,10 @@
>>   #include <asm/bootm.h>
>>   #include <linux/compiler.h>
>>
>> +#ifdef CONFIG_ARMV7_VIRT
>> +#include <asm/armv7.h>
>> +#endif
>> +
>>   DECLARE_GLOBAL_DATA_PTR;
>>
>>   #if defined(CONFIG_SETUP_MEMORY_TAGS) || \
>> @@ -319,6 +323,22 @@ static void boot_prep_linux(bootm_headers_t *images)
>>   		hang();
>>   #endif /* all tags */
>>   	}
>> +#ifdef CONFIG_ARMV7_VIRT
>> +	switch (armv7_switch_nonsec()) {
>> +	case 0:
>> +		debug("entered non-secure state\n");
>> +		break;
>> +	case 2:
>> +		printf("HYP mode: Security extensions not implemented.\n");
>> +		break;
>> +	case 3:
>> +		printf("HYP mode: CPU not supported (must be Cortex-A15 or A7).\n");
>
> I would drop the specifics of what's supported here.
>

This is in particular here since I rely on the PERIPHBASE register, 
which is A15/A7 implementation specific. This is different from case 2 
(which will later be changed to "Virtualization extensions...")

>> +		break;
>> +	case 4:
>> +		printf("HYP mode: PERIPHBASE is above 4 GB, cannot access this.\n");
>> +		break;
>> +	}
>
> I think these hard-coded numbers are a bit ugly, either define an enum
> or some defines somewhere, or just make the armv7_switch_nonsec return a
> boolean value and put the prints in there.
>
> That will also make it easier to read the other function and not go
> "return 2" hmmm, I wonder what that means ;)

Right, will fix this.

>> +#endif
>>   }
>>
>>   /* Subcommand: GO */
>> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
>> new file mode 100644
>> index 0000000..3a48aee
>> --- /dev/null
>> +++ b/arch/arm/lib/virt-v7.c
>> @@ -0,0 +1,113 @@
>> +/*
>> + * (C) Copyright 2013
>> + * Andre Przywara, Linaro
>> + *
>> + * routines to push ARMv7 processors from secure into non-secure state
>> + * needed to enable ARMv7 virtualization for current hypervisors
>
> Nits: Routines should be capitalized.  Also not completely sure about
> the wording about pushing between secure and non-secure state, changing,
> transitioning, seems like more commonly used terms, but I dont' feel
> strongly about any of this.
>
> Again, I really think this is the wrong way to go about it.
> Transitioning from secure to non-secure is really a feature of its own
> which is a useful feature in U-boot.  Orthogonal to that is the need to
> boot kernels in Hyp-mode, and being booted in secure more and
> controlling all of the non-secure worlds is just one scenario for that.

OK. Originally I had an implementation which separated the non-sec 
switch and the HYP switch. Later I got the impression that there is no 
real use-case for a non-sec switch only, so I dropped the specific 
command and just kept the logical separation in the patch split. That 
simplified the patches quite a bit.

So do you want to have a new u-boot command switching to non-secure 
state? I think that would make the patches more complicated, but if you 
recommend to have such a thing, I am happy to provide it.
Core problem here is that there is no easy way to check whether you are 
non-secure. Accessing the SCR register will trap if the NS bit is set. I 
could just catch this and return back with an error condition. Just not 
sure this is really worth the effort.

Also leaves one question: How to handle if the users switched 
deliberately to non-secure before and now the bootm routine wants to go 
to HYP mode? Do we want to stay in SVC in this case?

>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#include <common.h>
>> +#include <asm/armv7.h>
>> +
>> +#define GICD_CTLR	0x000
>> +#define GICD_TYPER	0x004
>> +#define GICD_IGROUPR0	0x080
>> +#define GICD_SGIR	0xf00
>> +
>> +#define CPU_ARM_CORTEX_A15	0x4100c0f0
>> +#define CPU_ARM_CORTEX_A7	0x4100c070
>> +
>> +static inline unsigned int read_cpsr(void)
>
> inline is typically not used in C-files - at least not in the kernel.
> GCC is pretty smart about this on its own.

Right. I think newer GCCs inline short static functions automatically.

>> +{
>> +	unsigned int reg;
>> +
>> +	asm volatile ("mrs %0, cpsr\n" : "=r" (reg));
>> +	return reg;
>> +}
>> +
>> +int armv7_switch_nonsec(void)
>> +{
>> +	unsigned int reg;
>> +	volatile unsigned int *gicdptr;
>> +	unsigned itlinesnr, i;
>> +
>> +	/* check whether the CPU supports the security extensions */
>> +	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
>
> My brain hasn't managed to learn all the coprocessor register by heart
> yet, so if you could provide the name for these registers it would be
> cool.  Alternatively you could just create nice static little functions
> just like you do with the cpsr.

OK. Actually I found this quite some overhead if its only called once 
(cpsr is called twice). Would just a comment suffice?

>> +	if ((reg & 0xF0) == 0)
>> +		return 2;
>> +
>> +	/* the timer frequency for the generic timer needs to be
>> +	 * programmed still in secure state, should be done by firmware.
>
> nit: drop 'still'
> nit: the 'should be done by firmware' is not very descriptive, consider
> stating clearly that this is a work-around for broken bootloaders.

ACK.

>> +	 * check whether we have the generic timer first
>> +	 */
>> +#ifdef CONFIG_SYS_CLK_FREQ
>> +	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
>> +	if ((reg & 0xF0000) == 0x10000)
>> +		asm("mcr p15, 0, %0, c14, c0, 0\n"
>> +		: : "r"(CONFIG_SYS_CLK_FREQ));
>> +#endif
>> +
>> +	/* the SCR register will be set directly in the monitor mode handler,
>> +	 * according to the spec one should not tinker with it in secure state
>> +	 * in SVC mode. Do not try to read it once in non-secure state,
>> +	 * any access to it will trap.
>> +	 */
>> +
>> +	/* check whether we are an Cortex-A15 or A7.
>
> nit: s/whether/if/
>
>> +	 * The actual non-secure switch should work with all CPUs supporting
>> +	 * the security extension, but we need the GIC address,
>> +	 * which we know only for sure for those two CPUs.
>> +	 */
>> +	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(reg));
>> +	if (((reg & 0xFF00FFF0) != 0x4100C0F0) &&
>> +	    ((reg & 0xFF00FFF0) != 0x4100C070))
>
> Are there really no defines for this in U-boot already?
>
> It seems to me that a static inline in a header file somewhere that
> gives you a processor type back would be useful for other things as
> well, but I don't know U-boot enough to properly say...

You are right, this is barely readable. Will fix this.

>> +		return 3;
>> +
>> +	/* get the GIC base address from the A15 PERIPHBASE register */
>> +	asm("mrc p15, 4, %0, c15, c0, 0\n" : "=r" (reg));
>> +
>> +	/* the PERIPHBASE can be mapped above 4 GB (lower 8 bits used to
>> +	 * encode this). Bail out here since we cannot access this without
>> +	 * enabling paging.
>> +	 */
>
> ah, here you check for it...
>
>> +	if ((reg & 0xff) != 0)
>> +		return 4;
>
> if you're getting the PERIPHBASE here anyway, why not just pass it as a
> parameter to the non-secure gic init routine?

Because this routine is also called directly out of the SMP pen, so I 
cannot pass any parameters there.

>> +
>> +	/* GIC distributor registers start at offset 0x1000 */
>> +	gicdptr = (unsigned *)(reg + 0x1000);
>> +
>> +	/* enable the GIC distributor */
>> +	gicdptr[GICD_CTLR / 4] |= 0x03;
>
> so this is I/O accesses, so you could consider using readl for
> consistency, which would also get rid of the division in the array
> element accessors.

Good hint, thanks.

>> +
>> +	/* TYPER[4:0] contains an encoded number of all interrupts */
>> +	itlinesnr = gicdptr[GICD_TYPER / 4] & 0x1f;
>> +
>> +	/* set all bits in the GIC group registers to one to allow access
>> +	 * from non-secure state
>> +	 */
>> +	for (i = 0; i <= itlinesnr; i++)
>> +		gicdptr[GICD_IGROUPR0 / 4 + i] = (unsigned)-1;
>
> didn't you also do this in the assembly routine for IGROUP0 only?
>
> oh wait, it's dawning on me, the _nonsec_gic_switch is actually per-cpu
> non-secure init, right?

Right. I take this as an advice to explain this earlier...

Thanks,
Andre.

>> +
>> +	/* call the non-sec switching code on this CPU */
>> +	_nonsec_gic_switch();
>> +
>> +	return 0;
>> +}
>> --
>> 1.7.12.1
>>

[U-Boot] [PATCH 4/6] ARM: add SMP support for non-secure switch

[Andre Przywara]

On 05/31/2013 07:32 AM, Christoffer Dall wrote:
> On Mon, May 06, 2013 at 03:17:48PM +0200, Andre Przywara wrote:
>> Currently the non-secure switch is only done for the boot processor.
>> To later allow full SMP support, we have to switch all secondary
>> cores into non-secure state also.
>>
>> So we add an entry point for secondary CPUs coming out of low-power
>> state and make sure we put them into WFI again after having switched
>> to non-secure state.
>> For this we acknowledge and EOI the wake-up IPI, then go into WFI.
>> Once being kicked out of it later, we sanity check that the start
>> address has actually been changed (since another attempt to switch
>> to non-secure would block the core) and jump to the new address.
>>
>> The actual CPU kick is done by sending an inter-processor interrupt
>> via the GIC to all CPU interfaces except the requesting processor.
>> The secondary cores will then setup their respective GIC CPU
>> interface.
>>
>> The address secondary cores jump to is board specific, we provide
>> the value here for the Versatile Express board.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   arch/arm/cpu/armv7/start.S          | 27 ++++++++++++++++++++++++++-
>>   arch/arm/include/asm/armv7.h        |  1 +
>>   arch/arm/lib/virt-v7.c              |  9 ++++++++-
>>   include/configs/vexpress_ca15_tc2.h |  1 +
>>   4 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>> index e63e892..02234c7 100644
>> --- a/arch/arm/cpu/armv7/start.S
>> +++ b/arch/arm/cpu/armv7/start.S
>> @@ -575,8 +575,19 @@ fiq:
>>
>>   #ifdef CONFIG_ARMV7_VIRT
>>   /* Routine to initialize GIC CPU interface and switch to nonsecure state.
>> + * Will be executed directly by secondary CPUs after coming out of
>> + * WFI, or can be called directly by C code for CPU 0.
>> + * Those two paths mandate to not use any stack and to only use registers
>> + * r0-r3 to comply with both the C ABI and the requirement of SMP startup
>> + * code.
>>    */
>>   .globl _nonsec_gic_switch
>> +.globl _smp_pen
>> +_smp_pen:
>> +	mrs	r0, cpsr
>> +	orr	r0, r0, #0xc0
>> +	msr	cpsr, r0			@ disable interrupts
>> +	mov	lr, #0				@ clear LR to mark secondary
>
> instead of this subtle abuse of lr, why not make this routine simply
> take a parameter?

How would this work if this is called out of SMP pen? Shall I rely on 
the registers being zero, then? Not very stable, I guess.
I think this whole routine is special anyways, so I felt this "subtle 
abuse" is OK.
An option would be to set r0 to 1 in the smp_pen path and pass 0 as the 
first parameter when calling from C. But then I'd need to save this 
value - possibly in the LR register ;-)

> I also slightly object against wrapping the _smp_pen around the
> _nonsec_gic_switch, I really think these are separate routines, where
> one can just call the other...?

The actual routine and the purpose are the same, just the entry and exit 
code is different. So this fitted nicely in here. I can add a more 
specific comment on the different entry points.

>>   _nonsec_gic_switch:
>>   	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
>>   	add	r3, r2, #0x1000			@ GIC dist i/f offset
>> @@ -617,5 +628,19 @@ _nonsec_gic_switch:
>>   	add	r2, r2, #0x1000			@ GIC dist i/f offset
>>   	str	r1, [r2]			@ allow private interrupts
>>
>> -	mov	pc, lr
>> +	cmp	lr, #0
>> +	movne	pc, lr				@ CPU 0 to return
>> +						@ all others: go to sleep
>> +_ack_int:
>> +	ldr	r1, [r3, #0x0c]			@ read GICD acknowledge
>> +	str	r1, [r3, #0x10]			@ write GICD EOI
>> +
>> +	adr	r1, _smp_pen
>> +waitloop:
>> +	wfi
>> +	ldr	r0, =CONFIG_SYSFLAGS_ADDR	@ load start address
>> +	ldr	r0, [r0]
>> +	cmp	r0, r1			@ make sure we dont execute this code
>
> I think I raised this issue previously, but this code is in a core
> u-boot file, but I could imagine a board with a different crazy boot
> protocol that required you to check two different fields and jump
> through other hoops to wake up from the smp pen, so I really think the
> whole smp pen belongs in a board specific place.

Right, but I didn't want to do this prematurely without knowing what is 
really needed. So my plan is to refactor this when adding Arndale 
support. I think this is special anyways because of the SPL/non-SPL split.
And although you are right about it being a core u-boot file, this whole 
code is protected by CONFIG_ARMV7_VIRT. For non-compliant boards one 
would either not enable this or add support for it.

> Also, the boot-wrapper code used wfe instead, not sure if there are any
> users that just send an event and doesn't send an IPI?

Good point. Is WFE a complete superset of WFI? Then I could just change 
this.

>> +	beq	waitloop		@ again (due to a spurious wakeup)
>> +	mov	pc, r0
>>   #endif /* CONFIG_ARMV7_VIRT */
>> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
>> index 25afffe..296dc92 100644
>> --- a/arch/arm/include/asm/armv7.h
>> +++ b/arch/arm/include/asm/armv7.h
>> @@ -78,6 +78,7 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
>>   int armv7_switch_nonsec(void);
>>
>>   /* defined in cpu/armv7/start.S */
>> +void _smp_pen(void);
>>   void _nonsec_gic_switch(void);
>>   #endif /* CONFIG_ARMV7_VIRT */
>>
>> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
>> index 3a48aee..0248010 100644
>> --- a/arch/arm/lib/virt-v7.c
>> +++ b/arch/arm/lib/virt-v7.c
>> @@ -48,6 +48,7 @@ int armv7_switch_nonsec(void)
>>   	unsigned int reg;
>>   	volatile unsigned int *gicdptr;
>>   	unsigned itlinesnr, i;
>> +	unsigned int *sysflags;
>>
>>   	/* check whether the CPU supports the security extensions */
>>   	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
>> @@ -106,7 +107,13 @@ int armv7_switch_nonsec(void)
>>   	for (i = 0; i <= itlinesnr; i++)
>>   		gicdptr[GICD_IGROUPR0 / 4 + i] = (unsigned)-1;
>>
>> -	/* call the non-sec switching code on this CPU */
>> +	/* now kick all CPUs (expect this one) by writing to GICD_SIGR */
>> +	sysflags = (void *)CONFIG_SYSFLAGS_ADDR;
>> +	sysflags[1] = (unsigned)-1;
>> +	sysflags[0] = (uintptr_t)_smp_pen;
>> +	gicdptr[GICD_SGIR / 4] = 1U << 24;
>
> here you definitely want a barrier to make sure you don't kick the cpus
> before the sysflags addresses have been written.

Right.

> What does the
> (unsigned)-1 write to sysflags[1] do?

This is either a ARM-TC or a VExpress "goodie": sysflags+0 is bit-set 
only, sysflags+4 is bit-clear only. So to write arbitrary addresses in 
here you better clear all bits first and then write in the value.
Cost me at least one day to find this out ;-)
So this should be guarded by VExpress specific defines, I guess. But 
again didn't want to do this before support for a board with a different 
behavior is actually implemented.

Regards,
Andre.

>> +
>> +	/* call the non-sec switching code on this CPU also */
>>   	_nonsec_gic_switch();
>>
>>   	return 0;
>> diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
>> index 9e230ad..210a27c 100644
>> --- a/include/configs/vexpress_ca15_tc2.h
>> +++ b/include/configs/vexpress_ca15_tc2.h
>> @@ -32,5 +32,6 @@
>>   #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
>>
>>   #define CONFIG_SYS_CLK_FREQ 24000000
>> +#define CONFIG_SYSFLAGS_ADDR 0x1c010030
>>
>>   #endif
>> --
>> 1.7.12.1
>>

[U-Boot] [PATCH 5/6] ARM: extend non-secure switch to also go into HYP mode

[Andre Przywara]

On 05/31/2013 07:43 AM, Christoffer Dall wrote:
> On Mon, May 06, 2013 at 03:17:49PM +0200, Andre Przywara wrote:
>> For the KVM and XEN hypervisors to be usable, we need to enter the
>> kernel in HYP mode. Now that we already are in non-secure state,
>> HYP mode switching is within short reach.
>>
>> While doing the non-secure switch, we have to enable the HVC
>> instruction and setup the HYP mode HVBAR (while still secure).
>>
>> The actual switch is done by dropping back from a HYP mode handler
>> without actually leaving HYP mode, so we introduce a new handler
>> routine in the exception vector table.
>>
>> In the assembly switching routine - which we rename to hyp_gic_switch
>> on the way - we save and restore the banked LR and SP registers
>> around the hypercall to do the actual HYP mode switch.
>>
>> The C routine first checks whether we are in HYP mode already and
>> also whether the virtualization extensions are available. It also
>> checks whether the HYP mode switch was finally successful.
>> The bootm command part only adds and adjusts some error reporting.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   arch/arm/cpu/armv7/start.S   | 34 +++++++++++++++++++++++-----------
>>   arch/arm/include/asm/armv7.h |  4 ++--
>>   arch/arm/lib/bootm.c         | 12 +++++++++---
>>   arch/arm/lib/virt-v7.c       | 22 +++++++++++++++-------
>>   4 files changed, 49 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>> index 02234c7..921e9d9 100644
>> --- a/arch/arm/cpu/armv7/start.S
>> +++ b/arch/arm/cpu/armv7/start.S
>> @@ -41,7 +41,7 @@ _start: b	reset
>>   	ldr	pc, _software_interrupt
>>   	ldr	pc, _prefetch_abort
>>   	ldr	pc, _data_abort
>> -	ldr	pc, _not_used
>> +	ldr	pc, _hyp_trap
>>   	ldr	pc, _irq
>>   	ldr	pc, _fiq
>>   #ifdef CONFIG_SPL_BUILD
>> @@ -49,7 +49,7 @@ _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
>> +_hyp_trap:		.word _hyp_trap
>>   _irq:			.word _irq
>>   _fiq:			.word _fiq
>>   _pad:			.word 0x12345678 /* now 16*4=64 */
>> @@ -58,7 +58,7 @@ _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
>> +_hyp_trap:		.word hyp_trap
>>   _irq:			.word irq
>>   _fiq:			.word fiq
>>   _pad:			.word 0x12345678 /* now 16*4=64 */
>> @@ -513,12 +513,18 @@ software_interrupt:
>>   	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
>>   	bic	r1, r1, #0x07f
>>   	orr	r1, r1, #0x31			@ enable NS, AW, FW
>> +	mrc	p15, 0, r0, c0, c1, 1		@ check for Virt ext
>> +	and	r0, r0, #0xf000
>> +	cmp	r0, #0x1000
>
> you can just do ands r0, r0 ,#0xf000 and change the orreq below to orrne

Can I? But I want to make sure that Virt ext is of version 1 only, 
anything beyond that I cannot reliably support, right? Or is there a 
guarantee that this is backwards compatible?

>> +	orreq	r1, r1, #0x100			@ allow HVC instruction
>>
>>   	mrc	p15, 0, r0, c12, c0, 0		@ save secure copy of VBAR
>>   	mcr	p15, 0, r1, c1, c1, 0		@ write SCR, switch to non-sec
>>   	isb
>>   	mcr	p15, 0, r0, c12, c0, 0		@ write non-secure copy of VBAR
>>
>> +	mcreq	p15, 4, r0, c12, c0, 0		@ write HYP mode HVBAR
>> +
>
> nit: s/HYP mode//
>
>>   	movs	pc, lr
>>
>>   	.align	5
>> @@ -534,10 +540,9 @@ data_abort:
>>   	bl	do_data_abort
>>
>>   	.align	5
>> -not_used:
>> -	get_bad_stack
>> -	bad_save_user_regs
>> -	bl	do_not_used
>> +hyp_trap:
>> +	.byte 0x00, 0xe3, 0x0e, 0xe1		@ mrs lr, elr_hyp
>
> do we really need to support this on assemblers that old?

What do you mean with old? I can check again, but I think it didn't work 
with my self-compiled binutils 2.23. Or do I need a directive to enable 
this?

>> +	mov pc, lr
>>
>>   #ifdef CONFIG_USE_IRQ
>>
>> @@ -574,21 +579,21 @@ fiq:
>>   #endif /* CONFIG_SPL_BUILD */
>>
>>   #ifdef CONFIG_ARMV7_VIRT
>> -/* Routine to initialize GIC CPU interface and switch to nonsecure state.
>> - * Will be executed directly by secondary CPUs after coming out of
>> +/* Routine to initialize GIC CPU interface, switch to nonsecure and to HYP
>> + * mode. Will be executed directly by secondary CPUs after coming out of
>
> So now this routine does three different things in different context at
> once, why?

Not three. At most two. But actually it does only one thing: switch a 
single core to HYP mode. Only the entry and exit points are different.
I just see that I could make the smp_pen a separate function, calling 
the switch function. This would also solve this "LR abuse" thing.
Will try this.

>>    * WFI, or can be called directly by C code for CPU 0.
>>    * Those two paths mandate to not use any stack and to only use registers
>>    * r0-r3 to comply with both the C ABI and the requirement of SMP startup
>>    * code.
>>    */
>> -.globl _nonsec_gic_switch
>> +.globl _hyp_gic_switch
>>   .globl _smp_pen
>>   _smp_pen:
>>   	mrs	r0, cpsr
>>   	orr	r0, r0, #0xc0
>>   	msr	cpsr, r0			@ disable interrupts
>>   	mov	lr, #0				@ clear LR to mark secondary
>> -_nonsec_gic_switch:
>> +_hyp_gic_switch:
>>   	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
>>   	add	r3, r2, #0x1000			@ GIC dist i/f offset
>>   	mvn	r1, #0
>> @@ -628,6 +633,13 @@ _nonsec_gic_switch:
>>   	add	r2, r2, #0x1000			@ GIC dist i/f offset
>>   	str	r1, [r2]			@ allow private interrupts
>>
>> +	mov	r2, lr
>> +	mov	r1, sp
>> +	.byte 0x70, 0x00, 0x40, 0xe1		@ hvc #0
>> +	isb
>
> again, I'm doubtful this isb is necessary when you just did an exception
> return.

Good point. Exception returns should do this automatically, right? Is 
this an official fact or just a feeling?

...

>> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
>> index 0248010..3883463 100644
>> --- a/arch/arm/lib/virt-v7.c
>> +++ b/arch/arm/lib/virt-v7.c
>> @@ -3,6 +3,7 @@
>>    * Andre Przywara, Linaro
>>    *
>>    * routines to push ARMv7 processors from secure into non-secure state
>> + * and from non-secure SVC into HYP mode
>>    * needed to enable ARMv7 virtualization for current hypervisors
>>    *
>>    * See file CREDITS for list of people who contributed to this
>> @@ -43,16 +44,20 @@ static inline unsigned int read_cpsr(void)
>>   	return reg;
>>   }
>>
>> -int armv7_switch_nonsec(void)
>> +int armv7_switch_hyp(void)
>>   {
>>   	unsigned int reg;
>>   	volatile unsigned int *gicdptr;
>>   	unsigned itlinesnr, i;
>>   	unsigned int *sysflags;
>>
>> -	/* check whether the CPU supports the security extensions */
>> +	/* check whether we are in HYP mode already */
>> +	if ((read_cpsr() & 0x1F) == 0x1a)
>> +		return 1;
>> +
>> +	/* check whether the CPU supports the virtualization extensions */
>>   	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
>> -	if ((reg & 0xF0) == 0)
>> +	if ((reg & 0xF000) != 0x1000)
>>   		return 2;
>>
>>   	/* the timer frequency for the generic timer needs to be
>> @@ -73,8 +78,8 @@ int armv7_switch_nonsec(void)
>>   	 */
>>
>>   	/* check whether we are an Cortex-A15 or A7.
>> -	 * The actual non-secure switch should work with all CPUs supporting
>> -	 * the security extension, but we need the GIC address,
>> +	 * The actual HYP switch should work with all CPUs supporting
>> +	 * the virtualization extension, but we need the GIC address,
>>   	 * which we know only for sure for those two CPUs.
>>   	 */
>>   	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(reg));
>> @@ -113,8 +118,11 @@ int armv7_switch_nonsec(void)
>>   	sysflags[0] = (uintptr_t)_smp_pen;
>>   	gicdptr[GICD_SGIR / 4] = 1U << 24;
>>
>> -	/* call the non-sec switching code on this CPU also */
>> -	_nonsec_gic_switch();
>> +	/* call the HYP switching code on this CPU also */
>> +	_hyp_gic_switch();
>> +
>> +	if ((read_cpsr() & 0x1F) != 0x1a)
>> +		return 5;
>
> this is really a fatal crash right? We probably don't want to try and
> proceed with boot at this point.

Well not really. If we reach this point without being in HYP mode, that 
just hasn't worked - for various reasons. Since this new bootm 
functionality is unconditional, I felt we should better not crash. 
Actually Linux will run fine, just without KVM (for certains meanings of 
"fine" that is - as a KVM developer ;-)


Regards,
Andre.

>
>>
>>   	return 0;
>>   }
>> --
>> 1.7.12.1
>>

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Albert ARIBAUD]

Hi Andre,

On Fri, 31 May 2013 11:23:16 +0200, Andre Przywara
<andre.przywara@linaro.org> wrote:

> software_interrupt is currently a panic routine. So it is not actually 
> used by u-boot, it's just there to dump some state and eventually call 
> reset_cpu().
> So I feel that since I am now the only user of software_interrupt I 
> don't need any special precautions and consider this routine now 
> actually part of the HYP mode switcher. But of course I can retain the 
> original "functionality" if CONFIG_ARMV7_VIRT is not defined.

You should, actually. Rule #1 goes (*) 'a new config option should
have zero effect on the code when not defined'.

> Regards,
> Andre.

(*) Rule #1 actually goes, 'there is an infinite number of "Rule #1"',
but that's slightly beyond the point.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 0/6] ARMv7: Add HYP mode switching support

[Christoffer Dall]

On Fri, May 31, 2013 at 08:36:12AM +0200, Andre Przywara wrote:
> On 05/31/2013 08:11 AM, Christoffer Dall wrote:
> > On Mon, May 06, 2013 at 03:17:44PM +0200, Andre Przywara wrote:
> >> (for GIT URL and Changelog see below)
> >>
> >> ARM CPUs with the virtualization extension have a new mode called
> >> HYP mode, which allows hypervisors to safely control and monitor
> >> guests. The current hypervisor (KVM and Xen) implementations
> >> require the kernel to be entered in that HYP mode.
> >>
> >> This patch series introduces a configuration variable
> >> CONFIG_ARMV7_VIRT which enables code to switch all cores into HYP
> >> mode. This is done automatically during execution of the bootm
> >> command (but could also be done earlier - U-Boot runs fine in HYP
> >> mode without MMU usage).
> >
> > I forget the u-boot specifics here, but if you boot over networking, for
> > example, does that eventually end up calling bootm or would Hyp mode not
> > be entered in this case?
> 
> Despite the naming "bootm" is eventually always called when booting 
> Linux, even bootz is a wrapper around this. Same with network boot. So 
> unless you show me a case where this isn't, I think this is fine.
> 
Cool, I have no idea, I just wanted to be sure.

Thanks,
-Christoffer

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Christoffer Dall]

On Fri, May 31, 2013 at 11:23:16AM +0200, Andre Przywara wrote:
> On 05/31/2013 03:02 AM, Christoffer Dall wrote:
> 
> Christoffer,
> 
> thanks a lot for the thorough review. Comments inline.
> 
> >On Mon, May 06, 2013 at 03:17:45PM +0200, Andre Przywara wrote:
> >>A prerequisite for using virtualization is to be in HYP mode, which
> >>requires the CPU to be in non-secure state.
> >>Introduce a monitor handler routine which switches the CPU to
> >>non-secure state by setting the NS and associated bits.
> >>According to the ARM ARM this should not be done in SVC mode, so we
> >>have to setup a SMC handler for this. We reuse the current vector
> >>table for this and make sure that we only access the MVBAR register
> >>if the CPU supports the security extension and only if we
> >>configured the board to use it, since boards entering u-boot already
> >>in non-secure mode would crash on accessing MVBAR otherwise.
> >>
> >>Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >>---
> >>  arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++---
> >>  1 file changed, 28 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> >>index e9e57e6..da48b36 100644
> >>--- a/arch/arm/cpu/armv7/start.S
> >>+++ b/arch/arm/cpu/armv7/start.S
> >>@@ -155,6 +155,13 @@ reset:
> >>  	/* Set vector address in CP15 VBAR register */
> >>  	ldr	r0, =_start
> >>  	mcr	p15, 0, r0, c12, c0, 0	@Set VBAR
> >>+
> >>+#ifdef CONFIG_ARMV7_VIRT
> >>+	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
> >>+	ands	r1, r1, #0x30
> >>+	mcrne	p15, 0, r0, c12, c0, 1	@ Set secure monitor MVBAR
> >
> >Hmm, this smells a bit simplified to me.
> >
> >Support for ARMv7_VIRT should easy to integrate into u-boot even for
> >platforms that do not boot U-boot directly into secure mode (OMAP5 GP
> >platforms are such an example).  In this case you cannot assume that you
> >can write the secure monitor mvbar.
> 
> Right, Albert kind of hinted on this already. I already fixed this
> in my private tree, totally removing these MVBAR writes here and
> instead copying the values from VBAR later just before we do the
> smc.
> Will send out a fixed version.
> 
> >>+#endif
> >>+
> >>  #endif
> >>
> >>  	/* the mask ROM code should have PLL and others stable */
> >>@@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup)
> >>  	ldr     r0, =_start
> >>  	mcr     p15, 0, r0, c12, c0, 0  @Set VBAR
> >>
> >>+#ifdef CONFIG_ARMV7_VIRT
> >>+	mrc	p15, 0, r1, c0, c1, 1	@ check for security extension
> >>+	ands	r1, r1, #0x30
> >>+	mcrne	p15, 0, r0, c12, c0, 1  @ Set secure monitor MVBAR
> >>+#endif
> >>+
> >>  	bx	lr
> >>
> >>  ENDPROC(c_runtime_cpu_setup)
> >>@@ -490,11 +503,23 @@ undefined_instruction:
> >>  	bad_save_user_regs
> >>  	bl	do_undefined_instruction
> >>
> >>+/*
> >>+ * software interrupt aka. secure monitor handler
> >>+ * This is executed on a "smc" instruction, we use a "smc #0" to switch
> >>+ * to non-secure state
> >>+ */
> >>  	.align	5
> >>  software_interrupt:
> >>-	get_bad_stack_swi
> >>-	bad_save_user_regs
> >>-	bl	do_software_interrupt
> >
> >Why is the following block not conditional on CONFIG_ARMV7_VIRT?
> >
> >Again, it feels a bit funny to modify this generic mechanism to contain
> >this code for boards that boot in NS mode but have a way to enter Hyp
> >mode using an HVC or SMC instruction.
> 
> software_interrupt is currently a panic routine. So it is not
> actually used by u-boot, it's just there to dump some state and
> eventually call reset_cpu().

Which is pretty useful to catch if something went wrong.

> So I feel that since I am now the only user of software_interrupt I
> don't need any special precautions and consider this routine now
> actually part of the HYP mode switcher. But of course I can retain
> the original "functionality" if CONFIG_ARMV7_VIRT is not defined.
> 

Yeah, I don't really think it's cool if a software interrupt happens
somewhere completely else in the system that we do this dance, which
could be super hard to detect, which is why I'm not even a fan of
re-using the vector in the first place.

> >>+	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
> >>+	bic	r1, r1, #0x07f
> >>+	orr	r1, r1, #0x31			@ enable NS, AW, FW
> >
> >Are you sure you want to always route FIQ to non-secure here?
> 
> Since we actually don't install any secure software and just use the
> secure state to do the HYP switch I don't feel like we should
> restrict FIQ. Since we don't use it for our own purposes, no one
> would be able to use it then, right? So my idea was to allow as much
> as possible.

Yeah, makes sense.

> 
> >Don't you need to set the HCE bit?  The whole register resets to
> >0register resets to zero.
> 
> This is added later in 5/6. For reviewing purposes I split the
> patches up to do the non-secure switch only first. Later I add the
> bits to actually go to HYP mode.

The splitting of patches is fine, but it would be helpful to explain the
scope a little more in the commit text perhaps, maybe I'm just being
silly.

> 
> >>+
> >>+	mrc	p15, 0, r0, c12, c0, 0		@ save secure copy of VBAR
> >>+	mcr	p15, 0, r1, c1, c1, 0		@ write SCR, switch to non-sec
> >
> >Not quite a "swith to non-sec"; you're still in monitor mode.
> 
> Right, _non-secure_ monitor mode. If I got this thing correctly,
> then secure/non-secure is a state, not a mode. Switching from secure
> to non-secure can only be done in monitor mode. And the state
> totally depends on the NS bit in SCR, so by setting this we enter
> non-secure world immediately.

I think the ARM ARM is pretty clear on the fact that the NS bit in the
SCR determine the secure state, *except* from monitor mode, which is
always in secure state:

  "Software executing in Monitor mode executes in the Secure state
  regardless of the value of the SCR.NS bit."
                               ARM ARM, DDI 0406C.b, B1-1157

> 
> >>+	isb
> >>+	mcr	p15, 0, r0, c12, c0, 0		@ write non-secure copy of VBAR
> >
> >I don't actually think that you are, I think you're writing the secure
> >copy here.
> 
> With the mcr above I set the NS bit, so I am non-secure from that
> point on (hence the isb). Writing the VBAR with the NS bit set
> should set the non-secure copy. Not doing this was a problem I
> chased down for some days, so I believe this is correct.

Hmmm, that seems to contradict the quote from above.  Did you verify
this emprically?  If so, maybe there's a special caveat for writing
banked co-processor registers (I doubt it though).

> 
> >In that case, I'm also wondering if the isb is superflous, because we
> >perform an exception return below, but we of course want to make damn
> >sure that the write of the NS bit is set before the exception return,
> >maybe some ARM guys have the right expertise here.

thinking about it, it really shouldn't be necessary, because the movs
implies all that's necessary.

> >
> >>+
> >>+	movs	pc, lr
> >
> >This movs is pretty drastic, because it changes from secure to
> >non-secure world, and yes, you can tell by looking at the orr
> >instruction above, but I would prefer a (potentially big fat) comment
> >here as well.
> 
> OK.
> 
> Regards,
> Andre.
> 
> >
> >>
> >>  	.align	5
> >>  prefetch_abort:
> >>--
> >>1.7.12.1
> >>
> 

[U-Boot] [PATCH 2/6] ARM: add assembly routine to switch to non-secure state

[Christoffer Dall]

On Fri, May 31, 2013 at 11:26:06AM +0200, Andre Przywara wrote:
> On 05/31/2013 05:04 AM, Christoffer Dall wrote:
> >On Mon, May 06, 2013 at 03:17:46PM +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
> >>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 register. We check explicitly for
> >>ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those
> >>cores we know the offsets for the GIC CPU interface from the
> >>PERIPHBASE content. Other cores could be added as needed.
> >>
> >>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 = 0x80)
> >>
> >>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.
> >>
> >>For reasons obvious later we only use caller saved registers r0-r3.
> >
> >You probably want to put that in a comment in the code, and it would
> >also be super helpful to explain the obvious part here, because most
> >readers don't look "forward in time" to understand this patch...
> 
> Agreed.
> 
> >>
> >>Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >>---
> >>  arch/arm/cpu/armv7/start.S | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 47 insertions(+)
> >>
> >>diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> >>index da48b36..e63e892 100644
> >>--- a/arch/arm/cpu/armv7/start.S
> >>+++ b/arch/arm/cpu/armv7/start.S
> >>@@ -572,3 +572,50 @@ fiq:
> >>
> >>  #endif /* CONFIG_USE_IRQ */
> >>  #endif /* CONFIG_SPL_BUILD */
> >>+
> >>+#ifdef CONFIG_ARMV7_VIRT
> >>+/* Routine to initialize GIC CPU interface and switch to nonsecure state.
> >>+ */
> >>+.globl _nonsec_gic_switch
> >>+_nonsec_gic_switch:
> >>+	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
> >
> >You should probably check if bits [7:0] == 0x00 here, otherwise you may
> >end up doing some very strange stuff - if any of those bits are set you
> >can just error out at this point, but it should be gracefully handled.
> >
> >Also since it's core specific, you'd probably want to check that before
> >using it.
> 
> As you found out later, I am doing this before calling this routine.
> But I will add a comment here to avoid confusion.
> 

yeah, or just expect that this address is in r0 upon calling the
routine, then you're in the clear.

> >>+	add	r3, r2, #0x1000			@ GIC dist i/f offset
> >
> >Since this is core specific, could the offset please go in an
> >appropriate header file?  It will also eliminate the need for the
> >comment if you just have a proper define (i.e. GIC_DIST_OFFSET ...)
> >
> >>+	mvn	r1, #0
> >>+	str	r1, [r3, #0x80]			@ allow private interrupts
> >
> >Aren't you makeing an assumption about the number of available
> >interrupts here?  You should read the ITLinesNumber field from the
> >GICD_TYPER if I'm not mistaking.
> 
> This is the per core private interrupts. All bits should be implemented.
> From the GIC spec, chapter 4.3.4:
> "In a multiprocessor implementation, GICD_IGROUPR0 is banked for
> each connected processor. This register holds the group status bits
> for interrupts 0-31."
> 

I understand it, but the comments or naming of the routine never
suggested that this was the code that was called per-core.  I really
think that is the core objective of this function: The NS-init that each
core must do, it's not really GIC specific, so I suggest you rename it.

> >I also think it would be much cleaner if you again used a define for the
> >0x80 offset.
> >
> >Also, don't you need to set some enable fields on the GICD_CTLR here to
> >enable group 1 interrupts?
> 
> Since this a non-banked per-system register, I do this later in the C part.
> 

later in the patch series, before in the flow of the code, right? :)

> >>+
> >>+	mrc	p15, 0, r0, c0, c0, 0		@ MIDR
> >>+	bfc	r0, #16, #8			@ mask out variant, arch
> >>+	bfc	r0, #0, #4			@ and revision
> >>+	movw	r1, #0xc070
> >>+	movt	r1, #0x4100
> >>+	cmp	r0, r1				@ check for Cortex-A7
> >>+	orr	r1, #0xf0
> >
> >wow, nice bit fiddling.  It may be quite a bit easier to read if you
> >simply had defines for the bitmasks and real values and just did a load
> >and placed a literal section accordingly.
> 
> The sequence is necessary since we are short on registers. I agree
> it is a bit obfuscated, will make it more readable.
> 

yeah but:

#define ARM_CORTEX_A15_ID	0x4100c070

.ltorg
[...]
ldr	r0, =ARM_CORTEX_A15_ID	

only uses a single register as well.


> >>+	cmpne	r0, r1				@ check for Cortex-A15
> >>+	movne	r1, #0x100			@ GIC CPU offset for A9
> >
> >So I read the ARMV7_VIRT config flag as something you can only enable on
> >a board that actually supports the virtulization extensions, which the
> >A9 doesn't so this should probably error out instead (or do an endless
> >loop, or ignore the case in the code, ...).
> 
> Yeah, originally I had the idea to support a non-sec switch only on
> non-virt capable CPUs. My first version had a separate non-sec
> switch command. This here is kind of a leftover of that early
> version. But until patch 5/6 is applied this actually works (and we
> had a use-case internally here), so I decided to leave this in.
> 

I think it's a good idea (as I said on one of the other patches) to
follow your initial approach, mainly because it's going to make the code
easier to read and understand, and I think you're going to pay only a
penalty for a few call in/out instructions in terms of code size.  But
then you need to introduce two different config options.

> >>+	moveq	r1, #0x2000			@ GIC CPU offset for A15/A7
> >
> >Again, I think defines for these are appropriate.
> 
> Good point. Will fix this.
> 
> >>+	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
> >>+
> >>+	mov	r1, #1
> >>+	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
> >>+	mov	r1, #0x80
> >
> >Why are you not setting this to #0xff ?
> 
> Because a certain Christoffer Dall did this the same way in his
> Arndale specific patch ;-)

lol, got me.

> +       /* Set GIC priority mask bit [7] = 1 */
> +       addr = EXYNOS5_GIC_CPU_BASE;
> +       writel(0x80, addr + ARM_GICV2_ICCPMR);
> (and I remember having read this recommendation somewhere)
> 

I may have read the Arndale data sheet and seen that there was a max of
128 interrupts, but I really don't remember why - it was clear from the
get go that my patch was just a fast path from Arndale.

But as I read the GIC2 specs, if there are less than 256 interrupts,
those other bits will just be RAZ/WI.

> >>+	str	r1, [r3, #4]			@ set GICC_PIMR[7]
> >>+
> >
> >here it seems we're moving into non-gic related stuff in a function
> >called _nonsec_gic_switch
> 
> Right, but this is per CPU and needs to be done in secure monitor
> mode, so it belongs here. Shall I rename the function to be called
> non_secure_init or the like?
> 

cpu_non_secure_init would be a good name.

> >
> >>+	movw	r1, #0x3fff
> >>+	movt	r1, #0x0006
> >>+	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
> >>+
> >>+	ldr	r1, =_start
> >>+	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
> >>+	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR
> >
> >I thought you already took care of the MVBAR during init?
> 
> Right, as mentioned earlier I have fixed this already.
> 
> >>+
> >>+	isb
> >>+	smc	#0				@ call into MONITOR mode
> >>+	isb					@ clobbers r0 and r1
> >
> >This isb shouldn't be necessary if you just did an exception return?
> 
> Seems to be a paranoid leftover of one debug session...
> 

actually, neither isb should be necessary...

> >>+
> >>+	mov	r1, #1
> >>+	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
> >
> >you're doing more than setting the enable bit: you're setting the EOImodeNS,
> >IRQBypDisGrp1, and FIQBypDisGrp1, as you should.
> 
> But 0x01 is the correct value? And the reset value is 0, right?
> I will extend the comment.
> 

yeah my comment was only in reference to the code comment, the code is
correct.

> >>+	add	r2, r2, #0x1000			@ GIC dist i/f offset
> >>+	str	r1, [r2]			@ allow private interrupts
> >
> >It seems a bit brittle to rely on the smc handler to not clobber r2 and
> >r3, and it may an annoying thing to track down.  You could just
> >re-generate the the gic base address from the cp15 register.  Your
> >choice.
> 
> So shall I put comments here and at the smc handler?
> 

hmm, so I was thinking before that you should not rely on whatever the
smc handler does, but with the code that's added later, and especially
if you change this routine to take parameters, then it's fine as it is,
and we should just make sure the smc routine follows the AAPCS.


Thanks,
-Christoffer

[U-Boot] [PATCH 3/6] ARM: switch to non-secure state during bootm execution

[Christoffer Dall]

On Fri, May 31, 2013 at 11:30:32AM +0200, Andre Przywara wrote:
> On 05/31/2013 07:10 AM, Christoffer Dall wrote:
> >On Mon, May 06, 2013 at 03:17:47PM +0200, Andre Przywara wrote:
> >>To actually trigger the non-secure switch we just implemented, call
> >>the switching routine from within the bootm command implementation.
> >>This way we automatically enable this feature without further user
> >>intervention.
> >>
> >>Some part of the work is done in the assembly routine in start.S,
> >>introduced with the previous patch, but for the full glory we need
> >>to setup the GIC distributor interface once for the whole system,
> >>which is done in C here.
> >>The routine is placed in arch/arm/lib to allow easy access from
> >>different boards or CPUs.
> >
> >I'm beginning to loose track of exactly which parts is in assembly and
> >which parts are in C, and why.  We are fiddling with some gic dist.
> >settings in assembly, and some of them in C as well.
> 
> I fear I dropped the explanation some time during a patch split earlier.
> So the assembly code is the per core part - including GICD_IGROUPR0,
> which is banked per core. The reason this is in assembly is to make
> it easily run right out of the SMP pen.
> 
> In C I do anything that needs to be only done once for the whole system.
> 
> More comments inline...
> 

I think renaming the assembly routine will go a along way.  Ordering
this patch before the assembly patch with just a stub function
implementation may also have been easier to read, but that's easy for me
to say at this point.

> >I think it's just the ordering or naming of the patches that is a little
> >confusing.
> >
> >>
> >>First we check for the availability of the security extensions.
> >>
> >>The generic timer base frequency register is only accessible from
> >>secure state, so we have to program it now. Actually this should be
> >>done from primary firmware before, but some boards seems to omit
> >>this, so if needed we do this here with a board specific value.
> >>
> >>Since we need a safe way to access the GIC, we use the PERIPHBASE
> >>registers on Cortex-A15 and A7 CPUs and do some sanity checks.
> >>
> >>Then we actually do the GIC enablement:
> >>a) enable the GIC distributor, both for non-secure and secure state
> >>    (GICD_CTLR[1:0] = 11b)
> >>b) allow all interrupts to be handled from non-secure state
> >>    (GICD_IGROUPRn = 0xFFFFFFFF)
> >>The core specific GIC setup is then done in the assembly routine.
> >>
> >>The actual bootm trigger is pretty small: calling the routine and
> >>doing some error reporting. A return value of 1 will be added later.
> >>
> >>To enable the whole code we introduce the CONFIG_ARMV7_VIRT variable.
> >>
> >>Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >>---
> >>  arch/arm/include/asm/armv7.h |   7 +++
> >>  arch/arm/lib/Makefile        |   2 +
> >>  arch/arm/lib/bootm.c         |  20 ++++++++
> >>  arch/arm/lib/virt-v7.c       | 113 +++++++++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 142 insertions(+)
> >>  create mode 100644 arch/arm/lib/virt-v7.c
> >>
> >>diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> >>index a73630b..25afffe 100644
> >>--- a/arch/arm/include/asm/armv7.h
> >>+++ b/arch/arm/include/asm/armv7.h
> >>@@ -74,4 +74,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
> >>+int armv7_switch_nonsec(void);
> >>+
> >>+/* defined in cpu/armv7/start.S */
> >>+void _nonsec_gic_switch(void);
> >>+#endif /* CONFIG_ARMV7_VIRT */
> >>+
> >>  #endif
> >>diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> >>index 6ae161a..37a0e71 100644
> >>--- a/arch/arm/lib/Makefile
> >>+++ b/arch/arm/lib/Makefile
> >>@@ -58,6 +58,8 @@ COBJS-y	+= reset.o
> >>  COBJS-y	+= cache.o
> >>  COBJS-y	+= cache-cp15.o
> >>
> >>+COBJS-$(CONFIG_ARMV7_VIRT) += virt-v7.o
> >>+
> >>  SRCS	:= $(GLSOBJS:.o=.S) $(GLCOBJS:.o=.c) \
> >>  	   $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
> >>  OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
> >>diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> >>index f3b30c5..a3d3aae 100644
> >>--- a/arch/arm/lib/bootm.c
> >>+++ b/arch/arm/lib/bootm.c
> >>@@ -35,6 +35,10 @@
> >>  #include <asm/bootm.h>
> >>  #include <linux/compiler.h>
> >>
> >>+#ifdef CONFIG_ARMV7_VIRT
> >>+#include <asm/armv7.h>
> >>+#endif
> >>+
> >>  DECLARE_GLOBAL_DATA_PTR;
> >>
> >>  #if defined(CONFIG_SETUP_MEMORY_TAGS) || \
> >>@@ -319,6 +323,22 @@ static void boot_prep_linux(bootm_headers_t *images)
> >>  		hang();
> >>  #endif /* all tags */
> >>  	}
> >>+#ifdef CONFIG_ARMV7_VIRT
> >>+	switch (armv7_switch_nonsec()) {
> >>+	case 0:
> >>+		debug("entered non-secure state\n");
> >>+		break;
> >>+	case 2:
> >>+		printf("HYP mode: Security extensions not implemented.\n");
> >>+		break;
> >>+	case 3:
> >>+		printf("HYP mode: CPU not supported (must be Cortex-A15 or A7).\n");
> >
> >I would drop the specifics of what's supported here.
> >
> 
> This is in particular here since I rely on the PERIPHBASE register,
> which is A15/A7 implementation specific. This is different from case
> 2 (which will later be changed to "Virtualization extensions...")
> 

I'm just thinking this will be weird when there's added Krait (I know, I
know )support or something else, and this comment is not updated, and
everyone is confused.  But it's also helpful, I guess, your call.

> >>+		break;
> >>+	case 4:
> >>+		printf("HYP mode: PERIPHBASE is above 4 GB, cannot access this.\n");
> >>+		break;
> >>+	}
> >
> >I think these hard-coded numbers are a bit ugly, either define an enum
> >or some defines somewhere, or just make the armv7_switch_nonsec return a
> >boolean value and put the prints in there.
> >
> >That will also make it easier to read the other function and not go
> >"return 2" hmmm, I wonder what that means ;)
> 
> Right, will fix this.
> 
> >>+#endif
> >>  }
> >>
> >>  /* Subcommand: GO */
> >>diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
> >>new file mode 100644
> >>index 0000000..3a48aee
> >>--- /dev/null
> >>+++ b/arch/arm/lib/virt-v7.c
> >>@@ -0,0 +1,113 @@
> >>+/*
> >>+ * (C) Copyright 2013
> >>+ * Andre Przywara, Linaro
> >>+ *
> >>+ * routines to push ARMv7 processors from secure into non-secure state
> >>+ * needed to enable ARMv7 virtualization for current hypervisors
> >
> >Nits: Routines should be capitalized.  Also not completely sure about
> >the wording about pushing between secure and non-secure state, changing,
> >transitioning, seems like more commonly used terms, but I dont' feel
> >strongly about any of this.
> >
> >Again, I really think this is the wrong way to go about it.
> >Transitioning from secure to non-secure is really a feature of its own
> >which is a useful feature in U-boot.  Orthogonal to that is the need to
> >boot kernels in Hyp-mode, and being booted in secure more and
> >controlling all of the non-secure worlds is just one scenario for that.
> 
> OK. Originally I had an implementation which separated the non-sec
> switch and the HYP switch. Later I got the impression that there is
> no real use-case for a non-sec switch only, so I dropped the
> specific command and just kept the logical separation in the patch
> split. That simplified the patches quite a bit.

Really, did it simplify it a lot? I would see almost the identical lines
of code, just placed elsewhere and instead of one assembly instruction
falling through into the other one, there would be an explicit branch.
I'm sure that code will be easier to read, because right now that
assembly routine does *a lot* of things.

> 
> So do you want to have a new u-boot command switching to non-secure
> state? I think that would make the patches more complicated, but if
> you recommend to have such a thing, I am happy to provide it.

No, I just want the cpu_switch_to_non_secure function to do just that,
and be a separately contained function (preferebly in its own file as
well; there's no need for anyone else trying to understand U-boot
unrelated of Hyp to read through all that).

So instead of what you have now, you would have something like this:

@ in secure_smp.S

secondary_non_secure_init:
	mrc     p15, 4, r1, c15, c0, 0		@ r1 = PREIPHBASE
	mov	r0, #0
	bl	cpu_non_secure_init
	b	smp_pen

smp_pen:
	b	smp_pen		@ This is a poor smp_pen


@ in secure_boot.S
/**
 * void cpu_non_secure_init(bool boot_cpu, unsigned periphbase)
 *
 * Blah blah blah...
 */
cpu_non_secure_init:
	...
	mov	pc, lr


/* in secure_boot.c */

int armv7_switch_nonsec(void)
{

	...

	writel(vexpress_sysflags + 4, 0xffffffff);
	writel(vexpress_sysflags, secondary_non_secure_init);

	cpu_non_secure_init(true, periphbase);
};

> Core problem here is that there is no easy way to check whether you
> are non-secure. Accessing the SCR register will trap if the NS bit
> is set. I could just catch this and return back with an error
> condition.

But that's an orthogonal issue though right?  There is no good way to
determine if you're in secure on non-secure mode, afaik, so there we
simply have to rely on proper configuration of the boot loader per
board, which is exactly why this is not done in the kernel.

> Just not sure this is really worth the effort.

Meta comment: I think making critical code like this as readable as
possible is almost always worth the effort, for the educational purpose
for other readers, to save time for everyone, and for our own sanity
when we have to go back and debug/extend the code in a couple of years.



> 
> Also leaves one question: How to handle if the users switched
> deliberately to non-secure before and now the bootm routine wants to
> go to HYP mode? Do we want to stay in SVC in this case?
> 

No no, I never wanted users to have a separate command, just structure
the code differently.

> >>+ *
> >>+ * See file CREDITS for list of people who contributed to this
> >>+ * project.
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or
> >>+ * modify it under the terms of the GNU General Public License as
> >>+ * published by the Free Software Foundation; either version 2 of
> >>+ * the License, or (at your option) any later version.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>+ * GNU General Public License for more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License
> >>+ * along with this program; if not, write to the Free Software
> >>+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> >>+ * MA 02111-1307 USA
> >>+ */
> >>+
> >>+#include <common.h>
> >>+#include <asm/armv7.h>
> >>+
> >>+#define GICD_CTLR	0x000
> >>+#define GICD_TYPER	0x004
> >>+#define GICD_IGROUPR0	0x080
> >>+#define GICD_SGIR	0xf00
> >>+
> >>+#define CPU_ARM_CORTEX_A15	0x4100c0f0
> >>+#define CPU_ARM_CORTEX_A7	0x4100c070
> >>+
> >>+static inline unsigned int read_cpsr(void)
> >
> >inline is typically not used in C-files - at least not in the kernel.
> >GCC is pretty smart about this on its own.
> 
> Right. I think newer GCCs inline short static functions automatically.
> 

Depends a bit on the optimization flag, but yes, I was once told that
using the inline keyword on C files on any compiler rolled after the
90's was considered bad style :)

> >>+{
> >>+	unsigned int reg;
> >>+
> >>+	asm volatile ("mrs %0, cpsr\n" : "=r" (reg));
> >>+	return reg;
> >>+}
> >>+
> >>+int armv7_switch_nonsec(void)
> >>+{
> >>+	unsigned int reg;
> >>+	volatile unsigned int *gicdptr;
> >>+	unsigned itlinesnr, i;
> >>+
> >>+	/* check whether the CPU supports the security extensions */
> >>+	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> >
> >My brain hasn't managed to learn all the coprocessor register by heart
> >yet, so if you could provide the name for these registers it would be
> >cool.  Alternatively you could just create nice static little functions
> >just like you do with the cpsr.
> 
> OK. Actually I found this quite some overhead if its only called
> once (cpsr is called twice). Would just a comment suffice?
> 

sure, see whatever is prettiest. I had a slight feeling that this
function would read more nicely with some accessor functions, but it's
your call.

> >>+	if ((reg & 0xF0) == 0)
> >>+		return 2;
> >>+
> >>+	/* the timer frequency for the generic timer needs to be
> >>+	 * programmed still in secure state, should be done by firmware.
> >
> >nit: drop 'still'
> >nit: the 'should be done by firmware' is not very descriptive, consider
> >stating clearly that this is a work-around for broken bootloaders.
> 
> ACK.
> 
> >>+	 * check whether we have the generic timer first
> >>+	 */
> >>+#ifdef CONFIG_SYS_CLK_FREQ
> >>+	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> >>+	if ((reg & 0xF0000) == 0x10000)
> >>+		asm("mcr p15, 0, %0, c14, c0, 0\n"
> >>+		: : "r"(CONFIG_SYS_CLK_FREQ));
> >>+#endif
> >>+
> >>+	/* the SCR register will be set directly in the monitor mode handler,
> >>+	 * according to the spec one should not tinker with it in secure state
> >>+	 * in SVC mode. Do not try to read it once in non-secure state,
> >>+	 * any access to it will trap.
> >>+	 */
> >>+
> >>+	/* check whether we are an Cortex-A15 or A7.
> >
> >nit: s/whether/if/
> >
> >>+	 * The actual non-secure switch should work with all CPUs supporting
> >>+	 * the security extension, but we need the GIC address,
> >>+	 * which we know only for sure for those two CPUs.
> >>+	 */
> >>+	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(reg));
> >>+	if (((reg & 0xFF00FFF0) != 0x4100C0F0) &&
> >>+	    ((reg & 0xFF00FFF0) != 0x4100C070))
> >
> >Are there really no defines for this in U-boot already?
> >
> >It seems to me that a static inline in a header file somewhere that
> >gives you a processor type back would be useful for other things as
> >well, but I don't know U-boot enough to properly say...
> 
> You are right, this is barely readable. Will fix this.
> 
> >>+		return 3;
> >>+
> >>+	/* get the GIC base address from the A15 PERIPHBASE register */
> >>+	asm("mrc p15, 4, %0, c15, c0, 0\n" : "=r" (reg));
> >>+
> >>+	/* the PERIPHBASE can be mapped above 4 GB (lower 8 bits used to
> >>+	 * encode this). Bail out here since we cannot access this without
> >>+	 * enabling paging.
> >>+	 */
> >
> >ah, here you check for it...
> >
> >>+	if ((reg & 0xff) != 0)
> >>+		return 4;
> >
> >if you're getting the PERIPHBASE here anyway, why not just pass it as a
> >parameter to the non-secure gic init routine?
> 
> Because this routine is also called directly out of the SMP pen, so
> I cannot pass any parameters there.
> 

you can just read that value once before you start looping in the smp
pen and pass it as an argument, see my example above.

> >>+
> >>+	/* GIC distributor registers start at offset 0x1000 */
> >>+	gicdptr = (unsigned *)(reg + 0x1000);
> >>+
> >>+	/* enable the GIC distributor */
> >>+	gicdptr[GICD_CTLR / 4] |= 0x03;
> >
> >so this is I/O accesses, so you could consider using readl for
> >consistency, which would also get rid of the division in the array
> >element accessors.
> 
> Good hint, thanks.
> 
> >>+
> >>+	/* TYPER[4:0] contains an encoded number of all interrupts */
> >>+	itlinesnr = gicdptr[GICD_TYPER / 4] & 0x1f;
> >>+
> >>+	/* set all bits in the GIC group registers to one to allow access
> >>+	 * from non-secure state
> >>+	 */
> >>+	for (i = 0; i <= itlinesnr; i++)
> >>+		gicdptr[GICD_IGROUPR0 / 4 + i] = (unsigned)-1;
> >
> >didn't you also do this in the assembly routine for IGROUP0 only?
> >
> >oh wait, it's dawning on me, the _nonsec_gic_switch is actually per-cpu
> >non-secure init, right?
> 
> Right. I take this as an advice to explain this earlier...
> 
More an advice to rename the assembly routine.

Thanks,
-Christoffer

[U-Boot] [PATCH 4/6] ARM: add SMP support for non-secure switch

[Christoffer Dall]

On Fri, May 31, 2013 at 11:32:40AM +0200, Andre Przywara wrote:
> On 05/31/2013 07:32 AM, Christoffer Dall wrote:
> > On Mon, May 06, 2013 at 03:17:48PM +0200, Andre Przywara wrote:
> >> Currently the non-secure switch is only done for the boot processor.
> >> To later allow full SMP support, we have to switch all secondary
> >> cores into non-secure state also.
> >>
> >> So we add an entry point for secondary CPUs coming out of low-power
> >> state and make sure we put them into WFI again after having switched
> >> to non-secure state.
> >> For this we acknowledge and EOI the wake-up IPI, then go into WFI.
> >> Once being kicked out of it later, we sanity check that the start
> >> address has actually been changed (since another attempt to switch
> >> to non-secure would block the core) and jump to the new address.
> >>
> >> The actual CPU kick is done by sending an inter-processor interrupt
> >> via the GIC to all CPU interfaces except the requesting processor.
> >> The secondary cores will then setup their respective GIC CPU
> >> interface.
> >>
> >> The address secondary cores jump to is board specific, we provide
> >> the value here for the Versatile Express board.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >> ---
> >>   arch/arm/cpu/armv7/start.S          | 27 ++++++++++++++++++++++++++-
> >>   arch/arm/include/asm/armv7.h        |  1 +
> >>   arch/arm/lib/virt-v7.c              |  9 ++++++++-
> >>   include/configs/vexpress_ca15_tc2.h |  1 +
> >>   4 files changed, 36 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> >> index e63e892..02234c7 100644
> >> --- a/arch/arm/cpu/armv7/start.S
> >> +++ b/arch/arm/cpu/armv7/start.S
> >> @@ -575,8 +575,19 @@ fiq:
> >>
> >>   #ifdef CONFIG_ARMV7_VIRT
> >>   /* Routine to initialize GIC CPU interface and switch to nonsecure state.
> >> + * Will be executed directly by secondary CPUs after coming out of
> >> + * WFI, or can be called directly by C code for CPU 0.
> >> + * Those two paths mandate to not use any stack and to only use registers
> >> + * r0-r3 to comply with both the C ABI and the requirement of SMP startup
> >> + * code.
> >>    */
> >>   .globl _nonsec_gic_switch
> >> +.globl _smp_pen
> >> +_smp_pen:
> >> +	mrs	r0, cpsr
> >> +	orr	r0, r0, #0xc0
> >> +	msr	cpsr, r0			@ disable interrupts
> >> +	mov	lr, #0				@ clear LR to mark secondary
> >
> > instead of this subtle abuse of lr, why not make this routine simply
> > take a parameter?
> 
> How would this work if this is called out of SMP pen? Shall I rely on 
> the registers being zero, then? Not very stable, I guess.
> I think this whole routine is special anyways, so I felt this "subtle 
> abuse" is OK.
> An option would be to set r0 to 1 in the smp_pen path and pass 0 as the 
> first parameter when calling from C. But then I'd need to save this 
> value - possibly in the LR register ;-)

see my example in the previous patch comments, I don't see why using the
lr like this makes anything anymore clear or more stable?

> 
> > I also slightly object against wrapping the _smp_pen around the
> > _nonsec_gic_switch, I really think these are separate routines, where
> > one can just call the other...?
> 
> The actual routine and the purpose are the same, just the entry and exit 
> code is different. So this fitted nicely in here. I can add a more 
> specific comment on the different entry points.
> 

eh, to me it reads kind of like this C function (but maybe I just choked
on this somehow):

void do_magic(bool mode)
{
	if (mode) {
		pull_rabit_out_of_hat();
		do_card_trick();
	}

	saw_a_woman_in_half(); /* we always do this */
	linking_rings();

	if (!mode)
		return;

	while (1) {
		vanish_statue_of_liberty();
		walk_through_great_wall_of_china();
	}
}

> >>   _nonsec_gic_switch:
> >>   	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
> >>   	add	r3, r2, #0x1000			@ GIC dist i/f offset
> >> @@ -617,5 +628,19 @@ _nonsec_gic_switch:
> >>   	add	r2, r2, #0x1000			@ GIC dist i/f offset
> >>   	str	r1, [r2]			@ allow private interrupts
> >>
> >> -	mov	pc, lr
> >> +	cmp	lr, #0
> >> +	movne	pc, lr				@ CPU 0 to return
> >> +						@ all others: go to sleep
> >> +_ack_int:
> >> +	ldr	r1, [r3, #0x0c]			@ read GICD acknowledge
> >> +	str	r1, [r3, #0x10]			@ write GICD EOI
> >> +
> >> +	adr	r1, _smp_pen
> >> +waitloop:
> >> +	wfi
> >> +	ldr	r0, =CONFIG_SYSFLAGS_ADDR	@ load start address
> >> +	ldr	r0, [r0]
> >> +	cmp	r0, r1			@ make sure we dont execute this code
> >
> > I think I raised this issue previously, but this code is in a core
> > u-boot file, but I could imagine a board with a different crazy boot
> > protocol that required you to check two different fields and jump
> > through other hoops to wake up from the smp pen, so I really think the
> > whole smp pen belongs in a board specific place.
> 
> Right, but I didn't want to do this prematurely without knowing what is 
> really needed. So my plan is to refactor this when adding Arndale 
> support. I think this is special anyways because of the SPL/non-SPL split.
> And although you are right about it being a core u-boot file, this whole 
> code is protected by CONFIG_ARMV7_VIRT. For non-compliant boards one 
> would either not enable this or add support for it.
> 

But I do think another important point is that when someone tries to
understand U-boot (which was for example me not long ago) you start
looking in start.S to get the basic gist of things, and this whole code
is pretty complicated, and called only in a specific use case, so I
think it should properly go in a separate file, why not?

We generally don't just keep giant files around filled with ifdefs,
because it's simply harder to read than having a separate file.

The only real benefit I see is reusing the vector, but I think it's a
small benefit, saving you 32 bytes.

> > Also, the boot-wrapper code used wfe instead, not sure if there are any
> > users that just send an event and doesn't send an IPI?
> 
> Good point. Is WFE a complete superset of WFI? Then I could just change 
> this.
> 
Don't think so, maybe we could get Catalin's input on this one, as he
wrote the wfe in the boot-wrapper code.

> >> +	beq	waitloop		@ again (due to a spurious wakeup)
> >> +	mov	pc, r0
> >>   #endif /* CONFIG_ARMV7_VIRT */
> >> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> >> index 25afffe..296dc92 100644
> >> --- a/arch/arm/include/asm/armv7.h
> >> +++ b/arch/arm/include/asm/armv7.h
> >> @@ -78,6 +78,7 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
> >>   int armv7_switch_nonsec(void);
> >>
> >>   /* defined in cpu/armv7/start.S */
> >> +void _smp_pen(void);
> >>   void _nonsec_gic_switch(void);
> >>   #endif /* CONFIG_ARMV7_VIRT */
> >>
> >> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
> >> index 3a48aee..0248010 100644
> >> --- a/arch/arm/lib/virt-v7.c
> >> +++ b/arch/arm/lib/virt-v7.c
> >> @@ -48,6 +48,7 @@ int armv7_switch_nonsec(void)
> >>   	unsigned int reg;
> >>   	volatile unsigned int *gicdptr;
> >>   	unsigned itlinesnr, i;
> >> +	unsigned int *sysflags;
> >>
> >>   	/* check whether the CPU supports the security extensions */
> >>   	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> >> @@ -106,7 +107,13 @@ int armv7_switch_nonsec(void)
> >>   	for (i = 0; i <= itlinesnr; i++)
> >>   		gicdptr[GICD_IGROUPR0 / 4 + i] = (unsigned)-1;
> >>
> >> -	/* call the non-sec switching code on this CPU */
> >> +	/* now kick all CPUs (expect this one) by writing to GICD_SIGR */
> >> +	sysflags = (void *)CONFIG_SYSFLAGS_ADDR;
> >> +	sysflags[1] = (unsigned)-1;
> >> +	sysflags[0] = (uintptr_t)_smp_pen;
> >> +	gicdptr[GICD_SGIR / 4] = 1U << 24;
> >
> > here you definitely want a barrier to make sure you don't kick the cpus
> > before the sysflags addresses have been written.
> 
> Right.
> 

(which the writel should give you)

> > What does the
> > (unsigned)-1 write to sysflags[1] do?
> 
> This is either a ARM-TC or a VExpress "goodie": sysflags+0 is bit-set 
> only, sysflags+4 is bit-clear only. So to write arbitrary addresses in 
> here you better clear all bits first and then write in the value.
> Cost me at least one day to find this out ;-)

ouch, that wasn't trivial.

> So this should be guarded by VExpress specific defines, I guess. But 
> again didn't want to do this before support for a board with a different 
> behavior is actually implemented.
> 

I think that's pretty important actually, because it suggests how other
people should do it, and makes it easier for SoC vendors to intigrate
such support nicely in U-boot instead of placing a large refactoring
burden on them dealing with code that they may not understand...

Thanks,
-Christoffer

[U-Boot] [PATCH 5/6] ARM: extend non-secure switch to also go into HYP mode

[Christoffer Dall]

On Fri, May 31, 2013 at 11:34:38AM +0200, Andre Przywara wrote:
> On 05/31/2013 07:43 AM, Christoffer Dall wrote:
> >On Mon, May 06, 2013 at 03:17:49PM +0200, Andre Przywara wrote:
> >>For the KVM and XEN hypervisors to be usable, we need to enter the
> >>kernel in HYP mode. Now that we already are in non-secure state,
> >>HYP mode switching is within short reach.
> >>
> >>While doing the non-secure switch, we have to enable the HVC
> >>instruction and setup the HYP mode HVBAR (while still secure).
> >>
> >>The actual switch is done by dropping back from a HYP mode handler
> >>without actually leaving HYP mode, so we introduce a new handler
> >>routine in the exception vector table.
> >>
> >>In the assembly switching routine - which we rename to hyp_gic_switch
> >>on the way - we save and restore the banked LR and SP registers
> >>around the hypercall to do the actual HYP mode switch.
> >>
> >>The C routine first checks whether we are in HYP mode already and
> >>also whether the virtualization extensions are available. It also
> >>checks whether the HYP mode switch was finally successful.
> >>The bootm command part only adds and adjusts some error reporting.
> >>
> >>Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >>---
> >>  arch/arm/cpu/armv7/start.S   | 34 +++++++++++++++++++++++-----------
> >>  arch/arm/include/asm/armv7.h |  4 ++--
> >>  arch/arm/lib/bootm.c         | 12 +++++++++---
> >>  arch/arm/lib/virt-v7.c       | 22 +++++++++++++++-------
> >>  4 files changed, 49 insertions(+), 23 deletions(-)
> >>
> >>diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> >>index 02234c7..921e9d9 100644
> >>--- a/arch/arm/cpu/armv7/start.S
> >>+++ b/arch/arm/cpu/armv7/start.S
> >>@@ -41,7 +41,7 @@ _start: b	reset
> >>  	ldr	pc, _software_interrupt
> >>  	ldr	pc, _prefetch_abort
> >>  	ldr	pc, _data_abort
> >>-	ldr	pc, _not_used
> >>+	ldr	pc, _hyp_trap
> >>  	ldr	pc, _irq
> >>  	ldr	pc, _fiq
> >>  #ifdef CONFIG_SPL_BUILD
> >>@@ -49,7 +49,7 @@ _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
> >>+_hyp_trap:		.word _hyp_trap
> >>  _irq:			.word _irq
> >>  _fiq:			.word _fiq
> >>  _pad:			.word 0x12345678 /* now 16*4=64 */
> >>@@ -58,7 +58,7 @@ _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
> >>+_hyp_trap:		.word hyp_trap
> >>  _irq:			.word irq
> >>  _fiq:			.word fiq
> >>  _pad:			.word 0x12345678 /* now 16*4=64 */
> >>@@ -513,12 +513,18 @@ software_interrupt:
> >>  	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
> >>  	bic	r1, r1, #0x07f
> >>  	orr	r1, r1, #0x31			@ enable NS, AW, FW
> >>+	mrc	p15, 0, r0, c0, c1, 1		@ check for Virt ext
> >>+	and	r0, r0, #0xf000
> >>+	cmp	r0, #0x1000
> >
> >you can just do ands r0, r0 ,#0xf000 and change the orreq below to orrne
> 
> Can I? But I want to make sure that Virt ext is of version 1 only,
> anything beyond that I cannot reliably support, right? Or is there a
> guarantee that this is backwards compatible?

In the ARMv7 specifications it mentions either 1 or 0 as the possible
values of this field, and doesn't say anything about reserved values
iirc, but maybe that shouldn't be taken that literally.  Just keep the
code as it is then.

> 
> >>+	orreq	r1, r1, #0x100			@ allow HVC instruction
> >>
> >>  	mrc	p15, 0, r0, c12, c0, 0		@ save secure copy of VBAR
> >>  	mcr	p15, 0, r1, c1, c1, 0		@ write SCR, switch to non-sec
> >>  	isb
> >>  	mcr	p15, 0, r0, c12, c0, 0		@ write non-secure copy of VBAR
> >>
> >>+	mcreq	p15, 4, r0, c12, c0, 0		@ write HYP mode HVBAR
> >>+
> >
> >nit: s/HYP mode//
> >
> >>  	movs	pc, lr
> >>
> >>  	.align	5
> >>@@ -534,10 +540,9 @@ data_abort:
> >>  	bl	do_data_abort
> >>
> >>  	.align	5
> >>-not_used:
> >>-	get_bad_stack
> >>-	bad_save_user_regs
> >>-	bl	do_not_used
> >>+hyp_trap:
> >>+	.byte 0x00, 0xe3, 0x0e, 0xe1		@ mrs lr, elr_hyp
> >
> >do we really need to support this on assemblers that old?
> 
> What do you mean with old? I can check again, but I think it didn't
> work with my self-compiled binutils 2.23. Or do I need a directive
> to enable this?
> 

You need a directive, see the Makefile in arch/arm/kvm/Makefile and the
corresponding source files in the kernel.

> >>+	mov pc, lr
> >>
> >>  #ifdef CONFIG_USE_IRQ
> >>
> >>@@ -574,21 +579,21 @@ fiq:
> >>  #endif /* CONFIG_SPL_BUILD */
> >>
> >>  #ifdef CONFIG_ARMV7_VIRT
> >>-/* Routine to initialize GIC CPU interface and switch to nonsecure state.
> >>- * Will be executed directly by secondary CPUs after coming out of
> >>+/* Routine to initialize GIC CPU interface, switch to nonsecure and to HYP
> >>+ * mode. Will be executed directly by secondary CPUs after coming out of
> >
> >So now this routine does three different things in different context at
> >once, why?
> 
> Not three. At most two. But actually it does only one thing: switch
> a single core to HYP mode. Only the entry and exit points are
> different.
> I just see that I could make the smp_pen a separate function,
> calling the switch function. This would also solve this "LR abuse"
> thing.
> Will try this.
> 

1: smp_pen
2: switch from secure to non-secure
3: switch from non-secure svc to hyp

but yes, I think we understand each other at this point.

> >>   * WFI, or can be called directly by C code for CPU 0.
> >>   * Those two paths mandate to not use any stack and to only use registers
> >>   * r0-r3 to comply with both the C ABI and the requirement of SMP startup
> >>   * code.
> >>   */
> >>-.globl _nonsec_gic_switch
> >>+.globl _hyp_gic_switch
> >>  .globl _smp_pen
> >>  _smp_pen:
> >>  	mrs	r0, cpsr
> >>  	orr	r0, r0, #0xc0
> >>  	msr	cpsr, r0			@ disable interrupts
> >>  	mov	lr, #0				@ clear LR to mark secondary
> >>-_nonsec_gic_switch:
> >>+_hyp_gic_switch:
> >>  	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
> >>  	add	r3, r2, #0x1000			@ GIC dist i/f offset
> >>  	mvn	r1, #0
> >>@@ -628,6 +633,13 @@ _nonsec_gic_switch:
> >>  	add	r2, r2, #0x1000			@ GIC dist i/f offset
> >>  	str	r1, [r2]			@ allow private interrupts
> >>
> >>+	mov	r2, lr
> >>+	mov	r1, sp
> >>+	.byte 0x70, 0x00, 0x40, 0xe1		@ hvc #0
> >>+	isb
> >
> >again, I'm doubtful this isb is necessary when you just did an exception
> >return.
> 
> Good point. Exception returns should do this automatically, right?
> Is this an official fact or just a feeling?

It's official, and I've been reminded numerous times by the ARM people
reviewing my KVM code :)  It's somewhere in the ARM ARM.

> 
> ...
> 
> >>diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
> >>index 0248010..3883463 100644
> >>--- a/arch/arm/lib/virt-v7.c
> >>+++ b/arch/arm/lib/virt-v7.c
> >>@@ -3,6 +3,7 @@
> >>   * Andre Przywara, Linaro
> >>   *
> >>   * routines to push ARMv7 processors from secure into non-secure state
> >>+ * and from non-secure SVC into HYP mode
> >>   * needed to enable ARMv7 virtualization for current hypervisors
> >>   *
> >>   * See file CREDITS for list of people who contributed to this
> >>@@ -43,16 +44,20 @@ static inline unsigned int read_cpsr(void)
> >>  	return reg;
> >>  }
> >>
> >>-int armv7_switch_nonsec(void)
> >>+int armv7_switch_hyp(void)
> >>  {
> >>  	unsigned int reg;
> >>  	volatile unsigned int *gicdptr;
> >>  	unsigned itlinesnr, i;
> >>  	unsigned int *sysflags;
> >>
> >>-	/* check whether the CPU supports the security extensions */
> >>+	/* check whether we are in HYP mode already */
> >>+	if ((read_cpsr() & 0x1F) == 0x1a)
> >>+		return 1;
> >>+
> >>+	/* check whether the CPU supports the virtualization extensions */
> >>  	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> >>-	if ((reg & 0xF0) == 0)
> >>+	if ((reg & 0xF000) != 0x1000)
> >>  		return 2;
> >>
> >>  	/* the timer frequency for the generic timer needs to be
> >>@@ -73,8 +78,8 @@ int armv7_switch_nonsec(void)
> >>  	 */
> >>
> >>  	/* check whether we are an Cortex-A15 or A7.
> >>-	 * The actual non-secure switch should work with all CPUs supporting
> >>-	 * the security extension, but we need the GIC address,
> >>+	 * The actual HYP switch should work with all CPUs supporting
> >>+	 * the virtualization extension, but we need the GIC address,
> >>  	 * which we know only for sure for those two CPUs.
> >>  	 */
> >>  	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(reg));
> >>@@ -113,8 +118,11 @@ int armv7_switch_nonsec(void)
> >>  	sysflags[0] = (uintptr_t)_smp_pen;
> >>  	gicdptr[GICD_SGIR / 4] = 1U << 24;
> >>
> >>-	/* call the non-sec switching code on this CPU also */
> >>-	_nonsec_gic_switch();
> >>+	/* call the HYP switching code on this CPU also */
> >>+	_hyp_gic_switch();
> >>+
> >>+	if ((read_cpsr() & 0x1F) != 0x1a)
> >>+		return 5;
> >
> >this is really a fatal crash right? We probably don't want to try and
> >proceed with boot at this point.
> 
> Well not really. If we reach this point without being in HYP mode,
> that just hasn't worked - for various reasons. Since this new bootm
> functionality is unconditional, I felt we should better not crash.
> Actually Linux will run fine, just without KVM (for certains
> meanings of "fine" that is - as a KVM developer ;-)
> 
Really, but we checked all the prerequisites before calling
_hyp_gic_switch, so we really expect it to work.  If it didn't, it means
that something fatally bad happened as far as I can tell.

As for the integration with the bootm command, you already can't enable
CONFIG_ARMV7_VIRT on a board that's not booted in the secure world, and
you probably can never support determinig that at run-time.  Perhaps
things should look like this:

#ifdef CONFIG_ARMV7_SECURE_BOOT
... do all the non-secure boot stuff
#endif

#ifdef CONFIG_ARMV7_CHANGE_TO_HYP
... call board-specific go-to-hyp-function
... if no board-specific function, exists, tell the user
#endif

But it's just a suggestion, the U-boot guys would know how they want
this I suppose.

Thanks,
-Christoffer

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Albert ARIBAUD]

Hi Christoffer,

On Fri, 31 May 2013 16:50:09 -0700, Christoffer Dall
<cdall@cs.columbia.edu> wrote:

> On Fri, May 31, 2013 at 11:23:16AM +0200, Andre Przywara wrote:
> > On 05/31/2013 03:02 AM, Christoffer Dall wrote:

> > This is added later in 5/6. For reviewing purposes I split the
> > patches up to do the non-secure switch only first. Later I add the
> > bits to actually go to HYP mode.
> 
> The splitting of patches is fine, but it would be helpful to explain the
> scope a little more in the commit text perhaps, maybe I'm just being
> silly.

That's the point where a cover letter will be very useful in V2.

Also, speaking of V2 (and, presumably, later versions), that's where
patman will be useful (see tools/patman/README) as it allows one's
local branch to contain not only things like cover letter, but also
patch history, which will be a must IMO for this series.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/6] ARM: add secure monitor handler to switch to non-secure state

[Albert ARIBAUD]

On Sat, 1 Jun 2013 12:06:24 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Hi Christoffer,
> 
> On Fri, 31 May 2013 16:50:09 -0700, Christoffer Dall
> <cdall@cs.columbia.edu> wrote:
> 
> > On Fri, May 31, 2013 at 11:23:16AM +0200, Andre Przywara wrote:
> > > On 05/31/2013 03:02 AM, Christoffer Dall wrote:
> 
> > > This is added later in 5/6. For reviewing purposes I split the
> > > patches up to do the non-secure switch only first. Later I add the
> > > bits to actually go to HYP mode.
> > 
> > The splitting of patches is fine, but it would be helpful to explain the
> > scope a little more in the commit text perhaps, maybe I'm just being
> > silly.
> 
> That's the point where a cover letter will be very useful in V2.

Correction: V1 has a cover letter; what I meant and should have said
is, that's where an overall description in the cover letter of how
commits are organized will be very useful in V2.

> Also, speaking of V2 (and, presumably, later versions), that's where
> patman will be useful (see tools/patman/README) as it allows one's
> local branch to contain not only things like cover letter, but also
> patch history, which will be a must IMO for this series.

Correction: maybe/probably Andre used patman already; in this case,
scratch the paragraph above.

Apologies for any misunderstanding,
-- 
Albert.

[U-Boot] [PATCH 4/6] ARM: add SMP support for non-secure switch

[TigerLiu at viatech.com.cn]

Hi, experts:
Do these patch code exist in current u-boot code base?
Or, i have to download linaro version uboot?

Best wishes,

-----????-----
???: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de] ?? Andre Przywara
????: 2013?5?31? 17:33
???: Christoffer Dall
??: geoff.levand at linaro.org; cdall at cs.columbia.edu; peter.maydell at linaro.org; agraf at suse.de; marc.zyngier at arm.com; trini at ti.com; kvmarm at lists.cs.columbia.edu; u-boot at lists.denx.de
??: Re: [U-Boot] [PATCH 4/6] ARM: add SMP support for non-secure switch

On 05/31/2013 07:32 AM, Christoffer Dall wrote:
> On Mon, May 06, 2013 at 03:17:48PM +0200, Andre Przywara wrote:
>> Currently the non-secure switch is only done for the boot processor.
>> To later allow full SMP support, we have to switch all secondary
>> cores into non-secure state also.
>>
>> So we add an entry point for secondary CPUs coming out of low-power
>> state and make sure we put them into WFI again after having switched
>> to non-secure state.
>> For this we acknowledge and EOI the wake-up IPI, then go into WFI.
>> Once being kicked out of it later, we sanity check that the start
>> address has actually been changed (since another attempt to switch
>> to non-secure would block the core) and jump to the new address.
>>
>> The actual CPU kick is done by sending an inter-processor interrupt
>> via the GIC to all CPU interfaces except the requesting processor.
>> The secondary cores will then setup their respective GIC CPU
>> interface.
>>
>> The address secondary cores jump to is board specific, we provide
>> the value here for the Versatile Express board.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   arch/arm/cpu/armv7/start.S          | 27 ++++++++++++++++++++++++++-
>>   arch/arm/include/asm/armv7.h        |  1 +
>>   arch/arm/lib/virt-v7.c              |  9 ++++++++-
>>   include/configs/vexpress_ca15_tc2.h |  1 +
>>   4 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>> index e63e892..02234c7 100644
>> --- a/arch/arm/cpu/armv7/start.S
>> +++ b/arch/arm/cpu/armv7/start.S
>> @@ -575,8 +575,19 @@ fiq:
>>
>>   #ifdef CONFIG_ARMV7_VIRT
>>   /* Routine to initialize GIC CPU interface and switch to nonsecure state.
>> + * Will be executed directly by secondary CPUs after coming out of
>> + * WFI, or can be called directly by C code for CPU 0.
>> + * Those two paths mandate to not use any stack and to only use registers
>> + * r0-r3 to comply with both the C ABI and the requirement of SMP startup
>> + * code.
>>    */
>>   .globl _nonsec_gic_switch
>> +.globl _smp_pen
>> +_smp_pen:
>> +	mrs	r0, cpsr
>> +	orr	r0, r0, #0xc0
>> +	msr	cpsr, r0			@ disable interrupts
>> +	mov	lr, #0				@ clear LR to mark secondary
>
> instead of this subtle abuse of lr, why not make this routine simply
> take a parameter?

How would this work if this is called out of SMP pen? Shall I rely on 
the registers being zero, then? Not very stable, I guess.
I think this whole routine is special anyways, so I felt this "subtle 
abuse" is OK.
An option would be to set r0 to 1 in the smp_pen path and pass 0 as the 
first parameter when calling from C. But then I'd need to save this 
value - possibly in the LR register ;-)

> I also slightly object against wrapping the _smp_pen around the
> _nonsec_gic_switch, I really think these are separate routines, where
> one can just call the other...?

The actual routine and the purpose are the same, just the entry and exit 
code is different. So this fitted nicely in here. I can add a more 
specific comment on the different entry points.

>>   _nonsec_gic_switch:
>>   	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
>>   	add	r3, r2, #0x1000			@ GIC dist i/f offset
>> @@ -617,5 +628,19 @@ _nonsec_gic_switch:
>>   	add	r2, r2, #0x1000			@ GIC dist i/f offset
>>   	str	r1, [r2]			@ allow private interrupts
>>
>> -	mov	pc, lr
>> +	cmp	lr, #0
>> +	movne	pc, lr				@ CPU 0 to return
>> +						@ all others: go to sleep
>> +_ack_int:
>> +	ldr	r1, [r3, #0x0c]			@ read GICD acknowledge
>> +	str	r1, [r3, #0x10]			@ write GICD EOI
>> +
>> +	adr	r1, _smp_pen
>> +waitloop:
>> +	wfi
>> +	ldr	r0, =CONFIG_SYSFLAGS_ADDR	@ load start address
>> +	ldr	r0, [r0]
>> +	cmp	r0, r1			@ make sure we dont execute this code
>
> I think I raised this issue previously, but this code is in a core
> u-boot file, but I could imagine a board with a different crazy boot
> protocol that required you to check two different fields and jump
> through other hoops to wake up from the smp pen, so I really think the
> whole smp pen belongs in a board specific place.

Right, but I didn't want to do this prematurely without knowing what is 
really needed. So my plan is to refactor this when adding Arndale 
support. I think this is special anyways because of the SPL/non-SPL split.
And although you are right about it being a core u-boot file, this whole 
code is protected by CONFIG_ARMV7_VIRT. For non-compliant boards one 
would either not enable this or add support for it.

> Also, the boot-wrapper code used wfe instead, not sure if there are any
> users that just send an event and doesn't send an IPI?

Good point. Is WFE a complete superset of WFI? Then I could just change 
this.

>> +	beq	waitloop		@ again (due to a spurious wakeup)
>> +	mov	pc, r0
>>   #endif /* CONFIG_ARMV7_VIRT */
>> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
>> index 25afffe..296dc92 100644
>> --- a/arch/arm/include/asm/armv7.h
>> +++ b/arch/arm/include/asm/armv7.h
>> @@ -78,6 +78,7 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
>>   int armv7_switch_nonsec(void);
>>
>>   /* defined in cpu/armv7/start.S */
>> +void _smp_pen(void);
>>   void _nonsec_gic_switch(void);
>>   #endif /* CONFIG_ARMV7_VIRT */
>>
>> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
>> index 3a48aee..0248010 100644
>> --- a/arch/arm/lib/virt-v7.c
>> +++ b/arch/arm/lib/virt-v7.c
>> @@ -48,6 +48,7 @@ int armv7_switch_nonsec(void)
>>   	unsigned int reg;
>>   	volatile unsigned int *gicdptr;
>>   	unsigned itlinesnr, i;
>> +	unsigned int *sysflags;
>>
>>   	/* check whether the CPU supports the security extensions */
>>   	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
>> @@ -106,7 +107,13 @@ int armv7_switch_nonsec(void)
>>   	for (i = 0; i <= itlinesnr; i++)
>>   		gicdptr[GICD_IGROUPR0 / 4 + i] = (unsigned)-1;
>>
>> -	/* call the non-sec switching code on this CPU */
>> +	/* now kick all CPUs (expect this one) by writing to GICD_SIGR */
>> +	sysflags = (void *)CONFIG_SYSFLAGS_ADDR;
>> +	sysflags[1] = (unsigned)-1;
>> +	sysflags[0] = (uintptr_t)_smp_pen;
>> +	gicdptr[GICD_SGIR / 4] = 1U << 24;
>
> here you definitely want a barrier to make sure you don't kick the cpus
> before the sysflags addresses have been written.

Right.

> What does the
> (unsigned)-1 write to sysflags[1] do?

This is either a ARM-TC or a VExpress "goodie": sysflags+0 is bit-set 
only, sysflags+4 is bit-clear only. So to write arbitrary addresses in 
here you better clear all bits first and then write in the value.
Cost me at least one day to find this out ;-)
So this should be guarded by VExpress specific defines, I guess. But 
again didn't want to do this before support for a board with a different 
behavior is actually implemented.

Regards,
Andre.

>> +
>> +	/* call the non-sec switching code on this CPU also */
>>   	_nonsec_gic_switch();
>>
>>   	return 0;
>> diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
>> index 9e230ad..210a27c 100644
>> --- a/include/configs/vexpress_ca15_tc2.h
>> +++ b/include/configs/vexpress_ca15_tc2.h
>> @@ -32,5 +32,6 @@
>>   #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
>>
>>   #define CONFIG_SYS_CLK_FREQ 24000000
>> +#define CONFIG_SYSFLAGS_ADDR 0x1c010030
>>
>>   #endif
>> --
>> 1.7.12.1
>>

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot