On Mon, Apr 09, 2018 at 11:22:43PM +0200, Christoffer Dall wrote:
> Hi Dave,
>
> On Mon, Apr 09, 2018 at 11:53:02AM +0100, Dave Martin wrote:
> > This patch refactors KVM to align the host and guest FPSIMD
> > save/restore logic with each other for arm64. This reduces the
> > number of redundant save/restore operations that must occur, and
> > reduces the common-case IRQ blackout time during guest exit storms
> > by saving the host state lazily and optimising away the need to
> > restore the host state before returning to the run loop.
> >
>
> [...]
>
> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> > index db08a54..74c5a46 100644
> > --- a/arch/arm64/kernel/fpsimd.c
> > +++ b/arch/arm64/kernel/fpsimd.c
>
> [...]
>
> > @@ -1054,15 +1066,20 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state)
> > local_bh_enable();
> > }
> >
> > +void fpsimd_flush_state(unsigned int *cpu)
>
> This API looks strange to me, and doesn't seem to be called from
> elsewhere. Wouldn't it be more clear if it took a struct thread_struct
> pointer instead, or if the logic remained embedded in
> fpsimd_flush_task_state ?
Hmmm, thanks for spotting this -- it's a throwback to my previous
approach.
I had intended to align KVM fully with the way host tasks' context is
tracked, and this would involve a "most recent cpu FPSIMD loaded on"
field in struct vcpu_arch: for ABI reasons this can't easily be tacked
onto the end of struct user_fpsimd_state, so it would be necessary for
it to be a separate field and passed to the relevant maintenance
functions as a separate parameter.
This approach would allow the vcpu FPSIMD state to remain in the regs
across a context switch without the need to reload it, but this also
means that some flushing/invalidation of this cached view of the state
would be needed around KVM_GET_ONE_REG etc. and at vcpu destruction
time. This function would be part of such a maintenance API.
For now though, this seemed like extra complexity for dubious benefit.
Unless you think it's worth pursuing this optimisation I should
probably get rid of this function. We can always bring this back
later if we choose.
> > +{
> > + *cpu = NR_CPUS;
> > +}
> > +
> > /*
> > * Invalidate live CPU copies of task t's FPSIMD state
> > */
> > void fpsimd_flush_task_state(struct task_struct *t)
> > {
> > - t->thread.fpsimd_cpu = NR_CPUS;
> > + fpsimd_flush_state(&t->thread.fpsimd_cpu);
> > }
> >
> > -static inline void fpsimd_flush_cpu_state(void)
> > +void fpsimd_flush_cpu_state(void)
> > {
> > __this_cpu_write(fpsimd_last_state.st, NULL);
> > }
>
> [...]
>
> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > index 8605e04..797b259 100644
> > --- a/arch/arm64/kvm/hyp/switch.c
> > +++ b/arch/arm64/kvm/hyp/switch.c
> > @@ -27,6 +27,7 @@
> > #include <asm/kvm_mmu.h>
> > #include <asm/fpsimd.h>
> > #include <asm/debug-monitors.h>
> > +#include <asm/thread_info.h>
> >
> > static bool __hyp_text __fpsimd_enabled_nvhe(void)
> > {
> > @@ -47,24 +48,40 @@ bool __hyp_text __fpsimd_enabled(void)
> > return __fpsimd_is_enabled()();
> > }
> >
> > -static void __hyp_text __activate_traps_vhe(void)
> > +static bool update_fp_enabled(struct kvm_vcpu *vcpu)
>
> I think this needs a __hyp_text in the unlikely case that this function
> is not inlined in the _nvhe caller by the compiler.
You're right. I'll add it.
> > +{
> > + if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE) {
> > + vcpu->arch.host_fpsimd_state = NULL;
> > + vcpu->arch.fp_enabled = false;
> > + }
>
> I'm not clear why the above logic can't go into kvm_arch_vcpu_load_fp
> and why we can't simply check TIF_FOREIGN_FPSTATE in __hyp_switch_fpsimd
> instead?
The situation can change in between _load_fp() and here, because of
kernel-mode NEON.
Also, we can't defer this check to __hyp_switch_fpsimd() because this is
the logic for deciding whether to re-enable the Hyp FPSIMD trap in the
first place.
Here's a scenario:
* We're on a second iteration of the run loop, with the vcpu state loaded:
* fp_enabled = true, TIF_FOREIGN_FPSTATE is clear,
executing in the host with irqs enabled.
* A softirq uses kernel-mode NEON:
* vcpu FPSIMD state is saved back to memory
* TIF_FOREIGN_FPSTATE now set
* CPU FPSIMD regs now contain garbage
* local_irq_disable(), and enter guest
* fp_enabled == true, but out of date:
* update_fp_enabled detects this condition by observing that
TIF_FOREIGN_FPSTATE is set and clearing fp_enabled.
* the (updated) value of fp_enabled determines that the FPSIMD trap
should be enabled
* __hyp_switch_fpsimd() saves no host state (because it was already
saved and anyway host_fpsimd_state is NULL)
* __hyp_switch_fpsimd() loads the guest state
Is there a way to simplify the code that doesn't break this?
>
> > +
> > + return vcpu->arch.fp_enabled;
> > +}
> > +
> > +static void __hyp_text __activate_traps_vhe(struct kvm_vcpu *vcpu)
> > {
> > u64 val;
> >
> > val = read_sysreg(cpacr_el1);
> > val |= CPACR_EL1_TTA;
> > - val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN);
> > + val &= ~CPACR_EL1_ZEN;
> > + if (!update_fp_enabled(vcpu))
> > + val &= ~CPACR_EL1_FPEN;
> > +
> > write_sysreg(val, cpacr_el1);
> >
> > write_sysreg(kvm_get_hyp_vector(), vbar_el1);
> > }
> >
> > -static void __hyp_text __activate_traps_nvhe(void)
> > +static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
> > {
> > u64 val;
> >
> > val = CPTR_EL2_DEFAULT;
> > - val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
> > + val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> > + if (!update_fp_enabled(vcpu))
> > + val |= CPTR_EL2_TFP;
> > +
> > write_sysreg(val, cptr_el2);
> > }
> >
>
> [...]
>
> Otherwise this approach looks quite good to me overall. Are you
> planning to add SVE support before removing the RFC from this series?
Yes :)
(I've been delaying that while we get the basic approach sorted out.)
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
