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

[Christoffer Dall <cdall@cs.columbia.edu>]

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