On 30/05/14 08:57, Paolo Bonzini wrote: > Il 29/05/2014 22:44, James Hogan ha scritto: >> Yes, I agree with your analysis and had considered something like this, >> although it doesn't particularly appeal to my sense of perfectionism :). > > I can see that. But I think the simplification of the code is worth it. > > It is hard to explain why the invalid times-goes-backwards case can > happen if env->count_save_time is overwritten with data from another > machine. I think the explanation is that (due to > timers_state.cpu_ticks_enabled) the value of "cpu_get_clock_at(now) - > env->count_save_time" does not depend on get_clock(), but the code > doesn't have any comment for that. Exactly. I think of it in terms of count_save_time being in the time frame of the vm clock, which is also migrated. In fact since the VM clock is stopped during migration, the saved/restored count_save_time will likely be the same as the saved vm clock, so the delta calculated above at restore time is the time since the vm clock was resumed. > Rather than adding comments, we might as well force it to be always zero > and just write get_clock() to COUNT_RESUME. > > Finally, having to serialize env->count_save_time makes harder to > support migration from TCG to KVM and back. Yes, I'm not keen on that bit of code. > >> It would be race free though, and if you're stopping the VM at all you >> expect to lose some time anyway. > > Since you mentioned perfectionism, :) your code also loses some time; > COUNT_RESUME is written based on when the CPU state becomes clean, not > on when the CPU was restarted. The offset you suggest removing is what ensures time isn't lost there. The VM time when the cpu is restarted == the VM time when it is stopped, so by saving vm time to count_save_time at vm stop, the timer can be restarted at exactly the right interval into the past (COUNT_RESUME=now-interval) so that no time is lost. Now there is one case I believe where time will be gained which is hard to avoid without getting a notification before VM stop rather than after. When the VM is stopped and qemu's state is clean, the state is read very soon after (not immediately), so the saved state will be at slightly after the recorded vm time. I.e. the timer was allowed to continue slightly past when the vm clock was actually stopped. Cheers James
