Re: [PATCH v2 1/2] KVM: x86: hyper-v: Properly divide maybe-negative 'hv_clock->system_time' in compute_tsc_page_parameters()

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 30/03/21 16:44, Vitaly Kuznetsov wrote:
>> We could've solved the problem by reducing the precision a bit and
>> instead of doing
>> 
>>   now_ns = get_kvmclock_ns(kvm);
>> 
>> in KVM_SET_CLOCK() handling we could do
>> 
>>   now_ns = ka->master_kernel_ns
>> 
>> and that would make vcpu->hv_clock.system_time == 0 after
>> kvm_guest_time_update() but it'd hurt 'normal' use-cases to fix the
>> corner case.
>
> Marcelo is right, and I guess instead of a negative system time you 
> *should* have a slightly larger tsc_timestamp that corresponds to a zero 
> system_time.  E.g. instead of -70 ns in the system time you'd have 210 
> more clock cycles in the tsc_timestamp and 0 in the system time.
>
> In the end it's impossible to achieve absolute precision; does the 
> KVM_SET_CLOCK value refer to the nanosecond value before entering the 
> kernel, or after, or what?  The difference is much more than these 70 
> ns.  So what you propose above seems to be really the best solution 
> within the constraints of the KVM_SET_CLOCK API (a better API, which 
> Maxim had started working on and I should revisit, would pass a 
> TSC+nanosecond pair).

I'm leaning towards making a v3 and depending on how complex it's going
to look like we can decide which way to go.

>
> On the other hand you'd have to take into account what happens if the 
> masterclock is not in use, which would make things a bit more complex 
> than what you sketched. 

(I really wish we establish a plan to get rid of !masterclock logic some
day ... )

> If guests probably do not look at the 
> system_time and just add it blindly to the result, then treating 
> system_time as signed as in v2 is the easiest.

-- 
Vitaly