diff mbox

[U-Boot,3/9] ARM: HYP/non-sec: add a barrier after setting SCR.NS==1

Message ID 1385024402-23585-4-git-send-email-marc.zyngier@arm.com
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Commit Message

Marc Zyngier Nov. 21, 2013, 8:59 a.m. UTC
A CP15 instruction execution can be reordered, requiring an
isb to be sure it is executed in program order.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/cpu/armv7/nonsec_virt.S | 1 +
 1 file changed, 1 insertion(+)

Comments

Christoffer Dall Nov. 22, 2013, 1:51 a.m. UTC | #1
On 21 November 2013 00:59, Marc Zyngier <marc.zyngier@arm.com> wrote:
> A CP15 instruction execution can be reordered, requiring an
> isb to be sure it is executed in program order.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/cpu/armv7/nonsec_virt.S | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
> index 29987cd..648066f 100644
> --- a/arch/arm/cpu/armv7/nonsec_virt.S
> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
> @@ -47,6 +47,7 @@ _secure_monitor:
>  #endif
>
>         mcr     p15, 0, r1, c1, c1, 0           @ write SCR (with NS bit set)
> +       isb
>
>  #ifdef CONFIG_ARMV7_VIRT
>         mrceq   p15, 0, r0, c12, c0, 1          @ get MVBAR value
> --
> 1.8.2.3
>
Does this matter?  Are we not still in monitor mode and therefore
secure and the exception return below will surely be ordered by the
cpu after the mcr, right? or no?

-Christoffer
Marc Zyngier Nov. 22, 2013, 10:56 a.m. UTC | #2
On 22/11/13 01:51, Christoffer Dall wrote:
> On 21 November 2013 00:59, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> A CP15 instruction execution can be reordered, requiring an
>> isb to be sure it is executed in program order.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm/cpu/armv7/nonsec_virt.S | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
>> index 29987cd..648066f 100644
>> --- a/arch/arm/cpu/armv7/nonsec_virt.S
>> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
>> @@ -47,6 +47,7 @@ _secure_monitor:
>>  #endif
>>
>>         mcr     p15, 0, r1, c1, c1, 0           @ write SCR (with NS bit set)
>> +       isb
>>
>>  #ifdef CONFIG_ARMV7_VIRT
>>         mrceq   p15, 0, r0, c12, c0, 1          @ get MVBAR value
>> --
>> 1.8.2.3
>>
> Does this matter?  Are we not still in monitor mode and therefore
> secure and the exception return below will surely be ordered by the
> cpu after the mcr, right? or no?

You need to look at what is between the SCR access and the exception
return (and you cannot see that from the patch): There's a write to
HVBAR that can only be executed if SCR.NS==1.

If the write to SCR is delayed, the whole thing will stop very quickly...

	M.
Christoffer Dall Nov. 22, 2013, 4:53 p.m. UTC | #3
On Fri, Nov 22, 2013 at 10:56:05AM +0000, Marc Zyngier wrote:
> On 22/11/13 01:51, Christoffer Dall wrote:
> > On 21 November 2013 00:59, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> A CP15 instruction execution can be reordered, requiring an
> >> isb to be sure it is executed in program order.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm/cpu/armv7/nonsec_virt.S | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
> >> index 29987cd..648066f 100644
> >> --- a/arch/arm/cpu/armv7/nonsec_virt.S
> >> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
> >> @@ -47,6 +47,7 @@ _secure_monitor:
> >>  #endif
> >>
> >>         mcr     p15, 0, r1, c1, c1, 0           @ write SCR (with NS bit set)
> >> +       isb
> >>
> >>  #ifdef CONFIG_ARMV7_VIRT
> >>         mrceq   p15, 0, r0, c12, c0, 1          @ get MVBAR value
> >> --
> >> 1.8.2.3
> >>
> > Does this matter?  Are we not still in monitor mode and therefore
> > secure and the exception return below will surely be ordered by the
> > cpu after the mcr, right? or no?
> 
> You need to look at what is between the SCR access and the exception
> return (and you cannot see that from the patch): There's a write to
> HVBAR that can only be executed if SCR.NS==1.
> 
> If the write to SCR is delayed, the whole thing will stop very quickly...
> 
Ah, I didn't realize this specific about PL2-mode system control
registers and relied on the fact that we were just in monitor mode and
that the setting of this bit essentially didn't matter; I obviously got
this wrong, and I think to be fair that Andre had this isb in his
original code and removed it after my review.

