[Qemu-devel] [PATCH v7 00/14] replay additions

[Qemu-devel] [PATCH v7 00/14] replay additions

[Pavel Dovgalyuk]

This set of patches includes several fixes for replay and vmstate.

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.

With these patches operations with audio devices can also be recorded
and replayed. All interactions with passthrough audio (including
microphone input) are recorded automatically when -soundhw is specified
at the command line.

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

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

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

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

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

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

---

Pavel Dovgalyuk (14):
      icount: update instruction counter on apic patching
      replay: improve interrupt handling
      replay: exception replay fix
      icount: exit cpu loop on expire
      apic: save apic_delivered flag
      replay: don't use rtc clock on loadvm phase
      integratorcp: adding vmstate for save/restore
      savevm: add public save_vmstate function
      replay: save/load initial state
      block: implement bdrv_snapshot_goto for blkreplay
      blkreplay: create temporary overlay for underlaying devices
      replay: disable default snapshot for record/replay
      audio: make audio poll timer deterministic
      replay: add record/replay for audio passthrough


 audio/audio.c                   |   15 +++++--
 audio/audio.h                   |    5 ++
 audio/mixeng.c                  |   31 ++++++++++++++
 block/blkreplay.c               |   84 +++++++++++++++++++++++++++++++++++++++
 cpu-exec.c                      |   21 ++++++++--
 docs/replay.txt                 |   23 +++++++++++
 hw/arm/integratorcp.c           |   62 +++++++++++++++++++++++++++++
 hw/i386/kvmvapic.c              |    6 +++
 hw/intc/apic_common.c           |   32 +++++++++++++++
 hw/timer/mc146818rtc.c          |   14 +++++--
 include/hw/i386/apic_internal.h |    2 +
 include/sysemu/replay.h         |   16 +++++++
 include/sysemu/sysemu.h         |    1 
 migration/savevm.c              |   33 ++++++++++-----
 qemu-options.hx                 |    8 +++-
 replay/Makefile.objs            |    1 
 replay/replay-audio.c           |   79 +++++++++++++++++++++++++++++++++++++
 replay/replay-internal.h        |    4 ++
 replay/replay-snapshot.c        |   17 ++++++++
 replay/replay.c                 |    5 ++
 stubs/replay.c                  |    1 
 target/i386/seg_helper.c        |    1 
 vl.c                            |   17 +++++++-
 23 files changed, 450 insertions(+), 28 deletions(-)
 create mode 100644 replay/replay-audio.c

-- 
Pavel Dovgalyuk

[Qemu-devel] [PATCH v7 01/14] icount: update instruction counter on apic patching

[Pavel Dovgalyuk]

kvmvapic patches the code when some instructions are executed.
E.g. mov 0xff, 0xfffe0080 is interpreted as push 0xff/call ...
This patching is also followed by some side effects (changing apic
and guest memory state). Therefore deterministic execution should take
this operation into account. This patch decreases icount when original
mov instruction is trying to execute. Therefore patching becomes
deterministic and can be replayed correctly.

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

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index b30d1b9..146d47c 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -412,6 +412,12 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
     if (!kvm_enabled()) {
         cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
                              &current_flags);
+        /* Account this instruction, because we will exit the tb.
+           This is the first instruction in the block. Therefore
+           there is no need in restoring CPU state. */
+        if (use_icount) {
+            --cs->icount_decr.u16.low;
+        }
     }
 
     pause_all_vcpus();

[Qemu-devel] [PATCH v7 02/14] replay: improve interrupt handling

[Pavel Dovgalyuk]

This patch improves interrupt handling in record/replay mode.
Now "interrupt" event is saved only when cc->cpu_exec_interrupt returns true.
This patch also adds missing return to cpu_exec_interrupt function.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 cpu-exec.c               |    2 +-
 target/i386/seg_helper.c |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 4188fed..fa08c73 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -508,8 +508,8 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
            True when it is, and we should restart on a new TB,
            and via longjmp via cpu_loop_exit.  */
         else {
-            replay_interrupt();
             if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
+                replay_interrupt();
                 *last_tb = NULL;
             }
             /* The target hook may have updated the 'cpu->interrupt_request';
diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index fb79f31..d24574d 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -1331,6 +1331,7 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 #endif
     if (interrupt_request & CPU_INTERRUPT_SIPI) {
         do_cpu_sipi(cpu);
+        ret = true;
     } else if (env->hflags2 & HF2_GIF_MASK) {
         if ((interrupt_request & CPU_INTERRUPT_SMI) &&
             !(env->hflags & HF_SMM_MASK)) {

[Qemu-devel] [PATCH v7 03/14] replay: exception replay fix

[Pavel Dovgalyuk]

This patch fixes replaying the exception when TB cache is full.
It breaks cpu loop execution through setting exception_index
to process such queued work as TB flush.

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

diff --git a/cpu-exec.c b/cpu-exec.c
index fa08c73..79a2167 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -451,6 +451,10 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
 #ifndef CONFIG_USER_ONLY
     } else if (replay_has_exception()
                && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
+        /* Break the execution loop in case of running out of TB cache.
+           This is needed to make flushing of the TB cache, because
+           real flush is queued to be executed outside the cpu loop. */
+        cpu->exception_index = EXCP_INTERRUPT;
         /* try to cause an exception pending in the log */
         cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0), true);
         *ret = -1;

[Qemu-devel] [PATCH v7 04/14] icount: exit cpu loop on expire

[Pavel Dovgalyuk]

This patch adds check to break cpu loop when icount expires without
setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no
available translated blocks and all instructions were executed.
In icount replay mode unnecessary tb_find will be called (which may
cause an exception) and execution will be non-deterministic.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 cpu-exec.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 79a2167..e603da4 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -582,9 +582,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
                 cpu_exec_nocache(cpu, insns_left, *last_tb, false);
                 align_clocks(sc, cpu);
             }
-            cpu->exception_index = EXCP_INTERRUPT;
-            *last_tb = NULL;
-            cpu_loop_exit(cpu);
         }
         break;
 #endif
