Re: [Qemu-devel] [PATCH v4 4/7] RTC: Set internal millisecond register to 500ms when reset divider

[Stefano Stabellini]

On Wed, 21 Mar 2012, Zhang, Yang Z wrote:
> > >      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.
 
I am starting to understand the intention of this code, but I am still
unconvinced that the start_usec == 0 case is correct.
If I am not mistaken you are calculating:

offset = guest + old_guest - host

while it should be:

offset = guest + old_start - host

where old_start is start_usec as it was set last time, correct? Am I
missing something? In any case please add a comment to explain what the
change is trying to accomplish.