From: David Woodhouse <[email protected]>
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 <[email protected]>
---
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
