[Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Sebastian Tanase]

The icount option already implemented in QEMU allows the guest to run at a theoretical
frequency of 1/(2^N) GHz (N is the icount parameter). The goal of this patch is to have a
real guest frequency close to the one imposed by using the icount option.

The main idea behind the algorithm is that we compare the virtual monotonic clock and the
host monotonic clock. For big icounts (on our test machine, an i5 CPU @ 3.10GHz, icounts
starting at 6) the guest clock will be ahead of the host clock. In this case, we try to
sleep QEMU for the difference between the 2 clocks. Therefore, the guest would have
executed for a period almost equally to the one imposed by icount. We should point out
that the algorithm works only for those icounts that allow the guest clock to be in front
of the host clock.

The first patch adds support for QemuOpts for the 'icount' parameter. It also adds a
suboption called 'shift' that will hold the value for 'icount'. Therefore we now have
-icount shift=N|auto or -icount N|auto.

The second patch adds the 'align' suboption for icount.

The third patch exports 'icount_time_shift' so that it can be used in places other than
cpus.c; we need it in cpu-exec.c for calculating for how long we want QEMU to sleep.

The forth patch implements the algorithm used for calculating the delay we want to sleep.
It uses the number of instructions executed by the virtual cpu and also 'icount_time_shift'.

The fifth patch prints to the console whenever the guest clock runs behind the host
clock. The fastest printing speed is every 2 seconds, and we only print if the align option
is enabled. We also have a limit to 100 printed messages.

The sixth patch adds information about the difference between the host and guest clocks
(taking into account the offset) in the 'info jit' command. We also print the maximum
delay and advance of the guest clock compared to the host clock.

v3 -> v4

* Add better error handling for 'strtol' in patch 2
* Add 'Sleep' instead of 'nanosleep' for Windows hosts in patch 4
* Remove function pointers from patches 4 and 5

Sebastian Tanase (6):
  icount: Add QemuOpts for icount
  icount: Add align option to icount
  icount: Make icount_time_shift available everywhere
  cpu_exec: Add sleeping algorithm
  cpu_exec: Print to console if the guest is late
  monitor: Add drift info to 'info jit'

 cpu-exec.c            | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++
 cpus.c                |  44 ++++++++++++++--
 include/qemu-common.h |  10 +++-
 monitor.c             |   1 +
 qemu-options.hx       |  17 +++++--
 qtest.c               |  13 ++++-
 vl.c                  |  39 ++++++++++++---
 7 files changed, 241 insertions(+), 18 deletions(-)

-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V4 1/6] icount: Add QemuOpts for icount

[Sebastian Tanase]

Make icount parameter use QemuOpts style options in order
to easily add other suboptions.

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c                | 10 +++++++++-
 include/qemu-common.h |  3 ++-
 qemu-options.hx       |  4 ++--
 qtest.c               | 13 +++++++++++--
 vl.c                  | 35 ++++++++++++++++++++++++++++-------
 5 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/cpus.c b/cpus.c
index 5e7f2cf..dcca96a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -440,13 +440,21 @@ static const VMStateDescription vmstate_timers = {
     }
 };
 
-void configure_icount(const char *option)
+void configure_icount(QemuOpts *opts, Error **errp)
 {
+    const char *option;
+
     seqlock_init(&timers_state.vm_clock_seqlock, NULL);
     vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
+    option = qemu_opt_get(opts, "shift");
     if (!option) {
         return;
     }
+    /* When using -icount shift, the shift option will be
+       misinterpreted as a boolean */
+    if (strcmp(option, "on") == 0 || strcmp(option, "off") == 0) {
+        error_setg(errp, "The shift option must be a number or auto");
+    }
 
     icount_warp_timer = timer_new_ns(QEMU_CLOCK_REALTIME,
                                           icount_warp_rt, NULL);
diff --git a/include/qemu-common.h b/include/qemu-common.h
index ae76197..cc346ec 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -41,6 +41,7 @@
 #include <assert.h>
 #include <signal.h>
 #include "glib-compat.h"
+#include "qemu/option.h"
 
 #ifdef _WIN32
 #include "sysemu/os-win32.h"
@@ -105,7 +106,7 @@ static inline char *realpath(const char *path, char *resolved_path)
 #endif
 
 /* icount */
-void configure_icount(const char *option);
+void configure_icount(QemuOpts *opts, Error **errp);
 extern int use_icount;
 
 #include "qemu/osdep.h"
diff --git a/qemu-options.hx b/qemu-options.hx
index 9e54686..143def4 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3011,11 +3011,11 @@ re-inject them.
 ETEXI
 
 DEF("icount", HAS_ARG, QEMU_OPTION_icount, \
-    "-icount [N|auto]\n" \
+    "-icount [shift=N|auto]\n" \
     "                enable virtual instruction counter with 2^N clock ticks per\n" \
     "                instruction\n", QEMU_ARCH_ALL)
 STEXI
-@item -icount [@var{N}|auto]
+@item -icount [shift=@var{N}|auto]
 @findex -icount
 Enable virtual instruction counter.  The virtual cpu will execute one
 instruction every 2^@var{N} ns of virtual time.  If @code{auto} is specified
diff --git a/qtest.c b/qtest.c
index 04a6dc1..ef0d991 100644
--- a/qtest.c
+++ b/qtest.c
@@ -19,6 +19,9 @@
 #include "hw/irq.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpus.h"
+#include "qemu/config-file.h"
+#include "qemu/option.h"
+#include "qemu/error-report.h"
 
 #define MAX_IRQ 256
 
@@ -509,10 +512,16 @@ static void qtest_event(void *opaque, int event)
     }
 }
 
-int qtest_init_accel(MachineClass *mc)
+static void configure_qtest_icount(const char *options)
 {
-    configure_icount("0");
+    QemuOpts *opts  = qemu_opts_parse(qemu_find_opts("icount"), options, 1);
+    configure_icount(opts, &error_abort);
+    qemu_opts_del(opts);
+}
 
+int qtest_init_accel(MachineClass *mc)
+{
+    configure_qtest_icount("0");
     return 0;
 }
 
