[Qemu-devel] [PATCH v5 0/9] replay additions

[Qemu-devel] [PATCH v5 0/9] replay additions

[Pavel Dovgalyuk]

This set of patches includes several fixes for replay and
adds network record/replay for network devices. It also makes possible
saving/restoring vmstate in replay mode.

Record and replay for network interactions is performed with the network filter.
Each backend must have its own instance of the replay filter as follows:
 -netdev user,id=net1 -device rtl8139,netdev=net1
 -object filter-replay,id=replay,netdev=net1

This patches add rrsnapshot option for icount. rrshapshot option creates
start snapshot at record and loads it at replay. It allows preserving
the state of disk images used by virtual machine. This vm state can also
use used to roll back the execution while replaying.

This set of patches includes fixes and additions for icount and
record/replay implementation:
 - VM start/stop in replay mode
 - Network interaction record/replay
 - overlay option for blkreplay filter
 - rrsnapshot option for record/replay
 - vmstate fix for integratorcp ARM platform

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 (9):
      replay: move internal data to the structure
      replay: vmstate for replay module
      replay: allow replay stopping and restarting
      record/replay: add network support
      savevm: add public save_vmstate function
      replay: save/load initial state
      block: don't make snapshots for filters
      block: implement bdrv_recurse_is_first_non_filter for blkreplay
      integratorcp: adding vmstate for save/restore


 block/blkreplay.c        |   24 ++++++-----
 block/snapshot.c         |    3 +
 cpus.c                   |    1 
 docs/replay.txt          |   30 ++++++++++++++
 hw/arm/integratorcp.c    |   62 ++++++++++++++++++++++++++++
 include/sysemu/replay.h  |   25 +++++++++++
 include/sysemu/sysemu.h  |    1 
 migration/savevm.c       |   33 ++++++++++-----
 net/Makefile.objs        |    1 
 net/filter-replay.c      |   92 +++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx          |    8 +++-
 replay/Makefile.objs     |    2 +
 replay/replay-events.c   |   21 +++++++++
 replay/replay-internal.c |   20 ++++-----
 replay/replay-internal.h |   33 +++++++++++++--
 replay/replay-net.c      |  102 ++++++++++++++++++++++++++++++++++++++++++++++
 replay/replay-snapshot.c |   78 +++++++++++++++++++++++++++++++++++
 replay/replay-time.c     |    2 -
 replay/replay.c          |   23 +++++++---
 stubs/replay.c           |    5 ++
 vl.c                     |   14 +++++-
 21 files changed, 528 insertions(+), 52 deletions(-)
 create mode 100644 net/filter-replay.c
 create mode 100644 replay/replay-net.c
 create mode 100644 replay/replay-snapshot.c

-- 
Pavel Dovgalyuk

[Qemu-devel] [PATCH v5 1/9] replay: move internal data to the structure

[Pavel Dovgalyuk]

This patch moves replay static variables into the structure
to allow saving and loading them with savevm/loadvm.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 replay/replay-events.c   |    2 +-
 replay/replay-internal.c |   20 +++++++++-----------
 replay/replay-internal.h |    8 +++++---
 replay/replay-time.c     |    2 +-
 replay/replay.c          |   15 ++++++++-------
 5 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/replay/replay-events.c b/replay/replay-events.c
