On Fri, May 31, 2013 at 11:34:38AM +0200, Andre Przywara wrote:
> On 05/31/2013 07:43 AM, Christoffer Dall wrote:
> >On Mon, May 06, 2013 at 03:17:49PM +0200, Andre Przywara wrote:
> >>For the KVM and XEN hypervisors to be usable, we need to enter the
> >>kernel in HYP mode. Now that we already are in non-secure state,
> >>HYP mode switching is within short reach.
> >>
> >>While doing the non-secure switch, we have to enable the HVC
> >>instruction and setup the HYP mode HVBAR (while still secure).
> >>
> >>The actual switch is done by dropping back from a HYP mode handler
> >>without actually leaving HYP mode, so we introduce a new handler
> >>routine in the exception vector table.
> >>
> >>In the assembly switching routine - which we rename to hyp_gic_switch
> >>on the way - we save and restore the banked LR and SP registers
> >>around the hypercall to do the actual HYP mode switch.
> >>
> >>The C routine first checks whether we are in HYP mode already and
> >>also whether the virtualization extensions are available. It also
> >>checks whether the HYP mode switch was finally successful.
> >>The bootm command part only adds and adjusts some error reporting.
> >>
> >>Signed-off-by: Andre Przywara <andre.przyw...@linaro.org>
> >>---
> >> arch/arm/cpu/armv7/start.S | 34 +++++++++++++++++++++++-----------
> >> arch/arm/include/asm/armv7.h | 4 ++--
> >> arch/arm/lib/bootm.c | 12 +++++++++---
> >> arch/arm/lib/virt-v7.c | 22 +++++++++++++++-------
> >> 4 files changed, 49 insertions(+), 23 deletions(-)
> >>
> >>diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> >>index 02234c7..921e9d9 100644
> >>--- a/arch/arm/cpu/armv7/start.S
> >>+++ b/arch/arm/cpu/armv7/start.S
> >>@@ -41,7 +41,7 @@ _start: b reset
> >> ldr pc, _software_interrupt
> >> ldr pc, _prefetch_abort
> >> ldr pc, _data_abort
> >>- ldr pc, _not_used
> >>+ ldr pc, _hyp_trap
> >> ldr pc, _irq
> >> ldr pc, _fiq
> >> #ifdef CONFIG_SPL_BUILD
> >>@@ -49,7 +49,7 @@ _undefined_instruction: .word _undefined_instruction
> >> _software_interrupt: .word _software_interrupt
> >> _prefetch_abort: .word _prefetch_abort
> >> _data_abort: .word _data_abort
> >>-_not_used: .word _not_used
> >>+_hyp_trap: .word _hyp_trap
> >> _irq: .word _irq
> >> _fiq: .word _fiq
> >> _pad: .word 0x12345678 /* now 16*4=64 */
> >>@@ -58,7 +58,7 @@ _undefined_instruction: .word undefined_instruction
> >> _software_interrupt: .word software_interrupt
> >> _prefetch_abort: .word prefetch_abort
> >> _data_abort: .word data_abort
> >>-_not_used: .word not_used
> >>+_hyp_trap: .word hyp_trap
> >> _irq: .word irq
> >> _fiq: .word fiq
> >> _pad: .word 0x12345678 /* now 16*4=64 */
> >>@@ -513,12 +513,18 @@ software_interrupt:
> >> mrc p15, 0, r1, c1, c1, 0 @ read SCR
> >> bic r1, r1, #0x07f
> >> orr r1, r1, #0x31 @ enable NS, AW, FW
> >>+ mrc p15, 0, r0, c0, c1, 1 @ check for Virt ext
> >>+ and r0, r0, #0xf000
> >>+ cmp r0, #0x1000
> >
> >you can just do ands r0, r0 ,#0xf000 and change the orreq below to orrne
>
> Can I? But I want to make sure that Virt ext is of version 1 only,
> anything beyond that I cannot reliably support, right? Or is there a
> guarantee that this is backwards compatible?
In the ARMv7 specifications it mentions either 1 or 0 as the possible
values of this field, and doesn't say anything about reserved values
iirc, but maybe that shouldn't be taken that literally. Just keep the
code as it is then.
>
> >>+ orreq r1, r1, #0x100 @ allow HVC instruction
> >>
> >> 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
> >> isb
> >> mcr p15, 0, r0, c12, c0, 0 @ write non-secure copy of VBAR
> >>
> >>+ mcreq p15, 4, r0, c12, c0, 0 @ write HYP mode HVBAR
> >>+
> >
> >nit: s/HYP mode//
> >
> >> movs pc, lr
> >>
> >> .align 5
> >>@@ -534,10 +540,9 @@ data_abort:
> >> bl do_data_abort
> >>
> >> .align 5
> >>-not_used:
> >>- get_bad_stack
> >>- bad_save_user_regs
> >>- bl do_not_used
> >>+hyp_trap:
> >>+ .byte 0x00, 0xe3, 0x0e, 0xe1 @ mrs lr, elr_hyp
> >
> >do we really need to support this on assemblers that old?
>
> What do you mean with old? I can check again, but I think it didn't
> work with my self-compiled binutils 2.23. Or do I need a directive
> to enable this?
>
You need a directive, see the Makefile in arch/arm/kvm/Makefile and the
corresponding source files in the kernel.
> >>+ mov pc, lr
> >>
> >> #ifdef CONFIG_USE_IRQ
> >>
> >>@@ -574,21 +579,21 @@ fiq:
> >> #endif /* CONFIG_SPL_BUILD */
> >>
> >> #ifdef CONFIG_ARMV7_VIRT
> >>-/* Routine to initialize GIC CPU interface and switch to nonsecure state.
> >>- * Will be executed directly by secondary CPUs after coming out of
> >>+/* Routine to initialize GIC CPU interface, switch to nonsecure and to HYP
> >>+ * mode. Will be executed directly by secondary CPUs after coming out of
> >
> >So now this routine does three different things in different context at
> >once, why?
>
> Not three. At most two. But actually it does only one thing: switch
> a single core to HYP mode. Only the entry and exit points are
> different.
> I just see that I could make the smp_pen a separate function,
> calling the switch function. This would also solve this "LR abuse"
> thing.
> Will try this.
>
1: smp_pen
2: switch from secure to non-secure
3: switch from non-secure svc to hyp
but yes, I think we understand each other at this point.
> >> * WFI, or can be called directly by C code for CPU 0.
> >> * Those two paths mandate to not use any stack and to only use registers
> >> * r0-r3 to comply with both the C ABI and the requirement of SMP startup
> >> * code.
> >> */
> >>-.globl _nonsec_gic_switch
> >>+.globl _hyp_gic_switch
> >> .globl _smp_pen
> >> _smp_pen:
> >> mrs r0, cpsr
> >> orr r0, r0, #0xc0
> >> msr cpsr, r0 @ disable interrupts
> >> mov lr, #0 @ clear LR to mark secondary
> >>-_nonsec_gic_switch:
> >>+_hyp_gic_switch:
> >> mrc p15, 4, r2, c15, c0, 0 @ r2 = PERIPHBASE
> >> add r3, r2, #0x1000 @ GIC dist i/f offset
> >> mvn r1, #0
> >>@@ -628,6 +633,13 @@ _nonsec_gic_switch:
> >> add r2, r2, #0x1000 @ GIC dist i/f offset
> >> str r1, [r2] @ allow private interrupts
> >>
> >>+ mov r2, lr
> >>+ mov r1, sp
> >>+ .byte 0x70, 0x00, 0x40, 0xe1 @ hvc #0
> >>+ isb
> >
> >again, I'm doubtful this isb is necessary when you just did an exception
> >return.
>
> Good point. Exception returns should do this automatically, right?
> Is this an official fact or just a feeling?
It's official, and I've been reminded numerous times by the ARM people
reviewing my KVM code :) It's somewhere in the ARM ARM.
>
> ...
>
> >>diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
> >>index 0248010..3883463 100644
> >>--- a/arch/arm/lib/virt-v7.c
> >>+++ b/arch/arm/lib/virt-v7.c
> >>@@ -3,6 +3,7 @@
> >> * Andre Przywara, Linaro
> >> *
> >> * routines to push ARMv7 processors from secure into non-secure state
> >>+ * and from non-secure SVC into HYP mode
> >> * needed to enable ARMv7 virtualization for current hypervisors
> >> *
> >> * See file CREDITS for list of people who contributed to this
> >>@@ -43,16 +44,20 @@ static inline unsigned int read_cpsr(void)
> >> return reg;
> >> }
> >>
> >>-int armv7_switch_nonsec(void)
> >>+int armv7_switch_hyp(void)
> >> {
> >> unsigned int reg;
> >> volatile unsigned int *gicdptr;
> >> unsigned itlinesnr, i;
> >> unsigned int *sysflags;
> >>
> >>- /* check whether the CPU supports the security extensions */
> >>+ /* check whether we are in HYP mode already */
> >>+ if ((read_cpsr() & 0x1F) == 0x1a)
> >>+ return 1;
> >>+
> >>+ /* check whether the CPU supports the virtualization extensions */
> >> asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> >>- if ((reg & 0xF0) == 0)
> >>+ if ((reg & 0xF000) != 0x1000)
> >> return 2;
> >>
> >> /* the timer frequency for the generic timer needs to be
> >>@@ -73,8 +78,8 @@ int armv7_switch_nonsec(void)
> >> */
> >>
> >> /* check whether we are an Cortex-A15 or A7.
> >>- * The actual non-secure switch should work with all CPUs supporting
> >>- * the security extension, but we need the GIC address,
> >>+ * The actual HYP switch should work with all CPUs supporting
> >>+ * the virtualization extension, but we need the GIC address,
> >> * which we know only for sure for those two CPUs.
> >> */
> >> asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(reg));
> >>@@ -113,8 +118,11 @@ int armv7_switch_nonsec(void)
> >> sysflags[0] = (uintptr_t)_smp_pen;
> >> gicdptr[GICD_SGIR / 4] = 1U << 24;
> >>
> >>- /* call the non-sec switching code on this CPU also */
> >>- _nonsec_gic_switch();
> >>+ /* call the HYP switching code on this CPU also */
> >>+ _hyp_gic_switch();
> >>+
> >>+ if ((read_cpsr() & 0x1F) != 0x1a)
> >>+ return 5;
> >
> >this is really a fatal crash right? We probably don't want to try and
> >proceed with boot at this point.
>
> Well not really. If we reach this point without being in HYP mode,
> that just hasn't worked - for various reasons. Since this new bootm
> functionality is unconditional, I felt we should better not crash.
> Actually Linux will run fine, just without KVM (for certains
> meanings of "fine" that is - as a KVM developer ;-)
>
Really, but we checked all the prerequisites before calling
_hyp_gic_switch, so we really expect it to work. If it didn't, it means
that something fatally bad happened as far as I can tell.
As for the integration with the bootm command, you already can't enable
CONFIG_ARMV7_VIRT on a board that's not booted in the secure world, and
you probably can never support determinig that at run-time. Perhaps
things should look like this:
#ifdef CONFIG_ARMV7_SECURE_BOOT
... do all the non-secure boot stuff
#endif
#ifdef CONFIG_ARMV7_CHANGE_TO_HYP
... call board-specific go-to-hyp-function
... if no board-specific function, exists, tell the user
#endif
But it's just a suggestion, the U-boot guys would know how they want
this I suppose.
Thanks,
-Christoffer
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
