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 31/03/21 08:29, Vitaly Kuznetsov wrote:
>> 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.
>
> As you prefer, but I would have no problem with committing v2 for now. 
>  From the point of view of system_time being a signed delta your v2 is 
> not a regression.

Ok, I convinced myself this is correct:

@@ -5744,8 +5745,22 @@ long kvm_arch_vm_ioctl(struct file *filp,
                 * pvclock_update_vm_gtod_copy().
                 */
                kvm_gen_update_masterclock(kvm);
-               now_ns = get_kvmclock_ns(kvm);
-               kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
+
+               /*
+                * This pairs with kvm_guest_time_update(): when masterclock is
+                * in use, we use master_kernel_ns + kvmclock_offset to set
+                * unsigned 'system_time' so if we use get_kvmclock_ns() (which
+                * is slightly ahead) here we risk going negative on unsigned
+                * 'system_time' when 'user_ns.clock' is very small.
+                */
+               spin_lock(&ka->pvclock_gtod_sync_lock);
+               if (kvm->arch.use_master_clock)
+                       now_ns = ka->master_kernel_ns;
+               else
+                       now_ns = get_kvmclock_base_ns();
+               ka->kvmclock_offset = user_ns.clock - now_ns;
+               spin_unlock(&ka->pvclock_gtod_sync_lock);
+
                kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);
                break;
        }

In !masterclock case kvm_guest_time_update() uses get_kvmclock_base_ns()
(and get_kvmclock_ns() is just get_kvmclock_base_ns() + ka->kvmclock_offset)
so we're good. Also, it looks like a good idea to put the whole
calculation under spinlock here.

Personally, I like this a little bit more than treating 'system_time' as
signed, what do you guys think?

-- 
Vitaly