[RFC,v8,19/21] replay: initialization and deinitialization

This patch introduces the functions for enabling the record/replay and for
freeing the resources when simulator closes.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 block.c                  |    2 -
 exec.c                   |    1 
 replay/replay-internal.c |    1 
 replay/replay-internal.h |    4 +
 replay/replay.c          |  140 ++++++++++++++++++++++++++++++++++++++++++++++
 replay/replay.h          |   13 ++++
 stubs/replay.c           |    1 
 vl.c                     |    5 ++
 8 files changed, 165 insertions(+), 2 deletions(-)

On 22/01/2015 09:53, Pavel Dovgalyuk wrote:
> This patch introduces the functions for enabling the record/replay and for
> freeing the resources when simulator closes.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  block.c                  |    2 -
>  exec.c                   |    1 
>  replay/replay-internal.c |    1 
>  replay/replay-internal.h |    4 +
>  replay/replay.c          |  140 ++++++++++++++++++++++++++++++++++++++++++++++
>  replay/replay.h          |   13 ++++
>  stubs/replay.c           |    1 
>  vl.c                     |    5 ++
>  8 files changed, 165 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 7f6fa8b..9923a80 100644
> --- a/block.c
> +++ b/block.c
> @@ -2010,7 +2010,7 @@ void bdrv_drain_all(void)
>              aio_context_release(aio_context);
>          }
>      }
> -    if (replay_mode == REPLAY_MODE_PLAY) {
> +    if (replay_mode == REPLAY_MODE_PLAY && replay_checkpoints) {
>          /* Skip checkpoints from the log */
>          while (replay_checkpoint(CHECKPOINT_BDRV_DRAIN)) {
>              /* Nothing */
> diff --git a/exec.c b/exec.c
> index 410371d..f70f03d 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -783,6 +783,7 @@ void cpu_abort(CPUState *cpu, const char *fmt, ...)
>      }
>      va_end(ap2);
>      va_end(ap);
> +    replay_finish();
>  #if defined(CONFIG_USER_ONLY)
>      {
>          struct sigaction act;
> diff --git a/replay/replay-internal.c b/replay/replay-internal.c
> index 49b37a6..d608b71 100755
> --- a/replay/replay-internal.c
> +++ b/replay/replay-internal.c
> @@ -33,6 +33,7 @@ void replay_put_byte(uint8_t byte)
>  
>  void replay_put_event(uint8_t event)
>  {
> +    assert(event <= EVENT_END);

Please move (way) earlier in the series.

>      replay_put_byte(event);
>  }
>  
> diff --git a/replay/replay-internal.h b/replay/replay-internal.h
> index 4d242aa..1e5d037 100755
> --- a/replay/replay-internal.h
> +++ b/replay/replay-internal.h
> @@ -34,7 +34,9 @@ enum ReplayEvents {
>      EVENT_CLOCK,
>      /* for checkpoint event */
>      /* some of grteater codes are reserved for checkpoints */
> -    EVENT_CHECKPOINT = EVENT_CLOCK + REPLAY_CLOCK_COUNT
> +    EVENT_CHECKPOINT = EVENT_CLOCK + REPLAY_CLOCK_COUNT,
> +    /* end of log event */
> +    EVENT_END = EVENT_CHECKPOINT + CHECKPOINT_COUNT

Please add EVENT_END in the beginning of the series.  If you do:

    EVENT_CLOCK,
    EVENT_CLOCK_LAST = EVENT_CLOCK + REPLAY_CLOCK_COUNT - 1,
    EVENT_CHECKPOINT,
    EVENT_CHECKPOINT_LAST = EVENT_CHECKPOINT + CHECKPOINT_COUNT - 1,
    EVENT_END,

the patches look quite nice.

>  };
>  
>  /* Asynchronous events IDs */
> diff --git a/replay/replay.c b/replay/replay.c
> index 7c4a801..fa738c2 100755
> --- a/replay/replay.c
> +++ b/replay/replay.c
> @@ -15,9 +15,18 @@
>  #include "qemu/timer.h"
>  #include "sysemu/sysemu.h"
>  
> +/* Current version of the replay mechanism.
> +   Increase it when file format changes. */
> +#define REPLAY_VERSION              0xe02002

Any meaning?  Perhaps make it 'QRR\0' or something identifiable?

> +/* Size of replay log header */
> +#define HEADER_SIZE                 (sizeof(uint32_t) + sizeof(uint64_t))
> +
>  ReplayMode replay_mode = REPLAY_MODE_NONE;
>  
> +/* Name of replay file  */
> +static char *replay_filename;
>  ReplayState replay_state;
> +bool replay_checkpoints;

Why is replay_checkpoints only defined and used in this patch?  Is it a
duplicate of replay_events_enabled?

>  bool skip_async_events(int stop_event)
>  {
> @@ -167,6 +176,10 @@ void replay_shutdown_request(void)
>  bool replay_checkpoint(ReplayCheckpoint checkpoint)
>  {
>      bool res = false;
> +    if (!replay_checkpoints) {
> +        return true;
> +    }
> +
>      replay_save_instructions();
>  
>      if (replay_file) {
> @@ -199,3 +212,130 @@ bool replay_checkpoint(ReplayCheckpoint checkpoint)
>  
>      return true;
>  }
> +
> +static void replay_enable(const char *fname, int mode)
> +{
> +    const char *fmode = NULL;
> +    if (replay_file) {
> +        fprintf(stderr,
> +                "Replay: some record/replay operation is already started\n");

Would this have to be an assertion?

> +        return;
> +    }
> +
> +    switch (mode) {
> +    case REPLAY_MODE_RECORD:
> +        fmode = "wb";
> +        break;
> +    case REPLAY_MODE_PLAY:
> +        fmode = "rb";
> +        break;
> +    default:
> +        fprintf(stderr, "Replay: internal error: invalid replay mode\n");
> +        exit(1);
> +    }
> +
> +    atexit(replay_finish);
> +
> +    replay_mutex_init();
> +
> +    replay_file = fopen(fname, fmode);
> +    if (replay_file == NULL) {
> +        fprintf(stderr, "Replay: open %s: %s\n", fname, strerror(errno));
> +        exit(1);
> +    }
> +
> +    replay_filename = g_strdup(fname);
> +
> +    replay_mode = mode;
> +    replay_has_unread_data = 0;
> +    replay_data_kind = -1;
> +    replay_state.instructions_count = 0;
> +    replay_state.current_step = 0;
> +
> +    /* skip file header for RECORD and check it for PLAY */
> +    if (replay_mode == REPLAY_MODE_RECORD) {
> +        fseek(replay_file, HEADER_SIZE, SEEK_SET);

Why can't you write the header here?

> +    } else if (replay_mode == REPLAY_MODE_PLAY) {
> +        unsigned int version = replay_get_dword();
> +        uint64_t offset = replay_get_qword();
> +        if (version != REPLAY_VERSION) {
> +            fprintf(stderr, "Replay: invalid input log file version\n");
> +            exit(1);
> +        }
> +        /* go to the beginning */
> +        fseek(replay_file, 12, SEEK_SET);
> +    }
> +
> +    replay_init_events();
> +}
> +
> +void replay_configure(QemuOpts *opts)
> +{
> +    const char *fname;
> +    const char *rr;
> +    ReplayMode mode = REPLAY_MODE_NONE;
> +
> +    rr = qemu_opt_get(opts, "rr");
> +    if (!rr) {
> +        /* Just enabling icount */
> +        return;
> +    } else if (!strcmp(rr, "record")) {
> +        mode = REPLAY_MODE_RECORD;
> +    } else if (!strcmp(rr, "replay")) {
> +        mode = REPLAY_MODE_PLAY;
> +    } else {
> +        fprintf(stderr, "Invalid icount rr option: %s\n", rr);
> +        exit(1);
> +    }
> +
> +    fname = qemu_opt_get(opts, "rrfname");
> +    if (!fname) {
> +        fprintf(stderr, "File name not specified for replay\n");
> +        exit(1);
> +    }
> +
> +    replay_enable(fname, mode);
> +}
> +
> +void replay_init_timer(void)
> +{
> +    if (replay_mode == REPLAY_MODE_NONE) {
> +        return;
> +    }
> +
> +    replay_checkpoints = true;
> +    replay_enable_events();

Move if to replay_enable_events() and drop replay_init_timer() altogether?

> +}
> +
> +void replay_finish(void)
> +{
> +    if (replay_mode == REPLAY_MODE_NONE) {
> +        return;
> +    }
> +
> +    replay_save_instructions();
> +
> +    /* finalize the file */
> +    if (replay_file) {
> +        if (replay_mode == REPLAY_MODE_RECORD) {
> +            uint64_t offset = 0;
> +            /* write end event */
> +            replay_put_event(EVENT_END);
> +
> +            /* write header */
> +            fseek(replay_file, 0, SEEK_SET);
> +            replay_put_dword(REPLAY_VERSION);
> +            replay_put_qword(offset);

Why is this done here?  You're writing a constant zero.

> +        }
> +
> +        fclose(replay_file);
> +        replay_file = NULL;
> +    }
> +    if (replay_filename) {
> +        g_free(replay_filename);
> +        replay_filename = NULL;
> +    }
> +
> +    replay_mutex_destroy();
> +    replay_finish_events();
> +}
> diff --git a/replay/replay.h b/replay/replay.h
> index 3110d46..231b8ec 100755
> --- a/replay/replay.h
> +++ b/replay/replay.h
> @@ -17,6 +17,8 @@
>  #include <time.h>
>  #include "qapi-types.h"
>  
> +struct QemuOpts;

#include instead?

>  /* replay clock kinds */
>  enum ReplayClockKind {
>      /* rdtsc */
> @@ -45,6 +47,17 @@ enum ReplayCheckpoint {
>  typedef enum ReplayCheckpoint ReplayCheckpoint;
>  
>  extern ReplayMode replay_mode;
> +/*! Is set to true after finishing initialization */
> +extern bool replay_checkpoints;
> +
> +/* Replay process control functions */
> +
> +/*! Enables recording or saving event log with specified parameters */
> +void replay_configure(struct QemuOpts *opts);
> +/*! Initializes timers used for snapshotting and enables events recording */
> +void replay_init_timer(void);
> +/*! Closes replay log file and frees other resources. */
> +void replay_finish(void);
>  
>  /* Processing the instructions */
>  
> diff --git a/stubs/replay.c b/stubs/replay.c
> index 81eddae..7d87964 100755
> --- a/stubs/replay.c
> +++ b/stubs/replay.c
> @@ -2,6 +2,7 @@
>  #include "sysemu/sysemu.h"
>  
>  ReplayMode replay_mode;
> +bool replay_checkpoints;
>  
>  int64_t replay_save_clock(unsigned int kind, int64_t clock)
>  {
> diff --git a/vl.c b/vl.c
> index 86ba385..ae3e97e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4376,7 +4376,12 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> +    replay_init_timer();

replay_enable_events()

>      main_loop();
> +    if (replay_mode != REPLAY_MODE_NONE) {
> +        replay_disable_events();

Move "if" to replay_disable_events.

Paolo

> +    }
>      bdrv_close_all();
>      pause_all_vcpus();
>      res_free();
>

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 22/01/2015 09:53, Pavel Dovgalyuk wrote:
> > This patch introduces the functions for enabling the record/replay and for
> > freeing the resources when simulator closes.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> 
> >  };
> >
> >  /* Asynchronous events IDs */
> > diff --git a/replay/replay.c b/replay/replay.c
> > index 7c4a801..fa738c2 100755
> > --- a/replay/replay.c
> > +++ b/replay/replay.c
> > @@ -15,9 +15,18 @@
> >  #include "qemu/timer.h"
> >  #include "sysemu/sysemu.h"
> >
> > +/* Current version of the replay mechanism.
> > +   Increase it when file format changes. */
> > +#define REPLAY_VERSION              0xe02002
> 
> Any meaning?  Perhaps make it 'QRR\0' or something identifiable?

Version id has no special meaning, but has to be distinct from other RR versions.
When format of the log file changes we have to update version id to prevent
reading incompatible files.

> > +    replay_file = fopen(fname, fmode);
> > +    if (replay_file == NULL) {
> > +        fprintf(stderr, "Replay: open %s: %s\n", fname, strerror(errno));
> > +        exit(1);
> > +    }
> > +
> > +    replay_filename = g_strdup(fname);
> > +
> > +    replay_mode = mode;
> > +    replay_has_unread_data = 0;
> > +    replay_data_kind = -1;
> > +    replay_state.instructions_count = 0;
> > +    replay_state.current_step = 0;
> > +
> > +    /* skip file header for RECORD and check it for PLAY */
> > +    if (replay_mode == REPLAY_MODE_RECORD) {
> > +        fseek(replay_file, HEADER_SIZE, SEEK_SET);
> 
> Why can't you write the header here?

We don't write header here to detect broken log files at the replay stage.

> > +    replay_save_instructions();
> > +
> > +    /* finalize the file */
> > +    if (replay_file) {
> > +        if (replay_mode == REPLAY_MODE_RECORD) {
> > +            uint64_t offset = 0;
> > +            /* write end event */
> > +            replay_put_event(EVENT_END);
> > +
> > +            /* write header */
> > +            fseek(replay_file, 0, SEEK_SET);
> > +            replay_put_dword(REPLAY_VERSION);
> > +            replay_put_qword(offset);
> 
> Why is this done here?  You're writing a constant zero.

Right, this offset is the field reserved for snapshots table.
It will be introduced in other patches.

Pavel Dovgalyuk

On 09/02/2015 13:59, Pavel Dovgaluk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 22/01/2015 09:53, Pavel Dovgalyuk wrote:
>>> This patch introduces the functions for enabling the record/replay and for
>>> freeing the resources when simulator closes.
>>>
>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>>
>>>  };
>>>
>>>  /* Asynchronous events IDs */
>>> diff --git a/replay/replay.c b/replay/replay.c
>>> index 7c4a801..fa738c2 100755
>>> --- a/replay/replay.c
>>> +++ b/replay/replay.c
>>> @@ -15,9 +15,18 @@
>>>  #include "qemu/timer.h"
>>>  #include "sysemu/sysemu.h"
>>>
>>> +/* Current version of the replay mechanism.
>>> +   Increase it when file format changes. */
>>> +#define REPLAY_VERSION              0xe02002
>>
>> Any meaning?  Perhaps make it 'QRR\0' or something identifiable?
> 
> Version id has no special meaning, but has to be distinct from other RR versions.
> When format of the log file changes we have to update version id to prevent
> reading incompatible files.
> 
>>> +    replay_file = fopen(fname, fmode);
>>> +    if (replay_file == NULL) {
>>> +        fprintf(stderr, "Replay: open %s: %s\n", fname, strerror(errno));
>>> +        exit(1);
>>> +    }
>>> +
>>> +    replay_filename = g_strdup(fname);
>>> +
>>> +    replay_mode = mode;
>>> +    replay_has_unread_data = 0;
>>> +    replay_data_kind = -1;
>>> +    replay_state.instructions_count = 0;
>>> +    replay_state.current_step = 0;
>>> +
>>> +    /* skip file header for RECORD and check it for PLAY */
>>> +    if (replay_mode == REPLAY_MODE_RECORD) {
>>> +        fseek(replay_file, HEADER_SIZE, SEEK_SET);
>>
>> Why can't you write the header here?
> 
> We don't write header here to detect broken log files at the replay stage.

I guess this is also a provision for something that you'll do in the future.

>>> +    replay_save_instructions();
>>> +
>>> +    /* finalize the file */
>>> +    if (replay_file) {
>>> +        if (replay_mode == REPLAY_MODE_RECORD) {
>>> +            uint64_t offset = 0;
>>> +            /* write end event */
>>> +            replay_put_event(EVENT_END);
>>> +
>>> +            /* write header */
>>> +            fseek(replay_file, 0, SEEK_SET);
>>> +            replay_put_dword(REPLAY_VERSION);
>>> +            replay_put_qword(offset);
>>
>> Why is this done here?  You're writing a constant zero.
> 
> Right, this offset is the field reserved for snapshots table.
> It will be introduced in other patches.

Okay, then please add comments.

Paolo