@@ -638,6 +635,18 @@ int cpu_exec(CPUState *cpu)
 
             for(;;) {
                 cpu_handle_interrupt(cpu, &last_tb);
+                /* icount has expired, we need to break the execution loop.
+                   This check is needed before tb_find to make execution
+                   deterministic - tb_find may cause an exception
+                   while translating the code from non-mapped page. */
+                if (use_icount && ((cpu->icount_extra == 0
+                                    && cpu->icount_decr.u16.low == 0)
+                                || (int32_t)cpu->icount_decr.u32 < 0)) {
+                    if (cpu->exception_index == -1) {
+                        cpu->exception_index = EXCP_INTERRUPT;
+                    }
+                    cpu_loop_exit(cpu);
+                }
                 tb = tb_find(cpu, last_tb, tb_exit);
                 cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
                 /* Try to align the host and virtual clocks

[Qemu-devel] [PATCH v7 05/14] apic: save apic_delivered flag

[Pavel Dovgalyuk]

This patch implements saving/restoring of static apic_delivered variable.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/intc/apic_common.c           |   32 ++++++++++++++++++++++++++++++++
 include/hw/i386/apic_internal.h |    2 ++
 2 files changed, 34 insertions(+)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index d78c885..ac6cc67 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -384,6 +384,24 @@ static bool apic_common_sipi_needed(void *opaque)
     return s->wait_for_sipi != 0;
 }
 
+static bool apic_irq_delivered_needed(void *opaque)
+{
+    return true;
+}
+
+static void apic_irq_delivered_pre_save(void *opaque)
+{
+    APICCommonState *s = APIC_COMMON(opaque);
+    s->apic_irq_delivered = apic_irq_delivered;
+}
+
+static int apic_irq_delivered_post_load(void *opaque, int version_id)
+{
+    APICCommonState *s = APIC_COMMON(opaque);
+    apic_irq_delivered = s->apic_irq_delivered;
+    return 0;
+}
+
 static const VMStateDescription vmstate_apic_common_sipi = {
     .name = "apic_sipi",
     .version_id = 1,
@@ -396,6 +414,19 @@ static const VMStateDescription vmstate_apic_common_sipi = {
     }
 };
 
+static const VMStateDescription vmstate_apic_irq_delivered = {
+    .name = "apic_irq_delivered",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = apic_irq_delivered_needed,
+    .pre_save = apic_irq_delivered_pre_save,
+    .post_load = apic_irq_delivered_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(apic_irq_delivered, APICCommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_apic_common = {
     .name = "apic",
     .version_id = 3,
@@ -430,6 +461,7 @@ static const VMStateDescription vmstate_apic_common = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmstate_apic_common_sipi,
+        &vmstate_apic_irq_delivered,
         NULL
     }
 };
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 1209eb4..20ad28c 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -189,6 +189,8 @@ struct APICCommonState {
     DeviceState *vapic;
     hwaddr vapic_paddr; /* note: persistence via kvmvapic */
     bool legacy_instance_id;
+
+    int apic_irq_delivered; /* for saving static variable */
 };
 
 typedef struct VAPICState {

[Qemu-devel] [PATCH v7 06/14] replay: don't use rtc clock on loadvm phase

[Pavel Dovgalyuk]

This patch disables the update of the periodic timer of mc146818rtc
in record/replay mode. State of this timer is saved and therefore does
not need to be updated in record/replay mode.
Read of RTC breaks the replay because all rtc reads have to be the same
as in record mode.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/timer/mc146818rtc.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index da209d0..5638777 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -27,6 +27,7 @@
 #include "hw/hw.h"
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/replay.h"
 #include "hw/timer/mc146818rtc.h"
 #include "qapi/visitor.h"
 #include "qapi-event.h"
@@ -734,10 +735,15 @@ static int rtc_post_load(void *opaque, int version_id)
         check_update_timer(s);
     }
 
-    uint64_t now = qemu_clock_get_ns(rtc_clock);
-    if (now < s->next_periodic_time ||
-        now > (s->next_periodic_time + get_max_clock_jump())) {
-        periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+    /* Periodic timer is deterministic in record/replay mode.
+       No need to update it after loading the vmstate.
+       Reading RTC here may break the execution. */
+    if (replay_mode == REPLAY_MODE_NONE) {
+        uint64_t now = qemu_clock_get_ns(rtc_clock);
+        if (now < s->next_periodic_time ||
+            now > (s->next_periodic_time + get_max_clock_jump())) {
+            periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
+        }
     }
 
 #ifdef TARGET_I386

[Qemu-devel] [PATCH v7 07/14] 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 039812a..ca06e1b 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)
 {
@@ -309,6 +330,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;
@@ -438,6 +472,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)
 {
@@ -640,6 +685,21 @@ static void core_class_init(ObjectClass *klass, void *data)
 
     dc->props = core_properties;
     dc->realize = integratorcm_realize;
+    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 = {
@@ -655,6 +715,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 = {
@@ -662,6 +723,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)

[Qemu-devel] [PATCH v7 08/14] 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 cccde56..4cdba34 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -75,6 +75,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 f9c06e9..1e69225 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2039,38 +2039,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);
 
@@ -2079,7 +2081,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);
 
@@ -2125,13 +2127,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 v7 09/14] 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                     |    7 ++++++-
 6 files changed, 59 insertions(+), 3 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 c534a2f..32c7d2b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3390,12 +3390,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
@@ -3428,6 +3428,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 4980597..f2cf748 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -59,3 +59,20 @@ 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 c643d3f..52a6ac8 100644
--- a/vl.c
+++ b/vl.c
@@ -462,6 +462,9 @@ static QemuOptsList qemu_icount_opts = {
         }, {
             .name = "rrfile",
             .type = QEMU_OPT_STRING,
+        }, {
+            .name = "rrsnapshot",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
@@ -4651,7 +4654,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 v7 10/14] block: implement bdrv_snapshot_goto for blkreplay

[Pavel Dovgalyuk]

This patch enables making snapshots with blkreplay used in
block devices.

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

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

[Qemu-devel] [PATCH v7 11/14] blkreplay: create temporary overlay for underlaying devices

[Pavel Dovgalyuk]

This patch allows using '-snapshot' behavior in record/replay mode.
blkreplay layer creates temporary overlays on top of underlaying
disk images. It is needed, because creating an overlay over blkreplay
breaks the determinism.

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

diff --git a/block/blkreplay.c b/block/blkreplay.c
index 8a03d62..172642f 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -14,12 +14,76 @@
 #include "block/block_int.h"
 #include "sysemu/replay.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qstring.h"
 
 typedef struct Request {
     Coroutine *co;
     QEMUBH *bh;
 } Request;
 
+static BlockDriverState *blkreplay_append_snapshot(BlockDriverState *bs,
+                                                   Error **errp)
+{
+    int ret;
+    BlockDriverState *bs_snapshot;
+    int64_t total_size;
+    QemuOpts *opts = NULL;
+    char tmp_filename[PATH_MAX + 1];
+    QDict *snapshot_options = qdict_new();
+
+    /* Prepare options QDict for the overlay file */
+    qdict_put(snapshot_options, "file.driver",
+              qstring_from_str("file"));
+    qdict_put(snapshot_options, "driver",
+              qstring_from_str("qcow2"));
+
+    /* Create temporary file */
+    ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not get temporary filename");
+        goto out;
+    }
+    qdict_put(snapshot_options, "file.filename",
+              qstring_from_str(tmp_filename));
+
+    /* Get the required size from the image */
+    total_size = bdrv_getlength(bs);
+    if (total_size < 0) {
+        error_setg_errno(errp, -total_size, "Could not get image size");
+        goto out;
+    }
+
+    opts = qemu_opts_create(bdrv_qcow2.create_opts, NULL, 0, &error_abort);
+    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, &error_abort);
+    ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, errp);
+    qemu_opts_del(opts);
+    if (ret < 0) {
+        error_prepend(errp, "Could not create temporary overlay '%s': ",
+                      tmp_filename);
+        goto out;
+    }
+
+    bs_snapshot = bdrv_open(NULL, NULL, snapshot_options,
+                            BDRV_O_RDWR | BDRV_O_TEMPORARY, errp);
+    snapshot_options = NULL;
+    if (!bs_snapshot) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    /* bdrv_append() consumes a strong reference to bs_snapshot (i.e. it will
+     * call bdrv_unref() on it), so in order to be able to return one, we have
+     * to increase bs_snapshot's refcount here */
+    bdrv_ref(bs_snapshot);
+    bdrv_append(bs_snapshot, bs);
+
+    return bs_snapshot;
+
+out:
+    QDECREF(snapshot_options);
+    return NULL;
+}
+
 static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
