Skip to contentSkip to conversation

MC146818 RTC: coordinate guest clock base to destination host after migration

PATCH Superseded Paolo Bonzini 2 participants 18 messages Sep 20, 2016 – Sep 27, 2016 1 message not in the archive

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw
Quoted 2 lines · On 20/09/2016 09:19, [email protected] wrote:

qemu tracks guest time based on vector [base_rtc, last_update], in which last_update stands for a monotonic tick which is actually uptime of the host.

But last_update is not a monotonic tick, it's basically gettimeofday unless you're using the "-rtc clock=..." option.

Review

Quoted 13 lines
(quoted diff) +13 -0
+static void rtc_flush_time(RTCState *s)
+{
+    struct tm ret;
+    time_t guest_sec;
+    int64_t guest_nsec;
+    uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
+
+    guest_nsec = s->base_rtc * NANOSECONDS_PER_SECOND
+                 + guest_clock - s->last_update;
+    guest_sec = (guest_nsec + NANOSECONDS_PER_SECOND/2)/ NANOSECONDS_PER_SECOND;
+    gmtime_r(&guest_sec, &ret);
+
+    rtc_set_cmos(s, &ret);

This should be just rtc_update_time(s).

Similarly:

Review

Quoted 6 lines
(quoted diff) +5 -0
+    rtc_get_time(s, &tm);
+    diff = mktimegm(&tm) - s->base_rtc;
+    assert(diff >= 0);
+    s->last_update = qemu_clock_get_ns(rtc_clock)
+                    - diff * NANOSECONDS_PER_SECOND;

This should be rtc_set_time.

However, there are two problems with this approach. First, if you migrate old QEMU to new QEMU, the new QEMU expects to have an up-to-date s->base_rtc, while old QEMU provided an old QEMU. Second, every migration will delay the RTC by a few tenths of a second. So the call to rtc_set_time should be conditional on rtc_clock == QEMU_CLOCK_REALTIME.

Paolo

#2 lore raw

yes, the full options on clock is '-rtc driftfix=slew,clock=rt,base=localtime'

[email protected]

From: Paolo Bonzini
Date: 2016-09-20 16:17
To: [email protected]; qemu-devel
CC: Michael S. Tsirkin
Subject: Re: [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration

Quoted 2 lines · On 20/09/2016 09:19, [email protected] wrote:

qemu tracks guest time based on vector [base_rtc, last_update], in which last_update stands for a monotonic tick which is actually uptime of the host.

But last_update is not a monotonic tick, it's basically gettimeofday unless you're using the "-rtc clock=..." option.

Review

Quoted 13 lines
(quoted diff) +13 -0
+static void rtc_flush_time(RTCState *s)
+{
+    struct tm ret;
+    time_t guest_sec;
+    int64_t guest_nsec;
+    uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
+
+    guest_nsec = s->base_rtc * NANOSECONDS_PER_SECOND
+                 + guest_clock - s->last_update;
+    guest_sec = (guest_nsec + NANOSECONDS_PER_SECOND/2)/ NANOSECONDS_PER_SECOND;
+    gmtime_r(&guest_sec, &ret);
+
+    rtc_set_cmos(s, &ret);

This should be just rtc_update_time(s).

Similarly:

Review

Quoted 6 lines
(quoted diff) +5 -0
+    rtc_get_time(s, &tm);
+    diff = mktimegm(&tm) - s->base_rtc;
+    assert(diff >= 0);
+    s->last_update = qemu_clock_get_ns(rtc_clock)
+                    - diff * NANOSECONDS_PER_SECOND;

This should be rtc_set_time.

However, there are two problems with this approach. First, if you migrate old QEMU to new QEMU, the new QEMU expects to have an up-to-date s->base_rtc, while old QEMU provided an old QEMU. Second, every migration will delay the RTC by a few tenths of a second. So the call to rtc_set_time should be conditional on rtc_clock == QEMU_CLOCK_REALTIME.

Paolo

#3 lore raw
Quoted 2 lines · On 20/09/2016 10:34, [email protected] wrote:

yes, the full options on clock is
'-rtc driftfix=slew,clock=rt,base=localtime'

Good, then we should include a fixed patch that checks for clock=rt. Please remember to include the command-line options in the commit message.

Thanks!

Paolo

Quoted 59 lines

------------------------------------------------------------------------
[email protected]

    *From:* Paolo Bonzini <mailto:[email protected]>
    *Date:* 2016-09-20 16:17
    *To:* [email protected] <mailto:[email protected]>;
    qemu-devel <mailto:[email protected]>
    *CC:* Michael S. Tsirkin <mailto:[email protected]>
    *Subject:* Re: [PATCH]MC146818 RTC: coordinate guest clock base to
    destination host after migration
     
     
    On 20/09/2016 09:19, [email protected] wrote:
    > qemu tracks guest time based on vector [base_rtc, last_update], in
    which
    > last_update stands for a monotonic tick which is actually uptime
    of the host.
     
    But last_update is not a monotonic tick, it's basically gettimeofday
    unless you're using the "-rtc clock=..." option.
     
    > +static void rtc_flush_time(RTCState *s)
    > +{
    > +    struct tm ret;
    > +    time_t guest_sec;
    > +    int64_t guest_nsec;
    > +    uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
    > +
    > +    guest_nsec = s->base_rtc * NANOSECONDS_PER_SECOND
    > +                 + guest_clock - s->last_update;
    > +    guest_sec = (guest_nsec + NANOSECONDS_PER_SECOND/2)/
    NANOSECONDS_PER_SECOND;
    > +    gmtime_r(&guest_sec, &ret);
    > +
    > +    rtc_set_cmos(s, &ret);
     
    This should be just rtc_update_time(s).
     
    Similarly:
     
    >
    > +    rtc_get_time(s, &tm);
    > +    diff = mktimegm(&tm) - s->base_rtc;
    > +    assert(diff >= 0);
    > +    s->last_update = qemu_clock_get_ns(rtc_clock)
    > +                    - diff * NANOSECONDS_PER_SECOND;
     
    This should be rtc_set_time.
     
    However, there are two problems with this approach.  First, if you
    migrate old QEMU to new QEMU, the new QEMU expects to have an up-to-date
    s->base_rtc, while old QEMU provided an old QEMU.  Second, every
    migration will delay the RTC by a few tenths of a second.  So the call
    to rtc_set_time should be conditional on rtc_clock ==
    QEMU_CLOCK_REALTIME.
     
    Paolo
#4 lore raw
In reply to Paolo Bonzini

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. Additionally , with a new function rtc_flush_time, we can do some special adjustment to reduce deviation of guest time caused by migration. because granularity of cmos time is second, so maximum deviation of time base transfer via cmos is one second.

        guest_sec = (guest_nsec + NANOSECONDS_PER_SECOND/2)/ NANOSECONDS_PER_SECOND;

Adjusted by this statement, the deviation reduce to half a second.
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.

Thanks!

below is full options.

./qemu \
-id 1738826302644 \
-chardev socket,id=qmp,host=200.200.100.112,port=5111,server,nowait \
-mon chardev=qmp,mode=control \
-vnc 0.0.0.0:30 \
-enable-kvm \
-pidfile /var/run/qemu-server/1738826302644.pid \
-name win2008x64_Sangfor123clone \
-cpu core2duo,hv_spinlocks=0xffff,hv_relaxed,hv_time,hv_vapic,+sse4.1,+sse4.2,+x2apic,+pcid,+pdcm,+xtpr,+ht,+ss,+acpi,+ds \
-nodefaults \
-vga cirrus \
-no-hpet \
-k en-us \
-boot menu=on,splash-time=20000,ostype=windows,reboot-timeout=5000 \
-smp sockets=1,cores=2 \
-m 4096 \
-rtc driftfix=slew,clock=rt,base=localtime \
-global kvm-pit.lost_tick_policy=discard \
-global PIIX4_PM.disable_s3=1 \
-global PIIX4_PM.disable_s4=1 \
-device pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f \
-device pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e \
-usb \
-device usb-tablet,id=input0 \
-hda /home/test/img/winxp.qcow2 \
-bios /sf/share/kvm/bios.bin \
-machine mem-merge=off \
-post win2008x64_Sangfor123clone

---

Quoted 41 lines · On 20/09/2016 09:19, [email protected] wrote:
Quoted 2 lines

qemu tracks guest time based on vector [base_rtc, last_update], in which last_update stands for a monotonic tick which is actually uptime of the host.

But last_update is not a monotonic tick, it's basically gettimeofday unless you're using the "-rtc clock=..." option.

Review

Quoted 13 lines
(quoted diff) +13 -0
+static void rtc_flush_time(RTCState *s)
+{
+    struct tm ret;
+    time_t guest_sec;
+    int64_t guest_nsec;
+    uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
+
+    guest_nsec = s->base_rtc * NANOSECONDS_PER_SECOND
+                 + guest_clock - s->last_update;
+    guest_sec = (guest_nsec + NANOSECONDS_PER_SECOND/2)/ NANOSECONDS_PER_SECOND;
+    gmtime_r(&guest_sec, &ret);
+
+    rtc_set_cmos(s, &ret);

This should be just rtc_update_time(s).

Similarly:

Review

Quoted 6 lines
(quoted diff) +5 -0
+    rtc_get_time(s, &tm);
+    diff = mktimegm(&tm) - s->base_rtc;
+    assert(diff >= 0);
+    s->last_update = qemu_clock_get_ns(rtc_clock)
+                    - diff * NANOSECONDS_PER_SECOND;

This should be rtc_set_time.

However, there are two problems with this approach. First, if you migrate old QEMU to new QEMU, the new QEMU expects to have an up-to-date s->base_rtc, while old QEMU provided an old QEMU. Second, every migration will delay the RTC by a few tenths of a second. So the call to rtc_set_time should be conditional on rtc_clock == QEMU_CLOCK_REALTIME.

Paolo

#5 lore raw
Quoted 7 lines · On 20/09/2016 14:54, [email protected] 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.

Quoted 7 lines

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

hw/timer/mc146818rtc.c +1 -1
index ea625f2..dd4ef5c 100644
@@ -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

#6 lore raw

Hi, Paolo
YES, in terms of logic, both approaches do nearly the same things, and meanwhile, your approach is quite simple. exactly, I like it really. I tried to accept your way, but I found there is still some key difference between them, especially in terms of design and concept model.

What the qemu virtual clock does, the thought behind codes, is to create a virtual time axis for guest. Guest time goes by continuely on this virtual time axis independently of time axis of qemu itself. Then based on the model , what we need do next is clear. We maintain the time axis model consistent, then time in guest works fine naturally.

To define a axis system, we just define the original point. Here, we define a base vector [base_rtc, last_update], as the original point of virtual time axis for guest. So what we need to maintian consistence of the model is to retain fixity of base vector. This is a elementary principle kept properly in our implementation. If we change base_rtc while migration, thngs look work fine as normal, but in fact some important information lost. QEMU lost knowledge of time when the guest starts. This infomation maybe hasn't users nowadays. But maybe in the futhure we need, on the other hand, maybe some derived release spawned from community version needs and depends on this feature, who knows! The correct thing
we should keep in mind is we defined the model then we obligated to maintain model consistence.

In terms of engineering, we should try to make our code extensible and flexible, because we make way to extend functions and fix potential bugs in the future. I am sure there are more bugs in rtc. First, s->offset should extend its logic meaning, it should not hold only the deviation due to virtual hardware resets, but also deviations due to operations like migration. So offset should not just be 0 or half a second, it can be more variable and larger. Should we merge offset into last_update? I suggest better not, that disrupts time axis model. Second there is bug on offset when virtual hardware reset multiple times. and More.

After this patch, our next step is to eliminate deviation during migration. That time meaning of s->offset will be extended.
In the further future, we may implement some functions on statistics of guest lifetime despite it has been migrated or not. That needs a fixed base_rtc.

Thinks!
the model disrupted.

yes, approach we choose looks a little tediously long. but we'll gain in the future, when we trying to improve

[email protected]

From: Paolo Bonzini
Date: 2016-09-20 21:12
To: [email protected]; qemu-devel
CC: Michael S. Tsirkin
Subject: Re: [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration

Quoted 7 lines · On 20/09/2016 14:54, [email protected] 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.

Quoted 7 lines

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

hw/timer/mc146818rtc.c +0 -0
index ea625f2..dd4ef5c 100644
@@ -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

#7 lore raw

By the way, is this full patch of you?
---

hw/timer/mc146818rtc.c +0 -0
index ea625f2..dd4ef5c 100644
@@ -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);

---

Could you send me the complete patch?

[email protected]

From: [email protected]
Date: 2016-09-21 15:36
To: Paolo Bonzini; qemu-devel
CC: Michael S. Tsirkin
Subject: Re: Re: [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration Hi, Paolo
YES, in terms of logic, both approaches do nearly the same things, and meanwhile, your approach is quite simple. exactly, I like it really. I tried to accept your way, but I found there is still some key difference between them, especially in terms of design and concept model.

What the qemu virtual clock does, the thought behind codes, is to create a virtual time axis for guest. Guest time goes by continuely on this virtual time axis independently of time axis of qemu itself. Then based on the model , what we need do next is clear. We maintain the time axis model consistent, then time in guest works fine naturally.

To define a axis system, we just define the original point. Here, we define a base vector [base_rtc, last_update], as the original point of virtual time axis for guest. So what we need to maintian consistence of the model is to retain fixity of base vector. This is a elementary principle kept properly in our implementation. If we change base_rtc while migration, thngs look work fine as normal, but in fact some important information lost. QEMU lost knowledge of time when the guest starts. This infomation maybe hasn't users nowadays. But maybe in the futhure we need, on the other hand, maybe some derived release spawned from community version needs and depends on this feature, who knows! The correct thing
we should keep in mind is we defined the model then we obligated to maintain model consistence.

In terms of engineering, we should try to make our code extensible and flexible, because we make way to extend functions and fix potential bugs in the future. I am sure there are more bugs in rtc. First, s->offset should extend its logic meaning, it should not hold only the deviation due to virtual hardware resets, but also deviations due to operations like migration. So offset should not just be 0 or half a second, it can be more variable and larger. Should we merge offset into last_update? I suggest better not, that disrupts time axis model. Second there is bug on offset when virtual hardware reset multiple times. and More.

After this patch, our next step is to eliminate deviation during migration. That time meaning of s->offset will be extended.
In the further future, we may implement some functions on statistics of guest lifetime despite it has been migrated or not. That needs a fixed base_rtc.

Thinks!
the model disrupted.

yes, approach we choose looks a little tediously long. but we'll gain in the future, when we trying to improve

[email protected]

From: Paolo Bonzini
Date: 2016-09-20 21:12
To: [email protected]; qemu-devel
CC: Michael S. Tsirkin
Subject: Re: [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration

Quoted 7 lines · On 20/09/2016 14:54, [email protected] 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.

Quoted 7 lines

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

hw/timer/mc146818rtc.c +0 -0
index ea625f2..dd4ef5c 100644
@@ -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

#8 lore raw
In reply to [email protected]
Quoted 4 lines · On 21/09/2016 09:36, [email protected] wrote:

If we change base_rtc while migration, thngs look work fine as normal, but in fact some important information lost. QEMU lost knowledge of time when the guest starts. This infomation maybe hasn't users nowadays.

I see your point. However, I think it's simpler to have a single "delta" between qemu_clock_get_ns(rtc_clock) and the guest RTC clock.

To do this, base_rtc and last_update could even be merged in a single variable (offset is different).

Quoted 3 lines

But maybe in the futhure we need, on the other hand, maybe some derived release spawned from community version needs and depends on this feature, who knows!

This is not a problem now. If it's needed in the future one can complicate the code later. If it's needed by some downstream release, they will deal with it. The goal of QEMU is to have simple code.

Quoted 2 lines

Should we merge offset into last_update? I suggest better not, that disrupts time axis model

I agree. But I don't see a reason to keep base_rtc and last_update separate. Together they are "rtc_delta". They are separate mostly for debugging.

Quoted 5 lines

After this patch, our next step is to eliminate deviation during migration. That time meaning of s->offset will be extended. In the further future, we may implement some functions on statistics of guest lifetime despite it has been migrated or not. That needs a fixed base_rtc.

Extending the meaning of s->offset is fine, but a fixed s->base_rtc is not good. The guest can already modify base_rtc: it will do it every time it calls rtc_set_time, for example if the time of day is changed in the guest.

I suggest that you post the simplest possible patch to fix your issue, which calls rtc_update_time pre-save and rtc_set_time post-load (resetting s->offset after the rtc_set_time, as in the version <= 2 code). I'm interested in the code to eliminate the pause during migration, too.

Thanks,

Paolo

#9 lore raw

Hi, Paolo
Thanks for your suggestions, although I don't comply with the idea of rtc_delta yet. But beyond this, I understand your position, and I think the principle you insist on, I mean being simpler, is necessary for the health and sustainability of the project. I'll submit a simpler patch for review, but at the same time, I'll also company it with another more comprehensive patch, which include most of things I have mentioned that we want to do besides this patch I'm discussing with you. Then it's all after you to decide which one is worthy of living.

Thanks again, sincerely.

[email protected]

From: Paolo Bonzini
Date: 2016-09-21 23:15
To: [email protected]; qemu-devel
CC: Michael S. Tsirkin
Subject: Re: [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration

Quoted 4 lines · On 21/09/2016 09:36, [email protected] wrote:

If we change base_rtc while migration, thngs look work fine as normal, but in fact some important information lost. QEMU lost knowledge of time when the guest starts. This infomation maybe hasn't users nowadays.

I see your point. However, I think it's simpler to have a single "delta" between qemu_clock_get_ns(rtc_clock) and the guest RTC clock.

To do this, base_rtc and last_update could even be merged in a single variable (offset is different).

Quoted 3 lines

But maybe in the futhure we need, on the other hand, maybe some derived release spawned from community version needs and depends on this feature, who knows!

This is not a problem now. If it's needed in the future one can complicate the code later. If it's needed by some downstream release, they will deal with it. The goal of QEMU is to have simple code.

Quoted 2 lines

Should we merge offset into last_update? I suggest better not, that disrupts time axis model

I agree. But I don't see a reason to keep base_rtc and last_update separate. Together they are "rtc_delta". They are separate mostly for debugging.

Quoted 5 lines

After this patch, our next step is to eliminate deviation during migration. That time meaning of s->offset will be extended. In the further future, we may implement some functions on statistics of guest lifetime despite it has been migrated or not. That needs a fixed base_rtc.

Extending the meaning of s->offset is fine, but a fixed s->base_rtc is not good. The guest can already modify base_rtc: it will do it every time it calls rtc_set_time, for example if the time of day is changed in the guest.

I suggest that you post the simplest possible patch to fix your issue, which calls rtc_update_time pre-save and rtc_set_time post-load (resetting s->offset after the rtc_set_time, as in the version <= 2 code). I'm interested in the code to eliminate the pause during migration, too.

Thanks,

Paolo

#10 lore raw

PATCH v2 1/1 MC146818 RTC: coordinate guest clock base to destination host after migration

In reply to Paolo Bonzini

Hi, Paolo
This is a simplified patch according to your advice. Would you please reiview it again.

------------------------------------separation line--------------------------------------------

MC146818 RTC: coordinate guest clock base to destination host after migration

qemu tracks guest time based on vector [base_rtc, last_update], in which last_update stands for a monotonic tick which is actually uptime of the host. according to rtc implementation codes of recent releases and upstream, after migration, the time base vector [base_rtc, last_update] isn't updated to coordinate with the destionation host, ie. qemu doesnt update last_update to uptime of the destination host.
what problem have we got because of this bug? after migration, guest time may jump back to several days ago, that will make some critical business applications, such as lotus notes, malfunction.
this patch is trying to fix the problem. first, when vmsave in progress, we rtc_update_time to refresh time stamp in cmos array, then during vmrestore, we rtc_set_time to update qemu base_rtc and last_update variable according to time stamp in cmos array.

---
 hw/timer/mc146818rtc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
hw/timer/mc146818rtc.c +10 -5
index ea625f2..1df17af 100644
@@ -717,15 +717,20 @@ static void rtc_set_date_from_host(ISADevice *dev)
     rtc_set_cmos(s, &tm);
 }
 
+static void rtc_pre_save(void *opaque) 
+{
+    RTCState *s = opaque;
+
+    rtc_update_time(s);
+}
+
 static int rtc_post_load(void *opaque, int version_id)
 {
     RTCState *s = opaque;
 
-    if (version_id <= 2) {
-        rtc_set_time(s);
-        s->offset = 0;
-        check_update_timer(s);
-    }
+    rtc_set_time(s);
+    s->offset = 0;
+    check_update_timer(s);
 
     uint64_t now = qemu_clock_get_ns(rtc_clock);
     if (now < s->next_periodic_time ||
Signature · 1 line
2.9.0.windows.1
#11 lore raw
Quoted 45 lines · On 26/09/2016 05:37, [email protected] wrote:

Hi, Paolo
This is a simplified patch according to your advice. Would you please reiview it again.

------------------------------------separation line--------------------------------------------

MC146818 RTC: coordinate guest clock base to destination host after migration

qemu tracks guest time based on vector [base_rtc, last_update], in which last_update stands for a monotonic tick which is actually uptime of the host. according to rtc implementation codes of recent releases and upstream, after migration, the time base vector [base_rtc, last_update] isn't updated to coordinate with the destionation host, ie. qemu doesnt update last_update to uptime of the destination host.
what problem have we got because of this bug? after migration, guest time may jump back to several days ago, that will make some critical business applications, such as lotus notes, malfunction.
this patch is trying to fix the problem. first, when vmsave in progress, we rtc_update_time to refresh time stamp in cmos array, then during vmrestore, we rtc_set_time to update qemu base_rtc and last_update variable according to time stamp in cmos array.

---
 hw/timer/mc146818rtc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
hw/timer/mc146818rtc.c +7 -0
index ea625f2..1df17af 100644
@@ -717,15 +717,20 @@ static void rtc_set_date_from_host(ISADevice *dev)
     rtc_set_cmos(s, &tm);
 }
 
+static void rtc_pre_save(void *opaque) 
+{
+    RTCState *s = opaque;
+
+    rtc_update_time(s);
+}
+
 static int rtc_post_load(void *opaque, int version_id)
 {

hw/timer/mc146818rtc.c static void rtc_set_date_from_host(ISADevice *dev) · line 733

Quoted 10 lines
hw/timer/mc146818rtc.c +3 -5
     RTCState *s = opaque;
 
-    if (version_id <= 2) {
-        rtc_set_time(s);
-        s->offset = 0;
-        check_update_timer(s);
-    }
+    rtc_set_time(s);
+    s->offset = 0;
+    check_update_timer(s);

This is wrong if rtc_clock is not QEMU_CLOCK_REALTIME.

Paolo

#12 lore raw

PATCH v3 1/1 MC146818 RTC: coordinate guest clock base to destination host after migration

MC146818 RTC: coordinate guest clock base to destination host after migration

qemu tracks guest time based on vector [base_rtc, last_update], in which last_update stands for a monotonic tick which is actually uptime of the host. according to rtc implementation codes of recent releases and upstream, after migration, the time base vector [base_rtc, last_update] isn't updated to coordinate with the destionation host, ie. qemu doesnt update last_update to uptime of the destination host.
what problem have we got because of this bug? after migration, guest time may jump back to several days ago, that will make some critical business applications, such as lotus notes, malfunction.
this patch is trying to fix the problem. first, when vmsave in progress, we rtc_update_time to refresh time stamp in cmos array, then during vmrestore, we rtc_set_time to update qemu base_rtc and last_update variable according to time stamp in cmos array.

---
 hw/timer/mc146818rtc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
hw/timer/mc146818rtc.c +9 -1
index ea625f2..1e0ed14 100644
@@ -717,11 +717,19 @@ static void rtc_set_date_from_host(ISADevice *dev)
     rtc_set_cmos(s, &tm);
 }
 
+static void rtc_pre_save(void *opaque) 
+{
+    RTCState *s = opaque;
+
+    rtc_update_time(s);
+}
+
 static int rtc_post_load(void *opaque, int version_id)
 {
     RTCState *s = opaque;
 
-    if (version_id <= 2) {
+    if (version_id <= 2 ||
+            rtc_clock == QEMU_CLOCK_REALTIME){
         rtc_set_time(s);
         s->offset = 0;
         check_update_timer(s);
Signature · 1 line
2.9.0.windows.1
#13 lore raw
Quoted 26 lines · On 26/09/2016 10:42, [email protected] wrote:

MC146818 RTC: coordinate guest clock base to destination host after migration

qemu tracks guest time based on vector [base_rtc, last_update], in which last_update stands for a monotonic tick which is actually uptime of the host. according to rtc implementation codes of recent releases and upstream, after migration, the time base vector [base_rtc, last_update] isn't updated to coordinate with the destionation host, ie. qemu doesnt update last_update to uptime of the destination host.
what problem have we got because of this bug? after migration, guest time may jump back to several days ago, that will make some critical business applications, such as lotus notes, malfunction.
this patch is trying to fix the problem. first, when vmsave in progress, we rtc_update_time to refresh time stamp in cmos array, then during vmrestore, we rtc_set_time to update qemu base_rtc and last_update variable according to time stamp in cmos array.

---
 hw/timer/mc146818rtc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
hw/timer/mc146818rtc.c +0 -0
index ea625f2..1e0ed14 100644

hw/timer/mc146818rtc.c static void rtc_set_date_from_host(ISADevice *dev) · line 720

Quoted 4 lines
hw/timer/mc146818rtc.c +1 -0
@@ -717,11 +717,19 @@ static void rtc_set_date_from_host(ISADevice *dev)
     rtc_set_cmos(s, &tm);
 }
 
+static void rtc_pre_save(void *opaque) 

This patch doesn't compile cleanly (rtc_pre_save is unused, which gives a warning), and also doesn't pass scripts/checkpatch.pl.

Thanks,

Paolo

Quoted 19 lines
hw/timer/mc146818rtc.c +8 -1
+{
+    RTCState *s = opaque;
+
+    rtc_update_time(s);
+}
+
 static int rtc_post_load(void *opaque, int version_id)
 {
     RTCState *s = opaque;
 
-    if (version_id <= 2) {
+    if (version_id <= 2 ||
+            rtc_clock == QEMU_CLOCK_REALTIME){
         rtc_set_time(s);
         s->offset = 0;
         check_update_timer(s);
Signature · 1 line
2.9.0.windows.1
#14 lore raw

PATCH v4 1/1 MC146818 RTC: coordinate guest clock base to destination host after migration

MC146818 RTC: coordinate guest clock base to destination host after migration

qemu tracks guest time based on vector [base_rtc, last_update], in which last_update stands for a monotonic tick which is actually uptime of the host. according to rtc implementation codes of recent releases and upstream, after migration, the time base vector [base_rtc, last_update] isn't updated to coordinate with the destionation host, ie. qemu doesnt update last_update to uptime of the destination host.
what problem have we got because of this bug? after migration, guest time may jump back to several days ago, that will make some critical business applications, such as lotus notes, malfunction.
this patch is trying to fix the problem. first, when vmsave in progress, we rtc_update_time to refresh time stamp in cmos array, then during vmrestore, we rtc_set_time to update qemu base_rtc and last_update variable according to time stamp in cmos array.

---
 hw/timer/mc146818rtc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
hw/timer/mc146818rtc.c +10 -1
index ea625f2..4e4af43 100644
@@ -717,11 +717,19 @@ static void rtc_set_date_from_host(ISADevice *dev)
     rtc_set_cmos(s, &tm);
 }
 
+static void rtc_pre_save(void *opaque)
+{
+    RTCState *s = opaque;
+
+    rtc_update_time(s);
+}
+
 static int rtc_post_load(void *opaque, int version_id)
 {
     RTCState *s = opaque;
 
-    if (version_id <= 2) {
+    if (version_id <= 2 ||
+            rtc_clock == QEMU_CLOCK_REALTIME){
         rtc_set_time(s);
         s->offset = 0;
         check_update_timer(s);
@@ -764,6 +772,7 @@ static const VMStateDescription vmstate_rtc = {
     .name = "mc146818rtc",
     .version_id = 3,
     .minimum_version_id = 1,
+    .pre_save = rtc_pre_save,
     .post_load = rtc_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_BUFFER(cmos_data, RTCState),
Signature · 1 line
2.9.0.windows.1
#15 lore raw
Quoted 57 lines · On 26/09/2016 12:54, [email protected] wrote:

MC146818 RTC: coordinate guest clock base to destination host after migration

qemu tracks guest time based on vector [base_rtc, last_update], in which last_update stands for a monotonic tick which is actually uptime of the host. according to rtc implementation codes of recent releases and upstream, after migration, the time base vector [base_rtc, last_update] isn't updated to coordinate with the destionation host, ie. qemu doesnt update last_update to uptime of the destination host.
what problem have we got because of this bug? after migration, guest time may jump back to several days ago, that will make some critical business applications, such as lotus notes, malfunction.
this patch is trying to fix the problem. first, when vmsave in progress, we rtc_update_time to refresh time stamp in cmos array, then during vmrestore, we rtc_set_time to update qemu base_rtc and last_update variable according to time stamp in cmos array.

---
 hw/timer/mc146818rtc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
hw/timer/mc146818rtc.c +10 -1
index ea625f2..4e4af43 100644
@@ -717,11 +717,19 @@ static void rtc_set_date_from_host(ISADevice *dev)
     rtc_set_cmos(s, &tm);
 }
 
+static void rtc_pre_save(void *opaque)
+{
+    RTCState *s = opaque;
+
+    rtc_update_time(s);
+}
+
 static int rtc_post_load(void *opaque, int version_id)
 {
     RTCState *s = opaque;
 
-    if (version_id <= 2) {
+    if (version_id <= 2 ||
+            rtc_clock == QEMU_CLOCK_REALTIME){
         rtc_set_time(s);
         s->offset = 0;
         check_update_timer(s);
@@ -764,6 +772,7 @@ static const VMStateDescription vmstate_rtc = {
     .name = "mc146818rtc",
     .version_id = 3,
     .minimum_version_id = 1,
+    .pre_save = rtc_pre_save,
     .post_load = rtc_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_BUFFER(cmos_data, RTCState),
Signature · 1 line
2.9.0.windows.1

Still doesn't pass scripts/checkpatch.pl. Also please avoid HTML email.

Paolo

#16 lore raw

PATCH 1/1 MC146818 RTC: Get correct guest time when irq coalesced

In reply to Paolo Bonzini

Hi, Paolo
This is another patch arround RTC. Would you please have a review.

------------------separation-------------------------------------------------

MC146818 RTC: Get correct guest time when irq coalesced

When irq coalesce occurred, irq_coalesced actually store the seconds that the time sawn in guest lags behind real guest virtual time. At this time , if guest read cmos for virtual time, it shouldn't see those delayed seconds, so we must substract irq_coalesced from guest virtual time. Otherwise, after seconds queued in irq_coalesced applied to guest, time in guest will go ahead of time it should be.

---
 hw/timer/mc146818rtc.c | 3 +++
 1 file changed, 3 insertions(+)
hw/timer/mc146818rtc.c +3 -0
index 1df17af..4cb8e5e 100644
@@ -549,6 +549,8 @@ static void rtc_set_time(RTCState *s)
     rtc_get_time(s, &tm);
     s->base_rtc = mktimegm(&tm);
     s->last_update = qemu_clock_get_ns(rtc_clock);
+ s->irq_coalesced = 0;
+ s->irq_reinject_on_ack_count = 0;
 
     qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
 }
@@ -585,6 +587,7 @@ static void rtc_update_time(RTCState *s)
 
     guest_nsec = get_guest_rtc_ns(s);
     guest_sec = guest_nsec / NANOSECONDS_PER_SECOND;
+ guest_sec -= s->irq_coalesced;
     gmtime_r(&guest_sec, &ret);
 
     /* Is SET flag of Register B disabled? */
Signature · 1 line
2.9.0.windows.1
#17 lore raw

Re: [Qemu-devel] [PATCH]MC146818 RTC: Get correct guest time when irq coalesced

Quoted 5 lines · On 26/09/2016 06:37, [email protected] wrote:

Hi, Paolo
This is another patch arround RTC. Would you please have a review.

------------------separation-------------------------------------------------

Quoted 8 lines

MC146818 RTC: Get correct guest time when irq coalesced

When irq coalesce occurred, irq_coalesced actually store the seconds that the time sawn in guest lags behind real guest virtual time. At this time , if guest read cmos for virtual time, it shouldn't see those delayed seconds, so we must substract irq_coalesced from guest virtual time. Otherwise, after seconds queued in irq_coalesced applied to guest, time in guest will go ahead of time it should be.

No, it's not seconds. It depends on the interval of the periodic interrupt.

In any case this approach is not acceptable, unfortunately; it causes the time to go backwards for the guest as soon as an interrupt is coalesced.

What is the guest that you're seeing issues with?

Thanks,

Paolo

Quoted 26 lines
---
 hw/timer/mc146818rtc.c | 3 +++
 1 file changed, 3 insertions(+)
hw/timer/mc146818rtc.c +3 -0
index 1df17af..4cb8e5e 100644
@@ -549,6 +549,8 @@ static void rtc_set_time(RTCState *s)
     rtc_get_time(s, &tm);
     s->base_rtc = mktimegm(&tm);
     s->last_update = qemu_clock_get_ns(rtc_clock);
+ s->irq_coalesced = 0;
+ s->irq_reinject_on_ack_count = 0;
 
     qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
 }
@@ -585,6 +587,7 @@ static void rtc_update_time(RTCState *s)
 
     guest_nsec = get_guest_rtc_ns(s);
     guest_sec = guest_nsec / NANOSECONDS_PER_SECOND;
+ guest_sec -= s->irq_coalesced;
     gmtime_r(&guest_sec, &ret);
 
     /* Is SET flag of Register B disabled? */
#18 lore raw

Re: [Qemu-devel] [PATCH]MC146818 RTC: Get correct guest time when irq coalesced

Quoted 7 lines

No, it's not seconds. It depends on the interval of the periodic interrupt.

In any case this approach is not acceptable, unfortunately; it causes the time to go backwards for the guest as soon as an interrupt is coalesced.

What is the guest that you're seeing issues with?

Yes, it's length depend on settings in register A. Sorry for my mistake, but the issue ought to be still exist, which make guest clock runs ahead of time.
We are testing winxp. Mechanism of windows, at least winxp, is reading cmos once for initial time while boot time, then set up period interrupt trigger.
After initialization, windows will run clock in response of period interrupt received, besides that it also read cmos seldomly for synchronization or something else. So if period interrupt is queued in qemu, clock in windows will not run. When those interrupts injected hurrily, windows clock catch up with guest virtual time quickly.

------------t1-------------------t2--t3-----------------t4-------------
^ ^ ^ ^
| | inject finish final time driven by intrs
timer stall read cmos

For example, when guest virtual time goes to t1, qemu is busy in some heavy job and cause qemu timer delayed. when t2, qemu finish jobs and is free to run timers. At this time, all period interrupts between t1 and t2 will be generated and queued in irq_coalesced. Those intrs will be injected a little faster but one by one into guest. Normally, they will be all injected completely at t3. After all timers are proceeded, guest also have chance to run. if it read cmos time at that moment, it will read t2, but actuallly it's clock is now at t1, any way, it trust rtc hardware, so it set its clock to t2, then it continue to drive clock by period interrupts, because there are still lots of queued interrupts injecting, so guest's clock runs quite quickly. When time goes to t3, all interrupt injected, but guest's clock already goes to t4, where t4-t3 equal t2-t1.

According to figure above, because: t2-t1=t4-t2 t3-t2 <<< t4-t2so: t3 <<< t4
That is to say, guest goes ahead of time.