qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 4/7] RTC: Set internal millisecond register t


From: Zhang, Yang Z
Subject: Re: [Qemu-devel] [PATCH v4 4/7] RTC: Set internal millisecond register to 500ms when reset divider
Date: Wed, 21 Mar 2012 07:42:49 +0000

> -----Original Message-----
> From: Stefano Stabellini [mailto:address@hidden
> Sent: Wednesday, March 21, 2012 1:39 AM
> To: Zhang, Yang Z
> Cc: address@hidden; Paolo Bonzini; address@hidden;
> address@hidden
> 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 <address@hidden>
> > ---
> >  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]