@@ -35,6 +99,14 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    /* Add temporary snapshot to preserve the image */
+    if (!replay_snapshot
+        && !blkreplay_append_snapshot(bs->file->bs, &local_err)) {
+        ret = -EINVAL;
+        error_propagate(errp, local_err);
+        goto fail;
+    }
+
     ret = 0;
 fail:
     if (ret < 0) {
@@ -45,6 +117,10 @@ fail:
 
 static void blkreplay_close(BlockDriverState *bs)
 {
+    if (!replay_snapshot) {
+        /* Unref created snapshot file */
+        bdrv_unref(bs->file->bs);
+    }
 }
 
 static int64_t blkreplay_getlength(BlockDriverState *bs)
diff --git a/stubs/replay.c b/stubs/replay.c
index d9a6da9..e43e467 100644
--- a/stubs/replay.c
+++ b/stubs/replay.c
@@ -3,6 +3,7 @@
 #include "sysemu/sysemu.h"
 
 ReplayMode replay_mode;
+char *replay_snapshot;
 
 int64_t replay_save_clock(unsigned int kind, int64_t clock)
 {
diff --git a/vl.c b/vl.c
index 52a6ac8..def0520 100644
--- a/vl.c
+++ b/vl.c
@@ -4474,7 +4474,7 @@ int main(int argc, char **argv, char **envp)
     }
 
     /* open the virtual block devices */
-    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
+    if (snapshot) {
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
                           NULL, NULL);
     }

[Qemu-devel] [PATCH v7 12/14] replay: disable default snapshot for record/replay

[Pavel Dovgalyuk]

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

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

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

[Qemu-devel] [PATCH v7 13/14] audio: make audio poll timer deterministic

[Pavel Dovgalyuk]

This patch changes resetting strategy of the audio polling timer.
It does not change expiration time if the timer is already set.

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

