[Qemu-devel] [PATCH v9 00/10] replay additions

[Qemu-devel] [PATCH v9 00/10] replay additions

[Pavel Dovgalyuk]

This set of patches fixes some vmstate creation (and loading) issues
in record/replay modes:
 - VM start/stop fixes in replay mode
 - overlay creation for blkreplay filter
 - fixes for vmstate save/load in record/replay mode
 - fixes for host clock vmstate
 - fixes for icount timers vmstate

v9 changes:
 - Added vmstate for host clock parameters and icount timers
 - Fixed replay event queue issues
 - Fixed vm stopping while recording or replaying the execution
 - Fixed save/load vmstate issues in record/replay mode

v8 changes:
 - Refined replay exception processing (as suggested by Paolo Bonzini)
 - Saving/restoring static variable for APIC only once (as suggested by Paolo Bonzini)
 - Removed already queued patches
 - Minor fixes

v7 changes:
 - Fixed exception replaying when TB cache is full and
   when tb_find is called when there are no instructions about to execute
 - Added record/replay for audio devices

v6 changes:
 - Added overlay creation for blkreplay driver
 - Fixed vmstate loading for apic and rtc
 - Fixed instruction counting for apic instruction patching

v5 changes:
 - Recording is stopped when initial snapshot cannot be created
 - Minor changes

v4 changes:
 - Overlay option is removed from blkreplay driver (as suggested by Paolo Bonzini)
 - Minor changes

v3 changes:
 - Added rrsnapshot option for specifying the initial snapshot name (as suggested by Paolo Bonzini)
 - Minor changes

---

Pavel Dovgalyuk (10):
      block: implement bdrv_snapshot_goto for blkreplay
      blkreplay: create temporary overlay for underlaying devices
      replay: disable default snapshot for record/replay
      replay: fix processing async events
      replay: fixed replay_enable_events
      replay: fix save/load vm for non-empty queue
      replay: added replay log format description
      replay: make safe vmstop at record/replay
      replay: save prior value of the host clock
      icount: fixed saving/restoring of icount warp timers


 block/blkreplay.c        |   73 +++++++++++++++++++++++++++++++++++++
 cpus.c                   |   90 +++++++++++++++++++++++++++++++++++-----------
 docs/replay.txt          |   69 +++++++++++++++++++++++++++++++++++
 include/qemu/timer.h     |   14 +++++++
 include/sysemu/replay.h  |    3 ++
 migration/savevm.c       |   13 +++++++
 replay/replay-events.c   |   12 ++++--
 replay/replay-internal.h |    2 +
 replay/replay-snapshot.c |    9 +++++
 stubs/replay.c           |    1 +
 util/qemu-timer.c        |   12 ++++++
 vl.c                     |   10 ++++-
 12 files changed, 279 insertions(+), 29 deletions(-)

-- 
Pavel Dovgalyuk

[Qemu-devel] [PATCH v9 01/10] block: implement bdrv_snapshot_goto for blkreplay

[Pavel Dovgalyuk]

This patch enables making snapshots with blkreplay used in
block devices.
This function is required to make bdrv_snapshot_goto without
calling .bdrv_open which is not implemented.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 block/blkreplay.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/blkreplay.c b/block/blkreplay.c
index e1102119fb..d13a98d691 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -130,6 +130,12 @@ static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs)
     return ret;
 }
 
+static int blkreplay_snapshot_goto(BlockDriverState *bs,
+                                   const char *snapshot_id)
+{
+    return bdrv_snapshot_goto(bs->file->bs, snapshot_id);
+}
+
 static BlockDriver bdrv_blkreplay = {
     .format_name            = "blkreplay",
     .protocol_name          = "blkreplay",
@@ -146,6 +152,8 @@ static BlockDriver bdrv_blkreplay = {
     .bdrv_co_pwrite_zeroes  = blkreplay_co_pwrite_zeroes,
     .bdrv_co_pdiscard       = blkreplay_co_pdiscard,
     .bdrv_co_flush          = blkreplay_co_flush,
+
+    .bdrv_snapshot_goto     = blkreplay_snapshot_goto,
 };
 
 static void bdrv_blkreplay_init(void)

[Qemu-devel] [PATCH v9 02/10] blkreplay: create temporary overlay for underlaying devices

[Pavel Dovgalyuk]

This patch allows using '-snapshot' behavior in record/replay mode.
blkreplay layer creates temporary overlays on top of underlaying
disk images. It is needed, because creating an overlay over blkreplay
breaks the determinism.
This patch creates similar temporary overlay (when it is needed)
under the blkreplay driver. Therefore all block operations are controlled
by blkreplay.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 block/blkreplay.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 stubs/replay.c    |    1 +
 vl.c              |    2 +-
 3 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/block/blkreplay.c b/block/blkreplay.c
index d13a98d691..a9bab72bfc 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -14,12 +14,69 @@
 #include "block/block_int.h"
 #include "sysemu/replay.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qstring.h"
 
 typedef struct Request {
     Coroutine *co;
     QEMUBH *bh;
 } Request;
 
+static BlockDriverState *blkreplay_append_snapshot(BlockDriverState *bs,
+                                                   Error **errp)
+{
+    int ret;
+    BlockDriverState *bs_snapshot;
+    int64_t total_size;
+    QemuOpts *opts = NULL;
+    char tmp_filename[PATH_MAX + 1];
+    QDict *snapshot_options = qdict_new();
+
+    /* Prepare options QDict for the overlay file */
+    qdict_put(snapshot_options, "file.driver", qstring_from_str("file"));
+    qdict_put(snapshot_options, "driver", qstring_from_str("qcow2"));
+
+    /* Create temporary file */
+    ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not get temporary filename");
+        goto out;
+    }
+    qdict_put(snapshot_options, "file.filename",
+              qstring_from_str(tmp_filename));
+
+    /* Get the required size from the image */
+    total_size = bdrv_getlength(bs);
+    if (total_size < 0) {
+        error_setg_errno(errp, -total_size, "Could not get image size");
+        goto out;
+    }
+
+    opts = qemu_opts_create(bdrv_qcow2.create_opts, NULL, 0, &error_abort);
+    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, &error_abort);
+    ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, errp);
+    qemu_opts_del(opts);
+    if (ret < 0) {
+        error_prepend(errp, "Could not create temporary overlay '%s': ",
+                      tmp_filename);
+        goto out;
+    }
+
+    bs_snapshot = bdrv_open(NULL, NULL, snapshot_options,
+                            BDRV_O_RDWR | BDRV_O_TEMPORARY, errp);
+    snapshot_options = NULL;
+    if (!bs_snapshot) {
+        goto out;
+    }
+
+    bdrv_append(bs_snapshot, bs, errp);
+
+    return bs_snapshot;
+
+out:
+    QDECREF(snapshot_options);
+    return NULL;
+}
+
 static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
