Re: KVM: x86: fix tsc catchup issue with tsc scaling

[Marcelo Tosatti <mtosatti@redhat.com>]

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@redhat.com>
> > 
> > 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?