Re: [Qemu-devel] [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration

[Paolo Bonzini]

On 20/09/2016 14:54, address@hidden wrote:
> Hi, Paolo
> The reason that use rtc_flush_time/rtc_adjust_timebase pairs instead
> of rtc_update_time/rtc_set_time  is a trick.
> what all we do is to coordinate the base point of time line for guest on
> a new host.  So, we don't flush realtime
> of the guest when it's stopped into cmos, but only convert vector
> [base_rtc, last_update] into cmos.

Isn't this the same?

In fact, rtc_flush_time and rtc_update_time are exactly the same code, 
except that rtc_update_time sums s->offset (which is <1 second) while 
rtc_flush_time sums a fixes 500 ns.

Likewise for rtc_set_time and rtc_adjust_timebase, except that 
rtc_adjust_timebase leaves s->base_rtc untouched and subtracts it from 
s->last_update; rtc_set_time instead changes both.  But this makes no 
difference because, according to get_guest_rtc_ns, what matters is only 
s->base_rtc * NANOSECONDS_PER_SECOND + s->offset - s->last_update.  So, 
say rtc_set_time would set

    s->base_rtc = mktimegm(&tm)
    s->last_update = qemu_clock_get_ns(rtc_clock)

while rtc_adjust_timebase would set 

    s->base_rtc = source_base_rtc
    s->last_update = qemu_clock_get_ns(rtc_clock)
                     - (mktimegm(&tm) - source_base_rtc) * 
NANOSECONDS_PER_SECOND

Then, after rtc_adjust_timebase, get_guest_rtc_ns returns

  s->base_rtc * NANOSECONDS_PER_SECOND + guest_clock - s->last_update + 
s->offset
  = source_base_rtc * NANOSECONDS_PER_SECOND + guest_clock
    - qemu_clock_get_ns(rtc_clock)
    + (mktimegm(&tm) - source_base_rtc) * NANOSECONDS_PER_SECOND
    + s->offset
  = mktimegm(&tm)  * NANOSECONDS_PER_SECOND + guest_clock
    - qemu_clock_get_ns(rtc_clock)
    + s->offset

and this is exactly what you'd get after rtc_set_time.

So I don't understand what's the difference, except for rounding the
nanoseconds component.

> On the other hand, the problem of rtc_update_time is it add time up plus
> s->offset, then when rtc_set_time 
> recalculate new last_update, it actually introduce s->offset into base
> vector [base_rtc, last_update].  further, 
> when guest continue to run and read realtime from cmos, rtc_update_time
> will add s->offset again, so s->offset
> is doubled.

This is true.  In fact rtc_post_load is already setting s->offset = 0 after
calling rtc_set_time.  Thus the load-side part of the patch can be simply

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index ea625f2..dd4ef5c 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -721,7 +722,7 @@ static int rtc_post_load(void *opaque, int version_id)
 {
     RTCState *s = opaque;
 
-    if (version_id <= 2) {
+    if (rtc_clock == QEMU_CLOCK_REALTIME || version_id <= 2) {
         rtc_set_time(s);
         s->offset = 0;
         check_update_timer(s);


Thanks,

Paolo