@@ -35,6 +92,14 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    /* Add temporary snapshot to preserve the image */
+    if (!replay_snapshot
+        && !blkreplay_append_snapshot(bs->file->bs, &local_err)) {
+        ret = -EINVAL;
+        error_propagate(errp, local_err);
+        goto fail;
+    }
+
     ret = 0;
 fail:
     if (ret < 0) {
diff --git a/stubs/replay.c b/stubs/replay.c
index 9c8aa48c9c..9991ee5120 100644
--- a/stubs/replay.c
+++ b/stubs/replay.c
@@ -3,6 +3,7 @@
 #include "sysemu/sysemu.h"
 
 ReplayMode replay_mode;
+char *replay_snapshot;
 
 int64_t replay_save_clock(unsigned int kind, int64_t clock)
 {
diff --git a/vl.c b/vl.c
index f46e070e0d..eafb3ff7a4 100644
--- a/vl.c
+++ b/vl.c
@@ -4492,7 +4492,7 @@ int main(int argc, char **argv, char **envp)
         qapi_free_BlockdevOptions(bdo->bdo);
         g_free(bdo);
     }
-    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
+    if (snapshot) {
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
                           NULL, NULL);
     }

[Qemu-devel] [PATCH v9 03/10] replay: disable default snapshot for record/replay

[Pavel Dovgalyuk]

This patch disables setting '-snapshot' option on by default
in record/replay mode. This is needed for creating vmstates in record
and replay modes.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 vl.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index eafb3ff7a4..222610ab7a 100644
--- a/vl.c
+++ b/vl.c
@@ -3168,7 +3168,13 @@ int main(int argc, char **argv, char **envp)
                 drive_add(IF_PFLASH, -1, optarg, PFLASH_OPTS);
                 break;
             case QEMU_OPTION_snapshot:
-                snapshot = 1;
+                {
+                    Error *blocker = NULL;
+                    snapshot = 1;
+                    error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED,
+                               "-snapshot");
+                    replay_add_blocker(blocker);
+                }
                 break;
             case QEMU_OPTION_hdachs:
                 {

[Qemu-devel] [PATCH v9 04/10] replay: fix processing async events

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

Asynchronous events saved at checkpoints may invoke
callbacks when processed. These callbacks may also generate/read
new events (e.g. clock reads). Therefore event processing flag must be
reset before callback invocation.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 replay/replay-events.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/replay/replay-events.c b/replay/replay-events.c
index 94a6dcccfc..768b505f3d 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -295,13 +295,13 @@ void replay_read_events(int checkpoint)
         if (!event) {
             break;
         }
+        replay_finish_event();
+        read_event_kind = -1;
         replay_mutex_unlock();
         replay_run_event(event);
         replay_mutex_lock();
 
         g_free(event);
-        replay_finish_event();
-        read_event_kind = -1;
     }
 }
 

[Qemu-devel] [PATCH v9 05/10] replay: fixed replay_enable_events

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch fixes assignment to internal events_enabled variable.
Now it is set only in record/replay mode. This affects the behavior
of the external functions that check this flag.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 replay/replay-events.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/replay/replay-events.c b/replay/replay-events.c
index 768b505f3d..e858254074 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -67,7 +67,9 @@ static void replay_run_event(Event *event)
 
 void replay_enable_events(void)
 {
-    events_enabled = true;
+    if (replay_mode != REPLAY_MODE_NONE) {
+        events_enabled = true;
+    }
 }
 
 bool replay_has_events(void)
