On 15/08/2015 12:03, Paolo Bonzini wrote:
>
>
> On 15/08/2015 11:57, Pavel Dovgalyuk wrote:
>> Hi, Paolo!
>>
>> Will you apply these patches to 2.5?
>
> Yes, I'll put them in my next pull request.
Hi Pavel,
unfortunately I do have some more review comments; that can happen when
going back to the code after a few months, and it's also a good thing
because it means that the code _is_ actually getting cleaner.
However, I am fairly sure that v17 is going to be the good one and will
be in 2.5 if I get it by mid September when I'll be back from vacation.
In particular:
* patch 3 seems to be unnecessary (for now at least)
* replay_next_event_is is modified in patch 8 ("cpu: replay instructions
sequence") after it's introduced in patch 6 ("replay: introduce icount
event"). Please fold the change in patch 6.
* replay_add_event is not used; please remove it, rename
replay_add_event_internal to replay_add_event, and add an assertion that
the rr mode is the right one (e.g. RECORD only?)
* a couple of comments say "grteater" instead of "greater"
* the replay_save_clock and replay_read_clock stubs should abort
* please inline replay_input_event into qemu_input_event_send and
replay_input_sync_event into qemu_input_event_sync, so that the
corresponding *_impl functions can be static; this also means moving the
prototypes of replay_add_input_event and replay_add_input_sync_event to
replay/replay.h (I acknowledge this might undo a request from a previous
review of mine; I don't remember)
* most stubs are unnecessary (replay_get_current_step,
replay_checkpoint, qemu_system_shutdown_request,
qemu_input_event_send_impl, qemu_input_event_sync_impl)
* please squash this in "replay: checkpoints"
diff --git a/vl.c b/vl.c
index 5a509dc..3c69563 100644
--- a/vl.c
+++ b/vl.c
@@ -1662,8 +1662,11 @@ static void qemu_kill_report(void)
static int qemu_reset_requested(void)
{
int r = reset_requested;
- reset_requested = 0;
- return r;
+ if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) {
+ reset_requested = 0;
+ return r;
+ }
+ return false;
}
static int qemu_suspend_requested(void)
@@ -1862,9 +1865,7 @@ static bool main_loop_should_exit(void)
return true;
}
}
- if (qemu_reset_requested_get()
- && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) {
- qemu_reset_requested();
+ if (qemu_reset_requested()) {
pause_all_vcpus();
cpu_synchronize_all_states();
qemu_system_reset(VMRESET_REPORT);
And a few questions. The first three are the "if the answer is yes,
please do this" kind to questions, the others can have more
open/subjective answers:
* does it make sense to call replay_check_error from
replay_finish_event, and remove the call from replay_read_next_clock?
* should qemu_clock_use_for_deadline always return false in replay mode?
The clocks are all deterministic, so it doesn't make sense to take them
into account in the poll() deadline.
* now that qemu_clock_warp has to be called in main_loop_wait, should
the icount_warp_timer have a dummy callback? icount_warp_rt is then
only called from qemu_clock_warp. If so, this (adding the call to
qemu_clock_warp in main_loop_wait, making the icount_warp_timer dummy,
removing the now-unnecessary argument of icount_warp_rt) should be a
separate patch before "replay: checkpoints"
* can you explain why both CHECKPOINT_INIT and CHECKPOINT_RESET are
needed? What events are typically found in each of them?
* would it make sense to test "replay_mode != REPLAY_MODE_NONE &&
!runstate_is_running()" in replay_checkpoint, for all checkpoints, like
if (replay_mode != REPLAY_MODE_NONE && !runstate_is_running()) {
return false;
}
?
* do we need an event for suspend?
Thanks,
Paolo
