On Tue, Sep 16, 2014 at 01:48:24PM -0300, Marcelo Tosatti wrote: > On Tue, Sep 16, 2014 at 06:22:15PM +0200, Paolo Bonzini wrote: > > Il 16/09/2014 18:07, Marcelo Tosatti ha scritto: > > >> > The cpu_synchronize_all_states() call in kvmclock_vm_state_change() is > > >> > needed to make env->tsc up to date with the value on the source, right? > > > Its there to make sure the pair > > > > > > env->tsc, s->clock = data.clock > > > > > > are relative to point close in time. > > > > Ok. But why are they not close in time? > > Scenario 1 > > A. s->clock = get_clock() > B. env->tsc = rdtsc() > > Scenario 2 > > A. s->clock = get_clock() > C. VM callbacks, bdrv_flush, ... > B. env->tsc = rdtsc() > > They are not "close in time" because of C. > > > Could we have the opposite situation where env->tsc is loaded a long > > time _after_ s->clock, and something breaks? > > This particular read avoids an overflow. See > > + assert(time.tsc_timestamp <= migration_tsc); > > > About your question, perhaps, would have to make up an > env->tsc, s->clock pair which breaks the code or a guest > application. > > > Also, there is no reason to do kvmclock_current_nsec() during normal > > execution of the VM. Is the s->clock sent by the source ever good > > across migration, and could the source send kvmclock_current_nsec() > > instead of whatever KVM_GET_CLOCK returns? > > Yes it could. What difference does it make? > > > I don't understand this code very well, but it seems to me that the > > migration handling and VM state change handler are mixed up... > > I don't see what the problem is. I am sure you can understand the code.
If you have a suggestion to improve the code, i am all ears. But IMHO, its not terrible (migration and VM state change handling are mixed in other drivers as well).