/my bad.

Thanks for the explanation.

-Christoffer
Andre Przywara Nov. 26, 2013, 2:39 p.m. UTC | #4
On 11/21/2013 09:59 AM, Marc Zyngier wrote:
> A CP15 instruction execution can be reordered, requiring an
> isb to be sure it is executed in program order.

Makes sense ;-) and works on the VExpress TC2.

Albert, Tom, please apply for v2014.01.

Acked-by: Andre Przywara <andre.przywara@linaro.org>

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   arch/arm/cpu/armv7/nonsec_virt.S | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
> index 29987cd..648066f 100644
> --- a/arch/arm/cpu/armv7/nonsec_virt.S
> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
> @@ -47,6 +47,7 @@ _secure_monitor:
>   #endif
>
>   	mcr	p15, 0, r1, c1, c1, 0		@ write SCR (with NS bit set)
> +	isb
>
>   #ifdef CONFIG_ARMV7_VIRT
>   	mrceq	p15, 0, r0, c12, c0, 1		@ get MVBAR value
>
TigerLiu@viatech.com.cn Dec. 30, 2013, 3:10 a.m. UTC | #5
Hi, Dall:
I have a few questions about switching cpu's state from secure to
non-sec in uboot.
1. I found do_nonsec_virt_switch() function had been integrated in
uboot_2014_01_RC2.
    This function would switch cpu from secure state to non-sec, even
into hyp-state.
    So, my question is:
    If a SOC is based on ARMv7 architecture:
    Virt-v7.c / nonsec_virt.S are common files to all kinds of SOCs
which are produced by different Vendors?

2. Does uboot need to switch its state to non-sec?
    Based on ARM company released doc:
    U-boot should run at non-sec state, so no need to swith to non-sec
again.

Best wishes,
Christoffer Dall Dec. 30, 2013, 4:57 a.m. UTC | #6
On 29 December 2013 19:10,  <TigerLiu@viatech.com.cn> wrote:
> Hi, Dall:
> I have a few questions about switching cpu's state from secure to
> non-sec in uboot.
> 1. I found do_nonsec_virt_switch() function had been integrated in
> uboot_2014_01_RC2.
>     This function would switch cpu from secure state to non-sec, even
> into hyp-state.
>     So, my question is:
>     If a SOC is based on ARMv7 architecture:
>     Virt-v7.c / nonsec_virt.S are common files to all kinds of SOCs
> which are produced by different Vendors?

Yes, the aim is to be able to reuse as much of this code for as many
platforms as possible.

>
> 2. Does uboot need to switch its state to non-sec?
>     Based on ARM company released doc:
>     U-boot should run at non-sec state, so no need to swith to non-sec
> again.
>
It depends on the board.  Which ARM doc are you referring to?

In general, there are three options for how u-boot is booted:
1. In secure mode
2. In non-secure hyp mode
3. in non-secure svc mode

for (1) you can just switch to non-secure hyp.  for (2) you don't have
to do anything.  for (3) you're screwed, unless there's a backdoor
call to enter Hyp mode (typically found on TI hardware).

If we are talking PSCI, that's a different story, and if that's
provided by the board and not U-boot (see Marc's recent work), then I
would expect that board to always boot U-boot in Hyp mode to be
coherent with the documentation.

-Christoffer
TigerLiu@viatech.com.cn Dec. 30, 2013, 5:15 a.m. UTC | #7
Hi, Dall:
Thanks for your quick response!
>It depends on the board.  Which ARM doc are you referring to?
ARM Security Technology : Building a Secure System using TrustZone
Technology.
(PRD29-GENC-009492C)
Figure 5-2 in Chapter 5.2.1 Boot Sequence.
Based on my understanding, U-boot is classfied as normal world boot
loader.
Maybe my understanding is wrong! :)

