replay additions

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 <[email protected]>

---
 cpu-exec.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

cpu-exec.c +3 -1

index fa08c73..f9b8ec8 100644
@@ -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())) {
+}
+
+

cpu-exec.c +0 -0

                 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

From: Paolo Bonzini [mailto:[email protected]] On Behalf Of Paolo Bonzini

Quoted 25 lines · 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 <[email protected]>

---
 cpu-exec.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

cpu-exec.c +3 -1

index fa08c73..f9b8ec8 100644
@@ -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())) {
+}
+
+

cpu-exec.c static inline void cpu_handle_interrupt(CPUState *cpu, · line 538

Quoted 10 lines

cpu-exec.c +10 -0

+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.

cpu-exec.c static inline void cpu_handle_interrupt(CPUState *cpu, · line 545

Quoted 8 lines

cpu-exec.c +5 -1

         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;

Quoted 5 lines

Simpler:

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

Right.

Quoted 1 line

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.

From: Paolo Bonzini [mailto:[email protected]]

Quoted 9 lines · Pavel Dovgalyuk

Quoted 5 lines

Simpler:

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

Right.

Quoted 1 line

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.

Quoted 21 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 9 lines · Pavel Dovgalyuk

Quoted 5 lines

Simpler:

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

Right.

Quoted 1 line

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".

Quoted 2 lines

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.

From: Paolo Bonzini [mailto:[email protected]]

Quoted 21 lines · Pavel Dovgalyuk

Quoted 21 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 9 lines · Pavel Dovgalyuk

Quoted 5 lines

Simpler:

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

Right.

Quoted 1 line

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".

Quoted 7 lines

Quoted 2 lines

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.
*/

Quoted 35 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 21 lines · Pavel Dovgalyuk

Quoted 21 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 9 lines · Pavel Dovgalyuk

Quoted 5 lines

Simpler:

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

Right.

Quoted 1 line

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".

Quoted 7 lines

Quoted 2 lines

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:

Quoted 10 lines

/* 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.

(quoted diff) +0 -3

-             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

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 <[email protected]>

---
 hw/intc/apic_common.c           |   37 +++++++++++++++++++++++++++++++++++++
 include/hw/i386/apic_internal.h |    2 ++
 2 files changed, 39 insertions(+)

hw/intc/apic_common.c +0 -0

index d78c885..edacb16 100644
@@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
     return s->wait_for_sipi != 0;

hw/intc/apic_common.c +29 -0

+}
+
+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
     }
 };

include/hw/i386/apic_internal.h +2 -0

index 1209eb4..20ad28c 100644
@@ -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 {

From: Paolo Bonzini [mailto:[email protected]]

Quoted 18 lines · 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 <[email protected]>

---
 hw/intc/apic_common.c           |   37 +++++++++++++++++++++++++++++++++++++
 include/hw/i386/apic_internal.h |    2 ++
 2 files changed, 39 insertions(+)

hw/intc/apic_common.c +0 -0

index d78c885..edacb16 100644
@@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
     return s->wait_for_sipi != 0;
 }
 

hw/intc/apic_common.c static bool apic_common_sipi_needed(void *opaque) · line 394

Quoted 8 lines

hw/intc/apic_common.c +8 -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".

Quoted 20 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 18 lines · 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 <[email protected]>

---
 hw/intc/apic_common.c           |   37 +++++++++++++++++++++++++++++++++++++
 include/hw/i386/apic_internal.h |    2 ++
 2 files changed, 39 insertions(+)

hw/intc/apic_common.c +0 -0

index d78c885..edacb16 100644
@@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
     return s->wait_for_sipi != 0;
 }
 

Quoted 10 lines

hw/intc/apic_common.c static bool apic_common_sipi_needed(void *opaque) · line 394

Quoted 8 lines

hw/intc/apic_common.c +8 -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.

From: Paolo Bonzini [mailto:[email protected]] On Behalf Of Paolo Bonzini

Quoted 31 lines · Pavel Dovgalyuk

Quoted 30 lines

From: Paolo Bonzini [mailto:[email protected]]

Quoted 16 lines · 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 <[email protected]>

---
 hw/intc/apic_common.c           |   37 +++++++++++++++++++++++++++++++++++++
 include/hw/i386/apic_internal.h |    2 ++
 2 files changed, 39 insertions(+)

hw/intc/apic_common.c +0 -0

index d78c885..edacb16 100644
@@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
     return s->wait_for_sipi != 0;

hw/intc/apic_common.c static bool apic_common_sipi_needed(void *opaque) · line 394

Quoted 10 lines

hw/intc/apic_common.c +8 -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".

Quoted 1 line

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?

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