On 25/05/2017 11:40, Roman Kagan wrote: >>>> + kvmclock_struct_pa = env->system_time_msr & ~1ULL; >>>> + >>>> if (!(env->system_time_msr & 1ULL)) { >>>> /* KVM clock not active */ >>>> return 0; >>> Roman. >> Can't you avoid that call to each CPU? (ie fix the synchronization >> of the system time address problem in some other way?) > Sorry, what call do you mean? On one hand I suggested exactly to only > call cpu_synchronize_state on the current (== first) cpu. On the other, > cpu_synchronize_state is heavier than just fetching a single msr. > > Anyway kvmclock_current_nsec is only called in kvmclock_vm_state_change > callback which is certainly not performance-critical, so IMO less new > code here is better than more efficiency. > > Or maybe I misunderstand your reason to request that the synchronization > problem is fixed in some other way?
Denis's patch is problematic in that KVM_GET_MSRS should run in the VCPU thread (using run_on_cpu). cpu_synchronize_state() is heavier, but solves this problem nicely. Since it's not performance critical as you say, calling cpu_synchronize_state() from kvmclock_current_nsec() seems the best solution. Paolo