[PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put

[Christoffer Dall]

On Wed, Feb 14, 2018 at 02:43:42PM +0000, Dave Martin wrote:
> [CC Ard, in case he has a view on how much we care about softirq NEON
> performance regressions ... and whether my suggestions make sense]
> 
> On Wed, Feb 14, 2018 at 11:15:54AM +0100, Christoffer Dall wrote:
> > On Tue, Feb 13, 2018 at 02:08:47PM +0000, Dave Martin wrote:
> > > On Tue, Feb 13, 2018 at 09:51:30AM +0100, Christoffer Dall wrote:
> > > > On Fri, Feb 09, 2018 at 03:59:30PM +0000, Dave Martin wrote:

[...]

> > > 
> > > kvm_fpsimd_flush_cpu_state() is just an invalidation.  No state is
> > > actually saved today because we explicitly don't care about preserving
> > > the SVE state, because the syscall ABI throws the SVE regs away as
> > > a side effect any syscall including ioctl(KVM_RUN); also (currently) KVM
> > > ensures that the non-SVE FPSIMD bits _are_ restored by itself.
> > > 
> > > I think my proposal is that this hook might take on the role of
> > > actually saving the state too, if we move that out of the KVM host
> > > context save/restore code.
> > > 
> > > Perhaps we could even replace
> > > 
> > > 	preempt_disable();
> > > 	kvm_fpsimd_flush_cpu_state();
> > > 	/* ... */
> > > 	preempt_enable();
> > > 
> > > with
> > > 
> > > 	kernel_neon_begin();
> > > 	/* ... */
> > > 	kernel_neon_end();
> > 
> > I'm not entirely sure where the begin and end points would be in the
> > context of KVM?
> 
> Hmmm, actually there's a bug in your VHE changes now I look more
> closely in this area:
> 
> You assume that the only way for the FPSIMD regs to get unexpectedly
> dirtied is through a context switch, but actually this is not the case:
> a softirq can use kernel-mode NEON any time that softirqs are enabled.
> 
> This means that in between kvm_arch_vcpu_load() and _put() (whether via
> preempt notification or not), the guest's FPSIMD state in the regs may
> be trashed by a softirq.

ouch.

> 
> The simplest fix is to disable softirqs and preemption for that whole
> region, but since we can stay in it indefinitely that's obviously not
> the right approach.  Putting kernel_neon_begin() in _load() and
> kernel_neon_end() in _put() achieves the same without disabling
> softirq, but preemption is still disabled throughout, which is bad.
> This effectively makes the run ioctl nonpreemptible...
> 
> A better fix would be to set the cpu's kernel_neon_busy flag, which
> makes softirq code use non-NEON fallback code.
> 
> We could expose an interface from fpsimd.c to support that.
> 
> It still comes at a cost though: due to the switching from NEON to
> fallback code in softirq handlers, we may get a big performance
> regression in setups that rely heavily on NEON in softirq for
> performance.
> 

I wasn't aware that softirqs would use fpsimd.

> 
> Alternatively we could do something like the following, but it's a
> rather gross abstraction violation:
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 2e43f9d..6a1ff3a 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -746,9 +746,24 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		 * the effect of taking the interrupt again, in SVC
>  		 * mode this time.
>  		 */
> +		local_bh_disable();
>  		local_irq_enable();
>  
>  		/*
> +		 * If we exited due to one or mode pending interrupts, they
> +		 * have now been handled.  If such an interrupt pended a
> +		 * softirq, we shouldn't prevent that softirq from using
> +		 * kernel-mode NEON indefinitely: instead, give FPSIMD back to
> +		 * the host to manage as it likes.  We'll grab it again on the
> +		 * next FPSIMD trap from the guest (if any).
> +		 */
> +		if (local_softirq_pending() && FPSIMD untrapped for guest) {
> +			/* save vcpu FPSIMD context */
> +			/* enable FPSIMD trap for guest */
> +		}
> +		local_bh_enable();
> +
> +		/*
>  		 * We do local_irq_enable() before calling guest_exit() so
>  		 * that if a timer interrupt hits while running the guest we
>  		 * account that tick as being spent in the guest.  We enable
> 
> [...]
> 

I can't see this working, what if an IRQ comes in and a softirq gets
pending immediately after local_bh_enable() above?

And as you say, it's really not pretty.

This is really making me think that I'll drop this part of the
optimization and when we do optimize fpsimd handling, we do it properly
by integrating it with the kernel tracking.

What do you think?

Thanks,
-Christoffer