diff --git a/vl.c b/vl.c
index 41ddcd2..103027f 100644
--- a/vl.c
+++ b/vl.c
@@ -537,6 +537,20 @@ static QemuOptsList qemu_mem_opts = {
     },
 };
 
+static QemuOptsList qemu_icount_opts = {
+    .name = "icount",
+    .implied_opt_name = "shift",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_icount_opts.head),
+    .desc = {
+        {
+            .name = "shift",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
+
 /**
  * Get machine options
  *
@@ -2896,13 +2910,12 @@ int main(int argc, char **argv, char **envp)
 {
     int i;
     int snapshot, linux_boot;
-    const char *icount_option = NULL;
     const char *initrd_filename;
     const char *kernel_filename, *kernel_cmdline;
     const char *boot_order;
     DisplayState *ds;
     int cyls, heads, secs, translation;
-    QemuOpts *hda_opts = NULL, *opts, *machine_opts;
+    QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
     QemuOptsList *olist;
     int optind;
     const char *optarg;
@@ -2967,6 +2980,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_msg_opts);
     qemu_add_opts(&qemu_name_opts);
     qemu_add_opts(&qemu_numa_opts);
+    qemu_add_opts(&qemu_icount_opts);
 
     runstate_init();
 
@@ -3817,7 +3831,11 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_icount:
-                icount_option = optarg;
+                icount_opts = qemu_opts_parse(qemu_find_opts("icount"),
+                                              optarg, 1);
+                if (!icount_opts) {
+                    exit(1);
+                }
                 break;
             case QEMU_OPTION_incoming:
                 incoming = optarg;
@@ -4294,11 +4312,14 @@ int main(int argc, char **argv, char **envp)
     qemu_spice_init();
 #endif
 
-    if (icount_option && (kvm_enabled() || xen_enabled())) {
-        fprintf(stderr, "-icount is not allowed with kvm or xen\n");
-        exit(1);
+    if (icount_opts) {
+        if (kvm_enabled() || xen_enabled()) {
+            fprintf(stderr, "-icount is not allowed with kvm or xen\n");
+            exit(1);
+        }
+        configure_icount(icount_opts, &error_abort);
+        qemu_opts_del(icount_opts);
     }
-    configure_icount(icount_option);
 
     /* clean up network at qemu process termination */
     atexit(&net_cleanup);
-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V4 2/6] icount: Add align option to icount

[Sebastian Tanase]

The align option is used for activating the align algorithm
in order to synchronise the host clock and the guest clock.

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
---
 cpus.c                | 19 ++++++++++++-------
 include/qemu-common.h |  1 +
 qemu-options.hx       | 15 +++++++++++++--
 vl.c                  |  4 ++++
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/cpus.c b/cpus.c
index dcca96a..34cc4c8 100644
--- a/cpus.c
+++ b/cpus.c
@@ -443,25 +443,30 @@ static const VMStateDescription vmstate_timers = {
 void configure_icount(QemuOpts *opts, Error **errp)
 {
     const char *option;
+    char *rem_str = NULL;
 
     seqlock_init(&timers_state.vm_clock_seqlock, NULL);
     vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
     option = qemu_opt_get(opts, "shift");
     if (!option) {
+        if (qemu_opt_get(opts, "align") != NULL) {
+            error_setg(errp, "Please specify shift option when using align");
+        }
         return;
     }
-    /* When using -icount shift, the shift option will be
-       misinterpreted as a boolean */
-    if (strcmp(option, "on") == 0 || strcmp(option, "off") == 0) {
-        error_setg(errp, "The shift option must be a number or auto");
-    }
-
+    icount_align_option = qemu_opt_get_bool(opts, "align", false);
     icount_warp_timer = timer_new_ns(QEMU_CLOCK_REALTIME,
                                           icount_warp_rt, NULL);
     if (strcmp(option, "auto") != 0) {
-        icount_time_shift = strtol(option, NULL, 0);
+        errno = 0;
+        icount_time_shift = strtol(option, &rem_str, 0);
+        if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
+            error_setg(errp, "icount: Invalid shift value");
+        }
         use_icount = 1;
         return;
+    } else if (icount_align_option) {
+        error_setg(errp, "shift=auto and align=on are incompatible");
     }
 
     use_icount = 2;
diff --git a/include/qemu-common.h b/include/qemu-common.h
index cc346ec..860bb15 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -108,6 +108,7 @@ static inline char *realpath(const char *path, char *resolved_path)
 /* icount */
 void configure_icount(QemuOpts *opts, Error **errp);
 extern int use_icount;
+extern int icount_align_option;
 
 #include "qemu/osdep.h"
 #include "qemu/bswap.h"
diff --git a/qemu-options.hx b/qemu-options.hx
index 143def4..379d932 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3011,9 +3011,9 @@ re-inject them.
 ETEXI
 
 DEF("icount", HAS_ARG, QEMU_OPTION_icount, \
-    "-icount [shift=N|auto]\n" \
+    "-icount [shift=N|auto][,align=on|off]\n" \
     "                enable virtual instruction counter with 2^N clock ticks per\n" \
-    "                instruction\n", QEMU_ARCH_ALL)
+    "                instruction and enable aligning the host and virtual clocks\n", QEMU_ARCH_ALL)
 STEXI
 @item -icount [shift=@var{N}|auto]
 @findex -icount
@@ -3026,6 +3026,17 @@ Note that while this option can give deterministic behavior, it does not
 provide cycle accurate emulation.  Modern CPUs contain superscalar out of
 order cores with complex cache hierarchies.  The number of instructions
 executed often has little or no correlation with actual performance.
+
+@option{align=on} will activate the delay algorithm which will try to
+to synchronise the host clock and the virtual clock. The goal is to
+have a guest running at the real frequency imposed by the shift option.
+Whenever the guest clock is behind the host clock and if
+@option{align=on} is specified then we print a messsage to the user
+to inform about the delay.
+Currently this option does not work when @option{shift} is @code{auto}.
+Note: The sync algorithm will work for those shift values for which
+the guest clock runs ahead of the host clock. Typically this happens
+when the shift value is high (how high depends on the host machine).
 ETEXI
 
 DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
diff --git a/vl.c b/vl.c
index 103027f..d270070 100644
--- a/vl.c
+++ b/vl.c
@@ -183,6 +183,7 @@ uint8_t *boot_splash_filedata;
 size_t boot_splash_filedata_size;
 uint8_t qemu_extra_params_fw[2];
 
+int icount_align_option;
 typedef struct FWBootEntry FWBootEntry;
 
 struct FWBootEntry {
@@ -546,6 +547,9 @@ static QemuOptsList qemu_icount_opts = {
         {
             .name = "shift",
             .type = QEMU_OPT_STRING,
+        }, {
+            .name = "align",
+            .type = QEMU_OPT_BOOL,
         },
         { /* end of list */ }
     },
-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V4 3/6] icount: Make icount_time_shift available everywhere

[Sebastian Tanase]

icount_time_shift is used for calculting the delay
qemu has to sleep in order to synchronise the
host and guest clocks. Therefore, we need it in
cpu-exec.c.

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c                | 8 ++++++--
 include/qemu-common.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 34cc4c8..de0a8b2 100644
--- a/cpus.c
+++ b/cpus.c
@@ -105,8 +105,12 @@ static bool all_cpu_threads_idle(void)
 /* Compensate for varying guest execution speed.  */
 static int64_t qemu_icount_bias;
 static int64_t vm_clock_warp_start;
-/* Conversion factor from emulated instructions to virtual clock ticks.  */
-static int icount_time_shift;
+/* Conversion factor from emulated instructions to virtual clock ticks.
+ * icount_time_shift is defined as extern in include/qemu-common.h because
+ * it is used (in cpu-exec.c) for calculating the delay for sleeping
+ * qemu in order to align the host and virtual clocks.
+ */
+int icount_time_shift;
 /* Arbitrarily pick 1MIPS as the minimum allowable speed.  */
 #define MAX_ICOUNT_SHIFT 10
 
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 860bb15..d47aa02 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -109,6 +109,7 @@ static inline char *realpath(const char *path, char *resolved_path)
 void configure_icount(QemuOpts *opts, Error **errp);
 extern int use_icount;
 extern int icount_align_option;
+extern int icount_time_shift;
 
 #include "qemu/osdep.h"
 #include "qemu/bswap.h"
-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V4 4/6] cpu_exec: Add sleeping algorithm

