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

[Paolo Bonzini]

On 28/06/2017 16:27, Wanpeng Li wrote:
> 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?

In that case, set_hv_timer should probably always enable the preemption
timer.  You can then cancel it if it returns 1 _and_ the APIC timer's
mode is oneshot/tscdeadline.

Paolo