[Qemu-devel] [qemu patch 0/2] improve kvmclock difference on migration

[Qemu-devel] [qemu patch 0/2] improve kvmclock difference on migration

[Marcelo Tosatti]

See patches for details.

[Qemu-devel] [qemu patch 1/2] kvm: sync linux headers

[Marcelo Tosatti]

[-- Attachment #1: sync-linux-headers.patch --]
[-- Type: text/plain, Size: 5447 bytes --]

Import KVM_CLOCK_TSC_STABLE.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/include/standard-headers/linux/input.h b/include/standard-headers/linux/input.h
index 7361a16..b472b85 100644
--- a/include/standard-headers/linux/input.h
+++ b/include/standard-headers/linux/input.h
@@ -245,6 +245,7 @@ struct input_mask {
 #define BUS_SPI			0x1C
 #define BUS_RMI			0x1D
 #define BUS_CEC			0x1E
+#define BUS_INTEL_ISHTP		0x1F
 
 /*
  * MT_TOOL types
diff --git a/include/standard-headers/linux/pci_regs.h b/include/standard-headers/linux/pci_regs.h
index 4040951..e5a2e68 100644
--- a/include/standard-headers/linux/pci_regs.h
+++ b/include/standard-headers/linux/pci_regs.h
@@ -612,6 +612,8 @@
  */
 #define PCI_EXP_DEVCAP2		36	/* Device Capabilities 2 */
 #define  PCI_EXP_DEVCAP2_ARI		0x00000020 /* Alternative Routing-ID */
+#define  PCI_EXP_DEVCAP2_ATOMIC_ROUTE	0x00000040 /* Atomic Op routing */
+#define PCI_EXP_DEVCAP2_ATOMIC_COMP64	0x00000100 /* Atomic 64-bit compare */
 #define  PCI_EXP_DEVCAP2_LTR		0x00000800 /* Latency tolerance reporting */
 #define  PCI_EXP_DEVCAP2_OBFF_MASK	0x000c0000 /* OBFF support mechanism */
 #define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
@@ -619,6 +621,7 @@
 #define PCI_EXP_DEVCTL2		40	/* Device Control 2 */
 #define  PCI_EXP_DEVCTL2_COMP_TIMEOUT	0x000f	/* Completion Timeout Value */
 #define  PCI_EXP_DEVCTL2_ARI		0x0020	/* Alternative Routing-ID */
+#define PCI_EXP_DEVCTL2_ATOMIC_REQ	0x0040	/* Set Atomic requests */
 #define  PCI_EXP_DEVCTL2_IDO_REQ_EN	0x0100	/* Allow IDO for requests */
 #define  PCI_EXP_DEVCTL2_IDO_CMP_EN	0x0200	/* Allow IDO for completions */
 #define  PCI_EXP_DEVCTL2_LTR_EN		0x0400	/* Enable LTR mechanism */
@@ -671,7 +674,8 @@
 #define PCI_EXT_CAP_ID_PMUX	0x1A	/* Protocol Multiplexing */
 #define PCI_EXT_CAP_ID_PASID	0x1B	/* Process Address Space ID */
 #define PCI_EXT_CAP_ID_DPC	0x1D	/* Downstream Port Containment */
-#define PCI_EXT_CAP_ID_MAX	PCI_EXT_CAP_ID_DPC
+#define PCI_EXT_CAP_ID_PTM	0x1F	/* Precision Time Measurement */
+#define PCI_EXT_CAP_ID_MAX	PCI_EXT_CAP_ID_PTM
 
 #define PCI_EXT_CAP_DSN_SIZEOF	12
 #define PCI_EXT_CAP_MCAST_ENDPOINT_SIZEOF 40
@@ -964,4 +968,13 @@
 
 #define PCI_EXP_DPC_SOURCE_ID		10	/* DPC Source Identifier */
 
+/* Precision Time Measurement */
+#define PCI_PTM_CAP			0x04	    /* PTM Capability */
+#define  PCI_PTM_CAP_REQ		0x00000001  /* Requester capable */
+#define  PCI_PTM_CAP_ROOT		0x00000004  /* Root capable */
+#define  PCI_PTM_GRANULARITY_MASK	0x0000FF00  /* Clock granularity */
+#define PCI_PTM_CTRL			0x08	    /* PTM Control */
+#define  PCI_PTM_CTRL_ENABLE		0x00000001  /* PTM enable */
+#define  PCI_PTM_CTRL_ROOT		0x00000002  /* Root select */
+
 #endif /* LINUX_PCI_REGS_H */
diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index 541268c..2fb7859 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -84,6 +84,13 @@ struct kvm_regs {
 #define KVM_VGIC_V2_DIST_SIZE		0x1000
 #define KVM_VGIC_V2_CPU_SIZE		0x2000
 
+/* Supported VGICv3 address types  */
+#define KVM_VGIC_V3_ADDR_TYPE_DIST	2
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST	3
+
+#define KVM_VGIC_V3_DIST_SIZE		SZ_64K
+#define KVM_VGIC_V3_REDIST_SIZE		(2 * SZ_64K)
+
 #define KVM_ARM_VCPU_POWER_OFF		0 /* CPU is started in OFF state */
 #define KVM_ARM_VCPU_PSCI_0_2		1 /* CPU uses PSCI v0.2 */
 
diff --git a/linux-headers/asm-x86/unistd_32.h b/linux-headers/asm-x86/unistd_32.h
index abeaf40..d45ea28 100644
--- a/linux-headers/asm-x86/unistd_32.h
+++ b/linux-headers/asm-x86/unistd_32.h
@@ -377,5 +377,8 @@
 #define __NR_copy_file_range 377
 #define __NR_preadv2 378
 #define __NR_pwritev2 379
+#define __NR_pkey_mprotect 380
+#define __NR_pkey_alloc 381
+#define __NR_pkey_free 382
 
 #endif /* _ASM_X86_UNISTD_32_H */
diff --git a/linux-headers/asm-x86/unistd_64.h b/linux-headers/asm-x86/unistd_64.h
index 73c3d1f..e22db91 100644
--- a/linux-headers/asm-x86/unistd_64.h
+++ b/linux-headers/asm-x86/unistd_64.h
@@ -330,5 +330,8 @@
 #define __NR_copy_file_range 326
 #define __NR_preadv2 327
 #define __NR_pwritev2 328
+#define __NR_pkey_mprotect 329
+#define __NR_pkey_alloc 330
+#define __NR_pkey_free 331
 
 #endif /* _ASM_X86_UNISTD_64_H */
diff --git a/linux-headers/asm-x86/unistd_x32.h b/linux-headers/asm-x86/unistd_x32.h
index e5aea76..84e58b2 100644
--- a/linux-headers/asm-x86/unistd_x32.h
+++ b/linux-headers/asm-x86/unistd_x32.h
@@ -283,6 +283,9 @@
 #define __NR_membarrier (__X32_SYSCALL_BIT + 324)
 #define __NR_mlock2 (__X32_SYSCALL_BIT + 325)
 #define __NR_copy_file_range (__X32_SYSCALL_BIT + 326)
+#define __NR_pkey_mprotect (__X32_SYSCALL_BIT + 329)
+#define __NR_pkey_alloc (__X32_SYSCALL_BIT + 330)
+#define __NR_pkey_free (__X32_SYSCALL_BIT + 331)
 #define __NR_rt_sigaction (__X32_SYSCALL_BIT + 512)
 #define __NR_rt_sigreturn (__X32_SYSCALL_BIT + 513)
 #define __NR_ioctl (__X32_SYSCALL_BIT + 514)
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 4806e06..bb0ed71 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -972,12 +972,19 @@ struct kvm_irqfd {
 	__u8  pad[16];
 };
 
+/* For KVM_CAP_ADJUST_CLOCK */
+
+/* Do not use 1, KVM_CHECK_EXTENSION returned it before we had flags.  */
+#define KVM_CLOCK_TSC_STABLE		2
+
 struct kvm_clock_data {
 	__u64 clock;
 	__u32 flags;
 	__u32 pad[9];
 };
 
+/* For KVM_CAP_SW_TLB */
+
 #define KVM_MMU_FSL_BOOKE_NOHV		0
 #define KVM_MMU_FSL_BOOKE_HV		1
 

[Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

[-- Attachment #1: kvmclock-advance-clock.patch --]
[-- Type: text/plain, Size: 6621 bytes --]

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which
indicates that KVM_GET_CLOCK returns a value as seen by the guest at
that moment.

For new machine types, use this value rather than reading 
from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s
(when max_downtime == 5s).

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

---
 hw/i386/kvm/clock.c    |   88 +++++++++++++++++++++++++++++++++++++++++++++++--
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1 
 4 files changed, 98 insertions(+), 3 deletions(-)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================
--- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c	2016-11-14 09:07:55.519856329 -0200
+++ qemu-mig-advance-clock/hw/i386/kvm/clock.c	2016-11-14 10:19:45.723254737 -0200
@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {
@@ -91,15 +97,37 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t time_at_migration = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                time_at_migration = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and host where vm was executing
+             * supported reliable get clock
+             */
+            if (!s->mach_use_reliable_get_clock ||
+                !s->src_use_reliable_get_clock) {
+                time_at_migration = kvmclock_current_nsec(s);
+            }
+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
+        /* We can't rely on the saved clock value, just discard it */
         if (time_at_migration) {
             s->clock = time_at_migration;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {
@@ -152,22 +180,76 @@
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }
+
+    return s->src_use_reliable_get_clock;
+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/src_use_reliable_get_clock",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_src_use_reliable_get_clock,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    s->clock = data.clock;
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_save = kvmclock_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &kvmclock_reliable_get_clock,
+        NULL
     }
 };
 
+static Property kvmclock_properties[] = {
+    DEFINE_PROP_BOOL("mach_use_reliable_get_clock", KVMClockState,
+                      mach_use_reliable_get_clock, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void kvmclock_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = kvmclock_realize;
     dc->vmsd = &kvmclock_vmsd;
+    dc->props = kvmclock_properties;
 }
 
 static const TypeInfo kvmclock_info = {
Index: qemu-mig-advance-clock/include/hw/i386/pc.h
===================================================================
--- qemu-mig-advance-clock.orig/include/hw/i386/pc.h	2016-11-14 09:07:55.519856329 -0200
+++ qemu-mig-advance-clock/include/hw/i386/pc.h	2016-11-14 09:11:47.112200123 -0200
@@ -370,6 +370,11 @@
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
     {\
+        .driver   = "kvmclock",\
+        .property = "mach_use_reliable_get_clock",\
+        .value    = "off",\
+    },\
+    {\
         .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\
Index: qemu-mig-advance-clock/target-i386/kvm.c
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm.c	2016-11-14 09:07:55.520856330 -0200
+++ qemu-mig-advance-clock/target-i386/kvm.c	2016-11-14 09:11:47.125200142 -0200
@@ -117,6 +117,13 @@
     return kvm_check_extension(kvm_state, KVM_CAP_X86_SMM);
 }
 
+bool kvm_has_adjust_clock_stable(void)
+{
+    int ret = kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK);
+
+    return (ret == KVM_CLOCK_TSC_STABLE);
+}
+
 bool kvm_allows_irq0_override(void)
 {
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
Index: qemu-mig-advance-clock/target-i386/kvm_i386.h
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm_i386.h	2016-11-14 09:07:55.520856330 -0200
+++ qemu-mig-advance-clock/target-i386/kvm_i386.h	2016-11-14 09:11:47.125200142 -0200
@@ -17,6 +17,7 @@
 
 bool kvm_allows_irq0_override(void);
 bool kvm_has_smm(void);
+bool kvm_has_adjust_clock_stable(void);
 void kvm_synchronize_all_tsc(void);
 void kvm_arch_reset_vcpu(X86CPU *cs);
 void kvm_arch_do_init_vcpu(X86CPU *cs);

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini]

On 14/11/2016 13:36, Marcelo Tosatti wrote:
> +        /* local (running VM) restore */
> +        if (s->clock_valid) {
> +            /*
> +             * if host does not support reliable KVM_GET_CLOCK,
> +             * read kvmclock value from memory
> +             */
> +            if (!kvm_has_adjust_clock_stable()) {
> +                time_at_migration = kvmclock_current_nsec(s);

Just assign to s->clock here...


> +            }
> +        /* migration/savevm/init restore */
> +        } else {
> +            /*
> +             * use s->clock in case machine uses reliable
> +             * get clock and host where vm was executing
> +             * supported reliable get clock
> +             */
> +            if (!s->mach_use_reliable_get_clock ||
> +                !s->src_use_reliable_get_clock) {
> +                time_at_migration = kvmclock_current_nsec(s);

... and here, so that time_at_migration is not needed anymore.

Also here it's enough to look at s->src_user_reliable_get_clock, because
if s->mach_use_reliable_get_clock is false,
s->src_use_reliable_get_clock will be false as well.

> +            }
> +        }
>  
> -        /* We can't rely on the migrated clock value, just discard it */
> +        /* We can't rely on the saved clock value, just discard it */
>          if (time_at_migration) {
>              s->clock = time_at_migration;

[...]

> 
> +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> +{
> +    KVMClockState *s = opaque;
> +
> +    /*
> +     * On machine types that support reliable KVM_GET_CLOCK,
> +     * if host kernel does provide reliable KVM_GET_CLOCK,
> +     * set src_use_reliable_get_clock=true so that destination
> +     * avoids reading kvmclock from memory.
> +     */
> +    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
> +        s->src_use_reliable_get_clock = true;
> +    }
> +
> +    return s->src_use_reliable_get_clock;
> +}

Here you can just return s->mach_use_reliable_get_clock.  To set
s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look
at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.

You don't actually need kvm_has_adjust_clock_stable(), but please place
the KVM_GET_CLOCK code for kvmclock_pre_save and
kvmclock_vm_state_change in a common function.

Also, just another small nit: please make your scripts use the "-p"
option on diff. :)

Thanks,

Paolo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

On Mon, Nov 14, 2016 at 02:54:38PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/11/2016 13:36, Marcelo Tosatti wrote:
> > +        /* local (running VM) restore */
> > +        if (s->clock_valid) {
> > +            /*
> > +             * if host does not support reliable KVM_GET_CLOCK,
> > +             * read kvmclock value from memory
> > +             */
> > +            if (!kvm_has_adjust_clock_stable()) {
> > +                time_at_migration = kvmclock_current_nsec(s);
> 
> Just assign to s->clock here...

If kvmclock is not enabled, you want to use s->clock,
rather than 0.

> > +            }
> > +        /* migration/savevm/init restore */
> > +        } else {
> > +            /*
> > +             * use s->clock in case machine uses reliable
> > +             * get clock and host where vm was executing
> > +             * supported reliable get clock
> > +             */
> > +            if (!s->mach_use_reliable_get_clock ||
> > +                !s->src_use_reliable_get_clock) {
> > +                time_at_migration = kvmclock_current_nsec(s);
> 
> ... and here, so that time_at_migration is not needed anymore.

Same as above.

> Also here it's enough to look at s->src_user_reliable_get_clock, because
> if s->mach_use_reliable_get_clock is false,
> s->src_use_reliable_get_clock will be false as well.

Yes, but i like the code annotation.

> > +            }
> > +        }
> >  
> > -        /* We can't rely on the migrated clock value, just discard it */
> > +        /* We can't rely on the saved clock value, just discard it */
> >          if (time_at_migration) {
> >              s->clock = time_at_migration;
> 
> [...]
> 
> > 
> > +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> > +{
> > +    KVMClockState *s = opaque;
> > +
> > +    /*
> > +     * On machine types that support reliable KVM_GET_CLOCK,
> > +     * if host kernel does provide reliable KVM_GET_CLOCK,
> > +     * set src_use_reliable_get_clock=true so that destination
> > +     * avoids reading kvmclock from memory.
> > +     */
> > +    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
> > +        s->src_use_reliable_get_clock = true;
> > +    }
> > +
> > +    return s->src_use_reliable_get_clock;
> > +}
> 
> Here you can just return s->mach_use_reliable_get_clock. 

mach_use_reliable_get_clock can be true but host might not support it.

>  To set
> s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look
> at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.

KVM_CLOCK_TSC_STABLE bit in the kvmclock structure != 
KVM_GET_CLOCK returns reliable value, right?


> You don't actually need kvm_has_adjust_clock_stable(), but please place
> the KVM_GET_CLOCK code for kvmclock_pre_save and
> kvmclock_vm_state_change in a common function.

Sure.

> Also, just another small nit: please make your scripts use the "-p"
> option on diff. :)

Sure.

> 
> Thanks,
> 
> Paolo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini]

On 14/11/2016 15:00, Marcelo Tosatti wrote:
> On Mon, Nov 14, 2016 at 02:54:38PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 14/11/2016 13:36, Marcelo Tosatti wrote:
>>> +        /* local (running VM) restore */
>>> +        if (s->clock_valid) {
>>> +            /*
>>> +             * if host does not support reliable KVM_GET_CLOCK,
>>> +             * read kvmclock value from memory
>>> +             */
>>> +            if (!kvm_has_adjust_clock_stable()) {
>>> +                time_at_migration = kvmclock_current_nsec(s);
>>
>> Just assign to s->clock here...
> 
> If kvmclock is not enabled, you want to use s->clock,
> rather than 0.
> 
>>> +            }
>>> +        /* migration/savevm/init restore */
>>> +        } else {
>>> +            /*
>>> +             * use s->clock in case machine uses reliable
>>> +             * get clock and host where vm was executing
>>> +             * supported reliable get clock
>>> +             */
>>> +            if (!s->mach_use_reliable_get_clock ||
>>> +                !s->src_use_reliable_get_clock) {
>>> +                time_at_migration = kvmclock_current_nsec(s);
>>
>> ... and here, so that time_at_migration is not needed anymore.
> 
> Same as above.

You're right.

>> Also here it's enough to look at s->src_user_reliable_get_clock, because
>> if s->mach_use_reliable_get_clock is false,
>> s->src_use_reliable_get_clock will be false as well.
> 
> Yes, but i like the code annotation.

Ah, I think we're looking at it differently.

I'm thinking "mach_use_reliable_get_clock is just for migration,
src_use_reliable_get_clock is the state".  Perhaps you're thinking of
enabling/disabling the whole new code for old machines?  What is the
advantage?

>>> +            }
>>> +        }
>>>  
>>> -        /* We can't rely on the migrated clock value, just discard it */
>>> +        /* We can't rely on the saved clock value, just discard it */
>>>          if (time_at_migration) {
>>>              s->clock = time_at_migration;
>>
>> [...]
>>
>>>
>>> +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
>>> +{
>>> +    KVMClockState *s = opaque;
>>> +
>>> +    /*
>>> +     * On machine types that support reliable KVM_GET_CLOCK,
>>> +     * if host kernel does provide reliable KVM_GET_CLOCK,
>>> +     * set src_use_reliable_get_clock=true so that destination
>>> +     * avoids reading kvmclock from memory.
>>> +     */
>>> +    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
>>> +        s->src_use_reliable_get_clock = true;
>>> +    }
>>> +
>>> +    return s->src_use_reliable_get_clock;
>>> +}
>>
>> Here you can just return s->mach_use_reliable_get_clock. 
> 
> mach_use_reliable_get_clock can be true but host might not support it.

Yes, but the "needed" function is only required to avoid breaking
pc-i440fx-2.7 and earlier.  If you return true here, you can still
migrate a "false" value for src_use_reliable_get_clock.

>>  To set
>> s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look
>> at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.
> 
> KVM_CLOCK_TSC_STABLE bit in the kvmclock structure != 
> KVM_GET_CLOCK returns reliable value, right?

It is the same as "is using masterclock", which is actually a stricter
condition than the KVM_CHECK_EXTENSION return value.  The right check to
use is whether masterclock is in use, and then the idea is to treat
clock,src_use_reliable_get_clock as one tuple that is updated atomically.

Paolo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

On Mon, Nov 14, 2016 at 03:22:02PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/11/2016 15:00, Marcelo Tosatti wrote:
> > On Mon, Nov 14, 2016 at 02:54:38PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 14/11/2016 13:36, Marcelo Tosatti wrote:
> >>> +        /* local (running VM) restore */
> >>> +        if (s->clock_valid) {
> >>> +            /*
> >>> +             * if host does not support reliable KVM_GET_CLOCK,
> >>> +             * read kvmclock value from memory
> >>> +             */
> >>> +            if (!kvm_has_adjust_clock_stable()) {
> >>> +                time_at_migration = kvmclock_current_nsec(s);
> >>
> >> Just assign to s->clock here...
> > 
> > If kvmclock is not enabled, you want to use s->clock,
> > rather than 0.
> > 
> >>> +            }
> >>> +        /* migration/savevm/init restore */
> >>> +        } else {
> >>> +            /*
> >>> +             * use s->clock in case machine uses reliable
> >>> +             * get clock and host where vm was executing
> >>> +             * supported reliable get clock
> >>> +             */
> >>> +            if (!s->mach_use_reliable_get_clock ||
> >>> +                !s->src_use_reliable_get_clock) {
> >>> +                time_at_migration = kvmclock_current_nsec(s);
> >>
> >> ... and here, so that time_at_migration is not needed anymore.
> > 
> > Same as above.
> 
> You're right.
> 
> >> Also here it's enough to look at s->src_user_reliable_get_clock, because
> >> if s->mach_use_reliable_get_clock is false,
> >> s->src_use_reliable_get_clock will be false as well.
> > 
> > Yes, but i like the code annotation.
> 
> Ah, I think we're looking at it differently.

Well, i didnt want to mix the meaning of the variables:

+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;

See the comments on top (later if you look at the variable, 
then have to think: well it has one name, but its disabled 
by that other path as well, so its more than its 
name,etc...).

> I'm thinking "mach_use_reliable_get_clock is just for migration,

Thats whether the machine supports it. New machines have it enabled,
olders don't.
> src_use_reliable_get_clock is the state". 

Thats whether the migration source supported it.

> Perhaps you're thinking of
> enabling/disabling the whole new code for old machines? 

source          destination         behaviour
supports        supports            on migration use s->clock,
                                    on stop/cont as well

supports        ~supports
                                    on migration use s->clock,
                                    on stop/cont read from guest mem

~support        supports            on migration read from guest,
                                    on stop/cont use
                                    kvm_get_clock/kvm_set_clock

~support        ~support            always read from guest memory


Thats what should happen (and thats what the patch should implement).


"support" means host supports your new KVM_GET_CLOCK/KVM_SET_CLOCK.

>  What is the
> advantage?

Well its necessary to use the correct thing, otherwise
you see a time backwards event.

> 
> >>> +            }
> >>> +        }
> >>>  
> >>> -        /* We can't rely on the migrated clock value, just discard it */
> >>> +        /* We can't rely on the saved clock value, just discard it */
> >>>          if (time_at_migration) {
> >>>              s->clock = time_at_migration;
> >>
> >> [...]
> >>
> >>>
> >>> +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> >>> +{
> >>> +    KVMClockState *s = opaque;
> >>> +
> >>> +    /*
> >>> +     * On machine types that support reliable KVM_GET_CLOCK,
> >>> +     * if host kernel does provide reliable KVM_GET_CLOCK,
> >>> +     * set src_use_reliable_get_clock=true so that destination
> >>> +     * avoids reading kvmclock from memory.
> >>> +     */
> >>> +    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
> >>> +        s->src_use_reliable_get_clock = true;
> >>> +    }
> >>> +
> >>> +    return s->src_use_reliable_get_clock;
> >>> +}
> >>
> >> Here you can just return s->mach_use_reliable_get_clock. 
> > 
> > mach_use_reliable_get_clock can be true but host might not support it.
> 
> Yes, but the "needed" function is only required to avoid breaking
> pc-i440fx-2.7 and earlier. 

"needed" is required so that the migration between:

SRC             DEST                BEHAVIOUR
~support        supports            on migration read from guest,
                                    on stop/cont use
                                    kvm_get_clock/kvm_set_clock

Destination does not use KVM_GET_CLOCK value (which is
broken and should not be used).

> If you return true here, you can still
> migrate a "false" value for src_use_reliable_get_clock.

But the source only uses a reliable KVM_GET_CLOCK if 
both conditions are true.

And the subsection is only needed if the source
uses a reliable KVM_GET_CLOCK.

> >>  To set
> >> s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look
> >> at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.
> > 
> > KVM_CLOCK_TSC_STABLE bit in the kvmclock structure != 
> > KVM_GET_CLOCK returns reliable value, right?
> 
> It is the same as "is using masterclock", which is actually a stricter
> condition than the KVM_CHECK_EXTENSION return value.  The right check to
> use is whether masterclock is in use, 

Actually its "has a reliable KVM_GET_CLOCK" (which returns 
get_kernel_clock() + (rdtsc() - tsc_timestamp), 

"broken KVM_GET_CLOCK" =  get_kernel_clock()

> and then the idea is to treat
> clock,src_use_reliable_get_clock as one tuple that is updated atomically.
> 
> Paolo

Hum, not sure i get this...

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini]

On 14/11/2016 15:50, Marcelo Tosatti wrote:
> Well, i didnt want to mix the meaning of the variables:
> 
> +    /* whether machine supports reliable KVM_GET_CLOCK */
> +    bool mach_use_reliable_get_clock;
> +
> +    /* whether source host supported reliable KVM_GET_CLOCK */
> +    bool src_use_reliable_get_clock;
> 
> See the comments on top (later if you look at the variable, 
> then have to think: well it has one name, but its disabled 
> by that other path as well, so its more than its 
> name,etc...).
> 
>> I'm thinking "mach_use_reliable_get_clock is just for migration,
> 
> Thats whether the machine supports it. New machines have it enabled,
> olders don't.

Yes.

>> src_use_reliable_get_clock is the state". 
> 
> Thats whether the migration source supported it.

But it's not used only for migration.  It's used on every vmstate change
(running->stop and stop->running, isn't it?  I think that, apart from
the migration case, it's better to use s->clock if kvmclock is stable,
even on older machine types.

>>>>> +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
>>>>> +{
>>>>> +    KVMClockState *s = opaque;
>>>>> +
>>>>> +    /*
>>>>> +     * On machine types that support reliable KVM_GET_CLOCK,
>>>>> +     * if host kernel does provide reliable KVM_GET_CLOCK,
>>>>> +     * set src_use_reliable_get_clock=true so that destination
>>>>> +     * avoids reading kvmclock from memory.
>>>>> +     */
>>>>> +    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
>>>>> +        s->src_use_reliable_get_clock = true;
>>>>> +    }
>>>>> +
>>>>> +    return s->src_use_reliable_get_clock;
>>>>> +}
>>>>
>>>> Here you can just return s->mach_use_reliable_get_clock. 
>>>
>>> mach_use_reliable_get_clock can be true but host might not support it.
>>
>> Yes, but the "needed" function is only required to avoid breaking
>> pc-i440fx-2.7 and earlier. 
> 
> "needed" is required so that the migration between:
> 
> SRC             DEST                BEHAVIOUR
> ~support        supports            on migration read from guest,
>                                     on stop/cont use
>                                     kvm_get_clock/kvm_set_clock
> 
> Destination does not use KVM_GET_CLOCK value (which is
> broken and should not be used).

If needed returns false, the destination will see
src_use_reliable_get_clock = false anyway.

If needed returns true, the destination can still see
src_use_reliable_get_clock = false if that's the value.

needed	src_use_reliable_get_clock	effect
false	false				use kvmclock_current_nsec
false	true				use kvmclock_current_nsec
true	false				use kvmclock_current_nsec
true	true				use s->clock


So the idea is:

- set s->clock and s->reliable_get_clock on every KVM_GET_CLOCK

- on migration source, use subsections and
s->mach_use_reliable_get_clock to avoid breaking migration on pre-2.8
machine types

- on migration destination, use .pre_load so that s->reliable_get_clock
is initialized to false on older machine types

>> If you return true here, you can still
>> migrate a "false" value for src_use_reliable_get_clock.
> 
> But the source only uses a reliable KVM_GET_CLOCK if 
> both conditions are true.
> 
> And the subsection is only needed if the source
> uses a reliable KVM_GET_CLOCK.

Yes, but the point of the subsection is just to avoid breaking migration
format.

>> It is the same as "is using masterclock", which is actually a stricter
>> condition than the KVM_CHECK_EXTENSION return value.  The right check to
>> use is whether masterclock is in use, 
> 
> Actually its "has a reliable KVM_GET_CLOCK" (which returns 
> get_kernel_clock() + (rdtsc() - tsc_timestamp), 
> 
> "broken KVM_GET_CLOCK" =  get_kernel_clock()

Yes.  And these end up being the same.

Paolo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

On Mon, Nov 14, 2016 at 04:00:38PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/11/2016 15:50, Marcelo Tosatti wrote:
> > Well, i didnt want to mix the meaning of the variables:
> > 
> > +    /* whether machine supports reliable KVM_GET_CLOCK */
> > +    bool mach_use_reliable_get_clock;
> > +
> > +    /* whether source host supported reliable KVM_GET_CLOCK */
> > +    bool src_use_reliable_get_clock;
> > 
> > See the comments on top (later if you look at the variable, 
> > then have to think: well it has one name, but its disabled 
> > by that other path as well, so its more than its 
> > name,etc...).
> > 
> >> I'm thinking "mach_use_reliable_get_clock is just for migration,
> > 
> > Thats whether the machine supports it. New machines have it enabled,
> > olders don't.
> 
> Yes.
> 
> >> src_use_reliable_get_clock is the state". 
> > 
> > Thats whether the migration source supported it.
> 
> But it's not used only for migration.  It's used on every vmstate change
> (running->stop and stop->running, isn't it?  

No. Its used only if s->clock_valid == false, which means either:

migration/savevm/init.

Now for savevm there is a bug: 

> I think that, apart from
> the migration case, it's better to use s->clock if kvmclock is stable,
> even on older machine types.

Yes, its already doing that:

        /* local (running VM) restore */
        if (s->clock_valid) {
            /*
             * if host does not support reliable KVM_GET_CLOCK,
             * read kvmclock value from memory
             */
            if (!kvm_has_adjust_clock_stable()) {
                time_at_migration = kvmclock_current_nsec(s);
            }

It only uses kvmclock_current_nsec() if kvm_has_adjust_clock_stable is 
false.

> >>>>> +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> >>>>> +{
> >>>>> +    KVMClockState *s = opaque;
> >>>>> +
> >>>>> +    /*
> >>>>> +     * On machine types that support reliable KVM_GET_CLOCK,
> >>>>> +     * if host kernel does provide reliable KVM_GET_CLOCK,
> >>>>> +     * set src_use_reliable_get_clock=true so that destination
> >>>>> +     * avoids reading kvmclock from memory.
> >>>>> +     */
> >>>>> +    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
> >>>>> +        s->src_use_reliable_get_clock = true;
> >>>>> +    }
> >>>>> +
> >>>>> +    return s->src_use_reliable_get_clock;
> >>>>> +}
> >>>>
> >>>> Here you can just return s->mach_use_reliable_get_clock. 
> >>>
> >>> mach_use_reliable_get_clock can be true but host might not support it.
> >>
> >> Yes, but the "needed" function is only required to avoid breaking
> >> pc-i440fx-2.7 and earlier. 
> > 
> > "needed" is required so that the migration between:
> > 
> > SRC             DEST                BEHAVIOUR
> > ~support        supports            on migration read from guest,
> >                                     on stop/cont use
> >                                     kvm_get_clock/kvm_set_clock
> > 
> > Destination does not use KVM_GET_CLOCK value (which is
> > broken and should not be used).
> 
> If needed returns false, the destination will see
> src_use_reliable_get_clock = false anyway.

Causing it to read the clock from memory, thats what 
should happen.

> If needed returns true, the destination can still see
> src_use_reliable_get_clock = false if that's the value.

You are asking me to change from:

+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }
+
+    return s->src_use_reliable_get_clock;
+}

to

static bool kvmclock_src_use_reliable_get_clock(void *opaque)
{
    KVMClockState *s = opaque;

    /*
     * On machine types that support reliable KVM_GET_CLOCK,
     * if host kernel does provide reliable KVM_GET_CLOCK,
     * set src_use_reliable_get_clock=true so that destination
     * avoids reading kvmclock from memory.
     */
    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
    {
        s->src_use_reliable_get_clock = true;
    }

    return s->mach_use_reliable_get_clock;
}


Ah, OK, done.


> needed	src_use_reliable_get_clock	effect
> false	false				use kvmclock_current_nsec
> false	true				use kvmclock_current_nsec
> true	false				use kvmclock_current_nsec
> true	true				use s->clock
> 
> 
> So the idea is:
> 
> - set s->clock and s->reliable_get_clock on every KVM_GET_CLOCK

Already do that (apart from s->clock_valid which is necessary).

> - on migration source, use subsections and
> s->mach_use_reliable_get_clock to avoid breaking migration on pre-2.8
> machine types

Migration is already broken when you use different machine types.

So s->src_use_reliable_get_clock is only used to indicate 
to the destination that: "you can use KVM_GET_CLOCK value, 
its safe".

> - on migration destination, use .pre_load so that s->reliable_get_clock
> is initialized to false on older machine types

Thats what mach_use_reliable_get_clock does already:

static Property kvmclock_properties[] = {
    DEFINE_PROP_BOOL("mach_use_reliable_get_clock", KVMClockState,
                      mach_use_reliable_get_clock, true),
    DEFINE_PROP_END_OF_LIST(),
};

+        .driver   = "kvmclock",\
+        .property = "mach_use_reliable_get_clock",\
+        .value    = "off",\
+    },\
+    {\

> 
> >> If you return true here, you can still
> >> migrate a "false" value for src_use_reliable_get_clock.
> > 
> > But the source only uses a reliable KVM_GET_CLOCK if 
> > both conditions are true.
> > 
> > And the subsection is only needed if the source
> > uses a reliable KVM_GET_CLOCK.
> 
> Yes, but the point of the subsection is just to avoid breaking migration
> format.

Its a new creative use of the subsection. 

> >> It is the same as "is using masterclock", which is actually a stricter
> >> condition than the KVM_CHECK_EXTENSION return value.  The right check to
> >> use is whether masterclock is in use, 
> > 
> > Actually its "has a reliable KVM_GET_CLOCK" (which returns 
> > get_kernel_clock() + (rdtsc() - tsc_timestamp), 
> > 
> > "broken KVM_GET_CLOCK" =  get_kernel_clock()
> 
> Yes.  And these end up being the same.

No. You can have masterclock enabled, KVM_TSC_STABLE set in pvclock
flags, and unreliable KVM_GET_CLOCK (without your patch to
KVM_GET_CLOCK).


Paolo not sure if there is anything further you want me to 
change at this point ? 


Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================
--- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c	2016-11-14 10:40:39.748116312 -0200
+++ qemu-mig-advance-clock/hw/i386/kvm/clock.c	2016-11-14 13:38:29.299955042 -0200
@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {
@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {
@@ -91,15 +110,37 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and source host supported
+             * reliable get clock
+             */
+            if (!(s->mach_use_reliable_get_clock &&
+                  s->src_use_reliable_get_clock)) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* 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;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {
@@ -120,8 +161,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;
@@ -129,13 +168,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return
@@ -152,22 +185,69 @@
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }
+
+    return s->mach_use_reliable_get_clock;
+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/src_use_reliable_get_clock",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_src_use_reliable_get_clock,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    s->clock = kvm_get_clock();
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_save = kvmclock_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &kvmclock_reliable_get_clock,
+        NULL
     }
 };
 
+static Property kvmclock_properties[] = {
+    DEFINE_PROP_BOOL("mach_use_reliable_get_clock", KVMClockState,
+                      mach_use_reliable_get_clock, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void kvmclock_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = kvmclock_realize;
     dc->vmsd = &kvmclock_vmsd;
+    dc->props = kvmclock_properties;
 }
 
 static const TypeInfo kvmclock_info = {
Index: qemu-mig-advance-clock/include/hw/i386/pc.h
===================================================================
--- qemu-mig-advance-clock.orig/include/hw/i386/pc.h	2016-11-14 10:40:39.748116312 -0200
+++ qemu-mig-advance-clock/include/hw/i386/pc.h	2016-11-14 10:40:48.059128649 -0200
@@ -370,6 +370,11 @@
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
     {\
+        .driver   = "kvmclock",\
+        .property = "mach_use_reliable_get_clock",\
+        .value    = "off",\
+    },\
+    {\
         .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\
Index: qemu-mig-advance-clock/target-i386/kvm.c
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm.c	2016-11-14 10:40:39.750116314 -0200
+++ qemu-mig-advance-clock/target-i386/kvm.c	2016-11-14 10:40:48.061128652 -0200
@@ -117,6 +117,13 @@
     return kvm_check_extension(kvm_state, KVM_CAP_X86_SMM);
 }
 
+bool kvm_has_adjust_clock_stable(void)
+{
+    int ret = kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK);
+
+    return (ret == KVM_CLOCK_TSC_STABLE);
+}
+
 bool kvm_allows_irq0_override(void)
 {
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
Index: qemu-mig-advance-clock/target-i386/kvm_i386.h
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm_i386.h	2016-11-14 10:40:39.751116316 -0200
+++ qemu-mig-advance-clock/target-i386/kvm_i386.h	2016-11-14 10:40:48.061128652 -0200
@@ -17,6 +17,7 @@
 
 bool kvm_allows_irq0_override(void);
 bool kvm_has_smm(void);
+bool kvm_has_adjust_clock_stable(void);
 void kvm_synchronize_all_tsc(void);
 void kvm_arch_reset_vcpu(X86CPU *cs);
 void kvm_arch_do_init_vcpu(X86CPU *cs);

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini]

On 14/11/2016 16:40, Marcelo Tosatti wrote:
> static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> {
>     KVMClockState *s = opaque;
> 
>     /*
>      * On machine types that support reliable KVM_GET_CLOCK,
>      * if host kernel does provide reliable KVM_GET_CLOCK,
>      * set src_use_reliable_get_clock=true so that destination
>      * avoids reading kvmclock from memory.
>      */
>     if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
>     {
>         s->src_use_reliable_get_clock = true;
>     }
> 
>     return s->mach_use_reliable_get_clock;
> }
> 
> 
> Ah, OK, done.

s->src_use_reliable_get_clock should not be set with
KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK.

> So s->src_use_reliable_get_clock is only used to indicate 
> to the destination that: "you can use KVM_GET_CLOCK value, 
> its safe".

Yes, we agree.  I was listing all the points, not just those where we
disagree.  Actually I'm not sure where we disagree, except on using
flags from KVM_CHECK_EXTENSION vs. flags from KVM_GET_CLOCK...

Paolo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/11/2016 16:40, Marcelo Tosatti wrote:
> > static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> > {
> >     KVMClockState *s = opaque;
> > 
> >     /*
> >      * On machine types that support reliable KVM_GET_CLOCK,
> >      * if host kernel does provide reliable KVM_GET_CLOCK,
> >      * set src_use_reliable_get_clock=true so that destination
> >      * avoids reading kvmclock from memory.
> >      */
> >     if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
> >     {
> >         s->src_use_reliable_get_clock = true;
> >     }
> > 
> >     return s->mach_use_reliable_get_clock;
> > }
> > 
> > 
> > Ah, OK, done.
> 
> s->src_use_reliable_get_clock should not be set with
> KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK.

Well, thats not right: What matters is the presence of get_kvmclock_ns 
which returns a value that the guest sees. 

                get_kernel_monotonic_clock() + kvmclock_offset +
                (rdtsc() - tsc_timestamp)

IOW what the guest sees. And you changed that in 

commit 108b249c453dd7132599ab6dc7e435a7036c193f
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Thu Sep 1 14:21:03 2016 +0200

    KVM: x86: introduce get_kvmclock_ns

And the correct behaviour (once KVM_GET_CLOCK is fixed per 
previous message to return rdtsc - tsc_timestamp for the 
non masterclock case) depends on this commit above, 
not on masterclock.

> > So s->src_use_reliable_get_clock is only used to indicate 
> > to the destination that: "you can use KVM_GET_CLOCK value, 
> > its safe".
> 
> Yes, we agree.  I was listing all the points, not just those where we
> disagree.  Actually I'm not sure where we disagree, except on using
> flags from KVM_CHECK_EXTENSION vs. flags from KVM_GET_CLOCK...
> 
> Paolo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini]

On 14/11/2016 18:13, Marcelo Tosatti wrote:
> On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 14/11/2016 16:40, Marcelo Tosatti wrote:
>>> static bool kvmclock_src_use_reliable_get_clock(void *opaque)
>>> {
>>>     KVMClockState *s = opaque;
>>>
>>>     /*
>>>      * On machine types that support reliable KVM_GET_CLOCK,
>>>      * if host kernel does provide reliable KVM_GET_CLOCK,
>>>      * set src_use_reliable_get_clock=true so that destination
>>>      * avoids reading kvmclock from memory.
>>>      */
>>>     if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
>>>     {
>>>         s->src_use_reliable_get_clock = true;
>>>     }
>>>
>>>     return s->mach_use_reliable_get_clock;
>>> }
>>>
>>>
>>> Ah, OK, done.
>>
>> s->src_use_reliable_get_clock should not be set with
>> KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK.
> 
> Well, thats not right: What matters is the presence of get_kvmclock_ns 
> which returns a value that the guest sees. 
> 
>                 get_kernel_monotonic_clock() + kvmclock_offset +
>                 (rdtsc() - tsc_timestamp)
> 
> IOW what the guest sees. And you changed that in 
> 
> commit 108b249c453dd7132599ab6dc7e435a7036c193f
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Thu Sep 1 14:21:03 2016 +0200
> 
>     KVM: x86: introduce get_kvmclock_ns
> 
> And the correct behaviour (once KVM_GET_CLOCK is fixed per 
> previous message to return rdtsc - tsc_timestamp for the 
> non masterclock case) depends on this commit above, 
> not on masterclock.

This commit in turn only gets the correct behavior if 
"vcpu->arch.hv_clock.flags & PVCLOCK_TSC_STABLE_BIT" (and it will be 
changed soon to ka->use_masterclock).  KVM_CHECK_EXTENSION can still 
return KVM_CLOCK_TSC_STABLE even if the masterclock is disabled, 
because KVM_CHECK_EXTENSION only tells you which flags are known for
this version of the KVM module.

To see if the masterclock is enabled _now_, you need to check what
KVM_GET_CLOCK sets in the flags.  From the KVM_CLOCK_TSC_STABLE patch:

		user_ns.flags = kvm->arch.use_master_clock ? KVM_CLOCK_TSC_STABLE : 0;

Thanks,

Paolo

>>> So s->src_use_reliable_get_clock is only used to indicate 
>>> to the destination that: "you can use KVM_GET_CLOCK value, 
>>> its safe".
>>
>> Yes, we agree.  I was listing all the points, not just those where we
>> disagree.  Actually I'm not sure where we disagree, except on using
>> flags from KVM_CHECK_EXTENSION vs. flags from KVM_GET_CLOCK...
>>
>> Paolo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

On Mon, Nov 14, 2016 at 06:20:29PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/11/2016 18:13, Marcelo Tosatti wrote:
> > On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 14/11/2016 16:40, Marcelo Tosatti wrote:
> >>> static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> >>> {
> >>>     KVMClockState *s = opaque;
> >>>
> >>>     /*
> >>>      * On machine types that support reliable KVM_GET_CLOCK,
> >>>      * if host kernel does provide reliable KVM_GET_CLOCK,
> >>>      * set src_use_reliable_get_clock=true so that destination
> >>>      * avoids reading kvmclock from memory.
> >>>      */
> >>>     if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
> >>>     {
> >>>         s->src_use_reliable_get_clock = true;
> >>>     }
> >>>
> >>>     return s->mach_use_reliable_get_clock;
> >>> }
> >>>
> >>>
> >>> Ah, OK, done.
> >>
> >> s->src_use_reliable_get_clock should not be set with
> >> KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK.
> > 
> > Well, thats not right: What matters is the presence of get_kvmclock_ns 
> > which returns a value that the guest sees. 
> > 
> >                 get_kernel_monotonic_clock() + kvmclock_offset +
> >                 (rdtsc() - tsc_timestamp)
> > 
> > IOW what the guest sees. And you changed that in 
> > 
> > commit 108b249c453dd7132599ab6dc7e435a7036c193f
> > Author: Paolo Bonzini <pbonzini@redhat.com>
> > Date:   Thu Sep 1 14:21:03 2016 +0200
> > 
> >     KVM: x86: introduce get_kvmclock_ns
> > 
> > And the correct behaviour (once KVM_GET_CLOCK is fixed per 
> > previous message to return rdtsc - tsc_timestamp for the 
> > non masterclock case) depends on this commit above, 
> > not on masterclock.
> 
> This commit in turn only gets the correct behavior if 
> "vcpu->arch.hv_clock.flags & PVCLOCK_TSC_STABLE_BIT" (and it will be 
> changed soon to ka->use_masterclock).  KVM_CHECK_EXTENSION can still 
> return KVM_CLOCK_TSC_STABLE even if the masterclock is disabled, 
> because KVM_CHECK_EXTENSION only tells you which flags are known for
> this version of the KVM module.

What QEMU wants is to use KVM_GET_CLOCK at pre_save independently
of whether masterclock is enabled or not... it just depends
on KVM_GET_CLOCK being correct for the masterclock case
(108b249c453dd7132599ab6dc7e435a7036c193f).

So a "reliable KVM_GET_CLOCK" (that does not timebackward
when masterclock is enabled) is much simpler to userspace
than "whether masterclock is enabled or not".

If you have a reason why that should not be the case,
let me know.

> To see if the masterclock is enabled _now_, you need to check what
> KVM_GET_CLOCK sets in the flags.  From the KVM_CLOCK_TSC_STABLE patch:
> 
> 		user_ns.flags = kvm->arch.use_master_clock ? KVM_CLOCK_TSC_STABLE : 0;

Again, whether masterclock is enable is independent of 
being able to use KVM_GET_CLOCK at pre_save.

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

On Mon, Nov 14, 2016 at 04:15:18PM -0200, Marcelo Tosatti wrote:
> On Mon, Nov 14, 2016 at 06:20:29PM +0100, Paolo Bonzini wrote:
> > 
> > 
> > On 14/11/2016 18:13, Marcelo Tosatti wrote:
> > > On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote:
> > >>
> > >>
> > >> On 14/11/2016 16:40, Marcelo Tosatti wrote:
> > >>> static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> > >>> {
> > >>>     KVMClockState *s = opaque;
> > >>>
> > >>>     /*
> > >>>      * On machine types that support reliable KVM_GET_CLOCK,
> > >>>      * if host kernel does provide reliable KVM_GET_CLOCK,
> > >>>      * set src_use_reliable_get_clock=true so that destination
> > >>>      * avoids reading kvmclock from memory.
> > >>>      */
> > >>>     if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
> > >>>     {
> > >>>         s->src_use_reliable_get_clock = true;
> > >>>     }
> > >>>
> > >>>     return s->mach_use_reliable_get_clock;
> > >>> }
> > >>>
> > >>>
> > >>> Ah, OK, done.
> > >>
> > >> s->src_use_reliable_get_clock should not be set with
> > >> KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK.
> > > 
> > > Well, thats not right: What matters is the presence of get_kvmclock_ns 
> > > which returns a value that the guest sees. 
> > > 
> > >                 get_kernel_monotonic_clock() + kvmclock_offset +
> > >                 (rdtsc() - tsc_timestamp)
> > > 
> > > IOW what the guest sees. And you changed that in 
> > > 
> > > commit 108b249c453dd7132599ab6dc7e435a7036c193f
> > > Author: Paolo Bonzini <pbonzini@redhat.com>
> > > Date:   Thu Sep 1 14:21:03 2016 +0200
> > > 
> > >     KVM: x86: introduce get_kvmclock_ns
> > > 
> > > And the correct behaviour (once KVM_GET_CLOCK is fixed per 
> > > previous message to return rdtsc - tsc_timestamp for the 
> > > non masterclock case) depends on this commit above, 
> > > not on masterclock.
> > 
> > This commit in turn only gets the correct behavior if 
> > "vcpu->arch.hv_clock.flags & PVCLOCK_TSC_STABLE_BIT" (and it will be 
> > changed soon to ka->use_masterclock).  KVM_CHECK_EXTENSION can still 
> > return KVM_CLOCK_TSC_STABLE even if the masterclock is disabled, 
> > because KVM_CHECK_EXTENSION only tells you which flags are known for
> > this version of the KVM module.
> 
> What QEMU wants is to use KVM_GET_CLOCK at pre_save independently
> of whether masterclock is enabled or not... it just depends
> on KVM_GET_CLOCK being correct for the masterclock case
> (108b249c453dd7132599ab6dc7e435a7036c193f).
> 
> So a "reliable KVM_GET_CLOCK" (that does not timebackward
> when masterclock is enabled) is much simpler to userspace
> than "whether masterclock is enabled or not".
> 
> If you have a reason why that should not be the case,
> let me know.
> 
> > To see if the masterclock is enabled _now_, you need to check what
> > KVM_GET_CLOCK sets in the flags.  From the KVM_CLOCK_TSC_STABLE patch:
> > 
> > 		user_ns.flags = kvm->arch.use_master_clock ? KVM_CLOCK_TSC_STABLE : 0;
> 
> Again, whether masterclock is enable is independent of 
> being able to use KVM_GET_CLOCK at pre_save.

Is this point OK ?

Using 

                break;
+   case KVM_CAP_ADJUST_CLOCK:
+               r = KVM_CLOCK_TSC_STABLE;
+               break;

To infer whether KVM_GET_CLOCK is fixed for the monotonic case.

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini]

On 17/11/2016 13:16, Marcelo Tosatti wrote:
> On Mon, Nov 14, 2016 at 04:15:18PM -0200, Marcelo Tosatti wrote:
>> On Mon, Nov 14, 2016 at 06:20:29PM +0100, Paolo Bonzini wrote:
>>>
>>>
>>> On 14/11/2016 18:13, Marcelo Tosatti wrote:
>>>> On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 14/11/2016 16:40, Marcelo Tosatti wrote:
>>>>>> static bool kvmclock_src_use_reliable_get_clock(void *opaque)
>>>>>> {
>>>>>>     KVMClockState *s = opaque;
>>>>>>
>>>>>>     /*
>>>>>>      * On machine types that support reliable KVM_GET_CLOCK,
>>>>>>      * if host kernel does provide reliable KVM_GET_CLOCK,
>>>>>>      * set src_use_reliable_get_clock=true so that destination
>>>>>>      * avoids reading kvmclock from memory.
>>>>>>      */
>>>>>>     if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
>>>>>>     {
>>>>>>         s->src_use_reliable_get_clock = true;
>>>>>>     }
>>>>>>
>>>>>>     return s->mach_use_reliable_get_clock;
>>>>>> }
>>>>>>
>>>>>>
>>>>>> Ah, OK, done.
>>>>>
>>>>> s->src_use_reliable_get_clock should not be set with
>>>>> KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK.
>>>>
>>>> Well, thats not right: What matters is the presence of get_kvmclock_ns 
>>>> which returns a value that the guest sees. 
>>>>
>>>>                 get_kernel_monotonic_clock() + kvmclock_offset +
>>>>                 (rdtsc() - tsc_timestamp)
>>>>
>>>> IOW what the guest sees. And you changed that in 
>>>>
>>>> commit 108b249c453dd7132599ab6dc7e435a7036c193f
>>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>>> Date:   Thu Sep 1 14:21:03 2016 +0200
>>>>
>>>>     KVM: x86: introduce get_kvmclock_ns
>>>>
>>>> And the correct behaviour (once KVM_GET_CLOCK is fixed per 
>>>> previous message to return rdtsc - tsc_timestamp for the 
>>>> non masterclock case) depends on this commit above, 
>>>> not on masterclock.
>>>
>>> This commit in turn only gets the correct behavior if 
>>> "vcpu->arch.hv_clock.flags & PVCLOCK_TSC_STABLE_BIT" (and it will be 
>>> changed soon to ka->use_masterclock).  KVM_CHECK_EXTENSION can still 
>>> return KVM_CLOCK_TSC_STABLE even if the masterclock is disabled, 
>>> because KVM_CHECK_EXTENSION only tells you which flags are known for
>>> this version of the KVM module.
>>
>> What QEMU wants is to use KVM_GET_CLOCK at pre_save independently
>> of whether masterclock is enabled or not... it just depends
>> on KVM_GET_CLOCK being correct for the masterclock case
>> (108b249c453dd7132599ab6dc7e435a7036c193f).
>>
>> So a "reliable KVM_GET_CLOCK" (that does not timebackward
>> when masterclock is enabled) is much simpler to userspace
>> than "whether masterclock is enabled or not".
>>
>> If you have a reason why that should not be the case,
>> let me know.
>>
>>> To see if the masterclock is enabled _now_, you need to check what
>>> KVM_GET_CLOCK sets in the flags.  From the KVM_CLOCK_TSC_STABLE patch:
>>>
>>> 		user_ns.flags = kvm->arch.use_master_clock ? KVM_CLOCK_TSC_STABLE : 0;
>>
>> Again, whether masterclock is enable is independent of 
>> being able to use KVM_GET_CLOCK at pre_save.
> 
> Is this point OK ?
> 
> Using 
> 
>                 break;
> +   case KVM_CAP_ADJUST_CLOCK:
> +               r = KVM_CLOCK_TSC_STABLE;
> +               break;
> 
> To infer whether KVM_GET_CLOCK is fixed for the monotonic case.

Yes, I still haven't digested why it is correct (I need to read again
what you wrote), but it is indeed correct to use KVM_CAP_ADJUST_CLOCK
this way.

Paolo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini]

On 17/11/2016 13:16, Marcelo Tosatti wrote:
> What QEMU wants is to use KVM_GET_CLOCK at pre_save independently
> of whether masterclock is enabled or not... it just depends
> on KVM_GET_CLOCK being correct for the masterclock case
> (108b249c453dd7132599ab6dc7e435a7036c193f).
> 
> So a "reliable KVM_GET_CLOCK" (that does not timebackward
> when masterclock is enabled) is much simpler to userspace
> than "whether masterclock is enabled or not".
> 
> If you have a reason why that should not be the case,
> let me know.

I still cannot understand this case.

If the source has masterclock _disabled_, shouldn't it read kvmclock 
from memory?  But that's not what your patch does.  To be clear, what
I mean is:

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 653b0b4..6f6e2dc 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -97,6 +97,7 @@ static uint64_t kvm_get_clock(void)
         fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
                 abort();
     }