diff --git a/audio/audio.c b/audio/audio.c
index c845a44..1ee95a5 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1112,8 +1112,10 @@ static int audio_is_timer_needed (void)
 static void audio_reset_timer (AudioState *s)
 {
     if (audio_is_timer_needed ()) {
-        timer_mod (s->ts,
-            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
+        if (!timer_pending(s->ts)) {
+            timer_mod (s->ts,
+                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
+        }
     }
     else {
         timer_del (s->ts);

[Qemu-devel] [PATCH v7 14/14] replay: add record/replay for audio passthrough

[Pavel Dovgalyuk]

This patch adds recording and replaying audio data. Is saves synchronization
information for audio out and inputs from the microphone.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 audio/audio.c            |    9 ++++-
 audio/audio.h            |    5 +++
 audio/mixeng.c           |   31 ++++++++++++++++++
 docs/replay.txt          |    7 ++++
 include/sysemu/replay.h  |    7 ++++
 replay/Makefile.objs     |    1 +
 replay/replay-audio.c    |   79 ++++++++++++++++++++++++++++++++++++++++++++++
 replay/replay-internal.h |    4 ++
 8 files changed, 141 insertions(+), 2 deletions(-)
 create mode 100644 replay/replay-audio.c

diff --git a/audio/audio.c b/audio/audio.c
index 1ee95a5..415ee1b 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -28,6 +28,7 @@
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
 #include "qemu/cutils.h"
+#include "sysemu/replay.h"
 
 #define AUDIO_CAP "audio"
 #include "audio_int.h"
@@ -1389,6 +1390,7 @@ static void audio_run_out (AudioState *s)
 
         prev_rpos = hw->rpos;
         played = hw->pcm_ops->run_out (hw, live);
+        replay_audio_out(&played);
         if (audio_bug (AUDIO_FUNC, hw->rpos >= hw->samples)) {
             dolog ("hw->rpos=%d hw->samples=%d played=%d\n",
                    hw->rpos, hw->samples, played);
@@ -1452,9 +1454,12 @@ static void audio_run_in (AudioState *s)
 
     while ((hw = audio_pcm_hw_find_any_enabled_in (hw))) {
         SWVoiceIn *sw;
-        int captured, min;
+        int captured = 0, min;
 
-        captured = hw->pcm_ops->run_in (hw);
+        if (replay_mode != REPLAY_MODE_PLAY) {
+            captured = hw->pcm_ops->run_in (hw);
+        }
+        replay_audio_in(&captured, hw->conv_buf, &hw->wpos, hw->samples);
 
         min = audio_pcm_hw_find_min_in (hw);
         hw->total_samples_captured += captured - min;
diff --git a/audio/audio.h b/audio/audio.h
index c3c5198..f4339a1 100644
--- a/audio/audio.h
+++ b/audio/audio.h
@@ -166,4 +166,9 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
 bool audio_is_cleaning_up(void);
 void audio_cleanup(void);
 
+void audio_sample_to_uint64(void *samples, int pos,
+                            uint64_t *left, uint64_t *right);
+void audio_sample_from_uint64(void *samples, int pos,
+                            uint64_t left, uint64_t right);
+
 #endif /* QEMU_AUDIO_H */
diff --git a/audio/mixeng.c b/audio/mixeng.c
index 66c0328..c23508e 100644
--- a/audio/mixeng.c
+++ b/audio/mixeng.c
@@ -267,6 +267,37 @@ f_sample *mixeng_clip[2][2][2][3] = {
     }
 };
 
+
+void audio_sample_to_uint64(void *samples, int pos,
+                            uint64_t *left, uint64_t *right)
+{
+    struct st_sample *sample = samples;
+    sample += pos;
+#ifdef FLOAT_MIXENG
+    error_report(
+        "Coreaudio and floating point samples are not supported by replay yet");
+    abort();
+#else
+    *left = sample->l;
+    *right = sample->r;
+#endif
+}
+
+void audio_sample_from_uint64(void *samples, int pos,
+                            uint64_t left, uint64_t right)
+{
+    struct st_sample *sample = samples;
+    sample += pos;
+#ifdef FLOAT_MIXENG
+    error_report(
+        "Coreaudio and floating point samples are not supported by replay yet");
+    abort();
+#else
+    sample->l = left;
+    sample->r = right;
+#endif
+}
+
 /*
  * August 21, 1998
  * Copyright 1998 Fabrice Bellard.
diff --git a/docs/replay.txt b/docs/replay.txt
index 03e1931..486c1e0 100644
--- a/docs/replay.txt
+++ b/docs/replay.txt
@@ -225,3 +225,10 @@ 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.
+
+Audio devices
+-------------
+
+Audio data is recorded and replay automatically. The command line for recording
+and replaying must contain identical specifications of audio hardware, e.g.:
+ -soundhw ac97
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index 740b425..073ec59 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -152,6 +152,13 @@ void replay_unregister_net(ReplayNetState *rns);
 void replay_net_packet_event(ReplayNetState *rns, unsigned flags,
                              const struct iovec *iov, int iovcnt);
 
+/* Audio */
+
+/*! Saves/restores number of played samples of audio out operation. */
+void replay_audio_out(int *played);
+/*! Saves/restores recorded samples of audio in operation. */
+void replay_audio_in(int *recorded, void *samples, int *wpos, int size);
+
 /* VM state operations */
 
 /*! Called at the start of execution.
diff --git a/replay/Makefile.objs b/replay/Makefile.objs
index b2afd40..cee6539 100644
--- a/replay/Makefile.objs
+++ b/replay/Makefile.objs
@@ -6,3 +6,4 @@ common-obj-y += replay-input.o
 common-obj-y += replay-char.o
 common-obj-y += replay-snapshot.o
 common-obj-y += replay-net.o
+common-obj-y += replay-audio.o
\ No newline at end of file
diff --git a/replay/replay-audio.c b/replay/replay-audio.c
new file mode 100644
index 0000000..3d83743
--- /dev/null
+++ b/replay/replay-audio.c
@@ -0,0 +1,79 @@
+/*
+ * replay-audio.c
+ *
+ * Copyright (c) 2010-2017 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 "audio/audio.h"
+
+void replay_audio_out(int *played)
+{
+    if (replay_mode == REPLAY_MODE_RECORD) {
+        replay_save_instructions();
+        replay_mutex_lock();
+        replay_put_event(EVENT_AUDIO_OUT);
+        replay_put_dword(*played);
+        replay_mutex_unlock();
+    } else if (replay_mode == REPLAY_MODE_PLAY) {
+        replay_account_executed_instructions();
+        replay_mutex_lock();
+        if (replay_next_event_is(EVENT_AUDIO_OUT)) {
+            *played = replay_get_dword();
+            replay_finish_event();
+            replay_mutex_unlock();
+        } else {
+            replay_mutex_unlock();
+            error_report("Missing audio out event in the replay log");
+            abort();
+        }
+    }
+}
+
+void replay_audio_in(int *recorded, void *samples, int *wpos, int size)
+{
+    int pos;
+    uint64_t left, right;
+    if (replay_mode == REPLAY_MODE_RECORD) {
+        replay_save_instructions();
+        replay_mutex_lock();
+        replay_put_event(EVENT_AUDIO_IN);
+        replay_put_dword(*recorded);
+        replay_put_dword(*wpos);
+        for (pos = (*wpos - *recorded + size) % size ; pos != *wpos
+             ; pos = (pos + 1) % size) {
+            audio_sample_to_uint64(samples, pos, &left, &right);
+            replay_put_qword(left);
+            replay_put_qword(right);
+        }
+        replay_mutex_unlock();
+    } else if (replay_mode == REPLAY_MODE_PLAY) {
+        replay_account_executed_instructions();
+        replay_mutex_lock();
+        if (replay_next_event_is(EVENT_AUDIO_IN)) {
+            *recorded = replay_get_dword();
+            *wpos = replay_get_dword();
+            for (pos = (*wpos - *recorded + size) % size ; pos != *wpos
+                 ; pos = (pos + 1) % size) {
+                left = replay_get_qword();
+                right = replay_get_qword();
+                audio_sample_from_uint64(samples, pos, left, right);
+            }
+            replay_finish_event();
+            replay_mutex_unlock();
+        } else {
+            replay_mutex_unlock();
+            error_report("Missing audio in event in the replay log");
+            abort();
+        }
+    }
+}
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index c26d079..ed66ed8 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -29,6 +29,10 @@ enum ReplayEvents {
     /* for character device read all event */
     EVENT_CHAR_READ_ALL,
     EVENT_CHAR_READ_ALL_ERROR,
+    /* for audio out event */
+    EVENT_AUDIO_OUT,
+    /* for audio in event */
+    EVENT_AUDIO_IN,
     /* for clock read/writes */
     /* some of greater codes are reserved for clocks */
     EVENT_CLOCK,

Re: [Qemu-devel] [PATCH v7 00/14] replay additions

[no-reply]

Hi,

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

Type: series
Subject: [Qemu-devel] [PATCH v7 00/14] replay additions
Message-id: 20170124071654.4572.41407.stgit@PASHA-ISP

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

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

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

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

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1485207141-1941-1-git-send-email-quintela@redhat.com -> patchew/1485207141-1941-1-git-send-email-quintela@redhat.com
 - [tag update]      patchew/20160908145158.30720-1-paul.burton@imgtec.com -> patchew/20160908145158.30720-1-paul.burton@imgtec.com
 * [new tag]         patchew/20170124071654.4572.41407.stgit@PASHA-ISP -> patchew/20170124071654.4572.41407.stgit@PASHA-ISP
Switched to a new branch 'test'
0191b3c replay: add record/replay for audio passthrough
ca532c5 audio: make audio poll timer deterministic
8690408 replay: disable default snapshot for record/replay
2d11131 blkreplay: create temporary overlay for underlaying devices
7d471b1 block: implement bdrv_snapshot_goto for blkreplay
eab8ef5 replay: save/load initial state
870fbf1 savevm: add public save_vmstate function
aff7d02 integratorcp: adding vmstate for save/restore
f604803 replay: don't use rtc clock on loadvm phase
b01b9f2 apic: save apic_delivered flag
06b4384 icount: exit cpu loop on expire
8995a01 replay: exception replay fix
1de2b63 replay: improve interrupt handling
84b9049 icount: update instruction counter on apic patching

=== OUTPUT BEGIN ===
Checking PATCH 1/14: icount: update instruction counter on apic patching...
Checking PATCH 2/14: replay: improve interrupt handling...
Checking PATCH 3/14: replay: exception replay fix...
Checking PATCH 4/14: icount: exit cpu loop on expire...
Checking PATCH 5/14: apic: save apic_delivered flag...
Checking PATCH 6/14: replay: don't use rtc clock on loadvm phase...
Checking PATCH 7/14: integratorcp: adding vmstate for save/restore...
Checking PATCH 8/14: savevm: add public save_vmstate function...
Checking PATCH 9/14: replay: save/load initial state...
Checking PATCH 10/14: block: implement bdrv_snapshot_goto for blkreplay...
Checking PATCH 11/14: blkreplay: create temporary overlay for underlaying devices...
Checking PATCH 12/14: replay: disable default snapshot for record/replay...
Checking PATCH 13/14: audio: make audio poll timer deterministic...
ERROR: space prohibited between function name and open parenthesis '('
#23: FILE: audio/audio.c:1116:
+            timer_mod (s->ts,

total: 1 errors, 0 warnings, 12 lines checked

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

Checking PATCH 14/14: replay: add record/replay for audio passthrough...
ERROR: space prohibited between function name and open parenthesis '('
#41: FILE: audio/audio.c:1460:
+            captured = hw->pcm_ops->run_in (hw);

total: 1 errors, 0 warnings, 190 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


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

Re: [Qemu-devel] [PATCH v7 03/14] replay: exception replay fix

[Paolo Bonzini]

On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> @@ -451,6 +451,10 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
>  #ifndef CONFIG_USER_ONLY
>      } else if (replay_has_exception()
>                 && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
> +        /* Break the execution loop in case of running out of TB cache.
> +           This is needed to make flushing of the TB cache, because
> +           real flush is queued to be executed outside the cpu loop. */
> +        cpu->exception_index = EXCP_INTERRUPT;
>          /* try to cause an exception pending in the log */
>          cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0), true);
>          *ret = -1;

Why is replay_has_exception() related to be running out of TB cache?

Paolo

Re: [Qemu-devel] [PATCH v7 04/14] icount: exit cpu loop on expire

[Paolo Bonzini]

On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> This patch adds check to break cpu loop when icount expires without
> setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no
> available translated blocks and all instructions were executed.
> In icount replay mode unnecessary tb_find will be called (which may
> cause an exception) and execution will be non-deterministic.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  cpu-exec.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 79a2167..e603da4 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -582,9 +582,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
>                  cpu_exec_nocache(cpu, insns_left, *last_tb, false);
>                  align_clocks(sc, cpu);
>              }
> -            cpu->exception_index = EXCP_INTERRUPT;
> -            *last_tb = NULL;
> -            cpu_loop_exit(cpu);
>          }
>          break;
>  #endif
> @@ -638,6 +635,18 @@ int cpu_exec(CPUState *cpu)
>  
>              for(;;) {
>                  cpu_handle_interrupt(cpu, &last_tb);
> +                /* icount has expired, we need to break the execution loop.
> +                   This check is needed before tb_find to make execution
> +                   deterministic - tb_find may cause an exception
> +                   while translating the code from non-mapped page. */
> +                if (use_icount && ((cpu->icount_extra == 0
> +                                    && cpu->icount_decr.u16.low == 0)
> +                                || (int32_t)cpu->icount_decr.u32 < 0)) {
> +                    if (cpu->exception_index == -1) {
> +                        cpu->exception_index = EXCP_INTERRUPT;
> +                    }
> +                    cpu_loop_exit(cpu);
> +                }

Can this can be placed in cpu_handle_interrupt itself?  Also, can the
cpu->exception_index != -1 case happen?

We really should add assertions on cpu->exception_index, maybe after
cpu_handle_exception and cc->cpu_exec_interrupt returns true.  Without
some idea of the invariants, I'm not competent enough to review this patch.

Paolo

>                  tb = tb_find(cpu, last_tb, tb_exit);
>                  cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
>                  /* Try to align the host and virtual clocks
> 

Re: [Qemu-devel] [PATCH v7 05/14] apic: save apic_delivered flag

[Paolo Bonzini]

On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> This patch implements saving/restoring of static apic_delivered variable.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/intc/apic_common.c           |   32 ++++++++++++++++++++++++++++++++
>  include/hw/i386/apic_internal.h |    2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index d78c885..ac6cc67 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -384,6 +384,24 @@ static bool apic_common_sipi_needed(void *opaque)
>      return s->wait_for_sipi != 0;
>  }
>  
> +static bool apic_irq_delivered_needed(void *opaque)
> +{
> +    return true;

Is it needed for CPUs except the first (or the last?)?

Paolo


> +}
> +
> +static void apic_irq_delivered_pre_save(void *opaque)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    s->apic_irq_delivered = apic_irq_delivered;
> +}
> +
> +static int apic_irq_delivered_post_load(void *opaque, int version_id)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    apic_irq_delivered = s->apic_irq_delivered;
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_apic_common_sipi = {
>      .name = "apic_sipi",
>      .version_id = 1,
> @@ -396,6 +414,19 @@ static const VMStateDescription vmstate_apic_common_sipi = {
>      }
>  };
>  
> +static const VMStateDescription vmstate_apic_irq_delivered = {
> +    .name = "apic_irq_delivered",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = apic_irq_delivered_needed,
> +    .pre_save = apic_irq_delivered_pre_save,
> +    .post_load = apic_irq_delivered_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(apic_irq_delivered, APICCommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_apic_common = {
>      .name = "apic",
>      .version_id = 3,
> @@ -430,6 +461,7 @@ static const VMStateDescription vmstate_apic_common = {
>      },
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_apic_common_sipi,
> +        &vmstate_apic_irq_delivered,
>          NULL
>      }
>  };
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 1209eb4..20ad28c 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -189,6 +189,8 @@ struct APICCommonState {
>      DeviceState *vapic;
>      hwaddr vapic_paddr; /* note: persistence via kvmvapic */
>      bool legacy_instance_id;
> +
> +    int apic_irq_delivered; /* for saving static variable */
>  };
>  
>  typedef struct VAPICState {
> 
> 
> 

Re: [Qemu-devel] [PATCH v7 00/14] replay additions

[Paolo Bonzini]

On 24/01/2017 08:16, Pavel Dovgalyuk wrote:
> This set of patches includes several fixes for replay and vmstate.
> 
> 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.
> 
> With these patches operations with audio devices can also be recorded
> and replayed. All interactions with passthrough audio (including
> microphone input) are recorded automatically when -soundhw is specified
> at the command line.
> 
> This set of patches includes fixes and additions for icount and
> record/replay implementation:
>  - VM start/stop in replay mode
>  - overlay creation for blkreplay filter
>  - rrsnapshot option for record/replay
>  - vmstate fix for integratorcp ARM platform
>  - vmstate fixes for apic and rtc
>  - fixes for icount record/replay mode
>  - record/replay for audio devices
> 
> v7 changes:
>  - Fixed exception replaying when TB cache is full and
>    when tb_find is called when there are no instructions about to execute
>  - Added record/replay for audio devices

Queued patches 1, 2, 6, 8, 9.

Please post patches for ARM, block and audio maintainers separately if
you don't get attention soon.

Paolo

> v6 changes:
>  - Added overlay creation for blkreplay driver
>  - Fixed vmstate loading for apic and rtc
>  - Fixed instruction counting for apic instruction patching
> 
> v5 changes:
>  - Recording is stopped when initial snapshot cannot be created
>  - Minor changes
> 
> v4 changes:
>  - Overlay option is removed from blkreplay driver (as suggested by Paolo Bonzini)
>  - Minor changes
> 
> v3 changes:
>  - Added rrsnapshot option for specifying the initial snapshot name (as suggested by Paolo Bonzini)
>  - Minor changes
> 
> ---
> 
> Pavel Dovgalyuk (14):
>       icount: update instruction counter on apic patching
>       replay: improve interrupt handling
>       replay: exception replay fix
>       icount: exit cpu loop on expire
>       apic: save apic_delivered flag
>       replay: don't use rtc clock on loadvm phase
>       integratorcp: adding vmstate for save/restore
>       savevm: add public save_vmstate function
>       replay: save/load initial state
>       block: implement bdrv_snapshot_goto for blkreplay
>       blkreplay: create temporary overlay for underlaying devices
>       replay: disable default snapshot for record/replay
>       audio: make audio poll timer deterministic
>       replay: add record/replay for audio passthrough
> 
> 
>  audio/audio.c                   |   15 +++++--
>  audio/audio.h                   |    5 ++
>  audio/mixeng.c                  |   31 ++++++++++++++
>  block/blkreplay.c               |   84 +++++++++++++++++++++++++++++++++++++++
>  cpu-exec.c                      |   21 ++++++++--
>  docs/replay.txt                 |   23 +++++++++++
>  hw/arm/integratorcp.c           |   62 +++++++++++++++++++++++++++++
>  hw/i386/kvmvapic.c              |    6 +++
>  hw/intc/apic_common.c           |   32 +++++++++++++++
>  hw/timer/mc146818rtc.c          |   14 +++++--
>  include/hw/i386/apic_internal.h |    2 +
>  include/sysemu/replay.h         |   16 +++++++
>  include/sysemu/sysemu.h         |    1 
>  migration/savevm.c              |   33 ++++++++++-----
>  qemu-options.hx                 |    8 +++-
>  replay/Makefile.objs            |    1 
>  replay/replay-audio.c           |   79 +++++++++++++++++++++++++++++++++++++
>  replay/replay-internal.h        |    4 ++
>  replay/replay-snapshot.c        |   17 ++++++++
>  replay/replay.c                 |    5 ++
>  stubs/replay.c                  |    1 
>  target/i386/seg_helper.c        |    1 
>  vl.c                            |   17 +++++++-
>  23 files changed, 450 insertions(+), 28 deletions(-)
>  create mode 100644 replay/replay-audio.c
> 

Re: [Qemu-devel] [PATCH v7 03/14] replay: exception replay fix

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> > @@ -451,6 +451,10 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> >  #ifndef CONFIG_USER_ONLY
> >      } else if (replay_has_exception()
> >                 && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
> > +        /* Break the execution loop in case of running out of TB cache.
> > +           This is needed to make flushing of the TB cache, because
> > +           real flush is queued to be executed outside the cpu loop. */
> > +        cpu->exception_index = EXCP_INTERRUPT;
> >          /* try to cause an exception pending in the log */
> >          cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0), true);
> >          *ret = -1;
> 
> Why is replay_has_exception() related to be running out of TB cache?

It doesn't.
Calling tb_find when there is not space in cache causes tb_flush and cpu_loop_exit.
But execution loop will continue, because there is no reason to break it
(like setting exception_index).

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v7 03/14] replay: exception replay fix

