[v5,3/8] ARM: add assembly routine to switch to non-secure state

While actually switching to non-secure state is one thing, another
part of this process is to make sure that we still have full access
to the interrupt controller (GIC).
The GIC is fully aware of secure vs. non-secure state, some
registers are banked, others may be configured to be accessible from
secure state only.
To be as generic as possible, we get the GIC memory mapped address
based on the PERIPHBASE value in the CBAR register. Since this
register is not architecturally defined, we check the MIDR before to
be from an A15 or A7.
For CPUs not having the CBAR or boards with wrong information herein
we allow providing the base address as a configuration variable.

Now that we know the GIC address, we:
a) allow private interrupts to be delivered to the core
   (GICD_IGROUPR0 = 0xFFFFFFFF)
b) enable the CPU interface (GICC_CTLR[0] = 1)
c) set the priority filter to allow non-secure interrupts
   (GICC_PMR = 0xFF)

Also we allow access to all coprocessor interfaces from non-secure
state by writing the appropriate bits in the NSACR register.

The generic timer base frequency register is only accessible from
secure state, so we have to program it now. Actually this should be
done from primary firmware before, but some boards seems to omit
this, so if needed we do this here with a board specific value.
The Versatile Express board does not need this, so we remove the
frequency from the configuration file here.

After having switched to non-secure state, we also enable the
non-secure GIC CPU interface, since this register is banked.