[Sebastian Tanase]

The goal is to sleep qemu whenever the guest clock
is in advance compared to the host clock (we use
the monotonic clocks). The amount of time to sleep
is calculated in the execution loop in cpu_exec.

At first, we tried to approximate at each for loop the real time elapsed
while searching for a TB (generating or retrieving from cache) and
executing it. We would then approximate the virtual time corresponding
to the number of virtual instructions executed. The difference between
these 2 values would allow us to know if the guest is in advance or delayed.
However, the function used for measuring the real time
(qemu_clock_get_ns(QEMU_CLOCK_REALTIME)) proved to be very expensive.
We had an added overhead of 13% of the total run time.

Therefore, we modified the algorithm and only take into account the
difference between the 2 clocks at the begining of the cpu_exec function.
During the for loop we try to reduce the advance of the guest only by
computing the virtual time elapsed and sleeping if necessary. The overhead
is thus reduced to 3%. Even though this method still has a noticeable
overhead, it no longer is a bottleneck in trying to achieve a better
guest frequency for which the guest clock is faster than the host one.

As for the the alignement of the 2 clocks, with the first algorithm
the guest clock was oscillating between -1 and 1ms compared to the host clock.
Using the second algorithm we notice that the guest is 5ms behind the host, which
is still acceptable for our use case.

The tests where conducted using fio and stress. The host machine in an i5 CPU at
3.10GHz running Debian Jessie (kernel 3.12). The guest machine is an arm versatile-pb
built with buildroot.

Currently, on our test machine, the lowest icount we can achieve that is suitable for
aligning the 2 clocks is 6. However, we observe that the IO tests (using fio) are
slower than the cpu tests (using stress).

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/cpu-exec.c b/cpu-exec.c
index 38e5f02..9ed6ac1 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -22,6 +22,90 @@
 #include "tcg.h"
 #include "qemu/atomic.h"
 #include "sysemu/qtest.h"