[Paolo Bonzini]

On 25/01/2017 12:12, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
>>> @@ -451,6 +451,10 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
>>>  #ifndef CONFIG_USER_ONLY
>>>      } else if (replay_has_exception()
>>>                 && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
>>> +        /* Break the execution loop in case of running out of TB cache.
>>> +           This is needed to make flushing of the TB cache, because
>>> +           real flush is queued to be executed outside the cpu loop. */
>>> +        cpu->exception_index = EXCP_INTERRUPT;
>>>          /* try to cause an exception pending in the log */
>>>          cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0), true);
>>>          *ret = -1;
>>
>> Why is replay_has_exception() related to be running out of TB cache?
> 
> It doesn't.
> Calling tb_find when there is not space in cache causes tb_flush and cpu_loop_exit.
> But execution loop will continue, because there is no reason to break it
> (like setting exception_index).

What about setting cpu->exit_request?  queue_work_on_cpu calls
qemu_cpu_kick.

Paolo

Re: [Qemu-devel] [PATCH v7 03/14] replay: exception replay fix

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 25/01/2017 12:12, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> >>> @@ -451,6 +451,10 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> >>>  #ifndef CONFIG_USER_ONLY
> >>>      } else if (replay_has_exception()
> >>>                 && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
> >>> +        /* Break the execution loop in case of running out of TB cache.
> >>> +           This is needed to make flushing of the TB cache, because
> >>> +           real flush is queued to be executed outside the cpu loop. */
> >>> +        cpu->exception_index = EXCP_INTERRUPT;
> >>>          /* try to cause an exception pending in the log */
> >>>          cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0), true);
> >>>          *ret = -1;
> >>
> >> Why is replay_has_exception() related to be running out of TB cache?
> >
> > It doesn't.
> > Calling tb_find when there is not space in cache causes tb_flush and cpu_loop_exit.
> > But execution loop will continue, because there is no reason to break it
> > (like setting exception_index).
> 
> What about setting cpu->exit_request?  queue_work_on_cpu calls
> qemu_cpu_kick.

