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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68c0d7f8-683f-c7fa-b328-e90e8dd9a789@redhat.com>
Date:   Wed, 28 Jun 2017 14:10:05 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Wanpeng Li <kernellwp@...il.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     Radim Krčmář <rkrcmar@...hat.com>,
        Wanpeng Li <wanpeng.li@...mail.com>
Subject: Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay



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