Re: [PATCH v7 08/16] KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing

[Dave Martin <Dave.Martin@arm.com>]

On Wed, May 09, 2018 at 05:54:25PM +0100, Marc Zyngier wrote:
> On 09/05/18 17:12, 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.
> > 
> > Four hooks are defined in order to enable this:
> > 
> >  * kvm_arch_vcpu_run_map_fp():
> >    Called on PID change to map necessary bits of current to Hyp.
> > 
> >  * kvm_arch_vcpu_load_fp():
> >    Set up FP/SIMD for entering the KVM run loop (parse as
> >    "vcpu_load fp").
> > 
> >  * kvm_arch_vcpu_ctxsync_fp():
> >    Get FP/SIMD into a safe state for re-enabling interrupts after a
> >    guest exit back to the run loop.
> > 
> >    For arm64 specifically, this involves updating the host kernel's
> >    FPSIMD context tracking metadata so that kernel-mode NEON use
> >    will cause the vcpu's FPSIMD state to be saved back correctly
> >    into the vcpu struct.  This must be done before re-enabling
> >    interrupts because kernel-mode NEON may be used by softirqs.
> > 
> >  * kvm_arch_vcpu_put_fp():
> >    Save guest FP/SIMD state back to memory and dissociate from the
> >    CPU ("vcpu_put fp").
> > 
> > Also, the arm64 FPSIMD context switch code is updated to enable it
> > to save back FPSIMD state for a vcpu, not just current.  A few
> > helpers drive this:
> > 
> >  * fpsimd_bind_state_to_cpu(struct user_fpsimd_state *fp):
> >    mark this CPU as having context fp (which may belong to a vcpu)
> >    currently loaded in its registers.  This is the non-task
> >    equivalent of the static function fpsimd_bind_to_cpu() in
> >    fpsimd.c.
> > 
> >  * task_fpsimd_save():
> >    exported to allow KVM to save the guest's FPSIMD state back to
> >    memory on exit from the run loop.
> > 
> >  * fpsimd_flush_state():
> >    invalidate any context's FPSIMD state that is currently loaded.
> >    Used to disassociate the vcpu from the CPU regs on run loop exit.
> > 
> > These changes allow the run loop to enable interrupts (and thus
> > softirqs that may use kernel-mode NEON) without having to save the
> > guest's FPSIMD state eagerly.
> > 
> > Some new vcpu_arch fields are added to make all this work.  Because
> > host FPSIMD state can now be saved back directly into current's
> > thread_struct as appropriate, host_cpu_context is no longer used
> > for preserving the FPSIMD state.  However, it is still needed for
> > preserving other things such as the host's system registers.  To
> > avoid ABI churn, the redundant storage space in host_cpu_context is
> > not removed for now.
> > 
> > arch/arm is not addressed by this patch and continues to use its
> > current save/restore logic.  It could provide implementations of
> > the helpers later if desired.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > 
> > ---
> > 
> > Dropped Reviewed-bys due to non-trivial changes.
> > 
> > Changes since v6:
> > 
> >  * Don't define kvm_arch_vcpu_run_pid_change() unless CONFIG_KVM=y.
> > 
> >    <asm/kvm_host.h> may be used for its declarations even with
> >    CONFIG_KVM=n (e.g., in asm-offsets.c).
> > 
> >    This patch avoids conflicts with the core headers in this config.
> > 
> >  * Rebind current's FP state to the cpu in vcpu_put() if it is
> >    still loaded, to ensure that the SVE trapping setup for userspace is
> >    properly restored.
> > 
> > Requested by Marc Zyngier:
> > 
> >  * Add a comment to explain the purpose of update_fp_enabled().
> > 
> >  * Migrate vcpu_arch.flags definitions to kvm_host.h.
> > 
> >  * Eliminate magic NULL semantics for vcpu_arch.host_fpsimd_state so
> >    that we can just assign this pointer once in the pid_change hook.
> > 
> >    A new flag KVM_ARM64_FP_HOST flag is added to capture the former
> >    semantics of vcpu->arch.host_fpsimd_state != NULL.
> Thanks for the additional rework, that looks much better now.
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

Cool, thanks for wrapping your head around it.

Cheers
---Dave