On Wed, May 10, 2017 at 10:07:34AM +0200, Christoffer Dall wrote:
> On Wed, May 10, 2017 at 08:58:15AM +0200, Andrew Jones wrote:
> > On Tue, May 09, 2017 at 01:13:47PM -0700, Christoffer Dall wrote:
> > > On Tue, May 09, 2017 at 07:40:57PM +0200, Andrew Jones wrote:
> > > > On Sat, May 06, 2017 at 08:27:15PM +0200, Christoffer Dall wrote:
> > > > > On Wed, May 03, 2017 at 06:06:32PM +0200, Andrew Jones wrote:
> > > > >
> > > > > nit: can you make the subject of this patch a bit more specific?
> > > > >
> > > > > For example: Optimize checking power_off flag in KVM_RUN
> > > >
> > > > OK
> > > >
> > > > >
> > > > > > We can make a small optimization by not checking the state of
> > > > > > the power_off field on each run. This is done by treating
> > > > > > power_off like pause, only checking it when we get the EXIT
> > > > > > VCPU request. When a VCPU powers off another VCPU the EXIT
> > > > > > request is already made, so we just need to make sure the
> > > > > > request is also made on self power off. kvm_vcpu_kick() isn't
> > > > > > necessary for these cases, as the VCPU would just be kicking
> > > > > > itself, but we add it anyway as a self kick doesn't cost much,
> > > > > > and it makes the code more future-proof.
> > > > > >
> > > > > > Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> > > > > > ---
> > > > > > arch/arm/kvm/arm.c | 16 ++++++++++------
> > > > > > arch/arm/kvm/psci.c | 2 ++
> > > > > > 2 files changed, 12 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> > > > > > index 26d9d4d72853..24bbc7671d89 100644
> > > > > > --- a/arch/arm/kvm/arm.c
> > > > > > +++ b/arch/arm/kvm/arm.c
> > > > > > @@ -371,6 +371,13 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> > > > > > kvm_timer_vcpu_put(vcpu);
> > > > > > }
> > > > > >
> > > > > > +static void vcpu_power_off(struct kvm_vcpu *vcpu)
> > > > > > +{
> > > > > > + vcpu->arch.power_off = true;
> > > > > > + kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
> > > > > > + kvm_vcpu_kick(vcpu);
> > > > > > +}
> > > > > > +
> > > > > > int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
> > > > > > struct kvm_mp_state *mp_state)
> > > > > > {
> > > > > > @@ -390,7 +397,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> > > > > > vcpu->arch.power_off = false;
> > > > > > break;
> > > > > > case KVM_MP_STATE_STOPPED:
> > > > > > - vcpu->arch.power_off = true;
> > > > > > + vcpu_power_off(vcpu);
> > > > > > break;
> > > > > > default:
> > > > > > return -EINVAL;
> > > > > > @@ -626,14 +633,11 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> > > > > >
> > > > > > if (kvm_request_pending(vcpu)) {
> > > > > > if (kvm_check_request(KVM_REQ_VCPU_EXIT, vcpu)) {
> > > > > > - if (vcpu->arch.pause)
> > > > > > + if (vcpu->arch.power_off || vcpu->arch.pause)
> > > > > > vcpu_sleep(vcpu);
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > - if (vcpu->arch.power_off)
> > > > > > - vcpu_sleep(vcpu);
> > > > > > -
> > > > >
> > > > > Hmmm, even though I just gave a reviewed-by on the pause side, I'm not
> > > > > realizing that I don't think this works. Because you're now only
> > > > > checking requests in the vcpu loop, but the vcpu_sleep() function is
> > > > > implemented using swait_event_interruptible(), which can wake up if you
> > > > > have a pending signal for example, and then the loop can wrap around and
> > > > > you can run the VCPU even though you should be paused. Am I missing
> > > > > something?
> > > >
> > > > Hmm, I think I missed something. I missed that swait_event_interruptible()
> > > > doesn't check its condition again when awoken by a signal (which, as far
> > > > as I can tell, is the only other way we can stop vcpu_sleep() while
> > > > power_off and/or pause are true. Had I noticed that, I could have
> > > > addressed it in one of two ways:
> > > >
> > > > 1) Leave power_off and pause in the condition that stops guest entry.
> > > > Easy to see we'll never enter guest mode with one or both set.
> > > >
> > > > 2) Add a comment somewhere to explain the subtle dependency vcpu_sleep()
> > > > has on the pending signal check done after its call and before the
> > > > condition that stops guest entry is run. (IOW, I don't think we have
> > > > a bug with this series, but we do have a non-commented subtlety.)
> > > >
> > >
> > > But, then it can return to userspace and enter the kernel again, at
> > > which time there will be no pending signal and no pending VCPU requests,
> > > so the VCPU will enter the guest, but the pause flag can still be true
> > > and it shouldn't enter the guest. So I think there is a bug.
> >
> > Ah, indeed.
> >
> > >
> > > And I think the only nice way to solve it is to not clear the request
> > > until the CPU is really not paused any more.
> >
> > This would sort of circle back to the original approach of using the
> > request bit as the state, but I've already convinced myself that that's
> > too much abuse of VCPU requests to want to do it. (1) above would also
> > work and also allow VCPU requests to be used as designed.
> >
> > To tidy up the repeated 'vcpu->arch.power_off || vcpu->arch.pause'
> > condition I think I'll just introduce a vcpu_should_sleep() to encapsulate
> > it.
> >
>
> Fair enough, but could we keep these two booleans as flags in a single
> unsigned long on the vcpu struct then, so that we can do a single
> check on them and call out to handle_run_flags or whatever, analogous to
> how we handle requests?
Could do that.
>
> The other way to do it would be to set the request on the VCPU itself
> when returning from the sleep function if pause is still set...
I like this suggestion more. I'll do that for v4.
Thanks,
drew
