Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay

[Wanpeng Li]

2017-06-28 20:10 GMT+08:00 Paolo Bonzini <pbonzini@xxxxxxxxxx>:
>
>
> On 28/06/2017 03:29, Wanpeng Li wrote:
>>       u64 tscdeadline = apic->lapic_timer.tscdeadline;
>> +     int ret = 0;
>>
>>       if ((atomic_read(&apic->lapic_timer.pending) &&
>>               !apic_lvtt_period(apic)) ||
>> -             kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>> +             (ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
>>               if (apic->lapic_timer.hv_timer_in_use)
>>                       cancel_hv_timer(apic);
>> +             if (ret == 1) {
>> +                     apic_timer_expired(apic);
>> +                     return true;
>> +             }
>
> The preemption timer can also be used for modes other than TSC deadline.
>
> In periodic mode, your patch would miss a call to
> advance_periodic_target_expiration, which is only called by
> kvm_lapic_expired_hv_timer.
>
> You could use something like this:
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index d24c8742d9b0..15b751aa7625 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
>  static bool start_hv_timer(struct kvm_lapic *apic)
>  {
>         u64 tscdeadline = apic->lapic_timer.tscdeadline;
> +       bool need_cancel = apic->lapic_timer.hv_timer_in_use;
> +       if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
> +               int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
> +               if (r >= 0) {
> +                       need_cancel = false;
> +                       apic->lapic_timer.hv_timer_in_use = true;
> +                       hrtimer_cancel(&apic->lapic_timer.timer);
>
> -       if ((atomic_read(&apic->lapic_timer.pending) &&
> -               !apic_lvtt_period(apic)) ||
> -               kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
> -               if (apic->lapic_timer.hv_timer_in_use)
> -                       cancel_hv_timer(apic);
> -       } else {
> -               apic->lapic_timer.hv_timer_in_use = true;
> -               hrtimer_cancel(&apic->lapic_timer.timer);
> -
> -               /* In case the sw timer triggered in the window */
> -               if (atomic_read(&apic->lapic_timer.pending) &&
> -                       !apic_lvtt_period(apic))
> -                       cancel_hv_timer(apic);
> +                       /* In case the sw timer triggered in the window */
> +                       if (atomic_read(&apic->lapic_timer.pending) &&
> +                           !apic_lvtt_period(apic))
> +                               need_cancel = true;
> +                       else if (r)
> +                               kvm_lapic_expired_hv_timer(vcpu);
> +               }
>         }
> +
> +       if (need_cancel)
> +               cancel_hv_timer(apic);
> +
>         trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
>                         apic->lapic_timer.hv_timer_in_use);
>         return apic->lapic_timer.hv_timer_in_use;
>
> but I'm afraid of introducing a mutual recursion between
> start_hv_timer and kvm_lapic_expired_hv_timer.

We can just handle the apic timer oneshot/tscdeadline mode instead of
periodic mode just like which is emulated by hrtimer to avoid the
mutual recusion, what do you think?

Regards,
Wanpeng Li