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

[U-Boot] [PATCH v2 0/7] 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 implementations (KVM and Xen)
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.

The process of switching into HYP mode requires the CPU to be in
secure state initially when entering u-boot, it will then setup some
register and switch to non-secure state. This requires the GIC to be
programmed properly first. Explanations about the details are in the
commit messages of the respective patches.

The patches are structured like this:
1/7: prepare header file
2/7: add monitor handler (assembly)
3/7: add per CPU non-secure switch routine (assembly)
4/7: add system wide non-secure setup and link to bootm command (C)
5/7: add SMP functionality
6/7: add HYP mode switching
7/7: enable code on Versatile Express TC2

So up to patch 5/7 this code should work on non-virtualization
capable CPUs also.

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.

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

Changes RFC..v1
* 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

Changes v1..v2
mostly style and code layout changes 
* restructure assembly code to live in a new file and not start.S
* split smp, nonsec_init and hyp_init to be separate functions
* used named constants from common header files
* split C function to be more readable
* extend comments to be more precise and elaborate
* add provision to override GIC base address (needed for Arndale?)
* add configuration variable to enable VExpress specific SMP code
* use writel/readl for MMIO GIC accesses
* remove superfluous isb instructions
* more minor fixes

Please review and comment!
I am pretty sure I missed some of the comments from the earlier
series, so if you find something that was mentioned before, feel
free to repeat it.

Contributions and comments to support other boards are welcome.

Andre Przywara (7):
  ARM: prepare armv7.h to be included from assembly source
  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/Makefile         |   4 +
 arch/arm/cpu/armv7/nonsec_virt.S    | 172 ++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/armv7.h        |  33 ++++++-
 arch/arm/include/asm/gic.h          |  17 ++++
 arch/arm/lib/Makefile               |   2 +
 arch/arm/lib/bootm.c                |  26 ++++++
 arch/arm/lib/virt-v7.c              | 171 +++++++++++++++++++++++++++++++++++
 include/configs/vexpress_ca15_tc2.h |   5 +-
 8 files changed, 428 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/nonsec_virt.S
 create mode 100644 arch/arm/include/asm/gic.h
 create mode 100644 arch/arm/lib/virt-v7.c

-- 
1.7.12.1

[U-Boot] [PATCH v2 1/7] ARM: prepare armv7.h to be included from assembly source

[Andre Przywara]

armv7.h contains some useful constants, but also C prototypes.
To include it also in assembly files, protect the non-assembly
part appropriately.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/include/asm/armv7.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index a73630b..20caa7c 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -23,7 +23,6 @@
  */
 #ifndef ARMV7_H
 #define ARMV7_H
-#include <linux/types.h>
 
 /* Cortex-A9 revisions */
 #define MIDR_CORTEX_A9_R0P1	0x410FC091
@@ -57,6 +56,9 @@
 #define ARMV7_CLIDR_CTYPE_INSTRUCTION_DATA	3
 #define ARMV7_CLIDR_CTYPE_UNIFIED		4
 
+#ifndef __ASSEMBLY__
+#include <linux/types.h>
+
 /*
  * CP15 Barrier instructions
  * Please note that we have separate barrier instructions in ARMv7
@@ -74,4 +76,6 @@ 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);
 
+#endif /* ! __ASSEMBLY__ */
+
 #endif
-- 
1.7.12.1

[U-Boot] [PATCH v2 2/7] 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 first.
Add new file in arch/arm/cpu/armv7 to hold a monitor handler routine
which switches the CPU to non-secure state by setting the NS and
associated bits.
According to the ARM architecture reference manual this should not be
done in SVC mode, so we have to setup a SMC handler for this.
We create a new vector table to avoid interference with other boards.
The MVBAR register will be programmed later just before the smc call.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/cpu/armv7/Makefile      |  4 +++
 arch/arm/cpu/armv7/nonsec_virt.S | 54 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/nonsec_virt.S

diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
index 7a8c2d0..b64f210 100644
--- a/arch/arm/cpu/armv7/Makefile
+++ b/arch/arm/cpu/armv7/Makefile
@@ -36,6 +36,10 @@ ifneq ($(CONFIG_AM33XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CONFIG_TEGRA)$(CONF
 SOBJS	+= lowlevel_init.o
 endif
 
+ifneq ($(CONFIG_ARMV7_VIRT),)
+SOBJS   += nonsec_virt.o
+endif
+
 SRCS	:= $(START:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))
 START	:= $(addprefix $(obj),$(START))
diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
new file mode 100644
index 0000000..f5572f5
--- /dev/null
+++ b/arch/arm/cpu/armv7/nonsec_virt.S
@@ -0,0 +1,54 @@
+/*
+ * code for switching cores into non-secure state
+ *
+ * Copyright (c) 2013	Andre Przywara <andre.przywara@linaro.org>
+ *
+ * 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 <config.h>
+
+/* the vector table for secure state */
+_secure_vectors:
+	.word 0	/* reset */
+	.word 0 /* undef */
+	adr pc, _secure_monitor
+	.word 0
+	.word 0
+	.word 0
+	.word 0
+	.word 0
+	.word 0	/* pad */
+
+/*
+ * software interrupt aka. secure monitor handler
+ * This is executed on a "smc" instruction, we use a "smc #0" to switch
+ * to non-secure state.
+ * We use only r0 and r1 here, due to constraints in the caller.
+ */
+	.align	5
+_secure_monitor:
+	mrc	p15, 0, r1, c1, c1, 0		@ read SCR
+	bic	r1, r1, #0x4e			@ clear IRQ, FIQ, EA, nET bits
+	orr	r1, r1, #0x31			@ enable NS, AW, FW bits
+
+	mcr	p15, 0, r1, c1, c1, 0		@ write SCR (with NS bit set)
+
+	movs	pc, lr				@ return to non-secure SVC
+
-- 
1.7.12.1

[U-Boot] [PATCH v2 3/7] 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 value in the CBAR register. Since this
register is not architecturally defined, we check the MIDR before to
be from an A15 or A7.
For CPUs not having the CBAR or boards with wrong information herein
we allow providing the base address as a configuration variable.

With the GIC accessible, we:
a) allow private interrupts to be delivered to the core
   (GICD_IGROUPR0 = 0xFFFFFFFF)
b) enable the CPU interface (GICC_CTLR[0] = 1)
c) set the priority filter to allow non-secure interrupts
   (GICC_PMR = 0xFF)

After having switched to non-secure state, we also enable the
non-secure GIC CPU interface, since this register is banked.

Also we allow access to all coprocessor interfaces from non-secure
state by writing the appropriate bits in the NSACR register.

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

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/cpu/armv7/nonsec_virt.S | 66 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/armv7.h     | 16 ++++++++++
 arch/arm/include/asm/gic.h       | 17 +++++++++++
 3 files changed, 99 insertions(+)
 create mode 100644 arch/arm/include/asm/gic.h

diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
index f5572f5..656d99b 100644
--- a/arch/arm/cpu/armv7/nonsec_virt.S
+++ b/arch/arm/cpu/armv7/nonsec_virt.S
@@ -23,6 +23,8 @@
  */
 
 #include <config.h>
+#include <asm/gic.h>
+#include <asm/armv7.h>
 
 /* the vector table for secure state */
 _secure_vectors:
@@ -52,3 +54,67 @@ _secure_monitor:
 
 	movs	pc, lr				@ return to non-secure SVC
 
