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

[Wanpeng Li]

2017-06-28 22:00 GMT+08:00 Paolo Bonzini <pbonzini@xxxxxxxxxx>:
>
>
> On 28/06/2017 15:55, Wanpeng Li wrote:
>>>>       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.
>> Actually I considered this before, however, I referred to apic timer
>> periodic mode which is emulated by hrtimer
>
> Periodic mode can also be emulated by preemption timer... it was added
> by some Wanpeng Li in commit 8003c9ae204e ("KVM: LAPIC: add APIC Timer
> periodic/oneshot mode VMX preemption timer support", 2016-11-02), do you
> know him? ;)

Indeed. :)

>
>> , there is no hrtimer start
>> for the next period in start_sw_period(). If it is also buggy?
>
> start_sw_period always goes through the hrtimer for periodic timer:
>
>         if (apic_lvtt_oneshot(apic) &&
>             ktime_after(ktime_get(),
>                         apic->lapic_timer.target_expiration)) {
>                 apic_timer_expired(apic);
>                 return;
>         }
>
>         hrtimer_start(&apic->lapic_timer.timer,
>                 apic->lapic_timer.target_expiration,
>                 HRTIMER_MODE_ABS_PINNED);
>
> (the direct call to apic_timer_expired is conditonal to
> apic_lvtt_oneshot).  This way, apic_timer_fn takes care of advancing the
> hrtimer deadline and returning HRTIMER_RESTART.

Ah, yes, I miss the apic_lvtt_oneshot() check here.

Regards,
Wanpeng Li