[Qemu-devel] [PATCH v2 0/4] Fix and improve core RTC function and documentation

[Qemu-devel] [PATCH v2 0/4] Fix and improve core RTC function and documentation

[Artem Pisarenko]

Modifications are motivated by bug https://bugs.launchpad.net/qemu/+bug/1797033 I've encountered recently.

Trying to fix it and analyzing its effect on all use cases (not covered in bug report) revealed much deeper problems.
This is my first patch to QEMU and I'm not sure whether the way I addressed them is proper.
They definitely require either modifications to current implementation (breaking compability), or adding clarifications to user documentation. I've tried to find compromise.
Changes I propose are driven mostly by my interpretation of user documentation and comments in code, rather than by existing implementation.

I've splitted patches in a way that at least some of them may be accepted. Following subsets are make sense on their own:
- 1
- 1,2,3

I limited refactoring only to vl.c, although it leads to reworking of all hw/* rtc models which have unreasonable duplications of actions performed by vl.c.
I'm sure such kind of rework will reveal more hidden bugs/features, but so far I've had enough (it's a long story how muсh effort and pain cost me to find that tricky bug above).
I suppose just to draw attention of relevant hw/* maintainers.


v2 changes:
 - fixed compiler warning caused non-debug build fail


Artem Pisarenko (4):
  vl: improve/fix documentation related to RTC function
  vl: refactor -rtc option references
  Fixes RTC bug with base datetime shifts in clock=vm
  vl,qapi: offset value calculation in RTC_CHANGE event reverted to
    match behavior before #1797033 bugfix and documented

 qapi/misc.json  |   3 +-
 qemu-options.hx |  14 +++++---
 vl.c            | 105 +++++++++++++++++++++++++++++++++++++-------------------
 3 files changed, 80 insertions(+), 42 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/4] vl: improve/fix documentation related to RTC function

[Artem Pisarenko]

Documentation describing -rtc option updated to better match current implementation and highlight some important specifics.

Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---
 qemu-options.hx | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index f139459..4a9c065 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3458,25 +3458,29 @@ HXCOMM Silently ignored for compatibility
 DEF("clock", HAS_ARG, QEMU_OPTION_clock, "", QEMU_ARCH_ALL)
 
 DEF("rtc", HAS_ARG, QEMU_OPTION_rtc, \
-    "-rtc [base=utc|localtime|date][,clock=host|rt|vm][,driftfix=none|slew]\n" \
+    "-rtc [base=utc|localtime|<datetime>][,clock=host|rt|vm][,driftfix=none|slew]\n" \
     "                set the RTC base and clock, enable drift fix for clock ticks (x86 only)\n",
     QEMU_ARCH_ALL)
 
 STEXI
 
-@item -rtc [base=utc|localtime|@var{date}][,clock=host|vm][,driftfix=none|slew]
+@item -rtc [base=utc|localtime|@var{datetime}][,clock=host|rt|vm][,driftfix=none|slew]
 @findex -rtc
 Specify @option{base} as @code{utc} or @code{localtime} to let the RTC start at the current
 UTC or local time, respectively. @code{localtime} is required for correct date in
-MS-DOS or Windows. To start at a specific point in time, provide @var{date} in the
+MS-DOS or Windows. To start at a specific point in time, provide @var{datetime} in the
 format @code{2006-06-17T16:01:21} or @code{2006-06-17}. The default base is UTC.
 
 By default the RTC is driven by the host system time. This allows using of the
 RTC as accurate reference clock inside the guest, specifically if the host
 time is smoothly following an accurate external reference clock, e.g. via NTP.
 If you want to isolate the guest time from the host, you can set @option{clock}
-to @code{rt} instead.  To even prevent it from progressing during suspension,
-you can set it to @code{vm}.
+to @code{rt} instead, which provides host monotonic clock if host support it.
+To even prevent it from progressing during suspension, you can set it to
+@code{vm} (virtual clock). Virtual clock is the clock seen by guest,
+In icount mode of emulation its (long term) speed will be different from
+any host clock, when icount configured to non-auto value or virtual cpu sleeping
+is off, and no synchronization algorithm is active.
 
 Enable @option{driftfix} (i386 targets only) if you experience time drift problems,
 specifically with Windows' ACPI HAL. This option will try to figure out how
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/4] vl: refactor -rtc option references

[Artem Pisarenko]

Improve code readability and prepare for fixing bug #1797033

Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---

Notes:
    v2: fixed compiler warning

 vl.c | 85 ++++++++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 53 insertions(+), 32 deletions(-)

diff --git a/vl.c b/vl.c
index 4e25c78..10c4275 100644
--- a/vl.c
+++ b/vl.c
@@ -147,8 +147,13 @@ bool enable_cpu_pm = false;
 int nb_nics;
 NICInfo nd_table[MAX_NICS];
 int autostart;
-static int rtc_utc = 1;
-static int rtc_date_offset = -1; /* -1 means no change */
+static enum {
+    RTC_BASE_UTC,
+    RTC_BASE_LOCALTIME,
+    RTC_BASE_DATETIME,
+} rtc_base_type = RTC_BASE_UTC;
+static int rtc_host_datetime_offset = -1; /* valid only for host rtc_clock and
+                                             rtc_base_type=RTC_BASE_DATETIME */
 QEMUClockType rtc_clock;
 int vga_interface_type = VGA_NONE;
 static DisplayOptions dpy;
