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

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

Comments

Andre Przywara - May 6, 2013, 1:17 p.m.
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@linaro.org>
---
 arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)
Albert ARIBAUD - May 23, 2013, 10:52 a.m.
Hi Andre,

On Mon,  6 May 2013 15:17:45 +0200, Andre Przywara
<andre.przywara@linaro.org> 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

ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is
a more precise reference, please provide it.

> 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@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
> +#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
> +	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, 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
> +
> +	movs	pc, lr
>  
>  	.align	5
>  prefetch_abort:


Amicalement,
Marc Zyngier - May 23, 2013, 12:14 p.m.
On 23/05/13 11:52, Albert ARIBAUD wrote:
> Hi Andre,
> 
> On Mon,  6 May 2013 15:17:45 +0200, Andre Przywara
> <andre.przywara@linaro.org> 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
> 
> ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is
> a more precise reference, please provide it.

I believe the ARM ARM (as in ARM Architecture Reference Manual) is the
correct document here.

http://infocenter.arm.com/help/topic/com.arm.doc.ddi0406c/index.html

Cheers,

	M.
Albert ARIBAUD - May 23, 2013, 12:34 p.m.
Hi Marc,

On Thu, 23 May 2013 13:14:01 +0100, Marc Zyngier <marc.zyngier@arm.com>
wrote:

> On 23/05/13 11:52, Albert ARIBAUD wrote:
> > Hi Andre,
> > 
> > On Mon,  6 May 2013 15:17:45 +0200, Andre Przywara
> > <andre.przywara@linaro.org> 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
> > 
> > ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is
> > a more precise reference, please provide it.
> 
> I believe the ARM ARM (as in ARM Architecture Reference Manual) is the
> correct document here.
> 
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0406c/index.html

Well -- if you form the acronym, you do end with 'A R M' indeed, but
this is quite unfortunate, as 'ARM ARM' is redundant (as the acronym's
A already has ARM in it), confusion (as 'ARM' already bears a quite
established meaning) and ambiguous (as there are actually several
documents with the title of 'ARM<vXXX> Reference Manual' and which
would end up with the same acronym).

So if you don't want to use 'TRM' (which I can understand), then
at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'.
Stating the DDI* reference is not a must, unless you want to specify
a given revision (but then I suggest adding it after 'Manual' too).

> Cheers,
> 
> 	M.

Amicalement,
Albert ARIBAUD - May 23, 2013, 12:40 p.m.
On Thu, 23 May 2013 14:34:38 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Hi Marc,

> So if you don't want to use 'TRM' (which I can understand), then
> at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'.
> Stating the DDI* reference is not a must, unless you want to specify
> a given revision (but then I suggest adding it after 'Manual' too).

My bad: this last paragraph was actually directed at Andrew, not Marc.

Amicalement,
Albert ARIBAUD - May 23, 2013, 12:41 p.m.
On Thu, 23 May 2013 14:40:09 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> My bad: this last paragraph was actually directed at Andrew

Make that Andre.

Apologies,
Peter Maydell - May 23, 2013, 1 p.m.
On 23 May 2013 13:34, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> Well -- if you form the acronym, you do end with 'A R M' indeed, but
> this is quite unfortunate, as 'ARM ARM' is redundant (as the acronym's
> A already has ARM in it), confusion (as 'ARM' already bears a quite
> established meaning) and ambiguous (as there are actually several
> documents with the title of 'ARM<vXXX> Reference Manual' and which
> would end up with the same acronym).

