[v4,4/7] RTC: Set internal millisecond register to 500ms when reset divider

The first update cycle begins one - half seconds later when divider reset is removing.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 hw/mc146818rtc.c |   38 +++++++++++++++++++++++++++++++++-----
 1 files changed, 33 insertions(+), 5 deletions(-)

--
1.7.1

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@Intel.com>
> ---
>  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.

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.


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



>      s->offset_sec = (guest_usec - host_usec) / USEC_PER_SEC;
>      s->offset_usec = (guest_usec - host_usec) % USEC_PER_SEC;
>  }
> @@ -260,10 +270,22 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>              /* if in set mode, do not update the time */
>              if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
>                  rtc_set_time(s);
> -                rtc_set_offset(s);
> +                rtc_set_offset(s, 0);
>              }
>              break;
>          case RTC_REG_A:
> +            /* when the divider reset is removed, the first update cycle
> +             * begins one-half second later*/
> +            if (((s->cmos_data[RTC_REG_A] & 0x60) == 0x60) &&
> +                                    ((data & 0x70) >> 4) <= 2) {
> +                divider_reset = 1;
> +                if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
> +                    rtc_calibrate_time(s);
> +                    rtc_set_offset(s, 500000);
> +                    s->cmos_data[RTC_REG_A] &= ~REG_A_UIP;
> +                    divider_reset = 0;
> +                }
> +            }

This code is new: does it mean we were not handling divider reset
correctly before?
Also if we are trying to handle the DV registers, shouldn't we emulated
the other RTC frequencies as well? If so, we need a scale factor, in
addition to an offset to QEMU rtc_clock.

Il 20/03/2012 18:39, Stefano Stabellini ha scritto:
> This code is new: does it mean we were not handling divider reset
> correctly before?
> Also if we are trying to handle the DV registers, shouldn't we emulated
> the other RTC frequencies as well? If so, we need a scale factor, in
> addition to an offset to QEMU rtc_clock.

The other frequencies are never used in practice.  But divider reset's
500ms delay can be used in tests.

Paolo

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Wednesday, March 21, 2012 1:39 AM
> To: Zhang, Yang Z
> Cc: qemu-devel@nongnu.org; Paolo Bonzini; aliguori@us.ibm.com;
> kvm@vger.kernel.org
> 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@Intel.com>
> > ---
> >  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

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.