Re: [PATCH hci-dev v3 2/2]vfio/migration: don't touch the device on guest shutdown #VSTOR-153976

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio/migration: shutdown skip leaves PRE_COPY device unstopped on pause
- [Medium] vfio/migration: skip-stop predicate matches every pause-action SHUTDOWN, not only guest shutdowns
--

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

vfio/migration: don't touch the device on guest shutdown

This patch makes vfio_vmstate_change_prepare() and vfio_vmstate_change()
leave the VFIO device untouched when the VM stops in RUN_STATE_SHUTDOWN
and the shutdown action is "pause" (-no-shutdown). It avoids moving the
device to STOP on a libvirt soft reboot, which confuses the NVIDIA vGPU
manager. A quit or a migration of the stopped VM is expected to stop the
device through vfio_migration_stop_after_shutdown().

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index bfafc14e5b94..454c4e3f553a 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> +/*
> + * The guest has shut down and QEMU keeps the VM stopped (-no-shutdown). The
> + * guest has released the device, and only a reset, a quit or a migration can
> + * follow. The device is left running here: the reset resets it, and the two
> + * other paths stop it with vfio_migration_stop_after_shutdown().
> + *
> + * The same run state with the "poweroff" action is a quit of a running VM,
> + * which stops the device as usual.
> + */
> +static bool vfio_vm_stopped_by_guest_shutdown(bool running, RunState state)
> +{
> +    return !running && state == RUN_STATE_SHUTDOWN &&
> +           qemu_shutdown_action_is_pause();
> +}

[Severity: Medium]
Does this predicate cover only guest-initiated shutdowns?

It tests only !running, RUN_STATE_SHUTDOWN and
qemu_shutdown_action_is_pause().  It does not look at the ShutdownCause.

In main_loop_should_exit() the pause action leads to a single call:

    if (shutdown_action == SHUTDOWN_ACTION_PAUSE) {
        vm_stop(RUN_STATE_SHUTDOWN);

This is reached for every shutdown cause.  That includes
SHUTDOWN_CAUSE_HOST_UI, SHUTDOWN_CAUSE_HOST_ERROR and
SHUTDOWN_CAUSE_GUEST_PANIC requested directly by hw/misc/virt_ctrl.c.  In
those cases the guest driver may not have run its shutdown sequence.

The device is then left RUNNING, not P2P-quiesced and not STOP, while the
vCPUs are stopped.  The claims "The guest has released the device" and
"There is nothing to quiesce" are not established for these causes.

The predicate also reads the global shutdown_action, which qmp_quit() and
qmp_set_action() change at runtime:

    shutdown_action = SHUTDOWN_ACTION_POWEROFF;
    qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_QMP_QUIT);

Could the stop-time decision and a later decision then disagree?  Would
shutdown_caused_by_guest(cause), already used in qemu_system_shutdown(), or
a flag recorded at stop time be a more precise discriminator?

[ ... ]

> @@ -924,6 +939,10 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>          return;
>      }
>  
> +    if (vfio_vm_stopped_by_guest_shutdown(running, state)) {
> +        return;
> +    }
> +

[Severity: Medium]
Can this early return leave a device that is mid-precopy running?

The predicate does not look at migration->device_state.  Take a precopy
migration that is active, with vfio_save_setup() having put the device in
PRE_COPY, and then a guest shutdown with -no-shutdown.  Before this patch
the device went:

    PRE_COPY -> PRE_COPY_P2P -> STOP

Now both vfio_vmstate_change_prepare() and vfio_vmstate_change() return
early, and the device stays in PRE_COPY while the VM is stopped.

At switchover, migration_stop_vm() calls
vm_stop_force_state(RUN_STATE_FINISH_MIGRATE).  The runstate is already
stopped, so that only calls runstate_set() and sends no vm_state_notify.
Then vfio_save_complete_precopy() requests STOP_COPY with a recover state
of STOP:

    /* We reach here with device state STOP or STOP_COPY only */
    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
                                   VFIO_DEVICE_STATE_STOP, &local_err);

Does that comment still hold, given that the device can arrive in PRE_COPY?
Is STOP the right recover state in that case?

On quit, vfio_migration_shutdown_notifier() calls
vfio_migration_stop_after_shutdown(), which has this check:

    if (!runstate_check(RUN_STATE_SHUTDOWN) ||
        migration->device_state != VFIO_DEVICE_STATE_RUNNING) {
        return 0;
    }

That returns 0 for PRE_COPY, PRE_COPY_P2P and RUNNING_P2P.  The device is
then closed in PRE_COPY, or moved to RUNNING by the
MIG_EVENT_PRECOPY_FAILED handler.  vfio_save_setup() already ran before the
shutdown, so its call to vfio_migration_stop_after_shutdown() does not help.

The comment on vfio_vm_stopped_by_guest_shutdown() says "the two other
paths stop it with vfio_migration_stop_after_shutdown()", and the helper's
own comment says "Nothing else leaves a running device in the SHUTDOWN run
state".  Do those statements hold for a migration that was already in
precopy?

Would it be better to skip only when device_state is
VFIO_DEVICE_STATE_RUNNING, or to extend vfio_migration_stop_after_shutdown()
to cover PRE_COPY, PRE_COPY_P2P and RUNNING_P2P?

[ ... ]

> @@ -961,6 +980,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>          return;
>      }
>  
> +    if (vfio_vm_stopped_by_guest_shutdown(running, state)) {
> +        return;
> +    }
> +

[Severity: Medium]
The same early return in vfio_vmstate_change() skips the STOP transition
for a device in PRE_COPY, as described above, and for the non-guest
shutdown causes listed in the first comment.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109154346.153153-1-andrey.drobyshev@virtuozzo.com?part=2