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

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

[Paolo Bonzini]

On 20/09/2016 09:19, zhongjun@sangfor.com.cn 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

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

[zhongjun]

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



zhongjun@sangfor.com.cn
 
From: Paolo Bonzini
Date: 2016-09-20 16:17
To: zhongjun@sangfor.com.cn; qemu-devel
CC: Michael S. Tsirkin
Subject: Re: [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration
 
 
On 20/09/2016 09:19, zhongjun@sangfor.com.cn 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

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

[Paolo Bonzini]

On 20/09/2016 10:34, zhongjun@sangfor.com.cn 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

> ------------------------------------------------------------------------
> zhongjun@sangfor.com.cn
> 
>      
>     *From:* Paolo Bonzini <mailto:pbonzini@redhat.com>
>     *Date:* 2016-09-20 16:17
>     *To:* zhongjun@sangfor.com.cn <mailto:zhongjun@sangfor.com.cn>;
>     qemu-devel <mailto:qemu-devel@nongnu.org>
>     *CC:* Michael S. Tsirkin <mailto:mst@redhat.com>
>     *Subject:* Re: [PATCH]MC146818 RTC: coordinate guest clock base to
>     destination host after migration
>      
>      
>     On 20/09/2016 09:19, zhongjun@sangfor.com.cn 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
> 

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

[zhongjun]

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

---

>   On 20/09/2016 09:19, zhongjun@sangfor.com.cn 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

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

[Paolo Bonzini]

On 20/09/2016 14:54, zhongjun@sangfor.com.cn 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

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

[zhongjun]

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




zhongjun@sangfor.com.cn
 
From: Paolo Bonzini
Date: 2016-09-20 21:12
To: zhongjun@sangfor.com.cn; qemu-devel
CC: Michael S. Tsirkin
Subject: Re: [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration
 
 
On 20/09/2016 14:54, zhongjun@sangfor.com.cn 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

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

[zhongjun]

By the way, is this full patch of you?
---
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);
 ---

Could you send me the complete patch?



zhongjun@sangfor.com.cn
 
From: zhongjun@sangfor.com.cn
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




zhongjun@sangfor.com.cn
 
From: Paolo Bonzini
Date: 2016-09-20 21:12
To: zhongjun@sangfor.com.cn; qemu-devel
CC: Michael S. Tsirkin
Subject: Re: [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration
 
 
On 20/09/2016 14:54, zhongjun@sangfor.com.cn 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

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

[Paolo Bonzini]

On 21/09/2016 09:36, zhongjun@sangfor.com.cn 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).

> 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.

> 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.

> 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

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

[zhongjun]

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.



zhongjun@sangfor.com.cn
 
From: Paolo Bonzini
Date: 2016-09-21 23:15
To: zhongjun@sangfor.com.cn; qemu-devel
CC: Michael S. Tsirkin
Subject: Re: [PATCH]MC146818 RTC: coordinate guest clock base to destination host after migration
 
 
On 21/09/2016 09:36, zhongjun@sangfor.com.cn 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).
 
> 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.
 
> 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.
 
> 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

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

[zhongjun]

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.

Signed-off-by: Junlian Bell <zhongjun@sangfor.com.cn>
---
 hw/timer/mc146818rtc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index ea625f2..1df17af 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -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 ||
-- 
2.9.0.windows.1

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

[zhongjun]

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(+)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 1df17af..4cb8e5e 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -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? */
-- 
2.9.0.windows.1

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

[Paolo Bonzini]

On 26/09/2016 05:37, zhongjun@sangfor.com.cn 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.
> 
> Signed-off-by: Junlian Bell <zhongjun@sangfor.com.cn>
> ---
>  hw/timer/mc146818rtc.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index ea625f2..1df17af 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -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);

This is wrong if rtc_clock is not QEMU_CLOCK_REALTIME.

Paolo

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

[Paolo Bonzini]

On 26/09/2016 06:37, zhongjun@sangfor.com.cn wrote:
> 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.

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

> ---
>  hw/timer/mc146818rtc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 1df17af..4cb8e5e 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -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? */
> 

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

[zhongjun]

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.

Signed-off-by: Junlian Bell <zhongjun@sangfor.com.cn>
---
 hw/timer/mc146818rtc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index ea625f2..1e0ed14 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -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);
-- 
2.9.0.windows.1

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

[Paolo Bonzini]

On 26/09/2016 10:42, zhongjun@sangfor.com.cn 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.
> 
> Signed-off-by: Junlian Bell <zhongjun@sangfor.com.cn>
> ---
>  hw/timer/mc146818rtc.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index ea625f2..1e0ed14 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -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

> +{
> +    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);
> -- 
> 2.9.0.windows.1
> 

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

[zhongjun]

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.

Signed-off-by: Junlian Bell <zhongjun@sangfor.com.cn>
---
 hw/timer/mc146818rtc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index ea625f2..4e4af43 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -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),
-- 
2.9.0.windows.1

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

[Paolo Bonzini]

On 26/09/2016 12:54, zhongjun@sangfor.com.cn 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.
> 
> Signed-off-by: Junlian Bell <zhongjun@sangfor.com.cn>
> ---
>  hw/timer/mc146818rtc.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index ea625f2..4e4af43 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -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),
> -- 
> 2.9.0.windows.1
> 

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

Paolo

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

[zhongjun]

>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.