The bootwrapper is really doubling as firmware, so it doesn't make sense for it to drop out of the Secure World before getting a chance to parse its parameters and configuration.
Instead, it should make sense to delay switching to the Normal World for as long as possible so that we have a chance to do any required firmware-level configuration in the Secure World first.
This quick hack is ***completely untested***, since I'm not working with any suitable kernel tree right now.
If someone with a KVM tree ready to run could give it a try, that would definitely save me some time.
Review also welcome (naturally)
Changes since v1:
* Don't rely on preservation of lr or sp across enter_hyp (this doesn't work because those registers are banked per-mode).
I'm still not convinced this series works, due image/model/dtb mismatches in my testing, but execution at least reaches the kernel now.
Cheers ---Dave
Dave Martin (3): bootwrapper: Fix misaligned Hyp mode vector table bootwrapper: Refactor entry into Hyp mode to be more reusable bootwrapper: Delay switch to Hyp mode until kernel entry
boot.S | 58 +++++++++++++++++++++++++++++++++++++------------------- semi_loader.h | 6 +++- 2 files changed, 42 insertions(+), 22 deletions(-)
Currently, it looks like we rely on luck in order to fall through to the correct vector when a Hyp Trap exception occurs.
This patch aligns the Hyp mode vector table explicitly to a 32-byte boundary, as required by the architecture.
Signed-off-by: Dave Martin dave.martin@linaro.org --- boot.S | 24 ++++++++++++------------ 1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/boot.S b/boot.S index 61cd93f..0acd243 100644 --- a/boot.S +++ b/boot.S @@ -12,6 +12,17 @@ .arch_extension virt .text
+.align 5 +/* Once we get rid of monitor.S, use these smc vectors too! */ +hyp_vectors: + .word 0 /* reset */ + .word 0 /* undef */ + .word 0 /* svc */ + .word 0 /* pabt */ + .word 0 /* dabt */ + b into_hyp_mode /* hvc */ + .word 0 /* irq */ + .word 0 /* fiq */
.globl start start: @@ -68,7 +79,7 @@ start: mcr p15, 0, r0, c12, c0, 1 @ Monitor vector base address
@ Set up hvbar so hvc comes back here. - ldr r0, =vectors + ldr r0, =hyp_vectors mov r7, #0xfffffff0 smc #0 @ Set HVBAR
@@ -79,17 +90,6 @@ start: @ This is how we enter hyp mode, for booting the next stage. hvc #0
-/* Once we get rid of monitor.S, use these smc vectors too! */ -vectors: - .word 0 /* reset */ - .word 0 /* undef */ - .word 0 /* svc */ - .word 0 /* pabt */ - .word 0 /* dabt */ - b into_hyp_mode /* hvc */ - .word 0 /* irq */ - .word 0 /* fiq */ - into_hyp_mode: @ Check CPU nr again mrc p15, 0, r0, c0, c0, 5 @ MPIDR (ARMv7 only)
* Split Hyp mode entry out into a separate macro. * hvc now jumps back to the caller in Hyp mode instead of branching to a fixed label.
Signed-off-by: Dave Martin dave.martin@linaro.org --- boot.S | 23 +++++++++++++++-------- 1 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/boot.S b/boot.S index 0acd243..128f74e 100644 --- a/boot.S +++ b/boot.S @@ -12,6 +12,15 @@ .arch_extension virt .text
+.macro enter_hyp + @ We can't call hvc from secure mode, so drop down first. + mov r7, #0xffffffff + smc #0 @ Change to NS-mode + + @ This is how we enter hyp mode, for booting the next stage. + hvc #0 +.endm + .align 5 /* Once we get rid of monitor.S, use these smc vectors too! */ hyp_vectors: @@ -20,10 +29,14 @@ hyp_vectors: .word 0 /* svc */ .word 0 /* pabt */ .word 0 /* dabt */ - b into_hyp_mode /* hvc */ + b 1f .word 0 /* irq */ .word 0 /* fiq */
+/* Return directly back to the caller without leaving Hyp mode: */ +1: mrs lr, elr_hyp + mov pc, lr + .globl start start: #ifdef SMP @@ -83,14 +96,8 @@ start: mov r7, #0xfffffff0 smc #0 @ Set HVBAR
- @ We can't call hvc from secure mode, so drop down first. - mov r7, #0xffffffff - smc #0 @ Change to NS-mode - - @ This is how we enter hyp mode, for booting the next stage. - hvc #0 + enter_hyp
-into_hyp_mode: @ Check CPU nr again mrc p15, 0, r0, c0, c0, 5 @ MPIDR (ARMv7 only) and r0, r0, #15 @ CPU number
Signed-off-by: Dave Martin dave.martin@linaro.org --- boot.S | 15 +++++++++++++-- semi_loader.h | 6 ++++-- 2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/boot.S b/boot.S index 128f74e..fe7acdf 100644 --- a/boot.S +++ b/boot.S @@ -96,8 +96,6 @@ start: mov r7, #0xfffffff0 smc #0 @ Set HVBAR
- enter_hyp - @ Check CPU nr again mrc p15, 0, r0, c0, c0, 5 @ MPIDR (ARMv7 only) and r0, r0, #15 @ CPU number @@ -125,6 +123,8 @@ start: ldr r1, [r0] cmp r1, #0 beq 1b + + enter_hyp mov pc, r1 @ branch to the given address #endif
@@ -170,6 +170,17 @@ __semi_call: #endif mov pc, lr
+.globl __boot_kernel +__boot_kernel: + mov r4, r0 + stmfd sp!, {r1-r3} + ldmia sp, {r0-r3} + + enter_hyp + + bx r4 +.type __boot_kernel, %function + @ @ Data @ diff --git a/semi_loader.h b/semi_loader.h index 6afba40..29f3d63 100644 --- a/semi_loader.h +++ b/semi_loader.h @@ -90,10 +90,12 @@ struct loader_info {
void load_kernel(struct loader_info *info);
+void __boot_kernel(unsigned entry_point, + unsigned r0, unsigned r1, unsigned r2, unsigned r3); + static void boot_kernel(struct loader_info *info, unsigned r0, unsigned r1, unsigned r2, unsigned r3) { - ((void (*)(unsigned, unsigned, unsigned, unsigned))info->kernel_entry)( - r0, r1, r2, r3); + __boot_kernel(info->kernel_entry, r0, r1, r2, r3); }
#endif /* ! SEMI_LOADER_H */
On 6 September 2012 18:12, Dave Martin dave.martin@linaro.org wrote:
Signed-off-by: Dave Martin dave.martin@linaro.org
This patch causes the kernel to get stuck at "Calibrating delay loop...".
I'm not sure why this happens (investigating) but if I move the enter_hyp macro call back to where it used to be (leaving the rest of the patch intact) it boots OK...
boot.S | 15 +++++++++++++-- semi_loader.h | 6 ++++-- 2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/boot.S b/boot.S index 128f74e..fe7acdf 100644 --- a/boot.S +++ b/boot.S @@ -96,8 +96,6 @@ start: mov r7, #0xfffffff0 smc #0 @ Set HVBAR
enter_hyp@ Check CPU nr again mrc p15, 0, r0, c0, c0, 5 @ MPIDR (ARMv7 only) and r0, r0, #15 @ CPU number@@ -125,6 +123,8 @@ start: ldr r1, [r0] cmp r1, #0 beq 1b
enter_hyp mov pc, r1 @ branch to the given address#endif
@@ -170,6 +170,17 @@ __semi_call: #endif mov pc, lr
+.globl __boot_kernel +__boot_kernel:
mov r4, r0stmfd sp!, {r1-r3}ldmia sp, {r0-r3}
Since the kernel only really needs 3 arguments, it would be less hassle to drop the 4th param to boot_kernel() and then have __boot_kernel() be passed r0,r1,r2,entrypoint.
enter_hypbx r4+.type __boot_kernel, %function
@ @ Data @diff --git a/semi_loader.h b/semi_loader.h index 6afba40..29f3d63 100644 --- a/semi_loader.h +++ b/semi_loader.h @@ -90,10 +90,12 @@ struct loader_info {
void load_kernel(struct loader_info *info);
+void __boot_kernel(unsigned entry_point,
unsigned r0, unsigned r1, unsigned r2, unsigned r3);static void boot_kernel(struct loader_info *info, unsigned r0, unsigned r1, unsigned r2, unsigned r3) {
((void (*)(unsigned, unsigned, unsigned, unsigned))info->kernel_entry)(r0, r1, r2, r3);
__boot_kernel(info->kernel_entry, r0, r1, r2, r3);}
#endif /* ! SEMI_LOADER_H */
1.7.4.1
On 26 September 2012 14:37, Peter Maydell peter.maydell@linaro.org wrote:
On 6 September 2012 18:12, Dave Martin dave.martin@linaro.org wrote:
Signed-off-by: Dave Martin dave.martin@linaro.org
This patch causes the kernel to get stuck at "Calibrating delay loop...".
I'm not sure why this happens (investigating) but if I move the enter_hyp macro call back to where it used to be (leaving the rest of the patch intact) it boots OK...
...hmm, no, seems to be a false alarm. I must not have recompiled properly or something, it works OK now. Sorry for the noise.
-- PMM
On 26 September 2012 15:12, Peter Maydell peter.maydell@linaro.org wrote:
On 26 September 2012 14:37, Peter Maydell peter.maydell@linaro.org wrote:
On 6 September 2012 18:12, Dave Martin dave.martin@linaro.org wrote:
Signed-off-by: Dave Martin dave.martin@linaro.org
This patch causes the kernel to get stuck at "Calibrating delay loop...".
I'm not sure why this happens (investigating) but if I move the enter_hyp macro call back to where it used to be (leaving the rest of the patch intact) it boots OK...
...hmm, no, seems to be a false alarm. I must not have recompiled properly or something, it works OK now. Sorry for the noise.
I was right the first time. The model runs really slowly because CPU1+ have run off into the weeds. The patch has added an 'enter_hyp' call into the chunk of code which is relocated to some random address, which means the code is now too long and we only relocate half of it. So when the primary CPU wakes up the secondaries they just run off into the weeds. Solution: move the enter_hyp to before the secondary-CPU pen code rather than after it.
Unless anybody objects I propose to commit these three bootwrapper patches (with the above one-line fix) tomorrow.
-- PMM
On Wed, 2012-09-26 at 16:01 +0100, Peter Maydell wrote:
On 26 September 2012 15:12, Peter Maydell peter.maydell@linaro.org wrote:
On 26 September 2012 14:37, Peter Maydell peter.maydell@linaro.org wrote:
On 6 September 2012 18:12, Dave Martin dave.martin@linaro.org wrote:
Signed-off-by: Dave Martin dave.martin@linaro.org
This patch causes the kernel to get stuck at "Calibrating delay loop...".
I'm not sure why this happens (investigating) but if I move the enter_hyp macro call back to where it used to be (leaving the rest of the patch intact) it boots OK...
...hmm, no, seems to be a false alarm. I must not have recompiled properly or something, it works OK now. Sorry for the noise.
I was right the first time. The model runs really slowly because CPU1+ have run off into the weeds.
I saw something like that when I tried this patch. I was just about going to get back to looking at this after being sidetracked on more urgent issues, but I see you have solved this particular issue...
The patch has added an 'enter_hyp' call into the chunk of code which is relocated to some random address, which means the code is now too long and we only relocate half of it. So when the primary CPU wakes up the secondaries they just run off into the weeds. Solution: move the enter_hyp to before the secondary-CPU pen code rather than after it.
That analysis sounds reasonable to me and I have successfully tested this modified solution on an A15x4 model. And also on A15x4-A7x4 after first applying the CCI hack to get bit.LITTLE working. To avoid ambiguity about the code I tested, I applied the following diff on top of Dave's v2 patches...
---------------------------------------------------------------------- diff --git a/boot.S b/boot.S index fe7acdf..cd2c09d 100644 --- a/boot.S +++ b/boot.S @@ -106,6 +106,8 @@ start: @ Secondary CPUs (following the RealView SMP booting protocol) @
+ enter_hyp + ldr r1, =fs_start - 0x100 adr r2, 1f ldmia r2, {r3 - r7} @ move the code to a location @@ -124,7 +126,6 @@ start: cmp r1, #0 beq 1b
- enter_hyp mov pc, r1 @ branch to the given address #endif ----------------------------------------------------------------------
The further good news is that after this patchset, enabling the CCI works OK when done just before entering the kernel. This proves that we'll be able to implement functionality in the C code to conditionally setup CCI based on device-tree contents.
On 26 September 2012 18:08, Jon Medhurst (Tixy) tixy@linaro.org wrote:
On Wed, 2012-09-26 at 16:01 +0100, Peter Maydell wrote:
The patch has added an 'enter_hyp' call into the chunk of code which is relocated to some random address, which means the code is now too long and we only relocate half of it. So when the primary CPU wakes up the secondaries they just run off into the weeds. Solution: move the enter_hyp to before the secondary-CPU pen code rather than after it.
That analysis sounds reasonable to me and I have successfully tested this modified solution on an A15x4 model. And also on A15x4-A7x4 after first applying the CCI hack to get bit.LITTLE working.
Thanks for the testing -- I have now pushed this patchset to git://git.linaro.org/arm/models/boot-wrapper.git master .
-- PMM
On Thu, Sep 27, 2012 at 01:22:14PM +0100, Peter Maydell wrote:
On 26 September 2012 18:08, Jon Medhurst (Tixy) tixy@linaro.org wrote:
On Wed, 2012-09-26 at 16:01 +0100, Peter Maydell wrote:
The patch has added an 'enter_hyp' call into the chunk of code which is relocated to some random address, which means the code is now too long and we only relocate half of it. So when the primary CPU wakes up the secondaries they just run off into the weeds. Solution: move the enter_hyp to before the secondary-CPU pen code rather than after it.
That analysis sounds reasonable to me and I have successfully tested this modified solution on an A15x4 model. And also on A15x4-A7x4 after first applying the CCI hack to get bit.LITTLE working.
Thanks for the testing -- I have now pushed this patchset to git://git.linaro.org/arm/models/boot-wrapper.git master .
OK, I had an item on my todo list to rehabilitate those patches, so I can't complain about someone else doing it...
Thanks all
---Dave
-
Dave Martin -
Jon Medhurst (Tixy) -
Peter Maydell