Re: [Qemu-devel] [PATCH] kvmclock: clarify usage of cpu_clean_all_dirty

[Marcelo Tosatti]

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).