index 3807245..4eb2ea3 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -279,7 +279,7 @@ static Event *replay_read_event(int checkpoint)
 /* Called with replay mutex locked */
 void replay_read_events(int checkpoint)
 {
-    while (replay_data_kind == EVENT_ASYNC) {
+    while (replay_state.data_kind == EVENT_ASYNC) {
         Event *event = replay_read_event(checkpoint);
         if (!event) {
             break;
diff --git a/replay/replay-internal.c b/replay/replay-internal.c
index 5835e8d..bea7b4a 100644
--- a/replay/replay-internal.c
+++ b/replay/replay-internal.c
@@ -16,11 +16,8 @@
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 
-unsigned int replay_data_kind = -1;
-static unsigned int replay_has_unread_data;
-
 /* Mutex to protect reading and writing events to the log.
-   replay_data_kind and replay_has_unread_data are also protected
+   data_kind and has_unread_data are also protected
    by this mutex.
    It also protects replay events queue which stores events to be
    written or read to the log. */
@@ -150,15 +147,16 @@ void replay_check_error(void)
 void replay_fetch_data_kind(void)
 {
     if (replay_file) {
-        if (!replay_has_unread_data) {
-            replay_data_kind = replay_get_byte();
-            if (replay_data_kind == EVENT_INSTRUCTION) {
+        if (!replay_state.has_unread_data) {
+            replay_state.data_kind = replay_get_byte();
+            if (replay_state.data_kind == EVENT_INSTRUCTION) {
                 replay_state.instructions_count = replay_get_dword();
             }
             replay_check_error();
-            replay_has_unread_data = 1;
-            if (replay_data_kind >= EVENT_COUNT) {
-                error_report("Replay: unknown event kind %d", replay_data_kind);
+            replay_state.has_unread_data = 1;
+            if (replay_state.data_kind >= EVENT_COUNT) {
+                error_report("Replay: unknown event kind %d",
+                             replay_state.data_kind);
                 exit(1);
             }
         }
@@ -167,7 +165,7 @@ void replay_fetch_data_kind(void)
 
 void replay_finish_event(void)
 {
-    replay_has_unread_data = 0;
+    replay_state.has_unread_data = 0;
     replay_fetch_data_kind();
 }
 
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index efbf14c..9b02d7d 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -62,11 +62,13 @@ typedef struct ReplayState {
     uint64_t current_step;
     /*! Number of instructions to be executed before other events happen. */
     int instructions_count;
+    /*! Type of the currently executed event. */
+    unsigned int data_kind;
+    /*! Flag which indicates that event is not processed yet. */
+    unsigned int has_unread_data;
 } ReplayState;
 extern ReplayState replay_state;
 
-extern unsigned int replay_data_kind;
-
 /* File for replay writing */
 extern FILE *replay_file;
 
@@ -98,7 +100,7 @@ void replay_check_error(void);
     the next event from the log. */
 void replay_finish_event(void);
 /*! Reads data type from the file and stores it in the
-    replay_data_kind variable. */
+    data_kind variable. */
 void replay_fetch_data_kind(void);
 
 /*! Saves queued events (like instructions and sound). */
diff --git a/replay/replay-time.c b/replay/replay-time.c
index fffe072..f70382a 100644
--- a/replay/replay-time.c
+++ b/replay/replay-time.c
@@ -31,7 +31,7 @@ int64_t replay_save_clock(ReplayClockKind kind, int64_t clock)
 
 void replay_read_next_clock(ReplayClockKind kind)
 {
-    unsigned int read_kind = replay_data_kind - EVENT_CLOCK;
+    unsigned int read_kind = replay_state.data_kind - EVENT_CLOCK;
 
     assert(read_kind == kind);
 
diff --git a/replay/replay.c b/replay/replay.c
index 167fd29..cc2238d 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -38,15 +38,15 @@ bool replay_next_event_is(int event)
 
     /* nothing to skip - not all instructions used */
     if (replay_state.instructions_count != 0) {
-        assert(replay_data_kind == EVENT_INSTRUCTION);
+        assert(replay_state.data_kind == EVENT_INSTRUCTION);
         return event == EVENT_INSTRUCTION;
     }
 
     while (true) {
-        if (event == replay_data_kind) {
+        if (event == replay_state.data_kind) {
             res = true;
         }
-        switch (replay_data_kind) {
+        switch (replay_state.data_kind) {
         case EVENT_SHUTDOWN:
             replay_finish_event();
             qemu_system_shutdown_request();
@@ -85,7 +85,7 @@ void replay_account_executed_instructions(void)
             replay_state.instructions_count -= count;
             replay_state.current_step += count;
             if (replay_state.instructions_count == 0) {
-                assert(replay_data_kind == EVENT_INSTRUCTION);
+                assert(replay_state.data_kind == EVENT_INSTRUCTION);
                 replay_finish_event();
                 /* Wake up iothread. This is required because
                    timers will not expire until clock counters
@@ -188,7 +188,7 @@ bool replay_checkpoint(ReplayCheckpoint checkpoint)
     if (replay_mode == REPLAY_MODE_PLAY) {
         if (replay_next_event_is(EVENT_CHECKPOINT + checkpoint)) {
             replay_finish_event();
-        } else if (replay_data_kind != EVENT_ASYNC) {
+        } else if (replay_state.data_kind != EVENT_ASYNC) {
             res = false;
             goto out;
         }
@@ -196,7 +196,7 @@ bool replay_checkpoint(ReplayCheckpoint checkpoint)
         /* replay_read_events may leave some unread events.
            Return false if not all of the events associated with
            checkpoint were processed */
-        res = replay_data_kind != EVENT_ASYNC;
+        res = replay_state.data_kind != EVENT_ASYNC;
     } else if (replay_mode == REPLAY_MODE_RECORD) {
         replay_put_event(EVENT_CHECKPOINT + checkpoint);
         replay_save_events(checkpoint);
@@ -237,9 +237,10 @@ static void replay_enable(const char *fname, int mode)
     replay_filename = g_strdup(fname);
 
     replay_mode = mode;
-    replay_data_kind = -1;
+    replay_state.data_kind = -1;
     replay_state.instructions_count = 0;
     replay_state.current_step = 0;
+    replay_state.has_unread_data = 0;
 
     /* skip file header for RECORD and check it for PLAY */
     if (replay_mode == REPLAY_MODE_RECORD) {

[Qemu-devel] [PATCH v5 2/9] replay: vmstate for replay module

[Pavel Dovgalyuk]

This patch introduces vmstate for replay data structures.
It allows saving and loading vmstate while replaying.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 replay/Makefile.objs     |    1 +
 replay/replay-internal.h |    9 +++++++
 replay/replay-snapshot.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++
 replay/replay.c          |    1 +
 4 files changed, 71 insertions(+)
 create mode 100644 replay/replay-snapshot.c

diff --git a/replay/Makefile.objs b/replay/Makefile.objs
index fcb3f74..c8ad3eb 100644
--- a/replay/Makefile.objs
+++ b/replay/Makefile.objs
@@ -4,3 +4,4 @@ common-obj-y += replay-events.o
 common-obj-y += replay-time.o
 common-obj-y += replay-input.o
 common-obj-y += replay-char.o
+common-obj-y += replay-snapshot.o
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index 9b02d7d..e07eb7d 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -66,6 +66,8 @@ typedef struct ReplayState {
     unsigned int data_kind;
     /*! Flag which indicates that event is not processed yet. */
     unsigned int has_unread_data;
+    /*! Temporary variable for saving current log offset. */
+    uint64_t file_offset;
 } ReplayState;
 extern ReplayState replay_state;
 
@@ -157,4 +159,11 @@ void replay_event_char_read_save(void *opaque);
 /*! Reads char event read from the file. */
 void *replay_event_char_read_load(void);
 
+/* VMState-related functions */
+
+/* Registers replay VMState.
+   Should be called before virtual devices initialization
+   to make cached timers available for post_load functions. */
+void replay_vmstate_register(void);
+
 #endif
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
new file mode 100644
index 0000000..220248f
--- /dev/null
+++ b/replay/replay-snapshot.c
@@ -0,0 +1,60 @@
+/*
+ * replay-snapshot.c
+ *
+ * Copyright (c) 2010-2016 Institute for System Programming
+ *                         of the Russian Academy of Sciences.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "sysemu/replay.h"
+#include "replay-internal.h"
+#include "sysemu/sysemu.h"
+#include "monitor/monitor.h"
+#include "qapi/qmp/qstring.h"
+#include "qemu/error-report.h"
+#include "migration/vmstate.h"
+
+static void replay_pre_save(void *opaque)
+{
+    ReplayState *state = opaque;
+    state->file_offset = ftello64(replay_file);
+}
+
+static int replay_post_load(void *opaque, int version_id)
+{
+    ReplayState *state = opaque;
+    fseeko64(replay_file, state->file_offset, SEEK_SET);
+    /* If this was a vmstate, saved in recording mode,
+       we need to initialize replay data fields. */
+    replay_fetch_data_kind();
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_replay = {
+    .name = "replay",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .pre_save = replay_pre_save,
+    .post_load = replay_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64_ARRAY(cached_clock, ReplayState, REPLAY_CLOCK_COUNT),
+        VMSTATE_UINT64(current_step, ReplayState),
+        VMSTATE_INT32(instructions_count, ReplayState),
+        VMSTATE_UINT32(data_kind, ReplayState),
+        VMSTATE_UINT32(has_unread_data, ReplayState),
+        VMSTATE_UINT64(file_offset, ReplayState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+void replay_vmstate_register(void)
+{
+    vmstate_register(NULL, 0, &vmstate_replay, &replay_state);
+}
diff --git a/replay/replay.c b/replay/replay.c
index cc2238d..c797aea 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -292,6 +292,7 @@ void replay_configure(QemuOpts *opts)
         exit(1);
     }
 
+    replay_vmstate_register();
     replay_enable(fname, mode);
 
 out:

[Qemu-devel] [PATCH v5 3/9] replay: allow replay stopping and restarting

[Pavel Dovgalyuk]

This patch fixes bug with stopping and restarting replay
through monitor.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 block/blkreplay.c        |   15 +++++----------
 cpus.c                   |    1 +
 include/sysemu/replay.h  |    4 ++++
 replay/replay-events.c   |    8 ++++++++
 replay/replay-internal.h |    6 ++++--
 replay/replay-snapshot.c |    1 +
 stubs/replay.c           |    5 +++++
 vl.c                     |    1 +
 8 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/block/blkreplay.c b/block/blkreplay.c
index 30f9d5f..a741654 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -20,11 +20,6 @@ typedef struct Request {
     QEMUBH *bh;
 } Request;
 
-/* Next request id.
-   This counter is global, because requests from different
-   block devices should not get overlapping ids. */
-static uint64_t request_id;
-
 static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
@@ -84,7 +79,7 @@ static void block_request_create(uint64_t reqid, BlockDriverState *bs,
 static int coroutine_fn blkreplay_co_preadv(BlockDriverState *bs,
     uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
-    uint64_t reqid = request_id++;
+    uint64_t reqid = blkreplay_next_id();
     int ret = bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
@@ -95,7 +90,7 @@ static int coroutine_fn blkreplay_co_preadv(BlockDriverState *bs,
 static int coroutine_fn blkreplay_co_pwritev(BlockDriverState *bs,
     uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
-    uint64_t reqid = request_id++;
+    uint64_t reqid = blkreplay_next_id();
     int ret = bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
@@ -106,7 +101,7 @@ static int coroutine_fn blkreplay_co_pwritev(BlockDriverState *bs,
 static int coroutine_fn blkreplay_co_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int count, BdrvRequestFlags flags)
 {
-    uint64_t reqid = request_id++;
+    uint64_t reqid = blkreplay_next_id();
     int ret = bdrv_co_pwrite_zeroes(bs->file, offset, count, flags);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
@@ -117,7 +112,7 @@ static int coroutine_fn blkreplay_co_pwrite_zeroes(BlockDriverState *bs,
 static int coroutine_fn blkreplay_co_pdiscard(BlockDriverState *bs,
                                               int64_t offset, int count)
 {
-    uint64_t reqid = request_id++;
+    uint64_t reqid = blkreplay_next_id();
     int ret = bdrv_co_pdiscard(bs->file->bs, offset, count);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
@@ -127,7 +122,7 @@ static int coroutine_fn blkreplay_co_pdiscard(BlockDriverState *bs,
 
 static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs)
 {
-    uint64_t reqid = request_id++;
+    uint64_t reqid = blkreplay_next_id();
     int ret = bdrv_co_flush(bs->file->bs);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
diff --git a/cpus.c b/cpus.c
index e39ccb7..2f5684f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -751,6 +751,7 @@ static int do_vm_stop(RunState state)
     }
 
     bdrv_drain_all();
+    replay_disable_events();
     ret = blk_flush_all();
 
     return ret;
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index 0a88393..f80d6d2 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -105,6 +105,8 @@ bool replay_checkpoint(ReplayCheckpoint checkpoint);
 
 /*! Disables storing events in the queue */
 void replay_disable_events(void);
+/*! Enables storing events in the queue */
+void replay_enable_events(void);
 /*! Returns true when saving events is enabled */
 bool replay_events_enabled(void);
 /*! Adds bottom half event to the queue */
@@ -115,6 +117,8 @@ void replay_input_event(QemuConsole *src, InputEvent *evt);
 void replay_input_sync_event(void);
 /*! Adds block layer event to the queue */
 void replay_block_event(QEMUBH *bh, uint64_t id);
+/*! Returns ID for the next block event */
+uint64_t blkreplay_next_id(void);
 
 /* Character device */
 
diff --git a/replay/replay-events.c b/replay/replay-events.c
index 4eb2ea3..c513913 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -309,3 +309,11 @@ bool replay_events_enabled(void)
 {
     return events_enabled;
 }
+
+uint64_t blkreplay_next_id(void)
+{
+    if (replay_events_enabled()) {
+        return replay_state.block_request_id++;
+    }
+    return 0;
+}
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index e07eb7d..9117e44 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -68,6 +68,10 @@ typedef struct ReplayState {
     unsigned int has_unread_data;
     /*! Temporary variable for saving current log offset. */
     uint64_t file_offset;
+    /*! Next block operation id.
+        This counter is global, because requests from different
+        block devices should not get overlapping ids. */
+    uint64_t block_request_id;
 } ReplayState;
 extern ReplayState replay_state;
 
@@ -123,8 +127,6 @@ void replay_read_next_clock(unsigned int kind);
 void replay_init_events(void);
 /*! Clears internal data structures for events handling */
 void replay_finish_events(void);
-/*! Enables storing events in the queue */
-void replay_enable_events(void);
 /*! Flushes events queue */
 void replay_flush_events(void);
 /*! Clears events list before loading new VM state */
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 220248f..5609ea0 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -50,6 +50,7 @@ static const VMStateDescription vmstate_replay = {
         VMSTATE_UINT32(data_kind, ReplayState),
         VMSTATE_UINT32(has_unread_data, ReplayState),
         VMSTATE_UINT64(file_offset, ReplayState),
+        VMSTATE_UINT64(block_request_id, ReplayState),
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/stubs/replay.c b/stubs/replay.c
index de9fa1e..d9a6da9 100644
--- a/stubs/replay.c
+++ b/stubs/replay.c
@@ -67,3 +67,8 @@ void replay_char_read_all_save_buf(uint8_t *buf, int offset)
 void replay_block_event(QEMUBH *bh, uint64_t id)
 {
 }
+
+uint64_t blkreplay_next_id(void)
+{
+    return 0;
+}
diff --git a/vl.c b/vl.c
index fca0487..c92e319 100644
--- a/vl.c
+++ b/vl.c
@@ -746,6 +746,7 @@ void vm_start(void)
     if (runstate_is_running()) {
         qapi_event_send_stop(&error_abort);
     } else {
+        replay_enable_events();
         cpu_enable_ticks();
         runstate_set(RUN_STATE_RUNNING);
         vm_state_notify(1, RUN_STATE_RUNNING);

[Qemu-devel] [PATCH v5 4/9] record/replay: add network support

[Pavel Dovgalyuk]

This patch adds support of recording and replaying network packets in
irount rr mode.

Record and replay for network interactions is performed with the network filter.
Each backend must have its own instance of the replay filter as follows:
 -netdev user,id=net1 -device rtl8139,netdev=net1
 -object filter-replay,id=replay,netdev=net1

Replay network filter is used to record and replay network packets. While
recording the virtual machine this filter puts all packets coming from
the outer world into the log. In replay mode packets from the log are
injected into the network device. All interactions with network backend
in replay mode are disabled.

v5 changes:
 - using iov_to_buf function instead of loop

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 docs/replay.txt          |   14 ++++++
 include/sysemu/replay.h  |   12 +++++
 net/Makefile.objs        |    1 
 net/filter-replay.c      |   92 +++++++++++++++++++++++++++++++++++++++++
 replay/Makefile.objs     |    1 
 replay/replay-events.c   |   11 +++++
 replay/replay-internal.h |   10 +++++
 replay/replay-net.c      |  102 ++++++++++++++++++++++++++++++++++++++++++++++
 replay/replay.c          |    2 -
 vl.c                     |    3 +
 10 files changed, 246 insertions(+), 2 deletions(-)
 create mode 100644 net/filter-replay.c
 create mode 100644 replay/replay-net.c

diff --git a/docs/replay.txt b/docs/replay.txt
index 779c6c0..347b2ff 100644
--- a/docs/replay.txt
+++ b/docs/replay.txt
@@ -195,3 +195,17 @@ Queue is flushed at checkpoints and information about processed requests
 is recorded to the log. In replay phase the queue is matched with
 events read from the log. Therefore block devices requests are processed
 deterministically.
+
+Network devices
+---------------
+
+Record and replay for network interactions is performed with the network filter.
+Each backend must have its own instance of the replay filter as follows:
+ -netdev user,id=net1 -device rtl8139,netdev=net1
+ -object filter-replay,id=replay,netdev=net1
+
+Replay network filter is used to record and replay network packets. While
+recording the virtual machine this filter puts all packets coming from
+the outer world into the log. In replay mode packets from the log are
+injected into the network device. All interactions with network backend
+in replay mode are disabled.
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index f80d6d2..abb35ca 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -39,6 +39,8 @@ enum ReplayCheckpoint {
 };
 typedef enum ReplayCheckpoint ReplayCheckpoint;
 
+typedef struct ReplayNetState ReplayNetState;
+
 extern ReplayMode replay_mode;
 
 /* Replay process control functions */
@@ -137,4 +139,14 @@ void replay_char_read_all_save_error(int res);
 /*! Writes character read_all execution result into the replay log. */
 void replay_char_read_all_save_buf(uint8_t *buf, int offset);
 
+/* Network */
+
+/*! Registers replay network filter attached to some backend. */
+ReplayNetState *replay_register_net(NetFilterState *nfs);
+/*! Unregisters replay network filter. */
+void replay_unregister_net(ReplayNetState *rns);
+/*! Called to write network packet to the replay log. */
+void replay_net_packet_event(ReplayNetState *rns, unsigned flags,
+                             const struct iovec *iov, int iovcnt);
+
 #endif
diff --git a/net/Makefile.objs b/net/Makefile.objs
index b7c22fd..f787ba4 100644
--- a/net/Makefile.objs
+++ b/net/Makefile.objs
@@ -16,3 +16,4 @@ common-obj-$(CONFIG_NETMAP) += netmap.o
 common-obj-y += filter.o
 common-obj-y += filter-buffer.o
 common-obj-y += filter-mirror.o
+common-obj-y += filter-replay.o
diff --git a/net/filter-replay.c b/net/filter-replay.c
new file mode 100644
index 0000000..cff65f8
--- /dev/null
+++ b/net/filter-replay.c
@@ -0,0 +1,92 @@
+/*
+ * filter-replay.c
+ *
+ * Copyright (c) 2010-2016 Institute for System Programming
+ *                         of the Russian Academy of Sciences.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "clients.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "qemu/error-report.h"
+#include "qemu/iov.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+#include "qapi/visitor.h"
+#include "net/filter.h"
+#include "sysemu/replay.h"
+
+#define TYPE_FILTER_REPLAY "filter-replay"
+
+#define FILTER_REPLAY(obj) \
+    OBJECT_CHECK(NetFilterReplayState, (obj), TYPE_FILTER_REPLAY)
+
+struct NetFilterReplayState {
+    NetFilterState nfs;
+    ReplayNetState *rns;
+};
+typedef struct NetFilterReplayState NetFilterReplayState;
+
+static ssize_t filter_replay_receive_iov(NetFilterState *nf,
+                                         NetClientState *sndr,
+                                         unsigned flags,
+                                         const struct iovec *iov,
+                                         int iovcnt, NetPacketSent *sent_cb)
+{
+    NetFilterReplayState *nfrs = FILTER_REPLAY(nf);
+    switch (replay_mode) {
+    case REPLAY_MODE_RECORD:
+        if (nf->netdev == sndr) {
+            replay_net_packet_event(nfrs->rns, flags, iov, iovcnt);
+            return iov_size(iov, iovcnt);
+        }
+        return 0;
+    case REPLAY_MODE_PLAY:
+        /* Drop all packets in replay mode.
+           Packets from the log will be injected by the replay module. */
+        return iov_size(iov, iovcnt);
+    default:
+        /* Pass all the packets. */
+        return 0;
+    }
+}
+
+static void filter_replay_instance_init(Object *obj)
+{
+    NetFilterReplayState *nfrs = FILTER_REPLAY(obj);
+    nfrs->rns = replay_register_net(&nfrs->nfs);
+}
+
+static void filter_replay_instance_finalize(Object *obj)
+{
+    NetFilterReplayState *nfrs = FILTER_REPLAY(obj);
+    replay_unregister_net(nfrs->rns);
+}
+
+static void filter_replay_class_init(ObjectClass *oc, void *data)
+{
+    NetFilterClass *nfc = NETFILTER_CLASS(oc);
+
+    nfc->receive_iov = filter_replay_receive_iov;
+}
+
+static const TypeInfo filter_replay_info = {
+    .name = TYPE_FILTER_REPLAY,
+    .parent = TYPE_NETFILTER,
+    .class_init = filter_replay_class_init,
+    .instance_init = filter_replay_instance_init,
+    .instance_finalize = filter_replay_instance_finalize,
+    .instance_size = sizeof(NetFilterReplayState),
+};
+
+static void filter_replay_register_types(void)
+{
+    type_register_static(&filter_replay_info);
+}
+
+type_init(filter_replay_register_types);
diff --git a/replay/Makefile.objs b/replay/Makefile.objs
index c8ad3eb..b2afd40 100644
--- a/replay/Makefile.objs
+++ b/replay/Makefile.objs
@@ -5,3 +5,4 @@ common-obj-y += replay-time.o
 common-obj-y += replay-input.o
 common-obj-y += replay-char.o
 common-obj-y += replay-snapshot.o
+common-obj-y += replay-net.o
diff --git a/replay/replay-events.c b/replay/replay-events.c
index c513913..94a6dcc 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -54,6 +54,9 @@ static void replay_run_event(Event *event)
     case REPLAY_ASYNC_EVENT_BLOCK:
         aio_bh_call(event->opaque);
         break;
+    case REPLAY_ASYNC_EVENT_NET:
+        replay_event_net_run(event->opaque);
+        break;
     default:
         error_report("Replay: invalid async event ID (%d) in the queue",
                     event->event_kind);
@@ -189,6 +192,9 @@ static void replay_save_event(Event *event, int checkpoint)
         case REPLAY_ASYNC_EVENT_BLOCK:
             replay_put_qword(event->id);
             break;
+        case REPLAY_ASYNC_EVENT_NET:
+            replay_event_net_save(event->opaque);
+            break;
         default:
             error_report("Unknown ID %" PRId64 " of replay event", event->id);
             exit(1);
@@ -252,6 +258,11 @@ static Event *replay_read_event(int checkpoint)
             read_id = replay_get_qword();
         }
         break;
+    case REPLAY_ASYNC_EVENT_NET:
+        event = g_malloc0(sizeof(Event));
+        event->event_kind = read_event_kind;
+        event->opaque = replay_event_net_load();
+        return event;
     default:
         error_report("Unknown ID %d of replay event", read_event_kind);
         exit(1);
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index 9117e44..c26d079 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -50,6 +50,7 @@ enum ReplayAsyncEventKind {
     REPLAY_ASYNC_EVENT_INPUT_SYNC,
     REPLAY_ASYNC_EVENT_CHAR_READ,
     REPLAY_ASYNC_EVENT_BLOCK,
+    REPLAY_ASYNC_EVENT_NET,
     REPLAY_ASYNC_COUNT
 };
 
@@ -161,6 +162,15 @@ void replay_event_char_read_save(void *opaque);
 /*! Reads char event read from the file. */
 void *replay_event_char_read_load(void);
 
+/* Network devices */
+
+/*! Called to run network event. */
+void replay_event_net_run(void *opaque);
+/*! Writes network event to the file. */
+void replay_event_net_save(void *opaque);
+/*! Reads network from the file. */
+void *replay_event_net_load(void);
+
 /* VMState-related functions */
 
 /* Registers replay VMState.
diff --git a/replay/replay-net.c b/replay/replay-net.c
new file mode 100644
index 0000000..0838eab
--- /dev/null
+++ b/replay/replay-net.c
@@ -0,0 +1,102 @@
+/*
+ * replay-net.c
+ *
+ * Copyright (c) 2010-2016 Institute for System Programming
+ *                         of the Russian Academy of Sciences.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "sysemu/replay.h"
+#include "replay-internal.h"
+#include "sysemu/sysemu.h"
+#include "net/net.h"
+#include "net/filter.h"
+#include "qemu/iov.h"
+
+struct ReplayNetState {
+    NetFilterState *nfs;
+    int id;
+};
+
+typedef struct NetEvent {
+    uint8_t id;
+    uint32_t flags;
+    uint8_t *data;
+    size_t size;
+} NetEvent;
+
+static NetFilterState **network_filters;
+static int network_filters_count;
+
+ReplayNetState *replay_register_net(NetFilterState *nfs)
+{
+    ReplayNetState *rns = g_new0(ReplayNetState, 1);
+    rns->nfs = nfs;
+    rns->id = network_filters_count++;
+    network_filters = g_realloc(network_filters,
+                                network_filters_count
+                                    * sizeof(*network_filters));
+    network_filters[network_filters_count - 1] = nfs;
+    return rns;
+}
+
+void replay_unregister_net(ReplayNetState *rns)
+{
+    network_filters[rns->id] = NULL;
+    g_free(rns);
+}
+
+void replay_net_packet_event(ReplayNetState *rns, unsigned flags,
+                             const struct iovec *iov, int iovcnt)
+{
+    NetEvent *event = g_new(NetEvent, 1);
+    event->flags = flags;
+    event->data = g_malloc(iov_size(iov, iovcnt));
+    event->size = iov_size(iov, iovcnt);
+    event->id = rns->id;
+    iov_to_buf(iov, iovcnt, 0, event->data, event->size);
+
+    replay_add_event(REPLAY_ASYNC_EVENT_NET, event, NULL, 0);
+}
+
+void replay_event_net_run(void *opaque)
+{
+    NetEvent *event = opaque;
+    struct iovec iov = {
+        .iov_base = (void *)event->data,
+        .iov_len = event->size
+    };
+
+    assert(event->id < network_filters_count);
+
+    qemu_netfilter_pass_to_next(network_filters[event->id]->netdev,
+        event->flags, &iov, 1, network_filters[event->id]);
+
+    g_free(event->data);
+    g_free(event);
+}
+
+void replay_event_net_save(void *opaque)
+{
+    NetEvent *event = opaque;
+
+    replay_put_byte(event->id);
+    replay_put_dword(event->flags);
+    replay_put_array(event->data, event->size);
+}
+
+void *replay_event_net_load(void)
+{
+    NetEvent *event = g_new(NetEvent, 1);
+
+    event->id = replay_get_byte();
+    event->flags = replay_get_dword();
+    replay_get_array_alloc(&event->data, &event->size);
+
+    return event;
+}
diff --git a/replay/replay.c b/replay/replay.c
index c797aea..7f27cf1 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -21,7 +21,7 @@
 
 /* Current version of the replay mechanism.
    Increase it when file format changes. */
-#define REPLAY_VERSION              0xe02004
+#define REPLAY_VERSION              0xe02005
 /* Size of replay log header */
 #define HEADER_SIZE                 (sizeof(uint32_t) + sizeof(uint64_t))
 
diff --git a/vl.c b/vl.c
index c92e319..1298be1 100644
--- a/vl.c
+++ b/vl.c
@@ -2808,7 +2808,8 @@ static bool object_create_initial(const char *type)
     if (g_str_equal(type, "filter-buffer") ||
         g_str_equal(type, "filter-dump") ||
         g_str_equal(type, "filter-mirror") ||
-        g_str_equal(type, "filter-redirector")) {
+        g_str_equal(type, "filter-redirector") ||
+        g_str_equal(type, "filter-replay")) {
         return false;
     }
 

[Qemu-devel] [PATCH v5 5/9] savevm: add public save_vmstate function

[Pavel Dovgalyuk]

This patch introduces save_vmstate function to allow saving and loading
vmstates from the replay module.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 include/sysemu/sysemu.h |    1 +
 migration/savevm.c      |   33 ++++++++++++++++++++++-----------
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index ee7c760..b471b1e 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -78,6 +78,7 @@ void qemu_add_machine_init_done_notifier(Notifier *notify);
 void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
 void hmp_savevm(Monitor *mon, const QDict *qdict);
+int save_vmstate(Monitor *mon, const char *name);
 int load_vmstate(const char *name);
 void hmp_delvm(Monitor *mon, const QDict *qdict);
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
diff --git a/migration/savevm.c b/migration/savevm.c
index 33a2911..f571da6 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1958,38 +1958,40 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }
 
-void hmp_savevm(Monitor *mon, const QDict *qdict)
+int save_vmstate(Monitor *mon, const char *name)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
-    int ret;
+    int ret = -1;
     QEMUFile *f;
     int saved_vm_running;
     uint64_t vm_state_size;
     qemu_timeval tv;
     struct tm tm;
-    const char *name = qdict_get_try_str(qdict, "name");
     Error *local_err = NULL;
     AioContext *aio_context;
 
     if (!bdrv_all_can_snapshot(&bs)) {
         monitor_printf(mon, "Device '%s' is writable but does not "
                        "support snapshots.\n", bdrv_get_device_name(bs));
-        return;
+        return ret;
     }
 
     /* Delete old snapshots of the same name */
-    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
-        error_reportf_err(local_err,
-                          "Error while deleting snapshot on device '%s': ",
-                          bdrv_get_device_name(bs1));
-        return;
+    if (name) {
+        ret = bdrv_all_delete_snapshot(name, &bs1, &local_err);
+        if (ret < 0) {
+            error_reportf_err(local_err,
+                              "Error while deleting snapshot on device '%s': ",
+                              bdrv_get_device_name(bs1));
+            return ret;
+        }
     }
 
     bs = bdrv_all_find_vmstate_bs();
     if (bs == NULL) {
         monitor_printf(mon, "No block device can accept snapshots\n");
-        return;
+        return ret;
     }
     aio_context = bdrv_get_aio_context(bs);
 
@@ -1998,7 +2000,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     ret = global_state_store();
     if (ret) {
         monitor_printf(mon, "Error saving global state\n");
-        return;
+        return ret;
     }
     vm_stop(RUN_STATE_SAVE_VM);
 
@@ -2044,13 +2046,22 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     if (ret < 0) {
         monitor_printf(mon, "Error while creating snapshot on '%s'\n",
                        bdrv_get_device_name(bs));
+        goto the_end;
     }
 
+    ret = 0;
+
  the_end:
     aio_context_release(aio_context);
     if (saved_vm_running) {
         vm_start();
     }
+    return ret;
+}
+
+void hmp_savevm(Monitor *mon, const QDict *qdict)
+{
+    save_vmstate(mon, qdict_get_try_str(qdict, "name"));
 }
 
 void qmp_xen_save_devices_state(const char *filename, Error **errp)

[Qemu-devel] [PATCH v5 6/9] replay: save/load initial state

[Pavel Dovgalyuk]

This patch implements initial vmstate creation or loading at the start
of record/replay. It is needed for rewinding the execution in the replay mode.

v4 changes:
 - snapshots are not created by default anymore

v3 changes:
 - added rrsnapshot option

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 docs/replay.txt          |   16 ++++++++++++++++
 include/sysemu/replay.h  |    9 +++++++++
 qemu-options.hx          |    8 ++++++--
 replay/replay-snapshot.c |   17 +++++++++++++++++
 replay/replay.c          |    5 +++++
 vl.c                     |   10 ++++++++--
 6 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/docs/replay.txt b/docs/replay.txt
index 347b2ff..03e1931 100644
--- a/docs/replay.txt
+++ b/docs/replay.txt
@@ -196,6 +196,22 @@ is recorded to the log. In replay phase the queue is matched with
 events read from the log. Therefore block devices requests are processed
 deterministically.
 
+Snapshotting
+------------
+
+New VM snapshots may be created in replay mode. They can be used later
+to recover the desired VM state. All VM states created in replay mode
+are associated with the moment of time in the replay scenario.
+After recovering the VM state replay will start from that position.
+
+Default starting snapshot name may be specified with icount field
+rrsnapshot as follows:
+ -icount shift=7,rr=record,rrfile=replay.bin,rrsnapshot=snapshot_name
+
+This snapshot is created at start of recording and restored at start
+of replaying. It also can be loaded while replaying to roll back
+the execution.
+
 Network devices
 ---------------
 
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index abb35ca..740b425 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -43,6 +43,9 @@ typedef struct ReplayNetState ReplayNetState;
 
 extern ReplayMode replay_mode;
 
+/* Name of the initial VM snapshot */
+extern char *replay_snapshot;
+
 /* Replay process control functions */
 
 /*! Enables recording or saving event log with specified parameters */
@@ -149,4 +152,10 @@ void replay_unregister_net(ReplayNetState *rns);
 void replay_net_packet_event(ReplayNetState *rns, unsigned flags,
                              const struct iovec *iov, int iovcnt);
 
+/* VM state operations */
+
+/*! Called at the start of execution.
+    Loads or saves initial vmstate depending on execution mode. */
+void replay_vmstate_init(void);
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 0b621bb..1aa046e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3367,12 +3367,12 @@ re-inject them.
 ETEXI
 
 DEF("icount", HAS_ARG, QEMU_OPTION_icount, \
-    "-icount [shift=N|auto][,align=on|off][,sleep=on|off,rr=record|replay,rrfile=<filename>]\n" \
+    "-icount [shift=N|auto][,align=on|off][,sleep=on|off,rr=record|replay,rrfile=<filename>,rrsnapshot=<snapshot>]\n" \
     "                enable virtual instruction counter with 2^N clock ticks per\n" \
     "                instruction, enable aligning the host and virtual clocks\n" \
     "                or disable real time cpu sleeping\n", QEMU_ARCH_ALL)
 STEXI
-@item -icount [shift=@var{N}|auto][,rr=record|replay,rrfile=@var{filename}]
+@item -icount [shift=@var{N}|auto][,rr=record|replay,rrfile=@var{filename},rrsnapshot=@var{snapshot}]
 @findex -icount
 Enable virtual instruction counter.  The virtual cpu will execute one
 instruction every 2^@var{N} ns of virtual time.  If @code{auto} is specified
@@ -3405,6 +3405,10 @@ when the shift value is high (how high depends on the host machine).
 When @option{rr} option is specified deterministic record/replay is enabled.
 Replay log is written into @var{filename} file in record mode and
 read from this file in replay mode.
+
+Option rrsnapshot is used to create new vm snapshot named @var{snapshot}
+at the start of execution recording. In replay mode this option is used
+to load the initial VM state.
 ETEXI
 
 DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 5609ea0..9a57026 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -58,4 +58,21 @@ static const VMStateDescription vmstate_replay = {
 void replay_vmstate_register(void)
 {
     vmstate_register(NULL, 0, &vmstate_replay, &replay_state);
+}
+
+void replay_vmstate_init(void)
+{
+    if (replay_snapshot) {
+        if (replay_mode == REPLAY_MODE_RECORD) {
+            if (save_vmstate(cur_mon, replay_snapshot) != 0) {
+                error_report("Could not create snapshot for icount record");
+                exit(1);
+            }
+        } else if (replay_mode == REPLAY_MODE_PLAY) {
+            if (load_vmstate(replay_snapshot) != 0) {
+                error_report("Could not load snapshot for icount replay");
+                exit(1);
+            }
+        }
+    }
 }
diff --git a/replay/replay.c b/replay/replay.c
index 7f27cf1..1835b99 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -26,6 +26,7 @@
 #define HEADER_SIZE                 (sizeof(uint32_t) + sizeof(uint64_t))
 
 ReplayMode replay_mode = REPLAY_MODE_NONE;
+char *replay_snapshot;
 
 /* Name of replay file  */
 static char *replay_filename;
@@ -292,6 +293,7 @@ void replay_configure(QemuOpts *opts)
         exit(1);
     }
 
+    replay_snapshot = g_strdup(qemu_opt_get(opts, "rrsnapshot"));
     replay_vmstate_register();
     replay_enable(fname, mode);
 
@@ -346,6 +348,9 @@ void replay_finish(void)
         replay_filename = NULL;
     }
 
+    g_free(replay_snapshot);
+    replay_snapshot = NULL;
+
     replay_finish_events();
     replay_mutex_destroy();
 }
diff --git a/vl.c b/vl.c
index 1298be1..e1c90ef 100644
--- a/vl.c
+++ b/vl.c
@@ -460,6 +460,9 @@ static QemuOptsList qemu_icount_opts = {
         }, {
             .name = "rrfile",
             .type = QEMU_OPT_STRING,
+        }, {
+            .name = "rrsnapshot",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
@@ -4411,7 +4414,8 @@ int main(int argc, char **argv, char **envp)
     }
 
     /* open the virtual block devices */
-    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
+    if (snapshot
+        || (replay_mode != REPLAY_MODE_NONE && !replay_snapshot)) {
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
                           NULL, NULL);
     }
@@ -4591,7 +4595,9 @@ int main(int argc, char **argv, char **envp)
     replay_checkpoint(CHECKPOINT_RESET);
     qemu_system_reset(VMRESET_SILENT);
     register_global_state();
-    if (loadvm) {
+    if (replay_mode != REPLAY_MODE_NONE) {
+        replay_vmstate_init();
+    } else if (loadvm) {
         if (load_vmstate(loadvm) < 0) {
             autostart = 0;
         }

[Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

This patch disables snapshotting for block driver filters.
It is needed, because snapshots should be created
in underlying disk images, not in filters itself.

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

diff --git a/block/snapshot.c b/block/snapshot.c
index bf5c2ca..8998b8b 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -184,6 +184,9 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
     if (!drv) {
         return -ENOMEDIUM;
     }
+    if (drv->is_filter) {
+        return 0;
+    }
     if (drv->bdrv_snapshot_goto) {
         return drv->bdrv_snapshot_goto(bs, snapshot_id);
     }

[Qemu-devel] [PATCH v5 8/9] block: implement bdrv_recurse_is_first_non_filter for blkreplay

[Pavel Dovgalyuk]

This patch adds bdrv_recurse_is_first_non_filter implementation
for blkreplay driver. It allows creating snapshots when blkreplay
is enabled.

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

diff --git a/block/blkreplay.c b/block/blkreplay.c
index a741654..5a13fb3 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 bool blkreplay_recurse_is_first_non_filter(BlockDriverState *bs,
+                                                  BlockDriverState *candidate)
+{
+    return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
+}
+
 static BlockDriver bdrv_blkreplay = {
     .format_name            = "blkreplay",
     .protocol_name          = "blkreplay",
@@ -145,6 +151,9 @@ static BlockDriver bdrv_blkreplay = {
     .bdrv_co_pwrite_zeroes  = blkreplay_co_pwrite_zeroes,
     .bdrv_co_pdiscard       = blkreplay_co_pdiscard,
     .bdrv_co_flush          = blkreplay_co_flush,
+
+    .is_filter              = true,
+    .bdrv_recurse_is_first_non_filter = blkreplay_recurse_is_first_non_filter,
 };
 
 static void bdrv_blkreplay_init(void)

[Qemu-devel] [PATCH v5 9/9] integratorcp: adding vmstate for save/restore

[Pavel Dovgalyuk]

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

VMState added by this patch preserves correct
loading of the integratorcp device state.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/arm/integratorcp.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 96dc150..3653df2 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -53,6 +53,27 @@ static uint8_t integrator_spd[128] = {
    0xe, 4, 0x1c, 1, 2, 0x20, 0xc0, 0, 0, 0, 0, 0x30, 0x28, 0x30, 0x28, 0x40
 };
 
+static const VMStateDescription vmstate_integratorcm = {
+    .name = "integratorcm",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32(cm_osc, IntegratorCMState),
+        VMSTATE_UINT32(cm_ctrl, IntegratorCMState),
+        VMSTATE_UINT32(cm_lock, IntegratorCMState),
+        VMSTATE_UINT32(cm_auxosc, IntegratorCMState),
+        VMSTATE_UINT32(cm_sdram, IntegratorCMState),
+        VMSTATE_UINT32(cm_init, IntegratorCMState),
+        VMSTATE_UINT32(cm_flags, IntegratorCMState),
+        VMSTATE_UINT32(cm_nvflags, IntegratorCMState),
+        VMSTATE_UINT32(int_level, IntegratorCMState),
+        VMSTATE_UINT32(irq_enabled, IntegratorCMState),
+        VMSTATE_UINT32(fiq_enabled, IntegratorCMState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static uint64_t integratorcm_read(void *opaque, hwaddr offset,
                                   unsigned size)
 {
@@ -303,6 +324,19 @@ typedef struct icp_pic_state {
     qemu_irq parent_fiq;
 } icp_pic_state;
 
+static const VMStateDescription vmstate_icp_pic = {
+    .name = "icp_pic",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32(level, icp_pic_state),
+        VMSTATE_UINT32(irq_enabled, icp_pic_state),
+        VMSTATE_UINT32(fiq_enabled, icp_pic_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void icp_pic_update(icp_pic_state *s)
 {
     uint32_t flags;
@@ -432,6 +466,17 @@ typedef struct ICPCtrlRegsState {
 #define ICP_INTREG_WPROT        (1 << 0)
 #define ICP_INTREG_CARDIN       (1 << 3)
 
+static const VMStateDescription vmstate_icp_control = {
+    .name = "icp_control",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32(intreg_state, ICPCtrlRegsState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static uint64_t icp_control_read(void *opaque, hwaddr offset,
                                  unsigned size)
 {
@@ -633,6 +678,21 @@ static void core_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->props = core_properties;
+    dc->vmsd = &vmstate_integratorcm;
+}
+
+static void icp_pic_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &vmstate_icp_pic;
+}
+
+static void icp_control_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &vmstate_icp_control;
 }
 
 static const TypeInfo core_info = {
@@ -648,6 +708,7 @@ static const TypeInfo icp_pic_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(icp_pic_state),
     .instance_init = icp_pic_init,
+    .class_init    = icp_pic_class_init,
 };
 
 static const TypeInfo icp_ctrl_regs_info = {
@@ -655,6 +716,7 @@ static const TypeInfo icp_ctrl_regs_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(ICPCtrlRegsState),
     .instance_init = icp_control_init,
+    .class_init    = icp_control_class_init,
 };
 
 static void integratorcp_register_types(void)

Re: [Qemu-devel] [PATCH v5 5/9] savevm: add public save_vmstate function

[Paolo Bonzini]

On 26/09/2016 10:08, Pavel Dovgalyuk wrote:
> -void hmp_savevm(Monitor *mon, const QDict *qdict)
> +int save_vmstate(Monitor *mon, const char *name)

Please change this to return an Error **.

Paolo

>  {
>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> -    int ret;
> +    int ret = -1;
>      QEMUFile *f;
>      int saved_vm_running;
>      uint64_t vm_state_size;
>      qemu_timeval tv;
>      struct tm tm;
> -    const char *name = qdict_get_try_str(qdict, "name");
>      Error *local_err = NULL;
>      AioContext *aio_context;
>  
>      if (!bdrv_all_can_snapshot(&bs)) {
>          monitor_printf(mon, "Device '%s' is writable but does not "
>                         "support snapshots.\n", bdrv_get_device_name(bs));
> -        return;
> +        return ret;
>      }
>  
>      /* Delete old snapshots of the same name */
> -    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
> -        error_reportf_err(local_err,
> -                          "Error while deleting snapshot on device '%s': ",
> -                          bdrv_get_device_name(bs1));
> -        return;
> +    if (name) {
> +        ret = bdrv_all_delete_snapshot(name, &bs1, &local_err);
> +        if (ret < 0) {
> +            error_reportf_err(local_err,
> +                              "Error while deleting snapshot on device '%s': ",
> +                              bdrv_get_device_name(bs1));
> +            return ret;
> +        }
>      }
>  
>      bs = bdrv_all_find_vmstate_bs();
>      if (bs == NULL) {
>          monitor_printf(mon, "No block device can accept snapshots\n");
> -        return;
> +        return ret;
>      }

Re: [Qemu-devel] [PATCH v5 0/9] replay additions

[Paolo Bonzini]

On 26/09/2016 10:07, Pavel Dovgalyuk wrote:
> This set of patches includes several fixes for replay and
> adds network record/replay for network devices. It also makes possible
> saving/restoring vmstate in replay mode.
> 
> Record and replay for network interactions is performed with the network filter.
> Each backend must have its own instance of the replay filter as follows:
>  -netdev user,id=net1 -device rtl8139,netdev=net1
>  -object filter-replay,id=replay,netdev=net1
> 
> This patches add rrsnapshot option for icount. rrshapshot option creates
> start snapshot at record and loads it at replay. It allows preserving
> the state of disk images used by virtual machine. This vm state can also
> use used to roll back the execution while replaying.
> 
> This set of patches includes fixes and additions for icount and
> record/replay implementation:
>  - VM start/stop in replay mode
>  - Network interaction record/replay
>  - overlay option for blkreplay filter
>  - rrsnapshot option for record/replay
>  - vmstate fix for integratorcp ARM platform
> 
> v5 changes:
>  - Recording is stopped when initial snapshot cannot be created
>  - Minor changes

Queued patches 1-3, thanks.

Paolo

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf]

Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> This patch disables snapshotting for block driver filters.
> It is needed, because snapshots should be created
> in underlying disk images, not in filters itself.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>

But that's exactly what the existing code implements? If a driver
doesn't provide .bdrv_snapshot_goto, the request is redirected to
bs->file.

>  block/snapshot.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index bf5c2ca..8998b8b 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -184,6 +184,9 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>      if (!drv) {
>          return -ENOMEDIUM;
>      }
> +    if (drv->is_filter) {
> +        return 0;
> +    }

This, on the other hand, doesn't redirect the request, but silently
ignores it. That is, loading the snapshot will apparently succeed, but
it wouldn't actually load anything and the disk would stay in its
current state.

>      if (drv->bdrv_snapshot_goto) {
>          return drv->bdrv_snapshot_goto(bs, snapshot_id);
>      }

Kevin

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > This patch disables snapshotting for block driver filters.
> > It is needed, because snapshots should be created
> > in underlying disk images, not in filters itself.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> 
> But that's exactly what the existing code implements? If a driver
> doesn't provide .bdrv_snapshot_goto, the request is redirected to
> bs->file.
> 
> >  block/snapshot.c |    3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/block/snapshot.c b/block/snapshot.c
> > index bf5c2ca..8998b8b 100644
> > --- a/block/snapshot.c
> > +++ b/block/snapshot.c
> > @@ -184,6 +184,9 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> >      if (!drv) {
> >          return -ENOMEDIUM;
> >      }
> > +    if (drv->is_filter) {
> > +        return 0;
> > +    }
> 
> This, on the other hand, doesn't redirect the request, but silently
> ignores it. That is, loading the snapshot will apparently succeed, but
> it wouldn't actually load anything and the disk would stay in its
> current state.

In my use case bdrv_all_goto_snapshot iterates all block drivers, including
filters and disk images. Therefore skipping goto for images is ok.

Maybe I should move this check to bdrv_can_snapshot?

> 
> >      if (drv->bdrv_snapshot_goto) {
> >          return drv->bdrv_snapshot_goto(bs, snapshot_id);
> >      }


Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf]

Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > > This patch disables snapshotting for block driver filters.
> > > It is needed, because snapshots should be created
> > > in underlying disk images, not in filters itself.
> > >
> > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > 
> > But that's exactly what the existing code implements? If a driver
> > doesn't provide .bdrv_snapshot_goto, the request is redirected to
> > bs->file.
> > 
> > >  block/snapshot.c |    3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/block/snapshot.c b/block/snapshot.c
> > > index bf5c2ca..8998b8b 100644
> > > --- a/block/snapshot.c
> > > +++ b/block/snapshot.c
> > > @@ -184,6 +184,9 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > >      if (!drv) {
> > >          return -ENOMEDIUM;
> > >      }
> > > +    if (drv->is_filter) {
> > > +        return 0;
> > > +    }
> > 
> > This, on the other hand, doesn't redirect the request, but silently
> > ignores it. That is, loading the snapshot will apparently succeed, but
> > it wouldn't actually load anything and the disk would stay in its
> > current state.
> 
> In my use case bdrv_all_goto_snapshot iterates all block drivers, including
> filters and disk images. Therefore skipping goto for images is ok.

Hm, this can happy today indeed.

Originally, we only called bdrv_goto_snapshot() for all _top level_
BDSes, and this is still what you normally get. However, if you
explicitly create a BDS (e.g. with its own -drive option), it is
considered a top level BDS without actually being top level for the
guest, and therefore the snapshotting function is called for it.

Of course, this is highly inefficient because the goto_snapshot request
is passed by the filter driver and then called another time for the
lower node, effectively loading the snapshot a second time.

On the other hand if you use a single -drive option to create both the
qcow2 BDS and the blkreplay filter, we do need to pass down the
goto_snapshot request because it won't be called for the qcow2 layer
otherwise.

I'm not completely sure yet what the right behaviour would be here.

Kevin

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > > > This patch disables snapshotting for block driver filters.
> > > > It is needed, because snapshots should be created
> > > > in underlying disk images, not in filters itself.
> > > >
> > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > >
> > > But that's exactly what the existing code implements? If a driver
> > > doesn't provide .bdrv_snapshot_goto, the request is redirected to
> > > bs->file.
> > >
> > > >  block/snapshot.c |    3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/block/snapshot.c b/block/snapshot.c
> > > > index bf5c2ca..8998b8b 100644
> > > > --- a/block/snapshot.c
> > > > +++ b/block/snapshot.c
> > > > @@ -184,6 +184,9 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > > >      if (!drv) {
> > > >          return -ENOMEDIUM;
> > > >      }
> > > > +    if (drv->is_filter) {
> > > > +        return 0;
> > > > +    }
> > >
> > > This, on the other hand, doesn't redirect the request, but silently
> > > ignores it. That is, loading the snapshot will apparently succeed, but
> > > it wouldn't actually load anything and the disk would stay in its
> > > current state.
> >
> > In my use case bdrv_all_goto_snapshot iterates all block drivers, including
> > filters and disk images. Therefore skipping goto for images is ok.
> 
> Hm, this can happy today indeed.
> 
> Originally, we only called bdrv_goto_snapshot() for all _top level_
> BDSes, and this is still what you normally get. However, if you
> explicitly create a BDS (e.g. with its own -drive option), it is
> considered a top level BDS without actually being top level for the
> guest, and therefore the snapshotting function is called for it.
> 
> Of course, this is highly inefficient because the goto_snapshot request
> is passed by the filter driver and then called another time for the
> lower node, effectively loading the snapshot a second time.
> 
> On the other hand if you use a single -drive option to create both the
> qcow2 BDS and the blkreplay filter, we do need to pass down the
> goto_snapshot request because it won't be called for the qcow2 layer
> otherwise.

How this can be specified in command line?
I believed that separate -drive option is required.

> 
> I'm not completely sure yet what the right behaviour would be here.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf]

Am 27.09.2016 um 16:06 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > > > > This patch disables snapshotting for block driver filters.
> > > > > It is needed, because snapshots should be created
> > > > > in underlying disk images, not in filters itself.
> > > > >
> > > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > > >
> > > > But that's exactly what the existing code implements? If a driver
> > > > doesn't provide .bdrv_snapshot_goto, the request is redirected to
> > > > bs->file.
> > > >
> > > > >  block/snapshot.c |    3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/block/snapshot.c b/block/snapshot.c
> > > > > index bf5c2ca..8998b8b 100644
> > > > > --- a/block/snapshot.c
> > > > > +++ b/block/snapshot.c
> > > > > @@ -184,6 +184,9 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > > > >      if (!drv) {
> > > > >          return -ENOMEDIUM;
> > > > >      }
> > > > > +    if (drv->is_filter) {
> > > > > +        return 0;
> > > > > +    }
> > > >
> > > > This, on the other hand, doesn't redirect the request, but silently
> > > > ignores it. That is, loading the snapshot will apparently succeed, but
> > > > it wouldn't actually load anything and the disk would stay in its
> > > > current state.
> > >
> > > In my use case bdrv_all_goto_snapshot iterates all block drivers, including
> > > filters and disk images. Therefore skipping goto for images is ok.
> > 
> > Hm, this can happy today indeed.
> > 
> > Originally, we only called bdrv_goto_snapshot() for all _top level_
> > BDSes, and this is still what you normally get. However, if you
> > explicitly create a BDS (e.g. with its own -drive option), it is
> > considered a top level BDS without actually being top level for the
> > guest, and therefore the snapshotting function is called for it.
> > 
> > Of course, this is highly inefficient because the goto_snapshot request
> > is passed by the filter driver and then called another time for the
> > lower node, effectively loading the snapshot a second time.
> > 
> > On the other hand if you use a single -drive option to create both the
> > qcow2 BDS and the blkreplay filter, we do need to pass down the
> > goto_snapshot request because it won't be called for the qcow2 layer
> > otherwise.
> 
> How this can be specified in command line?
> I believed that separate -drive option is required.

Something like this:

    -drive driver=blkreplay,image.driver=file,image.filename=test.img

Kevin

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 27.09.2016 um 16:06 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > > Originally, we only called bdrv_goto_snapshot() for all _top level_
> > > BDSes, and this is still what you normally get. However, if you
> > > explicitly create a BDS (e.g. with its own -drive option), it is
> > > considered a top level BDS without actually being top level for the
> > > guest, and therefore the snapshotting function is called for it.
> > >
> > > Of course, this is highly inefficient because the goto_snapshot request
> > > is passed by the filter driver and then called another time for the
> > > lower node, effectively loading the snapshot a second time.

Maybe double-saving/loading does the smallest damage then?
And we should just document how to use blkreplay effectively?

> > >
> > > On the other hand if you use a single -drive option to create both the
> > > qcow2 BDS and the blkreplay filter, we do need to pass down the
> > > goto_snapshot request because it won't be called for the qcow2 layer
> > > otherwise.
> >
> > How this can be specified in command line?
> > I believed that separate -drive option is required.
> 
> Something like this:
> 
>     -drive driver=blkreplay,image.driver=file,image.filename=test.img
> 

I tried the following command line, but VM does not detect the hard drive
and cannot boot.

-drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-blkreplay 
-device ide-hd,drive=img-blkreplay


Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf]

Am 28.09.2016 um 11:32 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 27.09.2016 um 16:06 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > > > Originally, we only called bdrv_goto_snapshot() for all _top level_
> > > > BDSes, and this is still what you normally get. However, if you
> > > > explicitly create a BDS (e.g. with its own -drive option), it is
> > > > considered a top level BDS without actually being top level for the
> > > > guest, and therefore the snapshotting function is called for it.
> > > >
> > > > Of course, this is highly inefficient because the goto_snapshot request
> > > > is passed by the filter driver and then called another time for the
> > > > lower node, effectively loading the snapshot a second time.
> 
> Maybe double-saving/loading does the smallest damage then?
> And we should just document how to use blkreplay effectively?
> 
> > > >
> > > > On the other hand if you use a single -drive option to create both the
> > > > qcow2 BDS and the blkreplay filter, we do need to pass down the
> > > > goto_snapshot request because it won't be called for the qcow2 layer
> > > > otherwise.
> > >
> > > How this can be specified in command line?
> > > I believed that separate -drive option is required.
> > 
> > Something like this:
> > 
> >     -drive driver=blkreplay,image.driver=file,image.filename=test.img
> > 
> 
> I tried the following command line, but VM does not detect the hard drive
> and cannot boot.
> 
> -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-blkreplay 
> -device ide-hd,drive=img-blkreplay

My command line was assuming a raw image. It looks like you're using a
qcow (hopefully qcow2?) image. If so, then you need to include the qcow2
driver:

-drive driver=blkreplay,if=none,image.driver=qcow2,\
image.file.driver=file,image.file.filename=testdisk.qcow,id=img-blkreplay

Kevin

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 28.09.2016 um 11:32 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 27.09.2016 um 16:06 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben:
> > > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > > > > Originally, we only called bdrv_goto_snapshot() for all _top level_
> > > > > BDSes, and this is still what you normally get. However, if you
> > > > > explicitly create a BDS (e.g. with its own -drive option), it is
> > > > > considered a top level BDS without actually being top level for the
> > > > > guest, and therefore the snapshotting function is called for it.
> > > > >
> > > > > Of course, this is highly inefficient because the goto_snapshot request
> > > > > is passed by the filter driver and then called another time for the
> > > > > lower node, effectively loading the snapshot a second time.
> >
> > Maybe double-saving/loading does the smallest damage then?
> > And we should just document how to use blkreplay effectively?
> >
> > > > >
> > > > > On the other hand if you use a single -drive option to create both the
> > > > > qcow2 BDS and the blkreplay filter, we do need to pass down the
> > > > > goto_snapshot request because it won't be called for the qcow2 layer
> > > > > otherwise.
> > > >
> > > > How this can be specified in command line?
> > > > I believed that separate -drive option is required.
> > >
> > > Something like this:
> > >
> > >     -drive driver=blkreplay,image.driver=file,image.filename=test.img
> > >
> >
> > I tried the following command line, but VM does not detect the hard drive
> > and cannot boot.
> >
> > -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-
> blkreplay
> > -device ide-hd,drive=img-blkreplay
> 
> My command line was assuming a raw image. It looks like you're using a
> qcow (hopefully qcow2?) image. If so, then you need to include the qcow2
> driver:
> 
> -drive driver=blkreplay,if=none,image.driver=qcow2,\
> image.file.driver=file,image.file.filename=testdisk.qcow,id=img-blkreplay

This doesn't work for some reason. Replay just hangs at some moment.

Maybe there exists some internal difference between command line with one or two -drive options?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf]

Am 28.09.2016 um 14:49 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 28.09.2016 um 11:32 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 27.09.2016 um 16:06 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben:
> > > > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > > > > > Originally, we only called bdrv_goto_snapshot() for all _top level_
> > > > > > BDSes, and this is still what you normally get. However, if you
> > > > > > explicitly create a BDS (e.g. with its own -drive option), it is
> > > > > > considered a top level BDS without actually being top level for the
> > > > > > guest, and therefore the snapshotting function is called for it.
> > > > > >
> > > > > > Of course, this is highly inefficient because the goto_snapshot request
> > > > > > is passed by the filter driver and then called another time for the
> > > > > > lower node, effectively loading the snapshot a second time.
> > >
> > > Maybe double-saving/loading does the smallest damage then?
> > > And we should just document how to use blkreplay effectively?
> > >
> > > > > >
> > > > > > On the other hand if you use a single -drive option to create both the
> > > > > > qcow2 BDS and the blkreplay filter, we do need to pass down the
> > > > > > goto_snapshot request because it won't be called for the qcow2 layer
> > > > > > otherwise.
> > > > >
> > > > > How this can be specified in command line?
> > > > > I believed that separate -drive option is required.
> > > >
> > > > Something like this:
> > > >
> > > >     -drive driver=blkreplay,image.driver=file,image.filename=test.img
> > > >
> > >
> > > I tried the following command line, but VM does not detect the hard drive
> > > and cannot boot.
> > >
> > > -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-
> > blkreplay
> > > -device ide-hd,drive=img-blkreplay
> > 
> > My command line was assuming a raw image. It looks like you're using a
> > qcow (hopefully qcow2?) image. If so, then you need to include the qcow2
> > driver:
> > 
> > -drive driver=blkreplay,if=none,image.driver=qcow2,\
> > image.file.driver=file,image.file.filename=testdisk.qcow,id=img-blkreplay
> 
> This doesn't work for some reason. Replay just hangs at some moment.
> 
> Maybe there exists some internal difference between command line with one or two -drive options?

There are some subtle differences that affect the management of the
block driver nodes, but they shouldn't make a difference for the I/O
path. But I don't know the replay code well, so maybe it makes a
difference there (which would be a bug).

If you have two separate -drive options, a BlockBackend is created for
each of them. The lower layer one wouldn't be used normally, but if you
iterate over all BlockBackends somewhere, you would see it. Both BBs are
owned by the monitor, and if you remove the top layer, the monitor
reference will keep the lower layer alive.

In contrast, with a single -drive option, only the top layer (blkreplay)
has a BlockBackend attached, but the qcow2 layer doesn't. If the
blkreplay BlockBackend is deleted, the qcow2 layer automatically goes
away as well because the monitor doesn't hold a reference to it.

I think these are the most important (and possibly the only)
differences.

Kevin

Re: [Qemu-devel] [PATCH v5 4/9] record/replay: add network support

[Jason Wang]

On 2016年09月26日 16:08, Pavel Dovgalyuk wrote:
> This patch adds support of recording and replaying network packets in
> irount rr mode.
>
> Record and replay for network interactions is performed with the network filter.
> Each backend must have its own instance of the replay filter as follows:
>   -netdev user,id=net1 -device rtl8139,netdev=net1
>   -object filter-replay,id=replay,netdev=net1
>
> Replay network filter is used to record and replay network packets. While
> recording the virtual machine this filter puts all packets coming from
> the outer world into the log. In replay mode packets from the log are
> injected into the network device. All interactions with network backend
> in replay mode are disabled.
>
> v5 changes:
>   - using iov_to_buf function instead of loop
>
> Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@ispras.ru>
> ---

Reviewed-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH v5 4/9] record/replay: add network support

[Paolo Bonzini]

On 27/10/2016 05:36, Jason Wang wrote:
> 
> 
> On 2016年09月26日 16:08, Pavel Dovgalyuk wrote:
>> This patch adds support of recording and replaying network packets in
>> irount rr mode.
>>
>> Record and replay for network interactions is performed with the
>> network filter.
>> Each backend must have its own instance of the replay filter as follows:
>>   -netdev user,id=net1 -device rtl8139,netdev=net1
>>   -object filter-replay,id=replay,netdev=net1
>>
>> Replay network filter is used to record and replay network packets. While
>> recording the virtual machine this filter puts all packets coming from
>> the outer world into the log. In replay mode packets from the log are
>> injected into the network device. All interactions with network backend
>> in replay mode are disabled.
>>
>> v5 changes:
>>   - using iov_to_buf function instead of loop
>>
>> Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@ispras.ru>
>> ---
> 
> Reviewed-by: Jason Wang <jasowang@redhat.com>

Jason,

please merge this yourself through the net tree.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v5 4/9] record/replay: add network support

[Jason Wang]

On 2016年10月27日 15:58, Paolo Bonzini wrote:
>
> On 27/10/2016 05:36, Jason Wang wrote:
>>
>> On 2016年09月26日 16:08, Pavel Dovgalyuk wrote:
>>> This patch adds support of recording and replaying network packets in
>>> irount rr mode.
>>>
>>> Record and replay for network interactions is performed with the
>>> network filter.
>>> Each backend must have its own instance of the replay filter as follows:
>>>    -netdev user,id=net1 -device rtl8139,netdev=net1
>>>    -object filter-replay,id=replay,netdev=net1
>>>
>>> Replay network filter is used to record and replay network packets. While
>>> recording the virtual machine this filter puts all packets coming from
>>> the outer world into the log. In replay mode packets from the log are
>>> injected into the network device. All interactions with network backend
>>> in replay mode are disabled.
>>>
>>> v5 changes:
>>>    - using iov_to_buf function instead of loop
>>>
>>> Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@ispras.ru>
>>> ---
>> Reviewed-by: Jason Wang <jasowang@redhat.com>
> Jason,
>
> please merge this yourself through the net tree.
>
> Thanks,
>
> Paolo
>

Picked in net.

Thanks

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

Kevin,

> From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 28.09.2016 um 11:32 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 27.09.2016 um 16:06 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > Am 26.09.2016 um 11:51 hat Pavel Dovgalyuk geschrieben:
> > > > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > > > Am 26.09.2016 um 10:08 hat Pavel Dovgalyuk geschrieben:
> > > > > > Originally, we only called bdrv_goto_snapshot() for all _top level_
> > > > > > BDSes, and this is still what you normally get. However, if you
> > > > > > explicitly create a BDS (e.g. with its own -drive option), it is
> > > > > > considered a top level BDS without actually being top level for the
> > > > > > guest, and therefore the snapshotting function is called for it.
> > > > > >
> > > > > > Of course, this is highly inefficient because the goto_snapshot request
> > > > > > is passed by the filter driver and then called another time for the
> > > > > > lower node, effectively loading the snapshot a second time.
> > >
> > > Maybe double-saving/loading does the smallest damage then?
> > > And we should just document how to use blkreplay effectively?
> > >
> > > > > >
> > > > > > On the other hand if you use a single -drive option to create both the
> > > > > > qcow2 BDS and the blkreplay filter, we do need to pass down the
> > > > > > goto_snapshot request because it won't be called for the qcow2 layer
> > > > > > otherwise.
> > > > >
> > > > > How this can be specified in command line?
> > > > > I believed that separate -drive option is required.
> > > >
> > > > Something like this:
> > > >
> > > >     -drive driver=blkreplay,image.driver=file,image.filename=test.img
> > > >
> > >
> > > I tried the following command line, but VM does not detect the hard drive
> > > and cannot boot.
> > >
> > > -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-
> > blkreplay
> > > -device ide-hd,drive=img-blkreplay
> >
> > My command line was assuming a raw image. It looks like you're using a
> > qcow (hopefully qcow2?) image. If so, then you need to include the qcow2
> > driver:
> >
> > -drive driver=blkreplay,if=none,image.driver=qcow2,\
> > image.file.driver=file,image.file.filename=testdisk.qcow,id=img-blkreplay
> 
> This doesn't work for some reason. Replay just hangs at some moment.
> 
> Maybe there exists some internal difference between command line with one or two -drive
> options?

I've investigated this issue.
This command line works ok:
 -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-blkreplay 
 -device ide-hd,drive=img-blkreplay

And this does not:
 -drive
driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdisk.qcow
,id=img-blkreplay
 -device ide-hd,drive=img-blkreplay

QEMU hangs at some moment of replay.

I found that some dma requests do not pass through the blkreplay driver
due to the following line in block-backend.c:
    return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);

This line passes read request directly to qcow driver and blkreplay cannot
process it to make deterministic.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Paolo Bonzini]

> I've investigated this issue.
> This command line works ok:
>  -drive
>  driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-blkreplay
>  -device ide-hd,drive=img-blkreplay
> 
> And this does not:
>  -drive
> driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdisk.qcow
> ,id=img-blkreplay
>  -device ide-hd,drive=img-blkreplay
> 
> QEMU hangs at some moment of replay.
> 
> I found that some dma requests do not pass through the blkreplay driver
> due to the following line in block-backend.c:
>     return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
> 
> This line passes read request directly to qcow driver and blkreplay cannot
> process it to make deterministic.

I don't understand, blk->root should be the blkreplay here.

Paolo

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf]

Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben:
> > From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > My command line was assuming a raw image. It looks like you're using a
> > > qcow (hopefully qcow2?) image. If so, then you need to include the qcow2
> > > driver:
> > >
> > > -drive driver=blkreplay,if=none,image.driver=qcow2,\
> > > image.file.driver=file,image.file.filename=testdisk.qcow,id=img-blkreplay
> > 
> > This doesn't work for some reason. Replay just hangs at some moment.
> > 
> > Maybe there exists some internal difference between command line with one or two -drive
> > options?
> 
> I've investigated this issue.
> This command line works ok:
>  -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-blkreplay 
>  -device ide-hd,drive=img-blkreplay
> 
> And this does not:
>  -drive
> driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdisk.qcow
> ,id=img-blkreplay
>  -device ide-hd,drive=img-blkreplay
> 
> QEMU hangs at some moment of replay.
> 
> I found that some dma requests do not pass through the blkreplay driver
> due to the following line in block-backend.c:
>     return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
> 
> This line passes read request directly to qcow driver and blkreplay cannot
> process it to make deterministic.

How does that bypass blkreplay? blk->root is supposed to be the blkreply
node, do you see something different? If it were the qcow2 node, then I
would expect that no requests at all go through the blkreplay layer.

Kevin

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> > I've investigated this issue.
> > This command line works ok:
> >  -drive
> >  driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-blkreplay
> >  -device ide-hd,drive=img-blkreplay
> >
> > And this does not:
> >  -drive
> >
> driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdis
> k.qcow
> > ,id=img-blkreplay
> >  -device ide-hd,drive=img-blkreplay
> >
> > QEMU hangs at some moment of replay.
> >
> > I found that some dma requests do not pass through the blkreplay driver
> > due to the following line in block-backend.c:
> >     return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
> >
> > This line passes read request directly to qcow driver and blkreplay cannot
> > process it to make deterministic.
> 
> I don't understand, blk->root should be the blkreplay here.

I've got some more logs. I used the disk image which references the backing file.
It seems that some weird things happen with both command lines.

== For the first command line (blkreplay separated from image):
blk_co_preadv(img-blkreplay)
 -> bdrv_co_preadv(qcow2, temp_overlay1)
 -> bdrv_co_preadv(blkreplay, temp_overlay)
 -> bdrv_co_preadv(qcow2, temp_overlay2)
 -> bdrv_co_preadv(qcow2, image_overlay)
 -> bdrv_co_preadv(qcow2, image_backing)
 -> bdrv_co_preadv(file, image_backing)

But sometimes it changes to:
blk_co_preadv(img-blkreplay)
 -> bdrv_co_preadv(qcow2, temp_overlay1)
 -> bdrv_co_preadv(file, temp_overlay1)

== For the second command line (blkreplay combined with image):

In most cases we have the following call stack:
blk_co_preadv(img-blkreplay)
 -> bdrv_co_preadv(qcow2, temp_overlay)
 -> bdrv_co_preadv(blkreplay, image_overlay)
 -> bdrv_co_preadv(qcow2, image_overlay)
 -> bdrv_co_preadv(qcow2, image_backing)
 -> bdrv_co_preadv(file, image_backing)

But sometimes it changes to:
blk_co_preadv(img-blkreplay)
 -> bdrv_co_preadv(qcow2, temp overlay)
 -> bdrv_co_preadv(file, temp overlay)

========

It seems, that temporary overlay is created over blkreplay, which
it intended to work as a simple filter. Is that correct?
	
Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben:
> > > From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> >
> > I've investigated this issue.
> > This command line works ok:
> >  -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-
> blkreplay
> >  -device ide-hd,drive=img-blkreplay
> >
> > And this does not:
> >  -drive
> >
> driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdis
> k.qcow
> > ,id=img-blkreplay
> >  -device ide-hd,drive=img-blkreplay
> >
> > QEMU hangs at some moment of replay.
> >
> > I found that some dma requests do not pass through the blkreplay driver
> > due to the following line in block-backend.c:
> >     return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
> >
> > This line passes read request directly to qcow driver and blkreplay cannot
> > process it to make deterministic.
> 
> How does that bypass blkreplay? blk->root is supposed to be the blkreply
> node, do you see something different? If it were the qcow2 node, then I
> would expect that no requests at all go through the blkreplay layer.

It seems, that the problem is in -snapshot option.
We have one of the following block layers depending on command line:
 tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image
 tmp_overlay1 -> blkreplay -> disk_image

But the correct scheme is intended to be the following:
 blkreplay -> tmp_overlay1 -> disk_image

How can we fix it?
Maybe we should add blkreplay automatically to all block devices and not
to specify it in the command line?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf]

Am 21.11.2016 um 13:22 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben:
> > > > From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> > >
> > > I've investigated this issue.
> > > This command line works ok:
> > >  -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-
> > blkreplay
> > >  -device ide-hd,drive=img-blkreplay
> > >
> > > And this does not:
> > >  -drive
> > >
> > driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdis
> > k.qcow
> > > ,id=img-blkreplay
> > >  -device ide-hd,drive=img-blkreplay
> > >
> > > QEMU hangs at some moment of replay.
> > >
> > > I found that some dma requests do not pass through the blkreplay driver
> > > due to the following line in block-backend.c:
> > >     return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
> > >
> > > This line passes read request directly to qcow driver and blkreplay cannot
> > > process it to make deterministic.
> > 
> > How does that bypass blkreplay? blk->root is supposed to be the blkreply
> > node, do you see something different? If it were the qcow2 node, then I
> > would expect that no requests at all go through the blkreplay layer.
> 
> It seems, that the problem is in -snapshot option.
> We have one of the following block layers depending on command line:
>  tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image
>  tmp_overlay1 -> blkreplay -> disk_image
> 
> But the correct scheme is intended to be the following:
>  blkreplay -> tmp_overlay1 -> disk_image
> 
> How can we fix it?
> Maybe we should add blkreplay automatically to all block devices and not
> to specify it in the command line?

I think you found a pretty fundamental design problem with "global"
drive options that add a filter node such as -snapshot and replay mode
(replay mode isn't one of them today, but your suggestion to make it
automatic would turn it into one).

At the core of the problem I think we have two questions:

1. Which block nodes should be affected and get a filter node added, and
   which nodes shouldn't get one? In your case, disl_image is defined
   with a -drive option, but shouldn't get the snapshot.

2. In which order should filter nodes be added?

Both of these questions feel hard. As long as we haven't thought through
the concept as such (rather than discussing one-off hacks) and we're not
completely certain what the right answer to the questions is, we
shouldn't add more automatic filter nodes, because chances are that we
get it wrong and would regret it.

The obvious answer for a workaround would be: Make everything manual,
i.e. don't use -snapshot, but create a qcow2 overlay manually.

For getting the automatic thing right, please give me some time to think
about the design. I'll also meet Markus (CCed) in person end of next
week and I think this is a design question that would be useful to
discuss with him then.

Kevin

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

Paolo,

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 21.11.2016 um 13:22 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> > > >
> > > > I've investigated this issue.
> > > > This command line works ok:
> > > >  -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-
> > > blkreplay
> > > >  -device ide-hd,drive=img-blkreplay
> > > >
> > > > And this does not:
> > > >  -drive
> > > >
> > >
> driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdis
> > > k.qcow
> > > > ,id=img-blkreplay
> > > >  -device ide-hd,drive=img-blkreplay
> > > >
> > > > QEMU hangs at some moment of replay.
> > > >
> > > > I found that some dma requests do not pass through the blkreplay driver
> > > > due to the following line in block-backend.c:
> > > >     return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
> > > >
> > > > This line passes read request directly to qcow driver and blkreplay cannot
> > > > process it to make deterministic.
> > >
> > > How does that bypass blkreplay? blk->root is supposed to be the blkreply
> > > node, do you see something different? If it were the qcow2 node, then I
> > > would expect that no requests at all go through the blkreplay layer.
> >
> > It seems, that the problem is in -snapshot option.
> > We have one of the following block layers depending on command line:
> >  tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image
> >  tmp_overlay1 -> blkreplay -> disk_image
> >
> > But the correct scheme is intended to be the following:
> >  blkreplay -> tmp_overlay1 -> disk_image
> >
> > How can we fix it?
> > Maybe we should add blkreplay automatically to all block devices and not
> > to specify it in the command line?
> 
> I think you found a pretty fundamental design problem with "global"
> drive options that add a filter node such as -snapshot and replay mode
> (replay mode isn't one of them today, but your suggestion to make it
> automatic would turn it into one).
> 
> At the core of the problem I think we have two questions:
> 
> 1. Which block nodes should be affected and get a filter node added, and
>    which nodes shouldn't get one? In your case, disl_image is defined
>    with a -drive option, but shouldn't get the snapshot.
> 
> 2. In which order should filter nodes be added?
> 
> Both of these questions feel hard. As long as we haven't thought through
> the concept as such (rather than discussing one-off hacks) and we're not
> completely certain what the right answer to the questions is, we
> shouldn't add more automatic filter nodes, because chances are that we
> get it wrong and would regret it.
> 
> The obvious answer for a workaround would be: Make everything manual,
> i.e. don't use -snapshot, but create a qcow2 overlay manually.

What about to switching to manual overlay creation by default?
We can make rrsnapshot option mandatory.
Therefore user will have to create snapshot in image or overlay and
the disk image will not be corrupted.

It is not very convenient, but we could disable rrsnapshot again when
the solution for -snapshot will be found.

> For getting the automatic thing right, please give me some time to think
> about the design. I'll also meet Markus (CCed) in person end of next
> week and I think this is a design question that would be useful to
> discuss with him then.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf]

Am 05.12.2016 um 08:43 hat Pavel Dovgalyuk geschrieben:
> Paolo,
> 
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 21.11.2016 um 13:22 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> > > > >
> > > > > I've investigated this issue.
> > > > > This command line works ok:
> > > > >  -drive driver=blkreplay,if=none,image.driver=file,image.filename=testdisk.qcow,id=img-
> > > > blkreplay
> > > > >  -device ide-hd,drive=img-blkreplay
> > > > >
> > > > > And this does not:
> > > > >  -drive
> > > > >
> > > >
> > driver=blkreplay,if=none,image.driver=qcow2,image.file.driver=file,image.file.filename=testdis
> > > > k.qcow
> > > > > ,id=img-blkreplay
> > > > >  -device ide-hd,drive=img-blkreplay
> > > > >
> > > > > QEMU hangs at some moment of replay.
> > > > >
> > > > > I found that some dma requests do not pass through the blkreplay driver
> > > > > due to the following line in block-backend.c:
> > > > >     return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
> > > > >
> > > > > This line passes read request directly to qcow driver and blkreplay cannot
> > > > > process it to make deterministic.
> > > >
> > > > How does that bypass blkreplay? blk->root is supposed to be the blkreply
> > > > node, do you see something different? If it were the qcow2 node, then I
> > > > would expect that no requests at all go through the blkreplay layer.
> > >
> > > It seems, that the problem is in -snapshot option.
> > > We have one of the following block layers depending on command line:
> > >  tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image
> > >  tmp_overlay1 -> blkreplay -> disk_image
> > >
> > > But the correct scheme is intended to be the following:
> > >  blkreplay -> tmp_overlay1 -> disk_image
> > >
> > > How can we fix it?
> > > Maybe we should add blkreplay automatically to all block devices and not
> > > to specify it in the command line?
> > 
> > I think you found a pretty fundamental design problem with "global"
> > drive options that add a filter node such as -snapshot and replay mode
> > (replay mode isn't one of them today, but your suggestion to make it
> > automatic would turn it into one).
> > 
> > At the core of the problem I think we have two questions:
> > 
> > 1. Which block nodes should be affected and get a filter node added, and
> >    which nodes shouldn't get one? In your case, disl_image is defined
> >    with a -drive option, but shouldn't get the snapshot.
> > 
> > 2. In which order should filter nodes be added?
> > 
> > Both of these questions feel hard. As long as we haven't thought through
> > the concept as such (rather than discussing one-off hacks) and we're not
> > completely certain what the right answer to the questions is, we
> > shouldn't add more automatic filter nodes, because chances are that we
> > get it wrong and would regret it.
> > 
> > The obvious answer for a workaround would be: Make everything manual,
> > i.e. don't use -snapshot, but create a qcow2 overlay manually.
> 
> What about to switching to manual overlay creation by default?
> We can make rrsnapshot option mandatory.
> Therefore user will have to create snapshot in image or overlay and
> the disk image will not be corrupted.
> 
> It is not very convenient, but we could disable rrsnapshot again when
> the solution for -snapshot will be found.

Hm, what is this rrsnapshot option? git grep can't find it.

Anyway, it seems that doing things manually is the safe way as long as
we don't know the final solution, so I think I agree.

For a slightly more convenient way, one of the problems to solve seems
to be that snapshot=on always affects the top level node and you can't
create a temporary snapshot in the middle of the chain. Perhaps we
should introduce a 'temporary-overlay' driver or something like that, so
that you could specify things like this:

    -drive if=none,driver=file,filename=test.img,id=orig
    -drive if=none,driver=temporary-overlay,file=orig,id=snap
    -drive if=none,driver=blkreplay,image=snap

Which makes me wonder... Is blkreplay usable without the temporary
snapshot or is this pretty much a requirement? Because if it has to be
there, the next step could be that blkreplay creates temporary-overlay
internally in its .bdrv_open().

Kevin

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 05.12.2016 um 08:43 hat Pavel Dovgalyuk geschrieben:
> > Paolo,
> >
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 21.11.2016 um 13:22 hat Pavel Dovgalyuk geschrieben:
> > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben:
> > > > >
> > > > > How does that bypass blkreplay? blk->root is supposed to be the blkreply
> > > > > node, do you see something different? If it were the qcow2 node, then I
> > > > > would expect that no requests at all go through the blkreplay layer.
> > > >
> > > > It seems, that the problem is in -snapshot option.
> > > > We have one of the following block layers depending on command line:
> > > >  tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image
> > > >  tmp_overlay1 -> blkreplay -> disk_image
> > > >
> > > > But the correct scheme is intended to be the following:
> > > >  blkreplay -> tmp_overlay1 -> disk_image
> > > >
> > > > How can we fix it?
> > > > Maybe we should add blkreplay automatically to all block devices and not
> > > > to specify it in the command line?
> > >
> > > I think you found a pretty fundamental design problem with "global"
> > > drive options that add a filter node such as -snapshot and replay mode
> > > (replay mode isn't one of them today, but your suggestion to make it
> > > automatic would turn it into one).
> > >
> > > At the core of the problem I think we have two questions:
> > >
> > > 1. Which block nodes should be affected and get a filter node added, and
> > >    which nodes shouldn't get one? In your case, disl_image is defined
> > >    with a -drive option, but shouldn't get the snapshot.
> > >
> > > 2. In which order should filter nodes be added?
> > >
> > > Both of these questions feel hard. As long as we haven't thought through
> > > the concept as such (rather than discussing one-off hacks) and we're not
> > > completely certain what the right answer to the questions is, we
> > > shouldn't add more automatic filter nodes, because chances are that we
> > > get it wrong and would regret it.
> > >
> > > The obvious answer for a workaround would be: Make everything manual,
> > > i.e. don't use -snapshot, but create a qcow2 overlay manually.
> >
> > What about to switching to manual overlay creation by default?
> > We can make rrsnapshot option mandatory.
> > Therefore user will have to create snapshot in image or overlay and
> > the disk image will not be corrupted.
> >
> > It is not very convenient, but we could disable rrsnapshot again when
> > the solution for -snapshot will be found.
> 
> Hm, what is this rrsnapshot option? git grep can't find it.

It was a patch that was not included yet.
This option creates/loads vm snapshot in record/replay mode
leaving original disk image unchanged.
Record/replay without this option uses '-snapshot' to preserve
the state of the disk images.

> Anyway, it seems that doing things manually is the safe way as long as
> we don't know the final solution, so I think I agree.
> 
> For a slightly more convenient way, one of the problems to solve seems
> to be that snapshot=on always affects the top level node and you can't
> create a temporary snapshot in the middle of the chain. Perhaps we
> should introduce a 'temporary-overlay' driver or something like that, so
> that you could specify things like this:
> 
>     -drive if=none,driver=file,filename=test.img,id=orig
>     -drive if=none,driver=temporary-overlay,file=orig,id=snap
>     -drive if=none,driver=blkreplay,image=snap

This seems reasonable for manual way.

> Which makes me wonder... Is blkreplay usable without the temporary
> snapshot or is this pretty much a requirement? 

It's not a requirement. But to make replay deterministic we have to
start with the same image every time. As I know, this may be achieved by:
1. Restoring original disk image manually
2. Using vm snapshot to start execution from
3. Using -snapshot option
4. Not using disks at all

> Because if it has to be
> there, the next step could be that blkreplay creates temporary-overlay
> internally in its .bdrv_open().

Here is your answer about such an approach :)
https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04687.html

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf]

Am 05.12.2016 um 12:49 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 05.12.2016 um 08:43 hat Pavel Dovgalyuk geschrieben:
> > > Paolo,
> > >
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 21.11.2016 um 13:22 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben:
> > > > > >
> > > > > > How does that bypass blkreplay? blk->root is supposed to be the blkreply
> > > > > > node, do you see something different? If it were the qcow2 node, then I
> > > > > > would expect that no requests at all go through the blkreplay layer.
> > > > >
> > > > > It seems, that the problem is in -snapshot option.
> > > > > We have one of the following block layers depending on command line:
> > > > >  tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image
> > > > >  tmp_overlay1 -> blkreplay -> disk_image
> > > > >
> > > > > But the correct scheme is intended to be the following:
> > > > >  blkreplay -> tmp_overlay1 -> disk_image
> > > > >
> > > > > How can we fix it?
> > > > > Maybe we should add blkreplay automatically to all block devices and not
> > > > > to specify it in the command line?
> > > >
> > > > I think you found a pretty fundamental design problem with "global"
> > > > drive options that add a filter node such as -snapshot and replay mode
> > > > (replay mode isn't one of them today, but your suggestion to make it
> > > > automatic would turn it into one).
> > > >
> > > > At the core of the problem I think we have two questions:
> > > >
> > > > 1. Which block nodes should be affected and get a filter node added, and
> > > >    which nodes shouldn't get one? In your case, disl_image is defined
> > > >    with a -drive option, but shouldn't get the snapshot.
> > > >
> > > > 2. In which order should filter nodes be added?
> > > >
> > > > Both of these questions feel hard. As long as we haven't thought through
> > > > the concept as such (rather than discussing one-off hacks) and we're not
> > > > completely certain what the right answer to the questions is, we
> > > > shouldn't add more automatic filter nodes, because chances are that we
> > > > get it wrong and would regret it.
> > > >
> > > > The obvious answer for a workaround would be: Make everything manual,
> > > > i.e. don't use -snapshot, but create a qcow2 overlay manually.
> > >
> > > What about to switching to manual overlay creation by default?
> > > We can make rrsnapshot option mandatory.
> > > Therefore user will have to create snapshot in image or overlay and
> > > the disk image will not be corrupted.
> > >
> > > It is not very convenient, but we could disable rrsnapshot again when
> > > the solution for -snapshot will be found.
> > 
> > Hm, what is this rrsnapshot option? git grep can't find it.
> 
> It was a patch that was not included yet.
> This option creates/loads vm snapshot in record/replay mode
> leaving original disk image unchanged.

You mean internal qcow2 snapshots? Ok.

> Record/replay without this option uses '-snapshot' to preserve
> the state of the disk images.
> 
> > Anyway, it seems that doing things manually is the safe way as long as
> > we don't know the final solution, so I think I agree.
> > 
> > For a slightly more convenient way, one of the problems to solve seems
> > to be that snapshot=on always affects the top level node and you can't
> > create a temporary snapshot in the middle of the chain. Perhaps we
> > should introduce a 'temporary-overlay' driver or something like that, so
> > that you could specify things like this:
> > 
> >     -drive if=none,driver=file,filename=test.img,id=orig
> >     -drive if=none,driver=temporary-overlay,file=orig,id=snap
> >     -drive if=none,driver=blkreplay,image=snap
> 
> This seems reasonable for manual way.

Maybe another, easier to implement option could be something like this:

    -drive if=none,driver=file,filename=test.img,snapshot=on,overlay.node-name=snap
    -drive if=none,driver=blkreplay,image=snap

It would require that we implement support for overlay.* options like we
already support backing.* options. Allowing to specify options for the
overlay node is probably nice to have anyway.

However, there could be a few tricky parts there. For example, what
happens if someone uses overlay.backing=something-else? Perhaps
completely disallowing backing and backing.* for overlays would already
solve this.

> > Which makes me wonder... Is blkreplay usable without the temporary
> > snapshot or is this pretty much a requirement? 
> 
> It's not a requirement. But to make replay deterministic we have to
> start with the same image every time. As I know, this may be achieved by:
> 1. Restoring original disk image manually
> 2. Using vm snapshot to start execution from
> 3. Using -snapshot option
> 4. Not using disks at all
> 
> > Because if it has to be
> > there, the next step could be that blkreplay creates temporary-overlay
> > internally in its .bdrv_open().
> 
> Here is your answer about such an approach :)
> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04687.html

Right, and unfortunately these are still good points.

Especially the part where you allowed to give the overlay filename
really needs to work the way it does now with the 'image' option. We
might not need to be that strict with temporary overlays, restricting to
qcow2 with default options could be acceptable there - but whatever I
think of to support both cases results in something that isn't really
easier than the manual way that we figured out above.

Kevin

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 05.12.2016 um 12:49 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Am 05.12.2016 um 08:43 hat Pavel Dovgalyuk geschrieben:
> 
> > Record/replay without this option uses '-snapshot' to preserve
> > the state of the disk images.
> >
> > > Anyway, it seems that doing things manually is the safe way as long as
> > > we don't know the final solution, so I think I agree.
> > >
> > > For a slightly more convenient way, one of the problems to solve seems
> > > to be that snapshot=on always affects the top level node and you can't
> > > create a temporary snapshot in the middle of the chain. Perhaps we
> > > should introduce a 'temporary-overlay' driver or something like that, so
> > > that you could specify things like this:
> > >
> > >     -drive if=none,driver=file,filename=test.img,id=orig
> > >     -drive if=none,driver=temporary-overlay,file=orig,id=snap
> > >     -drive if=none,driver=blkreplay,image=snap
> >
> > This seems reasonable for manual way.
> 
> Maybe another, easier to implement option could be something like this:
> 
>     -drive if=none,driver=file,filename=test.img,snapshot=on,overlay.node-name=snap
>     -drive if=none,driver=blkreplay,image=snap
> 
> It would require that we implement support for overlay.* options like we
> already support backing.* options. Allowing to specify options for the
> overlay node is probably nice to have anyway.
> 
> However, there could be a few tricky parts there. For example, what
> happens if someone uses overlay.backing=something-else? Perhaps
> completely disallowing backing and backing.* for overlays would already
> solve this.
> 
> > > Which makes me wonder... Is blkreplay usable without the temporary
> > > snapshot or is this pretty much a requirement?
> >
> > It's not a requirement. But to make replay deterministic we have to
> > start with the same image every time. As I know, this may be achieved by:
> > 1. Restoring original disk image manually
> > 2. Using vm snapshot to start execution from
> > 3. Using -snapshot option
> > 4. Not using disks at all
> >
> > > Because if it has to be
> > > there, the next step could be that blkreplay creates temporary-overlay
> > > internally in its .bdrv_open().
> >
> > Here is your answer about such an approach :)
> > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04687.html
> 
> Right, and unfortunately these are still good points.
> 
> Especially the part where you allowed to give the overlay filename
> really needs to work the way it does now with the 'image' option. We
> might not need to be that strict with temporary overlays, restricting to
> qcow2 with default options could be acceptable there - but whatever I
> think of to support both cases results in something that isn't really
> easier than the manual way that we figured out above.

Can we stop on the following?
1. Don't create any overlays automatically when user wants to save/restore VM state
2. In the opposite case create snapshots, but do not use -snapshot option.
   Snapshots will be created by the blkreplay as in the link specified.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Pavel Dovgalyuk]

Kevin,

> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Am 05.12.2016 um 12:49 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> >
> > It's not a requirement. But to make replay deterministic we have to
> > start with the same image every time. As I know, this may be achieved by:
> > 1. Restoring original disk image manually
> > 2. Using vm snapshot to start execution from
> > 3. Using -snapshot option
> > 4. Not using disks at all
> >
> > > Because if it has to be
> > > there, the next step could be that blkreplay creates temporary-overlay
> > > internally in its .bdrv_open().
> >
> > Here is your answer about such an approach :)
> > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04687.html
> 
> Right, and unfortunately these are still good points.
> 
> Especially the part where you allowed to give the overlay filename
> really needs to work the way it does now with the 'image' option. We
> might not need to be that strict with temporary overlays, restricting to
> qcow2 with default options could be acceptable there - but whatever I
> think of to support both cases results in something that isn't really
> easier than the manual way that we figured out above.

What about the new version?

Pavel Dovgalyuk