From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33141) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YH9La-00059I-Ep for qemu-devel@nongnu.org; Fri, 30 Jan 2015 06:02:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YH9LV-0006W8-6m for qemu-devel@nongnu.org; Fri, 30 Jan 2015 06:02:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39545) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YH9LU-0006Vw-V4 for qemu-devel@nongnu.org; Fri, 30 Jan 2015 06:02:41 -0500 Message-ID: <54CB64C1.6080102@redhat.com> Date: Fri, 30 Jan 2015 12:02:25 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <20150122085127.5276.53895.stgit@PASHA-ISP.def.inno> <20150122085317.5276.57371.stgit@PASHA-ISP.def.inno> In-Reply-To: <20150122085317.5276.57371.stgit@PASHA-ISP.def.inno> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC PATCH v8 19/21] replay: initialization and deinitialization List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Dovgalyuk , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, peter.crosthwaite@xilinx.com, alex.bennee@linaro.org, mark.burton@greensocs.com, real@ispras.ru, batuzovk@ispras.ru, maria.klimushenkova@ispras.ru, afaerber@suse.de, fred.konrad@greensocs.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 > --- > 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 > #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(); >