On Thu, Aug 2, 2012 at 1:14 AM, Antonios Motakis
<a.motakis@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> Hello,
>
> On Thu, Aug 2, 2012 at 12:12 AM, Christoffer Dall
> <c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> On Fri, Jul 27, 2012 at 1:10 AM, Antonios Motakis
>> <a.motakis@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>> > This patch applies to the v9 patch series of KVM,
>> > i.e. commit d5321dceeaccf756755e76b38d8b5905bd99d250
>> >
>> > In this patch, we use the Hyp Coprocessor Trap Register
>> > (HPCTR) to trap VFP/NEON instructions, and switch the FPU
>> > state at that point. After a guest exit, the VFP state is
>> > returned to the host. When disabling access to floating point
>> > instructions, we also mask FPEXC_EN in order to avoid the guest
>> > receiving Undefined instruction exceptions before we have a
>> > chance to switch back the floating point state.
>> >
>> > Initial lazy switching implementation provided by Rusty Russell.
>> >
>> > In this version of the patch, FPEXC_EN is no longer clobbered for
>> > the host; we save FPEXC before we mask it.
>> >
>> > We are reusing vfp_hard_struct, so now we depend on VFPv3 being
>> > enabled in the host kernel. For now, if the kernel has not been
>> > built with VFPv3, we don't build the VFP switching code in KVM,
>> > however we still trap cp10 and cp11 in order to inject an undefined
>> > instruction exception whenever the guest tries to use VFP/NEON.
>> >
>> > Alternatives to this would be to either require VFPv3 for KVM in
>> > the first place, or to implement this as a feature that can be switched
>> > on and off. If we don't mind the #ifdefs we can keep the current
>> > solution.
>> >
>> > For now I don't reuse vfp_save_state. If we still want to do that, we
>> > would have to change it a bit and also add a vfp_restore_state, as
>> > already mentioned by reviewers. However I have noticed that
>> > vfp_save_state
>> > doesn't clear FPEXC_EX before reading the rest of the registers, as
>> > we do according to the ARM ARM p. AppxF-2388. Can someone comment on
>> > whether this is a bug in vfp_save_state?
>> >
>> > Changes since v2:
>> > * Fix clobbering the host's FPEXC_EN on entry
>> > * Reuse vfp_hard_struct from asm/fpstate.h
>> > * Reuse macros from asm/vfpmacros.h (warning: out of date mnemonics)
>> > * Disable VFP support if kernel hasn't been cofigured with VFPv3
>> > * Tweak and reuse set_hcptr instead of setting the HCPTR manually
>> >
>> > Changes since v1:
>> > * Lazy switching by Rusty Russell
>> > * Lazy switching fix; mask FPEXC_EN so guest processes don't crash
>> > * Simpler lazy switching handler
>> > * Replaced instruction mnemonics with coprocessor operations so the
>> > code can be built on gcc configurations with no full VFP-d32 support
>> > * VFP subarchitecture handling fix; disable FPEXC_EX before switching
>> > cp11 registers
>> >
>> > Signed-off-by: Rusty Russell <rusty.russell@xxxxxxxxxx>
>> > Signed-off-by: Antonios Motakis <a.motakis@xxxxxxxxxxxxxxxxxxxxxx>
>> > ---
>> > arch/arm/include/asm/kvm_host.h | 6 +++
>> > arch/arm/kernel/asm-offsets.c | 2 +
>> > arch/arm/kvm/interrupts.S | 104
>> > ++++++++++++++++++++++++++++++++++++---
>> > 3 files changed, 104 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/arch/arm/include/asm/kvm_host.h
>> > b/arch/arm/include/asm/kvm_host.h
>> > index 0c7e782..63d5cc1 100644
>> > --- a/arch/arm/include/asm/kvm_host.h
>> > +++ b/arch/arm/include/asm/kvm_host.h
>> > @@ -19,6 +19,8 @@
>> > #ifndef __ARM_KVM_HOST_H__
>> > #define __ARM_KVM_HOST_H__
>> >
>> > +#include <asm/fpstate.h>
>> > +
>> > #define KVM_MAX_VCPUS 4
>> > #define KVM_MEMORY_SLOTS 32
>> > #define KVM_PRIVATE_MEM_SLOTS 4
>> > @@ -103,6 +105,10 @@ struct kvm_vcpu_arch {
>> > /* System control coprocessor (cp15) */
>> > u32 cp15[nr_cp15_regs];
>> >
>> > + /* Floating point registers (VFP and Advanced SIMD/NEON) */
>> > + struct vfp_hard_struct vfp_guest;
>> > + struct vfp_hard_struct vfp_host;
>> > +
>> > /* Exception Information */
>> > u32 hsr; /* Hyp Syndrom Register */
>> > u32 hdfar; /* Hyp Data Fault Address Register */
>> > diff --git a/arch/arm/kernel/asm-offsets.c
>> > b/arch/arm/kernel/asm-offsets.c
>> > index 9c76b53..d9ed793 100644
>> > --- a/arch/arm/kernel/asm-offsets.c
>> > +++ b/arch/arm/kernel/asm-offsets.c
>> > @@ -168,6 +168,8 @@ int main(void)
>> > DEFINE(VCPU_TID_URW, offsetof(struct kvm_vcpu,
>> > arch.cp15[c13_TID_URW]));
>> > DEFINE(VCPU_TID_URO, offsetof(struct kvm_vcpu,
>> > arch.cp15[c13_TID_URO]));
>> > DEFINE(VCPU_TID_PRIV, offsetof(struct kvm_vcpu,
>> > arch.cp15[c13_TID_PRIV]));
>> > + DEFINE(VCPU_VFP_GUEST, offsetof(struct kvm_vcpu,
>> > arch.vfp_guest));
>> > + DEFINE(VCPU_VFP_HOST, offsetof(struct kvm_vcpu,
>> > arch.vfp_host));
>> > DEFINE(VCPU_REGS, offsetof(struct kvm_vcpu, arch.regs));
>> > DEFINE(VCPU_USR_REGS, offsetof(struct kvm_vcpu,
>> > arch.regs.usr_regs));
>> > DEFINE(VCPU_SVC_REGS, offsetof(struct kvm_vcpu,
>> > arch.regs.svc_regs));
>> > diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
>> > index fd7331c..3159ce7 100644
>> > --- a/arch/arm/kvm/interrupts.S
>> > +++ b/arch/arm/kvm/interrupts.S
>> > @@ -23,6 +23,7 @@
>> > #include <asm/asm-offsets.h>
>> > #include <asm/kvm_asm.h>
>> > #include <asm/kvm_arm.h>
>> > +#include <asm/vfpmacros.h>
>> >
>> > #define VCPU_USR_REG(_reg_nr) (VCPU_USR_REGS + (_reg_nr * 4))
>> > #define VCPU_USR_SP (VCPU_USR_REG(13))
>> > @@ -89,6 +90,49 @@ ENTRY(__kvm_flush_vm_context)
>> > bx lr
>> > ENDPROC(__kvm_flush_vm_context)
>> >
>> > +/* Clobbers {r2-r7} */
>> > +.macro store_vfp_state vfp_off, vcpup
>> > + add r6, \vcpup, \vfp_off
>> > +
>> > + @ The VFPFMRX and VFPFMXR macros are the VMRS and VMSR
>> > instructions
>> > + vfpfmrx r2, FPEXC
>>
>> nit: since they're macros and to be consistent, let's use VFPFMRX instead
>>
>
> Ack.
>
>>
>> > + @ Make sure VFP is enabled so we can touch the registers.
>> > + orr r7, r2, #FPEXC_EN
>> > + vfpfmxr FPEXC, r7
>> > +
>> > + vfpfmrx r3, FPSCR
>> > + tst r2, #FPEXC_EX @ Check for VFP Subarchitecture
>>
>> I think you need to first write 1 to the EX bit, which might mess with
>> the state, since the host could have written 0 there beforehand, in
>> which case it should still read-back as zero. You may want to check
>> all this before hand (it doesn't change once we're on certain
>> hardware).
>
>
> According to the VFP subarchitecture the EX bit needs to be cleared to 0,
> after saving the subarchitecture information. The version we store is in r2,
> which is the value stored before we meddle with it, which is the one the
> guest expects. In practice we can't really test this code path, because I've
> never seen this bit enabled in the Fast Models; the subarchitecture is maybe
> not even implemented. Interestingly the Linux kernel itself does not handle
> this at all in its own context switch code.
>
eh, on page AppxF-2394 it says:
Set FPEXC.EX=1 and FPEXC.FP2V=1
Read back the FPEXC register
if FPEXC.EX == 0 then
Neither FPINST nor FPINST2 are implemented
...
>>
>>
>> You could also make the operations execute conditionally and avoid the
>> potential pipeline flushes for mispredicted branches...
>
>
> Ack. We can do the FPINST2 operation conditional, however the first one
> skips a number of instructions so maybe it's cleaner to just branch
>
>>
>>
>> Additionally you can consider only performing the store of FPINST and
>> FPINST2 conditionally as well, not sure about the performance tradeoff
>> vs. code size though.
>>
>
> If EX is 0, then we also need to skip checking FP2V.
>
true
>>
>> > + beq 1f
>> > + vfpfmrx r4, FPINST
>> > + tst r2, #FPEXC_FP2V
>> > + beq 2f
>> > + vfpfmrx r5, FPINST2
>> > +2:
>> > + bic r7, r2, #FPEXC_EX @ FPEXC_EX disable
>> > + vfpfmxr FPEXC, r7
>> > +1:
>> > + vfpfstmia r6, r7 @ Save VFP registers
>> > + stm r6, {r2-r5} @ Save FPEXC, FPSCR, FPINST,
>> > FPINST2
>> > +.endm
>> > +
>> > +/* Assume FPEXC_EN is on and FPEXC_EX is off, clobbers {r2-r7} */
>> > +.macro restore_vfp_state vfp_off, vcpup
>> > + add r6, \vcpup, \vfp_off
>> > +
>> > + vfpfldmia r6, r7 @ Load VFP registers
>> > + ldm r6, {r2-r5} @ Load FPEXC, FPSCR, FPINST,
>> > FPINST2
>> > +
>> > + vfpfmxr FPSCR, r3
>> > + tst r2, #FPEXC_EX @ Check for VFP Subarchitecture
>> > + beq 1f
>> > + vfpfmxr FPINST, r4
>> > + tst r2, #FPEXC_FP2V
>> > + beq 1f
>>
>> again with setting EX bit first here, so you probably want to check
>> another pre-stored value, as mentioned above.
>>
>> also I would like to reduce the number of branches here by using
>> conditionals.
>>
>> > + vfpfmxr FPINST2, r5
>> > +1:
>> > + vfpfmxr FPEXC, r2 @ FPEXC (last, in case !EN)
>> > +.endm
>> > +
>> > @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>> > @ Hypervisor world-switch code
>> > @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>> > @@ -281,16 +325,16 @@ ENDPROC(__kvm_flush_vm_context)
>> > .endm
>> >
>> > /* Configures the HCPTR (Hyp Coprocessor Trap Register) on entry/return
>> > - * (hardware reset value is 0) */
>> > -.macro set_hcptr entry
>> > + * (hardware reset value is 0). Keep previous value in r2. */
>> > +.macro set_hcptr entry, mask
>> > mrc p15, 4, r2, c1, c1, 2
>> > - ldr r3, =(HCPTR_TTA)
>> > + ldr r3, =\mask
>> > .if \entry == 1
>> > - orr r2, r2, r3 @ Trap some coproc-accesses
>> > + orr r3, r2, r3 @ Trap some coproc-accesses
>> > .else
>> > - bic r2, r2, r3 @ Don't trap any coproc-
>> > accesses
>> > + bic r3, r2, r3 @ Don't trap any coproc-
>> > accesses
>>
>> the comments should be changed to something like:
>>
>> Trap coproc-accesses
>>
>> and
>>
>> Don't trap coproc accesses
>>
>> the "any" part is clearly wrong now.
>
>
> Ack.
>
>>
>>
>> > .endif
>> > - mcr p15, 4, r2, c1, c1, 2
>> > + mcr p15, 4, r3, c1, c1, 2
>> > .endm
>> >
>> > /* Enable/Disable: stage-2 trans., trap interrupts, trap wfi, trap smc
>> > */
>> > @@ -328,6 +372,14 @@ ENTRY(__kvm_vcpu_run)
>> > read_cp15_state
>> > write_cp15_state 1, r0
>> >
>> > +#ifdef CONFIG_VFPv3
>>
>> maybe a comment here about the discussion for CONFIG_VFPv3, this
>> define seems oddly out of place if you didn't follow the e-mail
>> thread.
>
>
> Ack.
>
>>
>>
>> > + @ Mask FPEXC_EN so the guest doesn't trap floating point
>> > instructions
>>
>> s/Mask/Set/ ?
>
>
> Ack
>
>>
>>
>>
>> > + vfpfmrx r2, FPEXC @ VMRS
>> > + push {r2}
>> > + orr r2, r2, #FPEXC_EN
>> > + vfpfmxr FPEXC, r2 @ VMSR
>> > +#endif
>> > +
>> > push {r0} @ Push the VCPU pointer
>> >
>> > @ Configure Hyp-role
>> > @@ -335,7 +387,7 @@ ENTRY(__kvm_vcpu_run)
>> >
>> > @ Trap coprocessor CRx accesses
>> > set_hstr 1
>> > - set_hcptr 1
>> > + set_hcptr 1, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11))
>> >
>> > @ Write configured ID register into MIDR alias
>> > ldr r1, [r0, #VCPU_MIDR]
>> > @@ -395,7 +447,22 @@ __kvm_vcpu_return:
>> >
>> > @ Don't trap coprocessor accesses for host kernel
>> > set_hstr 0
>> > - set_hcptr 0
>> > + set_hcptr 0, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11))
>> > +
>> > +#ifdef CONFIG_VFPv3
>> > + @ Save floating point registers we if let guest use them.
>> > + tst r2, #(HCPTR_TCP(10) | HCPTR_TCP(11))
>> > + bne after_vfp_restore
>>
>> nice!
>>
>> > +
>> > + @ Switch VFP/NEON hardware state to the host's
>> > + store_vfp_state #VCPU_VFP_GUEST, r1
>> > + restore_vfp_state #VCPU_VFP_HOST, r1
>> > +
>> > +after_vfp_restore:
>> > + @ Restore FPEXC_EN which we clobbered on entry
>> > + pop {r2}
>> > + vfpfmxr FPEXC, r2
>> > +#endif
>> >
>> > @ Reset Hyp-role
>> > configure_hyp_role 0, r1
>> > @@ -587,6 +654,10 @@ hyp_hvc:
>> > @ Check syndrome register
>> > mrc p15, 4, r0, c5, c2, 0 @ HSR
>> > lsr r1, r0, #HSR_EC_SHIFT
>> > +#ifdef CONFIG_VFPv3
>> > + cmp r1, #HSR_EC_CP_0_13
>> > + beq switch_to_guest_vfp
>> > +#endif
>>
>> if we don't configure the host for VFPv3 and still have the guest use
>> VFPv3 (regardless of whether it exists in hardware or not), does the
>> flow happen as expected with an undefined exception into the guest
>> through handle_exit? If this is as it should be, perhaps a comment is
>> warranted?
>
>
> Yes; when VFPv3 is off, then we don't compile any of the new code, however
> we still trap CP10/11 so it will be handled by handled_exit, which leads to
> an Undef exception (or we could emulate it by software in the future, etc).
> I'll add a comment to clear this up.
>
what would you emulate in software, the VFP operations? Let's not do that.
>>
>>
>> I am afraid, however, that this might no go very well, since the guest
>> may examine the faulting instruction and decide that it was an
>> undefined exception on some VFP operation and assume it can emulate
>> that and that may go into some weird loop?
>
>
> The Media and VFP Feature registers that tell to the guest whether VFP or
> NEON is implemented, are also part of the cp10 coprocessor registers, which
> we trap. So a proper guest should conclude that there is no hardware
> floating point support, and should expect an Undef for all VFP instructions
> and work around that. In any case that is how the real hardware would behave
> without floating point support.
>
what happens if the guest tries to set the FPEXC.EN bit without VFP3
support? Does it get an undefined exception from us and is that
equivalent to what would happen on real hardware?
> Though I don't know if the Linux kernel can fallback to *completely*
> emulating the full VFP or NEON instruction set. I think guests that don't
> outright assume hardware floating point are not very common these days.
>
in any case, not our concern.
>>
>>
>> > cmp r1, #HSR_EC_HVC
>> > bne guest_trap @ Not HVC instr.
>> >
>> > @@ -661,6 +732,23 @@ guest_trap:
>> > 1: mov r0, #ARM_EXCEPTION_HVC
>> > b __kvm_vcpu_return
>> >
>> > +#ifdef CONFIG_VFPv3
>> > +switch_to_guest_vfp:
>> > + ldr r0, [sp, #12] @ Load VCPU pointer
>> > + push {r3-r7}
>> > +
>> > + @ NEON/VFP used. Turn on VFP access.
>> > + set_hcptr 0, (HCPTR_TCP(10) | HCPTR_TCP(11))
>> > +
>> > + @ Switch VFP/NEON hardware state to the guest's
>> > + store_vfp_state #VCPU_VFP_HOST, r0
>> > + restore_vfp_state #VCPU_VFP_GUEST, r0
>> > +
>> > + pop {r3-r7}
>> > + pop {r0-r2}
>> > + eret
>> > +#endif
>> > +
>> > .align
>> > hyp_irq:
>> > push {r0}
>>
>> When compiling the v10-stage kernel in ARM mode with this patch applied, I
>> get:
>>
>> arch/arm/kvm/interrupts.S: Assembler messages:
>> arch/arm/kvm/interrupts.S:691: Error: invalid constant (438) after fixup
>> arch/arm/kvm/interrupts.S:697: Error: invalid constant (424) after fixup
>> make[1]: *** [arch/arm/kvm/interrupts.o] Error 1
>
>
> Ok, I will try to reproduce this on v10-stage. Thanks.
>
>>
>>
>> The change is pretty significant to the size and locations of the
>> kvm_vcpu_arch struct, so you may want to test for both ARM/Thumb2
>> kernel.
>>
>
> I will also test a Thumb2 kernel then to make sure, thanks for the tip.
>
>>
>> Otherwise, besides my small comments, looks pretty good to me.
>>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