+    s->src_use_reliable_get_clock = data.flags & KVM_CLOCK_TSC_STABLE;
     return data.clock;
 }
 
@@ -110,34 +111,19 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t pvclock_via_mem = 0;
 
-        /* local (running VM) restore */
-        if (s->clock_valid) {
-            /*
-             * if host does not support reliable KVM_GET_CLOCK,
-             * read kvmclock value from memory
-             */
-            if (!kvm_has_adjust_clock_stable()) {
-                pvclock_via_mem = kvmclock_current_nsec(s);
-            }
-        /* migration/savevm/init restore */
-        } else {
-            /*
-             * use s->clock in case machine uses reliable
-             * get clock and source host supported
-             * reliable get clock
-             */
-            if (!s->src_use_reliable_get_clock) {
-                pvclock_via_mem = kvmclock_current_nsec(s);
+        /*
+         * if last KVM_GET_CLOCK did not retrieve a reliable value,
+         * we can't rely on the saved clock value.  Just discard it and
+         * read kvmclock value from memory
+         */
+        if (!s->src_use_reliable_get_clock) {
+            uint64_t pvclock_via_mem = kvmclock_current_nsec(s);
+            if (pvclock_via_mem) {
+                s->clock = pvclock_via_mem;
             }
         }
 
-        /* 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;
 
         data.clock = s->clock;
@@ -181,16 +168,6 @@ static void kvmclock_realize(DeviceState *dev, Error **errp)
 {
     KVMClockState *s = KVM_CLOCK(dev);
 
-    /*
-     * On machine types that support reliable KVM_GET_CLOCK,
-     * if host kernel does provide reliable KVM_GET_CLOCK,
-     * set src_use_reliable_get_clock=true so that destination
-     * avoids reading kvmclock from memory.
-     */
-    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
-        s->src_use_reliable_get_clock = true;
-    }
-
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Eduardo Habkost]

