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.przyw...@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
> >>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
