Re: [PATCH QEMU HCI-8.1 v7 31/35] 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 2 potential issue(s) to consider:
- [Low] migration: missing vhost_net-stub.c stubs for vhost_net_cpr_set_owners()
- [Low] vhost-net: cpr-exec failure path does not restore per-vring busyloop timeout
--

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

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

This patch makes cpr_exec_cb() call a new function,
vhost_net_cpr_set_owners(), when execve() fails. The function takes back
the kernel vhost-net devices that vhost_net_cpr_reset_owners() released. It
does this with VHOST_SET_OWNER, and it sets the vring call and err fds
again.

> 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

[ ... ]

> +void vhost_net_cpr_set_owners(void)
> +{
> +    NetClientState *nc;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        VHostNetState *net = get_vhost_net(nc);
> +
> +        if (net && net->cpr_owner_reset) {

[ ... ]

> +            /* 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]
VHOST_RESET_OWNER also clears the per-vring busyloop timeout in the kernel.
This loop restores only the call and err fds. Can the busyloop timeout be
lost after a failed execve()?

vhost_dev_init() is the only place that applies it:

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 is not saved in vhost_dev or vhost_net. Neither vhost_net_start()
nor vhost_dev_start() applies it again. After vhost_net_cpr_set_owners(),
the VM would therefore keep running without the configured busyloop_timeout.
Should this loop re-apply it?

> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c

[ ... ]

> @@ -259,6 +259,8 @@ fail:
>      if (cpr_ram_rdonly) {
>          cpr_ram_restore_rw();
>      }
> +    /* Still the same mm, take back the vhost-net devices we released */
> +    vhost_net_cpr_set_owners();

[Severity: Low]
Will this link in builds that use hw/net/vhost_net-stub.c?

migration/cpr-exec.c is always compiled, and cpr_exec_cb() calls
vhost_net_cpr_set_owners() without any #ifdef guard.

hw/net/meson.build compiles vhost_net.c only when have_vhost_net is set and
CONFIG_VIRTIO_NET is enabled. Otherwise it compiles vhost_net-stub.c. That
covers --disable-vhost-net builds, non-Linux hosts, and builds without
CONFIG_VIRTIO_NET. The stub file has no stub for this function.

This problem wasn't introduced by this patch for vhost_net_cpr_reset_owners(),
which has the same missing stub. The patch adds a second unresolved symbol
and does not add either stub. Could stubs for both functions be added to
vhost_net-stub.c?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-vfio_v1-v7-0-4467fdccbbe6@virtuozzo.com?part=31