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

[Marcelo Tosatti]

On Tue, Sep 16, 2014 at 06:03:40PM +0200, Paolo Bonzini wrote:
> Il 16/09/2014 17:55, Marcelo Tosatti ha scritto:
> > On Tue, Sep 16, 2014 at 05:42:16PM +0200, Paolo Bonzini wrote:
> >> Il 16/09/2014 17:14, Marcelo Tosatti ha scritto:
> >>> + /*
> >>> +  * Make sure that CPU state is synchronized from KVM
> >>> +          * once every VM state change callback has finished.
> >>
> >> Which other callback could affect the in-kernel state, 
> > 
> > Marcin mentioned that APIC state was the culprit.
> > 
> > Perhaps 
> > 
> >      bdrv_drain_all(); 
> >      ret = bdrv_flush_all();
> > 
> > Can change the interrupt state ?
> 
> Ah, I thought Marcin was checking on the destination, not the source.
> 
> > Then that should read "once VM stop has finished".
> 
> But I still do not understand.
> 
> 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.

> But if the synchronize_all_states+clean_all_dirty pair runs on the
> source, why is the cpu_synchronize_all_states() call in
> qemu_savevm_state_complete() not enough?  It runs even later than
> kvmclock_vm_state_change.

Because of the "pair of time values" explanation above.

> I don't understand even the original patch without cpu_clean_all_dirty()...
> 
> Paolo