+#include "qemu/timer.h"
+
+/* -icount align implementation. */
+
+typedef struct SyncClocks {
+    int64_t diff_clk;
+    int64_t original_instr_counter;
+} SyncClocks;
+
+#if !defined(CONFIG_USER_ONLY)
+/* Allow the guest to have a max 3ms advance.
+ * The difference between the 2 clocks could therefore
+ * oscillate around 0.
+ */
+#define VM_CLOCK_ADVANCE 3000000
+
+static int64_t delay_host(int64_t diff_clk)
+{
+    if (diff_clk > VM_CLOCK_ADVANCE) {
+#ifndef _WIN32
+        struct timespec sleep_delay, rem_delay;
+        sleep_delay.tv_sec = diff_clk / 1000000000LL;
+        sleep_delay.tv_nsec = diff_clk % 1000000000LL;
+        if (nanosleep(&sleep_delay, &rem_delay) < 0) {
+            diff_clk -= (sleep_delay.tv_sec - rem_delay.tv_sec) * 1000000000LL;
+            diff_clk -= sleep_delay.tv_nsec - rem_delay.tv_nsec;
+        } else {
+            diff_clk = 0;
+        }
+#else
+        Sleep(diff_clk / SCALE_MS);
+        diff_clk = 0;
+#endif
+    }
+    return diff_clk;
+}
+
+static int64_t instr_to_vtime(int64_t instr_counter, const CPUState *cpu)
+{
+    int64_t instr_exec_time;
+    instr_exec_time = instr_counter -
+                      (cpu->icount_extra +
+                       cpu->icount_decr.u16.low);
+    instr_exec_time = instr_exec_time << icount_time_shift;
+
+    return instr_exec_time;
+}
+
+static void align_clocks(SyncClocks *sc, const CPUState *cpu)
+{
+    if (!icount_align_option) {
+        return;
+    }
+    sc->diff_clk += instr_to_vtime(sc->original_instr_counter, cpu);
+    sc->original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
+    sc->diff_clk = delay_host(sc->diff_clk);
+}
+
+static void init_delay_params(SyncClocks *sc,
+                              const CPUState *cpu)
+{
+    static int64_t clocks_offset;
+    if (!icount_align_option) {
+        return;
+    }
+    int64_t realtime_clock_value, virtual_clock_value;
+    realtime_clock_value = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    virtual_clock_value = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    /* Compute offset between the 2 clocks. */
+    if (!clocks_offset) {
+        clocks_offset = realtime_clock_value - virtual_clock_value;
+    }
+    sc->diff_clk = virtual_clock_value - realtime_clock_value + clocks_offset;
+    sc->original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
+}
+#else
+static void align_clocks(SyncClocks *sc, const CPUState *cpu)
+{
+}
+
+static void init_delay_params(SyncClocks *sc, const CPUState *cpu)
+{
+}
+#endif /* CONFIG USER ONLY */
 
 void cpu_loop_exit(CPUState *cpu)
 {
@@ -227,6 +311,8 @@ int cpu_exec(CPUArchState *env)
     TranslationBlock *tb;
     uint8_t *tc_ptr;
     uintptr_t next_tb;
+    SyncClocks sc;
+
     /* This must be volatile so it is not trashed by longjmp() */
     volatile bool have_tb_lock = false;
 
@@ -283,6 +369,13 @@ int cpu_exec(CPUArchState *env)
 #endif
     cpu->exception_index = -1;
 
+    /* Calculate difference between guest clock and host clock.
+     * This delay includes the delay of the last cycle, so
+     * what we have to do is sleep until it is 0. As for the
+     * advance/delay we gain here, we try to fix it next time.
+     */
+    init_delay_params(&sc, cpu);
+
     /* prepare setjmp context for exception handling */
     for(;;) {
         if (sigsetjmp(cpu->jmp_env, 0) == 0) {
@@ -672,6 +765,7 @@ int cpu_exec(CPUArchState *env)
                             if (insns_left > 0) {
                                 /* Execute remaining instructions.  */
                                 cpu_exec_nocache(env, insns_left, tb);
+                                align_clocks(&sc, cpu);
                             }
                             cpu->exception_index = EXCP_INTERRUPT;
                             next_tb = 0;
@@ -684,6 +778,9 @@ int cpu_exec(CPUArchState *env)
                     }
                 }
                 cpu->current_tb = NULL;
+                /* Try to align the host and virtual clocks
+                   if the guest is in advance */
+                align_clocks(&sc, cpu);
                 /* reset soft MMU for next block (it can currently
                    only be set by a memory fault) */
             } /* for(;;) */
-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V4 5/6] cpu_exec: Print to console if the guest is late

[Sebastian Tanase]

If the align option is enabled, we print to the user whenever
the guest clock is behind the host clock in order for he/she
to have a hint about the actual performance. The maximum
print interval is 2s and we limit the number of messages to 100.
If desired, this can be changed in cpu-exec.c

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>

Conflicts:
	cpu-exec.c
---
 cpu-exec.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 9ed6ac1..b62df15 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -29,6 +29,7 @@
 typedef struct SyncClocks {
     int64_t diff_clk;
     int64_t original_instr_counter;
+    int64_t realtime_clock;
 } SyncClocks;
 
 #if !defined(CONFIG_USER_ONLY)
