Il 30/07/2014 09:44, Pavel Dovgaluk ha scritto:
From: Paolo Bonzini [mailto:address@hidden On Behalf Of Paolo Bonzini
- patch 16 should also use subsections, and perhaps apply to all other
CPUs too?
We implemented replay only for i386 and ARM. If we'll change other targets,
it will not
add record/replay capabilities to them, but can confuse the reviewers.
Why are these fields only required by record/replay?
No, these fields can cause non-determinism for normal execution mode too.
But these changes are not related to reverse execution. I can make patches for
them
and submit them separately, if needed.
Yes, thanks. If it causes nondeterminism for normal execution mode too,
it's better to apply them to all targets. If possible use a subsection,
though.
- patch 27 makes sense but VIRTUAL is used to skip blinking when the VM
is stopped
Right, this is kind of hack. I haven't found better solution yet.
I think it's okay to use REALTIME, just add runstate_is_running() to the
"if (now >= s->cursor_blink_time)".
That said, "every read to virtual clock is written to the
replay log" worries me a bit from the point of view of thread-safety.
Do you need to log all reads because you don't use icount? Reads can
only happen at given points if you use icount, and you could simply log
the icount value at each synchronization point.
We made two implementations of virtual clock. One uses host realtime clock
and thus should be written into the log. Another one uses icount and calculated
in one of the replay modules (similar to regular icount implementation).
This patch series contains only first implementation, because it looks more
convenient:
user does not have to specify icount shift value, when starting the recording
or replaying.
I think from the upstreaming point of view it's better to stick with
what makes the code simpler. Start by submitting only the icount-based
implementation, the other can come later.
Paolo