Re: [PATCH V4 04/11] migration: preserve suspended for snapshot

[Peter Xu <peterx@redhat.com>]

On Tue, Aug 29, 2023 at 11:17:59AM -0700, Steve Sistare wrote:
> Restoring a snapshot can break a suspended guest.
> 
> If a guest is suspended and saved to a snapshot using savevm, and qemu
> is terminated and restarted with the -S option, then loadvm does not
> restore the guest.  The runstate is running, but the guest is not, because
> vm_start was not called.  The root cause is that loadvm does not restore
> the runstate (eg suspended) from global_state loaded from the state file.
> 
> Restore the runstate, and allow the new state transitions that are possible.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  migration/savevm.c | 1 +
>  softmmu/runstate.c | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index eba3653..7b9c477 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -3194,6 +3194,7 @@ bool load_snapshot(const char *name, const char *vmstate,
>      }
>      aio_context_acquire(aio_context);
>      ret = qemu_loadvm_state(f);
> +    migrate_set_runstate();

I see that some load_snapshot() callers manage the vm states on their own.
Take snapshot_load_job_bh() as an example:

    s->ret = load_snapshot(s->tag, s->vmstate, true, s->devices, s->errp);
    if (s->ret && orig_vm_running) {
        vm_start();
    }

I assume you wanted to unify the state changes here.  Need to fix the
callers too?

>      migration_incoming_state_destroy();
>      aio_context_release(aio_context);
>  
> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index f3bd862..21d7407 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -77,6 +77,8 @@ typedef struct {
>  
>  static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> +    { RUN_STATE_PRELAUNCH, RUN_STATE_PAUSED },
> +    { RUN_STATE_PRELAUNCH, RUN_STATE_SUSPENDED },
>  
>      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
>      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },

Many of the call sites also starts loadvm under RUN_STATE_RESTORE_VM.  Do
we need more entries for that?

-- 
Peter Xu