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 zhong...@sangfor.com.cn From: Paolo Bonzini Date: 2016-09-20 21:12 To: zhong...@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, zhong...@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