Fix and improve core RTC function and documentation
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.
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 | 102 ++++++++++++++++++++++++++++++++++++-------------------- 3 files changed, 76 insertions(+), 43 deletions(-)
Signature · 1 line
2.7.4
PATCH 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.
--- qemu-options.hx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
qemu-options.hx +9 -5
@@ -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
PATCH 2/4 vl: refactor -rtc option references
In reply to Artem PisarenkoImprove code readability and prepare for fixing bug #1797033
--- vl.c | 82 +++++++++++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 49 insertions(+), 33 deletions(-)
vl.c +49 -33
@@ -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,28 @@ 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; - -return seconds -qemu_time (); +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 ; + } + +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 +855,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 +874,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
PATCH 3/4 Fixes RTC bug with base datetime shifts in clock=vm
In reply to Artem PisarenkoThis 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.
--- vl.c | 54 +++++++++++++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 21 deletions(-)
vl.c +33 -21
@@ -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 ) +/* RTC reference time/date access */ +static time_t qemu_ref_timedate (void ) { -return qemu_clock_get_ms (QEMU_CLOCK_HOST ) /1000 ; +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; } -/***********************************************************/ -/* RTC reference time/date access */ 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,15 +842,12 @@ int qemu_timedate_diff(struct tm *tm) seconds =mktime (&tmp);break ; } -case RTC_BASE_DATETIME : - seconds =mktimegm (tm) + rtc_host_datetime_offset; -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;
@@ -863,8 +873,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)
@@ -883,7 +893,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" );
@@ -3034,6 +3044,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
PATCH 4/4 vl, qapi: offset value calculation in RTC_CHANGE event reverted to match behavior before #1797033 bugfix and documented
In reply to Artem PisarenkoReturn 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.
--- qapi/misc.json | 3 ++- vl.c | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-)
qapi/misc.json +2 -1
@@ -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. #
vl.c +5 -5
@@ -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;
@@ -844,7 +844,7 @@ int qemu_timedate_diff(struct tm *tm) } } -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
Re: [Qemu-devel] [PATCH 4/4] vl, qapi: offset value calculation in RTC_CHANGE event reverted to match behavior before #1797033 bugfix and documented
Quoted 1 line · 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.
Long line. Remember to manually wrap commit messages around 70 columns or so, so that they still fit in an 80-column window under 'git log' which adds leading indentation.
Also, the subject line is way too long (there, you should target 60 columns or less; 'git shortlog -30' will give you some ideas of typical summaries).
Quoted 13 lines
Added 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 +0
-0
qapi/misc.json line 3074
Quoted 6 lines
qapi/misc.json +2
-1@@ -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).
s/choosen/chosen/
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
I checked it with ./configure --enable-debug and didn't imagined that in non-debug build (with -O2) compiler will complain:
"vl.c:847:12: error: ‘seconds’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]"
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;
case RTC_BASE_LOCALTIME:
{
struct tm tmp = *tm;
tmp.tm_isdst = -1; /* use timezone to figure it out */
seconds = mktime(&tmp);
break;
}
}Quoted 1 line
Quoted 1 line
Quoted 1 line
return seconds -
qemu_ref_timedate(QEMU_CLOCK_HOST);
}
Switch covers all defined enum values, so 'second' cannot be left uninitialized. One way it could be is if someone assigns integer value to 'rtc_base_type' variable explicitly. Ok, let's look at it. This variable initialized with valid value at definition and elsewhere in code it's being assigned only valid enum values. Maybe it could be changed outside of compile unit ? No, it's defined as static. Maybe GCC isn't just smart enough to check it in whole compile unit scope context ? From my expirience I've seen that it's able to make even more complex assumptions and smart decisions. And with higher optimization levels GCC becomes just smarter. But this time GCC behavior differs...
Signature · 2 lines
С уважением, Артем Писаренко