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