@@ -782,26 +787,30 @@ void qemu_system_vmstop_request(RunState state)
 /***********************************************************/
 /* real time host monotonic timer */
 
-static time_t qemu_time(void)
+static time_t qemu_timedate(void)
 {
     return qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
 }
 
 /***********************************************************/
-/* host time/date access */
+/* RTC reference time/date access */
 void qemu_get_timedate(struct tm *tm, int offset)
 {
-    time_t ti = qemu_time();
+    time_t ti = qemu_timedate();
 
     ti += offset;
-    if (rtc_date_offset == -1) {
-        if (rtc_utc)
-            gmtime_r(&ti, tm);
-        else
-            localtime_r(&ti, tm);
-    } else {
-        ti -= rtc_date_offset;
+
+    switch (rtc_base_type) {
+    case RTC_BASE_UTC:
         gmtime_r(&ti, tm);
+        break;
+    case RTC_BASE_LOCALTIME:
+        localtime_r(&ti, tm);
+        break;
+    case RTC_BASE_DATETIME:
+        ti -= rtc_host_datetime_offset;
+        gmtime_r(&ti, tm);
+        break;
     }
 }
 
@@ -809,23 +818,33 @@ int qemu_timedate_diff(struct tm *tm)
 {
     time_t seconds;
 
-    if (rtc_date_offset == -1)
-        if (rtc_utc)
-            seconds = mktimegm(tm);
-        else {
-            struct tm tmp = *tm;
-            tmp.tm_isdst = -1; /* use timezone to figure it out */
-            seconds = mktime(&tmp);
-	}
-    else
-        seconds = mktimegm(tm) + rtc_date_offset;
+    switch (rtc_base_type) {
+    case RTC_BASE_UTC:
+        seconds = mktimegm(tm);
+        break;
+    case RTC_BASE_LOCALTIME:
+    {
+        struct tm tmp = *tm;
+        tmp.tm_isdst = -1; /* use timezone to figure it out */
+        seconds = mktime(&tmp);
+        break;
+    }
+    case RTC_BASE_DATETIME:
+        seconds = mktimegm(tm) + rtc_host_datetime_offset;
+        break;
+    default:
+        /* gcc complains: ‘seconds’ may be used uninitialized */
+        g_assert_not_reached();
+        seconds = -1;
+        break;
+    }
 
-    return seconds - qemu_time();
+    return seconds - qemu_timedate();
 }
 
-static void configure_rtc_date_offset(const char *startdate)
+static void configure_rtc_host_datetime_offset(const char *startdate)
 {
-    time_t rtc_start_date;
+    time_t rtc_start_datetime;
     struct tm tm;
 
     if (sscanf(startdate, "%d-%d-%dT%d:%d:%d", &tm.tm_year, &tm.tm_mon,
@@ -841,15 +860,16 @@ static void configure_rtc_date_offset(const char *startdate)
     }
     tm.tm_year -= 1900;
     tm.tm_mon--;
-    rtc_start_date = mktimegm(&tm);
-    if (rtc_start_date == -1) {
+    rtc_start_datetime = mktimegm(&tm);
+    if (rtc_start_datetime == -1) {
     date_fail:
-        error_report("invalid date format");
+        error_report("invalid datetime format");
         error_printf("valid formats: "
                      "'2006-06-17T16:01:21' or '2006-06-17'\n");
         exit(1);
     }
-    rtc_date_offset = qemu_time() - rtc_start_date;
+    rtc_host_datetime_offset = (qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000)
+                               - rtc_start_datetime;
 }
 
 static void configure_rtc(QemuOpts *opts)
@@ -859,15 +879,16 @@ static void configure_rtc(QemuOpts *opts)
     value = qemu_opt_get(opts, "base");
     if (value) {
         if (!strcmp(value, "utc")) {
-            rtc_utc = 1;
+            rtc_base_type = RTC_BASE_UTC;
         } else if (!strcmp(value, "localtime")) {
             Error *blocker = NULL;
-            rtc_utc = 0;
+            rtc_base_type = RTC_BASE_LOCALTIME;
             error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED,
                       "-rtc base=localtime");
             replay_add_blocker(blocker);
         } else {
-            configure_rtc_date_offset(value);
+            rtc_base_type = RTC_BASE_DATETIME;
+            configure_rtc_host_datetime_offset(value);
         }
     }
     value = qemu_opt_get(opts, "clock");
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/4] Fixes RTC bug with base datetime shifts in clock=vm

[Artem Pisarenko]

This makes all current "-rtc" option parameters combinations produce fixed/unambiguous RTC timedate reference for hardware emulation frontends.
It restores determinism of guest execution when used with clock=vm and specified base <datetime> value.

Buglink: https://bugs.launchpad.net/qemu/+bug/1797033
Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---
 vl.c | 58 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 23 deletions(-)

diff --git a/vl.c b/vl.c
index 10c4275..78a8a68 100644
--- a/vl.c
+++ b/vl.c
@@ -152,8 +152,10 @@ static enum {
     RTC_BASE_LOCALTIME,
     RTC_BASE_DATETIME,
 } rtc_base_type = RTC_BASE_UTC;
-static int rtc_host_datetime_offset = -1; /* valid only for host rtc_clock and
-                                             rtc_base_type=RTC_BASE_DATETIME */
+static time_t rtc_ref_start_datetime;
+static int rtc_realtime_clock_offset; /* used only with QEMU_CLOCK_REALTIME */
+static int rtc_host_datetime_offset = -1; /* valid & used only with
+                                             RTC_BASE_DATETIME */
 QEMUClockType rtc_clock;
 int vga_interface_type = VGA_NONE;
 static DisplayOptions dpy;
@@ -785,32 +787,42 @@ void qemu_system_vmstop_request(RunState state)
 }
 
 /***********************************************************/