On Mon, Nov 28, 2016 at 02:47:18PM +0100, Paolo Bonzini wrote:
> 
> 
> On 17/11/2016 13:16, Marcelo Tosatti wrote:
> > What QEMU wants is to use KVM_GET_CLOCK at pre_save independently
> > of whether masterclock is enabled or not... it just depends
> > on KVM_GET_CLOCK being correct for the masterclock case
> > (108b249c453dd7132599ab6dc7e435a7036c193f).
> > 
> > So a "reliable KVM_GET_CLOCK" (that does not timebackward
> > when masterclock is enabled) is much simpler to userspace
> > than "whether masterclock is enabled or not".
> > 
> > If you have a reason why that should not be the case,
> > let me know.
> 
> I still cannot understand this case.
> 
> If the source has masterclock _disabled_, shouldn't it read kvmclock 
> from memory?  But that's not what your patch does.  To be clear, what
> I mean is:
> 
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 653b0b4..6f6e2dc 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -97,6 +97,7 @@ static uint64_t kvm_get_clock(void)
>          fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
>                  abort();
>      }
> +    s->src_use_reliable_get_clock = data.flags & KVM_CLOCK_TSC_STABLE;

I still don't understand the reasoning behind
kvm_has_adjust_clock_stable() vs (flags & KVM_CLOCK_TSC_STABLE),
but on either case, updating src_use_reliable_get_clock inside
kvm_get_clock() looks like the right thing to do.

