On Mon, Jul 06, 2015 at 10:54:46AM -0700, Mario Smarduch wrote:
> On 07/05/2015 12:34 PM, Christoffer Dall wrote:
> > 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.
>
> Yes again that's on a todo list.
> >
> > /*
> > * 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).
>
> I built this patch on top of previous one. But IMO this series
> is not ready for upstream yet.
>
> >
> >> 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.
>
> What if we used the simplified approach (as Marc mentioned) and
> let it run for quite a while and then move this series?
Sounds good, let's merge these things separately.
-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
