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.
--
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
