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

[Paolo Bonzini <pbonzini@redhat.com>]

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;

?

Paolo