Re: [PATCH 5/5] KVM: x86: Using TSC deadline may cause multiple interrupts by user writes

From: Paolo Bonzini <pbonzini@redhat.com>
To: "Nadav Amit" <nadav.amit@gmail.com>, "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Nadav Amit <namit@cs.technion.ac.il>,
	joro@8bytes.org, kvm@vger.kernel.org
Subject: Re: [PATCH 5/5] KVM: x86: Using TSC deadline may cause multiple interrupts by user writes
Date: Fri, 10 Oct 2014 11:45:20 +0200	[thread overview]
Message-ID: <5437AAB0.4060908@redhat.com> (raw)
In-Reply-To: <99CA15AA-DDC1-4E95-A168-7AA640C5A3B9@gmail.com>

Il 10/10/2014 03:55, Nadav Amit ha scritto:
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index b8345dd..51428dd 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1096,9 +1096,12 @@ static void start_apic_timer(struct kvm_lapic *apic)
>>  		if (likely(tscdeadline > guest_tsc)) {
>>  			ns = (tscdeadline - guest_tsc) * 1000000ULL;
>>  			do_div(ns, this_tsc_khz);
>> +			hrtimer_start(&apic->lapic_timer.timer,
>> +				ktime_add_ns(now, ns), HRTIMER_MODE_ABS);
>> +		} else {
>> +			atomic_inc(&ktimer->pending);
>> +			kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
>>  		}
>> -		hrtimer_start(&apic->lapic_timer.timer,
>> -			ktime_add_ns(now, ns), HRTIMER_MODE_ABS);
>>
>>  		local_irq_restore(flags);
>>  	}
>> @@ -1355,9 +1358,6 @@ void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data)
>>  		return;
>>
>>  	hrtimer_cancel(&apic->lapic_timer.timer);
>> -	/* Inject here so clearing tscdeadline won't override new value */
>> -	if (apic_has_pending_timer(vcpu))
>> -		kvm_inject_apic_timer_irqs(vcpu);
>>  	apic->lapic_timer.tscdeadline = data;
>>  	start_apic_timer(apic);
>> }
> 
> Perhaps I am missing something, but I don’t see how it solves the problem I encountered.
> Recall the scenario:
> 1. A TSC deadline timer interrupt is pending.
> 2. TSC deadline was still not cleared (which happens during vcpu_run).
> 3. Userspace uses KVM_GET_MSRS/KVM_SET_MSRS to load the same deadline msr.
> 
> It appears that in such scenario the guest would still get spurious
> interrupt for no reason, as ktimer->pending may already be increased in
> apic_timer_fn.

If ktimer->pending == 2 you still get only one interrupt, not two.  Am I
misunderstanding your objection?

> Second, I think that the solution I proposed would perform better.
> Currently, there are many unnecessary cancellations and setups of the
> timer. This solution does not resolve this problem.

I think it does.  You do not get an hrtimer_start if tscdeadline <=
guest_tsc.  To avoid useless cancels, either check hrtimer_is_enqueued
before calling hrtimer_cancel, or go straight to the source and avoid
taking the lock in the easy cases:

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 1c2fe7de2842..6ce725007424 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1043,10 +1043,17 @@ int hrtimer_try_to_cancel(struct hrtimer *timer)
 {
 	struct hrtimer_clock_base *base;
 	unsigned long flags;
-	int ret = -1;
+	unsigned long state = timer->state;
+	int ret;
+
+	if (state & HRTIMER_STATE_ENQUEUED)
+		return 0;
+	if (state & HRTIMER_STATE_CALLBACK)
+		return -1;

 	base = lock_hrtimer_base(timer, &flags);

+	ret = -1;
 	if (!hrtimer_callback_running(timer))
 		ret = remove_hrtimer(timer, base);


> Last, I think that having less interrupts on deadline changes is not
> completely according to the SDM which says: "If software disarms the
> timer or postpones the deadline, race conditions may result in the
> delivery of a spurious timer interrupt.” It never says interrupts may
> be lost if you reprogram the deadline before you check it expired.

But the case when you rewrite the same value to the MSR is neither
disarming nor postponing.  You would be getting two interrupts for the
same event.  That is why I agree with Radim that checking host_initiated
is wrong.

Paolo