-/* real time host monotonic timer */
-
-static time_t qemu_timedate(void)
-{
-    return qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
-}
-
-/***********************************************************/
 /* RTC reference time/date access */
+static time_t qemu_ref_timedate(void)
+{
+    time_t value = qemu_clock_get_ms(rtc_clock) / 1000;
+    switch (rtc_clock) {
+    case QEMU_CLOCK_REALTIME:
+        value -= rtc_realtime_clock_offset;
+        /* no break */
+    case QEMU_CLOCK_VIRTUAL:
+        value += rtc_ref_start_datetime;
+        break;
+    case QEMU_CLOCK_HOST:
+        if (rtc_base_type == RTC_BASE_DATETIME) {
+            value -= rtc_host_datetime_offset;
+        }
+        break;
+    default:
+        assert(0);
+    }
+    return value;
+}
+
 void qemu_get_timedate(struct tm *tm, int offset)
 {
-    time_t ti = qemu_timedate();
+    time_t ti = qemu_ref_timedate();
 
     ti += offset;
 
     switch (rtc_base_type) {
+    case RTC_BASE_DATETIME:
     case RTC_BASE_UTC:
         gmtime_r(&ti, tm);
         break;
     case RTC_BASE_LOCALTIME:
         localtime_r(&ti, tm);
         break;
-    case RTC_BASE_DATETIME:
-        ti -= rtc_host_datetime_offset;
-        gmtime_r(&ti, tm);
-        break;
     }
 }
 