>In general, there are three options for how u-boot is booted:
>1. In secure mode
>2. In non-secure hyp mode
>3. in non-secure svc mode

>for (1) you can just switch to non-secure hyp.  for (2) you don't have
>to do anything.  for (3) you're screwed, unless there's a backdoor
>call to enter Hyp mode (typically found on TI hardware).
For (1), i didn't get it totally:
On a platform with a CA7 supporting TZ tech, but this SOC not support
VT, 
usually cpu is powered on in secure mode.
So, i could only switch it to non-sec state?
If this CA7 also supports VT, so i could select 2 goals:
Switch to non-sec state, or swith to non-sec hyp mode?

I think non-sec state is not identical with non-sec hyp mode.

Best wishes,


-Christoffer
Christoffer Dall Dec. 30, 2013, 5:22 a.m. UTC | #8
On 29 December 2013 21:15,  <TigerLiu@viatech.com.cn> wrote:
> Hi, Dall:
> Thanks for your quick response!
>>It depends on the board.  Which ARM doc are you referring to?
> ARM Security Technology : Building a Secure System using TrustZone
> Technology.
> (PRD29-GENC-009492C)
> Figure 5-2 in Chapter 5.2.1 Boot Sequence.
> Based on my understanding, U-boot is classfied as normal world boot
> loader.
> Maybe my understanding is wrong! :)

I am not an authority on classifying software according to some
specification.  All I know is how the architecture and hardware works
to some limited degree.  My take on this would be that it depends on
your use of TrustZone.

>
>>In general, there are three options for how u-boot is booted:
>>1. In secure mode
>>2. In non-secure hyp mode
>>3. in non-secure svc mode
>
>>for (1) you can just switch to non-secure hyp.  for (2) you don't have
>>to do anything.  for (3) you're screwed, unless there's a backdoor
>>call to enter Hyp mode (typically found on TI hardware).
> For (1), i didn't get it totally:
> On a platform with a CA7 supporting TZ tech, but this SOC not support
> VT,

If it's a Cortex-A7 you have both virtualization extensions and the
security extensions.  If it does not, it's not a Cortex-A7.

> usually cpu is powered on in secure mode.

yes, a CPU is powered on in secure mode, but it varies what the first
piece of software that runs is.  If we are talking u-boot:

> So, i could only switch it to non-sec state?

if there is no virtualization support on your SoC, then there is no
Hyp mode, and you can not switch to it.

> If this CA7 also supports VT, so i could select 2 goals:
> Switch to non-sec state, or swith to non-sec hyp mode?
>
> I think non-sec state is not identical with non-sec hyp mode.
>

I think you need to look at the ARM ARM more carefully:

non-secure *state* includes several non-secure CPU modes (usr, svc,
und, abt, irq, fiq, hyp).  You can choose to switch to either of them
as you want, but if you want to boot Linux you should choose Hyp mode
so KVM will work, unless you are writing your own hypervisor or use
Hyp for something else, in which case you can fall back to booting
Linux in svc mode.

-Christoffer
TigerLiu@viatech.com.cn Dec. 30, 2013, 5:33 a.m. UTC | #9
Hi, Dall:
Thanks a lot!
>non-secure *state* includes several non-secure CPU modes (usr, svc,
>und, abt, irq, fiq, hyp).  You can choose to switch to either of them
>as you want, but if you want to boot Linux you should choose Hyp mode
>so KVM will work, unless you are writing your own hypervisor or use
>Hyp for something else, in which case you can fall back to booting
>Linux in svc mode.
Yes, you are right, i got it!

Best wishes,
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
index 29987cd..648066f 100644
--- a/arch/arm/cpu/armv7/nonsec_virt.S
+++ b/arch/arm/cpu/armv7/nonsec_virt.S
@@ -47,6 +47,7 @@  _secure_monitor:
 #endif
 
 	mcr	p15, 0, r1, c1, c1, 0		@ write SCR (with NS bit set)
+	isb
 
 #ifdef CONFIG_ARMV7_VIRT
 	mrceq	p15, 0, r0, c12, c0, 1		@ get MVBAR value