MC146818 RTC: coordinate guest clock base to destination host after migration
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 2 lines · On 20/09/2016 09:19, [email protected] wrote:
qemu tracks guest time based on vector [
base_rtc,last_update], in whichlast_updatestands 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
yes, the full options on clock is '-rtc driftfix=slew,clock=rt,base=localtime'
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 whichlast_updatestands 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
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
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 whichlast_updatestands for a monotonic tick which is actually uptime of the host.But
last_updateis 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 tortc_set_timeshould be conditional onrtc_clock==QEMU_CLOCK_REALTIME.Paolo
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_SECONDThen, 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_timeis it add time up pluss->offset, then whenrtc_set_time
recalculate newlast_update, it actually introduces->offsetinto base vector [base_rtc,last_update]. further,
when guest continue to run and read realtime from cmos,rtc_update_timewill adds->offsetagain, sos->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
@@ -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
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
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_SECONDThen, 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_timeis it add time up pluss->offset, then whenrtc_set_time
recalculate newlast_update, it actually introduces->offsetinto base vector [base_rtc,last_update]. further,
when guest continue to run and read realtime from cmos,rtc_update_timewill adds->offsetagain, sos->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
@@ -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
By the way, is this full patch of you?
---
hw/timer/mc146818rtc.c +0 -0
@@ -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?
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
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_SECONDThen, 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_timeis it add time up pluss->offset, then whenrtc_set_time
recalculate newlast_update, it actually introduces->offsetinto base vector [base_rtc,last_update]. further,
when guest continue to run and read realtime from cmos,rtc_update_timewill adds->offsetagain, sos->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
@@ -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
Quoted 4 lines · On 21/09/2016 09:36, [email protected] wrote:
If we change
base_rtcwhile 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->offsetwill 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 fixedbase_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
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.
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_rtcwhile 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->offsetwill 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 fixedbase_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
PATCH v2 1/1 MC146818 RTC: coordinate guest clock base to destination host after migration
In reply to Paolo BonziniHi, 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
@@ -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
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 whichlast_updatestands 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 updatelast_updateto 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, wertc_update_timeto refresh time stamp in cmos array, then during vmrestore, wertc_set_timeto update qemubase_rtcandlast_updatevariable according to time stamp in cmos array.Signed-off-by Junlian Bell <[email protected]>--- hw/timer/mc146818rtc.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)hw/timer/mc146818rtc.c +7
-0@@ -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
-5RTCState *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
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
@@ -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
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 whichlast_updatestands 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 updatelast_updateto 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, wertc_update_timeto refresh time stamp in cmos array, then during vmrestore, wertc_set_timeto update qemubase_rtcandlast_updatevariable according to time stamp in cmos array.Signed-off-by Junlian Bell <[email protected]>--- hw/timer/mc146818rtc.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)hw/timer/mc146818rtc.c +0
-0
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
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
@@ -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
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 whichlast_updatestands 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 updatelast_updateto 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, wertc_update_timeto refresh time stamp in cmos array, then during vmrestore, wertc_set_timeto update qemubase_rtcandlast_updatevariable according to time stamp in cmos array.Signed-off-by Junlian Bell <[email protected]>--- hw/timer/mc146818rtc.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)hw/timer/mc146818rtc.c +10
-1@@ -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
PATCH 1/1 MC146818 RTC: Get correct guest time when irq coalesced
In reply to Paolo BonziniHi, 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
@@ -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
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_coalescedactually 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 substractirq_coalescedfrom guest virtual time. Otherwise, after seconds queued inirq_coalescedapplied 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@@ -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? */
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.