> -----Original Message----- > From: Stefano Stabellini [mailto:stefano.stabellini@xxxxxxxxxxxxx] > Sent: Wednesday, March 21, 2012 1:39 AM > To: Zhang, Yang Z > Cc: qemu-devel@xxxxxxxxxx; Paolo Bonzini; aliguori@xxxxxxxxxx; > kvm@xxxxxxxxxxxxxxx > Subject: Re: [Qemu-devel] [PATCH v4 4/7] RTC: Set internal millisecond register to > 500ms when reset divider > > On Mon, 19 Mar 2012, Zhang, Yang Z wrote: > > The first update cycle begins one - half seconds later when divider reset is > removing. > > > > Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx> > > --- > > hw/mc146818rtc.c | 38 +++++++++++++++++++++++++++++++++----- > > 1 files changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c > > index 6ebb8f6..5e7fbb5 100644 > > --- a/hw/mc146818rtc.c > > +++ b/hw/mc146818rtc.c > > @@ -110,6 +110,8 @@ static void rtc_set_time(RTCState *s); > > static void rtc_calibrate_time(RTCState *s); > > static void rtc_set_cmos(RTCState *s); > > > > +static int32_t divider_reset; > > this should be part of RTCState > > > > static uint64_t get_guest_rtc_us(RTCState *s) > > { > > int64_t host_usec, offset_usec, guest_usec; > > @@ -220,16 +222,24 @@ static void rtc_periodic_timer(void *opaque) > > } > > } > > > > -static void rtc_set_offset(RTCState *s) > > +static void rtc_set_offset(RTCState *s, int32_t start_usec) > > I noticed that you are always passing a positive number or 0 as > start_usec: it might be worth turning start_usec into a uint32_t or > uint64_t to avoid integer signedness errors. Agree. > Also it is not clear to me what this start_usec is supposed to be: if it > is a delay to be added to the guest time, then it is best to rename it > to "delay_usec" and add a comment on top to explain what it is for. Actually, the start_usec only used when divider is changed from reset to an valid time base. When the changing happened, the first update cycle is 500ms later, so the start_usec equals to 500ms. When pass 0 as start_usec, it means the rtc internal millisecond is not changed, it is synchronized with host's time. > > > struct tm *tm = &s->current_tm; > > - int64_t host_usec, guest_sec, guest_usec; > > + int64_t host_usec, guest_sec, guest_usec, offset_usec, old_guest_usec; > > > > host_usec = qemu_get_clock_ns(host_clock) / NS_PER_USEC; > > + offset_usec = s->offset_sec * USEC_PER_SEC + s->offset_usec; > > + old_guest_usec = (host_usec + offset_usec) % USEC_PER_SEC; > > > > guest_sec = mktimegm(tm); > > - guest_usec = guest_sec * USEC_PER_SEC; > > > > + /* start_usec equal 0 means rtc internal millisecond is > > + * same with before */ > > + if (start_usec == 0) { > > + guest_usec = guest_sec * USEC_PER_SEC + old_guest_usec; > > + } else { > > + guest_usec = guest_sec * USEC_PER_SEC + start_usec; > > + } > > This looks like a mistake to me: before guest_usec was derived > exclusively from mktimegm (take or leave USEC_PER_SEC), while now > guest_usec is the sum of the value returned by mktimegm and > old_guest_usec, even when start_usec is 0. > Are you sure that this is correct? The logic is right. Before this patch, we assume the rtc internal millisecond register is same with host time, so we don't need to consider it and using mktimeis enough. Now, the rtc internal millisecond can be changed, so I use the old_guest_usec to record the current internal millisecond. When start_usec is 0, it means we don't need to change the internal millisecond register and the offset_sec is same as before. best regards yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
