Skip to contentSkip to conversation

Fix and improve core RTC function and documentation

PATCH Applied Artem Pisarenko 2 participants 9 messages Oct 18, 2018 – Oct 19, 2018

lore
Share & tools
lore

Artem Pisarenko

author
#0 lore raw

v3 cover Fix and improve core RTC function and documentation

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.

v3 changes:
- commit messages rewritten/shortened/wrapped to match patch requirements
- fixed minor typo
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 calculation in RTC_CHANGE event reverted

 qapi/misc.json  |   3 +-
 qemu-options.hx |  14 +++++---
 vl.c            | 105 +++++++++++++++++++++++++++++++++++++-------------------
 3 files changed, 80 insertions(+), 42 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH v3 1/4 vl: improve/fix documentation related to RTC function

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

Signed-off-by Artem Pisarenko <[email protected]>
---
 qemu-options.hx | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
qemu-options.hx +9 -5
index f139459..4a9c065 100644
@@ -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
Signature · 1 line
2.7.4
#2 lore raw

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

qemu-options.hx HXCOMM Silently ignored for compatibility · line 3483

Quoted 6 lines · Artem Pisarenko
qemu-options.hx +5 -0
+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.

Slight rephrasing:

To even prevent the RTC from progressing during suspension, you can set @option{clock} to @code{vm} (virtual clock). @samp{clock=vm} is recommended especially in icount mode in order to preserve determinism; however, note that in icount mode the speed of the virtual clock is variable and can in general differ from the host clock.

(omitting the reference to -icount auto etc.)
Paolo

author
#3 lore raw

PATCH v3 2/4 vl: refactor -rtc option references

In reply to Artem Pisarenko

Improve code readability and prepare for fixing bug #1797033

Signed-off-by Artem Pisarenko <[email protected]>

---

Notes:
v2: fixed compiler warning

 vl.c | 85 ++++++++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 53 insertions(+), 32 deletions(-)
vl.c +53 -32
index 4e25c78..10c4275 100644
@@ -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");
Signature · 1 line
2.7.4
author
#4 lore raw

PATCH v3 3/4 Fixes RTC bug with base datetime shifts in clock=vm

In reply to 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 <[email protected]>
---
 vl.c | 58 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 23 deletions(-)
vl.c +35 -23
index 10c4275..78a8a68 100644
@@ -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();
Signature · 1 line
2.7.4
#5 lore raw

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

Review

Quoted 2 lines · Artem Pisarenko
(quoted diff) +2 -0
+    rtc_host_datetime_offset = rtc_ref_start_datetime - rtc_start_datetime;
+    rtc_ref_start_datetime = rtc_start_datetime;

Because of this, multiple rtc options have funny effects.

Can squash this instead:

vl.c +5 -0
index 1b349b78bd..4c332a5469 100644
@@ -249,6 +249,7 @@ static struct {
 static QemuOptsList qemu_rtc_opts = {
     .name = "rtc",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_rtc_opts.head),
+    .merge_lists = true,
     .desc = {
         {
             .name = "base",

@@ -886,6 +887,11 @@ static void configure_rtc(QemuOpts *opts)
 {
     const char *value;
 
+    /* Set defaults */
+    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;
+

     value = qemu_opt_get(opts, "base");
     if (value) {
         if (!strcmp(value, "utc")) {
@@ -3048,9 +3054,6 @@ int main(int argc, char **argv, char **envp)
         error_reportf_err(err, "cannot initialize crypto: ");
         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();
@@ -3770,7 +3771,6 @@ int main(int argc, char **argv, char **envp)
                 if (!opts) {
                     exit(1);
                 }
-                configure_rtc(opts);
                 break;
             case QEMU_OPTION_tb_size:
 #ifndef CONFIG_TCG
@@ -3988,6 +3988,8 @@ int main(int argc, char **argv, char **envp)
         exit(EXIT_FAILURE);
     }

+ configure_rtc(qemu_find_opts_singleton("rtc"));
+

     machine_class = select_machine();

     set_memory_options(&ram_slots, &maxram_size, machine_class);
author
#6 lore raw

PATCH v3 4/4 vl, qapi: offset calculation in RTC_CHANGE event reverted

In reply to Artem Pisarenko

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

Signed-off-by Artem Pisarenko <[email protected]>
---
 qapi/misc.json |  3 ++-
 vl.c           | 10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)
qapi/misc.json +2 -1
index ada9af5..d0f5381 100644
@@ -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 chosen to drive RTC (specified by -rtc clock).
 #
 # Note: This event is rate-limited.
 #
vl.c +5 -5
index 78a8a68..c350aba 100644
@@ -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)
Signature · 1 line
2.7.4
Applied
#7 lore raw
In reply to Artem Pisarenko
Quoted 7 lines · 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.

Quoted 7 lines

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.

Queued, thanks.

As a start of future refactoring, would you mind moving all this code to hw/timer/rtc.c or rtc.c? It was somewaht generic before, but now it's very tied to -rtc.

Paolo

Quoted 19 lines

v3 changes:
- commit messages rewritten/shortened/wrapped to match patch requirements
- fixed minor typo
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 calculation in RTC_CHANGE event reverted

 qapi/misc.json  |   3 +-
 qemu-options.hx |  14 +++++---
 vl.c            | 105 +++++++++++++++++++++++++++++++++++++-------------------
 3 files changed, 80 insertions(+), 42 deletions(-)
author
#8 lore raw
Quoted 3 lines

As a start of future refactoring, would you mind moving all this code to hw/timer/rtc.c or rtc.c? It was somewaht generic before, but now it's very tied to -rtc.

Yes, sure.