@@ -141,7 +143,7 @@ void replay_add_event(ReplayAsyncEventKind event_kind,
 
 void replay_bh_schedule_event(QEMUBH *bh)
 {
-    if (replay_mode != REPLAY_MODE_NONE && events_enabled) {
+    if (events_enabled) {
         uint64_t id = replay_get_current_step();
         replay_add_event(REPLAY_ASYNC_EVENT_BH, bh, NULL, id);
     } else {
@@ -161,7 +163,7 @@ void replay_add_input_sync_event(void)
 
 void replay_block_event(QEMUBH *bh, uint64_t id)
 {
-    if (replay_mode != REPLAY_MODE_NONE && events_enabled) {
+    if (events_enabled) {
         replay_add_event(REPLAY_ASYNC_EVENT_BLOCK, bh, NULL, id);
     } else {
         qemu_bh_schedule(bh);

[Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch does not allows saving/loading vmstate when
replay events queue is not empty. There is no reliable
way to save events queue, because it describes internal
coroutine state. Therefore saving and loading operations
should be deferred to another record/replay step.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 include/sysemu/replay.h  |    3 +++
 migration/savevm.c       |   13 +++++++++++++
 replay/replay-snapshot.c |    6 ++++++
 3 files changed, 22 insertions(+)

diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index f1c0712795..5c9db2a2ef 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -164,5 +164,8 @@ void replay_audio_in(int *recorded, void *samples, int *wpos, int size);
 /*! Called at the start of execution.
     Loads or saves initial vmstate depending on execution mode. */
 void replay_vmstate_init(void);
+/*! Called to ensure that replay state is consistent and VM snapshot
+    can be created */
+bool replay_can_snapshot(void);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 03ae1bdeb4..358d22170d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -54,6 +54,7 @@
 #include "qemu/cutils.h"
 #include "io/channel-buffer.h"
 #include "io/channel-file.h"
+#include "sysemu/replay.h"
 
 #ifndef ETH_P_RARP
 #define ETH_P_RARP 0x8035
@@ -2083,6 +2084,12 @@ int save_vmstate(Monitor *mon, const char *name)
     Error *local_err = NULL;
     AioContext *aio_context;
 
+    if (!replay_can_snapshot()) {
+        monitor_printf(mon, "Record/replay does not allow making snapshot right now. "
+                        "Try stopping at another step.\n");
+        return ret;
+    }
+
     if (!bdrv_all_can_snapshot(&bs)) {
         monitor_printf(mon, "Device '%s' is writable but does not "
                        "support snapshots.\n", bdrv_get_device_name(bs));
@@ -2244,6 +2251,12 @@ int load_vmstate(const char *name)
     AioContext *aio_context;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
+    if (!replay_can_snapshot()) {
+        error_report("Record/replay does not allow loading snapshot right now. "
+                     "Try stopping at another step.\n");
+        return -EINVAL;
+    }
+
     if (!bdrv_all_can_snapshot(&bs)) {
         error_report("Device '%s' is writable but does not support snapshots.",
                      bdrv_get_device_name(bs));
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 65e2d375c2..438d82ec33 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -76,3 +76,9 @@ void replay_vmstate_init(void)
         }
     }
 }
+
+bool replay_can_snapshot(void)
+{
+    return replay_mode == REPLAY_MODE_NONE
+        || !replay_has_events();
+}

[Qemu-devel] [PATCH v9 07/10] replay: added replay log format description

[Pavel Dovgalyuk]

This patch adds description of the replay log file format
into the docs/replay.txt.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 docs/replay.txt |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/docs/replay.txt b/docs/replay.txt
index 486c1e0e9d..c52407fe23 100644
--- a/docs/replay.txt
+++ b/docs/replay.txt
@@ -232,3 +232,72 @@ Audio devices
 Audio data is recorded and replay automatically. The command line for recording
 and replaying must contain identical specifications of audio hardware, e.g.:
  -soundhw ac97
+
+Replay log format
+-----------------
+
+Record/replay log consits of the header and the sequence of execution
+events. The header includes 4-byte replay version id and 8-byte reserved
+field. Version is updated every time replay log format changes to prevent
+using replay log created by another build of qemu.
+
+The sequence of the events describes virtual machine state changes.
+It includes all non-deterministic inputs of VM, synchronization marks and
+instruction counts used to correctly inject inputs at replay.
+
+Synchronization marks (checkpoints) are used for synchronizing qemu threads
+that perform operations with virtual hardware. These operations may change
+system's state (e.g., change some register or generate interrupt) and
+therefore should execute synchronously with CPU thread.
+
+Every event in the log includes 1-byte event id and optional arguments.
+When argument is an array, it is stored as 4-byte array length
+and corresponding number of bytes with data.
+Here is the list of events that are written into the log:
+
+ - EVENT_INSTRUCTION. Instructions executed since last event.
+   Argument: 4-byte number of executed instructions.
+ - EVENT_INTERRUPT. Used to synchronize interrupt processing.
+ - EVENT_EXCEPTION. Used to synchronize exception handling.
+ - EVENT_ASYNC. This is a group of events. They are always processed
+   together with checkpoints. When such an event is generated, it is
+   stored in the queue and processed only when checkpoint occurs.
+   Every such event is followed by 1-byte checkpoint id and 1-byte
+   async event id from the following list:
+     - REPLAY_ASYNC_EVENT_BH. Bottom-half callback. This event synchronizes
+       callbacks that affect virtual machine state, but normally called
+       asyncronously.
+       Argument: 8-byte operation id.
+     - REPLAY_ASYNC_EVENT_INPUT. Input device event. Contains
+       parameters of keyboard and mouse input operations
+       (key press/release, mouse pointer movement).
+       Arguments: 9-16 bytes depending of input event.
+     - REPLAY_ASYNC_EVENT_INPUT_SYNC. Internal input synchronization event.
+     - REPLAY_ASYNC_EVENT_CHAR_READ. Character (e.g., serial port) device input
+       initiated by the sender.
+       Arguments: 1-byte character device id.
+                  Array with bytes were read.
+     - REPLAY_ASYNC_EVENT_BLOCK. Block device operation. Used to synchronize
+       operations with disk and flash drives with CPU.
+       Argument: 8-byte operation id.
+     - REPLAY_ASYNC_EVENT_NET. Incoming network packet.
+       Arguments: 1-byte network adapter id.
+                  4-byte packet flags.
+                  Array with packet bytes.
+ - EVENT_SHUTDOWN. Occurs when user sends shutdown event to qemu,
+   e.g., by closing the window.
+ - EVENT_CHAR_WRITE. Used to synchronize character output operations.
+   Arguments: 4-byte output function return value.
+              4-byte offset in the output array.
+ - EVENT_CHAR_READ_ALL. Used to synchronize character input operations,
+   initiated by qemu.
+   Argument: Array with bytes that were read.
+ - EVENT_CHAR_READ_ALL_ERROR. Unsuccessful character input operation,
+   initiated by qemu.
+   Argument: 4-byte error code.
+ - EVENT_CLOCK + clock_id. Group of events for host clock read operations.
+   Argument: 8-byte clock value.
+ - EVENT_CHECKPOINT + checkpoint_id. Checkpoint for synchronization of
+   CPU, internal threads, and asynchronous input events. May be followed
+   by one or more EVENT_ASYNC events.
+ - EVENT_END. Last event in the log.

[Qemu-devel] [PATCH v9 08/10] replay: make safe vmstop at record/replay

[Pavel Dovgalyuk]

This patch disables bdrv flush/drain in record/replay mode.
When block request is in the replay queue it cannot be processed
with drain/flush until it is found in the log.
Therefore vm should just stop leaving unfinished operations
in the queue.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 cpus.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/cpus.c b/cpus.c
index 740b8dc3f8..f21e7f3166 100644
--- a/cpus.c
+++ b/cpus.c
@@ -932,9 +932,10 @@ static int do_vm_stop(RunState state)
         qapi_event_send_stop(&error_abort);
     }
 
-    bdrv_drain_all();
-    replay_disable_events();
-    ret = bdrv_flush_all();
+    if (!replay_events_enabled()) {
+        bdrv_drain_all();
+        ret = bdrv_flush_all();
+    }
 
     return ret;
 }

[Qemu-devel] [PATCH v9 09/10] replay: save prior value of the host clock

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch adds saving/restoring of the host clock field 'last'.
It is used in host clock calculation and therefore clock may
become incorrect when using restored vmstate.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 include/qemu/timer.h     |   14 ++++++++++++++
 replay/replay-internal.h |    2 ++
 replay/replay-snapshot.c |    3 +++
 util/qemu-timer.c        |   12 ++++++++++++
 4 files changed, 31 insertions(+)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 8a1eb74839..5e9adbbf01 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -251,6 +251,20 @@ bool qemu_clock_run_timers(QEMUClockType type);
  */
 bool qemu_clock_run_all_timers(void);
 
+/**
+ * qemu_clock_get_last:
+ *
+ * Returns last clock query time.
+ */
+uint64_t qemu_clock_get_last(QEMUClockType type);
+/**
+ * qemu_clock_set_last:
+ *
+ * Sets last clock query time.
+ */
+void qemu_clock_set_last(QEMUClockType type, uint64_t last);
+
+
 /*
  * QEMUTimerList
  */
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index ed66ed803c..738983e76a 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -77,6 +77,8 @@ typedef struct ReplayState {
         This counter is global, because requests from different
         block devices should not get overlapping ids. */
     uint64_t block_request_id;
+    /*! Prior value of the host clock */
+    uint64_t host_clock_last;
 } ReplayState;
 extern ReplayState replay_state;
 
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 438d82ec33..1a0b76b9ca 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -24,12 +24,14 @@ static void replay_pre_save(void *opaque)
 {
     ReplayState *state = opaque;
     state->file_offset = ftell(replay_file);
+    state->host_clock_last = qemu_clock_get_last(QEMU_CLOCK_HOST);
 }
 
 static int replay_post_load(void *opaque, int version_id)
 {
     ReplayState *state = opaque;
     fseek(replay_file, state->file_offset, SEEK_SET);
+    qemu_clock_set_last(QEMU_CLOCK_HOST, state->host_clock_last);
     /* If this was a vmstate, saved in recording mode,
        we need to initialize replay data fields. */
     replay_fetch_data_kind();
@@ -51,6 +53,7 @@ static const VMStateDescription vmstate_replay = {
         VMSTATE_UINT32(has_unread_data, ReplayState),
         VMSTATE_UINT64(file_offset, ReplayState),
         VMSTATE_UINT64(block_request_id, ReplayState),
+        VMSTATE_UINT64(host_clock_last, ReplayState),
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 82d56507a2..2ed1bf2778 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -622,6 +622,18 @@ int64_t qemu_clock_get_ns(QEMUClockType type)
     }
 }
 
+uint64_t qemu_clock_get_last(QEMUClockType type)
+{
+    QEMUClock *clock = qemu_clock_ptr(type);
+    return clock->last;
+}
+
+void qemu_clock_set_last(QEMUClockType type, uint64_t last)
+{
+    QEMUClock *clock = qemu_clock_ptr(type);
+    clock->last = last;
+}
+
 void qemu_clock_register_reset_notifier(QEMUClockType type,
                                         Notifier *notifier)
 {

[Qemu-devel] [PATCH v9 10/10] icount: fixed saving/restoring of icount warp timers

[Pavel Dovgalyuk]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch adds saving and restoring of the icount warp
timers in the vmstate.
It is needed because there timers affect the virtual clock value.
Therefore determinism of the execution in icount record/replay mode
depends on determinism of the timers.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 cpus.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 19 deletions(-)

diff --git a/cpus.c b/cpus.c
index f21e7f3166..5c955a709c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -118,16 +118,11 @@ static bool all_cpu_threads_idle(void)
 /* Protected by TimersState seqlock */
 
 static bool icount_sleep = true;
-static int64_t vm_clock_warp_start = -1;
 /* Conversion factor from emulated instructions to virtual clock ticks.  */
 static int icount_time_shift;
 /* Arbitrarily pick 1MIPS as the minimum allowable speed.  */
 #define MAX_ICOUNT_SHIFT 10
 
-static QEMUTimer *icount_rt_timer;
-static QEMUTimer *icount_vm_timer;
-static QEMUTimer *icount_warp_timer;
-
 typedef struct TimersState {
     /* Protected by BQL.  */
     int64_t cpu_ticks_prev;
@@ -145,6 +140,11 @@ typedef struct TimersState {
     int64_t qemu_icount_bias;
     /* Only written by TCG thread */
     int64_t qemu_icount;
+    /* for adjusting icount */
+    int64_t vm_clock_warp_start;
+    QEMUTimer *icount_rt_timer;
+    QEMUTimer *icount_vm_timer;
+    QEMUTimer *icount_warp_timer;
 } TimersState;
 
 static TimersState timers_state;
@@ -430,14 +430,14 @@ static void icount_adjust(void)
 
 static void icount_adjust_rt(void *opaque)
 {
-    timer_mod(icount_rt_timer,
+    timer_mod(timers_state.icount_rt_timer,
               qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) + 1000);
     icount_adjust();
 }
 
 static void icount_adjust_vm(void *opaque)
 {
-    timer_mod(icount_vm_timer,
+    timer_mod(timers_state.icount_vm_timer,
                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
                    NANOSECONDS_PER_SECOND / 10);
     icount_adjust();
@@ -458,7 +458,7 @@ static void icount_warp_rt(void)
      */
     do {
         seq = seqlock_read_begin(&timers_state.vm_clock_seqlock);
-        warp_start = vm_clock_warp_start;
+        warp_start = timers_state.vm_clock_warp_start;
     } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, seq));
 
     if (warp_start == -1) {
@@ -471,7 +471,7 @@ static void icount_warp_rt(void)
                                      cpu_get_clock_locked());
         int64_t warp_delta;
 
-        warp_delta = clock - vm_clock_warp_start;
+        warp_delta = clock - timers_state.vm_clock_warp_start;
         if (use_icount == 2) {
             /*
              * In adaptive mode, do not let QEMU_CLOCK_VIRTUAL run too
@@ -483,7 +483,7 @@ static void icount_warp_rt(void)
         }
         timers_state.qemu_icount_bias += warp_delta;
     }
-    vm_clock_warp_start = -1;
+    timers_state.vm_clock_warp_start = -1;
     seqlock_write_end(&timers_state.vm_clock_seqlock);
 
     if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
@@ -592,11 +592,11 @@ void qemu_start_warp_timer(void)
              * every 100ms.
              */
             seqlock_write_begin(&timers_state.vm_clock_seqlock);
-            if (vm_clock_warp_start == -1 || vm_clock_warp_start > clock) {
-                vm_clock_warp_start = clock;
+            if (timers_state.vm_clock_warp_start == -1 || timers_state.vm_clock_warp_start > clock) {
+                timers_state.vm_clock_warp_start = clock;
             }
             seqlock_write_end(&timers_state.vm_clock_seqlock);
-            timer_mod_anticipate(icount_warp_timer, clock + deadline);
+            timer_mod_anticipate(timers_state.icount_warp_timer, clock + deadline);
         }
     } else if (deadline == 0) {
         qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
@@ -621,7 +621,7 @@ static void qemu_account_warp_timer(void)
         return;
     }
 
-    timer_del(icount_warp_timer);
+    timer_del(timers_state.icount_warp_timer);
     icount_warp_rt();
 }
 
@@ -630,6 +630,44 @@ static bool icount_state_needed(void *opaque)
     return use_icount;
 }
 
+static bool warp_timer_state_needed(void *opaque)
+{
+    TimersState *s = opaque;
+    return s->icount_warp_timer != NULL;
+}
+
+static bool adjust_timers_state_needed(void *opaque)
+{
+    TimersState *s = opaque;
+    return s->icount_rt_timer != NULL;
+}
+
+/*
+ * Subsection for warp timer migration is optional, because may not be created
+ */
+static const VMStateDescription icount_vmstate_warp_timer = {
+    .name = "timer/icount/warp_timer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = warp_timer_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_TIMER_PTR(icount_warp_timer, TimersState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription icount_vmstate_adjust_timers = {
+    .name = "timer/icount/timers",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = adjust_timers_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_TIMER_PTR(icount_rt_timer, TimersState),
+        VMSTATE_TIMER_PTR(icount_vm_timer, TimersState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 /*
  * This is a subsection for icount migration.
  */
@@ -641,7 +679,13 @@ static const VMStateDescription icount_vmstate_timers = {
     .fields = (VMStateField[]) {
         VMSTATE_INT64(qemu_icount_bias, TimersState),
         VMSTATE_INT64(qemu_icount, TimersState),
+        VMSTATE_INT64(vm_clock_warp_start, TimersState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &icount_vmstate_warp_timer,
+        &icount_vmstate_adjust_timers,
+        NULL
     }
 };
 
@@ -752,7 +796,7 @@ void configure_icount(QemuOpts *opts, Error **errp)
 
     icount_sleep = qemu_opt_get_bool(opts, "sleep", true);
     if (icount_sleep) {
-        icount_warp_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
+        timers_state.icount_warp_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
                                          icount_timer_cb, NULL);
     }
 
@@ -786,13 +830,14 @@ void configure_icount(QemuOpts *opts, Error **errp)
        the virtual time trigger catches emulated time passing too fast.
        Realtime triggers occur even when idle, so use them less frequently
        than VM triggers.  */
-    icount_rt_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL_RT,
+    timers_state.vm_clock_warp_start = -1;
+    timers_state.icount_rt_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL_RT,
                                    icount_adjust_rt, NULL);
-    timer_mod(icount_rt_timer,
+    timer_mod(timers_state.icount_rt_timer,
                    qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) + 1000);
-    icount_vm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+    timers_state.icount_vm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                         icount_adjust_vm, NULL);
-    timer_mod(icount_vm_timer,
+    timer_mod(timers_state.icount_vm_timer,
                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
                    NANOSECONDS_PER_SECOND / 10);
 }

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Juan Quintela]

Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> This patch does not allows saving/loading vmstate when
> replay events queue is not empty. There is no reliable
> way to save events queue, because it describes internal
> coroutine state. Therefore saving and loading operations
> should be deferred to another record/replay step.
>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>

This functions have changed, see last series (but change is trivial from
monitor_printf to error_<something>)

> @@ -2083,6 +2084,12 @@ int save_vmstate(Monitor *mon, const char *name)
>      Error *local_err = NULL;
>      AioContext *aio_context;
>  
> +    if (!replay_can_snapshot()) {
> +        monitor_printf(mon, "Record/replay does not allow making snapshot right now. "
> +                        "Try stopping at another step.\n");
> +        return ret;
> +    }
> +

To issue a savevm/loadvm the user don't have to stop qemu, so I think we
can improve the message to something les in both places?

"Try saving/loading later"?

Thanks, Juan.

Re: [Qemu-devel] [PATCH v9 00/10] replay additions

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v9 00/10] replay additions
Message-id: 20170504084135.7488.24715.stgit@PASHA-ISP
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1493816238-33120-1-git-send-email-imammedo@redhat.com -> patchew/1493816238-33120-1-git-send-email-imammedo@redhat.com
 * [new tag]         patchew/1493888311-30839-1-git-send-email-thuth@redhat.com -> patchew/1493888311-30839-1-git-send-email-thuth@redhat.com
Switched to a new branch 'test'
bf3cfc0 icount: fixed saving/restoring of icount warp timers
37b3637 replay: save prior value of the host clock
0a8766b replay: make safe vmstop at record/replay
3aecd8c replay: added replay log format description
0d8963d replay: fix save/load vm for non-empty queue
9c14e13 replay: fixed replay_enable_events
70c9cd1 replay: fix processing async events
37292c3 replay: disable default snapshot for record/replay
5973813 blkreplay: create temporary overlay for underlaying devices
f34a13b block: implement bdrv_snapshot_goto for blkreplay

=== OUTPUT BEGIN ===
Checking PATCH 1/10: block: implement bdrv_snapshot_goto for blkreplay...
Checking PATCH 2/10: blkreplay: create temporary overlay for underlaying devices...
Checking PATCH 3/10: replay: disable default snapshot for record/replay...
Checking PATCH 4/10: replay: fix processing async events...
Checking PATCH 5/10: replay: fixed replay_enable_events...
Checking PATCH 6/10: replay: fix save/load vm for non-empty queue...
WARNING: line over 80 characters
#45: FILE: migration/savevm.c:2088:
+        monitor_printf(mon, "Record/replay does not allow making snapshot right now. "

ERROR: Error messages should not contain newlines
#59: FILE: migration/savevm.c:2256:
+                     "Try stopping at another step.\n");

total: 1 errors, 1 warnings, 48 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 7/10: replay: added replay log format description...
Checking PATCH 8/10: replay: make safe vmstop at record/replay...
Checking PATCH 9/10: replay: save prior value of the host clock...
Checking PATCH 10/10: icount: fixed saving/restoring of icount warp timers...
ERROR: line over 90 characters
#98: FILE: cpus.c:595:
+            if (timers_state.vm_clock_warp_start == -1 || timers_state.vm_clock_warp_start > clock) {

WARNING: line over 80 characters
#103: FILE: cpus.c:599:
+            timer_mod_anticipate(timers_state.icount_warp_timer, clock + deadline);

ERROR: spaces required around that '*' (ctx:VxV)
#168: FILE: cpus.c:685:
+    .subsections = (const VMStateDescription*[]) {
                                             ^

total: 2 errors, 1 warnings, 172 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Pavel Dovgalyuk]

> From: Juan Quintela [mailto:quintela@redhat.com]
> Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> wrote:
> > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> >
> > This patch does not allows saving/loading vmstate when
> > replay events queue is not empty. There is no reliable
> > way to save events queue, because it describes internal
> > coroutine state. Therefore saving and loading operations
> > should be deferred to another record/replay step.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> 
> This functions have changed, see last series (but change is trivial from
> monitor_printf to error_<something>)
> 
> > @@ -2083,6 +2084,12 @@ int save_vmstate(Monitor *mon, const char *name)
> >      Error *local_err = NULL;
> >      AioContext *aio_context;
> >
> > +    if (!replay_can_snapshot()) {
> > +        monitor_printf(mon, "Record/replay does not allow making snapshot right now. "
> > +                        "Try stopping at another step.\n");
> > +        return ret;
> > +    }
> > +
> 
> To issue a savevm/loadvm the user don't have to stop qemu, so I think we
> can improve the message to something les in both places?
> 
> "Try saving/loading later"?

Right, thanks.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v9 04/10] replay: fix processing async events

[Paolo Bonzini]

On 04/05/2017 10:41, Pavel Dovgalyuk wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> 
> Asynchronous events saved at checkpoints may invoke
> callbacks when processed. These callbacks may also generate/read
> new events (e.g. clock reads). Therefore event processing flag must be
> reset before callback invocation.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  replay/replay-events.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/replay/replay-events.c b/replay/replay-events.c
> index 94a6dcccfc..768b505f3d 100644
> --- a/replay/replay-events.c
> +++ b/replay/replay-events.c
> @@ -295,13 +295,13 @@ void replay_read_events(int checkpoint)
>          if (!event) {
>              break;
>          }
> +        replay_finish_event();
> +        read_event_kind = -1;
>          replay_mutex_unlock();
>          replay_run_event(event);
>          replay_mutex_lock();
>  
>          g_free(event);
> -        replay_finish_event();
> -        read_event_kind = -1;

While at it, g_free can be moved outside the lock.

Also, replay_flush_events and replay_save_events are leaving the event
in events_list while releasing the lock, and only doing QTAILQ_REMOVE
after taking it back.  This can cause events to be processed twice.

I think it is worth fixing all of these.

Paolo

>      }
>  }
>  
> 

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Paolo Bonzini]