@@ -37,6 +38,9 @@ typedef struct SyncClocks {
  * oscillate around 0.
  */
 #define VM_CLOCK_ADVANCE 3000000
+#define THRESHOLD_REDUCE 1.5
+#define MAX_DELAY_PRINT_RATE 2
+#define MAX_NB_PRINTS 100
 
 static int64_t delay_host(int64_t diff_clk)
 {
@@ -80,6 +84,28 @@ static void align_clocks(SyncClocks *sc, const CPUState *cpu)
     sc->diff_clk = delay_host(sc->diff_clk);
 }
 
+static void print_delay(const SyncClocks *sc)
+{
+    static float threshold_delay;
+    static int64_t last_realtime_clock;
+    static int nb_prints;
+
+    if (icount_align_option &&
+        (sc->realtime_clock - last_realtime_clock) / 1000000000LL
+        >= MAX_DELAY_PRINT_RATE && nb_prints < MAX_NB_PRINTS) {
+        if ((-sc->diff_clk / (float)1000000000LL > threshold_delay) ||
+            (-sc->diff_clk / (float)1000000000LL <
+             (threshold_delay - THRESHOLD_REDUCE))) {
+            threshold_delay = (-sc->diff_clk / 1000000000LL) + 1;
+            printf("Warning: The guest is now late by %.1f to %.1f seconds\n",
+                   threshold_delay - 1,
+                   threshold_delay);
+            nb_prints++;
+            last_realtime_clock = sc->realtime_clock;
+        }
+    }
+}
+
 static void init_delay_params(SyncClocks *sc,
                               const CPUState *cpu)
 {
@@ -87,15 +113,18 @@ static void init_delay_params(SyncClocks *sc,
     if (!icount_align_option) {
         return;
     }
-    int64_t realtime_clock_value, virtual_clock_value;
-    realtime_clock_value = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-    virtual_clock_value = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    sc->realtime_clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     /* Compute offset between the 2 clocks. */
     if (!clocks_offset) {
-        clocks_offset = realtime_clock_value - virtual_clock_value;
+        clocks_offset = sc->realtime_clock -
+                        qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     }
-    sc->diff_clk = virtual_clock_value - realtime_clock_value + clocks_offset;
+    sc->diff_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
+                   sc->realtime_clock + clocks_offset;
     sc->original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
+    /* Print every 2s max if the guest is late. We limit the number
+       of printed messages to NB_PRINT_MAX(currently 100) */
+    print_delay(sc);
 }
 #else
 static void align_clocks(SyncClocks *sc, const CPUState *cpu)
-- 
2.0.0.rc2

[Qemu-devel] [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit'

[Sebastian Tanase]

Show in 'info jit' the current delay between the host clock
and the guest clock. In addition, print the maximum advance
and delay of the guest compared to the host.

Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
Tested-by: Camille Bégué <camille.begue@openwide.fr>
---
 cpu-exec.c            | 19 ++++++++++++++-----
 cpus.c                | 17 +++++++++++++++++
 include/qemu-common.h |  5 +++++
 monitor.c             |  1 +
 4 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index b62df15..055e0e3 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -109,19 +109,28 @@ static void print_delay(const SyncClocks *sc)
 static void init_delay_params(SyncClocks *sc,
                               const CPUState *cpu)
 {
-    static int64_t clocks_offset;
-    if (!icount_align_option) {
-        return;
+    static int64_t realtime_clock_value;
+    if (icount_align_option || !realtime_clock_value) {
+        realtime_clock_value = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     }
-    sc->realtime_clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     /* Compute offset between the 2 clocks. */
     if (!clocks_offset) {
-        clocks_offset = sc->realtime_clock -
+        clocks_offset = realtime_clock_value -
                         qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     }
+    if (!icount_align_option) {
+        return;
+    }
+    sc->realtime_clock = realtime_clock_value;
     sc->diff_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
                    sc->realtime_clock + clocks_offset;
     sc->original_instr_counter = cpu->icount_extra + cpu->icount_decr.u16.low;
+    if (sc->diff_clk < max_delay) {
+        max_delay = sc->diff_clk;
+    }
+    if (sc->diff_clk > max_advance) {
+        max_advance = sc->diff_clk;
+    }
     /* Print every 2s max if the guest is late. We limit the number
        of printed messages to NB_PRINT_MAX(currently 100) */
     print_delay(sc);
diff --git a/cpus.c b/cpus.c
index de0a8b2..3c3fb64 100644
--- a/cpus.c
+++ b/cpus.c
@@ -64,6 +64,9 @@
 #endif /* CONFIG_LINUX */
 
 static CPUState *next_cpu;
+int64_t clocks_offset;
+int64_t max_delay;
+int64_t max_advance;
 
 bool cpu_is_stopped(CPUState *cpu)
 {
@@ -1504,3 +1507,17 @@ void qmp_inject_nmi(Error **errp)
     error_set(errp, QERR_UNSUPPORTED);
 #endif
 }
+
+void dump_drift_info(FILE *f, fprintf_function cpu_fprintf)
+{
+    cpu_fprintf(f, "Host - Guest clock  %ld(ms)\n",
+                (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - clocks_offset -
+                 qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL))/SCALE_MS);
+    if (icount_align_option) {
+        cpu_fprintf(f, "Max guest delay     %ld(ms)\n", -max_delay/SCALE_MS);
+        cpu_fprintf(f, "Max guest advance   %ld(ms)\n", max_advance/SCALE_MS);
+    } else {
+        cpu_fprintf(f, "Max guest delay     NA\n");
+        cpu_fprintf(f, "Max guest advance   NA\n");
+    }
+}
diff --git a/include/qemu-common.h b/include/qemu-common.h
index d47aa02..fbc1bca 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -110,6 +110,11 @@ void configure_icount(QemuOpts *opts, Error **errp);
 extern int use_icount;
 extern int icount_align_option;
 extern int icount_time_shift;
+extern int64_t clocks_offset;
+/* drift information for info jit command */
+extern int64_t max_delay;
+extern int64_t max_advance;
+void dump_drift_info(FILE *f, fprintf_function cpu_fprintf);
 
 #include "qemu/osdep.h"
 #include "qemu/bswap.h"
diff --git a/monitor.c b/monitor.c
index 5bc70a6..cdbaa60 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1047,6 +1047,7 @@ static void do_info_registers(Monitor *mon, const QDict *qdict)
 static void do_info_jit(Monitor *mon, const QDict *qdict)
 {
     dump_exec_info((FILE *)mon, monitor_fprintf);
+    dump_drift_info((FILE *)mon, monitor_fprintf);
 }
 
 static void do_info_history(Monitor *mon, const QDict *qdict)
-- 
2.0.0.rc2

Re: [Qemu-devel] [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit'

[Paolo Bonzini]

Il 16/07/2014 14:18, Sebastian Tanase ha scritto:
> -    static int64_t clocks_offset;
> -    if (!icount_align_option) {
> -        return;
> +    static int64_t realtime_clock_value;

Does this really need to be static?

> +    if (icount_align_option || !realtime_clock_value) {
> +        realtime_clock_value = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>      }
> -    sc->realtime_clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>      /* Compute offset between the 2 clocks. */
>      if (!clocks_offset) {
> -        clocks_offset = sc->realtime_clock -
> +        clocks_offset = realtime_clock_value -
>                          qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>      }

Isn't clocks_offset roughly the same as -timers_state.cpu_clock_offset? 
  If so, you could be some simplification in the code.  Feel free to 
move the TimersState struct definition to include/sysemu/cpus.h and make 
timers_state public.

> +    cpu_fprintf(f, "Host - Guest clock  %ld(ms)\n",
> +                (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - clocks_offset -
> +                 qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL))/SCALE_MS);

I think this is (cpu_get_clock() - cpu_get_icount()) / SCALE_MS.

Paolo

Re: [Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Paolo Bonzini]

Il 16/07/2014 14:18, Sebastian Tanase ha scritto:
> v3 -> v4
>
> * Add better error handling for 'strtol' in patch 2
> * Add 'Sleep' instead of 'nanosleep' for Windows hosts in patch 4
> * Remove function pointers from patches 4 and 5

Hi Sebastian, I think we're getting really close.

I asked a question about clocks_offset; I think it's not necessary to 
compute it in cpu-exec.c because the same information is available 
elsewhere.  It is also probably not necessary to make it public.  Can 
you please check this?  Once this is sorted out, the patch should be 
ready for inclusion in 2.2.

Thanks for your effort!

Paolo

Re: [Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Luiz Capitulino]

On Wed, 16 Jul 2014 14:18:00 +0200
Sebastian Tanase <sebastian.tanase@openwide.fr> wrote:

> The icount option already implemented in QEMU allows the guest to run at a theoretical
> frequency of 1/(2^N) GHz (N is the icount parameter). The goal of this patch is to have a
> real guest frequency close to the one imposed by using the icount option.

Is this really an RFC?

Re: [Qemu-devel] [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit'

[Sebastian Tanase]

----- Mail original -----
> De: "Paolo Bonzini" <pbonzini@redhat.com>
> À: "Sebastian Tanase" <sebastian.tanase@openwide.fr>, qemu-devel@nongnu.org
> Cc: aliguori@amazon.com, afaerber@suse.de, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>,
> michael@walle.cc, alex@alex.org.uk, stefanha@redhat.com, lcapitulino@redhat.com, crobinso@redhat.com,
> armbru@redhat.com, wenchaoqemu@gmail.com, quintela@redhat.com, kwolf@redhat.com, mst@redhat.com, "camille begue"
> <camille.begue@openwide.fr>
> Envoyé: Mercredi 16 Juillet 2014 15:18:33
> Objet: Re: [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit'
> 
> Il 16/07/2014 14:18, Sebastian Tanase ha scritto:
> > -    static int64_t clocks_offset;
> > -    if (!icount_align_option) {
> > -        return;
> > +    static int64_t realtime_clock_value;
> 
> Does this really need to be static?
> 
> > +    if (icount_align_option || !realtime_clock_value) {
> > +        realtime_clock_value =
> > qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> >      }
> > -    sc->realtime_clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> >      /* Compute offset between the 2 clocks. */
> >      if (!clocks_offset) {
> > -        clocks_offset = sc->realtime_clock -
> > +        clocks_offset = realtime_clock_value -
> >                          qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> >      }
> 
> Isn't clocks_offset roughly the same as
> -timers_state.cpu_clock_offset?
>   If so, you could be some simplification in the code.  Feel free to
> move the TimersState struct definition to include/sysemu/cpus.h and
> make
> timers_state public.


-timers_state.cpu_clock_offset contains the offset between the real and virtual clocks.
However, when using the value of the virtual clock (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)),
qemu_icount_bias already includes this offset because, on ARM, qemu_clock_warp (which 
then calls icount_warp_rt) is called for the first time in tcg_exec_all, making
qemu_icount_bias take the value of qemu_clock_get_ns(QEMU_CLOCK_REALTIME)

    static void icount_warp_rt(void *opaque) {
        if (atomic_read(&vm_clock_warp_start) == -1) {
            return;
        }

        seqlock_write_lock(&timers_state.vm_clock_seqlock);
        if (runstate_is_running()) {
            int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
            int64_t warp_delta;
            ...
            warp_delta = clock - vm_clock_warp_start; //vm_clock_warp_start is 0 the very first time
            qemu_icount_bias += warp_delta;
        }
        vm_clock_warp_start = -1;
        seqlock_write_unlock(&timers_state.vm_clock_seqlock);

        if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
            qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
        }
    }


Also, the first time qemu_clock_warp -> icount_warp_rt are called (on ARM), qemu_clock_expired(QEMU_CLOCK_VIRTUAL) 
is false because there are no active timers on the vm clock timer list; I'll explain why I bring this up
below.

A solution to not compute the initial offset in qemu_icount_bias would be to initialize 
vm_clock_warp_start to -1. The result  will be that qemu_icount_bias will start counting when
the vcpu goes from active to inactive. At that time, vm_clock_warp_start will already store the realtime clock 
value and a timer on the real clock will be set to expire at clock + deadline, making qemu_icount_bias increment
by deadline.
A consequence of initializing vm_clock_warp_start to -1 is also 
the fact that we'll skip the first check for a virtual expired timer. As I mentioned above, in ARM case, 
it's not dangerous because there are no timers active the first time we perform this check. However, this 
is just a potential scenario and I cannot guarantee that on other target architectures there won't be 
an expired timer pending the first time we check.

I also tested on x86:
qemu_clock_warp -> icount_warp_rt are first called on "pit_reset" after a virtual timer is set to expire.
However, in this case, the qemu_clock_expired(QEMU_CLOCK_VIRTUAL) fails because the current virtual clock is 0
so the timer doesn't have to expire yet. In this case also the above solution would work without breaking anything.


So, do you think it is worth taking this solution into account or it will cause more harm than good?

Sebastian

> 
> > +    cpu_fprintf(f, "Host - Guest clock  %ld(ms)\n",
> > +                (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) -
> > clocks_offset -
> > +                 qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL))/SCALE_MS);
> 
> I think this is (cpu_get_clock() - cpu_get_icount()) / SCALE_MS.
> 
> Paolo
> 

Re: [Qemu-devel] [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit'

[Paolo Bonzini]

Il 22/07/2014 11:58, Sebastian Tanase ha scritto:
> 
> -timers_state.cpu_clock_offset contains the offset between the real and virtual clocks.
> However, when using the value of the virtual clock (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)),
> qemu_icount_bias already includes this offset because, on ARM, qemu_clock_warp (which 
> then calls icount_warp_rt) is called for the first time in tcg_exec_all, making
> qemu_icount_bias take the value of qemu_clock_get_ns(QEMU_CLOCK_REALTIME)

Does this means that QEMU_CLOCK_VIRTUAL counts up from
qemu_clock_get_ns(QEMU_CLOCK_REALTIME) rather than 0?  This would be a
bug, and it would be good to fix it (by initializing vm_clock_warp_start
to -1).

> A solution to not compute the initial offset in qemu_icount_bias would be to initialize 
> vm_clock_warp_start to -1. The result  will be that qemu_icount_bias will start counting when
> the vcpu goes from active to inactive. At that time, vm_clock_warp_start will already store the realtime clock 
> value and a timer on the real clock will be set to expire at clock + deadline, making qemu_icount_bias increment
> by deadline.

That would be correct.

> A consequence of initializing vm_clock_warp_start to -1 is also 
> the fact that we'll skip the first check for a virtual expired timer. As I mentioned above, in ARM case, 
> it's not dangerous because there are no timers active the first time we perform this check. However, this 
> is just a potential scenario and I cannot guarantee that on other target architectures there won't be 
> an expired timer pending the first time we check.

Like a timer that expired in the past?  That would be caught in
tcg_cpu_exec, when it initializes the decrementer to
qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL).

> So, do you think it is worth taking this solution into account or it will cause more harm than good?

Yes, even more so.  If we add workarounds to complicated code, it will
become even more complicated.

Paolo

Re: [Qemu-devel] [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit'

[Sebastian Tanase]

----- Mail original -----
> De: "Paolo Bonzini" <pbonzini@redhat.com>
> À: "Sebastian Tanase" <sebastian.tanase@openwide.fr>
> Cc: aliguori@amazon.com, afaerber@suse.de, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>,
> michael@walle.cc, alex@alex.org.uk, stefanha@redhat.com, lcapitulino@redhat.com, crobinso@redhat.com,
> armbru@redhat.com, wenchaoqemu@gmail.com, quintela@redhat.com, kwolf@redhat.com, mst@redhat.com, "camille begue"
> <camille.begue@openwide.fr>, qemu-devel@nongnu.org
> Envoyé: Mardi 22 Juillet 2014 12:19:42
> Objet: Re: [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit'
> 
> Il 22/07/2014 11:58, Sebastian Tanase ha scritto:
> > 
> > -timers_state.cpu_clock_offset contains the offset between the real
> > and virtual clocks.
> > However, when using the value of the virtual clock
> > (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)),
> > qemu_icount_bias already includes this offset because, on ARM,
> > qemu_clock_warp (which
> > then calls icount_warp_rt) is called for the first time in
> > tcg_exec_all, making
> > qemu_icount_bias take the value of
> > qemu_clock_get_ns(QEMU_CLOCK_REALTIME)
> 
> Does this means that QEMU_CLOCK_VIRTUAL counts up from
> qemu_clock_get_ns(QEMU_CLOCK_REALTIME) rather than 0?  This would be
> a
> bug, and it would be good to fix it (by initializing
> vm_clock_warp_start
> to -1).

Yes, QEMU_CLOCK_VIRTUAL counts up from qemu_clock_get_ns(QEMU_CLOCK_REALTIME)
on ARM (I have only tested with the versatilepb and vexpress boards).

> 
> > A solution to not compute the initial offset in qemu_icount_bias
> > would be to initialize
> > vm_clock_warp_start to -1. The result  will be that
> > qemu_icount_bias will start counting when
> > the vcpu goes from active to inactive. At that time,
> > vm_clock_warp_start will already store the realtime clock
> > value and a timer on the real clock will be set to expire at clock
> > + deadline, making qemu_icount_bias increment
> > by deadline.
> 
> That would be correct.
> 
> > A consequence of initializing vm_clock_warp_start to -1 is also
> > the fact that we'll skip the first check for a virtual expired
> > timer. As I mentioned above, in ARM case,
> > it's not dangerous because there are no timers active the first
> > time we perform this check. However, this
> > is just a potential scenario and I cannot guarantee that on other
> > target architectures there won't be
> > an expired timer pending the first time we check.
> 
> Like a timer that expired in the past?  That would be caught in
> tcg_cpu_exec, when it initializes the decrementer to
> qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL).
> 
> > So, do you think it is worth taking this solution into account or
> > it will cause more harm than good?
> 
> Yes, even more so.  If we add workarounds to complicated code, it
> will
> become even more complicated.
> 
> Paolo
> 

I'll make a small patch that initializes vm_clock_warp_start to -1.


Sebastian

Re: [Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Sebastian Tanase]

----- Mail original -----
> De: "Paolo Bonzini" <pbonzini@redhat.com>
> À: "Sebastian Tanase" <sebastian.tanase@openwide.fr>, qemu-devel@nongnu.org
> Cc: aliguori@amazon.com, afaerber@suse.de, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>,
> michael@walle.cc, alex@alex.org.uk, stefanha@redhat.com, lcapitulino@redhat.com, crobinso@redhat.com,
> armbru@redhat.com, wenchaoqemu@gmail.com, quintela@redhat.com, kwolf@redhat.com, mst@redhat.com, "camille begue"
> <camille.begue@openwide.fr>
> Envoyé: Mercredi 16 Juillet 2014 15:20:00
> Objet: Re: [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks
> 
> Il 16/07/2014 14:18, Sebastian Tanase ha scritto:
> > v3 -> v4
> >
> > * Add better error handling for 'strtol' in patch 2
> > * Add 'Sleep' instead of 'nanosleep' for Windows hosts in patch 4
> > * Remove function pointers from patches 4 and 5
> 
> Hi Sebastian, I think we're getting really close.
> 
> I asked a question about clocks_offset; I think it's not necessary to
> compute it in cpu-exec.c because the same information is available
> elsewhere.  It is also probably not necessary to make it public.  Can
> you please check this?  Once this is sorted out, the patch should be
> ready for inclusion in 2.2.
> 
> Thanks for your effort!
> 
> Paolo
> 

Hello,

Supposing the patch that changes vm_clock_warp_start from 0 to -1 is accepted,
I could use the information in timers_state.cpu_clock_offset instead of recalculating
the offset. Besides, given that I only need this particular field from the whole 
structure, I think I don't have to make timers_state public; I could add a function 
in cpus.c, for example:

    int64_t cpu_get_clock_offset(void)
    {
        int64_t ti;
        unsigned start;

        do {
            start = seqlock_read_begin(&timers_state.vm_clock_seqlock);
            ti = -timers_state.cpu_clock_offset;
        } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, start));

        return ti;
    }

that will return the cpu_clock_offset field.


Best regards,

Sebastian

Re: [Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Paolo Bonzini]

Il 22/07/2014 16:02, Sebastian Tanase ha scritto:
> Yes, QEMU_CLOCK_VIRTUAL counts up from qemu_clock_get_ns(QEMU_CLOCK_REALTIME)
> on ARM (I have only tested with the versatilepb and vexpress boards).

That's a bug to fix indeed, then---it should count up from 0 without
icount, and icount shouldn't affect this.  Thanks for investigating it.

> Supposing the patch that changes vm_clock_warp_start from 0 to -1 is accepted,

... which shouldn't be a problem,... :)

> I could use the information in timers_state.cpu_clock_offset instead of recalculating
> the offset. Besides, given that I only need this particular field from the whole 
> structure, I think I don't have to make timers_state public; I could add a function 
> in cpus.c, for example:
> 
>     int64_t cpu_get_clock_offset(void)
>     {
>         int64_t ti;
>         unsigned start;
> 
>         do {
>             start = seqlock_read_begin(&timers_state.vm_clock_seqlock);
>             ti = -timers_state.cpu_clock_offset;
>         } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, start));
> 
>         return ti;
>     }
> 
> that will return the cpu_clock_offset field.

