Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Wed, Feb 14, 2018 at 06:38:11PM +0100, Christoffer Dall wrote:
> On Wed, Feb 14, 2018 at 02:43:42PM +, 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 +, 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 +, 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?
Sorry, I missed a crucial bit of information here.
For context: here's the remainder of my argument. This is not a
recommendation...
--8<--
We can inhibit softirqs from trashing the FPSIMD regs by setting the
per-cpu kernel_neon_busy flag: that's forces softirq code to use
non
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Wed, 14 Feb 2018 17:38:11 +,
Christoffer Dall wrote:
>
> On Wed, Feb 14, 2018 at 02:43:42PM +, 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 +, 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 +, 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?
[catching up with the discus
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On 14 February 2018 at 17:38, Christoffer Dall
wrote:
> On Wed, Feb 14, 2018 at 02:43:42PM +, 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 +, 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 +, 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.
>
It is not common but it is permitted by the API, and there is mac80211
code and IPsec code that does this.
Performance penalties incurred by switching from accelerated h/w
instruction based crypto to scalar code can be as high as 20x, so we
should really avoid this if we can.
>>
>> 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
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Wed, Feb 14, 2018 at 02:43:42PM +, 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 +, 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 +, 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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
[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 +, 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 +, Dave Martin wrote: [...] > > It's hard to gauge the impact of this: it seems unlikely to make a > > massive difference, but will be highly workload-dependent. > > So I thought it might be useful to have some idea of the frequency of > events on a balanced workload, so I ran an 8-way SMP guest on Ubuntu > 14.04 running SPECjvm2008, a memcached benchmark, a MySQL workloads, and > some networking benchmarks, and I counted a few events: > > - Out of all the exits, from the guest to run-loop in EL1 on a non-VHE >system, fewer than 1% of them result in an exit to userspace (0.57%). > > - The VCPU thread was preempted (voluntarily or forced) in the kernel >less than 3% of the exits (2.72%). That's just below 5 preemptions >per ioctl(KVM_RUN). > > - In 29% of the preemptions (vcpu_put), the guest had touched FPSIMD >registers and the host context was restored. > > - We store the host context about 1.38 times per ioctl(KVM_RUN). > > So that tells me that (1) it's worth restoring the guest FPSIMD state > lazily as opposed to proactively on vcpu_load, and (2) that there's a > small opportunity for improvement by reducing redundant host vfp state > saves. That's really useful. I guess it confirms that lazy guest FPSIMD restore is desirable (though I wasn't disputing this) and that the potential benefit from eliminating redundant host FPSIMD saves is modest, assume that this workload is representative. So we shouldn't over-optimise for the latter if there are side costs from doing so. > > The redundancy occurs because of the deferred restore of the FPSIMD > > registers for host userspace: as a result, the host FPSIMD regs are > > either discardable (i.e., already saved) or not live at all between > > and context switch and the next ret_to_user. > > > > This means that if the vcpu run loop is preempted, then when the host > > switches back to the run loop it is pointless to save or restore the > > host FPSIMD state. > > > > A typical sequence of events exposing this redundancy would be as > > follows. I assume here that there are two cpu-bound tasks A and B > > competing for a host CPU, where A is a vcpu thread: > > > > - vcpu A is in the guest running a compute-heavy task > > - FPSIMD typically traps to the host before context switch > > X kvm saves the host FPSIMD state > > - kvm loads the guest FPSIMD state > > - vcpu A reenters the guest > > - host context switch IRQ preempts A back to the run loop > > Y kvm loads the host FPSIMD state via vcpu_put > > > > - host context switch: > > - TIF_FOREIGN_FPSTATE is set -> no save of user FPSIMD state > > - switch to B > > - B reaches ret_to_user > > Y B's user FPSIMD state is loaded: TIF_FOREIGN_FPSTATE now clear > > - B enters userspace > > > > - host context switch: > > - B enters kernel > > X TIF_FOREIGN_FPSTATE now set -> host saves B's FPSIMD state > > - switch to A -> set TIF_FOREIGN_FPSTATE for A > > - back to the KVM run loop > > > > - vcpu A enters guest > > - redo from start > > > > Here, the two saves marked X are redundant with respect to each other, > > and the two restores marked Y are redundant with respect to each other. > > > > Right, ok, but if we have > > - ioctl(KVM_RUN) > - mark hardware FPSIMD register state as invalid > - load guest FPSIMD state > - enter guest > - exit guest > - save guest FPSIMD state > - return to user space > > (I.e. we don't do any preemption in the guest) > > Then we'll loose the host FPSIMD register state, potentially, right? Yes. However, (disregarding kernel-mode NEON) no host task's state can be live in the FPSIMD regs other than current's. If another context's state is in the regs, it is either stale or a clean copy and we can harmlessly invalidate the association with no ill effects. The subtlety here comes from the SVE syscall ABI, which allows current's non-FPSIMD SVE bits to be discarded across a syscall: in this code, current _is_ in a syscall, so the fact that we can lose current's SVE bits here is fine: TIF_SVE will have been cleared in entry.S on the way in, and that means that SVE will trap for userspace giving a chance to zero those regs lazily for userspace when/if they're used again. Conversely, current's FPSIMD regs are preserved separately by KVM. > Your original comment on this patch was that we didn't need to restore > the host FPSIMD state in kvm_vcpu_put_sysregs, which would result in the > scenario above. The only way I can see this working is by making sure > that kvm_fpsimd_flush_cpu_state() also saves the FPSIMD hardware > register state if the st
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Tue, Feb 13, 2018 at 02:08:47PM +, 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 +, Dave Martin wrote: > > > On Wed, Feb 07, 2018 at 06:56:44PM +0100, Christoffer Dall wrote: > > > > On Wed, Feb 07, 2018 at 04:49:55PM +, Dave Martin wrote: > > [...] > > > > Simply entering the kernel and returning to userspace doesn't have > > > this effect by itself. > > > > > > > > > Prior to the SVE patches, KVM makes itself orthogonal to the host > > > context switch machinery by ensuring that whatever the host had > > > in the FPSIMD regs at guest entry is restored before returning to > > > the host. (IIUC) > > > > Only if the guest actually touches FPSIMD state. If the guest doesn't > > touch FPSIMD (no trap to EL2), then we never touch the state at all. > > I should have been clearer: KVM ensures that the state is _unchanged_ > before returning to the host, but can elide the save/restore when the > guest doesn't touch the state... > > > > > > This means that redundant save/restore work is > > > done by KVM, but does have the advantage of simplicity. > > > > I don't understand what the redundant part here is? Isn't it only > > redundant in the case where the host (for some reason) has already saved > > its FPSIMD state? I assume that won't be the common case, since > > "userspace->kernel->kvm_run" won't save the FPSIMD state, as you just > > explained above. > > ...however, when this elision does not occur, it may duplicate > save/restore done by the kernel, or it may save/restore worthless data > if the host's FPSIMD state is non-live at the time. > > It's hard to gauge the impact of this: it seems unlikely to make a > massive difference, but will be highly workload-dependent. So I thought it might be useful to have some idea of the frequency of events on a balanced workload, so I ran an 8-way SMP guest on Ubuntu 14.04 running SPECjvm2008, a memcached benchmark, a MySQL workloads, and some networking benchmarks, and I counted a few events: - Out of all the exits, from the guest to run-loop in EL1 on a non-VHE system, fewer than 1% of them result in an exit to userspace (0.57%). - The VCPU thread was preempted (voluntarily or forced) in the kernel less than 3% of the exits (2.72%). That's just below 5 preemptions per ioctl(KVM_RUN). - In 29% of the preemptions (vcpu_put), the guest had touched FPSIMD registers and the host context was restored. - We store the host context about 1.38 times per ioctl(KVM_RUN). So that tells me that (1) it's worth restoring the guest FPSIMD state lazily as opposed to proactively on vcpu_load, and (2) that there's a small opportunity for improvement by reducing redundant host vfp state saves. > > > The redundancy occurs because of the deferred restore of the FPSIMD > registers for host userspace: as a result, the host FPSIMD regs are > either discardable (i.e., already saved) or not live at all between > and context switch and the next ret_to_user. > > This means that if the vcpu run loop is preempted, then when the host > switches back to the run loop it is pointless to save or restore the > host FPSIMD state. > > A typical sequence of events exposing this redundancy would be as > follows. I assume here that there are two cpu-bound tasks A and B > competing for a host CPU, where A is a vcpu thread: > > - vcpu A is in the guest running a compute-heavy task > - FPSIMD typically traps to the host before context switch > X kvm saves the host FPSIMD state > - kvm loads the guest FPSIMD state > - vcpu A reenters the guest > - host context switch IRQ preempts A back to the run loop > Y kvm loads the host FPSIMD state via vcpu_put > > - host context switch: > - TIF_FOREIGN_FPSTATE is set -> no save of user FPSIMD state > - switch to B > - B reaches ret_to_user > Y B's user FPSIMD state is loaded: TIF_FOREIGN_FPSTATE now clear > - B enters userspace > > - host context switch: > - B enters kernel > X TIF_FOREIGN_FPSTATE now set -> host saves B's FPSIMD state > - switch to A -> set TIF_FOREIGN_FPSTATE for A > - back to the KVM run loop > > - vcpu A enters guest > - redo from start > > Here, the two saves marked X are redundant with respect to each other, > and the two restores marked Y are redundant with respect to each other. > Right, ok, but if we have - ioctl(KVM_RUN) - mark hardware FPSIMD register state as invalid - load guest FPSIMD state - enter guest - exit guest - save guest FPSIMD state - return to user space (I.e. we don't do any preemption in the guest) Then we'll loose the host FPSIMD register state, potentially, right? Your original comment on this patch was that we didn't need to restore the host FPSIMD state in kvm_vcpu_put_sysregs, which would result in the scenario above. The only way I can see this working is by making sure that kvm_fpsimd_flush_cpu_state() also saves the FPSIMD hardware register state
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Tue, Feb 13, 2018 at 09:51:30AM +0100, Christoffer Dall wrote: > On Fri, Feb 09, 2018 at 03:59:30PM +, Dave Martin wrote: > > On Wed, Feb 07, 2018 at 06:56:44PM +0100, Christoffer Dall wrote: > > > On Wed, Feb 07, 2018 at 04:49:55PM +, Dave Martin wrote: [...] > > Simply entering the kernel and returning to userspace doesn't have > > this effect by itself. > > > > > > Prior to the SVE patches, KVM makes itself orthogonal to the host > > context switch machinery by ensuring that whatever the host had > > in the FPSIMD regs at guest entry is restored before returning to > > the host. (IIUC) > > Only if the guest actually touches FPSIMD state. If the guest doesn't > touch FPSIMD (no trap to EL2), then we never touch the state at all. I should have been clearer: KVM ensures that the state is _unchanged_ before returning to the host, but can elide the save/restore when the guest doesn't touch the state... > > > This means that redundant save/restore work is > > done by KVM, but does have the advantage of simplicity. > > I don't understand what the redundant part here is? Isn't it only > redundant in the case where the host (for some reason) has already saved > its FPSIMD state? I assume that won't be the common case, since > "userspace->kernel->kvm_run" won't save the FPSIMD state, as you just > explained above. ...however, when this elision does not occur, it may duplicate save/restore done by the kernel, or it may save/restore worthless data if the host's FPSIMD state is non-live at the time. It's hard to gauge the impact of this: it seems unlikely to make a massive difference, but will be highly workload-dependent. The redundancy occurs because of the deferred restore of the FPSIMD registers for host userspace: as a result, the host FPSIMD regs are either discardable (i.e., already saved) or not live at all between and context switch and the next ret_to_user. This means that if the vcpu run loop is preempted, then when the host switches back to the run loop it is pointless to save or restore the host FPSIMD state. A typical sequence of events exposing this redundancy would be as follows. I assume here that there are two cpu-bound tasks A and B competing for a host CPU, where A is a vcpu thread: - vcpu A is in the guest running a compute-heavy task - FPSIMD typically traps to the host before context switch X kvm saves the host FPSIMD state - kvm loads the guest FPSIMD state - vcpu A reenters the guest - host context switch IRQ preempts A back to the run loop Y kvm loads the host FPSIMD state via vcpu_put - host context switch: - TIF_FOREIGN_FPSTATE is set -> no save of user FPSIMD state - switch to B - B reaches ret_to_user Y B's user FPSIMD state is loaded: TIF_FOREIGN_FPSTATE now clear - B enters userspace - host context switch: - B enters kernel X TIF_FOREIGN_FPSTATE now set -> host saves B's FPSIMD state - switch to A -> set TIF_FOREIGN_FPSTATE for A - back to the KVM run loop - vcpu A enters guest - redo from start Here, the two saves marked X are redundant with respect to each other, and the two restores marked Y are redundant with respect to each other. > > This breaks for SVE though: the high bits of the Z-registers will be > > zeroed as a side effect of the FPSIMD save/restore done by KVM. > > This means that if the host has state in those bits then it must > > be saved before entring the guest: that's what the new > > kvm_fpsimd_flush_cpu_state() hook in kvm_arch_vcpu_ioctl_run() is for. > > Again, I'm confused, because to me it looks like > kvm_fpsimd_flush_cpu_state() boils down to fpsimd_flush_cpu_state() > which just sets a pointer to NULL, but doesn't actually save the state. > > So, when is the state in the hardware registers saved to memory? This _is_ quite confusing: in writing this answer I identified a bug and then realised why there is no bug... 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(); which does have the host user context saving built in -- though this may have unwanted side effects, such as masking softirqs. Possibly not a big deal though if the region is kept small (?) Understanding precisely what kvm_fpsimd_flush_cpu_state() does is not trivial... the explanation goes something like this: (*takes deep breath*) A link is maintained between tasks an
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Fri, Feb 09, 2018 at 03:26:59PM +, Julien Grall wrote:
> Hi Christoffer,
>
> On 01/12/2018 12:07 PM, Christoffer Dall wrote:
> >Avoid saving the guest VFP registers and restoring the host VFP
> >registers on every exit from the VM. Only when we're about to run
> >userspace or other threads in the kernel do we really have to switch the
>
> s/do// ?
>
> >state back to the host state.
> >
> >We still initially configure the VFP registers to trap when entering the
> >VM, but the difference is that we now leave the guest state in the
> >hardware registers as long as we're running this VCPU, even if we
> >occasionally trap to the host, and we only restore the host state when
> >we return to user space or when scheduling another thread.
> >
> >Reviewed-by: Andrew Jones
> >Reviewed-by: Marc Zyngier
> >Signed-off-by: Christoffer Dall
> >---
> > arch/arm64/include/asm/kvm_host.h | 3 +++
> > arch/arm64/kernel/asm-offsets.c | 1 +
> > arch/arm64/kvm/hyp/entry.S| 3 +++
> > arch/arm64/kvm/hyp/switch.c | 48
> > ---
> > arch/arm64/kvm/hyp/sysreg-sr.c| 21 ++---
> > 5 files changed, 40 insertions(+), 36 deletions(-)
> >
> >diff --git a/arch/arm64/include/asm/kvm_host.h
> >b/arch/arm64/include/asm/kvm_host.h
> >index 0e9e7291a7e6..9e23bc968668 100644
> >--- a/arch/arm64/include/asm/kvm_host.h
> >+++ b/arch/arm64/include/asm/kvm_host.h
> >@@ -213,6 +213,9 @@ struct kvm_vcpu_arch {
> > /* Guest debug state */
> > u64 debug_flags;
> >+/* 1 if the guest VFP state is loaded into the hardware */
> >+u8 guest_vfp_loaded;
> >+
> > /*
> > * We maintain more than a single set of debug registers to support
> > * debugging the guest from the host and to maintain separate host and
> >diff --git a/arch/arm64/kernel/asm-offsets.c
> >b/arch/arm64/kernel/asm-offsets.c
> >index 612021dce84f..99467327c043 100644
> >--- a/arch/arm64/kernel/asm-offsets.c
> >+++ b/arch/arm64/kernel/asm-offsets.c
> >@@ -133,6 +133,7 @@ int main(void)
> >DEFINE(CPU_GP_REGS, offsetof(struct kvm_cpu_context,
> > gp_regs));
> >DEFINE(CPU_USER_PT_REGS, offsetof(struct kvm_regs, regs));
> >DEFINE(CPU_FP_REGS, offsetof(struct kvm_regs, fp_regs));
> >+ DEFINE(VCPU_GUEST_VFP_LOADED, offsetof(struct kvm_vcpu,
> >arch.guest_vfp_loaded));
> >DEFINE(VCPU_FPEXC32_EL2, offsetof(struct kvm_vcpu,
> > arch.ctxt.sys_regs[FPEXC32_EL2]));
> >DEFINE(VCPU_HOST_CONTEXT,offsetof(struct kvm_vcpu,
> > arch.host_cpu_context));
> >DEFINE(HOST_CONTEXT_VCPU,offsetof(struct kvm_cpu_context,
> > __hyp_running_vcpu));
> >diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> >index a360ac6e89e9..53652287a236 100644
> >--- a/arch/arm64/kvm/hyp/entry.S
> >+++ b/arch/arm64/kvm/hyp/entry.S
> >@@ -184,6 +184,9 @@ alternative_endif
> > add x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
> > bl __fpsimd_restore_state
> >+mov x0, #1
> >+strbw0, [x3, #VCPU_GUEST_VFP_LOADED]
> >+
> > // Skip restoring fpexc32 for AArch64 guests
> > mrs x1, hcr_el2
> > tbnzx1, #HCR_RW_SHIFT, 1f
> >diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> >index 12dc647a6e5f..29e44a20f5e3 100644
> >--- a/arch/arm64/kvm/hyp/switch.c
> >+++ b/arch/arm64/kvm/hyp/switch.c
> >@@ -24,43 +24,32 @@
> > #include
> > #include
> >-static bool __hyp_text __fpsimd_enabled_nvhe(void)
> >-{
> >-return !(read_sysreg(cptr_el2) & CPTR_EL2_TFP);
> >-}
> >-
> >-static bool __hyp_text __fpsimd_enabled_vhe(void)
> >-{
> >-return !!(read_sysreg(cpacr_el1) & CPACR_EL1_FPEN);
> >-}
> >-
> >-static hyp_alternate_select(__fpsimd_is_enabled,
> >-__fpsimd_enabled_nvhe, __fpsimd_enabled_vhe,
> >-ARM64_HAS_VIRT_HOST_EXTN);
> >-
> >-bool __hyp_text __fpsimd_enabled(void)
>
> Now that __fpsimd_enabled is removed, I think you need to remove the
> prototype in arch/arm64/include/kvm_hyp.h too.
>
Will do.
Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Fri, Feb 09, 2018 at 03:59:30PM +, Dave Martin wrote:
> On Wed, Feb 07, 2018 at 06:56:44PM +0100, Christoffer Dall wrote:
> > On Wed, Feb 07, 2018 at 04:49:55PM +, Dave Martin wrote:
> > > On Thu, Jan 25, 2018 at 08:46:53PM +0100, Christoffer Dall wrote:
> > > > On Mon, Jan 22, 2018 at 05:33:28PM +, Dave Martin wrote:
> > > > > On Fri, Jan 12, 2018 at 01:07:15PM +0100, Christoffer Dall wrote:
> > > > > > Avoid saving the guest VFP registers and restoring the host VFP
> > > > > > registers on every exit from the VM. Only when we're about to run
> > > > > > userspace or other threads in the kernel do we really have to
> > > > > > switch the
> > > > > > state back to the host state.
> > > > > >
> > > > > > We still initially configure the VFP registers to trap when
> > > > > > entering the
> > > > > > VM, but the difference is that we now leave the guest state in the
> > > > > > hardware registers as long as we're running this VCPU, even if we
> > > > > > occasionally trap to the host, and we only restore the host state
> > > > > > when
> > > > > > we return to user space or when scheduling another thread.
> > > > > >
> > > > > > Reviewed-by: Andrew Jones
> > > > > > Reviewed-by: Marc Zyngier
> > > > > > Signed-off-by: Christoffer Dall
> > > > >
> > > > > [...]
> > > > >
> > > > > > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > > > b/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > > > index 883a6383cd36..848a46eb33bf 100644
> > > > > > --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > > > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > >
> > > > > [...]
> > > > >
> > > > > > @@ -213,6 +215,19 @@ void kvm_vcpu_load_sysregs(struct kvm_vcpu
> > > > > > *vcpu)
> > > > > > */
> > > > > > void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
> > > > > > {
> > > > > > + struct kvm_cpu_context *host_ctxt = vcpu->arch.host_cpu_context;
> > > > > > + struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
> > > > > > +
> > > > > > + /* Restore host FP/SIMD state */
> > > > > > + if (vcpu->arch.guest_vfp_loaded) {
> > > > > > + if (vcpu_el1_is_32bit(vcpu)) {
> > > > > > + kvm_call_hyp(__fpsimd32_save_state,
> > > > > > +kern_hyp_va(guest_ctxt));
> > > > > > + }
> > > > > > + __fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> > > > > > + __fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > > > > > + vcpu->arch.guest_vfp_loaded = 0;
> > > > >
> > > > > Provided we've already marked the host FPSIMD state as dirty on the
> > > > > way
> > > > > in, we probably don't need to restore it here.
> > > > >
> > > > > In v4.15, the kvm_fpsimd_flush_cpu_state() call in
> > > > > kvm_arch_vcpu_ioctl_run() is supposed to do this marking: currently
> > > > > it's only done for SVE, since KVM was previously restoring the host
> > > > > FPSIMD subset of the state anyway, but it could be made unconditional.
> > > > >
> > > > > For a returning run ioctl, this would have the effect of deferring the
> > > > > host FPSIMD reload until we return to userspace, which is probably
> > > > > no more costly since the kernel must check whether to do this in
> > > > > ret_to_user anyway; OTOH if the vcpu thread was preempted by some
> > > > > other thread we save the cost of restoring the host state entirely
> > > > > here
> > > > > ... I think.
> > > >
> > > > Yes, I agree. However, currently the low-level logic in
> > > > arch/arm64/kvm/hyp/entry.S:__fpsimd_guest_restore which saves the host
> > > > state into vcpu->arch.host_cpu_context->gp_regs.fp_regs (where
> > > > host_cpu_context is a KVM-specific per-cpu variable). I think means
> > > > that simply marking the state as invalid would cause the kernel to
> > > > restore some potentially stale values when returning to userspace. Am I
> > > > missing something?
> > >
> > > I think my point was that there would be no need for the low-level
> > > save of the host fpsimd state currently done by hyp. At all. The
> > > state would already have been saved off to thread_struct before
> > > entering the guest.
> >
> > Ah, so if userspace touched any FPSIMD state, then we always save that
> > state when entering the kernel, even if we're just going to return to
> > the same userspace process anyway? (For any system call etc.?)
>
> Not exactly. The state is saved when the corresponding user task is
> scheduled out, or when it would become stale because we're about to
> modify the task_struct view of the state (sigreturn/PTRACE_SETREGST).
> The state is also saved if necessary by kernel_neon_begin().
>
> Simply entering the kernel and returning to userspace doesn't have
> this effect by itself.
>
>
> Prior to the SVE patches, KVM makes itself orthogonal to the host
> context switch machinery by ensuring that whatever the host had
> in the FPSIMD regs at guest entry is restored before returning to
> the host. (IIUC)
Only if the guest actually touches FPSIMD
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Wed, Feb 07, 2018 at 06:56:44PM +0100, Christoffer Dall wrote:
> On Wed, Feb 07, 2018 at 04:49:55PM +, Dave Martin wrote:
> > On Thu, Jan 25, 2018 at 08:46:53PM +0100, Christoffer Dall wrote:
> > > On Mon, Jan 22, 2018 at 05:33:28PM +, Dave Martin wrote:
> > > > On Fri, Jan 12, 2018 at 01:07:15PM +0100, Christoffer Dall wrote:
> > > > > Avoid saving the guest VFP registers and restoring the host VFP
> > > > > registers on every exit from the VM. Only when we're about to run
> > > > > userspace or other threads in the kernel do we really have to switch
> > > > > the
> > > > > state back to the host state.
> > > > >
> > > > > We still initially configure the VFP registers to trap when entering
> > > > > the
> > > > > VM, but the difference is that we now leave the guest state in the
> > > > > hardware registers as long as we're running this VCPU, even if we
> > > > > occasionally trap to the host, and we only restore the host state when
> > > > > we return to user space or when scheduling another thread.
> > > > >
> > > > > Reviewed-by: Andrew Jones
> > > > > Reviewed-by: Marc Zyngier
> > > > > Signed-off-by: Christoffer Dall
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > > b/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > > index 883a6383cd36..848a46eb33bf 100644
> > > > > --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> > > >
> > > > [...]
> > > >
> > > > > @@ -213,6 +215,19 @@ void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
> > > > > */
> > > > > void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
> > > > > {
> > > > > + struct kvm_cpu_context *host_ctxt = vcpu->arch.host_cpu_context;
> > > > > + struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
> > > > > +
> > > > > + /* Restore host FP/SIMD state */
> > > > > + if (vcpu->arch.guest_vfp_loaded) {
> > > > > + if (vcpu_el1_is_32bit(vcpu)) {
> > > > > + kvm_call_hyp(__fpsimd32_save_state,
> > > > > + kern_hyp_va(guest_ctxt));
> > > > > + }
> > > > > + __fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> > > > > + __fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > > > > + vcpu->arch.guest_vfp_loaded = 0;
> > > >
> > > > Provided we've already marked the host FPSIMD state as dirty on the way
> > > > in, we probably don't need to restore it here.
> > > >
> > > > In v4.15, the kvm_fpsimd_flush_cpu_state() call in
> > > > kvm_arch_vcpu_ioctl_run() is supposed to do this marking: currently
> > > > it's only done for SVE, since KVM was previously restoring the host
> > > > FPSIMD subset of the state anyway, but it could be made unconditional.
> > > >
> > > > For a returning run ioctl, this would have the effect of deferring the
> > > > host FPSIMD reload until we return to userspace, which is probably
> > > > no more costly since the kernel must check whether to do this in
> > > > ret_to_user anyway; OTOH if the vcpu thread was preempted by some
> > > > other thread we save the cost of restoring the host state entirely here
> > > > ... I think.
> > >
> > > Yes, I agree. However, currently the low-level logic in
> > > arch/arm64/kvm/hyp/entry.S:__fpsimd_guest_restore which saves the host
> > > state into vcpu->arch.host_cpu_context->gp_regs.fp_regs (where
> > > host_cpu_context is a KVM-specific per-cpu variable). I think means
> > > that simply marking the state as invalid would cause the kernel to
> > > restore some potentially stale values when returning to userspace. Am I
> > > missing something?
> >
> > I think my point was that there would be no need for the low-level
> > save of the host fpsimd state currently done by hyp. At all. The
> > state would already have been saved off to thread_struct before
> > entering the guest.
>
> Ah, so if userspace touched any FPSIMD state, then we always save that
> state when entering the kernel, even if we're just going to return to
> the same userspace process anyway? (For any system call etc.?)
Not exactly. The state is saved when the corresponding user task is
scheduled out, or when it would become stale because we're about to
modify the task_struct view of the state (sigreturn/PTRACE_SETREGST).
The state is also saved if necessary by kernel_neon_begin().
Simply entering the kernel and returning to userspace doesn't have
this effect by itself.
Prior to the SVE patches, KVM makes itself orthogonal to the host
context switch machinery by ensuring that whatever the host had
in the FPSIMD regs at guest entry is restored before returning to
the host. (IIUC) This means that redundant save/restore work is
done by KVM, but does have the advantage of simplicity.
This breaks for SVE though: the high bits of the Z-registers will be
zeroed as a side effect of the FPSIMD save/restore done by KVM.
This means that if the host has state in those bits then it must
be saved before e
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
Hi Christoffer,
On 01/12/2018 12:07 PM, Christoffer Dall wrote:
Avoid saving the guest VFP registers and restoring the host VFP
registers on every exit from the VM. Only when we're about to run
userspace or other threads in the kernel do we really have to switch the
s/do// ?
state back to the host state.
We still initially configure the VFP registers to trap when entering the
VM, but the difference is that we now leave the guest state in the
hardware registers as long as we're running this VCPU, even if we
occasionally trap to the host, and we only restore the host state when
we return to user space or when scheduling another thread.
Reviewed-by: Andrew Jones
Reviewed-by: Marc Zyngier
Signed-off-by: Christoffer Dall
---
arch/arm64/include/asm/kvm_host.h | 3 +++
arch/arm64/kernel/asm-offsets.c | 1 +
arch/arm64/kvm/hyp/entry.S| 3 +++
arch/arm64/kvm/hyp/switch.c | 48 ---
arch/arm64/kvm/hyp/sysreg-sr.c| 21 ++---
5 files changed, 40 insertions(+), 36 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index 0e9e7291a7e6..9e23bc968668 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -213,6 +213,9 @@ struct kvm_vcpu_arch {
/* Guest debug state */
u64 debug_flags;
+ /* 1 if the guest VFP state is loaded into the hardware */
+ u8 guest_vfp_loaded;
+
/*
* We maintain more than a single set of debug registers to support
* debugging the guest from the host and to maintain separate host and
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 612021dce84f..99467327c043 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -133,6 +133,7 @@ int main(void)
DEFINE(CPU_GP_REGS, offsetof(struct kvm_cpu_context, gp_regs));
DEFINE(CPU_USER_PT_REGS,offsetof(struct kvm_regs, regs));
DEFINE(CPU_FP_REGS, offsetof(struct kvm_regs, fp_regs));
+ DEFINE(VCPU_GUEST_VFP_LOADED,offsetof(struct kvm_vcpu,
arch.guest_vfp_loaded));
DEFINE(VCPU_FPEXC32_EL2,offsetof(struct kvm_vcpu,
arch.ctxt.sys_regs[FPEXC32_EL2]));
DEFINE(VCPU_HOST_CONTEXT, offsetof(struct kvm_vcpu,
arch.host_cpu_context));
DEFINE(HOST_CONTEXT_VCPU, offsetof(struct kvm_cpu_context,
__hyp_running_vcpu));
diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index a360ac6e89e9..53652287a236 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -184,6 +184,9 @@ alternative_endif
add x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
bl __fpsimd_restore_state
+ mov x0, #1
+ strbw0, [x3, #VCPU_GUEST_VFP_LOADED]
+
// Skip restoring fpexc32 for AArch64 guests
mrs x1, hcr_el2
tbnzx1, #HCR_RW_SHIFT, 1f
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 12dc647a6e5f..29e44a20f5e3 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -24,43 +24,32 @@
#include
#include
-static bool __hyp_text __fpsimd_enabled_nvhe(void)
-{
- return !(read_sysreg(cptr_el2) & CPTR_EL2_TFP);
-}
-
-static bool __hyp_text __fpsimd_enabled_vhe(void)
-{
- return !!(read_sysreg(cpacr_el1) & CPACR_EL1_FPEN);
-}
-
-static hyp_alternate_select(__fpsimd_is_enabled,
- __fpsimd_enabled_nvhe, __fpsimd_enabled_vhe,
- ARM64_HAS_VIRT_HOST_EXTN);
-
-bool __hyp_text __fpsimd_enabled(void)
Now that __fpsimd_enabled is removed, I think you need to remove the
prototype in arch/arm64/include/kvm_hyp.h too.
-{
- return __fpsimd_is_enabled()();
-}
Cheers,
--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Wed, Feb 07, 2018 at 04:49:55PM +, Dave Martin wrote:
> On Thu, Jan 25, 2018 at 08:46:53PM +0100, Christoffer Dall wrote:
> > On Mon, Jan 22, 2018 at 05:33:28PM +, Dave Martin wrote:
> > > On Fri, Jan 12, 2018 at 01:07:15PM +0100, Christoffer Dall wrote:
> > > > Avoid saving the guest VFP registers and restoring the host VFP
> > > > registers on every exit from the VM. Only when we're about to run
> > > > userspace or other threads in the kernel do we really have to switch the
> > > > state back to the host state.
> > > >
> > > > We still initially configure the VFP registers to trap when entering the
> > > > VM, but the difference is that we now leave the guest state in the
> > > > hardware registers as long as we're running this VCPU, even if we
> > > > occasionally trap to the host, and we only restore the host state when
> > > > we return to user space or when scheduling another thread.
> > > >
> > > > Reviewed-by: Andrew Jones
> > > > Reviewed-by: Marc Zyngier
> > > > Signed-off-by: Christoffer Dall
> > >
> > > [...]
> > >
> > > > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > b/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > index 883a6383cd36..848a46eb33bf 100644
> > > > --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> > > > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> > >
> > > [...]
> > >
> > > > @@ -213,6 +215,19 @@ void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
> > > > */
> > > > void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
> > > > {
> > > > + struct kvm_cpu_context *host_ctxt = vcpu->arch.host_cpu_context;
> > > > + struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
> > > > +
> > > > + /* Restore host FP/SIMD state */
> > > > + if (vcpu->arch.guest_vfp_loaded) {
> > > > + if (vcpu_el1_is_32bit(vcpu)) {
> > > > + kvm_call_hyp(__fpsimd32_save_state,
> > > > +kern_hyp_va(guest_ctxt));
> > > > + }
> > > > + __fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> > > > + __fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > > > + vcpu->arch.guest_vfp_loaded = 0;
> > >
> > > Provided we've already marked the host FPSIMD state as dirty on the way
> > > in, we probably don't need to restore it here.
> > >
> > > In v4.15, the kvm_fpsimd_flush_cpu_state() call in
> > > kvm_arch_vcpu_ioctl_run() is supposed to do this marking: currently
> > > it's only done for SVE, since KVM was previously restoring the host
> > > FPSIMD subset of the state anyway, but it could be made unconditional.
> > >
> > > For a returning run ioctl, this would have the effect of deferring the
> > > host FPSIMD reload until we return to userspace, which is probably
> > > no more costly since the kernel must check whether to do this in
> > > ret_to_user anyway; OTOH if the vcpu thread was preempted by some
> > > other thread we save the cost of restoring the host state entirely here
> > > ... I think.
> >
> > Yes, I agree. However, currently the low-level logic in
> > arch/arm64/kvm/hyp/entry.S:__fpsimd_guest_restore which saves the host
> > state into vcpu->arch.host_cpu_context->gp_regs.fp_regs (where
> > host_cpu_context is a KVM-specific per-cpu variable). I think means
> > that simply marking the state as invalid would cause the kernel to
> > restore some potentially stale values when returning to userspace. Am I
> > missing something?
>
> I think my point was that there would be no need for the low-level
> save of the host fpsimd state currently done by hyp. At all. The
> state would already have been saved off to thread_struct before
> entering the guest.
Ah, so if userspace touched any FPSIMD state, then we always save that
state when entering the kernel, even if we're just going to return to
the same userspace process anyway? (For any system call etc.?)
>
> This would result in a redundant save, but only when the host fpsimd
> state is dirty and the guest vcpu doesn't touch fpsimd before trapping
> back to the host.
>
> For the host, the fpsimd state is only dirty after entering the kernel
> from userspace (or after certain other things like sigreturn or ptrace).
> So this approach would still avoid repeated save/restore when cycling
> between the guest and the kvm code in the host.
>
I see.
> > It might very well be possible to change the logic so that we store the
> > host logic the same place where task_fpsimd_save() would have, and I
> > think that would make what you suggest possible.
>
> That's certainly possible, but I viewed that as harder. It would be
> necessary to map the host thread_struct into hyp etc. etc.
>
And even then, unnecessary because it would duplicate the existing state
save, IIUC above.
> > I'd like to make that a separate change from this patch though, as we're
> > already changing quite a bit with this series, so I'm trying to make any
> > logical change as contained per patch as possible, so that problems can
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Thu, Jan 25, 2018 at 08:46:53PM +0100, Christoffer Dall wrote:
> On Mon, Jan 22, 2018 at 05:33:28PM +, Dave Martin wrote:
> > On Fri, Jan 12, 2018 at 01:07:15PM +0100, Christoffer Dall wrote:
> > > Avoid saving the guest VFP registers and restoring the host VFP
> > > registers on every exit from the VM. Only when we're about to run
> > > userspace or other threads in the kernel do we really have to switch the
> > > state back to the host state.
> > >
> > > We still initially configure the VFP registers to trap when entering the
> > > VM, but the difference is that we now leave the guest state in the
> > > hardware registers as long as we're running this VCPU, even if we
> > > occasionally trap to the host, and we only restore the host state when
> > > we return to user space or when scheduling another thread.
> > >
> > > Reviewed-by: Andrew Jones
> > > Reviewed-by: Marc Zyngier
> > > Signed-off-by: Christoffer Dall
> >
> > [...]
> >
> > > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c
> > > b/arch/arm64/kvm/hyp/sysreg-sr.c
> > > index 883a6383cd36..848a46eb33bf 100644
> > > --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> > > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> >
> > [...]
> >
> > > @@ -213,6 +215,19 @@ void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
> > > */
> > > void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
> > > {
> > > + struct kvm_cpu_context *host_ctxt = vcpu->arch.host_cpu_context;
> > > + struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
> > > +
> > > + /* Restore host FP/SIMD state */
> > > + if (vcpu->arch.guest_vfp_loaded) {
> > > + if (vcpu_el1_is_32bit(vcpu)) {
> > > + kvm_call_hyp(__fpsimd32_save_state,
> > > + kern_hyp_va(guest_ctxt));
> > > + }
> > > + __fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> > > + __fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > > + vcpu->arch.guest_vfp_loaded = 0;
> >
> > Provided we've already marked the host FPSIMD state as dirty on the way
> > in, we probably don't need to restore it here.
> >
> > In v4.15, the kvm_fpsimd_flush_cpu_state() call in
> > kvm_arch_vcpu_ioctl_run() is supposed to do this marking: currently
> > it's only done for SVE, since KVM was previously restoring the host
> > FPSIMD subset of the state anyway, but it could be made unconditional.
> >
> > For a returning run ioctl, this would have the effect of deferring the
> > host FPSIMD reload until we return to userspace, which is probably
> > no more costly since the kernel must check whether to do this in
> > ret_to_user anyway; OTOH if the vcpu thread was preempted by some
> > other thread we save the cost of restoring the host state entirely here
> > ... I think.
>
> Yes, I agree. However, currently the low-level logic in
> arch/arm64/kvm/hyp/entry.S:__fpsimd_guest_restore which saves the host
> state into vcpu->arch.host_cpu_context->gp_regs.fp_regs (where
> host_cpu_context is a KVM-specific per-cpu variable). I think means
> that simply marking the state as invalid would cause the kernel to
> restore some potentially stale values when returning to userspace. Am I
> missing something?
I think my point was that there would be no need for the low-level
save of the host fpsimd state currently done by hyp. At all. The
state would already have been saved off to thread_struct before
entering the guest.
This would result in a redundant save, but only when the host fpsimd
state is dirty and the guest vcpu doesn't touch fpsimd before trapping
back to the host.
For the host, the fpsimd state is only dirty after entering the kernel
from userspace (or after certain other things like sigreturn or ptrace).
So this approach would still avoid repeated save/restore when cycling
between the guest and the kvm code in the host.
> It might very well be possible to change the logic so that we store the
> host logic the same place where task_fpsimd_save() would have, and I
> think that would make what you suggest possible.
That's certainly possible, but I viewed that as harder. It would be
necessary to map the host thread_struct into hyp etc. etc.
> I'd like to make that a separate change from this patch though, as we're
> already changing quite a bit with this series, so I'm trying to make any
> logical change as contained per patch as possible, so that problems can
> be spotted by bisecting.
Yes, I think that's wise.
> > Ultimately I'd like to go one better and actually treat a vcpu as a
> > first-class fpsimd context, so that taking an interrupt to the host
> > and then reentering the guest doesn't cause any reload at all.
>
> That should be the case already; kvm_vcpu_put_sysregs() is only called
> when you run another thread (preemptively or voluntarily), or when you
> return to user space, but making the vcpu fpsimd context a first-class
> citizen fpsimd context would mean that you can run another thread (and
> maybe run userspace if it doesn't use fpsimd?) without
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Mon, Jan 22, 2018 at 05:33:28PM +, Dave Martin wrote:
> On Fri, Jan 12, 2018 at 01:07:15PM +0100, Christoffer Dall wrote:
> > Avoid saving the guest VFP registers and restoring the host VFP
> > registers on every exit from the VM. Only when we're about to run
> > userspace or other threads in the kernel do we really have to switch the
> > state back to the host state.
> >
> > We still initially configure the VFP registers to trap when entering the
> > VM, but the difference is that we now leave the guest state in the
> > hardware registers as long as we're running this VCPU, even if we
> > occasionally trap to the host, and we only restore the host state when
> > we return to user space or when scheduling another thread.
> >
> > Reviewed-by: Andrew Jones
> > Reviewed-by: Marc Zyngier
> > Signed-off-by: Christoffer Dall
>
> [...]
>
> > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> > index 883a6383cd36..848a46eb33bf 100644
> > --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
>
> [...]
>
> > @@ -213,6 +215,19 @@ void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
> > */
> > void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
> > {
> > + struct kvm_cpu_context *host_ctxt = vcpu->arch.host_cpu_context;
> > + struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
> > +
> > + /* Restore host FP/SIMD state */
> > + if (vcpu->arch.guest_vfp_loaded) {
> > + if (vcpu_el1_is_32bit(vcpu)) {
> > + kvm_call_hyp(__fpsimd32_save_state,
> > +kern_hyp_va(guest_ctxt));
> > + }
> > + __fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> > + __fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > + vcpu->arch.guest_vfp_loaded = 0;
>
> Provided we've already marked the host FPSIMD state as dirty on the way
> in, we probably don't need to restore it here.
>
> In v4.15, the kvm_fpsimd_flush_cpu_state() call in
> kvm_arch_vcpu_ioctl_run() is supposed to do this marking: currently
> it's only done for SVE, since KVM was previously restoring the host
> FPSIMD subset of the state anyway, but it could be made unconditional.
>
> For a returning run ioctl, this would have the effect of deferring the
> host FPSIMD reload until we return to userspace, which is probably
> no more costly since the kernel must check whether to do this in
> ret_to_user anyway; OTOH if the vcpu thread was preempted by some
> other thread we save the cost of restoring the host state entirely here
> ... I think.
Yes, I agree. However, currently the low-level logic in
arch/arm64/kvm/hyp/entry.S:__fpsimd_guest_restore which saves the host
state into vcpu->arch.host_cpu_context->gp_regs.fp_regs (where
host_cpu_context is a KVM-specific per-cpu variable). I think means
that simply marking the state as invalid would cause the kernel to
restore some potentially stale values when returning to userspace. Am I
missing something?
It might very well be possible to change the logic so that we store the
host logic the same place where task_fpsimd_save() would have, and I
think that would make what you suggest possible.
I'd like to make that a separate change from this patch though, as we're
already changing quite a bit with this series, so I'm trying to make any
logical change as contained per patch as possible, so that problems can
be spotted by bisecting.
>
> Ultimately I'd like to go one better and actually treat a vcpu as a
> first-class fpsimd context, so that taking an interrupt to the host
> and then reentering the guest doesn't cause any reload at all.
That should be the case already; kvm_vcpu_put_sysregs() is only called
when you run another thread (preemptively or voluntarily), or when you
return to user space, but making the vcpu fpsimd context a first-class
citizen fpsimd context would mean that you can run another thread (and
maybe run userspace if it doesn't use fpsimd?) without having to
save/restore anything. Am I getting this right?
> But
> that feels like too big a step for this series, and there are likely
> side-issues I've not thought about yet.
>
It should definitely be in separate patches, but I would be optn to
tagging something on to the end of this series if we can stabilize this
series early after -rc1 is out.
Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put
On Fri, Jan 12, 2018 at 01:07:15PM +0100, Christoffer Dall wrote:
> Avoid saving the guest VFP registers and restoring the host VFP
> registers on every exit from the VM. Only when we're about to run
> userspace or other threads in the kernel do we really have to switch the
> state back to the host state.
>
> We still initially configure the VFP registers to trap when entering the
> VM, but the difference is that we now leave the guest state in the
> hardware registers as long as we're running this VCPU, even if we
> occasionally trap to the host, and we only restore the host state when
> we return to user space or when scheduling another thread.
>
> Reviewed-by: Andrew Jones
> Reviewed-by: Marc Zyngier
> Signed-off-by: Christoffer Dall
[...]
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 883a6383cd36..848a46eb33bf 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
[...]
> @@ -213,6 +215,19 @@ void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
> */
> void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
> {
> + struct kvm_cpu_context *host_ctxt = vcpu->arch.host_cpu_context;
> + struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
> +
> + /* Restore host FP/SIMD state */
> + if (vcpu->arch.guest_vfp_loaded) {
> + if (vcpu_el1_is_32bit(vcpu)) {
> + kvm_call_hyp(__fpsimd32_save_state,
> + kern_hyp_va(guest_ctxt));
> + }
> + __fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> + __fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> + vcpu->arch.guest_vfp_loaded = 0;
Provided we've already marked the host FPSIMD state as dirty on the way
in, we probably don't need to restore it here.
In v4.15, the kvm_fpsimd_flush_cpu_state() call in
kvm_arch_vcpu_ioctl_run() is supposed to do this marking: currently
it's only done for SVE, since KVM was previously restoring the host
FPSIMD subset of the state anyway, but it could be made unconditional.
For a returning run ioctl, this would have the effect of deferring the
host FPSIMD reload until we return to userspace, which is probably
no more costly since the kernel must check whether to do this in
ret_to_user anyway; OTOH if the vcpu thread was preempted by some
other thread we save the cost of restoring the host state entirely here
... I think.
Ultimately I'd like to go one better and actually treat a vcpu as a
first-class fpsimd context, so that taking an interrupt to the host
and then reentering the guest doesn't cause any reload at all. But
that feels like too big a step for this series, and there are likely
side-issues I've not thought about yet.
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
