On Thu, Dec 05, 2013 at 02:40:00PM -0200, Marcelo Tosatti wrote: > On Thu, Dec 05, 2013 at 05:02:02PM +0100, Paolo Bonzini wrote: > > Il 05/12/2013 16:42, Fernando Luis Vazquez Cao ha scritto: > > > (2013/12/05 22:53), Paolo Bonzini wrote: > > >> Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto: > > >>> /* > > >>> * KVM is yet unable to synchronize TSC values of multiple > > >>> VCPUs on > > >>> * writeback. Until this is fixed, we only write the offset to > > >>> SMP > > >>> * guests after migration, desynchronizing the VCPUs, but > > >>> avoiding > > >>> * huge jump-backs that would occur without any writeback at > > >>> all. > > >>> */ > > >>> - if (smp_cpus == 1 || env->tsc != 0) { > > >>> + if (smp_cpus == 1 || env->tsc != 0 || level == > > >>> KVM_PUT_RESET_STATE) { > > >>> kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc); > > >>> } > > >> This is still a bit ugly, and desynchronizes the VCPUs on reset. > > > > > > I agree it is a bit ugly, but in my testing QEMU seemed to loop over all > > > the VCPUS fast enough for the kernel side kvm_write_tsc() to do a > > > reasonable job of matching the offsets (the Linux guest did not mark > > > the TSC unstable due to the TSCs being unsynchronized). Am I missing > > > something? > > > > No, probably not. > > > > > I understand the benefits of what you are proposing but, since it is > > > wider is scope and it would be more difficult to backport, I would > > > prefer to implement it as a follow-up patch, unless you think that > > > the current patch as a standalone fix does more harm than good. > > > > It does some harm in that it introduces a case where KVM_PUT_RESET_STATE > > restores something, but KVM_PUT_FULL_STATE doesn't. > > > > If it really usually works, there shouldn't be a need for this "if" > > statement at all. > > > > Marcelo, what do you think? > > > > Paolo > > Its OK to drop it, provided the following is tested on SMP guests: > > 1. initialization. > 2. reboot. > 3. migration. > > With both stable and unstable TSC hosts (use wrmsr tool to write TSC on > a given host CPU, to make it an unstable TSC host). (A=rdmsr ; sleep 1s; > wrmsr A). > > To make sure the code is not securing against a kvm_write_tsc > cornercase.
The TSCs should start synchronized, and remain synchronized across reboot and migration for stable TSC host case. It is not necessary to test the unstable TSC host case.