Indeed what I was proposing is a bit more sloppy.  If you do that, you
have to make the function a bit more general:

    ti = timers_state.cpu_clock_offset;
    if (!timers_state.cpu_ticks_enabled) {
        ti -= get_clock();
    }
    ...

    return -ti;

even though in cpus.c you'll only be using it when cpu_ticks_enabled is
true.  See cpu_enable_ticks() and cpu_disable_ticks().

Paolo

Re: [Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Sebastian Tanase]

----- Mail original -----
> De: "Paolo Bonzini" <pbonzini@redhat.com>
> À: "Sebastian Tanase" <sebastian.tanase@openwide.fr>
> Cc: aliguori@amazon.com, afaerber@suse.de, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>,
> michael@walle.cc, alex@alex.org.uk, stefanha@redhat.com, lcapitulino@redhat.com, crobinso@redhat.com,
> armbru@redhat.com, wenchaoqemu@gmail.com, quintela@redhat.com, kwolf@redhat.com, mst@redhat.com, "camille begue"
> <camille.begue@openwide.fr>, qemu-devel@nongnu.org
> Envoyé: Mardi 22 Juillet 2014 16:59:18
> Objet: Re: [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks
> 
> Il 22/07/2014 16:02, Sebastian Tanase ha scritto:
> > Yes, QEMU_CLOCK_VIRTUAL counts up from
> > qemu_clock_get_ns(QEMU_CLOCK_REALTIME)
> > on ARM (I have only tested with the versatilepb and vexpress
> > boards).
> 
> That's a bug to fix indeed, then---it should count up from 0 without
> icount, and icount shouldn't affect this.  Thanks for investigating
> it.

Just to be sure I don't missunderstand, when you say "without icount"
you refer to qemu_icount_bias (aka when the vcpu is inactive), right?

Sebastian
> 
> > Supposing the patch that changes vm_clock_warp_start from 0 to -1
> > is accepted,
> 
> ... which shouldn't be a problem,... :)
> 
> > I could use the information in timers_state.cpu_clock_offset
> > instead of recalculating
> > the offset. Besides, given that I only need this particular field
> > from the whole
> > structure, I think I don't have to make timers_state public; I
> > could add a function
> > in cpus.c, for example:
> > 
> >     int64_t cpu_get_clock_offset(void)
> >     {
> >         int64_t ti;
> >         unsigned start;
> > 
> >         do {
> >             start =
> >             seqlock_read_begin(&timers_state.vm_clock_seqlock);
> >             ti = -timers_state.cpu_clock_offset;
> >         } while (seqlock_read_retry(&timers_state.vm_clock_seqlock,
> >         start));
> > 
> >         return ti;
> >     }
> > 
> > that will return the cpu_clock_offset field.
> 
> Indeed what I was proposing is a bit more sloppy.  If you do that,
> you
> have to make the function a bit more general:
> 
>     ti = timers_state.cpu_clock_offset;
>     if (!timers_state.cpu_ticks_enabled) {
>         ti -= get_clock();
>     }
>     ...
> 
>     return -ti;
> 
> even though in cpus.c you'll only be using it when cpu_ticks_enabled
> is
> true.  See cpu_enable_ticks() and cpu_disable_ticks().
> 
> Paolo
> 

Re: [Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Paolo Bonzini]

Il 22/07/2014 17:17, Sebastian Tanase ha scritto:
>> > That's a bug to fix indeed, then---it should count up from 0 without
>> > icount, and icount shouldn't affect this.  Thanks for investigating
>> > it.
> Just to be sure I don't missunderstand, when you say "without icount"
> you refer to qemu_icount_bias (aka when the vcpu is inactive), right?

No, I mean without "-icount", i.e. when QEMU_CLOCK_VIRTUAL is just
nanoseconds.

Paolo

Re: [Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Sebastian Tanase]

----- Mail original -----
> De: "Paolo Bonzini" <pbonzini@redhat.com>
> À: "Sebastian Tanase" <sebastian.tanase@openwide.fr>
> Cc: aliguori@amazon.com, afaerber@suse.de, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>,
> michael@walle.cc, alex@alex.org.uk, stefanha@redhat.com, lcapitulino@redhat.com, crobinso@redhat.com,
> armbru@redhat.com, wenchaoqemu@gmail.com, quintela@redhat.com, kwolf@redhat.com, mst@redhat.com, "camille begue"
> <camille.begue@openwide.fr>, qemu-devel@nongnu.org
> Envoyé: Mardi 22 Juillet 2014 17:22:18
> Objet: Re: [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks
> 
> Il 22/07/2014 17:17, Sebastian Tanase ha scritto:
> >> > That's a bug to fix indeed, then---it should count up from 0
> >> > without
> >> > icount, and icount shouldn't affect this.  Thanks for
> >> > investigating
> >> > it.
> > Just to be sure I don't missunderstand, when you say "without
> > icount"
> > you refer to qemu_icount_bias (aka when the vcpu is inactive),
> > right?
> 
> No, I mean without "-icount", i.e. when QEMU_CLOCK_VIRTUAL is just
> nanoseconds.
> 
> Paolo
> 

When not using "-icount" everything is fine because QEMU_CLOCK_VIRTUAL 
is based on the real clock and the offset; qemu_icount_bias  
doesn't come into play. So the vm_clock_warp_start fix is only for the case 
where we use "-icount".

Sebastian

Re: [Qemu-devel] [RFC PATCH V4 0/6] icount: Implement delay algorithm between guest and host clocks

[Paolo Bonzini]

Il 22/07/2014 17:28, Sebastian Tanase ha scritto:
> When not using "-icount" everything is fine because QEMU_CLOCK_VIRTUAL 
> is based on the real clock and the offset; qemu_icount_bias  
> doesn't come into play. So the vm_clock_warp_start fix is only for the case 
> where we use "-icount".

I meant that with the fix the QEMU_CLOCK_VIRTUAL clock behaves the same
way (it is based at 0) with and without -icount.  This is why I think
the fix is correct.

Paolo