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 > 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 <[email protected]> > --- > 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? 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 > _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
