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

[Stefano Stabellini]

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.

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.