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

[address@hidden]

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




address@hidden
 
From: Paolo Bonzini
Date: 2016-09-20 21:12
To: address@hidden; 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, address@hidden wrote:
> Hi, Paolo
> The reason that use rtc_flush_time/rtc_adjust_timebase pairs instead
> of rtc_update_time/rtc_set_time  is a trick.
> what all we do is to coordinate the base point of time line for guest on
> a new host.  So, we don't flush realtime
> of the guest when it's stopped into cmos, but only convert vector
> [base_rtc, last_update] into cmos.
 
Isn't this the same?
 
In fact, rtc_flush_time and rtc_update_time are exactly the same code, 
except that rtc_update_time sums s->offset (which is <1 second) while 
rtc_flush_time sums a fixes 500 ns.
 
Likewise for rtc_set_time and rtc_adjust_timebase, except that 
rtc_adjust_timebase leaves s->base_rtc untouched and subtracts it from 
s->last_update; rtc_set_time instead changes both.  But this makes no 
difference because, according to get_guest_rtc_ns, what matters is only 
s->base_rtc * NANOSECONDS_PER_SECOND + s->offset - s->last_update.  So, 
say rtc_set_time would set
 
    s->base_rtc = mktimegm(&tm)
    s->last_update = qemu_clock_get_ns(rtc_clock)
 
while rtc_adjust_timebase would set 
 
    s->base_rtc = source_base_rtc
    s->last_update = qemu_clock_get_ns(rtc_clock)
                     - (mktimegm(&tm) - source_base_rtc) * 
NANOSECONDS_PER_SECOND
 
Then, after rtc_adjust_timebase, get_guest_rtc_ns returns
 
  s->base_rtc * NANOSECONDS_PER_SECOND + guest_clock - s->last_update + 
s->offset
  = source_base_rtc * NANOSECONDS_PER_SECOND + guest_clock
    - qemu_clock_get_ns(rtc_clock)
    + (mktimegm(&tm) - source_base_rtc) * NANOSECONDS_PER_SECOND
    + s->offset
  = mktimegm(&tm)  * NANOSECONDS_PER_SECOND + guest_clock
    - qemu_clock_get_ns(rtc_clock)
    + s->offset
 
and this is exactly what you'd get after rtc_set_time.
 
So I don't understand what's the difference, except for rounding the
nanoseconds component.
 
> On the other hand, the problem of rtc_update_time is it add time up plus
> s->offset, then when rtc_set_time 
> recalculate new last_update, it actually introduce s->offset into base
> vector [base_rtc, last_update].  further, 
> when guest continue to run and read realtime from cmos, rtc_update_time
> will add s->offset again, so s->offset
> is doubled.
 
This is true.  In fact rtc_post_load is already setting s->offset = 0 after
calling rtc_set_time.  Thus the load-side part of the patch can be simply
 
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index ea625f2..dd4ef5c 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -721,7 +722,7 @@ static int rtc_post_load(void *opaque, int version_id)
{
     RTCState *s = opaque;
-    if (version_id <= 2) {
+    if (rtc_clock == QEMU_CLOCK_REALTIME || version_id <= 2) {
         rtc_set_time(s);
         s->offset = 0;
         check_update_timer(s);
 
 
Thanks,
 
Paolo