cpu->exit_request does not checked in this loop.
We have to add this checking somewhere then?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v7 04/14] icount: exit cpu loop on expire

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> > This patch adds check to break cpu loop when icount expires without
> > setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no
> > available translated blocks and all instructions were executed.
> > In icount replay mode unnecessary tb_find will be called (which may
> > cause an exception) and execution will be non-deterministic.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  cpu-exec.c |   15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index 79a2167..e603da4 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -582,9 +582,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
> >                  cpu_exec_nocache(cpu, insns_left, *last_tb, false);
> >                  align_clocks(sc, cpu);
> >              }
> > -            cpu->exception_index = EXCP_INTERRUPT;
> > -            *last_tb = NULL;
> > -            cpu_loop_exit(cpu);
> >          }
> >          break;
> >  #endif
> > @@ -638,6 +635,18 @@ int cpu_exec(CPUState *cpu)
> >
> >              for(;;) {
> >                  cpu_handle_interrupt(cpu, &last_tb);
> > +                /* icount has expired, we need to break the execution loop.
> > +                   This check is needed before tb_find to make execution
> > +                   deterministic - tb_find may cause an exception
> > +                   while translating the code from non-mapped page. */
> > +                if (use_icount && ((cpu->icount_extra == 0
> > +                                    && cpu->icount_decr.u16.low == 0)
> > +                                || (int32_t)cpu->icount_decr.u32 < 0)) {
> > +                    if (cpu->exception_index == -1) {
> > +                        cpu->exception_index = EXCP_INTERRUPT;
> > +                    }
> > +                    cpu_loop_exit(cpu);
> > +                }
> 
> Can this can be placed in cpu_handle_interrupt itself?  

I guess it could. I placed it here because it doesn't related to interrupts.

> Also, can the cpu->exception_index != -1 case happen?

This branch is executed where there are no instructions to replay.
It may happen due to an exception (which is already set) or because
execution loop has to break. In the first case exception_index != -1.

> We really should add assertions on cpu->exception_index, maybe after
> cpu_handle_exception and cc->cpu_exec_interrupt returns true.  Without
> some idea of the invariants, I'm not competent enough to review this patch.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v7 05/14] apic: save apic_delivered flag

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> > This patch implements saving/restoring of static apic_delivered variable.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  hw/intc/apic_common.c           |   32 ++++++++++++++++++++++++++++++++
> >  include/hw/i386/apic_internal.h |    2 ++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> > index d78c885..ac6cc67 100644
> > --- a/hw/intc/apic_common.c
> > +++ b/hw/intc/apic_common.c
> > @@ -384,6 +384,24 @@ static bool apic_common_sipi_needed(void *opaque)
> >      return s->wait_for_sipi != 0;
> >  }
> >
> > +static bool apic_irq_delivered_needed(void *opaque)
> > +{
> > +    return true;
> 
> Is it needed for CPUs except the first (or the last?)?
> 

As this is global variable, it is needed only for one CPU.
Do you mean that APIC state is saved for every CPU? 

> > +}
> > +
> > +static void apic_irq_delivered_pre_save(void *opaque)
> > +{
> > +    APICCommonState *s = APIC_COMMON(opaque);
> > +    s->apic_irq_delivered = apic_irq_delivered;
> > +}
> > +
> > +static int apic_irq_delivered_post_load(void *opaque, int version_id)
> > +{
> > +    APICCommonState *s = APIC_COMMON(opaque);
> > +    apic_irq_delivered = s->apic_irq_delivered;
> > +    return 0;
> > +}
> > +
> >  static const VMStateDescription vmstate_apic_common_sipi = {
> >      .name = "apic_sipi",
> >      .version_id = 1,
> > @@ -396,6 +414,19 @@ static const VMStateDescription vmstate_apic_common_sipi = {
> >      }
> >  };
> >
> > +static const VMStateDescription vmstate_apic_irq_delivered = {
> > +    .name = "apic_irq_delivered",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = apic_irq_delivered_needed,
> > +    .pre_save = apic_irq_delivered_pre_save,
> > +    .post_load = apic_irq_delivered_post_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_INT32(apic_irq_delivered, APICCommonState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static const VMStateDescription vmstate_apic_common = {
> >      .name = "apic",
> >      .version_id = 3,
> > @@ -430,6 +461,7 @@ static const VMStateDescription vmstate_apic_common = {
> >      },
> >      .subsections = (const VMStateDescription*[]) {
> >          &vmstate_apic_common_sipi,
> > +        &vmstate_apic_irq_delivered,
> >          NULL
> >      }
> >  };
> > diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> > index 1209eb4..20ad28c 100644
> > --- a/include/hw/i386/apic_internal.h
> > +++ b/include/hw/i386/apic_internal.h
> > @@ -189,6 +189,8 @@ struct APICCommonState {
> >      DeviceState *vapic;
> >      hwaddr vapic_paddr; /* note: persistence via kvmvapic */
> >      bool legacy_instance_id;
> > +
> > +    int apic_irq_delivered; /* for saving static variable */
> >  };
> >
> >  typedef struct VAPICState {
> >
> >
> >

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v7 03/14] replay: exception replay fix

[Paolo Bonzini]

On 25/01/2017 12:33, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 25/01/2017 12:12, Pavel Dovgalyuk wrote:
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
>>>>> @@ -451,6 +451,10 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
>>>>>  #ifndef CONFIG_USER_ONLY
>>>>>      } else if (replay_has_exception()
>>>>>                 && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
>>>>> +        /* Break the execution loop in case of running out of TB cache.
>>>>> +           This is needed to make flushing of the TB cache, because
>>>>> +           real flush is queued to be executed outside the cpu loop. */
>>>>> +        cpu->exception_index = EXCP_INTERRUPT;
>>>>>          /* try to cause an exception pending in the log */
>>>>>          cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0), true);
>>>>>          *ret = -1;
>>>>
>>>> Why is replay_has_exception() related to be running out of TB cache?
>>>
>>> It doesn't.
>>> Calling tb_find when there is not space in cache causes tb_flush and cpu_loop_exit.
>>> But execution loop will continue, because there is no reason to break it
>>> (like setting exception_index).
>>
>> What about setting cpu->exit_request?  queue_work_on_cpu calls
>> qemu_cpu_kick.
> 
> cpu->exit_request does not checked in this loop.
> We have to add this checking somewhere then?

