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

[Paolo Bonzini]

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.

Paolo