On 04/05/2017 10:42, Pavel Dovgalyuk wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> 
> This patch does not allows saving/loading vmstate when
> replay events queue is not empty. There is no reliable
> way to save events queue, because it describes internal
> coroutine state. Therefore saving and loading operations
> should be deferred to another record/replay step.

Can it actually be non-empty after bdrv_drain_all?

Paolo

> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  include/sysemu/replay.h  |    3 +++
>  migration/savevm.c       |   13 +++++++++++++
>  replay/replay-snapshot.c |    6 ++++++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
> index f1c0712795..5c9db2a2ef 100644
> --- a/include/sysemu/replay.h
> +++ b/include/sysemu/replay.h
> @@ -164,5 +164,8 @@ void replay_audio_in(int *recorded, void *samples, int *wpos, int size);
>  /*! Called at the start of execution.
>      Loads or saves initial vmstate depending on execution mode. */
>  void replay_vmstate_init(void);
> +/*! Called to ensure that replay state is consistent and VM snapshot
> +    can be created */
> +bool replay_can_snapshot(void);
>  
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 03ae1bdeb4..358d22170d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -54,6 +54,7 @@
>  #include "qemu/cutils.h"
>  #include "io/channel-buffer.h"
>  #include "io/channel-file.h"
> +#include "sysemu/replay.h"
>  
>  #ifndef ETH_P_RARP
>  #define ETH_P_RARP 0x8035
> @@ -2083,6 +2084,12 @@ int save_vmstate(Monitor *mon, const char *name)
>      Error *local_err = NULL;
>      AioContext *aio_context;
>  
> +    if (!replay_can_snapshot()) {
> +        monitor_printf(mon, "Record/replay does not allow making snapshot right now. "
> +                        "Try stopping at another step.\n");
> +        return ret;
> +    }
> +
>      if (!bdrv_all_can_snapshot(&bs)) {
>          monitor_printf(mon, "Device '%s' is writable but does not "
>                         "support snapshots.\n", bdrv_get_device_name(bs));
> @@ -2244,6 +2251,12 @@ int load_vmstate(const char *name)
>      AioContext *aio_context;
>      MigrationIncomingState *mis = migration_incoming_get_current();
>  
> +    if (!replay_can_snapshot()) {
> +        error_report("Record/replay does not allow loading snapshot right now. "
> +                     "Try stopping at another step.\n");
> +        return -EINVAL;
> +    }
> +
>      if (!bdrv_all_can_snapshot(&bs)) {
>          error_report("Device '%s' is writable but does not support snapshots.",
>                       bdrv_get_device_name(bs));
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index 65e2d375c2..438d82ec33 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -76,3 +76,9 @@ void replay_vmstate_init(void)
>          }
>      }
>  }
> +
> +bool replay_can_snapshot(void)
> +{
> +    return replay_mode == REPLAY_MODE_NONE
> +        || !replay_has_events();
> +}
> 

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 04/05/2017 10:42, Pavel Dovgalyuk wrote:
> > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> >
> > This patch does not allows saving/loading vmstate when
> > replay events queue is not empty. There is no reliable
> > way to save events queue, because it describes internal
> > coroutine state. Therefore saving and loading operations
> > should be deferred to another record/replay step.
> 
> Can it actually be non-empty after bdrv_drain_all?

