On Wed, Jan 15, 2014 at 01:55:50PM +0100, Paolo Bonzini wrote: > Il 15/01/2014 13:34, Marcelo Tosatti ha scritto: > > On Wed, Jan 15, 2014 at 12:43:02PM +0100, Paolo Bonzini wrote: > >> Il 06/01/2014 15:18, Marcelo Tosatti ha scritto: > >>> > >>> To fix a problem related to different resolution of TSC and system clock, > >>> the offset in TSC units is approximated by > >>> > >>> delta = vcpu->hv_clock.tsc_timestamp - vcpu->last_guest_tsc > >>> > >>> (Guest TSC value at (Guest TSC value at last VM-exit) > >>> the last kvm_guest_time_update > >>> call) > >>> > >>> Delta is then later scaled using mult,shift pair found in hv_clock > >>> structure (which is correct against tsc_timestamp in that > >>> structure). > >>> > >>> However, if a frequency change is performed between these two points, > >>> this delta is measured using different TSC frequencies, but scaled using > >>> mult,shift pair for one frequency only. > >>> > >>> The end result is an incorrect delta. > >>> > >>> The bug which this code works around is not the only cause for > >>> clock backwards events. The global accumulator is still > >>> necessary, so remove the max_kernel_ns fix and rely on the > >>> global accumulator for no clock backwards events. > >> > >> This is basically reverting commit 1d5f066 (KVM: x86: Fix a possible > >> backwards warp of kvmclock, 2010-08-19). > >> > >> Your commit message basically says that the guest-side 489fb49 (x86, > >> paravirt: Add a global synchronization point for pvclock, 2010-05-11) is > >> the real solution to the problem that the host-side commit 1d5f066 was > >> trying to fix. Right? > >> > >> This patch makes vcpu->hv_clock.tsc_timestamp write only. Please > >> provide a follow up that drops the field entirely, then I'll apply both. > >> In the meanwhile: > >> > >> Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx> > > > > Can't do that: its inside hv_clock structure. > > Right. Another question, what about this comment: > > /* Reset of TSC must disable overshoot protection below */ > vcpu->arch.hv_clock.tsc_timestamp = 0; > vcpu->arch.last_guest_tsc = data; > > Should it be instead like this: > > vcpu->arch.hv_clock.tsc_timestamp += data - vcpu->arch.last_guest_tsc; > vcpu->arch.last_guest_tsc = data; > > ? Don't see why it should? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