@@ -819,6 +831,7 @@ int qemu_timedate_diff(struct tm *tm)
     time_t seconds;
 
     switch (rtc_base_type) {
+    case RTC_BASE_DATETIME:
     case RTC_BASE_UTC:
         seconds = mktimegm(tm);
         break;
@@ -829,9 +842,6 @@ int qemu_timedate_diff(struct tm *tm)
         seconds = mktime(&tmp);
         break;
     }
-    case RTC_BASE_DATETIME:
-        seconds = mktimegm(tm) + rtc_host_datetime_offset;
-        break;
     default:
         /* gcc complains: ‘seconds’ may be used uninitialized */
         g_assert_not_reached();
@@ -839,10 +849,10 @@ int qemu_timedate_diff(struct tm *tm)
         break;
     }
 
-    return seconds - qemu_timedate();
+    return seconds - qemu_ref_timedate();
 }
 
-static void configure_rtc_host_datetime_offset(const char *startdate)
+static void configure_rtc_base_datetime(const char *startdate)
 {
     time_t rtc_start_datetime;
     struct tm tm;
@@ -868,8 +878,8 @@ static void configure_rtc_host_datetime_offset(const char *startdate)
                      "'2006-06-17T16:01:21' or '2006-06-17'\n");
         exit(1);
     }
-    rtc_host_datetime_offset = (qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000)
-                               - rtc_start_datetime;
+    rtc_host_datetime_offset = rtc_ref_start_datetime - rtc_start_datetime;
+    rtc_ref_start_datetime = rtc_start_datetime;
 }
 
 static void configure_rtc(QemuOpts *opts)
@@ -888,7 +898,7 @@ static void configure_rtc(QemuOpts *opts)
             replay_add_blocker(blocker);
         } else {
             rtc_base_type = RTC_BASE_DATETIME;
-            configure_rtc_host_datetime_offset(value);
+            configure_rtc_base_datetime(value);
         }
     }
     value = qemu_opt_get(opts, "clock");
@@ -3039,6 +3049,8 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
     rtc_clock = QEMU_CLOCK_HOST;
+    rtc_ref_start_datetime = qemu_clock_get_ms(QEMU_CLOCK_HOST) / 1000;
+    rtc_realtime_clock_offset = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
 
     QLIST_INIT (&vm_change_state_head);
     os_setup_early_signal_handling();
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/4] vl, qapi: offset value calculation in RTC_CHANGE event reverted to match behavior before #1797033 bugfix and documented

[Artem Pisarenko]

Return value of qemu_timedate_diff(), used for calculation offset in QAPI 'RTC_CHANGE' event restored to keep compatibility, although it isn't documented that difference is relative to host clock advancement.
Added important note to 'RTC_CHANGE' event description to highlight established implementation specifics.

Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---
 qapi/misc.json |  3 ++-
 vl.c           | 10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index ada9af5..ed866f2 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3070,7 +3070,8 @@
 # Emitted when the guest changes the RTC time.
 #
 # @offset: offset between base RTC clock (as specified by -rtc base), and
-#          new RTC clock value
+#          new RTC clock value. Note that value will be different depending
+#          on clock choosen to drive RTC (specified by -rtc clock).
 #
 # Note: This event is rate-limited.
 #
