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

[Andre Przywara]

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

Christoffer,

thanks a lot for the thorough review. Comments inline.

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

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

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

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

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

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

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

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

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

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

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

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

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

OK.

Regards,
Andre.

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