Paolo Bonzini <pbonzini@xxxxxxxxxx> 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
