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

[Wanpeng Li]

2017-06-29 15:55 GMT+08:00 Paolo Bonzini <pbonzini@xxxxxxxxxx>:
>
>
> On 29/06/2017 05:42, Wanpeng Li wrote:
>> So how about something like this?
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index d24c874..900ce86 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);
>> +
>> +            /* 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 && (apic_lvtt_oneshot(apic) ||
>> apic_lvtt_tscdeadline(apic)))
>> +                apic_timer_expired(apic);
>> +        }
>> +    }
>>
>> -    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);
>> +    if (need_cancel)
>> +        cancel_hv_timer(apic);
>>
>> -        /* In case the sw timer triggered in the window */
>> -        if (atomic_read(&apic->lapic_timer.pending) &&
>> -            !apic_lvtt_period(apic))
>> -            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;
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 4f616db..0a0e696 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -11125,6 +11125,9 @@ static int vmx_set_hv_timer(struct kvm_vcpu
>> *vcpu, u64 guest_deadline_tsc)
>>      u64 guest_tscl = kvm_read_l1_tsc(vcpu, tscl);
>>      u64 delta_tsc = max(guest_deadline_tsc, guest_tscl) - guest_tscl;
>>
>> +    if (delta_tsc == 0)
>> +        return 1;
>
> You still need to enable the preemption timer even if you return 1, so
> in lapic.c it becomes
>
>         if (!apic_lvtt_period(apic)) {
>                 if (r)
>                         apic_timer_expired(apic);
>                 if (atomic_read(&apic->lapic_timer.pending))
>                         need_cancel = true;
>         }

As you point out, we should cancel the preemption timer if it returns
1 and the APIC timer's mode is oneshot/tscdeadline. How about
something like this:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d24c874..b801385 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1504,21 +1504,28 @@ 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);
+
+            /* In case the sw timer triggered in the window */
+            if (!apic_lvtt_period(apic)) {
+                if (r || atomic_read(&apic->lapic_timer.pending)) {
+                    need_cancel = true;
+                    if (r)
+                    apic_timer_expired(apic);
+                }
+            }
+        }
+    }

-    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);
+    if (need_cancel)
+        cancel_hv_timer(apic);

-        /* In case the sw timer triggered in the window */
-        if (atomic_read(&apic->lapic_timer.pending) &&
-            !apic_lvtt_period(apic))
-            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;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ca5d2b9..b8bde13 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11134,7 +11134,11 @@ static int vmx_set_hv_timer(struct kvm_vcpu
*vcpu, u64 guest_deadline_tsc)
     vmx->hv_deadline_tsc = tscl + delta_tsc;
     vmcs_set_bits(PIN_BASED_VM_EXEC_CONTROL,
             PIN_BASED_VMX_PREEMPTION_TIMER);
-    return 0;
+
+    if (delta_tsc == 0)
+        return 1;
+    else
+        return 0;
 }

 static void vmx_cancel_hv_timer(struct kvm_vcpu *vcpu)

Regards,
Wanpeng Li