Re: [Qemu-devel] [RFC PATCH v8 08/21] cpu: replay instructions sequence

[Paolo Bonzini]

On 22/01/2015 09:52, Pavel Dovgalyuk wrote:
> This patch adds calls to replay functions into the icount setup block.
> In record mode number of executed instructions is written to the log.
> In replay mode number of istructions to execute is taken from the replay log.
> 
> Signed-off-by: Pavel Dovgalyuk <address@hidden>
> ---
>  cpu-exec.c      |    1 +
>  cpus.c          |   28 ++++++++++++++++++----------
>  replay/replay.c |   24 ++++++++++++++++++++++++
>  replay/replay.h |    4 ++++
>  4 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 49f01f5..99a0993 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -531,6 +531,7 @@ int cpu_exec(CPUArchState *env)
>                              }
>                              cpu->exception_index = EXCP_INTERRUPT;
>                              next_tb = 0;
> +                            qemu_notify_event();

Why is this needed?

>                              cpu_loop_exit(cpu);
>                          }
>                          break;
> diff --git a/cpus.c b/cpus.c
> index c513275..8787277 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -41,6 +41,7 @@
>  #include "qemu/seqlock.h"
>  #include "qapi-event.h"
>  #include "hw/nmi.h"
> +#include "replay/replay.h"
>  
>  #ifndef _WIN32
>  #include "qemu/compatfd.h"
> @@ -1342,18 +1343,22 @@ static int tcg_cpu_exec(CPUArchState *env)
>                                      + cpu->icount_extra);
>          cpu->icount_decr.u16.low = 0;
>          cpu->icount_extra = 0;
> -        deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
> +        if (replay_mode != REPLAY_MODE_PLAY) {
> +            deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
>  
> -        /* Maintain prior (possibly buggy) behaviour where if no deadline
> -         * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more 
> than
> -         * INT32_MAX nanoseconds ahead, we still use INT32_MAX
> -         * nanoseconds.
> -         */
> -        if ((deadline < 0) || (deadline > INT32_MAX)) {
> -            deadline = INT32_MAX;
> -        }
> +            /* Maintain prior (possibly buggy) behaviour where if no deadline
> +             * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is 
> more than
> +             * INT32_MAX nanoseconds ahead, we still use INT32_MAX
> +             * nanoseconds.
> +             */
> +            if ((deadline < 0) || (deadline > INT32_MAX)) {
> +                deadline = INT32_MAX;
> +            }
>  
> -        count = qemu_icount_round(deadline);
> +            count = qemu_icount_round(deadline);
> +        } else {
> +            count = replay_get_instructions();
> +        }

Please extract the "if" to a separate function tcg_get_icount_limit().

>          timers_state.qemu_icount += count;
>          decr = (count > 0xffff) ? 0xffff : count;
>          count -= decr;
> @@ -1371,6 +1376,9 @@ static int tcg_cpu_exec(CPUArchState *env)
>                          + cpu->icount_extra);
>          cpu->icount_decr.u32 = 0;
>          cpu->icount_extra = 0;
> +        if (replay_mode == REPLAY_MODE_PLAY) {
> +            replay_exec_instructions();

replay_account_executed_instructions()

> +        }
>      }
>      return ret;
>  }
> diff --git a/replay/replay.c b/replay/replay.c
> index a43bbbc..d6f5c4b 100755
> --- a/replay/replay.c
> +++ b/replay/replay.c
> @@ -58,3 +58,27 @@ uint64_t replay_get_current_step(void)
>  {
>      return cpu_get_icount_raw();
>  }
> +
> +int replay_get_instructions(void)
> +{
> +    int res = 0;
> +    replay_mutex_lock();
> +    if (skip_async_events(EVENT_INSTRUCTION)) {
> +        res = replay_state.instructions_count;
> +    }
> +    replay_mutex_unlock();
> +    return res;
> +}
> +
> +void replay_exec_instructions(void)
> +{
> +    if (replay_state.instructions_count > 0) {
> +        int count = (int)(replay_get_current_step()
> +                          - replay_state.current_step);
> +        replay_state.instructions_count -= count;
> +        replay_state.current_step += count;
> +        if (replay_state.instructions_count == 0 && count != 0) {

If replay_state.instructions_count is now zero, count must be nonzero
(because replay_state.instructions_count was > 0) before.

> +            replay_has_unread_data = 0;
> +        }

Can replay_state.instructions_count be < count at all?  If not, and if
replay_state.instructions_count is zero, then count must also be zero.

If so, I suggest rewriting as

        int count = (int)(replay_get_current_step()
                          - replay_state.current_step);
        assert(replay_state.instructions_count >= count);
        replay_state.instructions_count -= count;
        replay_state.current_step += count;
        if (replay_state.instructions_count == 0) {
            replay_has_unread_data = 0;
        }

Paolo

> +    }
> +}
> diff --git a/replay/replay.h b/replay/replay.h
> index a03c748..e425dea 100755
> --- a/replay/replay.h
> +++ b/replay/replay.h
> @@ -22,5 +22,9 @@ extern ReplayMode replay_mode;
>  
>  /*! Returns number of executed instructions. */
>  uint64_t replay_get_current_step(void);
> +/*! Returns number of instructions to execute in replay mode. */
> +int replay_get_instructions(void);
> +/*! Updates instructions counter in replay mode. */
> +void replay_exec_instructions(void);
>  
>  #endif
>