diff --git a/vl.c b/vl.c
index 78a8a68..c350aba 100644
--- a/vl.c
+++ b/vl.c
@@ -788,10 +788,10 @@ void qemu_system_vmstop_request(RunState state)
 
 /***********************************************************/
 /* RTC reference time/date access */
-static time_t qemu_ref_timedate(void)
+static time_t qemu_ref_timedate(QEMUClockType clock)
 {
-    time_t value = qemu_clock_get_ms(rtc_clock) / 1000;
-    switch (rtc_clock) {
+    time_t value = qemu_clock_get_ms(clock) / 1000;
+    switch (clock) {
     case QEMU_CLOCK_REALTIME:
         value -= rtc_realtime_clock_offset;
         /* no break */
@@ -811,7 +811,7 @@ static time_t qemu_ref_timedate(void)
 
 void qemu_get_timedate(struct tm *tm, int offset)
 {
-    time_t ti = qemu_ref_timedate();
+    time_t ti = qemu_ref_timedate(rtc_clock);
 
     ti += offset;
 
@@ -849,7 +849,7 @@ int qemu_timedate_diff(struct tm *tm)
         break;
     }
 
-    return seconds - qemu_ref_timedate();
+    return seconds - qemu_ref_timedate(QEMU_CLOCK_HOST);
 }
 
 static void configure_rtc_base_datetime(const char *startdate)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 4/4] vl, qapi: offset value calculation in RTC_CHANGE event reverted to match behavior before #1797033 bugfix and documented

[Eric Blake]

On 10/17/18 7:23 AM, Artem Pisarenko wrote:
> Return value of qemu_timedate_diff(), used for calculation offset in QAPI 'RTC_CHANGE' event restored to keep compatibility, although it isn't documented that difference is relative to host clock advancement.
> Added important note to 'RTC_CHANGE' event description to highlight established implementation specifics.
> 

Hmm, I just replied to v1 before seeing that you sent v2; my comments 
about long lines and a typo still apply.

> Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> ---
>   qapi/misc.json |  3 ++-
>   vl.c           | 10 +++++-----
>   2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/misc.json b/qapi/misc.json
> index ada9af5..ed866f2 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -3070,7 +3070,8 @@
>   # Emitted when the guest changes the RTC time.
>   #
>   # @offset: offset between base RTC clock (as specified by -rtc base), and
> -#          new RTC clock value
> +#          new RTC clock value. Note that value will be different depending
> +#          on clock choosen to drive RTC (specified by -rtc clock).
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 4/4] vl, qapi: offset value calculation in RTC_CHANGE event reverted to match behavior before #1797033 bugfix and documented

[Artem Pisarenko]

I fixed them and ready to submit v3.
Anything else ?

ср, 17 окт. 2018 г. в 20:01, Eric Blake <eblake@redhat.com>:

> On 10/17/18 7:23 AM, Artem Pisarenko wrote:
> > Return value of qemu_timedate_diff(), used for calculation offset in
> QAPI 'RTC_CHANGE' event restored to keep compatibility, although it isn't
> documented that difference is relative to host clock advancement.
> > Added important note to 'RTC_CHANGE' event description to highlight
> established implementation specifics.
> >
>
> Hmm, I just replied to v1 before seeing that you sent v2; my comments
> about long lines and a typo still apply.
>
> > Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> > ---
> >   qapi/misc.json |  3 ++-
> >   vl.c           | 10 +++++-----
> >   2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index ada9af5..ed866f2 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -3070,7 +3070,8 @@
> >   # Emitted when the guest changes the RTC time.
> >   #
> >   # @offset: offset between base RTC clock (as specified by -rtc base),
> and
> > -#          new RTC clock value
> > +#          new RTC clock value. Note that value will be different
> depending
> > +#          on clock choosen to drive RTC (specified by -rtc clock).
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266 <(919)%20301-3266>
> Virtualization:  qemu.org | libvirt.org
>
-- 

С уважением,
  Артем Писаренко