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

[Qemu-devel] [PATCH v8 0/9] 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

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

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

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

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

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

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

---

Pavel Dovgalyuk (9):
      replay: exception replay fix
      icount: exit cpu loop on expire
      apic: save apic_delivered flag
      integratorcp: adding vmstate for save/restore
      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                      |   24 +++++++++--
 docs/replay.txt                 |    7 +++
 hw/arm/integratorcp.c           |   62 +++++++++++++++++++++++++++++
 hw/intc/apic_common.c           |   37 +++++++++++++++++
 include/hw/i386/apic_internal.h |    2 +
 include/sysemu/replay.h         |    7 +++
 replay/Makefile.objs            |    1 
 replay/replay-audio.c           |   79 +++++++++++++++++++++++++++++++++++++
 replay/replay-internal.h        |    4 ++
 stubs/replay.c                  |    1 
 translate-all.c                 |    3 +
 vl.c                            |   10 ++++-
 16 files changed, 361 insertions(+), 11 deletions(-)
 create mode 100644 replay/replay-audio.c

-- 
Pavel Dovgalyuk

[Qemu-devel] [PATCH v8 1/9] 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.

v8: moved setting of exeption_index to tb_gen_code

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

diff --git a/translate-all.c b/translate-all.c
index 2026293..abce8f1 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1290,6 +1290,9 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
         /* flush must be done */
         tb_flush(cpu);
         mmap_unlock();
+        /* Set exception index to make sure that the execution loop
+           will exit and go to flushing and other queued work. */
+        cpu->exception_index = EXCP_INTERRUPT;
         cpu_loop_exit(cpu);
     }
 

[Qemu-devel] [PATCH v8 2/9] 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.

v8: refactored loop exit code and moved it to separate function

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

diff --git a/cpu-exec.c b/cpu-exec.c
index fa08c73..f9b8ec8 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -523,9 +523,25 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
             *last_tb = NULL;
         }
     }
-    if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) {
+}
+
+
+static void cpu_check_loop_exit(CPUState *cpu)
+{
+    if (unlikely(atomic_read(&cpu->exit_request)
+        /* 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. */
+        || (use_icount && ((cpu->icount_extra == 0
+                        && cpu->icount_decr.u16.low == 0)
+                    || (int32_t)cpu->icount_decr.u32 < 0)))) {
         atomic_set(&cpu->exit_request, 0);
-        cpu->exception_index = EXCP_INTERRUPT;
+        /* If there is an exception that wasn't replayed yet,
+           don't change exception_index. */
+        if (cpu->exception_index == -1) {
+            cpu->exception_index = EXCP_INTERRUPT;
+        }
         cpu_loop_exit(cpu);
     }
 }
@@ -578,9 +594,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
@@ -634,6 +647,7 @@ int cpu_exec(CPUState *cpu)
 
             for(;;) {
                 cpu_handle_interrupt(cpu, &last_tb);
+                cpu_check_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 v8 3/9] apic: save apic_delivered flag

[Pavel Dovgalyuk]

This patch implements saving/restoring of static apic_delivered variable.

v8: saving static variable only for one of the APICs

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

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index d78c885..edacb16 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
     return s->wait_for_sipi != 0;
 }
 
+static bool apic_irq_delivered_needed(void *opaque)
+{
+    static APICCommonState *first_apic;
+    APICCommonState *s = APIC_COMMON(opaque);
+    if (!first_apic) {
+        first_apic = s;
+    }
+    return s == first_apic;
+}
+
+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 +419,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 +466,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 v8 4/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 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 v8 5/9] 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 v8 6/9] 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 v8 7/9] 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 v8 8/9] 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 v8 9/9] 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            |   11 +++++-
 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, 142 insertions(+), 3 deletions(-)
 create mode 100644 replay/replay-audio.c