It's checked by cpu_handle_interrupt.  Are you not reaching
cpu_handle_interrupt then?  Why?

Or perhaps cpu_handle_interrupt should not be testing cpu->exit_request,
but cpu->exception_index != -1 (and cpu_exit can cmpxchg
cpu->exception_index from -1 to EXCP_INTERRUPT)?

Again, it's hard to follow without knowing the invariants. :(

Paolo

Re: [Qemu-devel] [PATCH v7 05/14] apic: save apic_delivered flag

[Paolo Bonzini]

On 25/01/2017 12:52, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
>> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
>>> This patch implements saving/restoring of static apic_delivered variable.
>>>
>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>>> ---
>>>  hw/intc/apic_common.c           |   32 ++++++++++++++++++++++++++++++++
>>>  include/hw/i386/apic_internal.h |    2 ++
>>>  2 files changed, 34 insertions(+)
>>>
>>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>>> index d78c885..ac6cc67 100644
>>> --- a/hw/intc/apic_common.c
>>> +++ b/hw/intc/apic_common.c
>>> @@ -384,6 +384,24 @@ static bool apic_common_sipi_needed(void *opaque)
>>>      return s->wait_for_sipi != 0;
>>>  }
>>>
>>> +static bool apic_irq_delivered_needed(void *opaque)
>>> +{
>>> +    return true;
>>
>> Is it needed for CPUs except the first (or the last?)?
>>
> 
> As this is global variable, it is needed only for one CPU.
> Do you mean that APIC state is saved for every CPU? 

Yes, each CPU has its own APIC.

Paolo

>>> +}
>>> +
>>> +static void apic_irq_delivered_pre_save(void *opaque)
>>> +{
>>> +    APICCommonState *s = APIC_COMMON(opaque);
>>> +    s->apic_irq_delivered = apic_irq_delivered;
>>> +}
>>> +
>>> +static int apic_irq_delivered_post_load(void *opaque, int version_id)
>>> +{
>>> +    APICCommonState *s = APIC_COMMON(opaque);
>>> +    apic_irq_delivered = s->apic_irq_delivered;
>>> +    return 0;
>>> +}
>>> +
>>>  static const VMStateDescription vmstate_apic_common_sipi = {
>>>      .name = "apic_sipi",
>>>      .version_id = 1,
>>> @@ -396,6 +414,19 @@ static const VMStateDescription vmstate_apic_common_sipi = {
>>>      }
>>>  };
>>>
>>> +static const VMStateDescription vmstate_apic_irq_delivered = {
>>> +    .name = "apic_irq_delivered",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = apic_irq_delivered_needed,
>>> +    .pre_save = apic_irq_delivered_pre_save,
>>> +    .post_load = apic_irq_delivered_post_load,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_INT32(apic_irq_delivered, APICCommonState),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>>  static const VMStateDescription vmstate_apic_common = {
>>>      .name = "apic",
>>>      .version_id = 3,
>>> @@ -430,6 +461,7 @@ static const VMStateDescription vmstate_apic_common = {
>>>      },
>>>      .subsections = (const VMStateDescription*[]) {
>>>          &vmstate_apic_common_sipi,
>>> +        &vmstate_apic_irq_delivered,
>>>          NULL
>>>      }
>>>  };
>>> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
>>> index 1209eb4..20ad28c 100644
>>> --- a/include/hw/i386/apic_internal.h
>>> +++ b/include/hw/i386/apic_internal.h
>>> @@ -189,6 +189,8 @@ struct APICCommonState {
>>>      DeviceState *vapic;
>>>      hwaddr vapic_paddr; /* note: persistence via kvmvapic */
>>>      bool legacy_instance_id;
>>> +
>>> +    int apic_irq_delivered; /* for saving static variable */
>>>  };
>>>
>>>  typedef struct VAPICState {
>>>
>>>
>>>
> 
> Pavel Dovgalyuk
> 

Re: [Qemu-devel] [PATCH v7 04/14] icount: exit cpu loop on expire

[Paolo Bonzini]

On 25/01/2017 12:50, Pavel Dovgalyuk wrote:
>>> +                /* icount has expired, we need to break the execution loop.
>>> +                   This check is needed before tb_find to make execution
>>> +                   deterministic - tb_find may cause an exception
>>> +                   while translating the code from non-mapped page. */
>>> +                if (use_icount && ((cpu->icount_extra == 0
>>> +                                    && cpu->icount_decr.u16.low == 0)
>>> +                                || (int32_t)cpu->icount_decr.u32 < 0)) {
>>> +                    if (cpu->exception_index == -1) {
>>> +                        cpu->exception_index = EXCP_INTERRUPT;
>>> +                    }
>>> +                    cpu_loop_exit(cpu);
>>> +                }
>> Can this can be placed in cpu_handle_interrupt itself?  
> I guess it could. I placed it here because it doesn't related to interrupts.

True, on the other hand neither is

    if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) {
        atomic_set(&cpu->exit_request, 0);
        cpu->exception_index = EXCP_INTERRUPT;
        cpu_loop_exit(cpu);
    }

Except for the replay_has_interrupt() that you added, but I don't understand
that one either...

Paolo

Re: [Qemu-devel] [PATCH v7 03/14] replay: exception replay fix

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 25/01/2017 12:33, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 25/01/2017 12:12, Pavel Dovgalyuk wrote:
> >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >>>> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> >>>>> @@ -451,6 +451,10 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> >>>>>  #ifndef CONFIG_USER_ONLY
> >>>>>      } else if (replay_has_exception()
> >>>>>                 && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
> >>>>> +        /* Break the execution loop in case of running out of TB cache.
> >>>>> +           This is needed to make flushing of the TB cache, because
> >>>>> +           real flush is queued to be executed outside the cpu loop. */
> >>>>> +        cpu->exception_index = EXCP_INTERRUPT;
> >>>>>          /* try to cause an exception pending in the log */
> >>>>>          cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0), true);
> >>>>>          *ret = -1;
> >>>>
> >>>> Why is replay_has_exception() related to be running out of TB cache?
> >>>
> >>> It doesn't.
> >>> Calling tb_find when there is not space in cache causes tb_flush and cpu_loop_exit.
> >>> But execution loop will continue, because there is no reason to break it
> >>> (like setting exception_index).
> >>
> >> What about setting cpu->exit_request?  queue_work_on_cpu calls
> >> qemu_cpu_kick.
> >
> > cpu->exit_request does not checked in this loop.
> > We have to add this checking somewhere then?
> 
> It's checked by cpu_handle_interrupt.  Are you not reaching
> cpu_handle_interrupt then?  Why?

