diff mbox

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

Message ID 1373414059-22779-7-git-send-email-andre.przywara@linaro.org
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Commit Message

Andre Przywara July 9, 2013, 11:54 p.m. UTC 
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/Makefile      |  2 +-
 arch/arm/cpu/armv7/nonsec_virt.S | 43 +++++++++++++++++++++++++++++++++++-----
 arch/arm/cpu/armv7/virt-v7.c     | 31 +++++++++++++++++++++++++++++
 arch/arm/include/asm/armv7.h     |  9 +++++++--
 arch/arm/lib/bootm.c             | 19 +++++++++++++++---
 5 files changed, 93 insertions(+), 11 deletions(-)

Comments

Christoffer Dall July 29, 2013, 10:02 p.m. UTC | #1 
On Wed, Jul 10, 2013 at 01:54:18AM +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/Makefile      |  2 +-
>  arch/arm/cpu/armv7/nonsec_virt.S | 43 +++++++++++++++++++++++++++++++++++-----
>  arch/arm/cpu/armv7/virt-v7.c     | 31 +++++++++++++++++++++++++++++
>  arch/arm/include/asm/armv7.h     |  9 +++++++--
>  arch/arm/lib/bootm.c             | 19 +++++++++++++++---
>  5 files changed, 93 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
> index b59f59e..e5eaa56 100644
> --- a/arch/arm/cpu/armv7/Makefile
> +++ b/arch/arm/cpu/armv7/Makefile
> @@ -36,7 +36,7 @@ ifneq ($(CONFIG_AM33XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CONFIG_TEGRA)$(CONF
>  SOBJS	+= lowlevel_init.o
>  endif
>  
> -ifneq ($(CONFIG_ARMV7_NONSEC),)
> +ifneq ($(CONFIG_ARMV7_NONSEC)$(CONFIG_ARMV7_VIRT),)
>  SOBJS   += nonsec_virt.o
>  COBJS	+= virt-v7.o
>  endif
> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
> index f9b6b39..895c3b0 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>
>   *
> @@ -28,15 +28,16 @@
>  #include <asm/armv7.h>
>  
>  .arch_extension sec
> +.arch_extension virt
>  
> -/* the vector table for secure state */
> +/* the vector table for secure state and HYP mode */
>  _monitor_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 */
> @@ -53,10 +54,27 @@ _secure_monitor:
>  	bic	r1, r1, #0x4e			@ clear IRQ, FIQ, EA, nET bits
>  	orr	r1, r1, #0x31			@ enable NS, AW, FW bits
>  
> +#ifdef CONFIG_ARMV7_VIRT
> +	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
> +#endif
> +
>  	mcr	p15, 0, r1, c1, c1, 0		@ write SCR (with NS bit set)
>  
> +#ifdef CONFIG_ARMV7_VIRT
> +	mrceq	p15, 0, r0, c12, c0, 1		@ get MVBAR value
> +	mcreq	p15, 4, r0, c12, c0, 0		@ write HVBAR
> +#endif
> +
>  	movs	pc, lr				@ return to non-secure SVC
>  
> +_hyp_trap:
> +	mrs	lr, elr_hyp	@ for older asm: .byte 0x00, 0xe3, 0x0e, 0xe1

this comment just confuses: either make it intelligent to support an
older compiler or just get rid of these byte encodings.  You can always
disassemble the file and lookup the byte code with a modern compiler to
get back to the byte encoding.

> +	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
> @@ -71,9 +89,13 @@ ENTRY(_smp_pen)
>  	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
>  
>  	bl	_nonsec_init
> +	mov	r12, r0				@ save GICC address
> +#ifdef CONFIG_ARMV7_VIRT
> +	bl	_switch_to_hyp
> +#endif
>  
> -	ldr	r1, [r0, #GICC_IAR]		@ acknowledge IPI
> -	str	r1, [r0, #GICC_EOIR]		@ signal end of interrupt
> +	ldr	r1, [r12, #GICC_IAR]		@ acknowledge IPI
> +	str	r1, [r12, #GICC_EOIR]		@ signal end of interrupt
>  	adr	r1, _smp_pen
>  waitloop:
>  	wfi
> @@ -164,3 +186,14 @@ ENTRY(_nonsec_init)
>  
>  	bx	lr
>  ENDPROC(_nonsec_init)
> +
> +ENTRY(_switch_to_hyp)
> +	mov	r0, lr
> +	mov	r1, sp				@ save SVC copy of LR and SP
> +	isb

did you find out that this isb is indeed needed? if so, why?

> +	hvc #0			 @ for older asm: .byte 0x70, 0x00, 0x40, 0xe1

same comment as above?

> +	mov	sp, r1
> +	mov	lr, r0				@ restore SVC copy of LR and SP
> +
> +	bx	lr
> +ENDPROC(_switch_to_hyp)
> diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
> index a0d0b34..3645572 100644
> --- a/arch/arm/cpu/armv7/virt-v7.c
> +++ b/arch/arm/cpu/armv7/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;
> @@ -92,6 +101,28 @@ static void kick_secondary_cpus(unsigned int gicdaddr)
>  	writel(1U << 24, gicdaddr + GICD_SGIR);
>  }
>  
> +enum nonsec_virt_errors armv7_switch_hyp(void)
> +{
> +	unsigned int reg;
> +
> +	/* check whether we are in HYP mode already */
> +	if ((read_cpsr() & 0x1f) == 0x1a)
> +		return VIRT_ALREADY_HYP_MODE;
> +
> +	/* check whether the CPU supports the virtualization extensions */
> +	reg = read_id_pfr1();
> +	if ((reg & CPUID_ARM_VIRT_MASK) != 1 << CPUID_ARM_VIRT_SHIFT)
> +		return VIRT_ERR_NO_VIRT_EXT;
> +
> +	/* call the HYP switching code on this CPU also */
> +	_switch_to_hyp();
> +
> +	if ((read_cpsr() & 0x1F) != 0x1a)
> +		return VIRT_ERR_NOT_HYP_MODE;
> +
> +	return NONSEC_VIRT_SUCCESS;
> +}
> +
>  enum nonsec_virt_errors armv7_switch_nonsec(void)
>  {
>  	unsigned int reg, ret;
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index f6582a1..baa22fe 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -89,21 +89,26 @@ 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_NONSEC
> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>  
>  enum nonsec_virt_errors {
>  	NONSEC_VIRT_SUCCESS,
>  	NONSEC_ERR_NO_SEC_EXT,
>  	NONSEC_ERR_NO_GIC_ADDRESS,
>  	NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB,
> +	VIRT_ALREADY_HYP_MODE,
> +	VIRT_ERR_NO_VIRT_EXT,
> +	VIRT_ERR_NOT_HYP_MODE
>  };
>  
>  enum nonsec_virt_errors armv7_switch_nonsec(void);
> +enum nonsec_virt_errors armv7_switch_hyp(void);
>  
>  /* defined in assembly file */
>  unsigned int _nonsec_init(void);
>  void _smp_pen(void);
> -#endif /* CONFIG_ARMV7_NONSEC */
> +void _switch_to_hyp(void);
> +#endif /* CONFIG_ARMV7_NONSEC || CONFIG_ARMV7_VIRT */
>  
>  #endif /* ! __ASSEMBLY__ */
>  
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 7b0619e..90875b3 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -34,7 +34,7 @@
>  #include <asm/bootm.h>
>  #include <linux/compiler.h>
>  
> -#ifdef CONFIG_ARMV7_NONSEC
> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>  #include <asm/armv7.h>
>  #endif
>  
> @@ -192,13 +192,17 @@ __weak void setup_board_tags(struct tag **in_params) {}
>  
>  static void do_nonsec_virt_switch(void)
>  {
> -#ifdef CONFIG_ARMV7_NONSEC
> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>  	int ret;
>  
>  	ret = armv7_switch_nonsec();
> +#ifdef CONFIG_ARMV7_VIRT
> +	if (ret == NONSEC_VIRT_SUCCESS)
> +		ret = armv7_switch_hyp();
> +#endif
>  	switch (ret) {
>  	case NONSEC_VIRT_SUCCESS:
> -		debug("entered non-secure state\n");
> +		debug("entered non-secure state or HYP mode\n");
>  		break;
>  	case NONSEC_ERR_NO_SEC_EXT:
>  		printf("nonsec: Security extensions not implemented.\n");
> @@ -209,6 +213,15 @@ static void do_nonsec_virt_switch(void)
>  	case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
>  		printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
>  		break;
> +	case VIRT_ERR_NO_VIRT_EXT:
> +		printf("HYP mode: Virtualization extensions not implemented.\n");
> +		break;
> +	case VIRT_ALREADY_HYP_MODE:
> +		debug("CPU already in HYP mode\n");
> +		break;
> +	case VIRT_ERR_NOT_HYP_MODE:
> +		printf("HYP mode: switch not successful.\n");
> +		break;
>  	}
>  #endif
>  }
> -- 
> 1.7.12.1
>
Andre Przywara July 30, 2013, 11:59 a.m. UTC | #2 
On 07/30/2013 12:02 AM, Christoffer Dall wrote:
> On Wed, Jul 10, 2013 at 01:54:18AM +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/Makefile      |  2 +-
>>   arch/arm/cpu/armv7/nonsec_virt.S | 43 +++++++++++++++++++++++++++++++++++-----
>>   arch/arm/cpu/armv7/virt-v7.c     | 31 +++++++++++++++++++++++++++++
>>   arch/arm/include/asm/armv7.h     |  9 +++++++--
>>   arch/arm/lib/bootm.c             | 19 +++++++++++++++---
>>   5 files changed, 93 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
>> index b59f59e..e5eaa56 100644
>> --- a/arch/arm/cpu/armv7/Makefile
>> +++ b/arch/arm/cpu/armv7/Makefile
>> @@ -36,7 +36,7 @@ ifneq ($(CONFIG_AM33XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CONFIG_TEGRA)$(CONF
>>   SOBJS	+= lowlevel_init.o
>>   endif
>>
>> -ifneq ($(CONFIG_ARMV7_NONSEC),)
>> +ifneq ($(CONFIG_ARMV7_NONSEC)$(CONFIG_ARMV7_VIRT),)
>>   SOBJS   += nonsec_virt.o
>>   COBJS	+= virt-v7.o
>>   endif
>> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
>> index f9b6b39..895c3b0 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>
>>    *
>> @@ -28,15 +28,16 @@
>>   #include <asm/armv7.h>
>>
>>   .arch_extension sec
>> +.arch_extension virt
>>
>> -/* the vector table for secure state */
>> +/* the vector table for secure state and HYP mode */
>>   _monitor_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 */
>> @@ -53,10 +54,27 @@ _secure_monitor:
>>   	bic	r1, r1, #0x4e			@ clear IRQ, FIQ, EA, nET bits
>>   	orr	r1, r1, #0x31			@ enable NS, AW, FW bits
>>
>> +#ifdef CONFIG_ARMV7_VIRT
>> +	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
>> +#endif
>> +
>>   	mcr	p15, 0, r1, c1, c1, 0		@ write SCR (with NS bit set)
>>
>> +#ifdef CONFIG_ARMV7_VIRT
>> +	mrceq	p15, 0, r0, c12, c0, 1		@ get MVBAR value
>> +	mcreq	p15, 4, r0, c12, c0, 0		@ write HVBAR
>> +#endif
>> +
>>   	movs	pc, lr				@ return to non-secure SVC
>>
>> +_hyp_trap:
>> +	mrs	lr, elr_hyp	@ for older asm: .byte 0x00, 0xe3, 0x0e, 0xe1
>
> this comment just confuses: either make it intelligent to support an
> older compiler or just get rid of these byte encodings.  You can always
> disassemble the file and lookup the byte code with a modern compiler to
> get back to the byte encoding.

Well, I used a Debian 6 cross compiler before, which didn't support 
these instructions. After your remark I updated the system to Debian 7, 
but found it not appropriate to ask any user to do the same just to use 
a fixed, non-parametrized assembly instruction. I have the feeling that 
there are quite some users out there who cannot and don't want to easily 
update their compiler.
So I decided to leave the workaround in the comment to give a hint to a 
quick fix.

By "making it intelligent" you mean a macro which does some version 
checking and inserts the .byte sequence if needed? Are there any 
archetypes of such code?

Regards,
Andre.

>> +	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
>> @@ -71,9 +89,13 @@ ENTRY(_smp_pen)
>>   	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
>>
>>   	bl	_nonsec_init
>> +	mov	r12, r0				@ save GICC address
>> +#ifdef CONFIG_ARMV7_VIRT
>> +	bl	_switch_to_hyp
>> +#endif
>>
>> -	ldr	r1, [r0, #GICC_IAR]		@ acknowledge IPI
>> -	str	r1, [r0, #GICC_EOIR]		@ signal end of interrupt
>> +	ldr	r1, [r12, #GICC_IAR]		@ acknowledge IPI
>> +	str	r1, [r12, #GICC_EOIR]		@ signal end of interrupt
>>   	adr	r1, _smp_pen
>>   waitloop:
>>   	wfi
>> @@ -164,3 +186,14 @@ ENTRY(_nonsec_init)
>>
>>   	bx	lr
>>   ENDPROC(_nonsec_init)
>> +
>> +ENTRY(_switch_to_hyp)
>> +	mov	r0, lr
>> +	mov	r1, sp				@ save SVC copy of LR and SP
>> +	isb
>
> did you find out that this isb is indeed needed? if so, why?
>
>> +	hvc #0			 @ for older asm: .byte 0x70, 0x00, 0x40, 0xe1
>
> same comment as above?
>
>> +	mov	sp, r1
>> +	mov	lr, r0				@ restore SVC copy of LR and SP
>> +
>> +	bx	lr
>> +ENDPROC(_switch_to_hyp)
>> diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
>> index a0d0b34..3645572 100644
>> --- a/arch/arm/cpu/armv7/virt-v7.c
>> +++ b/arch/arm/cpu/armv7/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;
>> @@ -92,6 +101,28 @@ static void kick_secondary_cpus(unsigned int gicdaddr)
>>   	writel(1U << 24, gicdaddr + GICD_SGIR);
>>   }
>>
>> +enum nonsec_virt_errors armv7_switch_hyp(void)
>> +{
>> +	unsigned int reg;
>> +
>> +	/* check whether we are in HYP mode already */
>> +	if ((read_cpsr() & 0x1f) == 0x1a)
>> +		return VIRT_ALREADY_HYP_MODE;
>> +
>> +	/* check whether the CPU supports the virtualization extensions */
>> +	reg = read_id_pfr1();
>> +	if ((reg & CPUID_ARM_VIRT_MASK) != 1 << CPUID_ARM_VIRT_SHIFT)
>> +		return VIRT_ERR_NO_VIRT_EXT;
>> +
>> +	/* call the HYP switching code on this CPU also */
>> +	_switch_to_hyp();
>> +
>> +	if ((read_cpsr() & 0x1F) != 0x1a)
>> +		return VIRT_ERR_NOT_HYP_MODE;
>> +
>> +	return NONSEC_VIRT_SUCCESS;
>> +}
>> +
>>   enum nonsec_virt_errors armv7_switch_nonsec(void)
>>   {
>>   	unsigned int reg, ret;
>> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
>> index f6582a1..baa22fe 100644
>> --- a/arch/arm/include/asm/armv7.h
>> +++ b/arch/arm/include/asm/armv7.h
>> @@ -89,21 +89,26 @@ 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_NONSEC
>> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>>
>>   enum nonsec_virt_errors {
>>   	NONSEC_VIRT_SUCCESS,
>>   	NONSEC_ERR_NO_SEC_EXT,
>>   	NONSEC_ERR_NO_GIC_ADDRESS,
>>   	NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB,
>> +	VIRT_ALREADY_HYP_MODE,
>> +	VIRT_ERR_NO_VIRT_EXT,
>> +	VIRT_ERR_NOT_HYP_MODE
>>   };
>>
>>   enum nonsec_virt_errors armv7_switch_nonsec(void);
>> +enum nonsec_virt_errors armv7_switch_hyp(void);
>>
>>   /* defined in assembly file */
>>   unsigned int _nonsec_init(void);
>>   void _smp_pen(void);
>> -#endif /* CONFIG_ARMV7_NONSEC */
>> +void _switch_to_hyp(void);
>> +#endif /* CONFIG_ARMV7_NONSEC || CONFIG_ARMV7_VIRT */
>>
>>   #endif /* ! __ASSEMBLY__ */
>>
>> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
>> index 7b0619e..90875b3 100644
>> --- a/arch/arm/lib/bootm.c
>> +++ b/arch/arm/lib/bootm.c
>> @@ -34,7 +34,7 @@
>>   #include <asm/bootm.h>
>>   #include <linux/compiler.h>
>>
>> -#ifdef CONFIG_ARMV7_NONSEC
>> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>>   #include <asm/armv7.h>
>>   #endif
>>
>> @@ -192,13 +192,17 @@ __weak void setup_board_tags(struct tag **in_params) {}
>>
>>   static void do_nonsec_virt_switch(void)
>>   {
>> -#ifdef CONFIG_ARMV7_NONSEC
>> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>>   	int ret;
>>
>>   	ret = armv7_switch_nonsec();
>> +#ifdef CONFIG_ARMV7_VIRT
>> +	if (ret == NONSEC_VIRT_SUCCESS)
>> +		ret = armv7_switch_hyp();
>> +#endif
>>   	switch (ret) {
>>   	case NONSEC_VIRT_SUCCESS:
>> -		debug("entered non-secure state\n");
>> +		debug("entered non-secure state or HYP mode\n");
>>   		break;
>>   	case NONSEC_ERR_NO_SEC_EXT:
>>   		printf("nonsec: Security extensions not implemented.\n");
>> @@ -209,6 +213,15 @@ static void do_nonsec_virt_switch(void)
>>   	case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
>>   		printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
>>   		break;
>> +	case VIRT_ERR_NO_VIRT_EXT:
>> +		printf("HYP mode: Virtualization extensions not implemented.\n");
>> +		break;
>> +	case VIRT_ALREADY_HYP_MODE:
>> +		debug("CPU already in HYP mode\n");
>> +		break;
>> +	case VIRT_ERR_NOT_HYP_MODE:
>> +		printf("HYP mode: switch not successful.\n");
>> +		break;
>>   	}
>>   #endif
>>   }
>> --
>> 1.7.12.1
>>
Christoffer Dall July 30, 2013, 2:31 p.m. UTC | #3 
On Tue, Jul 30, 2013 at 01:59:29PM +0200, Andre Przywara wrote:
> On 07/30/2013 12:02 AM, Christoffer Dall wrote:
> >On Wed, Jul 10, 2013 at 01:54:18AM +0200, Andre Przywara wrote:

[...]

> >>
> >>+_hyp_trap:
> >>+	mrs	lr, elr_hyp	@ for older asm: .byte 0x00, 0xe3, 0x0e, 0xe1
> >
> >this comment just confuses: either make it intelligent to support an
> >older compiler or just get rid of these byte encodings.  You can always
> >disassemble the file and lookup the byte code with a modern compiler to
> >get back to the byte encoding.
> 
> Well, I used a Debian 6 cross compiler before, which didn't support
> these instructions. After your remark I updated the system to Debian
> 7, but found it not appropriate to ask any user to do the same just
> to use a fixed, non-parametrized assembly instruction. I have the
> feeling that there are quite some users out there who cannot and
> don't want to easily update their compiler.
> So I decided to leave the workaround in the comment to give a hint
> to a quick fix.

ok, so if Debian's built-in cross compilers are indeed that old and we
want to support those (that's ok with me), then let's fix it properly.

> 
> By "making it intelligent" you mean a macro which does some version
> checking and inserts the .byte sequence if needed? Are there any
> archetypes of such code?
> 
Yes, see arch/arm/include/asm/opcodes-*.h

-Christoffer
Masahiro Yamada Aug. 5, 2013, 5:15 a.m. UTC | #4 
Hello Andre,



> -#ifdef CONFIG_ARMV7_NONSEC
> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>  
>  enum nonsec_virt_errors {
>  	NONSEC_VIRT_SUCCESS,
>  	NONSEC_ERR_NO_SEC_EXT,
>  	NONSEC_ERR_NO_GIC_ADDRESS,
>  	NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB,
> +	VIRT_ALREADY_HYP_MODE,
> +	VIRT_ERR_NO_VIRT_EXT,
> +	VIRT_ERR_NOT_HYP_MODE
>  };

This looks weird to me.
If you want to use enum, why don't you separate it like follows? 

enum nonsec_errors {
	NONSEC_SUCCESS,
	NONSEC_ERR_NO_SEC_EXT,
	NONSEC_ERR_NO_GIC_ADDRESS,
	NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB,
};

enum virt_errors {
	VIRT_SUCCESS,
	VIRT_ALREADY_HYP_MODE,
	VIRT_ERR_NO_VIRT_EXT,
	VIRT_ERR_NOT_HYP_MODE
};


>  enum nonsec_virt_errors armv7_switch_nonsec(void);
> +enum nonsec_virt_errors armv7_switch_hyp(void);

enum nonsec_errors armv7_switch_nonsec(void);
+enum virt_errors armv7_switch_hyp(void);


But in the first place,
I like better to fix do_nonsec_virt_switch().



>  static void do_nonsec_virt_switch(void)
>  {
> -#ifdef CONFIG_ARMV7_NONSEC
> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>  	int ret;
>  
>  	ret = armv7_switch_nonsec();
> +#ifdef CONFIG_ARMV7_VIRT
> +	if (ret == NONSEC_VIRT_SUCCESS)
> +		ret = armv7_switch_hyp();
> +#endif
>  	switch (ret) {
>  	case NONSEC_VIRT_SUCCESS:
> -		debug("entered non-secure state\n");
> +		debug("entered non-secure state or HYP mode\n");
>  		break;
>  	case NONSEC_ERR_NO_SEC_EXT:
>  		printf("nonsec: Security extensions not implemented.\n");
> @@ -209,6 +213,15 @@ static void do_nonsec_virt_switch(void)
>  	case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
>  		printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
>  		break;
> +	case VIRT_ERR_NO_VIRT_EXT:
> +		printf("HYP mode: Virtualization extensions not implemented.\n");
> +		break;
> +	case VIRT_ALREADY_HYP_MODE:
> +		debug("CPU already in HYP mode\n");
> +		break;
> +	case VIRT_ERR_NOT_HYP_MODE:
> +		printf("HYP mode: switch not successful.\n");
> +		break;
>  	}
>  #endif


As for this part, I agreed on Christoffer's opinion:


On Tue, 30 Jul 2013 07:23:58 -0700
Christoffer Dall <christoffer.dall@linaro.org> wrote:
> I won't hold back my ack for the patch series based on this, but I do
> think it's over-engineered.  I think at least just returning -1 for
> error and 0 for success (or even make it a bool) and just printing a
> generic error message is cleaner - the level of details as to why the
> switch to hyp/nonsec didn't work could then be debug statements that a
> board developer could enable with a "#define DEBUG 1" in the
> corresponding file.




Best Regards
Masahiro Yamada
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
index b59f59e..e5eaa56 100644
--- a/arch/arm/cpu/armv7/Makefile
+++ b/arch/arm/cpu/armv7/Makefile
@@ -36,7 +36,7 @@  ifneq ($(CONFIG_AM33XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CONFIG_TEGRA)$(CONF
 SOBJS	+= lowlevel_init.o
 endif
 
-ifneq ($(CONFIG_ARMV7_NONSEC),)
+ifneq ($(CONFIG_ARMV7_NONSEC)$(CONFIG_ARMV7_VIRT),)
 SOBJS   += nonsec_virt.o
 COBJS	+= virt-v7.o
 endif
diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
index f9b6b39..895c3b0 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>
  *
@@ -28,15 +28,16 @@ 
 #include <asm/armv7.h>
 
 .arch_extension sec
+.arch_extension virt
 
-/* the vector table for secure state */
+/* the vector table for secure state and HYP mode */
 _monitor_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 */
@@ -53,10 +54,27 @@  _secure_monitor:
 	bic	r1, r1, #0x4e			@ clear IRQ, FIQ, EA, nET bits
 	orr	r1, r1, #0x31			@ enable NS, AW, FW bits
 
+#ifdef CONFIG_ARMV7_VIRT
+	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
+#endif
+
 	mcr	p15, 0, r1, c1, c1, 0		@ write SCR (with NS bit set)
 
+#ifdef CONFIG_ARMV7_VIRT
+	mrceq	p15, 0, r0, c12, c0, 1		@ get MVBAR value
+	mcreq	p15, 4, r0, c12, c0, 0		@ write HVBAR
+#endif
+
 	movs	pc, lr				@ return to non-secure SVC
 
+_hyp_trap:
+	mrs	lr, elr_hyp	@ for older asm: .byte 0x00, 0xe3, 0x0e, 0xe1
+	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
@@ -71,9 +89,13 @@  ENTRY(_smp_pen)
 	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
 
 	bl	_nonsec_init
+	mov	r12, r0				@ save GICC address
+#ifdef CONFIG_ARMV7_VIRT
+	bl	_switch_to_hyp
+#endif
 
-	ldr	r1, [r0, #GICC_IAR]		@ acknowledge IPI
-	str	r1, [r0, #GICC_EOIR]		@ signal end of interrupt
+	ldr	r1, [r12, #GICC_IAR]		@ acknowledge IPI
+	str	r1, [r12, #GICC_EOIR]		@ signal end of interrupt
 	adr	r1, _smp_pen
 waitloop:
 	wfi
@@ -164,3 +186,14 @@  ENTRY(_nonsec_init)
 
 	bx	lr
 ENDPROC(_nonsec_init)
+
+ENTRY(_switch_to_hyp)
+	mov	r0, lr
+	mov	r1, sp				@ save SVC copy of LR and SP
+	isb
+	hvc #0			 @ for older asm: .byte 0x70, 0x00, 0x40, 0xe1
+	mov	sp, r1
+	mov	lr, r0				@ restore SVC copy of LR and SP
+
+	bx	lr
+ENDPROC(_switch_to_hyp)
diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
index a0d0b34..3645572 100644
--- a/arch/arm/cpu/armv7/virt-v7.c
+++ b/arch/arm/cpu/armv7/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;
@@ -92,6 +101,28 @@  static void kick_secondary_cpus(unsigned int gicdaddr)
 	writel(1U << 24, gicdaddr + GICD_SGIR);
 }
 
+enum nonsec_virt_errors armv7_switch_hyp(void)
+{
+	unsigned int reg;
+
+	/* check whether we are in HYP mode already */
+	if ((read_cpsr() & 0x1f) == 0x1a)
+		return VIRT_ALREADY_HYP_MODE;
+
+	/* check whether the CPU supports the virtualization extensions */
+	reg = read_id_pfr1();
+	if ((reg & CPUID_ARM_VIRT_MASK) != 1 << CPUID_ARM_VIRT_SHIFT)
+		return VIRT_ERR_NO_VIRT_EXT;
+
+	/* call the HYP switching code on this CPU also */
+	_switch_to_hyp();
+
+	if ((read_cpsr() & 0x1F) != 0x1a)
+		return VIRT_ERR_NOT_HYP_MODE;
+
+	return NONSEC_VIRT_SUCCESS;
+}
+
 enum nonsec_virt_errors armv7_switch_nonsec(void)
 {
 	unsigned int reg, ret;
diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index f6582a1..baa22fe 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -89,21 +89,26 @@  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_NONSEC
+#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
 
 enum nonsec_virt_errors {
 	NONSEC_VIRT_SUCCESS,
 	NONSEC_ERR_NO_SEC_EXT,
 	NONSEC_ERR_NO_GIC_ADDRESS,
 	NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB,
+	VIRT_ALREADY_HYP_MODE,
+	VIRT_ERR_NO_VIRT_EXT,
+	VIRT_ERR_NOT_HYP_MODE
 };
 
 enum nonsec_virt_errors armv7_switch_nonsec(void);
+enum nonsec_virt_errors armv7_switch_hyp(void);
 
 /* defined in assembly file */
 unsigned int _nonsec_init(void);
 void _smp_pen(void);
-#endif /* CONFIG_ARMV7_NONSEC */
+void _switch_to_hyp(void);
+#endif /* CONFIG_ARMV7_NONSEC || CONFIG_ARMV7_VIRT */
 
 #endif /* ! __ASSEMBLY__ */
 
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 7b0619e..90875b3 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -34,7 +34,7 @@ 
 #include <asm/bootm.h>
 #include <linux/compiler.h>
 
-#ifdef CONFIG_ARMV7_NONSEC
+#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
 #include <asm/armv7.h>
 #endif
 
@@ -192,13 +192,17 @@  __weak void setup_board_tags(struct tag **in_params) {}
 
 static void do_nonsec_virt_switch(void)
 {
-#ifdef CONFIG_ARMV7_NONSEC
+#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
 	int ret;
 
 	ret = armv7_switch_nonsec();
+#ifdef CONFIG_ARMV7_VIRT
+	if (ret == NONSEC_VIRT_SUCCESS)
+		ret = armv7_switch_hyp();
+#endif
 	switch (ret) {
 	case NONSEC_VIRT_SUCCESS:
-		debug("entered non-secure state\n");
+		debug("entered non-secure state or HYP mode\n");
 		break;
 	case NONSEC_ERR_NO_SEC_EXT:
 		printf("nonsec: Security extensions not implemented.\n");
@@ -209,6 +213,15 @@  static void do_nonsec_virt_switch(void)
 	case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
 		printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
 		break;
+	case VIRT_ERR_NO_VIRT_EXT:
+		printf("HYP mode: Virtualization extensions not implemented.\n");
+		break;
+	case VIRT_ALREADY_HYP_MODE:
+		debug("CPU already in HYP mode\n");
+		break;
+	case VIRT_ERR_NOT_HYP_MODE:
+		printf("HYP mode: switch not successful.\n");
+		break;
 	}
 #endif
 }