On 31/03/21 11:59, Vitaly Kuznetsov wrote:
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.
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.
Yes, sounds good.
Note that I posted a series that changes pvclock_gtod_sync_lock to use
spin_lock_irqsave/spin_unlock_irqrestore, so please base your patch on
those and switch to spin_lock_irq.
Paolo