diff --git a/audio/audio.c b/audio/audio.c
index 1ee95a5..79c0788 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"
@@ -1113,7 +1114,7 @@ static void audio_reset_timer (AudioState *s)
 {
     if (audio_is_timer_needed ()) {
         if (!timer_pending(s->ts)) {
-            timer_mod (s->ts,
+            timer_mod(s->ts,
                 qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
         }
     }
@@ -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 v8 3/9] apic: save apic_delivered flag

[Paolo Bonzini]

On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
> This patch implements saving/restoring of static apic_delivered variable.
> 
> v8: saving static variable only for one of the APICs
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/intc/apic_common.c           |   37 +++++++++++++++++++++++++++++++++++++
>  include/hw/i386/apic_internal.h |    2 ++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index d78c885..edacb16 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
>      return s->wait_for_sipi != 0;
>  }
>  
> +static bool apic_irq_delivered_needed(void *opaque)
> +{
> +    static APICCommonState *first_apic;
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    if (!first_apic) {
> +        first_apic = s;
> +    }
> +    return s == first_apic;

Should also check " && apic_irq_delivered != 0".

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 +419,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 +466,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 v8 2/9] icount: exit cpu loop on expire

[Paolo Bonzini]

On 26/01/2017 13:34, 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.
> 
> v8: refactored loop exit code and moved it to separate function
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  cpu-exec.c |   24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index fa08c73..f9b8ec8 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -523,9 +523,25 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
>              *last_tb = NULL;
>          }
>      }
> -    if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) {
> +}
> +
> +
> +static void cpu_check_loop_exit(CPUState *cpu)
> +{
> +    if (unlikely(atomic_read(&cpu->exit_request)
> +        /* 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. */
> +        || (use_icount && ((cpu->icount_extra == 0
> +                        && cpu->icount_decr.u16.low == 0)
> +                    || (int32_t)cpu->icount_decr.u32 < 0)))) {

Simpler:

	use_icount &&
	((int32_t)cpu->icount_decr.u32 < 0 ||
	 cpu->icount_decr.low + cpu->icount_extra == 0)

But I'm not sure that you need to test u32.  After all you're not
testing tcg_exit_req, which is the equivalent when icount is disabled.

>          atomic_set(&cpu->exit_request, 0);
> -        cpu->exception_index = EXCP_INTERRUPT;
> +        /* If there is an exception that wasn't replayed yet,
> +           don't change exception_index. */
> +        if (cpu->exception_index == -1) {
> +            cpu->exception_index = EXCP_INTERRUPT;
> +        }
>          cpu_loop_exit(cpu);

The siglongjmp is effectively the same as exiting the for(;;) loop of
cpu_exec and going back to cpu_handle_exception.  So I would just merge
this with cpu_handle_interrupt, which exits a lot with cpu_loop_exit too.

All cpu_loop_exit() calls in cpu_handle_interrupt become "return true",
similar to cpu_handle_exception, and then in cpu_exec you have:

            /* if an exception is pending, we execute it here */
            while (!cpu_handle_exception(cpu, &ret)) {
                /* if an interrupt is pending, inject it and go back
                 * to cpu_handle_exception.
                 */
                while (!cpu_handle_interrupt(cpu, &last_tb)) {
                    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
                       if the guest is in advance */
                    align_clocks(&sc, cpu);
                }
            }
            break;



>  #endif
> @@ -634,6 +647,7 @@ int cpu_exec(CPUState *cpu)
>  
>              for(;;) {
>                  cpu_handle_interrupt(cpu, &last_tb);
> +                cpu_check_loop_exit(cpu);


	if (cpu_should_check_exception_or_exit(cpu)) {
	    break;
	}


>                  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 v8 3/9] apic: save apic_delivered flag

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
> > This patch implements saving/restoring of static apic_delivered variable.
> >
> > v8: saving static variable only for one of the APICs
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  hw/intc/apic_common.c           |   37 +++++++++++++++++++++++++++++++++++++
> >  include/hw/i386/apic_internal.h |    2 ++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> > index d78c885..edacb16 100644
> > --- a/hw/intc/apic_common.c
> > +++ b/hw/intc/apic_common.c
> > @@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
> >      return s->wait_for_sipi != 0;
> >  }
> >
> > +static bool apic_irq_delivered_needed(void *opaque)
> > +{
> > +    static APICCommonState *first_apic;
> > +    APICCommonState *s = APIC_COMMON(opaque);
> > +    if (!first_apic) {
> > +        first_apic = s;
> > +    }
> > +    return s == first_apic;
> 
> Should also check " && apic_irq_delivered != 0".

Reset of this variable when machine reboots is also forgotten.

Pavel Dovgalyuk

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

[Paolo Bonzini]

On 26/01/2017 13:34, 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
> 
> v8 changes:
>  - Refined replay exception processing (as suggested by Paolo Bonzini)
>  - Saving/restoring static variable for APIC only once (as suggested by Paolo Bonzini)
>  - Removed already queued patches
>  - Minor fixes
> 
> v7 changes:
>  - Fixed exception replaying when TB cache is full and
>    when tb_find is called when there are no instructions about to execute
>  - Added record/replay for audio devices
> 
> v6 changes:
>  - Added overlay creation for blkreplay driver
>  - Fixed vmstate loading for apic and rtc
>  - Fixed instruction counting for apic instruction patching
> 
> v5 changes:
>  - Recording is stopped when initial snapshot cannot be created
>  - Minor changes
> 
> v4 changes:
>  - Overlay option is removed from blkreplay driver (as suggested by Paolo Bonzini)
>  - Minor changes
> 
> v3 changes:
>  - Added rrsnapshot option for specifying the initial snapshot name (as suggested by Paolo Bonzini)
>  - Minor changes
> 
> ---
> 
> Pavel Dovgalyuk (9):
>       replay: exception replay fix
>       icount: exit cpu loop on expire
>       apic: save apic_delivered flag
>       integratorcp: adding vmstate for save/restore
>       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                      |   24 +++++++++--
>  docs/replay.txt                 |    7 +++
>  hw/arm/integratorcp.c           |   62 +++++++++++++++++++++++++++++
>  hw/intc/apic_common.c           |   37 +++++++++++++++++
>  include/hw/i386/apic_internal.h |    2 +
>  include/sysemu/replay.h         |    7 +++
>  replay/Makefile.objs            |    1 
>  replay/replay-audio.c           |   79 +++++++++++++++++++++++++++++++++++++
>  replay/replay-internal.h        |    4 ++
>  stubs/replay.c                  |    1 
>  translate-all.c                 |    3 +
>  vl.c                            |   10 ++++-
>  16 files changed, 361 insertions(+), 11 deletions(-)
>  create mode 100644 replay/replay-audio.c
> 

Queued patch 1 too.

Thanks for following my reviews.  Record/replay has the potential of
making the code either much better or much worse, and I know we can make
it better because it's what happened for icount.  But if it makes the
code worse, at some point it becomes a labyrinth and the only choice is
to remove RR support. :(  So let's avoid that!

Paolo
Paolo

Re: [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag

[Paolo Bonzini]

On 26/01/2017 14:03, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
>>> This patch implements saving/restoring of static apic_delivered variable.
>>>
>>> v8: saving static variable only for one of the APICs
>>>
>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>>> ---
>>>  hw/intc/apic_common.c           |   37 +++++++++++++++++++++++++++++++++++++
>>>  include/hw/i386/apic_internal.h |    2 ++
>>>  2 files changed, 39 insertions(+)
>>>
>>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>>> index d78c885..edacb16 100644
>>> --- a/hw/intc/apic_common.c
>>> +++ b/hw/intc/apic_common.c
>>> @@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
>>>      return s->wait_for_sipi != 0;
>>>  }
>>>
>>> +static bool apic_irq_delivered_needed(void *opaque)
>>> +{
>>> +    static APICCommonState *first_apic;
>>> +    APICCommonState *s = APIC_COMMON(opaque);
>>> +    if (!first_apic) {
>>> +        first_apic = s;
>>> +    }
>>> +    return s == first_apic;
>>
>> Should also check " && apic_irq_delivered != 0".
> 
> Reset of this variable when machine reboots is also forgotten.

Ok, I'll queue this patch with the condition changed, can you send one
for reset?

Paolo

Re: [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> On 26/01/2017 14:03, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
> >>> This patch implements saving/restoring of static apic_delivered variable.
> >>>
> >>> v8: saving static variable only for one of the APICs
> >>>
> >>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> >>> ---
> >>>  hw/intc/apic_common.c           |   37 +++++++++++++++++++++++++++++++++++++
> >>>  include/hw/i386/apic_internal.h |    2 ++
> >>>  2 files changed, 39 insertions(+)
> >>>
> >>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> >>> index d78c885..edacb16 100644
> >>> --- a/hw/intc/apic_common.c
> >>> +++ b/hw/intc/apic_common.c
> >>> @@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
> >>>      return s->wait_for_sipi != 0;
> >>>  }
> >>>
> >>> +static bool apic_irq_delivered_needed(void *opaque)
> >>> +{
> >>> +    static APICCommonState *first_apic;
> >>> +    APICCommonState *s = APIC_COMMON(opaque);
> >>> +    if (!first_apic) {
> >>> +        first_apic = s;
> >>> +    }
> >>> +    return s == first_apic;
> >>
> >> Should also check " && apic_irq_delivered != 0".
> >
> > Reset of this variable when machine reboots is also forgotten.
> 
> Ok, I'll queue this patch with the condition changed, can you send one
> for reset?

Ok, I'll send it with the next version of the series.


Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> On 26/01/2017 13:34, 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.
> >
> > v8: refactored loop exit code and moved it to separate function
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  cpu-exec.c |   24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index fa08c73..f9b8ec8 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -523,9 +523,25 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
> >              *last_tb = NULL;
> >          }
> >      }
> > -    if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) {
> > +}
> > +
> > +
> > +static void cpu_check_loop_exit(CPUState *cpu)
> > +{
> > +    if (unlikely(atomic_read(&cpu->exit_request)
> > +        /* 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. */
> > +        || (use_icount && ((cpu->icount_extra == 0
> > +                        && cpu->icount_decr.u16.low == 0)
> > +                    || (int32_t)cpu->icount_decr.u32 < 0)))) {
> 
> Simpler:
> 
> 	use_icount &&
> 	((int32_t)cpu->icount_decr.u32 < 0 ||
> 	 cpu->icount_decr.low + cpu->icount_extra == 0)

Right.

> But I'm not sure that you need to test u32.  After all you're not

Checking u32 is needed, because sometimes it is less than zero.
Consider the code in cpu_loop_exec_tb:
        int insns_left = cpu->icount_decr.u32;
            if (insns_left > 0) {
It is set to negative in tcg_handle_interrupt.
Ten days ago you also offered setting high bits of u32 in gen_icount:

+    /* Make the next TB exit immediately with TB_EXIT_ICOUNT_EXPIRED.  */
+    tcg_gen_st16_i32(-1, cpu_env,
+                     -ENV_OFFSET + offsetof(CPUState, icount_decr.u16.high));



> testing tcg_exit_req, which is the equivalent when icount is disabled.

Yes, if we want to refactor the whole loop.

> >          atomic_set(&cpu->exit_request, 0);
> > -        cpu->exception_index = EXCP_INTERRUPT;
> > +        /* If there is an exception that wasn't replayed yet,
> > +           don't change exception_index. */
> > +        if (cpu->exception_index == -1) {
> > +            cpu->exception_index = EXCP_INTERRUPT;
> > +        }
> >          cpu_loop_exit(cpu);
> 
> The siglongjmp is effectively the same as exiting the for(;;) loop of
> cpu_exec and going back to cpu_handle_exception.  So I would just merge
> this with cpu_handle_interrupt, which exits a lot with cpu_loop_exit too.

Do you want me to create new version of the patch, which changes the loop this way?

> All cpu_loop_exit() calls in cpu_handle_interrupt become "return true",
> similar to cpu_handle_exception, and then in cpu_exec you have:
> 
>             /* if an exception is pending, we execute it here */
>             while (!cpu_handle_exception(cpu, &ret)) {
>                 /* if an interrupt is pending, inject it and go back
>                  * to cpu_handle_exception.
>                  */
>                 while (!cpu_handle_interrupt(cpu, &last_tb)) {
>                     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
>                        if the guest is in advance */
>                     align_clocks(&sc, cpu);
>                 }
>             }
>             break;

It might even work faster without excessive cpu_loop_exit calls.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire

[Paolo Bonzini]

On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
>> Simpler:
>>
>> 	use_icount &&
>> 	((int32_t)cpu->icount_decr.u32 < 0 ||
>> 	 cpu->icount_decr.u16.low + cpu->icount_extra == 0)
> Right.
> 
>> But I'm not sure that you need to test u32.  After all you're not
> Checking u32 is needed, because sometimes it is less than zero.

If cpu->icount_decr.u32 is less than zero, the next translation block
would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing

            cpu->exception_index = EXCP_INTERRUPT;
            *last_tb = NULL;
            cpu_loop_exit(cpu);

from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".

And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
0, so I don't understand why this part of the patch is necessary.

Paolo

Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
> >> Simpler:
> >>
> >> 	use_icount &&
> >> 	((int32_t)cpu->icount_decr.u32 < 0 ||
> >> 	 cpu->icount_decr.u16.low + cpu->icount_extra == 0)
> > Right.
> >
> >> But I'm not sure that you need to test u32.  After all you're not
> > Checking u32 is needed, because sometimes it is less than zero.
> 
> If cpu->icount_decr.u32 is less than zero, the next translation block
> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
> 
>             cpu->exception_index = EXCP_INTERRUPT;
>             *last_tb = NULL;
>             cpu_loop_exit(cpu);
> 
> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
> 
> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
> 0, so I don't understand why this part of the patch is necessary.

I removed that lines because we have to check icount=0 not only when it is expired,
but also when all instructions were executed successfully. 
If there are no instructions to execute, calling tb_find (and translation then)
may cause an exception at the wrong moment.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire

[Paolo Bonzini]

On 26/01/2017 15:32, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
>>>> Simpler:
>>>>
>>>> 	use_icount &&
>>>> 	((int32_t)cpu->icount_decr.u32 < 0 ||
>>>> 	 cpu->icount_decr.u16.low + cpu->icount_extra == 0)
>>> Right.
>>>
>>>> But I'm not sure that you need to test u32.  After all you're not
>>> Checking u32 is needed, because sometimes it is less than zero.
>>
>> If cpu->icount_decr.u32 is less than zero, the next translation block
>> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
>>
>>             cpu->exception_index = EXCP_INTERRUPT;
>>             *last_tb = NULL;
>>             cpu_loop_exit(cpu);
>>
>> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
>>
>> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
>> 0, so I don't understand why this part of the patch is necessary.
> 
> I removed that lines because we have to check icount=0 not only when it is expired,
> but also when all instructions were executed successfully. 
> If there are no instructions to execute, calling tb_find (and translation then)
> may cause an exception at the wrong moment.

Ok, that makes sense for cpu->icount_decr.u16.low + cpu->icount_extra == 0.

But for decr.u32 < 0, the same reasoning of this comment is also true:

        /* Something asked us to stop executing
         * chained TBs; just continue round the main
         * loop. Whatever requested the exit will also
         * have set something else (eg exit_request or
         * interrupt_request) which we will handle
         * next time around the loop.  But we need to
         * ensure the tcg_exit_req read in generated code
         * comes before the next read of cpu->exit_request
         * or cpu->interrupt_request.
         */

(and by the way, this means that you need an smp_rmb in case
TB_EXIT_ICOUNT_EXPIRED).

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 26/01/2017 15:32, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
> >>>> Simpler:
> >>>>
> >>>> 	use_icount &&
> >>>> 	((int32_t)cpu->icount_decr.u32 < 0 ||
> >>>> 	 cpu->icount_decr.u16.low + cpu->icount_extra == 0)
> >>> Right.
> >>>
> >>>> But I'm not sure that you need to test u32.  After all you're not
> >>> Checking u32 is needed, because sometimes it is less than zero.
> >>
> >> If cpu->icount_decr.u32 is less than zero, the next translation block
> >> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
> >>
> >>             cpu->exception_index = EXCP_INTERRUPT;
> >>             *last_tb = NULL;	
> >>             cpu_loop_exit(cpu);
> >>
> >> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
> >>
> >> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
> >> 0, so I don't understand why this part of the patch is necessary.
> >
> > I removed that lines because we have to check icount=0 not only when it is expired,
> > but also when all instructions were executed successfully.
> > If there are no instructions to execute, calling tb_find (and translation then)
> > may cause an exception at the wrong moment.
> 
> Ok, that makes sense for cpu->icount_decr.u16.low + cpu->icount_extra == 0.
> 
> But for decr.u32 < 0, the same reasoning of this comment is also true:
> 
>         /* Something asked us to stop executing
>          * chained TBs; just continue round the main
>          * loop. Whatever requested the exit will also
>          * have set something else (eg exit_request or
>          * interrupt_request) which we will handle
>          * next time around the loop.  But we need to
>          * ensure the tcg_exit_req read in generated code
>          * comes before the next read of cpu->exit_request
>          * or cpu->interrupt_request.
>          */

Right. If the following lines will not be removed (as opposite to my patch) then checking
decr.u32 < 0 will not be needed.
-             cpu->exception_index = EXCP_INTERRUPT;
-             *last_tb = NULL;	
-             cpu_loop_exit(cpu);

What is your point about the new version of that patch?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire

[Paolo Bonzini]

On 27/01/2017 07:09, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 26/01/2017 15:32, Pavel Dovgalyuk wrote:
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
>>>>>> Simpler:
>>>>>>
>>>>>> 	use_icount &&
>>>>>> 	((int32_t)cpu->icount_decr.u32 < 0 ||
>>>>>> 	 cpu->icount_decr.u16.low + cpu->icount_extra == 0)
>>>>> Right.
>>>>>
>>>>>> But I'm not sure that you need to test u32.  After all you're not
>>>>> Checking u32 is needed, because sometimes it is less than zero.
>>>>
>>>> If cpu->icount_decr.u32 is less than zero, the next translation block
>>>> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
>>>>
>>>>             cpu->exception_index = EXCP_INTERRUPT;
>>>>             *last_tb = NULL;	
>>>>             cpu_loop_exit(cpu);
>>>>
>>>> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
>>>>
>>>> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
>>>> 0, so I don't understand why this part of the patch is necessary.
>>>
>>> I removed that lines because we have to check icount=0 not only when it is expired,
>>> but also when all instructions were executed successfully.
>>> If there are no instructions to execute, calling tb_find (and translation then)
>>> may cause an exception at the wrong moment.
>>
>> Ok, that makes sense for cpu->icount_decr.u16.low + cpu->icount_extra == 0.
>>
>> But for decr.u32 < 0, the same reasoning of this comment is also true:
>>
>>         /* Something asked us to stop executing
>>          * chained TBs; just continue round the main
>>          * loop. Whatever requested the exit will also
>>          * have set something else (eg exit_request or
>>          * interrupt_request) which we will handle
>>          * next time around the loop.  But we need to
>>          * ensure the tcg_exit_req read in generated code
>>          * comes before the next read of cpu->exit_request
>>          * or cpu->interrupt_request.
>>          */
> 
> Right. If the following lines will not be removed (as opposite to my patch) then checking
> decr.u32 < 0 will not be needed.

That's what I'm not sure about.  u32 < 0 is only true if you have set
the interrupt_request as well, but interrupt requests are processed in
cpu_handle_interrupt and that doesn't require going back to the main loop.

Let me try some cleanups early next week and come back to you with a
patch to base your work on.

Paolo

> -             cpu->exception_index = EXCP_INTERRUPT;
> -             *last_tb = NULL;	
> -             cpu_loop_exit(cpu);
> 
> What is your point about the new version of that patch?
> 
> Pavel Dovgalyuk
> 
> 
>