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

login
register
mail settings
Submitter Andre Przywara
Date June 13, 2013, 11:01 a.m.
Message ID <1371121273-18763-7-git-send-email-andre.przywara@linaro.org>
Download mbox | patch
Permalink /patch/251052/
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Comments

Andre Przywara - June 13, 2013, 11:01 a.m.
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(-)
Christoffer Dall - June 19, 2013, 11:40 p.m.
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
>
Nikolay Nikolaev - June 21, 2013, 2:38 p.m.
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@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
>

regards,
Nikolay Nikolaev
Andre Przywara - June 25, 2013, 8:27 a.m.
On 06/21/2013 04:38 PM, Nikolay Nikolaev wrote:
> Hello,
>
>
> On Thu, Jun 13, 2013 at 2:01 PM, Andre Przywara
> <andre.przywara@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@lists.cs.columbia.edu <mailto:kvmarm@lists.cs.columbia.edu>
>     https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
>
>
> regards,
> Nikolay Nikolaev
Masahiro Yamada - June 28, 2013, 3:51 a.m.
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
Andre Przywara - July 4, 2013, 11:29 a.m.
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
>
>

Patch

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;
 }