>      return data.clock;
>  }
>  
> @@ -110,34 +111,19 @@ static void kvmclock_vm_state_change(void *opaque, int running,
>  
>      if (running) {
>          struct kvm_clock_data data = {};
> -        uint64_t pvclock_via_mem = 0;
>  
> -        /* local (running VM) restore */
> -        if (s->clock_valid) {
> -            /*
> -             * if host does not support reliable KVM_GET_CLOCK,
> -             * read kvmclock value from memory
> -             */
> -            if (!kvm_has_adjust_clock_stable()) {
> -                pvclock_via_mem = kvmclock_current_nsec(s);
> -            }
> -        /* migration/savevm/init restore */
> -        } else {
> -            /*
> -             * use s->clock in case machine uses reliable
> -             * get clock and source host supported
> -             * reliable get clock
> -             */
> -            if (!s->src_use_reliable_get_clock) {
> -                pvclock_via_mem = kvmclock_current_nsec(s);
> +        /*
> +         * if last KVM_GET_CLOCK did not retrieve a reliable value,
> +         * we can't rely on the saved clock value.  Just discard it and
> +         * read kvmclock value from memory
> +         */
> +        if (!s->src_use_reliable_get_clock) {
> +            uint64_t pvclock_via_mem = kvmclock_current_nsec(s);
> +            if (pvclock_via_mem) {
> +                s->clock = pvclock_via_mem;
>              }
>          }

This is equivalent to the logic I suggested on my reply to v3.
Much simpler.

>  
> -        /* 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;
>  
>          data.clock = s->clock;
> @@ -181,16 +168,6 @@ static void kvmclock_realize(DeviceState *dev, Error **errp)
>  {
>      KVMClockState *s = KVM_CLOCK(dev);
>  
> -    /*
> -     * On machine types that support reliable KVM_GET_CLOCK,
> -     * if host kernel does provide reliable KVM_GET_CLOCK,
> -     * set src_use_reliable_get_clock=true so that destination
> -     * avoids reading kvmclock from memory.
> -     */
> -    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
> -        s->src_use_reliable_get_clock = true;
> -    }
> -
>      qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
>  }
>  

-- 
Eduardo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini]

On 28/11/2016 15:28, Eduardo Habkost wrote:
> > +    s->src_use_reliable_get_clock = data.flags & KVM_CLOCK_TSC_STABLE;
> 
> I still don't understand the reasoning behind
> kvm_has_adjust_clock_stable() vs (flags & KVM_CLOCK_TSC_STABLE),
> but on either case, updating src_use_reliable_get_clock inside
> kvm_get_clock() looks like the right thing to do.

There are three possibility: the kernel tells you the clock is stable,
the kernel tells you the clock is unstable, the kernel is too old and
doesn't tell you anything.  Then:

    kvm_has_adjust_clock_stable() == true:
	if the clock is stable, KVM_CLOCK_TSC_STABLE will be set in "flags"
	if the clock is unstable, KVM_CLOCK_TSC_STABLE will be unset

    kvm_has_adjust_clock_stable() == false:
	you cannot know if the clock is stable

Paolo

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

On Mon, Nov 28, 2016 at 02:47:18PM +0100, Paolo Bonzini wrote:
> 
> 
> On 17/11/2016 13:16, Marcelo Tosatti wrote:
> > What QEMU wants is to use KVM_GET_CLOCK at pre_save independently
> > of whether masterclock is enabled or not... it just depends
> > on KVM_GET_CLOCK being correct for the masterclock case
> > (108b249c453dd7132599ab6dc7e435a7036c193f).
> > 
> > So a "reliable KVM_GET_CLOCK" (that does not timebackward
> > when masterclock is enabled) is much simpler to userspace
> > than "whether masterclock is enabled or not".
> > 
> > If you have a reason why that should not be the case,
> > let me know.
> 
> I still cannot understand this case.
> 
> If the source has masterclock _disabled_, shouldn't it read kvmclock 
> from memory?  

If the source masterclock is disabled, then the guest does
not enable the optimization to not use a global variable 
to guarantee monotonicity. Therefore there will be no 
time backwards events (the timer backwards events crashed 
guests, and are the reason for reading from guest memory).

So if there are no flaws in the reasoning above, 
no, there is no need to read from memory if 
masterclock is disabled.

Can you state the reasons why you think it should be enabled?

> But that's not what your patch does.  

Its on purpose with the data available.

> To be clear, what
> I mean is:
> 
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 653b0b4..6f6e2dc 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -97,6 +97,7 @@ static uint64_t kvm_get_clock(void)
>          fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
>                  abort();
>      }
> +    s->src_use_reliable_get_clock = data.flags & KVM_CLOCK_TSC_STABLE;
>      return data.clock;
>  }
>  
> @@ -110,34 +111,19 @@ static void kvmclock_vm_state_change(void *opaque, int running,
>  
>      if (running) {
>          struct kvm_clock_data data = {};
> -        uint64_t pvclock_via_mem = 0;
>  
> -        /* local (running VM) restore */
> -        if (s->clock_valid) {
> -            /*
> -             * if host does not support reliable KVM_GET_CLOCK,
> -             * read kvmclock value from memory
> -             */
> -            if (!kvm_has_adjust_clock_stable()) {
> -                pvclock_via_mem = kvmclock_current_nsec(s);
> -            }
> -        /* migration/savevm/init restore */
> -        } else {
> -            /*
> -             * use s->clock in case machine uses reliable
> -             * get clock and source host supported
> -             * reliable get clock
> -             */
> -            if (!s->src_use_reliable_get_clock) {
> -                pvclock_via_mem = kvmclock_current_nsec(s);
> +        /*
> +         * if last KVM_GET_CLOCK did not retrieve a reliable value,
> +         * we can't rely on the saved clock value.  Just discard it and
> +         * read kvmclock value from memory
> +         */
> +        if (!s->src_use_reliable_get_clock) {
> +            uint64_t pvclock_via_mem = kvmclock_current_nsec(s);
> +            if (pvclock_via_mem) {
> +                s->clock = pvclock_via_mem;
>              }
>          }
>  
> -        /* 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;
>  
>          data.clock = s->clock;
> @@ -181,16 +168,6 @@ static void kvmclock_realize(DeviceState *dev, Error **errp)
>  {
>      KVMClockState *s = KVM_CLOCK(dev);
>  
> -    /*
> -     * On machine types that support reliable KVM_GET_CLOCK,
> -     * if host kernel does provide reliable KVM_GET_CLOCK,
> -     * set src_use_reliable_get_clock=true so that destination
> -     * avoids reading kvmclock from memory.
> -     */
> -    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
> -        s->src_use_reliable_get_clock = true;
> -    }
> -
>      qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
>  }
>  

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini]

On 28/11/2016 17:36, Marcelo Tosatti wrote:
> If the source masterclock is disabled, then the guest does
> not enable the optimization to not use a global variable 
> to guarantee monotonicity. Therefore there will be no 
> time backwards events (the timer backwards events crashed 
> guests, and are the reason for reading from guest memory).
> 
> So if there are no flaws in the reasoning above, 
> no, there is no need to read from memory if 
> masterclock is disabled.

Yeah, the reasoning is sound.  So you go from what Eduardo and I were
thinking:

    if last KVM_GET_CLOCK was not reliable then
        read from memory

to this:

    if last KVM_GET_CLOCK was not reliable && masterclock is enabled
        read from memory

but:

- on an old kernel, the left side is always true and the right side is
unknown (so we must assume it's true and read from memory)

- on a new kernel, the two sides of the "&&" are exactly the opposite,
so the result is always false

and then it becomes

    if old kernel then
        read from memory

Got it finally. :)

Paolo

> Can you state the reasons why you think it should be enabled?
> 

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 14/11/2016 13:36, Marcelo Tosatti wrote:
>> +        /* local (running VM) restore */
>> +        if (s->clock_valid) {
>> +            /*
>> +             * if host does not support reliable KVM_GET_CLOCK,
>> +             * read kvmclock value from memory
>> +             */
>> +            if (!kvm_has_adjust_clock_stable()) {
>> +                time_at_migration = kvmclock_current_nsec(s);
>
> Just assign to s->clock here...

Agreed, I just wanted to make so many changes.


> Also, just another small nit: please make your scripts use the "-p"
> option on diff. :)

YESSSS

I was searching what functions the code belonged for :p

Thanks, Juan.

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Juan Quintela]

Marcelo Tosatti <mtosatti@redhat.com> wrote:
> Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which
> indicates that KVM_GET_CLOCK returns a value as seen by the guest at
> that moment.
>
> For new machine types, use this value rather than reading 
> from guest memory.
>
> This reduces kvmclock difference on migration from 5s to 0.1s
> (when max_downtime == 5s).
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Acked-by: Juan Quintela <quintela@redhat.com>

But, if you have to respin it ....


> +    /* whether machine supports reliable KVM_GET_CLOCK */
> +    bool mach_use_reliable_get_clock;
> +
> +    /* whether source host supported reliable KVM_GET_CLOCK */
> +    bool src_use_reliable_get_clock;

This two names are really long, but I don't have better suggesitons :-()
>      if (running) {
>          struct kvm_clock_data data = {};
> -        uint64_t time_at_migration = kvmclock_current_nsec(s);
> +        uint64_t time_at_migration = 0;

This was not "time_at_migration", it was not already before, but just
now it looks really weird. (as it was already faulty, this is why it is
only a suggestion.)

>  
> -        s->clock_valid = false;
> +        /* local (running VM) restore */
> +        if (s->clock_valid) {
> +            /*
> +             * if host does not support reliable KVM_GET_CLOCK,
> +             * read kvmclock value from memory
> +             */
> +            if (!kvm_has_adjust_clock_stable()) {
> +                time_at_migration = kvmclock_current_nsec(s);
> +            }
> +        /* migration/savevm/init restore */
> +        } else {
> +            /*
> +             * use s->clock in case machine uses reliable
> +             * get clock and host where vm was executing
> +             * supported reliable get clock
> +             */

This comment is just weird.  Simplifying
    /* If A and B do C */
    if (!A and || !B) {
       then D();
    }

Doing the opposite comment?

Migration code looks rigth.

Once said that, I continue hating clocks.

Later, Juan.

Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

On Mon, Nov 14, 2016 at 03:09:46PM +0100, Juan Quintela wrote:
> Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which
> > indicates that KVM_GET_CLOCK returns a value as seen by the guest at
> > that moment.
> >
> > For new machine types, use this value rather than reading 
> > from guest memory.
> >
> > This reduces kvmclock difference on migration from 5s to 0.1s
> > (when max_downtime == 5s).
> >
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Acked-by: Juan Quintela <quintela@redhat.com>
> 
> But, if you have to respin it ....
> 
> 
> > +    /* whether machine supports reliable KVM_GET_CLOCK */
> > +    bool mach_use_reliable_get_clock;
> > +
> > +    /* whether source host supported reliable KVM_GET_CLOCK */
> > +    bool src_use_reliable_get_clock;
> 
> This two names are really long, but I don't have better suggesitons :-()
> >      if (running) {
> >          struct kvm_clock_data data = {};
> > -        uint64_t time_at_migration = kvmclock_current_nsec(s);
> > +        uint64_t time_at_migration = 0;
> 
> This was not "time_at_migration", it was not already before, but just
> now it looks really weird. (as it was already faulty, this is why it is
> only a suggestion.)
> 
> >  
> > -        s->clock_valid = false;
> > +        /* local (running VM) restore */
> > +        if (s->clock_valid) {
> > +            /*
> > +             * if host does not support reliable KVM_GET_CLOCK,
> > +             * read kvmclock value from memory
> > +             */
> > +            if (!kvm_has_adjust_clock_stable()) {
> > +                time_at_migration = kvmclock_current_nsec(s);
> > +            }
> > +        /* migration/savevm/init restore */
> > +        } else {
> > +            /*
> > +             * use s->clock in case machine uses reliable
> > +             * get clock and host where vm was executing
> > +             * supported reliable get clock
> > +             */
> 
> This comment is just weird.  Simplifying
>     /* If A and B do C */
>     if (!A and || !B) {
>        then D();
>     }
> 
> Doing the opposite comment?
> 
> Migration code looks rigth.

Fixed those.

> Once said that, I continue hating clocks.

Me too, especially the biological one.