Since we need to call this routine also directly from the smp_pen
later (where we don't have any stack), we can only use caller saved
registers r0-r3 and r12 to not mess with the compiler.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/cpu/armv7/nonsec_virt.S    | 86 +++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/armv7.h        | 21 +++++++++
 arch/arm/include/asm/gic.h          | 17 ++++++++
 include/configs/vexpress_ca15_tc2.h |  2 -
 4 files changed, 124 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/include/asm/gic.h

Changes:
v3..v4: clear reserved bits in CBAR
v4..v5: none

Hi Andre,

There is another approach taken in xen. (xen/arch/arm/mode_switch.S)
Which do you think is the better approach

Regards
-mj

On 9/19/13, Andre Przywara <andre.przywara@linaro.org> wrote:
> While actually switching to non-secure state is one thing, another
> part of this process is to make sure that we still have full access
> to the interrupt controller (GIC).
> The GIC is fully aware of secure vs. non-secure state, some
> registers are banked, others may be configured to be accessible from
> secure state only.
> To be as generic as possible, we get the GIC memory mapped address
> based on the PERIPHBASE value in the CBAR register. Since this
> register is not architecturally defined, we check the MIDR before to
> be from an A15 or A7.
> For CPUs not having the CBAR or boards with wrong information herein
> we allow providing the base address as a configuration variable.
>
> Now that we know the GIC address, we:
> a) allow private interrupts to be delivered to the core
>    (GICD_IGROUPR0 = 0xFFFFFFFF)
> b) enable the CPU interface (GICC_CTLR[0] = 1)
> c) set the priority filter to allow non-secure interrupts
>    (GICC_PMR = 0xFF)
>
> Also we allow access to all coprocessor interfaces from non-secure
> state by writing the appropriate bits in the NSACR register.
>
> The generic timer base frequency register is only accessible from
> secure state, so we have to program it now. Actually this should be
> done from primary firmware before, but some boards seems to omit
> this, so if needed we do this here with a board specific value.
> The Versatile Express board does not need this, so we remove the
> frequency from the configuration file here.
>
> After having switched to non-secure state, we also enable the
> non-secure GIC CPU interface, since this register is banked.
>
> Since we need to call this routine also directly from the smp_pen
> later (where we don't have any stack), we can only use caller saved
> registers r0-r3 and r12 to not mess with the compiler.
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  arch/arm/cpu/armv7/nonsec_virt.S    | 86
> +++++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/armv7.h        | 21 +++++++++
>  arch/arm/include/asm/gic.h          | 17 ++++++++
>  include/configs/vexpress_ca15_tc2.h |  2 -
>  4 files changed, 124 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/include/asm/gic.h
>
> Changes:
> v3..v4: clear reserved bits in CBAR
> v4..v5: none
>
> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S
> b/arch/arm/cpu/armv7/nonsec_virt.S
> index c21bca3..3dd60b7 100644
> --- a/arch/arm/cpu/armv7/nonsec_virt.S
> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
> @@ -23,6 +23,11 @@
>   */
>
>  #include <config.h>
> +#include <linux/linkage.h>
> +#include <asm/gic.h>
> +#include <asm/armv7.h>
> +
> +.arch_extension sec
>
>  /* the vector table for secure state */
>  _monitor_vectors:
> @@ -52,3 +57,84 @@ _secure_monitor:
>
>  	movs	pc, lr				@ return to non-secure SVC
>
> +/*
> + * Switch a core to non-secure state.
> + *
> + *  1. initialize the GIC per-core interface
> + *  2. allow coprocessor access in non-secure modes
> + *  3. switch the cpu mode (by calling "smc #0")
> + *
> + * Called from smp_pen by secondary cores and directly by the BSP.
> + * Do not assume that the stack is available and only use registers
> + * r0-r3 and r12.
> + *
> + * PERIPHBASE is used to get the GIC address. This could be 40 bits long,
> + * though, but we check this in C before calling this function.
> + */
> +ENTRY(_nonsec_init)
> +#ifdef CONFIG_ARM_GIC_BASE_ADDRESS
> +	ldr	r2, =CONFIG_ARM_GIC_BASE_ADDRESS
> +#else
> +	mrc	p15, 4, r2, c15, c0, 0		@ read CBAR
> +	bfc	r2, #0, #15			@ clear reserved bits
> +#endif
> +	add	r3, r2, #GIC_DIST_OFFSET	@ GIC dist i/f offset
> +	mvn	r1, #0				@ all bits to 1
> +	str	r1, [r3, #GICD_IGROUPRn]	@ allow private interrupts
> +
> +	mrc	p15, 0, r0, c0, c0, 0		@ read MIDR
> +	ldr	r1, =MIDR_PRIMARY_PART_MASK
> +	and	r0, r0, r1			@ mask out variant and revision
> +
> +	ldr	r1, =MIDR_CORTEX_A7_R0P0 & MIDR_PRIMARY_PART_MASK
> +	cmp	r0, r1				@ check for Cortex-A7
> +
> +	ldr	r1, =MIDR_CORTEX_A15_R0P0 & MIDR_PRIMARY_PART_MASK
> +	cmpne	r0, r1				@ check for Cortex-A15
> +
> +	movne	r1, #GIC_CPU_OFFSET_A9		@ GIC CPU offset for A9
> +	moveq	r1, #GIC_CPU_OFFSET_A15		@ GIC CPU offset for A15/A7
> +	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
> +
> +	mov	r1, #1				@ set GICC_CTLR[enable]
> +	str	r1, [r3, #GICC_CTLR]		@ and clear all other bits
> +	mov	r1, #0xff
> +	str	r1, [r3, #GICC_PMR]		@ set priority mask register
> +
> +	movw	r1, #0x3fff
> +	movt	r1, #0x0006
> +	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
> +
> +/* The CNTFRQ register of the generic timer needs to be
> + * programmed in secure state. Some primary bootloaders / firmware
> + * omit this, so if the frequency is provided in the configuration,
> + * we do this here instead.
> + * But first check if we have the generic timer.
> + */
> +#ifdef CONFIG_SYS_CLK_FREQ
> +	mrc	p15, 0, r0, c0, c1, 1		@ read ID_PFR1
> +	and	r0, r0, #CPUID_ARM_GENTIMER_MASK	@ mask arch timer bits
> +	cmp	r0, #(1 << CPUID_ARM_GENTIMER_SHIFT)
> +	ldreq	r1, =CONFIG_SYS_CLK_FREQ
> +	mcreq	p15, 0, r1, c14, c0, 0		@ write CNTFRQ
> +#endif
> +
> +	adr	r1, _monitor_vectors
> +	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR to secure vectors
> +
> +	mrc	p15, 0, ip, c12, c0, 0		@ save secure copy of VBAR
> +
> +	isb
> +	smc	#0				@ call into MONITOR mode
> +
> +	mcr	p15, 0, ip, c12, c0, 0		@ write non-secure copy of VBAR
> +
> +	mov	r1, #1
> +	str	r1, [r3, #GICC_CTLR]		@ enable non-secure CPU i/f
> +	add	r2, r2, #GIC_DIST_OFFSET
> +	str	r1, [r2, #GICD_CTLR]		@ allow private interrupts
> +
> +	mov	r0, r3				@ return GICC address
> +
> +	bx	lr
> +ENDPROC(_nonsec_init)
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index 0f7cbbf..3dcfc8f 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -18,6 +18,22 @@
>  #define MIDR_CORTEX_A15_R0P0	0x410FC0F0
>  #define MIDR_CORTEX_A15_R2P2	0x412FC0F2
>
> +/* Cortex-A7 revisions */
> +#define MIDR_CORTEX_A7_R0P0	0x410FC070
> +
> +#define MIDR_PRIMARY_PART_MASK	0xFF0FFFF0
> +
> +/* ID_PFR1 feature fields */
> +#define CPUID_ARM_SEC_SHIFT		4
> +#define CPUID_ARM_SEC_MASK		(0xF << CPUID_ARM_SEC_SHIFT)
> +#define CPUID_ARM_VIRT_SHIFT		12
> +#define CPUID_ARM_VIRT_MASK		(0xF << CPUID_ARM_VIRT_SHIFT)
> +#define CPUID_ARM_GENTIMER_SHIFT	16
> +#define CPUID_ARM_GENTIMER_MASK		(0xF << CPUID_ARM_GENTIMER_SHIFT)
> +
> +/* valid bits in CBAR register / PERIPHBASE value */
> +#define CBAR_MASK			0xFFFF8000
> +
>  /* CCSIDR */
>  #define CCSIDR_LINE_SIZE_OFFSET		0
>  #define CCSIDR_LINE_SIZE_MASK		0x7
> @@ -60,6 +76,11 @@ 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
> +/* defined in assembly file */
> +unsigned int _nonsec_init(void);
> +#endif /* CONFIG_ARMV7_NONSEC */
> +
>  #endif /* ! __ASSEMBLY__ */
>
>  #endif
> diff --git a/arch/arm/include/asm/gic.h b/arch/arm/include/asm/gic.h
> new file mode 100644
> index 0000000..c2b1e28
> --- /dev/null
> +++ b/arch/arm/include/asm/gic.h
> @@ -0,0 +1,17 @@
> +#ifndef __GIC_V2_H__
> +#define __GIC_V2_H__
> +
> +/* register offsets for the ARM generic interrupt controller (GIC) */
> +
> +#define GIC_DIST_OFFSET		0x1000
> +#define GICD_CTLR		0x0000
> +#define GICD_TYPER		0x0004
> +#define GICD_IGROUPRn		0x0080
> +#define GICD_SGIR		0x0F00
> +
> +#define GIC_CPU_OFFSET_A9	0x0100
> +#define GIC_CPU_OFFSET_A15	0x2000
> +#define GICC_CTLR		0x0000
> +#define GICC_PMR		0x0004
> +
> +#endif
> diff --git a/include/configs/vexpress_ca15_tc2.h
> b/include/configs/vexpress_ca15_tc2.h
> index d1431e5..89ce1c7 100644
> --- a/include/configs/vexpress_ca15_tc2.h
> +++ b/include/configs/vexpress_ca15_tc2.h
> @@ -15,6 +15,4 @@
>  #include "vexpress_common.h"
>  #define CONFIG_BOOTP_VCI_STRING     "U-boot.armv7.vexpress_ca15x2_tc2"
>
> -#define CONFIG_SYS_CLK_FREQ 24000000
> -
>  #endif
> --
> 1.7.12.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

On Thu, Sep 19, 2013 at 10:00:03PM +0530, Mj Embd wrote:
> Hi Andre,
> 
> There is another approach taken in xen. (xen/arch/arm/mode_switch.S)
> Which do you think is the better approach
> 
Hi there,

I'm not sure I completely understand your question.  Do you think this
patch series should be changed to take something from Xen?  If so, can
you please clarify why?

For the record, this patch series has been reviewed and tested quite a
lot for U-Boot and we would very much like to get this merged and avoid
further churn, unless there's a compelling technical reason to change
it.

Thanks,
-Christoffer

two quick points
(a) xen already has a mode_switch code, so AFAIK xen might not use it
(as suggested by comment in another patch in this patch set)
(b) There are 2 methods of switching from Secure to Hyp mode
one you have proposed another implemented in xen. I was suggesting
take the best approach



On 9/20/13, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Thu, Sep 19, 2013 at 10:00:03PM +0530, Mj Embd wrote:
>> Hi Andre,
>>
>> There is another approach taken in xen. (xen/arch/arm/mode_switch.S)
>> Which do you think is the better approach
>>
> Hi there,
>
> I'm not sure I completely understand your question.  Do you think this
> patch series should be changed to take something from Xen?  If so, can
> you please clarify why?
>
> For the record, this patch series has been reviewed and tested quite a
> lot for U-Boot and we would very much like to get this merged and avoid
> further churn, unless there's a compelling technical reason to change
> it.
>
> Thanks,
> -Christoffer
>

On Fri, Sep 20, 2013 at 01:27:48AM +0530, Mj Embd wrote:
> two quick points
> (a) xen already has a mode_switch code, so AFAIK xen might not use it
> (as suggested by comment in another patch in this patch set)

For KVM the boot procedure for Hyp mode is quite clearly defined: the
kernel must be booted in Hyp mode.

I was under the impression that Xen wanted to use the same paradigm and
rely on bootloaders to switch to Hyp mode and thereby get rid of the
code in Xen.

> (b) There are 2 methods of switching from Secure to Hyp mode
> one you have proposed another implemented in xen. I was suggesting
> take the best approach
> 

Can you please be more specific?  Not everyone here knows the Xen
low-level mode switch details by heart.  As far as I know, there is only
one architecturally defined proper mode to switch from secure mode to
non-secure mode, and the state that needs to be configured for Hyp-mode
and NS-mode is well defined.  Obviously two implementation can do things
differently (different order, different programminge environment, etc.)
but that doesn't mean one is better than the other.

I think it would be more productive if you can simply look at this code
and if you think some things are done more efficiently in Xen, please
comment on that, which would be very helpful.  I'm afraid there's no
magic way to apply a block of Xen code into U-Boot wihtout manual
adjustment anyway, or the other way around for that matter.

-Christoffer

Hello Christoffer,

I agree with both of you points.

What I found different in 2 approaches is that in your approach
S->Monitor->NS->Hyp using svc and hvc

While the other approach is setting the M bits directly in cpsr

Xen uses the following

cpsid aif, #0x16 /* Enter Monitor Mode*/
....
...
mrs   r0, cpsr               /* Copy the CPSR */
add   r0, r0, #0x4           /* 0x16 (Monitor) -> 0x1a (Hyp) */
msr   spsr_cxsf, r0          /* into the SPSR */
movs  pc, r3                 /* Exception-return into Hyp mode */

The second one is a bit simpler as it does not involve fault handlers.

I was just suggesting that the best approach to be used ...

Best Regards,
mj

On 9/20/13, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Fri, Sep 20, 2013 at 01:27:48AM +0530, Mj Embd wrote:
>> two quick points
>> (a) xen already has a mode_switch code, so AFAIK xen might not use it
>> (as suggested by comment in another patch in this patch set)
>
> For KVM the boot procedure for Hyp mode is quite clearly defined: the
> kernel must be booted in Hyp mode.
>
> I was under the impression that Xen wanted to use the same paradigm and
> rely on bootloaders to switch to Hyp mode and thereby get rid of the
> code in Xen.
>
>> (b) There are 2 methods of switching from Secure to Hyp mode
>> one you have proposed another implemented in xen. I was suggesting
>> take the best approach
>>
>
> Can you please be more specific?  Not everyone here knows the Xen
> low-level mode switch details by heart.  As far as I know, there is only
> one architecturally defined proper mode to switch from secure mode to
> non-secure mode, and the state that needs to be configured for Hyp-mode
> and NS-mode is well defined.  Obviously two implementation can do things
> differently (different order, different programminge environment, etc.)
> but that doesn't mean one is better than the other.
>
> I think it would be more productive if you can simply look at this code
> and if you think some things are done more efficiently in Xen, please
> comment on that, which would be very helpful.  I'm afraid there's no
> magic way to apply a block of Xen code into U-Boot wihtout manual
> adjustment anyway, or the other way around for that matter.
>
> -Christoffer
>

On 09/19/2013 09:57 PM, Mj Embd wrote:
> two quick points
> (a) xen already has a mode_switch code, so AFAIK xen might not use it
> (as suggested by comment in another patch in this patch set)

Just a few days ago Ian sent out patches to remove this code from Xen. 
That code was never meant to survive, just to have something working to 
let people play with Xen easily. Xen now requires to be started in HYP 
mode, just like KVM.

http://lists.xen.org/archives/html/xen-devel/2013-09/msg01690.html

Patch 5/7 has more details.

Regards,
Andre.

> (b) There are 2 methods of switching from Secure to Hyp mode
> one you have proposed another implemented in xen. I was suggesting
> take the best approach
>
>
>
> On 9/20/13, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>> On Thu, Sep 19, 2013 at 10:00:03PM +0530, Mj Embd wrote:
>>> Hi Andre,
>>>
>>> There is another approach taken in xen. (xen/arch/arm/mode_switch.S)
>>> Which do you think is the better approach
>>>
>> Hi there,
>>
>> I'm not sure I completely understand your question.  Do you think this
>> patch series should be changed to take something from Xen?  If so, can
>> you please clarify why?
>>
>> For the record, this patch series has been reviewed and tested quite a
>> lot for U-Boot and we would very much like to get this merged and avoid
>> further churn, unless there's a compelling technical reason to change
>> it.
>>
>> Thanks,
>> -Christoffer
>>
>
>

On Fri, 2013-09-20 at 01:27 +0530, Mj Embd wrote:
> two quick points
> (a) xen already has a mode_switch code, so AFAIK xen might not use it
> (as suggested by comment in another patch in this patch set)

Xen absolutely wants to use this code. The stuff in Xen was there as a
hack when this stuff wasn't present in the bootloader. The sooner it can
be removed the happier I will be.

Ian.

On Thu, 2013-09-19 at 21:11 +0100, Christoffer Dall wrote:
> On Fri, Sep 20, 2013 at 01:27:48AM +0530, Mj Embd wrote:
> > two quick points
> > (a) xen already has a mode_switch code, so AFAIK xen might not use it
> > (as suggested by comment in another patch in this patch set)
> 
> For KVM the boot procedure for Hyp mode is quite clearly defined: the
> kernel must be booted in Hyp mode.
> 
> I was under the impression that Xen wanted to use the same paradigm and
> rely on bootloaders to switch to Hyp mode and thereby get rid of the
> code in Xen.

Absolutely correct, our needs are the same as KVM and we want to be
booted in the same way.

Ian.

On 09/19/2013 10:38 PM, Mj Embd wrote:
> Hello Christoffer,
>
> I agree with both of you points.
>
> What I found different in 2 approaches is that in your approach
> S->Monitor->NS->Hyp using svc and hvc
>
> While the other approach is setting the M bits directly in cpsr
>
> Xen uses the following
>
> cpsid aif, #0x16 /* Enter Monitor Mode*/
> ....
> ...
> mrs   r0, cpsr               /* Copy the CPSR */
> add   r0, r0, #0x4           /* 0x16 (Monitor) -> 0x1a (Hyp) */
> msr   spsr_cxsf, r0          /* into the SPSR */
> movs  pc, r3                 /* Exception-return into Hyp mode */
>
> The second one is a bit simpler as it does not involve fault handlers.

The ARM ARM recommends _not_ to do this:
Read more at the ARMv7-A Architecture Reference Manual, section B1.5.1: 
Security states/Changing from Secure to Non-secure state:

"To avoid security holes, software must not:
--    Change from Secure to Non-secure state by using an MSR or CPS 
instruction to switch from Monitor mode to some other mode while SCR.NS 
is 1.
..."

>
> I was just suggesting that the best approach to be used ...

Looks like this is what we do ;-)

Regards,
Andre.

>
> On 9/20/13, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>> On Fri, Sep 20, 2013 at 01:27:48AM +0530, Mj Embd wrote:
>>> two quick points
>>> (a) xen already has a mode_switch code, so AFAIK xen might not use it
>>> (as suggested by comment in another patch in this patch set)
>>
>> For KVM the boot procedure for Hyp mode is quite clearly defined: the
>> kernel must be booted in Hyp mode.
>>
>> I was under the impression that Xen wanted to use the same paradigm and
>> rely on bootloaders to switch to Hyp mode and thereby get rid of the
>> code in Xen.
>>
>>> (b) There are 2 methods of switching from Secure to Hyp mode
>>> one you have proposed another implemented in xen. I was suggesting
>>> take the best approach
>>>
>>
>> Can you please be more specific?  Not everyone here knows the Xen
>> low-level mode switch details by heart.  As far as I know, there is only
>> one architecturally defined proper mode to switch from secure mode to
>> non-secure mode, and the state that needs to be configured for Hyp-mode
>> and NS-mode is well defined.  Obviously two implementation can do things
>> differently (different order, different programminge environment, etc.)
>> but that doesn't mean one is better than the other.
>>
>> I think it would be more productive if you can simply look at this code
>> and if you think some things are done more efficiently in Xen, please
>> comment on that, which would be very helpful.  I'm afraid there's no
>> magic way to apply a block of Xen code into U-Boot wihtout manual
>> adjustment anyway, or the other way around for that matter.
>>
>> -Christoffer
>>
>
>

On Fri, Sep 20, 2013 at 3:01 AM, Andre Przywara
<andre.przywara@linaro.org>wrote:

> On 09/19/2013 10:38 PM, Mj Embd wrote:
>
>> Hello Christoffer,
>>
>> I agree with both of you points.
>>
>> What I found different in 2 approaches is that in your approach
>> S->Monitor->NS->Hyp using svc and hvc
>>
>> While the other approach is setting the M bits directly in cpsr
>>
>> Xen uses the following
>>
>> cpsid aif, #0x16 /* Enter Monitor Mode*/
>> ....
>> ...
>> mrs   r0, cpsr               /* Copy the CPSR */
>> add   r0, r0, #0x4           /* 0x16 (Monitor) -> 0x1a (Hyp) */
>> msr   spsr_cxsf, r0          /* into the SPSR */
>> movs  pc, r3                 /* Exception-return into Hyp mode */
>>
>> The second one is a bit simpler as it does not involve fault handlers.
>>
>
> The ARM ARM recommends _not_ to do this:
> Read more at the ARMv7-A Architecture Reference Manual, section B1.5.1:
> Security states/Changing from Secure to Non-secure state:
>
> "To avoid security holes, software must not:
> --    Change from Secure to Non-secure state by using an MSR or CPS
> instruction to switch from Monitor mode to some other mode while SCR.NS is
> 1.
> ..."
>
>
> Good one, As per this statement from Secure Monitor to Hyp Mode,  HVC
should be used. But Secure Supervisor to Secure Monitor does not set NS=1
so first part cpsid aif, #0x16 is valid ?
Or there is some other statement in the manual which I am not aware of ?

>
>> I was just suggesting that the best approach to be used ...
>>
>
> Looks like this is what we do ;-)
>
> Regards,
> Andre.
>
>
>
>> On 9/20/13, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>
>>> On Fri, Sep 20, 2013 at 01:27:48AM +0530, Mj Embd wrote:
>>>
>>>> two quick points
>>>> (a) xen already has a mode_switch code, so AFAIK xen might not use it
>>>> (as suggested by comment in another patch in this patch set)
>>>>
>>>
>>> For KVM the boot procedure for Hyp mode is quite clearly defined: the
>>> kernel must be booted in Hyp mode.
>>>
>>> I was under the impression that Xen wanted to use the same paradigm and
>>> rely on bootloaders to switch to Hyp mode and thereby get rid of the
>>> code in Xen.
>>>
>>>  (b) There are 2 methods of switching from Secure to Hyp mode
>>>> one you have proposed another implemented in xen. I was suggesting
>>>> take the best approach
>>>>
>>>>
>>> Can you please be more specific?  Not everyone here knows the Xen
>>> low-level mode switch details by heart.  As far as I know, there is only
>>> one architecturally defined proper mode to switch from secure mode to
>>> non-secure mode, and the state that needs to be configured for Hyp-mode
>>> and NS-mode is well defined.  Obviously two implementation can do things
>>> differently (different order, different programminge environment, etc.)
>>> but that doesn't mean one is better than the other.
>>>
>>> I think it would be more productive if you can simply look at this code
>>> and if you think some things are done more efficiently in Xen, please
>>> comment on that, which would be very helpful.  I'm afraid there's no
>>> magic way to apply a block of Xen code into U-Boot wihtout manual
>>> adjustment anyway, or the other way around for that matter.
>>>
>>> -Christoffer
>>>
>>>
>>
>>
>

Hello Andre,
I need a bit clarification here, if you read the next line after the line
you have quoted. It clearly says that you can use a MCR to change from
Secure to NS in Monitor Mode
"Use an MCR instruction that writes SCR.NS to change from Secure to
Non-secure state. This means ARM recommends that software does not alter SCR.NS
in any mode except Monitor mode. ARM deprecates changing SCR.NS in any
other mode."



On Fri, Sep 20, 2013 at 3:09 AM, Mj Embd <mj.embd@gmail.com> wrote:

>
>
>
> On Fri, Sep 20, 2013 at 3:01 AM, Andre Przywara <andre.przywara@linaro.org
> > wrote:
>
>> On 09/19/2013 10:38 PM, Mj Embd wrote:
>>
>>> Hello Christoffer,
>>>
>>> I agree with both of you points.
>>>
>>> What I found different in 2 approaches is that in your approach
>>> S->Monitor->NS->Hyp using svc and hvc
>>>
>>> While the other approach is setting the M bits directly in cpsr
>>>
>>> Xen uses the following
>>>
>>> cpsid aif, #0x16 /* Enter Monitor Mode*/
>>> ....
>>> ...
>>> mrs   r0, cpsr               /* Copy the CPSR */
>>> add   r0, r0, #0x4           /* 0x16 (Monitor) -> 0x1a (Hyp) */
>>> msr   spsr_cxsf, r0          /* into the SPSR */
>>> movs  pc, r3                 /* Exception-return into Hyp mode */
>>>
>>> The second one is a bit simpler as it does not involve fault handlers.
>>>
>>
>> The ARM ARM recommends _not_ to do this:
>> Read more at the ARMv7-A Architecture Reference Manual, section B1.5.1:
>> Security states/Changing from Secure to Non-secure state:
>>
>> "To avoid security holes, software must not:
>> --    Change from Secure to Non-secure state by using an MSR or CPS
>> instruction to switch from Monitor mode to some other mode while SCR.NS is
>> 1.
>> ..."
>>
>>
>> Good one, As per this statement from Secure Monitor to Hyp Mode,  HVC
> should be used. But Secure Supervisor to Secure Monitor does not set NS=1
> so first part cpsid aif, #0x16 is valid ?
> Or there is some other statement in the manual which I am not aware of ?
>
>>
>>> I was just suggesting that the best approach to be used ...
>>>
>>
>> Looks like this is what we do ;-)
>>
>> Regards,
>> Andre.
>>
>>
>>
>>> On 9/20/13, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>>
>>>> On Fri, Sep 20, 2013 at 01:27:48AM +0530, Mj Embd wrote:
>>>>
>>>>> two quick points
>>>>> (a) xen already has a mode_switch code, so AFAIK xen might not use it
>>>>> (as suggested by comment in another patch in this patch set)
>>>>>
>>>>
>>>> For KVM the boot procedure for Hyp mode is quite clearly defined: the
>>>> kernel must be booted in Hyp mode.
>>>>
>>>> I was under the impression that Xen wanted to use the same paradigm and
>>>> rely on bootloaders to switch to Hyp mode and thereby get rid of the
>>>> code in Xen.
>>>>
>>>>  (b) There are 2 methods of switching from Secure to Hyp mode
>>>>> one you have proposed another implemented in xen. I was suggesting
>>>>> take the best approach
>>>>>
>>>>>
>>>> Can you please be more specific?  Not everyone here knows the Xen
>>>> low-level mode switch details by heart.  As far as I know, there is only
>>>> one architecturally defined proper mode to switch from secure mode to
>>>> non-secure mode, and the state that needs to be configured for Hyp-mode
>>>> and NS-mode is well defined.  Obviously two implementation can do things
>>>> differently (different order, different programminge environment, etc.)
>>>> but that doesn't mean one is better than the other.
>>>>
>>>> I think it would be more productive if you can simply look at this code
>>>> and if you think some things are done more efficiently in Xen, please
>>>> comment on that, which would be very helpful.  I'm afraid there's no
>>>> magic way to apply a block of Xen code into U-Boot wihtout manual
>>>> adjustment anyway, or the other way around for that matter.
>>>>
>>>> -Christoffer
>>>>
>>>>
>>>
>>>
>>
>
>
> --
> -mj
>

On 20 September 2013 06:55, Mj Embd <mj.embd@gmail.com> wrote:
> Hello Andre,
> I need a bit clarification here, if you read the next line after the line
> you have quoted. It clearly says that you can use a MCR to change from
> Secure to NS in Monitor Mode

No, it's not saying that, because Monitor mode is always Secure:
this is exactly why an MCR to change SCR.NS is OK only in
Monitor mode -- it doesn't change from Secure to Non-Secure.
Only when you do an exception-return to leave Monitor mode
will you drop into the NonSecure world.

> "Use an MCR instruction that writes SCR.NS to change from Secure to
> Non-secure state. This means ARM recommends that software does not alter
> SCR.NS in any mode except Monitor mode. ARM deprecates changing SCR.NS in
> any other mode."

The text says "don't change from Secure to NonSecure by flipping
SCR.NS". It then lays out the corollary: the only time you then can
change SCR.NS is when it won't switch from Secure to NonSecure,
which is when you're in Monitor mode.

-- PMM

On Fri, Sep 20, 2013 at 4:05 AM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 20 September 2013 06:55, Mj Embd <mj.embd@gmail.com> wrote:
> > Hello Andre,
> > I need a bit clarification here, if you read the next line after the line
> > you have quoted. It clearly says that you can use a MCR to change from
> > Secure to NS in Monitor Mode
>
> No, it's not saying that, because Monitor mode is always Secure:
> this is exactly why an MCR to change SCR.NS is OK only in
> Monitor mode -- it doesn't change from Secure to Non-Secure.
> Only when you do an exception-return to leave Monitor mode
> will you drop into the NonSecure world.
>
> > "Use an MCR instruction that writes SCR.NS to change from Secure to
> > Non-secure state. This means ARM recommends that software does not alter
> > SCR.NS in any mode except Monitor mode. ARM deprecates changing SCR.NS in
> > any other mode."
>
> The text says "don't change from Secure to NonSecure by flipping
> SCR.NS". It then lays out the corollary: the only time you then can
> change SCR.NS is when it won't switch from Secure to NonSecure,
> which is when you're in Monitor mode.
>
> [MJ] Ok got your point. Then what would be the steps to return from
Monitor to Hyp mode?


> -- PMM
>

On 20 September 2013 07:50, Mj Embd <mj.embd@gmail.com> wrote:
> [MJ] Ok got your point. Then what would be the steps to return from Monitor
> to Hyp mode?

You can't do it directly. While still in the Secure world you set up
the HVBAR and a suitable vector table for hyp mode exceptions.
Then you can drop back into NS-SVC, and execute an HVC
instruction which puts you into hyp mode at the hvc entry point
in your vector table. See patch 7 in the series.

-- PMM