Execution doesn't reach cpu_handle_interrupt, because tb_find 
calls cpu_loop_exit. And the execution loop enters cpu_handle_exception again and again.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v7 05/14] apic: save apic_delivered flag

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 25/01/2017 12:52, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> >> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> >>> This patch implements saving/restoring of static apic_delivered variable.
> >>>
> >>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> >>> ---
> >>>  hw/intc/apic_common.c           |   32 ++++++++++++++++++++++++++++++++
> >>>  include/hw/i386/apic_internal.h |    2 ++
> >>>  2 files changed, 34 insertions(+)
> >>>
> >>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> >>> index d78c885..ac6cc67 100644
> >>> --- a/hw/intc/apic_common.c
> >>> +++ b/hw/intc/apic_common.c
> >>> @@ -384,6 +384,24 @@ static bool apic_common_sipi_needed(void *opaque)
> >>>      return s->wait_for_sipi != 0;
> >>>  }
> >>>
> >>> +static bool apic_irq_delivered_needed(void *opaque)
> >>> +{
> >>> +    return true;
> >>
> >> Is it needed for CPUs except the first (or the last?)?
> >>
> >
> > As this is global variable, it is needed only for one CPU.
> > Do you mean that APIC state is saved for every CPU?
> 
> Yes, each CPU has its own APIC.

How to link this data to a single CPU?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v7 05/14] apic: save apic_delivered flag

[Paolo Bonzini]

On 25/01/2017 14:01, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 25/01/2017 12:52, Pavel Dovgalyuk wrote:
>>>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
>>>> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
>>>>> This patch implements saving/restoring of static apic_delivered variable.
>>>>>
>>>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>>>>> ---
>>>>>  hw/intc/apic_common.c           |   32 ++++++++++++++++++++++++++++++++
>>>>>  include/hw/i386/apic_internal.h |    2 ++
>>>>>  2 files changed, 34 insertions(+)
>>>>>
>>>>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>>>>> index d78c885..ac6cc67 100644
>>>>> --- a/hw/intc/apic_common.c
>>>>> +++ b/hw/intc/apic_common.c
>>>>> @@ -384,6 +384,24 @@ static bool apic_common_sipi_needed(void *opaque)
>>>>>      return s->wait_for_sipi != 0;
>>>>>  }
>>>>>
>>>>> +static bool apic_irq_delivered_needed(void *opaque)
>>>>> +{
>>>>> +    return true;
>>>>
>>>> Is it needed for CPUs except the first (or the last?)?
>>>>
>>>
>>> As this is global variable, it is needed only for one CPU.
>>> Do you mean that APIC state is saved for every CPU?
>>
>> Yes, each CPU has its own APIC.
> 
> How to link this data to a single CPU?

Since it's global, you only need to migrate it once.  You're migrating
the same value many times.

Paolo

Re: [Qemu-devel] [PATCH v7 03/14] replay: exception replay fix

[Paolo Bonzini]

On 25/01/2017 13:27, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 25/01/2017 12:33, Pavel Dovgalyuk wrote:
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> On 25/01/2017 12:12, Pavel Dovgalyuk wrote:
>>>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>>>> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
>>>>>>> @@ -451,6 +451,10 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
>>>>>>>  #ifndef CONFIG_USER_ONLY
>>>>>>>      } else if (replay_has_exception()
>>>>>>>                 && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
>>>>>>> +        /* Break the execution loop in case of running out of TB cache.
>>>>>>> +           This is needed to make flushing of the TB cache, because
>>>>>>> +           real flush is queued to be executed outside the cpu loop. */
>>>>>>> +        cpu->exception_index = EXCP_INTERRUPT;
>>>>>>>          /* try to cause an exception pending in the log */
>>>>>>>          cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0), true);
>>>>>>>          *ret = -1;
>>>>>>
>>>>>> Why is replay_has_exception() related to be running out of TB cache?
>>>>>
>>>>> It doesn't.
>>>>> Calling tb_find when there is not space in cache causes tb_flush and cpu_loop_exit.
>>>>> But execution loop will continue, because there is no reason to break it
>>>>> (like setting exception_index).
>>>>
>>>> What about setting cpu->exit_request?  queue_work_on_cpu calls
>>>> qemu_cpu_kick.
>>>
>>> cpu->exit_request does not checked in this loop.
>>> We have to add this checking somewhere then?
>>
>> It's checked by cpu_handle_interrupt.  Are you not reaching
>> cpu_handle_interrupt then?  Why?
> 
> Execution doesn't reach cpu_handle_interrupt, because tb_find 
> calls cpu_loop_exit. And the execution loop enters cpu_handle_exception again and again.

Perhaps tb_gen_code should set cpu->exception_index before calling
cpu_loop_exit.

Paolo

> Pavel Dovgalyuk
> 

Re: [Qemu-devel] [PATCH v7 03/14] replay: exception replay fix

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 25/01/2017 13:27, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 25/01/2017 12:33, Pavel Dovgalyuk wrote:
> >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >>>> On 25/01/2017 12:12, Pavel Dovgalyuk wrote:
> >>>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >>>>>> On 24/01/2017 08:17, Pavel Dovgalyuk wrote:
> >>>>>>> @@ -451,6 +451,10 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> >>>>>>>  #ifndef CONFIG_USER_ONLY
> >>>>>>>      } else if (replay_has_exception()
> >>>>>>>                 && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
> >>>>>>> +        /* Break the execution loop in case of running out of TB cache.
> >>>>>>> +           This is needed to make flushing of the TB cache, because
> >>>>>>> +           real flush is queued to be executed outside the cpu loop. */
> >>>>>>> +        cpu->exception_index = EXCP_INTERRUPT;
> >>>>>>>          /* try to cause an exception pending in the log */
> >>>>>>>          cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0), true);
> >>>>>>>          *ret = -1;
> >>>>>>
> >>>>>> Why is replay_has_exception() related to be running out of TB cache?
> >>>>>
> >>>>> It doesn't.
> >>>>> Calling tb_find when there is not space in cache causes tb_flush and cpu_loop_exit.
> >>>>> But execution loop will continue, because there is no reason to break it
> >>>>> (like setting exception_index).
> >>>>
> >>>> What about setting cpu->exit_request?  queue_work_on_cpu calls
> >>>> qemu_cpu_kick.
> >>>
> >>> cpu->exit_request does not checked in this loop.
> >>> We have to add this checking somewhere then?
> >>
> >> It's checked by cpu_handle_interrupt.  Are you not reaching
> >> cpu_handle_interrupt then?  Why?
> >
> > Execution doesn't reach cpu_handle_interrupt, because tb_find
> > calls cpu_loop_exit. And the execution loop enters cpu_handle_exception again and again.
> 
> Perhaps tb_gen_code should set cpu->exception_index before calling
> cpu_loop_exit.

Maybe it would be better, because tb_gen_code is called in many other paths.
I'll fix the patch.

Pavel Dovgalyuk