Re: [PATCH V5 02/25] cpr: reboot mode

[Steven Sistare]

Will do for all - steve

On 7/8/2021 8:25 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 7, 2021 at 9:45 PM Steve Sistare <steven.sistare@oracle.com 
> <mailto:steven.sistare@oracle.com>> wrote:
> 
>     Provide the cprsave and cprload functions for live update.  These save and
>     restore VM state, with minimal guest pause time, so that qemu may be 
> updated
>     to a new version in between.
> 
>     cprsave stops the VM and saves vmstate to an ordinary file.  It supports 
> any
>     type of guest image and block device, but the caller must not modify guest
>     block devices between cprsave and cprload.
> 
>     cprsave supports several modes, the first of which is reboot.  In this 
> mode,
>     the caller invokes cprsave and then terminates qemu.  The caller may then
>     update the host kernel and system software and reboot.  The caller resumes
>     the guest by running qemu with the same arguments as the original process
>     and invoking cprload.  To use this mode, guest ram must be mapped to a
>     persistent shared memory file such as /dev/dax0.0 or /dev/shm PKRAM.
> 
>     The reboot mode supports vfio devices if the caller first suspends the
>     guest, such as by issuing guest-suspend-ram to the qemu guest agent.  The
>     guest drivers' suspend methods flush outstanding requests and 
> re-initialize
>     the devices, and thus there is no device state to save and restore.
> 
>     cprload loads state from the file.  If the VM was running at cprsave time,
>     then VM execution resumes.  If the VM was suspended at cprsave time, then
>     the caller must issue a system_wakeup command to resume.
> 
>     Signed-off-by: Steve Sistare <steven.sistare@oracle.com 
> <mailto:steven.sistare@oracle.com>>
>     ---
>      MAINTAINERS               |   7 +++
>      include/migration/cpr.h   |  17 ++++++
>      include/sysemu/runstate.h |   1 +
>      migration/cpr.c           | 149 
> ++++++++++++++++++++++++++++++++++++++++++++++
>      migration/meson.build     |   1 +
>      migration/savevm.h        |   2 +
>      softmmu/runstate.c        |  21 ++++++-
>      7 files changed, 197 insertions(+), 1 deletion(-)
>      create mode 100644 include/migration/cpr.h
>      create mode 100644 migration/cpr.c
> 
>     diff --git a/MAINTAINERS b/MAINTAINERS
>     index 684142e..c3573aa 100644
>     --- a/MAINTAINERS
>     +++ b/MAINTAINERS
>     @@ -2858,6 +2858,13 @@ F: net/colo*
>      F: net/filter-rewriter.c
>      F: net/filter-mirror.c
> 
>     +CPR
>     +M: Steve Sistare <steven.sistare@oracle.com 
> <mailto:steven.sistare@oracle.com>>
>     +M: Mark Kanda <mark.kanda@oracle.com <mailto:mark.kanda@oracle.com>>
>     +S: Maintained
>     +F: include/migration/cpr.h
>     +F: migration/cpr.c
>     +
>      Record/replay
>      M: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru 
> <mailto:pavel.dovgaluk@ispras.ru>>
>      R: Paolo Bonzini <pbonzini@redhat.com <mailto:pbonzini@redhat.com>>
>     diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>     new file mode 100644
>     index 0000000..bffee19
>     --- /dev/null
>     +++ b/include/migration/cpr.h
>     @@ -0,0 +1,17 @@
>     +/*
>     + * Copyright (c) 2021 Oracle and/or its affiliates.
>     + *
>     + * This work is licensed under the terms of the GNU GPL, version 2.
>     + * See the COPYING file in the top-level directory.
>     + */
>     +
>     +#ifndef MIGRATION_CPR_H
>     +#define MIGRATION_CPR_H
>     +
>     +#include "qapi/qapi-types-cpr.h"
>     +
>     +void cprsave(const char *file, CprMode mode, Error **errp);
> 
> 
> I'd rather use "path" or "filename".
> 
>     +void cprexec(strList *args, Error **errp);
>     +void cprload(const char *file, Error **errp);
> 
> 
> same
> 
> It's recommended to return a bool value TRUE for success.
> (see include/qapi/error.h)
> 
>     +
>     +#endif
>     diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>     index a535691..ed4b735 100644
>     --- a/include/sysemu/runstate.h
>     +++ b/include/sysemu/runstate.h
>     @@ -51,6 +51,7 @@ void qemu_system_reset_request(ShutdownCause reason);
>      void qemu_system_suspend_request(void);
>      void qemu_register_suspend_notifier(Notifier *notifier);
>      bool qemu_wakeup_suspend_enabled(void);
>     +void qemu_system_start_on_wake_request(void);
> 
> 
> I suggest introducing the function in a preliminary commit.
> 
> Also for consistency with the rest of symbols, use "wakeup".
> 
>      void qemu_system_wakeup_request(WakeupReason reason, Error **errp);
>      void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
>      void qemu_register_wakeup_notifier(Notifier *notifier);
>     diff --git a/migration/cpr.c b/migration/cpr.c
>     new file mode 100644
>     index 0000000..c5bad8a
>     --- /dev/null
>     +++ b/migration/cpr.c
>     @@ -0,0 +1,149 @@
>     +/*
>     + * Copyright (c) 2021 Oracle and/or its affiliates.
>     + *
>     + * This work is licensed under the terms of the GNU GPL, version 2.
>     + * See the COPYING file in the top-level directory.
>     + */
>     +
>     +#include "qemu/osdep.h"
>     +#include "monitor/monitor.h"
>     +#include "migration.h"
>     +#include "migration/snapshot.h"
>     +#include "chardev/char.h"
>     +#include "migration/misc.h"
>     +#include "migration/cpr.h"
>     +#include "migration/global_state.h"
>     +#include "qemu-file-channel.h"
>     +#include "qemu-file.h"
>     +#include "savevm.h"
>     +#include "qapi/error.h"
>     +#include "qapi/qmp/qerror.h"
>     +#include "qemu/error-report.h"
>     +#include "io/channel-buffer.h"
>     +#include "io/channel-file.h"
>     +#include "sysemu/cpu-timers.h"
>     +#include "sysemu/runstate.h"
>     +#include "sysemu/runstate-action.h"
>     +#include "sysemu/sysemu.h"
>     +#include "sysemu/replay.h"
>     +#include "sysemu/xen.h"
>     +#include "hw/vfio/vfio-common.h"
>     +#include "hw/virtio/vhost.h"
>     +
>     +QEMUFile *qf_file_open(const char *path, int flags, int mode,
>     +                              const char *name, Error **errp)
> 
> 
> None of our functions have qf_ prefix. We are not very consistent with 
> QEMUFile functions, but I suggest to spell it out qemu_file_open().
> 
> Also, it should probably be in migration/qemu-file.c.
> 
>     +{
> 
> 
> I'd ERRP_GUARD on every function with an errp argument.
> 
>     +    QIOChannelFile *fioc;
> 
> 
> Let's not miss an opportunity to use g_auto
>     g_autoptr(QIOChannelFile) fioc = NULL;
> 
>     +    QIOChannel *ioc;
>     +    QEMUFile *f;
>     +
>     +    if (flags & O_RDWR) {
>     +        error_setg(errp, "qf_file_open %s: O_RDWR not supported", path);
>     +        return 0;
>     +    }
>     +
>     +    fioc = qio_channel_file_new_path(path, flags, mode, errp);
>     +    if (!fioc) {
>     +        return 0;
>     +    }
>     +
>     +    ioc = QIO_CHANNEL(fioc);
>     +    qio_channel_set_name(ioc, name);
>     +    f = (flags & O_WRONLY) ? qemu_fopen_channel_output(ioc) :
>     +                             qemu_fopen_channel_input(ioc);
> 
>  +    object_unref(OBJECT(fioc));
>  
> With g_auto, can be removed, and value returned directly.
> 
>     +    return f;
>     +}
>     +
>     +void cprsave(const char *file, CprMode mode, Error **errp)
>     +{
>     +    int ret;
>     +    QEMUFile *f;
>     +    int saved_vm_running = runstate_is_running();
>     +
>     +    if (mode == CPR_MODE_REBOOT && qemu_ram_volatile(errp)) {
>     +        return;
>     +    }
>     +
>     +    if (migrate_colo_enabled()) {
>     +        error_setg(errp, "error: cprsave does not support x-colo");
> 
> 
> Remove error:
> 
>     +        return;
>     +    }
>     +
>     +    if (replay_mode != REPLAY_MODE_NONE) {
>     +        error_setg(errp, "error: cprsave does not support replay");
> 
> 
> same
> 
>     +        return;
>     +    }
>     +
>     +    f = qf_file_open(file, O_CREAT | O_WRONLY | O_TRUNC, 0600, 
> "cprsave", errp);
>     +    if (!f) {
>     +        return;
>     +    }
>     +
>     +    if (global_state_store()) {
>     +        error_setg(errp, "Error saving global state");
>     +        qemu_fclose(f);
>     +        return;
>     +    }
> 
> 
> Could be called before opening cprsave file?
> 
>     +    if (runstate_check(RUN_STATE_SUSPENDED)) {
>     +        /* Update timers_state before saving.  Suspend did not so do. */
>     +        cpu_disable_ticks();
>     +    }
>     +    vm_stop(RUN_STATE_SAVE_VM);
>     +
>     +    ret = qemu_save_device_state(f);
>     +    qemu_fclose(f);
>     +    if (ret < 0) {
>     +        error_setg(errp, "Error %d while saving VM state", ret);
>     +        goto err;
> 
> 
> Needless goto / labels.
>  
> 
>     +    }
>     +
>     +    goto done;
>     +
>     +err:
>     +    if (saved_vm_running) {
>     +        vm_start();
>     +    }
>     +done:
>     +    return;
>     +}
>     +
>     +void cprload(const char *file, Error **errp)
>     +{
>     +    QEMUFile *f;
>     +    int ret;
>     +    RunState state;
>     +
>     +    if (runstate_is_running()) {
>     +        error_setg(errp, "cprload called for a running VM");
>     +        return;
>     +    }
>     +
>     +    f = qf_file_open(file, O_RDONLY, 0, "cprload", errp);
>     +    if (!f) {
>     +        return;
>     +    }
>     +
>     +    if (qemu_get_be32(f) != QEMU_VM_FILE_MAGIC ||
>     +        qemu_get_be32(f) != QEMU_VM_FILE_VERSION) {
>     +        error_setg(errp, "error: %s is not a vmstate file", file);
> 
> 
> f is leaked
> 
>     +        return;
>     +    }
>     +
>     +    ret = qemu_load_device_state(f);
>     +    qemu_fclose(f);
>     +    if (ret < 0) {
>     +        error_setg(errp, "Error %d while loading VM state", ret);
>     +        return;
>     +    }
>     +
>     +    state = global_state_get_runstate();
>     +    if (state == RUN_STATE_RUNNING) {
>     +        vm_start();
>     +    } else {
>     +        runstate_set(state);
>     +        if (runstate_check(RUN_STATE_SUSPENDED)) {
>     +            qemu_system_start_on_wake_request();
>     +        }
>     +    }
>     +}
>     diff --git a/migration/meson.build b/migration/meson.build
>     index f8714dc..fd59281 100644
>     --- a/migration/meson.build
>     +++ b/migration/meson.build
>     @@ -15,6 +15,7 @@ softmmu_ss.add(files(
>        'channel.c',
>        'colo-failover.c',
>        'colo.c',
>     +  'cpr.c',
>        'exec.c',
>        'fd.c',
>        'global_state.c',
>     diff --git a/migration/savevm.h b/migration/savevm.h
>     index 6461342..ce5d710 100644
>     --- a/migration/savevm.h
>     +++ b/migration/savevm.h
>     @@ -67,5 +67,7 @@ int qemu_loadvm_state_main(QEMUFile *f, 
> MigrationIncomingState *mis);
>      int qemu_load_device_state(QEMUFile *f);
>      int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>              bool in_postcopy, bool inactivate_disks);
>     +QEMUFile *qf_file_open(const char *path, int flags, int mode,
>     +                       const char *name, Error **errp);
> 
>      #endif
>     diff --git a/softmmu/runstate.c b/softmmu/runstate.c
>     index 10d9b73..7fe4967 100644
>     --- a/softmmu/runstate.c
>     +++ b/softmmu/runstate.c
>     @@ -115,6 +115,8 @@ static const RunStateTransition 
> runstate_transitions_def[] = {
>          { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
>          { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
>          { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
>     +    { RUN_STATE_PRELAUNCH, RUN_STATE_SUSPENDED },
>     +    { RUN_STATE_PRELAUNCH, RUN_STATE_PAUSED },
> 
>          { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
>          { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
>     @@ -335,6 +337,7 @@ void vm_state_notify(bool running, RunState state)
>          }
>      }
> 
>     +static bool start_on_wake_requested;
>      static ShutdownCause reset_requested;
>      static ShutdownCause shutdown_requested;
>      static int shutdown_signal;
>     @@ -562,6 +565,11 @@ void qemu_register_suspend_notifier(Notifier 
> *notifier)
>          notifier_list_add(&suspend_notifiers, notifier);
>      }
> 
>     +void qemu_system_start_on_wake_request(void)
>     +{
>     +    start_on_wake_requested = true;
>     +}
>     +
>      void qemu_system_wakeup_request(WakeupReason reason, Error **errp)
>      {
>          trace_system_wakeup_request(reason);
>     @@ -574,7 +582,18 @@ void qemu_system_wakeup_request(WakeupReason reason, 
> Error **errp)
>          if (!(wakeup_reason_mask & (1 << reason))) {
>              return;
>          }
>     -    runstate_set(RUN_STATE_RUNNING);
>     +
>     +    /*
>     +     * Must call vm_start if it has never been called, to invoke the 
> state
>     +     * change callbacks for the first time.
>     +     */
>     +    if (start_on_wake_requested) {
>     +        start_on_wake_requested = false;
>     +        vm_start();
>     +    } else {
>     +        runstate_set(RUN_STATE_RUNNING);
>     +    }
>     +
>          wakeup_reason = reason;
>          qemu_notify_event();
>      }
>     -- 
>     1.8.3.1
> 
> 
> 
> 
> -- 
> Marc-André Lureau