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

login
register
mail settings
Submitter Andre Przywara
Date May 6, 2013, 1:17 p.m.
Message ID <1367846270-1827-6-git-send-email-andre.przywara@linaro.org>
Download mbox | patch
Permalink /patch/241652/
State Changes Requested
Delegated to: Albert ARIBAUD
Headers show

Comments

Andre Przywara - May 6, 2013, 1:17 p.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 the exception vector table.

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

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

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/cpu/armv7/start.S   | 34 +++++++++++++++++++++++-----------
 arch/arm/include/asm/armv7.h |  4 ++--
 arch/arm/lib/bootm.c         | 12 +++++++++---
 arch/arm/lib/virt-v7.c       | 22 +++++++++++++++-------
 4 files changed, 49 insertions(+), 23 deletions(-)
Tom Rini - May 9, 2013, 6:56 p.m.
On Mon, May 06, 2013 at 03:17:49PM +0200, Andre Przywara wrote:

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

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

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

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

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

nit: s/HYP mode//

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

...

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

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


Regards,
Andre.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Thanks,
-Christoffer

Patch

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