"ARM ARM" is the standard abbreviated way of referring to the
ARM Architecture Reference Manual (and as you can see it's
not a redundant acronym, since the A doesn't stand for ARM).
A TRM (Technical Reference Manual) is a completely different
document type, describing a specific processor. Andre is correct
that the restriction in question is architectural (and thus
described in the ARM ARM), not implementation specific (which
would be what you'd find in a TRM).

> So if you don't want to use 'TRM' (which I can understand), then
> at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'.
> Stating the DDI* reference is not a must, unless you want to specify
> a given revision (but then I suggest adding it after 'Manual' too).

"ARMv7-AR Reference Manual" is confusing, because you've dropped
the "Architecture" bit.

Since this is only a git comment, I'd suggest "ARM architecture
reference manual" as both clear for non-ARM people and sufficiently
unambiguous, or "ARMv7-AR Architecture Reference Manual" if you
want to be a bit more formal about it.

thanks
-- PMM
Albert ARIBAUD - May 23, 2013, 2:08 p.m.
Hi Peter,

(sorry for the duplicate; first reply sent was missing recipients, and
I had a fix to do anyway)

On Thu, 23 May 2013 14:00:17 +0100, Peter Maydell
<peter.maydell@linaro.org> wrote:

> On 23 May 2013 13:34, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> > Well -- if you form the acronym, you do end with 'A R M' indeed, but
> > this is quite unfortunate, as 'ARM ARM' is redundant (as the acronym's
> > A already has ARM in it), confusion (as 'ARM' already bears a quite
> > established meaning) and ambiguous (as there are actually several
> > documents with the title of 'ARM<vXXX> Reference Manual' and which
> > would end up with the same acronym).
> 
> "ARM ARM" is the standard abbreviated way of referring to the
> ARM Architecture Reference Manual (and as you can see it's
> not a redundant acronym, since the A doesn't stand for ARM).

Before answering about the double 'ARM', I did google for possible
meanings of the acronym 'ARM' and did not find any indication of it
meaning 'Architecture Reference Manual' except on www.all-acronyms.com,
which is completely foreign to the embedded world and where the
definition is not backed by any substantial source.

OTOH, the ARM Information Center does not use the acronym ARM to mean
'Architecture Reference Manual', nor does the Glossary section of ARM
documents I've read so far - including "the" ARM Glossary (AEG0014E),
which lists quite a lot of 'ARM something' but no 'ARM' alone, except
in the preamble phrase: 'Where the term ARM is used it means “ARM
or any of its subsidiaries as appropriate”'.

Of course, I might have missed it, so any actual pointer to the
definition is heartily welcome.

> A TRM (Technical Reference Manual) is a completely different
> document type, describing a specific processor. Andre is correct
> that the restriction in question is architectural (and thus
> described in the ARM ARM), not implementation specific (which
> would be what you'd find in a TRM).

You are correct that here the document is not a TRM.

> > So if you don't want to use 'TRM' (which I can understand), then
> > at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'.
> > Stating the DDI* reference is not a must, unless you want to specify
> > a given revision (but then I suggest adding it after 'Manual' too).
> 
> "ARMv7-AR Reference Manual" is confusing, because you've dropped
> the "Architecture" bit.

That drop was involuntary.

> Since this is only a git comment, I'd suggest "ARM architecture
> reference manual" as both clear for non-ARM people and sufficiently
> unambiguous, or "ARMv7-AR Architecture Reference Manual" if you
> want to be a bit more formal about it.

Since a git comment is there for a reason, which includes helping its
readers understand the commit, I consider "ARMv7-AR Reference Manual"
to help them much more than 'ARM ARM', as it points them unambiguously
to the document by stating the exact title under which it is listed in
the ARM information center, but I am ok with 'ARMv7-AR Architecture
Reference Manual' as this how its title goes.

> thanks
> -- PMM

Amicalement,
Albert ARIBAUD - May 23, 2013, 2:47 p.m.
On Thu, 23 May 2013 16:08:35 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Since a git comment is there for a reason, which includes helping its
> readers understand the commit, I consider "ARMv7-AR Reference Manual"
> to help them much more than 'ARM ARM', as it points them unambiguously
> to the document by stating the exact title under which it is listed in
> the ARM information center, but I am ok with 'ARMv7-AR Architecture
> Reference Manual' as this how its title goes.

Just to clarify: yes, the document is listed (on the left, in the docs
tree as "ARMv7-AR Reference Manual", and yes, its title is 'ARMv7-AR
Architecture Reference Manual'. That is not a mistake apparently, or it
was a duly repeated one for all ARM architecture reference manuals on
the Information Center.

Amicalement,
Andre Przywara - May 26, 2013, 10:42 p.m.
On 05/23/2013 12:52 PM, Albert ARIBAUD wrote:
> Hi Andre,
>
> On Mon,  6 May 2013 15:17:45 +0200, Andre Przywara
> <andre.przywara@linaro.org> 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
>
> ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is
> a more precise reference, please provide it.

Albert,

my apologies for the confusion. As Peter already pointed out, the 
reference is really in the architectural manual. I just picked up that 
"ARM ARM" phrase lately and assumed that this is common knowledge. I 
will change it to something more precise in the next revision.

Thanks,
Andre.
Christoffer Dall - May 31, 2013, 1:02 a.m.
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@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.

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

> +	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?

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

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

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

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.

>  
>  	.align	5
>  prefetch_abort:
> -- 
> 1.7.12.1
>
Andre Przywara - May 31, 2013, 9:23 a.m.
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@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
>>
Albert ARIBAUD - May 31, 2013, 5:21 p.m.
Hi Andre,

On Fri, 31 May 2013 11:23:16 +0200, Andre Przywara
<andre.przywara@linaro.org> wrote:

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

You should, actually. Rule #1 goes (*) 'a new config option should
have zero effect on the code when not defined'.

> Regards,
> Andre.

(*) Rule #1 actually goes, 'there is an infinite number of "Rule #1"',
but that's slightly beyond the point.

Amicalement,
Christoffer Dall - May 31, 2013, 11:50 p.m.
On Fri, May 31, 2013 at 11:23:16AM +0200, Andre Przywara wrote:
> 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@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().

Which is pretty useful to catch if something went wrong.

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

Yeah, I don't really think it's cool if a software interrupt happens
somewhere completely else in the system that we do this dance, which
could be super hard to detect, which is why I'm not even a fan of
re-using the vector in the first place.

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

Yeah, makes sense.

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

The splitting of patches is fine, but it would be helpful to explain the
scope a little more in the commit text perhaps, maybe I'm just being
silly.

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

I think the ARM ARM is pretty clear on the fact that the NS bit in the
SCR determine the secure state, *except* from monitor mode, which is
always in secure state:

  "Software executing in Monitor mode executes in the Secure state
  regardless of the value of the SCR.NS bit."
                               ARM ARM, DDI 0406C.b, B1-1157

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

Hmmm, that seems to contradict the quote from above.  Did you verify
this emprically?  If so, maybe there's a special caveat for writing
banked co-processor registers (I doubt it though).

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

thinking about it, it really shouldn't be necessary, because the movs
implies all that's necessary.

> >
> >>+
> >>+	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
> >>
>
Albert ARIBAUD - June 1, 2013, 10:06 a.m.
Hi Christoffer,

On Fri, 31 May 2013 16:50:09 -0700, Christoffer Dall
<cdall@cs.columbia.edu> wrote:

> On Fri, May 31, 2013 at 11:23:16AM +0200, Andre Przywara wrote:
> > On 05/31/2013 03:02 AM, Christoffer Dall wrote:

> > 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.
> 
> The splitting of patches is fine, but it would be helpful to explain the
> scope a little more in the commit text perhaps, maybe I'm just being
> silly.

That's the point where a cover letter will be very useful in V2.

Also, speaking of V2 (and, presumably, later versions), that's where
patman will be useful (see tools/patman/README) as it allows one's
local branch to contain not only things like cover letter, but also
patch history, which will be a must IMO for this series.

Amicalement,
Albert ARIBAUD - June 1, 2013, 10:11 a.m.
On Sat, 1 Jun 2013 12:06:24 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Hi Christoffer,
> 
> On Fri, 31 May 2013 16:50:09 -0700, Christoffer Dall
> <cdall@cs.columbia.edu> wrote:
> 
> > On Fri, May 31, 2013 at 11:23:16AM +0200, Andre Przywara wrote:
> > > On 05/31/2013 03:02 AM, Christoffer Dall wrote:
> 
> > > 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.
> > 
> > The splitting of patches is fine, but it would be helpful to explain the
> > scope a little more in the commit text perhaps, maybe I'm just being
> > silly.
> 
> That's the point where a cover letter will be very useful in V2.

Correction: V1 has a cover letter; what I meant and should have said
is, that's where an overall description in the cover letter of how
commits are organized will be very useful in V2.

> Also, speaking of V2 (and, presumably, later versions), that's where
> patman will be useful (see tools/patman/README) as it allows one's
> local branch to contain not only things like cover letter, but also
> patch history, which will be a must IMO for this series.

Correction: maybe/probably Andre used patman already; in this case,
scratch the paragraph above.

Apologies for any misunderstanding,

Patch

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
+#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
+	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, 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
+
+	movs	pc, lr
 
 	.align	5
 prefetch_abort: