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.sist...@oracle.com
> <mailto:steven.sist...@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.sist...@oracle.com
> <mailto:steven.sist...@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.sist...@oracle.com
> <mailto:steven.sist...@oracle.com>>
> +M: Mark Kanda <mark.ka...@oracle.com <mailto:mark.ka...@oracle.com>>
> +S: Maintained
> +F: include/migration/cpr.h
> +F: migration/cpr.c
> +
> Record/replay
> M: Pavel Dovgalyuk <pavel.dovga...@ispras.ru
> <mailto:pavel.dovga...@ispras.ru>>
> R: Paolo Bonzini <pbonz...@redhat.com <mailto:pbonz...@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
