I'd like to make a few cleanups and add more documentation:
diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index eacc9dc..f767ea9 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -37,7 +37,7 @@ typedef struct KVMClockState {
uint64_t clock;
bool clock_valid;
- /* whether machine type supports reliable get clock */
+ /* whether machine type supports reliable KVM_GET_CLOCK */
bool mach_use_reliable_get_clock;
/* whether the 'clock' value was obtained in a host with
@@ -88,7 +88,7 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
return nsec + time.system_time;
}
-static uint64_t kvm_get_clock(void)
+static void kvm_update_clock(void)
{
struct kvm_clock_data data;
int ret;
@@ -98,7 +98,48 @@ static uint64_t kvm_get_clock(void)
fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
abort();
}
- return data.clock;
+ s->clock = data.clock;
+
+ /* If kvm_has_adjust_clock_stable() is false, KVM_GET_CLOCK returns
+ * essentially CLOCK_MONOTONIC plus a guest-specific adjustment. This
+ * can drift from the TSC-based value that is computed by the guest,
+ * so we need to go through kvmclock_current_nsec(). If
+ * kvm_has_adjust_clock_stable() is true, and the flags contain
+ * KVM_CLOCK_TSC_STABLE, then KVM_GET_CLOCK returns a TSC-based value
+ * and kvmclock_current_nsec() is not necessary.
+ *
+ * Here, however, we need not check KVM_CLOCK_TSC_STABLE. This is because:
+ *
+ * - if the host has disabled the kvmclock master clock, the guest already
+ * has protection against time going backwards. This "safety net" is
only
+ * absent when kvmclock is stable;
+ *
+ * - therefore, we can replace a check like
+ *
+ * if last KVM_GET_CLOCK was not reliable
+ * read from memory
+ *
+ * with
+ *
+ * if last KVM_GET_CLOCK was not reliable && masterclock is enabled
+ * read from memory
+ *
+ * However:
+ *
+ * - if kvm_has_adjust_clock_stable() returns false, the left side is
+ * always true (KVM_GET_CLOCK is never reliable), and the right side is
+ * unknown (because we don't have data.flags). We must assume it's true
+ * and read from memory.
+ *
+ * - if kvm_has_adjust_clock_stable() returns true, the result of the &&
+ * is always false (masterclock is enabled iff KVM_GET_CLOCK is reliable)
+ *
+ * So we can just use this instead:
+ *
+ * if !kvm_has_adjust_clock_stable() then
+ * read from memory
+ */
+ s->clock_is_reliable = kvm_has_adjust_clock_stable();
}
static void kvmclock_vm_state_change(void *opaque, int running,
@@ -111,19 +153,17 @@ static void kvmclock_vm_state_change(void *opaque, int
running,
if (running) {
struct kvm_clock_data data = {};
- uint64_t pvclock_via_mem = 0;
/*
* If the host where s->clock was read did not support reliable
* KVM_GET_CLOCK, read kvmclock value from memory.
*/
if (!s->clock_is_reliable) {
- pvclock_via_mem = kvmclock_current_nsec(s);
- }
-
- /* We can't rely on the saved clock value, just discard it */
- if (pvclock_via_mem) {
- s->clock = pvclock_via_mem;
+ uint64_t pvclock_via_mem = kvmclock_current_nsec(s);
+ /* We can't rely on the saved clock value, just discard it */
+ if (pvclock_via_mem) {
+ s->clock = pvclock_via_mem;
+ }
}
s->clock_valid = false;
@@ -155,11 +195,7 @@ static void kvmclock_vm_state_change(void *opaque, int
running,
kvm_synchronize_all_tsc();
- s->clock = kvm_get_clock();
- /* any code that sets s->clock needs to ensure clock_is_reliable
- * is correctly set.
- */
- s->clock_is_reliable = kvm_has_adjust_clock_stable();
+ kvm_update_clock();
/*
* If the VM is stopped, declare the clock state valid to
* avoid re-reading it on next vmsave (which would return
@@ -173,9 +209,7 @@ static void kvmclock_realize(DeviceState *dev, Error **errp)
{
KVMClockState *s = KVM_CLOCK(dev);
- if (kvm_has_adjust_clock_stable()) {
- s->clock_is_reliable = true;
- }
+ kvm_update_clock();
qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
}
@@ -216,7 +250,7 @@ static void kvmclock_pre_save(void *opaque)
{
KVMClockState *s = opaque;
- s->clock = kvm_get_clock();
+ kvm_update_clock();
}
static const VMStateDescription kvmclock_vmsd = {
