Re: [PATCH QEMU HCI-8.1 v4 27/31] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] vhost-net: cpr-exec failure path does not re-apply busyloop timeout (poll-us)
--

commit 3c0903d7d2ea3ce9e525bbd0a16e4a5b7320f49d
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

VZ: migration/cpr-exec: take the vhost-net devices back if exec fails

This patch makes the cpr-exec failure path take the kernel vhost-net
devices back with VHOST_SET_OWNER after they were released with
VHOST_RESET_OWNER. It also re-applies the vring call and err fds, and
marks released devices with a new cpr_owner_reset flag.

> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c

[ ... ]

> +/*
> + * Take the ownership back when the exec fails and the VM goes on in the
> + * old QEMU.  VHOST_RESET_OWNER has reset the kernel device, but the devices
> + * are stopped, and vhost_net_start() sets everything up again when the VM
> + * resumes.
> + */
> +void vhost_net_cpr_set_owners(void)
> +{

[ ... ]

> +            /* RESET_OWNER also dropped the vring fds set by vhost_dev_init() */
> +            for (i = 0; i < dev->nvqs; i++) {
> +                const VhostOps *ops = dev->vhost_ops;
> +                struct vhost_virtqueue *vq = &dev->vqs[i];
> +                struct vhost_vring_file file = {
> +                    .index = ops->vhost_get_vq_index(dev, dev->vq_index + i),
> +                    .fd = event_notifier_get_wfd(&vq->masked_notifier),
> +                };
> +
> +                r = ops->vhost_set_vring_call(dev, &file);
> +                if (r == 0 && ops->vhost_set_vring_err) {
> +                    file.fd = event_notifier_get_fd(&vq->error_notifier);
> +                    r = ops->vhost_set_vring_err(dev, &file);
> +                }

[Severity: Low]
Is the busyloop timeout also lost here?

vhost_dev_init() programs it only once at setup time:

hw/virtio/vhost.c:vhost_dev_init() {
    ...
    if (busyloop_timeout) {
        for (i = 0; i < hdev->nvqs; ++i) {
            r = vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
                                                     busyloop_timeout);
    ...
}

The value comes from the user-configurable poll-us tap option:

net/tap.c:net_init_tap_one() {
    ...
    if (tap->has_poll_us) {
        options.busyloop_timeout = tap->poll_us;
    }
    ...
}

The value is not stored in struct vhost_dev. As far as I can see, nothing in the vhost_dev_start()/vhost_net_start() path re-applies it.

So with tap,vhost=on,poll-us=N, after cpr_exec_cb() calls vhost_net_cpr_reset_owners() and execvp() fails, vhost_net_cpr_set_owners() restores only the vring call and err fds. Could the poll-us setting then be silently dropped on the old QEMU's device?

If so, the comment "vhost_net_start() sets everything up again when the VM resumes" does not hold for this setting.

I did not verify in the kernel whether VHOST_RESET_OWNER actually clears vq->busyloop_timeout, so this depends on that behavior. Could the timeout be saved and replayed here, or the comment and commit message be adjusted?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=27