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.)
Thanks,
drew
>
> Thanks,
> -Christoffer
>
> > /*
> > * Preparing the interrupts to be injected also
> > * involves poking the GIC, which must be done in a
> > @@ -903,7 +907,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
> > * Handle the "start in power-off" case.
> > */
> > if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
> > - vcpu->arch.power_off = true;
> > + vcpu_power_off(vcpu);
> > else
> > vcpu->arch.power_off = false;
> >
> > diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> > index f189d0ad30d5..4a436685c552 100644
> > --- a/arch/arm/kvm/psci.c
> > +++ b/arch/arm/kvm/psci.c
> > @@ -65,6 +65,8 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
> > static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> > {
> > vcpu->arch.power_off = true;
> > + kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
> > + kvm_vcpu_kick(vcpu);
> > }
> >
> > static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > --
> > 2.9.3
> >
