Re: [PATCH 1/2] KVM: x86: extract guest running logic from __vcpu_run

[David Matlack]

On Fri, Feb 6, 2015 at 4:16 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> Rename the old __vcpu_run to vcpu_run, and extract its main logic
> to a new function.
>
> The next patch will add a new early-exit condition to __vcpu_run,
> avoid extra indentation.
>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---

Reviewed-by: David Matlack <dmatlack@xxxxxxxxxx>

>  arch/x86/kvm/x86.c | 67 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 36 insertions(+), 31 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index bd7a70be41b3..0b8dd13676ef 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6165,7 +6165,7 @@ void kvm_arch_mmu_notifier_invalidate_page(struct kvm *kvm,
>  }
>
>  /*
> - * Returns 1 to let __vcpu_run() continue the guest execution loop without
> + * Returns 1 to let vcpu_run() continue the guest execution loop without
>   * exiting to the userspace.  Otherwise, the value will be returned to the
>   * userspace.
>   */
> @@ -6383,42 +6383,45 @@ out:
>         return r;
>  }
>
> +static inline int __vcpu_run(struct kvm *kvm, struct kvm_vcpu *vcpu)

Kind of confusing function body given the name. Maybe put

       if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
           !vcpu->arch.apf.halted)
               return vcpu_enter_guest(vcpu);

back in vcpu_run and rename this function? vcpu_wait?

> +{
> +       if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
> +           !vcpu->arch.apf.halted)
> +               return vcpu_enter_guest(vcpu);
> +
> +       srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
> +       kvm_vcpu_block(vcpu);
> +       vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
> +       if (!kvm_check_request(KVM_REQ_UNHALT, vcpu))
> +               return 1;
>
> -static int __vcpu_run(struct kvm_vcpu *vcpu)
> +       kvm_apic_accept_events(vcpu);
> +       switch(vcpu->arch.mp_state) {
> +       case KVM_MP_STATE_HALTED:
> +               vcpu->arch.pv.pv_unhalted = false;
> +               vcpu->arch.mp_state =
> +                       KVM_MP_STATE_RUNNABLE;
> +       case KVM_MP_STATE_RUNNABLE:
> +               vcpu->arch.apf.halted = false;
> +               break;
> +       case KVM_MP_STATE_INIT_RECEIVED:
> +               break;
> +       default:
> +               return -EINTR;
> +               break;
> +       }
> +       return 1;
> +}
> +
> +static int vcpu_run(struct kvm_vcpu *vcpu)

vcpu_run_loop might be a clearer name.

>  {
>         int r;
>         struct kvm *kvm = vcpu->kvm;
>
>         vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
>
> -       r = 1;
> -       while (r > 0) {
> -               if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
> -                   !vcpu->arch.apf.halted)
> -                       r = vcpu_enter_guest(vcpu);
> -               else {
> -                       srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
> -                       kvm_vcpu_block(vcpu);
> -                       vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
> -                       if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
> -                               kvm_apic_accept_events(vcpu);
> -                               switch(vcpu->arch.mp_state) {
> -                               case KVM_MP_STATE_HALTED:
> -                                       vcpu->arch.pv.pv_unhalted = false;
> -                                       vcpu->arch.mp_state =
> -                                               KVM_MP_STATE_RUNNABLE;
> -                               case KVM_MP_STATE_RUNNABLE:
> -                                       vcpu->arch.apf.halted = false;
> -                                       break;
> -                               case KVM_MP_STATE_INIT_RECEIVED:
> -                                       break;
> -                               default:
> -                                       r = -EINTR;
> -                                       break;
> -                               }
> -                       }
> -               }
> -
> +       for (;;) {
> +               r = __vcpu_run(kvm, vcpu);
>                 if (r <= 0)
>                         break;
>
> @@ -6430,6 +6433,7 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>                         r = -EINTR;
>                         vcpu->run->exit_reason = KVM_EXIT_INTR;
>                         ++vcpu->stat.request_irq_exits;
> +                       break;
>                 }
>
>                 kvm_check_async_pf_completion(vcpu);
> @@ -6438,6 +6442,7 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>                         r = -EINTR;
>                         vcpu->run->exit_reason = KVM_EXIT_INTR;
>                         ++vcpu->stat.signal_exits;
> +                       break;

The removal of the loop condition and the addition of these "break"s
change the loop behavior slightly, but I think it's safe. We'll start
breaking before checking need_resched(), but we're about to return to
userspace anyway so we'll reschedule then.

>                 }
>                 if (need_resched()) {
>                         srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
> @@ -6569,7 +6574,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>         } else
>                 WARN_ON(vcpu->arch.pio.count || vcpu->mmio_needed);
>
> -       r = __vcpu_run(vcpu);
> +       r = vcpu_run(vcpu);
>
>  out:
>         post_kvm_run_save(vcpu);
> --
> 1.8.3.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/