[PATCH v4 05/12] KVM: x86/xen: Don't lose the timer event when delivery fails transiently

[David Woodhouse]

From: David Woodhouse <dwmw@xxxxxxxxxxxx>

kvm_xen_inject_timer_irqs() can drop events if kvm_xen_set_evtchn()
fails transiently, as it can when a memslot is in the process of
being updated. Defer the delivery in that case by leaving
->arch.xen.timer_pending set, so that it gets retried.

Raise KVM_REQ_UNBLOCK for the retry, as the timer callback does when
it defers: without it, nothing aborts the guest entry, and a vCPU
which takes no further exits would hold the owed event indefinitely.
The request survives to kvm_vcpu_exit_request(), bouncing the vCPU
back through vcpu_run()'s injection pass until the memslot update
completes.

If the shared_info cache has been deactivated (-EINVAL), clear
timer_pending — a halted vCPU would otherwise spin forever, as
kvm_cpu_has_pending_timer() aborts every block while nothing can
deliver the event — but keep timer_expires, so the GET_ATTR/SET_ATTR
migration flow still carries the event and re-fires it on re-arm.
If delivery fails with -EFAULT, the shared_info gfn is genuinely
unbacked and can never succeed: drop the event and complete the
timer rather than spin on it.

True Xen does not have this problem: the shared_info page is owned
by Xen and always exists, even when not mapped into the guest. The
equivalent for a KVM/Xen VMM is KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA,
which has no memslot dependency and is the recommended mode; this
fixes the legacy GFN mode, which xen_shinfo_test exercises.

Don't re-arm the hrtimer in the KVM_XEN_VCPU_GET_ATTR(TIMER) flush
while a delivery is still pending; a re-fired callback racing the
retry would deliver twice. Re-arm through kvm_xen_start_timer(),
which recomputes the expiry from timer_expires: the hrtimer's own
expiry field is stale when the timer last fired through the direct
callback path, which doesn't program the hrtimer. A timer still
armed across the flush is always restored; one which already fired is
not re-fired if its port is out of range, where the immediate -EINVAL
would destroy the owed timer_expires state just reported to
userspace.

The drop is vanishingly rare upstream, but hangs roughly half of all
xen_shinfo_test runs once a subsequent atomic SRCU conversion of the
pfncache widens the window; hence fixing it first.

Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode")
Assisted-by: LLM
Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
---
 arch/x86/kvm/xen.c                            | 108 ++++++++++++++----
 .../selftests/kvm/x86/xen_shinfo_test.c       |  11 +-
 2 files changed, 89 insertions(+), 30 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 8c49ec8aa4d9..8a9d6963fc05 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -36,6 +36,19 @@ static bool kvm_xen_hcall_evtchn_send(struct kvm_vcpu *vcpu, u64 param, u64 *r);
 
 DEFINE_STATIC_KEY_DEFERRED_FALSE(kvm_xen_enabled, HZ);
 