drain/flush cannot succeed, because started requests are
prisoned in the replay events queue.

> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  include/sysemu/replay.h  |    3 +++
> >  migration/savevm.c       |   13 +++++++++++++
> >  replay/replay-snapshot.c |    6 ++++++
> >  3 files changed, 22 insertions(+)
> >
> > diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
> > index f1c0712795..5c9db2a2ef 100644
> > --- a/include/sysemu/replay.h
> > +++ b/include/sysemu/replay.h
> > @@ -164,5 +164,8 @@ void replay_audio_in(int *recorded, void *samples, int *wpos, int size);
> >  /*! Called at the start of execution.
> >      Loads or saves initial vmstate depending on execution mode. */
> >  void replay_vmstate_init(void);
> > +/*! Called to ensure that replay state is consistent and VM snapshot
> > +    can be created */
> > +bool replay_can_snapshot(void);
> >
> >  #endif
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 03ae1bdeb4..358d22170d 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -54,6 +54,7 @@
> >  #include "qemu/cutils.h"
> >  #include "io/channel-buffer.h"
> >  #include "io/channel-file.h"
> > +#include "sysemu/replay.h"
> >
> >  #ifndef ETH_P_RARP
> >  #define ETH_P_RARP 0x8035
> > @@ -2083,6 +2084,12 @@ int save_vmstate(Monitor *mon, const char *name)
> >      Error *local_err = NULL;
> >      AioContext *aio_context;
> >
> > +    if (!replay_can_snapshot()) {
> > +        monitor_printf(mon, "Record/replay does not allow making snapshot right now. "
> > +                        "Try stopping at another step.\n");
> > +        return ret;
> > +    }
> > +
> >      if (!bdrv_all_can_snapshot(&bs)) {
> >          monitor_printf(mon, "Device '%s' is writable but does not "
> >                         "support snapshots.\n", bdrv_get_device_name(bs));
> > @@ -2244,6 +2251,12 @@ int load_vmstate(const char *name)
> >      AioContext *aio_context;
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> >
> > +    if (!replay_can_snapshot()) {
> > +        error_report("Record/replay does not allow loading snapshot right now. "
> > +                     "Try stopping at another step.\n");
> > +        return -EINVAL;
> > +    }
> > +
> >      if (!bdrv_all_can_snapshot(&bs)) {
> >          error_report("Device '%s' is writable but does not support snapshots.",
> >                       bdrv_get_device_name(bs));
> > diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> > index 65e2d375c2..438d82ec33 100644
> > --- a/replay/replay-snapshot.c
> > +++ b/replay/replay-snapshot.c
> > @@ -76,3 +76,9 @@ void replay_vmstate_init(void)
> >          }
> >      }
> >  }
> > +
> > +bool replay_can_snapshot(void)
> > +{
> > +    return replay_mode == REPLAY_MODE_NONE
> > +        || !replay_has_events();
> > +}
> >

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Paolo Bonzini]

