Re: [PATCH 04/10] KVM: x86/xen: Don't lose the timer event when delivery fails transiently

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: x86/xen: infinite vcpu loop when shared_info deactivated
--

commit 51f8b9d346d917ad2de67baf6af85e344406d8e0
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86/xen: Don't lose the timer event when delivery fails transiently

This commit modifies KVM's Xen timer injection to handle transient failures
from kvm_xen_set_evtchn(). If the event channel set fails with -EAGAIN
or -EINVAL, it leaves the timer_pending flag set so the delivery can be
retried on the next entry pass.

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -106,13 +106,23 @@ 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);
> +
> +		/*
> +		 * Deliberately leave timer_pending set if the memslot is
> +		 * being updated (-EAGAIN) or the shared_info cache has
> +		 * been deactivated (-EINVAL); delivery is retried on the
> +		 * next entry pass.
> +		 */
> +		if (rc == -EAGAIN || rc == -EINVAL)
> +			return;

[Severity: High]
Does this early return cause an infinite loop in the vCPU run loop if the
shared_info cache is deactivated by userspace and returns -EINVAL?

When the vCPU attempts to halt, kvm_vcpu_check_block() evaluates
kvm_cpu_has_pending_timer(), which returns true because timer_pending is
still > 0. This aborts the block, immediately returning to vcpu_block().

However, vcpu_block() then checks kvm_arch_vcpu_runnable(), which does not
consider timer_pending, so it evaluates to false. This causes vcpu_block()
to return 1, forcing the vcpu_run() loop to spin indefinitely without
breaking, sleeping, or entering the guest:

virt/kvm/kvm_main.c:kvm_vcpu_check_block() {
    ...
    if (kvm_cpu_has_pending_timer(vcpu))
        goto out;
    ...
}

arch/x86/kvm/x86.c:vcpu_block() {
    ...
    if (!kvm_arch_vcpu_runnable(vcpu))
        return 1;
    ...
}

arch/x86/kvm/x86.c:vcpu_run() {
    for (;;) {
        if (kvm_vcpu_running(vcpu)) {
            r = vcpu_enter_guest(vcpu);
        } else {
            r = vcpu_block(vcpu);
        }

        if (r <= 0)
            break;
        ...
    }
}

With r being 1, the loop unconditionally retries, hits -EINVAL again, and
spins monopolizing the host CPU until the cache is reactivated. Could we
avoid this stall by either dropping the event or handling the sleep abort
differently when shared_info is unavailable?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013204824.454208-1-dwmw2@infradead.org?part=4