On Wed, Jun 24, 2015 at 08:30:27PM -0700, Mario Smarduch wrote:
> This patch implements the VFP context switch code called from vcpu_put in
> Host KVM. In addition it implements the logic to skip setting a VFP trap if one
> is not needed. Also resets the flag if Host KVM switched registers to trap new
> guest vfp accesses.
>
>
> Signed-off-by: Mario Smarduch <m.smarduch@xxxxxxxxxxx>
> ---
> arch/arm/kvm/interrupts.S | 49 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index 79caf79..0912edd 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -96,6 +96,21 @@ ENTRY(__kvm_flush_vm_context)
> bx lr
> ENDPROC(__kvm_flush_vm_context)
>
> +ENTRY(__kvm_restore_host_vfp_state)
> + push {r3-r7}
> +
> + mov r1, #0
> + str r1, [r0, #VCPU_VFP_SAVED]
> +
> + add r7, r0, #VCPU_VFP_GUEST
> + store_vfp_state r7
> + add r7, r0, #VCPU_VFP_HOST
> + ldr r7, [r7]
> + restore_vfp_state r7
> +
> + pop {r3-r7}
> + bx lr
> +ENDPROC(__kvm_restore_host_vfp_state)
it feels a bit strange to introduce this function here when I cannot see
how it's called.
At the very least, could you provide the C equivalent prototype in a
comment or specify what the input registers are? E.g.
/*
* void __kvm_restore_host_vfp_state(struct kvm_vcpu *vcpu);
*/
>
> /********************************************************************
> * Hypervisor world-switch code
> @@ -131,7 +146,13 @@ ENTRY(__kvm_vcpu_run)
>
> @ Trap coprocessor CRx accesses
> set_hstr vmentry
> +
> + ldr r1, [vcpu, #VCPU_VFP_SAVED]
> + cmp r1, #1
> + beq skip_guest_vfp_trap
> set_hcptr vmentry, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11))
> +skip_guest_vfp_trap:
> +
> set_hdcr vmentry
>
> @ Write configured ID register into MIDR alias
> @@ -173,18 +194,6 @@ __kvm_vcpu_return:
> set_hcptr vmexit, (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
> -
> - @ Switch VFP/NEON hardware state to the host's
> - add r7, vcpu, #VCPU_VFP_GUEST
> - store_vfp_state r7
> - add r7, vcpu, #VCPU_VFP_HOST
> - ldr r7, [r7]
> - restore_vfp_state r7
> -
> -after_vfp_restore:
> @ Restore FPEXC_EN which we clobbered on entry
> pop {r2}
> VFPFMXR FPEXC, r2
> @@ -363,10 +372,6 @@ hyp_hvc:
> @ Check syndrome register
> mrc p15, 4, r1, c5, c2, 0 @ HSR
> lsr r0, r1, #HSR_EC_SHIFT
> -#ifdef CONFIG_VFPv3
> - cmp r0, #HSR_EC_CP_0_13
> - beq switch_to_guest_vfp
> -#endif
> cmp r0, #HSR_EC_HVC
> bne guest_trap @ Not HVC instr.
>
> @@ -380,7 +385,10 @@ hyp_hvc:
> cmp r2, #0
> bne guest_trap @ Guest called HVC
>
> -host_switch_to_hyp:
> + /*
> + * Getting here means host called HVC, we shift parameters and branch
> + * to Hyp function.
> + */
not sure this comment change belongs in this patch (but the comment is
well-written).
> pop {r0, r1, r2}
>
> /* Check for __hyp_get_vectors */
> @@ -411,6 +419,10 @@ guest_trap:
>
> @ Check if we need the fault information
> lsr r1, r1, #HSR_EC_SHIFT
> +#ifdef CONFIG_VFPv3
> + cmp r1, #HSR_EC_CP_0_13
> + beq switch_to_guest_vfp
> +#endif
> cmp r1, #HSR_EC_IABT
> mrceq p15, 4, r2, c6, c0, 2 @ HIFAR
> beq 2f
> @@ -479,11 +491,12 @@ guest_trap:
> */
> #ifdef CONFIG_VFPv3
> switch_to_guest_vfp:
> - load_vcpu @ Load VCPU pointer to r0
> push {r3-r7}
>
> @ NEON/VFP used. Turn on VFP access.
> set_hcptr vmexit, (HCPTR_TCP(10) | HCPTR_TCP(11))
> + mov r1, #1
> + str r1, [vcpu, #VCPU_VFP_SAVED]
>
> @ Switch VFP/NEON hardware state to the guest's
> add r7, r0, #VCPU_VFP_HOST
> --
> 1.7.9.5
>
It would probably be easier to just rebase this on the previous series
and refer to that in the cover letter, but the approach here looks
otherwise right to me.
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