+#define lo(x) ((x) & 0xFFFF)
+#define hi(x) ((x) >> 16)
+
+/*
+ * Routine to switch a core to non-secure state.
+ * Initializes the GIC per-core interface, allows coprocessor access in
+ * non-secure modes and uses smc #0 to do the non-secure transition.
+ * To be called by smp_pen for secondary cores and directly for the BSP.
+ * For those two cases to work we must not use any stack and thus are
+ * limited to the caller saved registers r0-r3.
+ * PERIPHBASE is used to get the GIC address. This could be 40 bits long,
+ * though, but we check this in C before calling this function.
+ */
+.globl _nonsec_init
+_nonsec_init:
+#ifdef CONFIG_ARM_GIC_BASE_ADDRESS
+	ldr	r2, =CONFIG_ARM_GIC_BASE_ADDRESS
+#else
+	mrc	p15, 4, r2, c15, c0, 0		@ read CBAR
+#endif
+	add	r3, r2, #GIC_DIST_OFFSET	@ GIC dist i/f offset
+	mvn	r1, #0				@ all bits to 1
+	str	r1, [r3, #GICD_IGROUPRn]	@ allow private interrupts
+
+	mrc	p15, 0, r0, c0, c0, 0		@ read MIDR
+	bfc	r0, #20, #4			@ mask out variant
+	bfc	r0, #0, #4			@ and revision
+
+	movw	r1, #lo(MIDR_CORTEX_A7_R0P0 & MIDR_PRIMARY_PART_MASK)
+	movt	r1, #hi(MIDR_CORTEX_A7_R0P0 & MIDR_PRIMARY_PART_MASK)
+	cmp	r0, r1				@ check for Cortex-A7
+
+	orr	r1, #(MIDR_CORTEX_A15_R0P0 & 0xf0)
+	cmpne	r0, r1				@ check for Cortex-A15
+
+	movne	r1, #GIC_CPU_OFFSET_A9		@ GIC CPU offset for A9
+	moveq	r1, #GIC_CPU_OFFSET_A15		@ GIC CPU offset for A15/A7
+	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
+
+	mov	r1, #1				@ set GICC_CTLR[enable]
+	str	r1, [r3, #GICC_CTLR]		@ and clear all other bits
+	mov	r1, #0xff
+	str	r1, [r3, #GICC_PMR]		@ set priority mask register
+
+	movw	r1, #0x3fff
+	movt	r1, #0x0006
+	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
+
+	adr	r1, _secure_vectors
+	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR to secure vectors
+
+	mrc	p15, 0, ip, c12, c0, 0		@ save secure copy of VBAR
+
+	isb
+	smc	#0				@ call into MONITOR mode
+
+	mcr	p15, 0, ip, c12, c0, 0		@ write non-secure copy of VBAR
+
+	mov	r1, #1
+	str	r1, [r3, #GICC_CTLR]		@ enable non-secure CPU i/f
+	add	r2, r2, #GIC_DIST_OFFSET
+	str	r1, [r2]			@ allow private interrupts
+
+	bx	lr
diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index 20caa7c..989bb72 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -34,6 +34,17 @@
 #define MIDR_CORTEX_A15_R0P0	0x410FC0F0
 #define MIDR_CORTEX_A15_R2P2	0x412FC0F2
 
+/* Cortex-A7 revisions */
+#define MIDR_CORTEX_A7_R0P0	0x410FC070
+
+#define MIDR_PRIMARY_PART_MASK	0xFF0FFFF0
+
+/* ID_PFR1 feature fields */
+#define CPUID_ARM_VIRT_SHIFT	12
+#define CPUID_ARM_VIRT_MASK	(0xF << CPUID_ARM_VIRT_SHIFT)
+#define CPUID_ARM_TIMER_SHIFT	16
+#define CPUID_ARM_TIMER_MASK	(0xF << CPUID_ARM_TIMER_SHIFT)
+
 /* CCSIDR */
 #define CCSIDR_LINE_SIZE_OFFSET		0
 #define CCSIDR_LINE_SIZE_MASK		0x7
@@ -76,6 +87,11 @@ void v7_outer_cache_inval_all(void);
 void v7_outer_cache_flush_range(u32 start, u32 end);
 void v7_outer_cache_inval_range(u32 start, u32 end);
 
+#ifdef CONFIG_ARMV7_VIRT
+/* defined in cpu/armv7/nonsec_virt.S */
+void _nonsec_init(void);
+#endif /* CONFIG_ARMV7_VIRT */
+
 #endif /* ! __ASSEMBLY__ */
 
 #endif
diff --git a/arch/arm/include/asm/gic.h b/arch/arm/include/asm/gic.h
new file mode 100644
index 0000000..c2b1e28
--- /dev/null
+++ b/arch/arm/include/asm/gic.h
@@ -0,0 +1,17 @@
+#ifndef __GIC_V2_H__
+#define __GIC_V2_H__
+
+/* register offsets for the ARM generic interrupt controller (GIC) */
+
+#define GIC_DIST_OFFSET		0x1000
+#define GICD_CTLR		0x0000
+#define GICD_TYPER		0x0004
+#define GICD_IGROUPRn		0x0080
+#define GICD_SGIR		0x0F00
+
+#define GIC_CPU_OFFSET_A9	0x0100
+#define GIC_CPU_OFFSET_A15	0x2000
+#define GICC_CTLR		0x0000
+#define GICC_PMR		0x0004
+
+#endif
-- 
1.7.12.1

[U-Boot] [PATCH v2 4/7] 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.

The core specific part of the work is done in the assembly routine
in nonsec_virt.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.

We check the availability of the security extensions first.

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.
The Versatile Express board does not need this, so we remove the
frequency from the configuration file here.

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.
Board not implementing the CBAR can override this value via a
configuration file variable.

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.

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              | 137 ++++++++++++++++++++++++++++++++++++
 include/configs/vexpress_ca15_tc2.h |   2 -
 5 files changed, 166 insertions(+), 2 deletions(-)
 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 989bb72..56d0dd0 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -88,6 +88,13 @@ void v7_outer_cache_flush_range(u32 start, u32 end);
 void v7_outer_cache_inval_range(u32 start, u32 end);
 
 #ifdef CONFIG_ARMV7_VIRT
+
+#define HYP_ERR_NO_SEC_EXT		2
+#define HYP_ERR_NO_GIC_ADDRESS		3
+#define HYP_ERR_GIC_ADDRESS_ABOVE_4GB	4
+
+int armv7_switch_nonsec(void);
+
 /* defined in cpu/armv7/nonsec_virt.S */
 void _nonsec_init(void);
 #endif /* CONFIG_ARMV7_VIRT */
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 8ad9f66..1570ad5 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -60,6 +60,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 1b6e0ac..8251a89 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -34,6 +34,10 @@
 #include <asm/bootm.h>
 #include <linux/compiler.h>
 
+#ifdef CONFIG_ARMV7_VIRT
+#include <asm/armv7.h>
+#endif
+
 DECLARE_GLOBAL_DATA_PTR;
 
 static struct tag *params;
@@ -222,6 +226,22 @@ static void boot_prep_linux(bootm_headers_t *images)
 		printf("FDT and ATAGS support not compiled in - hanging\n");
 		hang();
 	}
+#ifdef CONFIG_ARMV7_VIRT
+	switch (armv7_switch_nonsec()) {
+	case 0:
+		debug("entered non-secure state\n");
+		break;
+	case HYP_ERR_NO_SEC_EXT:
+		printf("HYP mode: Security extensions not implemented.\n");
+		break;
+	case HYP_ERR_NO_GIC_ADDRESS:
+		printf("HYP mode: could not determine GIC address.\n");
+		break;
+	case HYP_ERR_GIC_ADDRESS_ABOVE_4GB:
+		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..7876a77
--- /dev/null
+++ b/arch/arm/lib/virt-v7.c
@@ -0,0 +1,137 @@
+/*
+ * (C) Copyright 2013
+ * Andre Przywara, Linaro
+ *
+ * Routines to transition 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>
+#include <asm/gic.h>
+#include <asm/io.h>
+
+static unsigned int read_id_pfr1(void)
+{
+	unsigned int reg;
+
+	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
+	return reg;
+}
+
+/* The timer frequency for the generic timer needs to be
+ * programmed in secure state. Some primary bootloaders / firmware
+ * omit this, so if the frequency is provided in the configuration,
+ * we do this here instead.
+ * But first check if we have the generic timer.
+ */
+static void set_generic_timer_frequency(void)
+{
+#ifdef CONFIG_SYS_CLK_FREQ
+	unsigned int reg;
+
+	reg = read_id_pfr1();
+	if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
+		asm("mcr p15, 0, %0, c14, c0, 0\n"
+		: : "r"(CONFIG_SYS_CLK_FREQ));
+#endif
+}
+
+static int get_gic_base_address(char **gicdptr)
+{
+#ifdef CONFIG_ARM_GIC_BASE_ADDRESS
+	*gicdptr = (void *)(CONFIG_ARM_GIC_BASE_ADDRESS + GIC_DIST_OFFSET);
+	return 0;
+#else
+	unsigned midr;
+	unsigned periphbase;
+
+	/* check whether we are an Cortex-A15 or A7.
+	 * 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"(midr));
+	switch (midr & MIDR_PRIMARY_PART_MASK) {
+	case MIDR_CORTEX_A9_R0P1:
+	case MIDR_CORTEX_A15_R0P0:
+	case MIDR_CORTEX_A7_R0P0:
+		break;
+	default:
+		return HYP_ERR_NO_GIC_ADDRESS;
+	}
+
+	/* get the GIC base address from the CBAR register */
+	asm("mrc p15, 4, %0, c15, c0, 0\n" : "=r" (periphbase));
+
+	/* 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 ((periphbase & 0xff) != 0)
+		return HYP_ERR_GIC_ADDRESS_ABOVE_4GB;
+
+	*gicdptr = (void *)(periphbase + GIC_DIST_OFFSET);
+
+	return 0;
+#endif
+}
+
+int armv7_switch_nonsec(void)
+{
+	unsigned int reg, ret;
+	char *gicdptr;
+	unsigned itlinesnr, i;
+
+	/* check whether the CPU supports the security extensions */
+	reg = read_id_pfr1();
+	if ((reg & 0xF0) == 0)
+		return HYP_ERR_NO_SEC_EXT;
+
+	set_generic_timer_frequency();
+
+	/* 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.
+	 */
+
+	ret = get_gic_base_address(&gicdptr);
+	if (ret != 0)
+		return ret;
+
+	/* enable the GIC distributor */
+	writel(readl(&gicdptr[GICD_CTLR]) | 0x03, &gicdptr[GICD_CTLR]);
+
+	/* TYPER[4:0] contains an encoded number of all interrupts */
+	itlinesnr = readl(&gicdptr[GICD_TYPER]) & 0x1f;
+
+	/* set all bits in the GIC group registers to one to allow access
+	 * from non-secure state
+	 */
+	for (i = 0; i <= itlinesnr; i++)
+		writel((unsigned)-1, &gicdptr[GICD_IGROUPRn + 4 * i]);
+
+	/* call the non-sec switching code on this CPU */
+	_nonsec_init();
+
+	return 0;
+}
diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
index 9e230ad..4f425ac 100644
--- a/include/configs/vexpress_ca15_tc2.h
+++ b/include/configs/vexpress_ca15_tc2.h
@@ -31,6 +31,4 @@
 #include "vexpress_common.h"
 #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
 
-#define CONFIG_SYS_CLK_FREQ 24000000
-
 #endif
-- 
1.7.12.1

[U-Boot] [PATCH v2 5/7] ARM: add SMP support for non-secure switch

[Andre Przywara]

Currently the non-secure switch is only done for the boot processor.
To enable 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/nonsec_virt.S    | 27 +++++++++++++++++++++++++++
 arch/arm/include/asm/armv7.h        |  1 +
 arch/arm/lib/virt-v7.c              | 19 ++++++++++++++++++-
 include/configs/vexpress_ca15_tc2.h |  3 +++
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
index 656d99b..919f6e9 100644
--- a/arch/arm/cpu/armv7/nonsec_virt.S
+++ b/arch/arm/cpu/armv7/nonsec_virt.S
@@ -54,6 +54,33 @@ _secure_monitor:
 
 	movs	pc, lr				@ return to non-secure SVC
 
+/*
+ * Secondary CPUs start here and call the code for the core specific parts
+ * of the non-secure and HYP mode transition. The GIC distributor specific
+ * code has already been executed by a C function before.
+ * Then they go back to wfi and wait to be woken up by the kernel again.
+ */
+.globl _smp_pen
+_smp_pen:
+	mrs	r0, cpsr
+	orr	r0, r0, #0xc0
+	msr	cpsr, r0			@ disable interrupts
+	ldr	r1, =_start
+	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
+
+	bl	_nonsec_init
+
+	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
+
 #define lo(x) ((x) & 0xFFFF)
 #define hi(x) ((x) >> 16)
 
diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index 56d0dd0..04545b9 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -97,6 +97,7 @@ int armv7_switch_nonsec(void);
 
 /* defined in cpu/armv7/nonsec_virt.S */
 void _nonsec_init(void);
+void _smp_pen(void);
 #endif /* CONFIG_ARMV7_VIRT */
 
 #endif /* ! __ASSEMBLY__ */
diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
index 7876a77..6946e4d 100644
--- a/arch/arm/lib/virt-v7.c
+++ b/arch/arm/lib/virt-v7.c
@@ -95,6 +95,21 @@ static int get_gic_base_address(char **gicdptr)
 #endif
 }
 
+static void kick_secondary_cpus(char *gicdptr)
+{
+	unsigned int *sysflags;
+
+	sysflags = (void *)CONFIG_SYSFLAGS_ADDR;
+#ifdef CONFIG_SYSFLAGS_NEED_CLEAR_BITS
+	sysflags[1] = (unsigned)-1;
+#endif
+	*sysflags = (uintptr_t)_smp_pen;
+	dmb();
+
+	/* now kick all CPUs (expect this one) by writing to GICD_SGIR */
+	writel(1U << 24, &gicdptr[GICD_SGIR]);
+}
+
 int armv7_switch_nonsec(void)
 {
 	unsigned int reg, ret;
@@ -130,7 +145,9 @@ int armv7_switch_nonsec(void)
 	for (i = 0; i <= itlinesnr; i++)
 		writel((unsigned)-1, &gicdptr[GICD_IGROUPRn + 4 * i]);
 
-	/* call the non-sec switching code on this CPU */
+	kick_secondary_cpus(gicdptr);
+
+	/* call the non-sec switching code on this CPU also */
 	_nonsec_init();
 
 	return 0;
diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
index 4f425ac..ade9e5b 100644
--- a/include/configs/vexpress_ca15_tc2.h
+++ b/include/configs/vexpress_ca15_tc2.h
@@ -31,4 +31,7 @@
 #include "vexpress_common.h"
 #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
 
+#define CONFIG_SYSFLAGS_ADDR 0x1c010030
+#define CONFIG_SYSFLAGS_NEED_CLEAR_BITS
+
 #endif
-- 
1.7.12.1

[U-Boot] [PATCH v2 6/7] 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 our new secure exception vector table.

In the assembly switching routine 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/nonsec_virt.S | 31 ++++++++++++++++++++++++++++---
 arch/arm/include/asm/armv7.h     |  7 +++++--
 arch/arm/lib/bootm.c             | 14 ++++++++++----
 arch/arm/lib/virt-v7.c           | 27 ++++++++++++++++++++++-----
 4 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
index 919f6e9..950da6f 100644
--- a/arch/arm/cpu/armv7/nonsec_virt.S
+++ b/arch/arm/cpu/armv7/nonsec_virt.S
@@ -1,5 +1,5 @@
 /*
- * code for switching cores into non-secure state
+ * code for switching cores into non-secure state and into HYP mode
  *
  * Copyright (c) 2013	Andre Przywara <andre.przywara@linaro.org>
  *
@@ -26,14 +26,14 @@
 #include <asm/gic.h>
 #include <asm/armv7.h>
 
-/* the vector table for secure state */
+/* the vector table for secure state and HYP mode */
 _secure_vectors:
 	.word 0	/* reset */
 	.word 0 /* undef */
 	adr pc, _secure_monitor
 	.word 0
 	.word 0
-	.word 0
+	adr pc, _hyp_trap
 	.word 0
 	.word 0
 	.word 0	/* pad */
@@ -50,10 +50,23 @@ _secure_monitor:
 	bic	r1, r1, #0x4e			@ clear IRQ, FIQ, EA, nET bits
 	orr	r1, r1, #0x31			@ enable NS, AW, FW bits
 
+	mrc	p15, 0, r0, c0, c1, 1		@ read ID_PFR1
+	and	r0, r0, #CPUID_ARM_VIRT_MASK	@ mask virtualization bits
+	cmp	r0, #(1 << CPUID_ARM_VIRT_SHIFT)
+	orreq	r1, r1, #0x100			@ allow HVC instruction
+
 	mcr	p15, 0, r1, c1, c1, 0		@ write SCR (with NS bit set)
 
+	mrceq	p15, 0, r0, c12, c0, 1		@ get MVBAR value
+	mcreq	p15, 4, r0, c12, c0, 0		@ write HVBAR
+
 	movs	pc, lr				@ return to non-secure SVC
 
+_hyp_trap:
+	.byte 0x00, 0xe3, 0x0e, 0xe1		@ mrs lr, elr_hyp
+	mov pc, lr				@ do no switch modes, but
+						@ return to caller
+
 /*
  * Secondary CPUs start here and call the code for the core specific parts
  * of the non-secure and HYP mode transition. The GIC distributor specific
@@ -69,6 +82,7 @@ _smp_pen:
 	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
 
 	bl	_nonsec_init
+	bl	_hyp_init
 
 	ldr	r1, [r3, #0x0c]			@ read GICD acknowledge
 	str	r1, [r3, #0x10]			@ write GICD EOI
@@ -145,3 +159,14 @@ _nonsec_init:
 	str	r1, [r2]			@ allow private interrupts
 
 	bx	lr
+
+.globl _hyp_init
+_hyp_init:
+	mov	r2, lr
+	mov	r3, sp				@ save SVC copy of LR and SP
+	isb
+	.byte 0x70, 0x00, 0x40, 0xe1		@ hvc #0
+	mov	sp, r3
+	mov	lr, r2				@ fix HYP mode banked LR and SP
+
+	bx	lr
diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index 04545b9..8c3a85e 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -89,15 +89,18 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
 
 #ifdef CONFIG_ARMV7_VIRT
 
-#define HYP_ERR_NO_SEC_EXT		2
+#define HYP_ERR_ALREADY_HYP_MODE	1
+#define HYP_ERR_NO_VIRT_EXT		2
 #define HYP_ERR_NO_GIC_ADDRESS		3
 #define HYP_ERR_GIC_ADDRESS_ABOVE_4GB	4
+#define HYP_ERR_NOT_HYP_MODE		5
 
-int armv7_switch_nonsec(void);
+int armv7_switch_hyp(void);
 
 /* defined in cpu/armv7/nonsec_virt.S */
 void _nonsec_init(void);
 void _smp_pen(void);
+void _hyp_init(void);
 #endif /* CONFIG_ARMV7_VIRT */
 
 #endif /* ! __ASSEMBLY__ */
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 8251a89..7edd84d 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -227,12 +227,15 @@ static void boot_prep_linux(bootm_headers_t *images)
 		hang();
 	}
 #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 HYP_ERR_NO_SEC_EXT:
-		printf("HYP mode: Security extensions not implemented.\n");
+	case HYP_ERR_ALREADY_HYP_MODE:
+		debug("CPU already in HYP mode\n");
+		break;
+	case HYP_ERR_NO_VIRT_EXT:
+		printf("HYP mode: Virtualization extensions not implemented.\n");
 		break;
 	case HYP_ERR_NO_GIC_ADDRESS:
 		printf("HYP mode: could not determine GIC address.\n");
@@ -240,6 +243,9 @@ static void boot_prep_linux(bootm_headers_t *images)
 	case HYP_ERR_GIC_ADDRESS_ABOVE_4GB:
 		printf("HYP mode: PERIPHBASE is above 4 GB, cannot access this.\n");
 		break;
+	case HYP_ERR_NOT_HYP_MODE:
+		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 6946e4d..1e206b9 100644
--- a/arch/arm/lib/virt-v7.c
+++ b/arch/arm/lib/virt-v7.c
@@ -3,6 +3,7 @@
  * Andre Przywara, Linaro
  *
  * Routines to transition 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
@@ -29,6 +30,14 @@
 #include <asm/gic.h>
 #include <asm/io.h>
 
+static unsigned int read_cpsr(void)
+{
+	unsigned int reg;
+
+	asm volatile ("mrs %0, cpsr\n" : "=r" (reg));
+	return reg;
+}
+
 static unsigned int read_id_pfr1(void)
 {
 	unsigned int reg;
@@ -110,16 +119,20 @@ static void kick_secondary_cpus(char *gicdptr)
 	writel(1U << 24, &gicdptr[GICD_SGIR]);
 }
 
-int armv7_switch_nonsec(void)
+int armv7_switch_hyp(void)
 {
 	unsigned int reg, ret;
 	char *gicdptr;
 	unsigned itlinesnr, i;
 
-	/* check whether the CPU supports the security extensions */
+	/* check whether we are in HYP mode already */
+	if ((read_cpsr() & 0x1f) == 0x1a)
+		return HYP_ERR_ALREADY_HYP_MODE;
+
+	/* check whether the CPU supports the virtualization extensions */
 	reg = read_id_pfr1();
-	if ((reg & 0xF0) == 0)
-		return HYP_ERR_NO_SEC_EXT;
+	if ((reg & CPUID_ARM_VIRT_MASK) != 1 << CPUID_ARM_VIRT_SHIFT)
+		return HYP_ERR_NO_VIRT_EXT;
 
 	set_generic_timer_frequency();
 
@@ -147,8 +160,12 @@ int armv7_switch_nonsec(void)
 
 	kick_secondary_cpus(gicdptr);
 
-	/* call the non-sec switching code on this CPU also */
+	/* call the HYP switching code on this CPU also */
 	_nonsec_init();
+	_hyp_init();
+
+	if ((read_cpsr() & 0x1F) != 0x1a)
+		return HYP_ERR_NOT_HYP_MODE;
 
 	return 0;
 }
-- 
1.7.12.1

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

[Andre Przywara]

To enable hypervisors utilizing the ARMv7 virtualization extension
on the Versatile Express board with the A15 core tile, we add the
respective configuration variable.

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 ade9e5b..47dc5f3 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_SYSFLAGS_ADDR 0x1c010030
 #define CONFIG_SYSFLAGS_NEED_CLEAR_BITS
 
-- 
1.7.12.1

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

[Christoffer Dall]

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

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

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

"With the GIC accessible" ?

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

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

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

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

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

you also use r12 (ip) ?

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

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

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

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

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

Forgot to push the last revision?

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

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

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

Looks great otherwise.

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

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

-Christoffer

[U-Boot] [PATCH v2 4/7] ARM: switch to non-secure state during bootm execution

[Christoffer Dall]

On Thu, Jun 13, 2013 at 01:01:10PM +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.
> 
> The core specific part of the work is done in the assembly routine
> in nonsec_virt.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.
> 
> We check the availability of the security extensions first.
> 
> 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.
> The Versatile Express board does not need this, so we remove the
> frequency from the configuration file here.
> 
> 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.
> Board not implementing the CBAR can override this value via a
> configuration file variable.
> 
> 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.
> 
> 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              | 137 ++++++++++++++++++++++++++++++++++++
>  include/configs/vexpress_ca15_tc2.h |   2 -
>  5 files changed, 166 insertions(+), 2 deletions(-)
>  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 989bb72..56d0dd0 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -88,6 +88,13 @@ void v7_outer_cache_flush_range(u32 start, u32 end);
>  void v7_outer_cache_inval_range(u32 start, u32 end);
>  
>  #ifdef CONFIG_ARMV7_VIRT
> +
> +#define HYP_ERR_NO_SEC_EXT		2
> +#define HYP_ERR_NO_GIC_ADDRESS		3
> +#define HYP_ERR_GIC_ADDRESS_ABOVE_4GB	4

enum?

> +
> +int armv7_switch_nonsec(void);
> +
>  /* defined in cpu/armv7/nonsec_virt.S */
>  void _nonsec_init(void);
>  #endif /* CONFIG_ARMV7_VIRT */
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index 8ad9f66..1570ad5 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -60,6 +60,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 1b6e0ac..8251a89 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -34,6 +34,10 @@
>  #include <asm/bootm.h>
>  #include <linux/compiler.h>
>  
> +#ifdef CONFIG_ARMV7_VIRT
> +#include <asm/armv7.h>
> +#endif
> +
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  static struct tag *params;
> @@ -222,6 +226,22 @@ static void boot_prep_linux(bootm_headers_t *images)
>  		printf("FDT and ATAGS support not compiled in - hanging\n");
>  		hang();
>  	}
> +#ifdef CONFIG_ARMV7_VIRT
> +	switch (armv7_switch_nonsec()) {
> +	case 0:
> +		debug("entered non-secure state\n");
> +		break;

this is weird, why not have a define for the success case?

I still think the debug printing should be done inside
armv7_switch_nonsec instead, and you can just have it be a void();

> +	case HYP_ERR_NO_SEC_EXT:
> +		printf("HYP mode: Security extensions not implemented.\n");
> +		break;
> +	case HYP_ERR_NO_GIC_ADDRESS:
> +		printf("HYP mode: could not determine GIC address.\n");
> +		break;
> +	case HYP_ERR_GIC_ADDRESS_ABOVE_4GB:
> +		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..7876a77
> --- /dev/null
> +++ b/arch/arm/lib/virt-v7.c
> @@ -0,0 +1,137 @@
> +/*
> + * (C) Copyright 2013
> + * Andre Przywara, Linaro
> + *
> + * Routines to transition 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>
> +#include <asm/gic.h>
> +#include <asm/io.h>
> +
> +static unsigned int read_id_pfr1(void)
> +{
> +	unsigned int reg;
> +
> +	asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> +	return reg;
> +}
> +
> +/* The timer frequency for the generic timer needs to be
> + * programmed in secure state. Some primary bootloaders / firmware
> + * omit this, so if the frequency is provided in the configuration,
> + * we do this here instead.
> + * But first check if we have the generic timer.
> + */
> +static void set_generic_timer_frequency(void)
> +{
> +#ifdef CONFIG_SYS_CLK_FREQ
> +	unsigned int reg;
> +
> +	reg = read_id_pfr1();
> +	if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
> +		asm("mcr p15, 0, %0, c14, c0, 0\n"
> +		: : "r"(CONFIG_SYS_CLK_FREQ));
> +#endif
> +}
> +
> +static int get_gic_base_address(char **gicdptr)

you could simplify this function and make it an unsigned function and
return (unsigned)-1 on error (and do the debug error print right away in
there).

> +{
> +#ifdef CONFIG_ARM_GIC_BASE_ADDRESS
> +	*gicdptr = (void *)(CONFIG_ARM_GIC_BASE_ADDRESS + GIC_DIST_OFFSET);
> +	return 0;
> +#else
> +	unsigned midr;
> +	unsigned periphbase;
> +
> +	/* check whether we are an Cortex-A15 or A7.
> +	 * 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"(midr));
> +	switch (midr & MIDR_PRIMARY_PART_MASK) {
> +	case MIDR_CORTEX_A9_R0P1:
> +	case MIDR_CORTEX_A15_R0P0:
> +	case MIDR_CORTEX_A7_R0P0:
> +		break;
> +	default:
> +		return HYP_ERR_NO_GIC_ADDRESS;
> +	}
> +
> +	/* get the GIC base address from the CBAR register */
> +	asm("mrc p15, 4, %0, c15, c0, 0\n" : "=r" (periphbase));
> +
> +	/* 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 ((periphbase & 0xff) != 0)
> +		return HYP_ERR_GIC_ADDRESS_ABOVE_4GB;
> +
> +	*gicdptr = (void *)(periphbase + GIC_DIST_OFFSET);

this is weird, the function is called get_gic_base_address, but you're
returning the distributor base address, and the GIC_DIST_OFFSET is
actually an A15/A7 specific thing.

> +
> +	return 0;
> +#endif
> +}
> +
> +int armv7_switch_nonsec(void)
> +{
> +	unsigned int reg, ret;
> +	char *gicdptr;

there's really no need having this be a pointer when you use writel /
readl to access it.

> +	unsigned itlinesnr, i;
> +
> +	/* check whether the CPU supports the security extensions */
> +	reg = read_id_pfr1();
> +	if ((reg & 0xF0) == 0)
> +		return HYP_ERR_NO_SEC_EXT;
> +
> +	set_generic_timer_frequency();
> +
> +	/* 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.
> +	 */
> +
> +	ret = get_gic_base_address(&gicdptr);
> +	if (ret != 0)
> +		return ret;
> +
> +	/* enable the GIC distributor */
> +	writel(readl(&gicdptr[GICD_CTLR]) | 0x03, &gicdptr[GICD_CTLR]);

I would do the readl first, and then the writel, but it's just a matter
of style.

"gicdptr + GICD_CTLR" seems cleaner

> +
> +	/* TYPER[4:0] contains an encoded number of all interrupts */

nit: s/all/avail./

> +	itlinesnr = readl(&gicdptr[GICD_TYPER]) & 0x1f;
> +
> +	/* set all bits in the GIC group registers to one to allow access
> +	 * from non-secure state
> +	 */
> +	for (i = 0; i <= itlinesnr; i++)
> +		writel((unsigned)-1, &gicdptr[GICD_IGROUPRn + 4 * i]);
> +
> +	/* call the non-sec switching code on this CPU */
> +	_nonsec_init();
> +
> +	return 0;
> +}
> diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
> index 9e230ad..4f425ac 100644
> --- a/include/configs/vexpress_ca15_tc2.h
> +++ b/include/configs/vexpress_ca15_tc2.h
> @@ -31,6 +31,4 @@
>  #include "vexpress_common.h"
>  #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
>  
> -#define CONFIG_SYS_CLK_FREQ 24000000
> -
>  #endif
> -- 
> 1.7.12.1
> 

Besides my crazy nit-picking, this looks good to me.

-Christoffer

[U-Boot] [PATCH v2 5/7] ARM: add SMP support for non-secure switch

[Christoffer Dall]

On Thu, Jun 13, 2013 at 01:01:11PM +0200, Andre Przywara wrote:
> Currently the non-secure switch is only done for the boot processor.
> To enable 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/nonsec_virt.S    | 27 +++++++++++++++++++++++++++
>  arch/arm/include/asm/armv7.h        |  1 +
>  arch/arm/lib/virt-v7.c              | 19 ++++++++++++++++++-
>  include/configs/vexpress_ca15_tc2.h |  3 +++
>  4 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
> index 656d99b..919f6e9 100644
> --- a/arch/arm/cpu/armv7/nonsec_virt.S
> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
> @@ -54,6 +54,33 @@ _secure_monitor:
>  
>  	movs	pc, lr				@ return to non-secure SVC
>  
> +/*
> + * Secondary CPUs start here and call the code for the core specific parts
> + * of the non-secure and HYP mode transition. The GIC distributor specific
> + * code has already been executed by a C function before.
> + * Then they go back to wfi and wait to be woken up by the kernel again.
> + */
> +.globl _smp_pen
> +_smp_pen:
> +	mrs	r0, cpsr
> +	orr	r0, r0, #0xc0
> +	msr	cpsr, r0			@ disable interrupts
> +	ldr	r1, =_start
> +	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
> +
> +	bl	_nonsec_init
> +
> +	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

the name sysflags addr is a very vexpress specific thingy.

I think you need to call this something like SMP_SECONDARY_BOOT_ADDR or
whatever, which is more generic.

> +	ldr	r0, [r0]
> +	cmp	r0, r1			@ make sure we dont execute this code
> +	beq	waitloop		@ again (due to a spurious wakeup)
> +	mov	pc, r0
> +
>  #define lo(x) ((x) & 0xFFFF)
>  #define hi(x) ((x) >> 16)
>  
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index 56d0dd0..04545b9 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -97,6 +97,7 @@ int armv7_switch_nonsec(void);
>  
>  /* defined in cpu/armv7/nonsec_virt.S */
>  void _nonsec_init(void);
> +void _smp_pen(void);
>  #endif /* CONFIG_ARMV7_VIRT */
>  
>  #endif /* ! __ASSEMBLY__ */
> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
> index 7876a77..6946e4d 100644
> --- a/arch/arm/lib/virt-v7.c
> +++ b/arch/arm/lib/virt-v7.c
> @@ -95,6 +95,21 @@ static int get_gic_base_address(char **gicdptr)
>  #endif
>  }
>  
> +static void kick_secondary_cpus(char *gicdptr)
> +{
> +	unsigned int *sysflags;

again, I think the use of the name sysflags here is misunderstood.

> +
> +	sysflags = (void *)CONFIG_SYSFLAGS_ADDR;
> +#ifdef CONFIG_SYSFLAGS_NEED_CLEAR_BITS
> +	sysflags[1] = (unsigned)-1;

this feels like we're wrapping some vexpress-logic into some
pseudo-generic logic here.  It feels like there should be a function
provided by the board code, that we simply call.

void set_board_smp_boot_addr(unsigned long addr);

> +#endif
> +	*sysflags = (uintptr_t)_smp_pen;
> +	dmb();
> +
> +	/* now kick all CPUs (expect this one) by writing to GICD_SGIR */

s/expect/except/

> +	writel(1U << 24, &gicdptr[GICD_SGIR]);
> +}
> +
>  int armv7_switch_nonsec(void)
>  {
>  	unsigned int reg, ret;
> @@ -130,7 +145,9 @@ int armv7_switch_nonsec(void)
>  	for (i = 0; i <= itlinesnr; i++)
>  		writel((unsigned)-1, &gicdptr[GICD_IGROUPRn + 4 * i]);
>  
> -	/* call the non-sec switching code on this CPU */
> +	kick_secondary_cpus(gicdptr);
> +
> +	/* call the non-sec switching code on this CPU also */
>  	_nonsec_init();
>  
>  	return 0;
> diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
> index 4f425ac..ade9e5b 100644
> --- a/include/configs/vexpress_ca15_tc2.h
> +++ b/include/configs/vexpress_ca15_tc2.h
> @@ -31,4 +31,7 @@
>  #include "vexpress_common.h"
>  #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
>  
> +#define CONFIG_SYSFLAGS_ADDR 0x1c010030
> +#define CONFIG_SYSFLAGS_NEED_CLEAR_BITS
> +
>  #endif
> -- 
> 1.7.12.1
> 

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

[Christoffer Dall]

On Thu, Jun 13, 2013 at 01:01:12PM +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 our new secure exception vector table.
> 
> In the assembly switching routine 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/nonsec_virt.S | 31 ++++++++++++++++++++++++++++---
>  arch/arm/include/asm/armv7.h     |  7 +++++--
>  arch/arm/lib/bootm.c             | 14 ++++++++++----
>  arch/arm/lib/virt-v7.c           | 27 ++++++++++++++++++++++-----
>  4 files changed, 65 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
> index 919f6e9..950da6f 100644
> --- a/arch/arm/cpu/armv7/nonsec_virt.S
> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
> @@ -1,5 +1,5 @@
>  /*
> - * code for switching cores into non-secure state
> + * code for switching cores into non-secure state and into HYP mode
>   *
>   * Copyright (c) 2013	Andre Przywara <andre.przywara@linaro.org>
>   *
> @@ -26,14 +26,14 @@
>  #include <asm/gic.h>
>  #include <asm/armv7.h>
>  
> -/* the vector table for secure state */
> +/* the vector table for secure state and HYP mode */
>  _secure_vectors:
>  	.word 0	/* reset */
>  	.word 0 /* undef */
>  	adr pc, _secure_monitor
>  	.word 0
>  	.word 0
> -	.word 0
> +	adr pc, _hyp_trap
>  	.word 0
>  	.word 0
>  	.word 0	/* pad */
> @@ -50,10 +50,23 @@ _secure_monitor:
>  	bic	r1, r1, #0x4e			@ clear IRQ, FIQ, EA, nET bits
>  	orr	r1, r1, #0x31			@ enable NS, AW, FW bits
>  
> +	mrc	p15, 0, r0, c0, c1, 1		@ read ID_PFR1
> +	and	r0, r0, #CPUID_ARM_VIRT_MASK	@ mask virtualization bits
> +	cmp	r0, #(1 << CPUID_ARM_VIRT_SHIFT)
> +	orreq	r1, r1, #0x100			@ allow HVC instruction
> +
>  	mcr	p15, 0, r1, c1, c1, 0		@ write SCR (with NS bit set)
>  
> +	mrceq	p15, 0, r0, c12, c0, 1		@ get MVBAR value

Why not just do load the address of _secure_vectors directly?  I think it
makes it more clear what happens.

> +	mcreq	p15, 4, r0, c12, c0, 0		@ write HVBAR
> +
>  	movs	pc, lr				@ return to non-secure SVC
>  
> +_hyp_trap:
> +	.byte 0x00, 0xe3, 0x0e, 0xe1		@ mrs lr, elr_hyp

Again, why not just add the necessary .arch_extension or assembler
directive in the makefile and use the instructions directly.

The only reason I would see for performing this obscurity would be to
support really old compilers, which I doubt we fill for future boards?

> +	mov pc, lr				@ do no switch modes, but
> +						@ return to caller
> +
>  /*
>   * Secondary CPUs start here and call the code for the core specific parts
>   * of the non-secure and HYP mode transition. The GIC distributor specific
> @@ -69,6 +82,7 @@ _smp_pen:
>  	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
>  
>  	bl	_nonsec_init
> +	bl	_hyp_init
>  
>  	ldr	r1, [r3, #0x0c]			@ read GICD acknowledge
>  	str	r1, [r3, #0x10]			@ write GICD EOI
> @@ -145,3 +159,14 @@ _nonsec_init:
>  	str	r1, [r2]			@ allow private interrupts
>  
>  	bx	lr
> +
> +.globl _hyp_init
> +_hyp_init:

nit: the naming here is a little misleading for someone not knowing
what's going on. You're not really initializing the mode, but switching
to it:

_switch_to_hyp: ???

> +	mov	r2, lr
> +	mov	r3, sp				@ save SVC copy of LR and SP
> +	isb

I don't think this isb is necessary.

> +	.byte 0x70, 0x00, 0x40, 0xe1		@ hvc #0

again, this is really funky.  If it's really necessary, can we please
have a define somewhere so you can just do HVC(0) instead?

> +	mov	sp, r3
> +	mov	lr, r2				@ fix HYP mode banked LR and SP

nit: s/fix HYP mode/restore S-SVC mode/

> +
> +	bx	lr
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index 04545b9..8c3a85e 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -89,15 +89,18 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
>  
>  #ifdef CONFIG_ARMV7_VIRT
>  
> -#define HYP_ERR_NO_SEC_EXT		2
> +#define HYP_ERR_ALREADY_HYP_MODE	1
> +#define HYP_ERR_NO_VIRT_EXT		2
>  #define HYP_ERR_NO_GIC_ADDRESS		3
>  #define HYP_ERR_GIC_ADDRESS_ABOVE_4GB	4
> +#define HYP_ERR_NOT_HYP_MODE		5

see, this is just messy. I really don't think you need to propogate
error values like this.

>  
> -int armv7_switch_nonsec(void);
> +int armv7_switch_hyp(void);
>  
>  /* defined in cpu/armv7/nonsec_virt.S */
>  void _nonsec_init(void);
>  void _smp_pen(void);
> +void _hyp_init(void);
>  #endif /* CONFIG_ARMV7_VIRT */
>  
>  #endif /* ! __ASSEMBLY__ */
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 8251a89..7edd84d 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -227,12 +227,15 @@ static void boot_prep_linux(bootm_headers_t *images)
>  		hang();
>  	}
>  #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 HYP_ERR_NO_SEC_EXT:
> -		printf("HYP mode: Security extensions not implemented.\n");
> +	case HYP_ERR_ALREADY_HYP_MODE:
> +		debug("CPU already in HYP mode\n");
> +		break;
> +	case HYP_ERR_NO_VIRT_EXT:
> +		printf("HYP mode: Virtualization extensions not implemented.\n");
>  		break;
>  	case HYP_ERR_NO_GIC_ADDRESS:
>  		printf("HYP mode: could not determine GIC address.\n");
> @@ -240,6 +243,9 @@ static void boot_prep_linux(bootm_headers_t *images)
>  	case HYP_ERR_GIC_ADDRESS_ABOVE_4GB:
>  		printf("HYP mode: PERIPHBASE is above 4 GB, cannot access this.\n");
>  		break;
> +	case HYP_ERR_NOT_HYP_MODE:
> +		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 6946e4d..1e206b9 100644
> --- a/arch/arm/lib/virt-v7.c
> +++ b/arch/arm/lib/virt-v7.c
> @@ -3,6 +3,7 @@
>   * Andre Przywara, Linaro
>   *
>   * Routines to transition 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
> @@ -29,6 +30,14 @@
>  #include <asm/gic.h>
>  #include <asm/io.h>
>  
> +static unsigned int read_cpsr(void)
> +{
> +	unsigned int reg;
> +
> +	asm volatile ("mrs %0, cpsr\n" : "=r" (reg));
> +	return reg;
> +}
> +
>  static unsigned int read_id_pfr1(void)
>  {
>  	unsigned int reg;
> @@ -110,16 +119,20 @@ static void kick_secondary_cpus(char *gicdptr)
>  	writel(1U << 24, &gicdptr[GICD_SGIR]);
>  }
>  
> -int armv7_switch_nonsec(void)
> +int armv7_switch_hyp(void)
>  {
>  	unsigned int reg, ret;
>  	char *gicdptr;
>  	unsigned itlinesnr, i;
>  
> -	/* check whether the CPU supports the security extensions */
> +	/* check whether we are in HYP mode already */
> +	if ((read_cpsr() & 0x1f) == 0x1a)
> +		return HYP_ERR_ALREADY_HYP_MODE;
> +
> +	/* check whether the CPU supports the virtualization extensions */
>  	reg = read_id_pfr1();
> -	if ((reg & 0xF0) == 0)
> -		return HYP_ERR_NO_SEC_EXT;
> +	if ((reg & CPUID_ARM_VIRT_MASK) != 1 << CPUID_ARM_VIRT_SHIFT)
> +		return HYP_ERR_NO_VIRT_EXT;
>  
>  	set_generic_timer_frequency();
>  
> @@ -147,8 +160,12 @@ int armv7_switch_nonsec(void)
>  
>  	kick_secondary_cpus(gicdptr);
>  
> -	/* call the non-sec switching code on this CPU also */
> +	/* call the HYP switching code on this CPU also */
>  	_nonsec_init();
> +	_hyp_init();
> +
> +	if ((read_cpsr() & 0x1F) != 0x1a)
> +		return HYP_ERR_NOT_HYP_MODE;
>  
>  	return 0;
>  }
> -- 
> 1.7.12.1
> 

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

[Nikolay Nikolaev]

Hello,


On Thu, Jun 13, 2013 at 2:01 PM, Andre Przywara
<andre.przywara@linaro.org>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 our new secure exception vector table.
>
> In the assembly switching routine 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/nonsec_virt.S | 31 ++++++++++++++++++++++++++++---
>  arch/arm/include/asm/armv7.h     |  7 +++++--
>  arch/arm/lib/bootm.c             | 14 ++++++++++----
>  arch/arm/lib/virt-v7.c           | 27 ++++++++++++++++++++++-----
>  4 files changed, 65 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S
> b/arch/arm/cpu/armv7/nonsec_virt.S
> index 919f6e9..950da6f 100644
> --- a/arch/arm/cpu/armv7/nonsec_virt.S
> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
> @@ -1,5 +1,5 @@
>  /*
> - * code for switching cores into non-secure state
> + * code for switching cores into non-secure state and into HYP mode
>   *
>   * Copyright (c) 2013  Andre Przywara <andre.przywara@linaro.org>
>   *
> @@ -26,14 +26,14 @@
>  #include <asm/gic.h>
>  #include <asm/armv7.h>
>
> -/* the vector table for secure state */
> +/* the vector table for secure state and HYP mode */
>  _secure_vectors:
>         .word 0 /* reset */
>         .word 0 /* undef */
>         adr pc, _secure_monitor
>         .word 0
>         .word 0
> -       .word 0
> +       adr pc, _hyp_trap
>         .word 0
>         .word 0
>         .word 0 /* pad */
> @@ -50,10 +50,23 @@ _secure_monitor:
>         bic     r1, r1, #0x4e                   @ clear IRQ, FIQ, EA, nET
> bits
>         orr     r1, r1, #0x31                   @ enable NS, AW, FW bits
>
> +       mrc     p15, 0, r0, c0, c1, 1           @ read ID_PFR1
> +       and     r0, r0, #CPUID_ARM_VIRT_MASK    @ mask virtualization bits
> +       cmp     r0, #(1 << CPUID_ARM_VIRT_SHIFT)
> +       orreq   r1, r1, #0x100                  @ allow HVC instruction
> +
>         mcr     p15, 0, r1, c1, c1, 0           @ write SCR (with NS bit
> set)
>
> +       mrceq   p15, 0, r0, c12, c0, 1          @ get MVBAR value
> +       mcreq   p15, 4, r0, c12, c0, 0          @ write HVBAR
> +
>         movs    pc, lr                          @ return to non-secure SVC
>
> +_hyp_trap:
> +       .byte 0x00, 0xe3, 0x0e, 0xe1            @ mrs lr, elr_hyp
> +       mov pc, lr                              @ do no switch modes, but
> +                                               @ return to caller
> +
>  /*
>   * Secondary CPUs start here and call the code for the core specific parts
>   * of the non-secure and HYP mode transition. The GIC distributor specific
> @@ -69,6 +82,7 @@ _smp_pen:
>         mcr     p15, 0, r1, c12, c0, 0          @ set VBAR
>
>         bl      _nonsec_init
> +       bl      _hyp_init
>

If I get it right, _nonsec_init stores  the GICC address.  Adding _hyp_init
here overwrites r3.
In effect the following lines do something on the stack (sp).

>
>         ldr     r1, [r3, #0x0c]                 @ read GICD acknowledge
>         str     r1, [r3, #0x10]                 @ write GICD EOI
>

can you add these 0x0c and 0x10 constants to gic.h.


> @@ -145,3 +159,14 @@ _nonsec_init:
>         str     r1, [r2]                        @ allow private interrupts
>
>         bx      lr
> +
> +.globl _hyp_init
> +_hyp_init:
> +       mov     r2, lr
> +       mov     r3, sp                          @ save SVC copy of LR and
> SP
> +       isb
> +       .byte 0x70, 0x00, 0x40, 0xe1            @ hvc #0
> +       mov     sp, r3
> +       mov     lr, r2                          @ fix HYP mode banked LR
> and SP
> +
> +       bx      lr
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index 04545b9..8c3a85e 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -89,15 +89,18 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
>
>  #ifdef CONFIG_ARMV7_VIRT
>
> -#define HYP_ERR_NO_SEC_EXT             2
> +#define HYP_ERR_ALREADY_HYP_MODE       1
> +#define HYP_ERR_NO_VIRT_EXT            2
>  #define HYP_ERR_NO_GIC_ADDRESS         3
>  #define HYP_ERR_GIC_ADDRESS_ABOVE_4GB  4
> +#define HYP_ERR_NOT_HYP_MODE           5
>
> -int armv7_switch_nonsec(void);
> +int armv7_switch_hyp(void);
>
>  /* defined in cpu/armv7/nonsec_virt.S */
>  void _nonsec_init(void);
>  void _smp_pen(void);
> +void _hyp_init(void);
>  #endif /* CONFIG_ARMV7_VIRT */
>
>  #endif /* ! __ASSEMBLY__ */
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 8251a89..7edd84d 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -227,12 +227,15 @@ static void boot_prep_linux(bootm_headers_t *images)
>                 hang();
>         }
>  #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 HYP_ERR_NO_SEC_EXT:
> -               printf("HYP mode: Security extensions not implemented.\n");
> +       case HYP_ERR_ALREADY_HYP_MODE:
> +               debug("CPU already in HYP mode\n");
> +               break;
> +       case HYP_ERR_NO_VIRT_EXT:
> +               printf("HYP mode: Virtualization extensions not
> implemented.\n");
>                 break;
>         case HYP_ERR_NO_GIC_ADDRESS:
>                 printf("HYP mode: could not determine GIC address.\n");
> @@ -240,6 +243,9 @@ static void boot_prep_linux(bootm_headers_t *images)
>         case HYP_ERR_GIC_ADDRESS_ABOVE_4GB:
>                 printf("HYP mode: PERIPHBASE is above 4 GB, cannot access
> this.\n");
>                 break;
> +       case HYP_ERR_NOT_HYP_MODE:
> +               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 6946e4d..1e206b9 100644
> --- a/arch/arm/lib/virt-v7.c
> +++ b/arch/arm/lib/virt-v7.c
> @@ -3,6 +3,7 @@
>   * Andre Przywara, Linaro
>   *
>   * Routines to transition 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
> @@ -29,6 +30,14 @@
>  #include <asm/gic.h>
>  #include <asm/io.h>
>
> +static unsigned int read_cpsr(void)
> +{
> +       unsigned int reg;
> +
> +       asm volatile ("mrs %0, cpsr\n" : "=r" (reg));
> +       return reg;
> +}
> +
>  static unsigned int read_id_pfr1(void)
>  {
>         unsigned int reg;
> @@ -110,16 +119,20 @@ static void kick_secondary_cpus(char *gicdptr)
>         writel(1U << 24, &gicdptr[GICD_SGIR]);
>  }
>
> -int armv7_switch_nonsec(void)
> +int armv7_switch_hyp(void)
>  {
>         unsigned int reg, ret;
>         char *gicdptr;
>         unsigned itlinesnr, i;
>
> -       /* check whether the CPU supports the security extensions */
> +       /* check whether we are in HYP mode already */
> +       if ((read_cpsr() & 0x1f) == 0x1a)
> +               return HYP_ERR_ALREADY_HYP_MODE;
> +
> +       /* check whether the CPU supports the virtualization extensions */
>         reg = read_id_pfr1();
> -       if ((reg & 0xF0) == 0)
> -               return HYP_ERR_NO_SEC_EXT;
> +       if ((reg & CPUID_ARM_VIRT_MASK) != 1 << CPUID_ARM_VIRT_SHIFT)
> +               return HYP_ERR_NO_VIRT_EXT;
>
>         set_generic_timer_frequency();
>
> @@ -147,8 +160,12 @@ int armv7_switch_nonsec(void)
>
>         kick_secondary_cpus(gicdptr);
>
> -       /* call the non-sec switching code on this CPU also */
> +       /* call the HYP switching code on this CPU also */
>         _nonsec_init();
> +       _hyp_init();
> +
> +       if ((read_cpsr() & 0x1F) != 0x1a)
> +               return HYP_ERR_NOT_HYP_MODE;
>
>         return 0;
>  }
> --
> 1.7.12.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
>

regards,
Nikolay Nikolaev

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

[Andre Przywara]

On 06/21/2013 04:38 PM, Nikolay Nikolaev wrote:
> Hello,
>
>
> On Thu, Jun 13, 2013 at 2:01 PM, Andre Przywara
> <andre.przywara at linaro.org <mailto:andre.przywara@linaro.org>> 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 our new secure exception vector table.
>
>     In the assembly switching routine 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
>     <mailto:andre.przywara@linaro.org>>
>     ---
>       arch/arm/cpu/armv7/nonsec_virt.S | 31 ++++++++++++++++++++++++++++---
>       arch/arm/include/asm/armv7.h     |  7 +++++--
>       arch/arm/lib/bootm.c             | 14 ++++++++++----
>       arch/arm/lib/virt-v7.c           | 27 ++++++++++++++++++++++-----
>       4 files changed, 65 insertions(+), 14 deletions(-)
>
>     diff --git a/arch/arm/cpu/armv7/nonsec_virt.S
>     b/arch/arm/cpu/armv7/nonsec_virt.S
>     index 919f6e9..950da6f 100644
>     --- a/arch/arm/cpu/armv7/nonsec_virt.S
>     +++ b/arch/arm/cpu/armv7/nonsec_virt.S
>     @@ -1,5 +1,5 @@
>       /*
>     - * code for switching cores into non-secure state
>     + * code for switching cores into non-secure state and into HYP mode
>        *
>        * Copyright (c) 2013  Andre Przywara <andre.przywara@linaro.org
>     <mailto:andre.przywara@linaro.org>>
>        *
>     @@ -26,14 +26,14 @@
>       #include <asm/gic.h>
>       #include <asm/armv7.h>
>
>     -/* the vector table for secure state */
>     +/* the vector table for secure state and HYP mode */
>       _secure_vectors:
>              .word 0 /* reset */
>              .word 0 /* undef */
>              adr pc, _secure_monitor
>              .word 0
>              .word 0
>     -       .word 0
>     +       adr pc, _hyp_trap
>              .word 0
>              .word 0
>              .word 0 /* pad */
>     @@ -50,10 +50,23 @@ _secure_monitor:
>              bic     r1, r1, #0x4e                   @ clear IRQ, FIQ,
>     EA, nET bits
>              orr     r1, r1, #0x31                   @ enable NS, AW, FW
>     bits
>
>     +       mrc     p15, 0, r0, c0, c1, 1           @ read ID_PFR1
>     +       and     r0, r0, #CPUID_ARM_VIRT_MASK    @ mask
>     virtualization bits
>     +       cmp     r0, #(1 << CPUID_ARM_VIRT_SHIFT)
>     +       orreq   r1, r1, #0x100                  @ allow HVC instruction
>     +
>              mcr     p15, 0, r1, c1, c1, 0           @ write SCR (with
>     NS bit set)
>
>     +       mrceq   p15, 0, r0, c12, c0, 1          @ get MVBAR value
>     +       mcreq   p15, 4, r0, c12, c0, 0          @ write HVBAR
>     +
>              movs    pc, lr                          @ return to
>     non-secure SVC
>
>     +_hyp_trap:
>     +       .byte 0x00, 0xe3, 0x0e, 0xe1            @ mrs lr, elr_hyp
>     +       mov pc, lr                              @ do no switch
>     modes, but
>     +                                               @ return to caller
>     +
>       /*
>        * Secondary CPUs start here and call the code for the core
>     specific parts
>        * of the non-secure and HYP mode transition. The GIC distributor
>     specific
>     @@ -69,6 +82,7 @@ _smp_pen:
>              mcr     p15, 0, r1, c12, c0, 0          @ set VBAR
>
>              bl      _nonsec_init
>     +       bl      _hyp_init
>
>
> If I get it right, _nonsec_init stores  the GICC address.  Adding
> _hyp_init here overwrites r3.
> In effect the following lines do something on the stack (sp).

Eek. Good catch. Thanks for spotting. The fix will be included in the 
next round.
Which kind of hints that acknowledging the wakeup IPI is not really 
necessary, as it was suspected earlier already.

>
>              ldr     r1, [r3, #0x0c]                 @ read GICD acknowledge
>              str     r1, [r3, #0x10]                 @ write GICD EOI
>
>
> can you add these 0x0c and 0x10 constants to gic.h.

Sure. And I will fix the wrong comments on the way ;-)

Thanks for the review!
Andre

>
>     @@ -145,3 +159,14 @@ _nonsec_init:
>              str     r1, [r2]                        @ allow private
>     interrupts
>
>              bx      lr
>     +
>     +.globl _hyp_init
>     +_hyp_init:
>     +       mov     r2, lr
>     +       mov     r3, sp                          @ save SVC copy of
>     LR and SP
>     +       isb
>     +       .byte 0x70, 0x00, 0x40, 0xe1            @ hvc #0
>     +       mov     sp, r3
>     +       mov     lr, r2                          @ fix HYP mode
>     banked LR and SP
>     +
>     +       bx      lr
>     diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
>     index 04545b9..8c3a85e 100644
>     --- a/arch/arm/include/asm/armv7.h
>     +++ b/arch/arm/include/asm/armv7.h
>     @@ -89,15 +89,18 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
>
>       #ifdef CONFIG_ARMV7_VIRT
>
>     -#define HYP_ERR_NO_SEC_EXT             2
>     +#define HYP_ERR_ALREADY_HYP_MODE       1
>     +#define HYP_ERR_NO_VIRT_EXT            2
>       #define HYP_ERR_NO_GIC_ADDRESS         3
>       #define HYP_ERR_GIC_ADDRESS_ABOVE_4GB  4
>     +#define HYP_ERR_NOT_HYP_MODE           5
>
>     -int armv7_switch_nonsec(void);
>     +int armv7_switch_hyp(void);
>
>       /* defined in cpu/armv7/nonsec_virt.S */
>       void _nonsec_init(void);
>       void _smp_pen(void);
>     +void _hyp_init(void);
>       #endif /* CONFIG_ARMV7_VIRT */
>
>       #endif /* ! __ASSEMBLY__ */
>     diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
>     index 8251a89..7edd84d 100644
>     --- a/arch/arm/lib/bootm.c
>     +++ b/arch/arm/lib/bootm.c
>     @@ -227,12 +227,15 @@ static void boot_prep_linux(bootm_headers_t
>     *images)
>                      hang();
>              }
>       #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 HYP_ERR_NO_SEC_EXT:
>     -               printf("HYP mode: Security extensions not
>     implemented.\n");
>     +       case HYP_ERR_ALREADY_HYP_MODE:
>     +               debug("CPU already in HYP mode\n");
>     +               break;
>     +       case HYP_ERR_NO_VIRT_EXT:
>     +               printf("HYP mode: Virtualization extensions not
>     implemented.\n");
>                      break;
>              case HYP_ERR_NO_GIC_ADDRESS:
>                      printf("HYP mode: could not determine GIC address.\n");
>     @@ -240,6 +243,9 @@ static void boot_prep_linux(bootm_headers_t *images)
>              case HYP_ERR_GIC_ADDRESS_ABOVE_4GB:
>                      printf("HYP mode: PERIPHBASE is above 4 GB, cannot
>     access this.\n");
>                      break;
>     +       case HYP_ERR_NOT_HYP_MODE:
>     +               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 6946e4d..1e206b9 100644
>     --- a/arch/arm/lib/virt-v7.c
>     +++ b/arch/arm/lib/virt-v7.c
>     @@ -3,6 +3,7 @@
>        * Andre Przywara, Linaro
>        *
>        * Routines to transition 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
>     @@ -29,6 +30,14 @@
>       #include <asm/gic.h>
>       #include <asm/io.h>
>
>     +static unsigned int read_cpsr(void)
>     +{
>     +       unsigned int reg;
>     +
>     +       asm volatile ("mrs %0, cpsr\n" : "=r" (reg));
>     +       return reg;
>     +}
>     +
>       static unsigned int read_id_pfr1(void)
>       {
>              unsigned int reg;
>     @@ -110,16 +119,20 @@ static void kick_secondary_cpus(char *gicdptr)
>              writel(1U << 24, &gicdptr[GICD_SGIR]);
>       }
>
>     -int armv7_switch_nonsec(void)
>     +int armv7_switch_hyp(void)
>       {
>              unsigned int reg, ret;
>              char *gicdptr;
>              unsigned itlinesnr, i;
>
>     -       /* check whether the CPU supports the security extensions */
>     +       /* check whether we are in HYP mode already */
>     +       if ((read_cpsr() & 0x1f) == 0x1a)
>     +               return HYP_ERR_ALREADY_HYP_MODE;
>     +
>     +       /* check whether the CPU supports the virtualization
>     extensions */
>              reg = read_id_pfr1();
>     -       if ((reg & 0xF0) == 0)
>     -               return HYP_ERR_NO_SEC_EXT;
>     +       if ((reg & CPUID_ARM_VIRT_MASK) != 1 << CPUID_ARM_VIRT_SHIFT)
>     +               return HYP_ERR_NO_VIRT_EXT;
>
>              set_generic_timer_frequency();
>
>     @@ -147,8 +160,12 @@ int armv7_switch_nonsec(void)
>
>              kick_secondary_cpus(gicdptr);
>
>     -       /* call the non-sec switching code on this CPU also */
>     +       /* call the HYP switching code on this CPU also */
>              _nonsec_init();
>     +       _hyp_init();
>     +
>     +       if ((read_cpsr() & 0x1F) != 0x1a)
>     +               return HYP_ERR_NOT_HYP_MODE;
>
>              return 0;
>       }
>     --
>     1.7.12.1
>
>     _______________________________________________
>     kvmarm mailing list
>     kvmarm at lists.cs.columbia.edu <mailto:kvmarm@lists.cs.columbia.edu>
>     https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
>
>
> regards,
> Nikolay Nikolaev

[U-Boot] [PATCH v2 1/7] ARM: prepare armv7.h to be included from assembly source

[Masahiro Yamada]

Hello Andre,

> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index a73630b..20caa7c 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -23,7 +23,6 @@
>   */
>  #ifndef ARMV7_H
>  #define ARMV7_H
> -#include <linux/types.h>
>  
>  /* Cortex-A9 revisions */
>  #define MIDR_CORTEX_A9_R0P1	0x410FC091
> @@ -57,6 +56,9 @@
>  #define ARMV7_CLIDR_CTYPE_INSTRUCTION_DATA	3
>  #define ARMV7_CLIDR_CTYPE_UNIFIED		4
>  
> +#ifndef __ASSEMBLY__
> +#include <linux/types.h>
> +
>  /*
>   * CP15 Barrier instructions
>   * Please note that we have separate barrier instructions in ARMv7
> @@ -74,4 +76,6 @@ 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);
>  
> +#endif /* ! __ASSEMBLY__ */
> +
>  #endif


Instread of moving #include <linux/types.h> line,
I'd like to suggest to add #ifndef __ASSEMBLY__ guard
to include/linux/types.h.
I think this is a more correct way of fixing.


If I see Linux Kernel "include/linux/types.h",
typedefs are placed?inside #ifndef  __ASSEMBLEY__ .. #endif.

On the other hand, "include/linux/types.h"?of U-Boot
does not have #ifndef __ASSEMBLY__ guard.



Best Regards
Masahiro Yamada

[U-Boot] [PATCH v2 2/7] ARM: add secure monitor handler to switch to non-secure state

[Masahiro Yamada]

Hi, Andre

> +_secure_vectors:
> +	.word 0	/* reset */
> +	.word 0 /* undef */
> +	adr pc, _secure_monitor
> +	.word 0
> +	.word 0
> +	.word 0
> +	.word 0
> +	.word 0
> +	.word 0	/* pad */


I think "_monitor_vectors" seems more precise name than "_secure_vectors".

For example, MVBAR is a short for "Monitor Vector Base Address Register".


Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

Hi, Andre


> +.globl _nonsec_init
> +_nonsec_init:

These two lines can be written:

ENTRY(_nonsec_init)



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

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

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

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

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

This code relies on  the value of MIDR_CORTEX_A15_R0P0.



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


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

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

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




> +	bx	lr

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


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

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


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

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



Masahiro Yamada

[U-Boot] [PATCH v2 4/7] ARM: switch to non-secure state during bootm execution

[Masahiro Yamada]

Hi Andre.


> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -60,6 +60,8 @@ COBJS-y	+= reset.o
>  COBJS-y	+= cache.o
>  COBJS-y	+= cache-cp15.o
>  
> +COBJS-$(CONFIG_ARMV7_VIRT) += virt-v7.o
> +

Judging from the file name virt-v7.c,
you are thinkig this file is specific to ARMv7, aren't you?

If so, why don't you move this file
to arch/arm/cpu/armv7/ ?


> +static void set_generic_timer_frequency(void)
> +{
> +#ifdef CONFIG_SYS_CLK_FREQ
> +	unsigned int reg;
> +
> +	reg = read_id_pfr1();
> +	if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
> +		asm("mcr p15, 0, %0, c14, c0, 0\n"
> +		: : "r"(CONFIG_SYS_CLK_FREQ));
> +#endif

CPUID_ARM_TIMER_MASK
CPUID_ARM_TIMER_SHIFT

I think these macro names are vague.
There are Generic Timer, Global Timer, Private Timer etc.


Unlike other parts, the care for Cortex-A9 is missing here.
To be more generic, I'd like to suggest to allow Non-secure access
to Global/Private timers before switching to non-secure state.


How about like this for armv7_switch_nonsec function?


    /* check whether the CPU supports the security extensions */
    reg = read_id_pfr1();
    if ((reg & 0xF0) == 0)
            return HYP_ERR_NO_SEC_EXT;

    if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
            set_generic_timer_frequency();
    else
            /* Allow Non-secure access to Global/Private timers */
            writel(0xfff, periph_base + SCU_SNSAC);



For more info about SCU Non-secure Access Control (SNSAC) Register,
please refer Cortex A9 mpcore TRM.  page 2-11
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0407g/DDI0407G_cortex_a9_mpcore_r3p0_trm.pdf



> +	/* enable the GIC distributor */
> +	writel(readl(&gicdptr[GICD_CTLR]) | 0x03, &gicdptr[GICD_CTLR]);

I am not sure this code is really necessary.

Because your are setting all available interrupts to Group1 just below,
I think we don't need to do this in secure state.


Best Regards
Masahiro Yamada

[U-Boot] [PATCH v2 5/7] ARM: add SMP support for non-secure switch

[Masahiro Yamada]

Hi Andre,


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

This issue might have been raised already,
I think how to kick secondary CPUs is board-specific.
Other boards might use "sev" instruction for that purpose.

> +.globl _smp_pen
> +_smp_pen:

ENTRY(_smp_pen)



Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

Hi Andre


[RFC]

I'd like to suggest to separate HYP-switching code
from Non-secure switching.

And define different macros, for example:

CONFIG_ARMV7_NONSECURE : switch to nonsecure
CONFIG_ARMV7_VIRT      : switch to hypervisor


Of cource, CONFIG_ARMV7_NONSECURE must be defined
when using CONFIG_ARMV7_VIRT.

(If we introduced Kconfig to U-boot,
we could handle nicely dependency between CONFIGs.)


I know your incentive to switch to non-secure state
is virtualization.
But I think this separation would make this code
useful for other boards and easy to understand.

For example (this situtation might be specific
to my board), non-secure switching is done
for the reason to use a hardware debugger,
because our debugger without security extension
can work only in non-secure state.



Best Regards
Masahiro Yamada

[U-Boot] [PATCH v2 1/7] ARM: prepare armv7.h to be included from assembly source

[Andre Przywara]

On 06/28/2013 03:00 AM, Masahiro Yamada wrote:
> Hello Andre,

Hi,

thanks a lot for the review! I included most of the fixes you proposed
in the next version I will send out soon.
Very useful comments, thanks again!

>> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
>> index a73630b..20caa7c 100644
>> --- a/arch/arm/include/asm/armv7.h
>> +++ b/arch/arm/include/asm/armv7.h
>> @@ -23,7 +23,6 @@
>>    */
>>   #ifndef ARMV7_H
>>   #define ARMV7_H
>> -#include <linux/types.h>
>>   
>>   /* Cortex-A9 revisions */
>>   #define MIDR_CORTEX_A9_R0P1	0x410FC091
>> @@ -57,6 +56,9 @@
>>   #define ARMV7_CLIDR_CTYPE_INSTRUCTION_DATA	3
>>   #define ARMV7_CLIDR_CTYPE_UNIFIED		4
>>   
>> +#ifndef __ASSEMBLY__
>> +#include <linux/types.h>
>> +
>>   /*
>>    * CP15 Barrier instructions
>>    * Please note that we have separate barrier instructions in ARMv7
>> @@ -74,4 +76,6 @@ 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);
>>   
>> +#endif /* ! __ASSEMBLY__ */
>> +
>>   #endif
> 
> 
> Instread of moving #include <linux/types.h> line,
> I'd like to suggest to add #ifndef __ASSEMBLY__ guard
> to include/linux/types.h.
> I think this is a more correct way of fixing.
> 
> 
> If I see Linux Kernel "include/linux/types.h",
> typedefs are placed?inside #ifndef  __ASSEMBLEY__ .. #endif.
> 
> On the other hand, "include/linux/types.h"?of U-Boot
> does not have #ifndef __ASSEMBLY__ guard.

I tried, but finally decided against it. It is not clear what parts of
types.h are actually usable from assembly files, so I stuck with my
solution. Feel free to send a follow-up patch if you think it's useful.

Regards,
Andre.

[U-Boot] [PATCH v2 4/7] ARM: switch to non-secure state during bootm execution

[Andre Przywara]

On 06/28/2013 05:18 AM, Masahiro Yamada wrote:
> Hi Andre.
>
>
>> --- a/arch/arm/lib/Makefile
>> +++ b/arch/arm/lib/Makefile
>> @@ -60,6 +60,8 @@ COBJS-y	+= reset.o
>>   COBJS-y	+= cache.o
>>   COBJS-y	+= cache-cp15.o
>>
>> +COBJS-$(CONFIG_ARMV7_VIRT) += virt-v7.o
>> +
>
> Judging from the file name virt-v7.c,
> you are thinkig this file is specific to ARMv7, aren't you?
>
> If so, why don't you move this file
> to arch/arm/cpu/armv7/ ?
>
>
>> +static void set_generic_timer_frequency(void)
>> +{
>> +#ifdef CONFIG_SYS_CLK_FREQ
>> +	unsigned int reg;
>> +
>> +	reg = read_id_pfr1();
>> +	if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
>> +		asm("mcr p15, 0, %0, c14, c0, 0\n"
>> +		: : "r"(CONFIG_SYS_CLK_FREQ));
>> +#endif
>
> CPUID_ARM_TIMER_MASK
> CPUID_ARM_TIMER_SHIFT
>
> I think these macro names are vague.
> There are Generic Timer, Global Timer, Private Timer etc.

Good point.

> Unlike other parts, the care for Cortex-A9 is missing here.
> To be more generic, I'd like to suggest to allow Non-secure access
> to Global/Private timers before switching to non-secure state.
>
>
> How about like this for armv7_switch_nonsec function?
>
>
>      /* check whether the CPU supports the security extensions */
>      reg = read_id_pfr1();
>      if ((reg & 0xF0) == 0)
>              return HYP_ERR_NO_SEC_EXT;
>
>      if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
>              set_generic_timer_frequency();
>      else
>              /* Allow Non-secure access to Global/Private timers */
>              writel(0xfff, periph_base + SCU_SNSAC);

Interesting, however I don't have access to an A9 board currently to 
properly test this. So I will do the renaming and let the access to the 
other timers up to a follow-up patch.

Thanks,
Andre.

>
> For more info about SCU Non-secure Access Control (SNSAC) Register,
> please refer Cortex A9 mpcore TRM.  page 2-11
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0407g/DDI0407G_cortex_a9_mpcore_r3p0_trm.pdf
>
>
>
>> +	/* enable the GIC distributor */
>> +	writel(readl(&gicdptr[GICD_CTLR]) | 0x03, &gicdptr[GICD_CTLR]);
>
> I am not sure this code is really necessary.
>
> Because your are setting all available interrupts to Group1 just below,
> I think we don't need to do this in secure state.
>
>
> Best Regards
> Masahiro Yamada
>

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

[Andre Przywara]

On 06/28/2013 05:51 AM, Masahiro Yamada wrote:
> Hi Andre
>
>
> [RFC]
>
> I'd like to suggest to separate HYP-switching code
> from Non-secure switching.

Thanks for stepping up and providing a use-case!
The first version of the patches had those two separate cases, but I
later merged them in favor of readability.
So I actually split those two cases in the code now and am about to fold 
this in the existing patches.

> And define different macros, for example:
>
> CONFIG_ARMV7_NONSECURE : switch to nonsecure
> CONFIG_ARMV7_VIRT      : switch to hypervisor

done.

>
> Of cource, CONFIG_ARMV7_NONSECURE must be defined
> when using CONFIG_ARMV7_VIRT.
>
> (If we introduced Kconfig to U-boot,
> we could handle nicely dependency between CONFIGs.)

I managed to get along without it. By clever use of ifdefs this 
dependency is now implicitly in the code.

I still have to test this, so the new version will be delayed a bit.

Thanks!
Andre.

>
> I know your incentive to switch to non-secure state
> is virtualization.
> But I think this separation would make this code
> useful for other boards and easy to understand.
>
> For example (this situtation might be specific
> to my board), non-secure switching is done
> for the reason to use a hardware debugger,
> because our debugger without security extension
> can work only in non-secure state.
>
>
>
> Best Regards
> Masahiro Yamada
>
>