On 04/05/2017 13:13, Pavel Dovgalyuk wrote:
>>> This patch does not allows saving/loading vmstate when
>>> replay events queue is not empty. There is no reliable
>>> way to save events queue, because it describes internal
>>> coroutine state. Therefore saving and loading operations
>>> should be deferred to another record/replay step.
>>
>> Can it actually be non-empty after bdrv_drain_all?
>
> drain/flush cannot succeed, because started requests are
> prisoned in the replay events queue.

But that would apply to loading only.  Saving should still be always
possible.

Paolo

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 04/05/2017 13:13, Pavel Dovgalyuk wrote:
> >>> This patch does not allows saving/loading vmstate when
> >>> replay events queue is not empty. There is no reliable
> >>> way to save events queue, because it describes internal
> >>> coroutine state. Therefore saving and loading operations
> >>> should be deferred to another record/replay step.
> >>
> >> Can it actually be non-empty after bdrv_drain_all?
> >
> > drain/flush cannot succeed, because started requests are
> > prisoned in the replay events queue.
> 
> But that would apply to loading only.  Saving should still be always
> possible.

We can save it. But it wouldn't load correctly - replay queue will be empty after loading.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Paolo Bonzini]

On 04/05/2017 13:54, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 04/05/2017 13:13, Pavel Dovgalyuk wrote:
>>>>> This patch does not allows saving/loading vmstate when
>>>>> replay events queue is not empty. There is no reliable
>>>>> way to save events queue, because it describes internal
>>>>> coroutine state. Therefore saving and loading operations
>>>>> should be deferred to another record/replay step.
>>>>
>>>> Can it actually be non-empty after bdrv_drain_all?
>>>
>>> drain/flush cannot succeed, because started requests are
>>> prisoned in the replay events queue.
>>
>> But that would apply to loading only.  Saving should still be always
>> possible.
> 
> We can save it. But it wouldn't load correctly - replay queue will be empty after loading.

