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).
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. 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.
Paolo