+static inline int max_evtchn_port(bool has_64bit_shinfo)
+{
+	if (has_64bit_shinfo)
+		return EVTCHN_2L_NR_CHANNELS;
+	else
+		return COMPAT_EVTCHN_2L_NR_CHANNELS;
+}
+
+static inline int kvm_max_evtchn_port(struct kvm *kvm)
+{
+	return max_evtchn_port(kvm_xen_has_64bit_shinfo(kvm));
+}
+
 static int kvm_xen_shared_info_init(struct kvm *kvm, bool wait)
 {
 	struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache;
@@ -126,14 +139,47 @@ void kvm_xen_inject_timer_irqs(struct kvm_vcpu *vcpu)
 {
 	if (atomic_read(&vcpu->arch.xen.timer_pending) > 0) {
 		struct kvm_xen_evtchn e;
+		int rc;
 
 		e.vcpu_id = vcpu->vcpu_id;
 		e.vcpu_idx = vcpu->vcpu_idx;
 		e.port = vcpu->arch.xen.timer_virq;
 		e.priority = KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL;
 
-		kvm_xen_set_evtchn(&e, vcpu->kvm);
+		rc = kvm_xen_set_evtchn(&e, vcpu->kvm);
+
+		/*
+		 * Leave timer_pending set if the memslot is being updated
+		 * (-EAGAIN), and raise KVM_REQ_UNBLOCK — as the timer
+		 * callback does when it defers — so that the entry is
+		 * aborted and this retried from vcpu_run(), rather than
+		 * the guest running with the owed event undelivered until
+		 * some unrelated VM exit.
+		 */
+		if (rc == -EAGAIN) {
+			kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
+			return;
+		}
+
+		/*
+		 * If the event cannot currently be delivered (-EINVAL:
+		 * deactivated shared_info, or an out-of-range port), clear
+		 * timer_pending — a halted vCPU would otherwise spin on
+		 * kvm_cpu_has_pending_timer() — but keep timer_expires,
+		 * which still reports the event as owed through
+		 * KVM_XEN_VCPU_GET_ATTR; re-arming the timer with an expiry
+		 * in the past fires it again.
+		 */
+		if (rc == -EINVAL) {
+			atomic_set(&vcpu->arch.xen.timer_pending, 0);
+			return;
+		}
 
+		/*
+		 * Delivered - or failed in a way which can never succeed
+		 * (-EFAULT: a genuinely unbacked shared_info gfn). Either
+		 * way the event is complete: drop it rather than spin.
+		 */
 		vcpu->arch.xen.timer_expires = 0;
 		atomic_set(&vcpu->arch.xen.timer_pending, 0);
 	}
@@ -1315,17 +1361,21 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 		r = 0;
 		break;
 
-	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
+	case KVM_XEN_VCPU_ATTR_TYPE_TIMER: {
+		bool timer_armed = false;
+
 		/*
-		 * Ensure a consistent snapshot of state is captured, with a
-		 * timer either being pending, or the event channel delivered
-		 * to the corresponding bit in the shared_info. Not still
-		 * lurking in the timer_pending flag for deferred delivery.
-		 * Purely as an optimisation, if the timer_expires field is
-		 * zero, that means the timer isn't active (or even in the
-		 * timer_pending flag) and there is no need to cancel it.
+		 * Ensure a consistent snapshot of state is captured: a timer
+		 * which already fired has either been delivered to the
+		 * corresponding bit in the shared_info, or is reported as
+		 * still owed (expires_ns != 0, if the shared_info page was
+		 * unavailable). Purely as an optimisation, if the
+		 * timer_expires field is zero, that means the timer isn't
+		 * active (or even in the timer_pending flag) and there is
+		 * no need to cancel it.
 		 */
 		if (vcpu->arch.xen.timer_expires) {
+			timer_armed = hrtimer_active(&vcpu->arch.xen.timer);
 			hrtimer_cancel(&vcpu->arch.xen.timer);
 			kvm_xen_inject_timer_irqs(vcpu);
 		}
@@ -1341,13 +1391,34 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 		 * That's fine, as the guest won't even have had a chance
 		 * to run and handle the interrupt. Asserting an already
 		 * pending event channel is idempotent.
+		 *
+		 * Don't re-arm if the flush above failed transiently and
+		 * left the timer pending: delivery is owned by the retry
+		 * on the guest entry path, and an immediate hrtimer fire
+		 * would race it — the callback's timer_pending suppression
+		 * check is not serialized against the entry path, so a
+		 * late callback could re-assert a port the entry path had
+		 * already delivered and the guest already consumed.
+		 *
+		 * An expiry already in the past fires immediately: the
+		 * event is still owed.
+		 *
+		 * A timer still armed (not yet fired) across the flush is
+		 * always restored, whatever its port. One which already
+		 * fired is not re-fired if its port is out of range, where
+		 * the immediate -EINVAL from the delivery path would destroy
+		 * the timer_expires state just reported to userspace.
 		 */
-		if (vcpu->arch.xen.timer_expires)
-			hrtimer_start_expires(&vcpu->arch.xen.timer,
-					      HRTIMER_MODE_ABS_HARD);
+		if (vcpu->arch.xen.timer_expires &&
+		    !atomic_read(&vcpu->arch.xen.timer_pending) &&
+		    (timer_armed ||
+		     vcpu->arch.xen.timer_virq < kvm_max_evtchn_port(vcpu->kvm)))
+			kvm_xen_start_timer(vcpu, vcpu->arch.xen.timer_expires,
+					    false);
 
 		r = 0;
 		break;
+	}
 
 	case KVM_XEN_VCPU_ATTR_TYPE_UPCALL_VECTOR:
 		data->u.vector = vcpu->arch.xen.upcall_vector;
@@ -1520,19 +1591,6 @@ static int kvm_xen_hypercall_complete_userspace(struct kvm_vcpu *vcpu)
 	return kvm_xen_hypercall_set_result(vcpu, run->xen.u.hcall.result);
 }
 
-static inline int max_evtchn_port(bool has_64bit_shinfo)
-{
-	if (has_64bit_shinfo)
-		return EVTCHN_2L_NR_CHANNELS;
-	else
-		return COMPAT_EVTCHN_2L_NR_CHANNELS;
-}
-
-static inline int kvm_max_evtchn_port(struct kvm *kvm)
-{
-	return max_evtchn_port(kvm_xen_has_64bit_shinfo(kvm));
-}
-
 static bool wait_pending_event(struct kvm_vcpu *vcpu, bool has_64bit_shinfo,
 			       int nr_ports, evtchn_port_t *ports)
 {
diff --git a/tools/testing/selftests/kvm/x86/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86/xen_shinfo_test.c
index aaa414c677ad..3b11d0ee253f 100644
--- a/tools/testing/selftests/kvm/x86/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86/xen_shinfo_test.c
@@ -356,11 +356,12 @@ static void guest_code(void)
 wait_for_timer:
 	/*
 	 * Poll for a timer wake event while the worker thread is mucking with
-	 * the shared info.  KVM XEN drops timer IRQs if the shared info is
-	 * invalid when the timer expires.  Arbitrarily poll 100 times before
-	 * giving up and asking the VMM to re-arm the timer.  100 polls should
-	 * consume enough time to beat on KVM without taking too long if the
-	 * timer IRQ is dropped due to an invalid event channel.
+	 * the shared info.  KVM is now supposed to retry a timer IRQ whose
+	 * delivery hit an invalid shared info rather than drop it, but older
+	 * kernels drop it on the floor.  Arbitrarily poll 100 times before
+	 * giving up and asking the VMM to re-arm the timer, so the test still
+	 * completes (and documents the VMM-side recovery protocol) on
+	 * unfixed kernels.
 	 */
 	for (i = 0; i < 100 && !guest_saw_irq; i++)
 		__xen_hypercall(__HYPERVISOR_sched_op, SCHEDOP_poll, &p);
-- 
2.43.0