When saving you can drain, and then the events queue should be empty.
Or I am misunderstanding how it works, which is possible too.

Paolo

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 04/05/2017 13:54, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 04/05/2017 13:13, Pavel Dovgalyuk wrote:
> >>>>> This patch does not allows saving/loading vmstate when
> >>>>> replay events queue is not empty. There is no reliable
> >>>>> way to save events queue, because it describes internal
> >>>>> coroutine state. Therefore saving and loading operations
> >>>>> should be deferred to another record/replay step.
> >>>>
> >>>> Can it actually be non-empty after bdrv_drain_all?
> >>>
> >>> drain/flush cannot succeed, because started requests are
> >>> prisoned in the replay events queue.
> >>
> >> But that would apply to loading only.  Saving should still be always
> >> possible.
> >
> > We can save it. But it wouldn't load correctly - replay queue will be empty after loading.
> 
> When saving you can drain, and then the events queue should be empty.
> Or I am misunderstanding how it works, which is possible too.

Drain will wait until the queue becomes empty.
Queue is processed only at checkpoints.
Checkpoints are met in iothread (at timers processing and so on).
But iothread is waiting for finishing the drain.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Paolo Bonzini]

On 04/05/2017 14:10, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 04/05/2017 13:54, Pavel Dovgalyuk wrote:
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> On 04/05/2017 13:13, Pavel Dovgalyuk wrote:
>>>>>>> This patch does not allows saving/loading vmstate when
>>>>>>> replay events queue is not empty. There is no reliable
>>>>>>> way to save events queue, because it describes internal
>>>>>>> coroutine state. Therefore saving and loading operations
>>>>>>> should be deferred to another record/replay step.
>>>>>>
>>>>>> Can it actually be non-empty after bdrv_drain_all?
>>>>>
>>>>> drain/flush cannot succeed, because started requests are
>>>>> prisoned in the replay events queue.
>>>>
>>>> But that would apply to loading only.  Saving should still be always
>>>> possible.
>>>
>>> We can save it. But it wouldn't load correctly - replay queue will be empty after loading.
>>
>> When saving you can drain, and then the events queue should be empty.
>> Or I am misunderstanding how it works, which is possible too.
> 
> Drain will wait until the queue becomes empty.
> Queue is processed only at checkpoints.
> Checkpoints are met in iothread (at timers processing and so on).
> But iothread is waiting for finishing the drain.

What checkpoint can hang the drain during a save?

Paolo

Re: [Qemu-devel] [PATCH v9 04/10] replay: fix processing async events

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 04/05/2017 10:41, Pavel Dovgalyuk wrote:
> > From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> >
> > Asynchronous events saved at checkpoints may invoke
> > callbacks when processed. These callbacks may also generate/read
> > new events (e.g. clock reads). Therefore event processing flag must be
> > reset before callback invocation.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  replay/replay-events.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/replay/replay-events.c b/replay/replay-events.c
> > index 94a6dcccfc..768b505f3d 100644
> > --- a/replay/replay-events.c
> > +++ b/replay/replay-events.c
> > @@ -295,13 +295,13 @@ void replay_read_events(int checkpoint)
> >          if (!event) {
> >              break;
> >          }
> > +        replay_finish_event();
> > +        read_event_kind = -1;
> >          replay_mutex_unlock();
> >          replay_run_event(event);
> >          replay_mutex_lock();
> >
> >          g_free(event);
> > -        replay_finish_event();
> > -        read_event_kind = -1;
> 
> While at it, g_free can be moved outside the lock.

There is no difference where to free the memory.
The only reason for unlocking and locking again is that replay_run_event may
try to read more data from the log.

> Also, replay_flush_events and replay_save_events are leaving the event
> in events_list while releasing the lock, and only doing QTAILQ_REMOVE
> after taking it back.  This can cause events to be processed twice.

