Re: [PATCH v4 12/12] KVM: selftests: Fix unreachable timer recovery in xen_shinfo_test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: selftests: wrong fix in guest_code() for juggle_shinfo_state() GFN 0 bug
--

commit 36db2d77eb09cc5d7b05a58f930d6724ba060038
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: selftests: Fix unreachable timer recovery in xen_shinfo_test

This commit adds a 10ms timeout to the SCHEDOP_poll hypercall in the
TEST_LOCKING_POLL_RACE phase of xen_shinfo_test so the guest does not hang if
the timer event is dropped while blocked in the poll.

> diff --git a/tools/testing/selftests/kvm/x86/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86/xen_shinfo_test.c
> index 3b11d0ee253f5..729c0ba9ab4e8 100644
> --- a/tools/testing/selftests/kvm/x86/xen_shinfo_test.c
> +++ b/tools/testing/selftests/kvm/x86/xen_shinfo_test.c
> @@ -350,7 +350,14 @@ static void guest_code(void)
>  	p = (struct sched_poll) {
>  		.ports = ports,
>  		.nr_ports = 1,
> -		.timeout = 0
> +		/*
> +		 * The poll must have a timeout: if the timer event is dropped
> +		 * while we are already blocked in the poll, nothing else will
> +		 * ever wake us, and the recovery protocol below could never
> +		 * run. (As with TEST_POLL_TIMEOUT above, KVM treats the
> +		 * timeout as relative.)
> +		 */
> +		.timeout = 10000000,
>  	};
>  
>  wait_for_timer:

[Severity: Medium]
Is SCHEDOP_poll in TEST_LOCKING_POLL_RACE actually supposed to block waiting
for the timer?

In main(), the host sets bit 0 of evtchn_pending[0] before arming the timer on
EVTCHN_TIMER:

tools/testing/selftests/kvm/x86/xen_shinfo_test.c:main() {
    ...
    case TEST_LOCKING_POLL_RACE:
        shinfo->evtchn_pending[0] = 1;

        evtchn_irq_expected = true;
        tmr.u.timer.expires_ns = rs->state_entry_time +
                                 SHINFO_RACE_TIMEOUT * 1000000000ULL;
        vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &tmr);
        break;
}

And in guest_code(), the guest polls on port 0 (ports[0] = 0), not on
EVTCHN_TIMER. In wait_pending_event(), ret defaults to true:

arch/x86/kvm/xen.c:wait_pending_event() {
    ...
    bool ret = true;
    int idx, i;

    idx = srcu_read_lock(&kvm->srcu);
    read_lock_irqsave(&gpc->lock, flags);
    if (!kvm_gpc_check(gpc, PAGE_SIZE))
        goto out_rcu;
    ...
}

When shinfo_cache is deactivated, kvm_gpc_check() returns false and
wait_pending_event() returns true immediately. When shinfo_cache is active on
shinfo, bit 0 of evtchn_pending[0] is set, so test_bit() also returns true
without halting the vCPU. Furthermore, if the timer expires while shinfo_cache
is deactivated, xen_timer_callback() sees -EWOULDBLOCK from
kvm_xen_set_evtchn_fast() and unconditionally raises KVM_REQ_UNBLOCK and calls
kvm_vcpu_kick().

Instead, isn't the vCPU blocking in kvm_vcpu_halt() because of a bug in
juggle_shinfo_state()?

tools/testing/selftests/kvm/x86/xen_shinfo_test.c:juggle_shinfo_state() {
    ...
    struct kvm_xen_hvm_attr cache_deactivate_hva = {
        .type = KVM_XEN_ATTR_TYPE_SHARED_INFO,
        .u.shared_info.hva = 0
    };
    ...
}

Because cache_deactivate_hva uses KVM_XEN_ATTR_TYPE_SHARED_INFO instead of
KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA with hva = 0 (which unions with gfn = 0),
kvm_xen_hvm_set_attr() activates shinfo_cache on guest GFN 0 rather than
deactivating it (since KVM_XEN_ATTR_TYPE_SHARED_INFO checks for
KVM_XEN_INVALID_GFN). At guest GFN 0, evtchn_pending[0] bit 0 is 0, so a
concurrent SCHEDOP_poll sees the port as not pending and blocks in
kvm_vcpu_halt().

Adding a 10ms timeout here causes the guest to sleep in kvm_vcpu_halt() for
10ms whenever SCHEDOP_poll coincides with cache_deactivate_hva, rather than
spinning in the tight hypercall loop to stress gpc->lock contention, while
leaving KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA deactivation untested and continuing
to overwrite guest GFN 0 with wallclock data.

Should this instead fix cache_deactivate_hva.type to
KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA in juggle_shinfo_state(), with:

  Fixes: 9397b5334af1 ("KVM: selftests: map Xen's shared_info page using HVA rather than GFN")

?

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