From: David Woodhouse 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. 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. The drop is vanishingly rare upstream, but hangs roughly half of all xen_shinfo_test runs once the following atomic SRCU conversion 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 --- arch/x86/kvm/xen.c | 51 ++++++++++++++++--- .../selftests/kvm/x86/xen_shinfo_test.c | 11 ++-- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index cd1063d659cc..f1be9022bc6f 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -115,13 +115,35 @@ 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); delivery is retried on the next entry pass. + */ + if (rc == -EAGAIN) + 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; + } vcpu->arch.xen.timer_expires = 0; atomic_set(&vcpu->arch.xen.timer_pending, 0); @@ -1272,12 +1294,13 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data) case KVM_XEN_VCPU_ATTR_TYPE_TIMER: /* - * 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 + * 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) { @@ -1296,8 +1319,20 @@ 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. */ - if (vcpu->arch.xen.timer_expires) + if (vcpu->arch.xen.timer_expires && + !atomic_read(&vcpu->arch.xen.timer_pending)) hrtimer_start_expires(&vcpu->arch.xen.timer, HRTIMER_MODE_ABS_HARD); 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