How this could happen? If something will try to call replay_save_events again,
while in replay_save_events, then we'll have an infinite loop and notice it.
But it couldn't, because saving only occurs at checkpoints and there are no
recursive ones.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 04/05/2017 14:10, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 04/05/2017 13:54, Pavel Dovgalyuk wrote:
> >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >>>> On 04/05/2017 13:13, Pavel Dovgalyuk wrote:
> >>>>>>> This patch does not allows saving/loading vmstate when
> >>>>>>> replay events queue is not empty. There is no reliable
> >>>>>>> way to save events queue, because it describes internal
> >>>>>>> coroutine state. Therefore saving and loading operations
> >>>>>>> should be deferred to another record/replay step.
> >>>>>>
> >>>>>> Can it actually be non-empty after bdrv_drain_all?
> >>>>>
> >>>>> drain/flush cannot succeed, because started requests are
> >>>>> prisoned in the replay events queue.
> >>>>
> >>>> But that would apply to loading only.  Saving should still be always
> >>>> possible.
> >>>
> >>> We can save it. But it wouldn't load correctly - replay queue will be empty after loading.
> >>
> >> When saving you can drain, and then the events queue should be empty.
> >> Or I am misunderstanding how it works, which is possible too.
> >
> > Drain will wait until the queue becomes empty.
> > Queue is processed only at checkpoints.
> > Checkpoints are met in iothread (at timers processing and so on).
> > But iothread is waiting for finishing the drain.
> 
> What checkpoint can hang the drain during a save?

No one. There are no checkpoins during vmsave and during drain.
Therefore the queue will never become empty.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Paolo Bonzini]

On 04/05/2017 14:34, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 04/05/2017 14:10, Pavel Dovgalyuk wrote:
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> On 04/05/2017 13:54, Pavel Dovgalyuk wrote:
>>>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>>>> On 04/05/2017 13:13, Pavel Dovgalyuk wrote:
>>>>>>>>> This patch does not allows saving/loading vmstate when
>>>>>>>>> replay events queue is not empty. There is no reliable
>>>>>>>>> way to save events queue, because it describes internal
>>>>>>>>> coroutine state. Therefore saving and loading operations
>>>>>>>>> should be deferred to another record/replay step.
>>>>>>>>
>>>>>>>> Can it actually be non-empty after bdrv_drain_all?
>>>>>>>
>>>>>>> drain/flush cannot succeed, because started requests are
>>>>>>> prisoned in the replay events queue.
>>>>>>
>>>>>> But that would apply to loading only.  Saving should still be always
>>>>>> possible.
>>>>>
>>>>> We can save it. But it wouldn't load correctly - replay queue will be empty after loading.
>>>>
>>>> When saving you can drain, and then the events queue should be empty.
>>>> Or I am misunderstanding how it works, which is possible too.
>>>
>>> Drain will wait until the queue becomes empty.
>>> Queue is processed only at checkpoints.
>>> Checkpoints are met in iothread (at timers processing and so on).
>>> But iothread is waiting for finishing the drain.
>>
>> What checkpoint can hang the drain during a save?
> 
> No one. There are no checkpoins during vmsave and during drain.
> Therefore the queue will never become empty.

Understood.  And what checkpoint will you be waiting for during drain,
causing a deadlock?

Paolo

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 04/05/2017 14:34, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 04/05/2017 14:10, Pavel Dovgalyuk wrote:
> >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >>>> On 04/05/2017 13:54, Pavel Dovgalyuk wrote:
> >>>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >>>>>> On 04/05/2017 13:13, Pavel Dovgalyuk wrote:
> >>>>>>>>> This patch does not allows saving/loading vmstate when
> >>>>>>>>> replay events queue is not empty. There is no reliable
> >>>>>>>>> way to save events queue, because it describes internal
> >>>>>>>>> coroutine state. Therefore saving and loading operations
> >>>>>>>>> should be deferred to another record/replay step.
> >>>>>>>>
> >>>>>>>> Can it actually be non-empty after bdrv_drain_all?
> >>>>>>>
> >>>>>>> drain/flush cannot succeed, because started requests are
> >>>>>>> prisoned in the replay events queue.
> >>>>>>
> >>>>>> But that would apply to loading only.  Saving should still be always
> >>>>>> possible.
> >>>>>
> >>>>> We can save it. But it wouldn't load correctly - replay queue will be empty after
> loading.
> >>>>
> >>>> When saving you can drain, and then the events queue should be empty.
> >>>> Or I am misunderstanding how it works, which is possible too.
> >>>
> >>> Drain will wait until the queue becomes empty.
> >>> Queue is processed only at checkpoints.
> >>> Checkpoints are met in iothread (at timers processing and so on).
> >>> But iothread is waiting for finishing the drain.
> >>
> >> What checkpoint can hang the drain during a save?
> >
> > No one. There are no checkpoins during vmsave and during drain.
> > Therefore the queue will never become empty.
> 
> Understood.  And what checkpoint will you be waiting for during drain,
> causing a deadlock?

Every checkpoint processes the queue, but none of them are invoked,
because iothread (which invokes all the checkpoints) is waiting for the end of the drain.

And we cannot add a checkpoint into the drain, because then vmstop will
affect the behavior of the guest.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Paolo Bonzini]

On 04/05/2017 15:02, Pavel Dovgalyuk wrote:
>> Understood.  And what checkpoint will you be waiting for during drain,
>> causing a deadlock?
> Every checkpoint processes the queue, but none of them are invoked,
> because iothread (which invokes all the checkpoints) is waiting for the end of the drain.
> 
> And we cannot add a checkpoint into the drain, because then vmstop will
> affect the behavior of the guest.

But it does affect the behavior already when RR is off.  And it
shouldn't be a problem if it does in record mode (as long as the effect
can be replayed).

If RR cannot do drain reliably, not even in record mode, that is a huge
problem because a lot of block layer operations assume they can.

Paolo

Re: [Qemu-devel] [PATCH v9 06/10] replay: fix save/load vm for non-empty queue

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 04/05/2017 15:02, Pavel Dovgalyuk wrote:
> >> Understood.  And what checkpoint will you be waiting for during drain,
> >> causing a deadlock?
> > Every checkpoint processes the queue, but none of them are invoked,
> > because iothread (which invokes all the checkpoints) is waiting for the end of the drain.
> >
> > And we cannot add a checkpoint into the drain, because then vmstop will
> > affect the behavior of the guest.
> 
> But it does affect the behavior already when RR is off.  And it
> shouldn't be a problem if it does in record mode (as long as the effect
> can be replayed).

Then we'll have to add something like "stop" event which flushes the queue.
And there is still a problem with stopping in replay mode, because there
is no way to flush the queue, because coroutines are executing somewhere.

> If RR cannot do drain reliably, not even in record mode, that is a huge
> problem because a lot of block layer operations assume they can.

I think it could be an acceptable limitation (like disabling savevm/loadvm
when queue is not empty).
I assume that RR is mostly used for debugging and analysis and it could
have some limitations of this kind.
Making our own block queue (instead of coroutines) is much worse, therefore
we have to replay the original internal behavior.

Pavel Dovgalyuk