Re: [Qemu-devel] [PATCH v4 2/7] RTC: Update the RTC clock only when reading it

[Stefano Stabellini]

On Mon, 19 Mar 2012, Zhang, Yang Z wrote:
> There has no need to use two periodic timer to update RTC time. In this 
> patch, we only update it when guest reading it.

So the basic idea here is that we don't need to two periodic timers
because we are going to calculate the RTC guest time from QEMU's
host_clock.

I only have a couple of observations:

- shouldn't we use QEMU rtc_clock, rather than host_clock?

- it would be better to use shifts rather than divisions whenever
possible, they are much cheaper;

- rtc_calibrate_time seems to be the new functions that updates the
guest rtc time based on QEMU host_clock. Are you sure we are calling
it all the times we need to call it? Could we just call it at the
beginning of cmos_ioport_write and cmos_ioport_read?



> Signed-off-by: Yang Zhang <address@hidden>
> ---
>  hw/mc146818rtc.c |  207 
> +++++++++++++++++-------------------------------------
>  1 files changed, 66 insertions(+), 141 deletions(-)
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 9b49cbc..82a5b8a 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -44,6 +44,9 @@
>  # define DPRINTF_C(format, ...)      do { } while (0)
>  #endif
> 
> +#define USEC_PER_SEC    1000000L
> +#define NS_PER_USEC     1000L
> +
>  #define RTC_REINJECT_ON_ACK_COUNT 20
> 
>  #define RTC_SECONDS             0
> @@ -85,6 +88,8 @@ typedef struct RTCState {
>      uint8_t cmos_data[128];
>      uint8_t cmos_index;
>      struct tm current_tm;
> +    int64_t offset_sec;
> +    int32_t offset_usec;
>      int32_t base_year;
>      qemu_irq irq;
>      qemu_irq sqw_irq;
> @@ -92,21 +97,29 @@ typedef struct RTCState {
>      /* periodic timer */
>      QEMUTimer *periodic_timer;
>      int64_t next_periodic_time;
> -    /* second update */
> -    int64_t next_second_time;
>      uint16_t irq_reinject_on_ack_count;
>      uint32_t irq_coalesced;
>      uint32_t period;
>      QEMUTimer *coalesced_timer;
> -    QEMUTimer *second_timer;
> -    QEMUTimer *second_timer2;
>      Notifier clock_reset_notifier;
>      LostTickPolicy lost_tick_policy;
>      Notifier suspend_notifier;
>  } RTCState;
> 
>  static void rtc_set_time(RTCState *s);
> -static void rtc_copy_date(RTCState *s);
> +static void rtc_calibrate_time(RTCState *s);
> +static void rtc_set_cmos(RTCState *s);
> +
> +static uint64_t get_guest_rtc_us(RTCState *s)
> +{
> +    int64_t host_usec, offset_usec, guest_usec;
> +
> +    host_usec = qemu_get_clock_ns(host_clock) / NS_PER_USEC;
> +    offset_usec = s->offset_sec * USEC_PER_SEC + s->offset_usec;
> +    guest_usec = host_usec + offset_usec;
> +
> +    return guest_usec;
> +}
> 
>  #ifdef TARGET_I386
>  static void rtc_coalesced_timer_update(RTCState *s)
> @@ -207,6 +220,20 @@ static void rtc_periodic_timer(void *opaque)
>      }
>  }
> 
> +static void rtc_set_offset(RTCState *s)
> +{
> +    struct tm *tm = &s->current_tm;
> +    int64_t host_usec, guest_sec, guest_usec;
> +
> +    host_usec = qemu_get_clock_ns(host_clock) / NS_PER_USEC;
> +
> +    guest_sec = mktimegm(tm);
> +    guest_usec = guest_sec * USEC_PER_SEC;
> +
> +    s->offset_sec = (guest_usec - host_usec) / USEC_PER_SEC;
> +    s->offset_usec = (guest_usec - host_usec) % USEC_PER_SEC;
> +}
> +
>  static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>  {
>      RTCState *s = opaque;
> @@ -233,6 +260,7 @@ static void cmos_ioport_write(void *opaque, uint32_t 
> addr, u                                                                       
>                int32_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);
>              }
>              break;
>          case RTC_REG_A:
> @@ -243,6 +271,11 @@ static void cmos_ioport_write(void *opaque, uint32_t 
> addr,                                                                         
>               uint32_t data)
>              break;
>          case RTC_REG_B:
>              if (data & REG_B_SET) {
> +                /* update cmos to when the rtc was stopping */
> +                if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
> +                    rtc_calibrate_time(s);
> +                    rtc_set_cmos(s);
> +                }
>                  /* set mode: reset UIP mode */
>                  s->cmos_data[RTC_REG_A] &= ~REG_A_UIP;
>                  data &= ~REG_B_UIE;
> @@ -250,6 +283,7 @@ static void cmos_ioport_write(void *opaque, uint32_t 
> addr, u                                                                       
>                int32_t data)
>                  /* if disabling set mode, update the time */
>                  if (s->cmos_data[RTC_REG_B] & REG_B_SET) {
>                      rtc_set_time(s);
> +                    rtc_set_offset(s);
>                  }
>              }
>              s->cmos_data[RTC_REG_B] = data;
> @@ -305,7 +339,7 @@ static void rtc_set_time(RTCState *s)
>      rtc_change_mon_event(tm);
>  }
> 
> -static void rtc_copy_date(RTCState *s)
> +static void rtc_set_cmos(RTCState *s)
>  {
>      const struct tm *tm = &s->current_tm;
>      int year;
> @@ -331,122 +365,16 @@ static void rtc_copy_date(RTCState *s)
>      s->cmos_data[RTC_YEAR] = rtc_to_bcd(s, year);
>  }
> 
> -/* month is between 0 and 11. */
> -static int get_days_in_month(int month, int year)
> -{
> -    static const int days_tab[12] = {
> -        31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
> -    };
> -    int d;
> -    if ((unsigned )month >= 12)
> -        return 31;
> -    d = days_tab[month];
> -    if (month == 1) {
> -        if ((year % 4) == 0 && ((year % 100) != 0 || (year % 400) == 0))
> -            d++;
> -    }
> -    return d;
> -}
> -
> -/* update 'tm' to the next second */
> -static void rtc_next_second(struct tm *tm)
> +static void rtc_calibrate_time(RTCState *s)
>  {
> -    int days_in_month;
> -
> -    tm->tm_sec++;
> -    if ((unsigned)tm->tm_sec >= 60) {
> -        tm->tm_sec = 0;
> -        tm->tm_min++;
> -        if ((unsigned)tm->tm_min >= 60) {
> -            tm->tm_min = 0;
> -            tm->tm_hour++;
> -            if ((unsigned)tm->tm_hour >= 24) {
> -                tm->tm_hour = 0;
> -                /* next day */
> -                tm->tm_wday++;
> -                if ((unsigned)tm->tm_wday >= 7)
> -                    tm->tm_wday = 0;
> -                days_in_month = get_days_in_month(tm->tm_mon,
> -                                                  tm->tm_year + 1900);
> -                tm->tm_mday++;
> -                if (tm->tm_mday < 1) {
> -                    tm->tm_mday = 1;
> -                } else if (tm->tm_mday > days_in_month) {
> -                    tm->tm_mday = 1;
> -                    tm->tm_mon++;
> -                    if (tm->tm_mon >= 12) {
> -                        tm->tm_mon = 0;
> -                        tm->tm_year++;
> -                    }
> -                }
> -            }
> -        }
> -    }
> -}
> -
> -
> -static void rtc_update_second(void *opaque)
> -{
> -    RTCState *s = opaque;
> -    int64_t delay;
> -
> -    /* if the oscillator is not in normal operation, we do not update */
> -    if ((s->cmos_data[RTC_REG_A] & 0x70) != 0x20) {
> -        s->next_second_time += get_ticks_per_sec();
> -        qemu_mod_timer(s->second_timer, s->next_second_time);
> -    } else {
> -        rtc_next_second(&s->current_tm);
> -
> -        if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
> -            /* update in progress bit */
> -            s->cmos_data[RTC_REG_A] |= REG_A_UIP;
> -        }
> -        /* should be 244 us = 8 / 32768 seconds, but currently the
> -           timers do not have the necessary resolution. */
> -        delay = (get_ticks_per_sec() * 1) / 100;
> -        if (delay < 1)
> -            delay = 1;
> -        qemu_mod_timer(s->second_timer2,
> -                       s->next_second_time + delay);
> -    }
> -}
> -
> -static void rtc_update_second2(void *opaque)
> -{
> -    RTCState *s = opaque;
> -
> -    if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
> -        rtc_copy_date(s);
> -    }
> -
> -    /* check alarm */
> -    if (((s->cmos_data[RTC_SECONDS_ALARM] & 0xc0) == 0xc0 ||
> -         rtc_from_bcd(s, s->cmos_data[RTC_SECONDS_ALARM]) == 
> s->current_tm.tm_s                                                            
>                           ec) &&
> -        ((s->cmos_data[RTC_MINUTES_ALARM] & 0xc0) == 0xc0 ||
> -         rtc_from_bcd(s, s->cmos_data[RTC_MINUTES_ALARM]) == 
> s->current_tm.tm_m                                                            
>                           in) &&
> -        ((s->cmos_data[RTC_HOURS_ALARM] & 0xc0) == 0xc0 ||
> -         rtc_from_bcd(s, s->cmos_data[RTC_HOURS_ALARM]) == 
> s->current_tm.tm_hou                                                          
>                             r)) {
> -
> -        s->cmos_data[RTC_REG_C] |= REG_C_AF;
> -        if (s->cmos_data[RTC_REG_B] & REG_B_AIE) {
> -            qemu_system_wakeup_request(QEMU_WAKEUP_REASON_RTC);
> -            qemu_irq_raise(s->irq);
> -            s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
> -        }
> -    }
> -
> -    /* update ended interrupt */
> -    s->cmos_data[RTC_REG_C] |= REG_C_UF;
> -    if (s->cmos_data[RTC_REG_B] & REG_B_UIE) {
> -        s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
> -        qemu_irq_raise(s->irq);
> -    }
> -
> -    /* clear update in progress bit */
> -    s->cmos_data[RTC_REG_A] &= ~REG_A_UIP;
> -
> -    s->next_second_time += get_ticks_per_sec();
> -    qemu_mod_timer(s->second_timer, s->next_second_time);
> +    struct tm *ret;
> +    time_t guest_sec;
> +    int64_t guest_usec;
> +
> +    guest_usec = get_guest_rtc_us(s);
> +    guest_sec = guest_usec / USEC_PER_SEC;
> +    ret = gmtime(&guest_sec);
> +    s->current_tm = *ret;
>  }
> 
>  static uint32_t cmos_ioport_read(void *opaque, uint32_t addr)
> @@ -464,6 +392,12 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t 
> add                                                                           
>            r)
>          case RTC_DAY_OF_MONTH:
>          case RTC_MONTH:
>          case RTC_YEAR:
> +            /* if not in set mode, calibrate cmos before
> +             * reading*/
> +            if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
> +                rtc_calibrate_time(s);
> +                rtc_set_cmos(s);
> +            }
>              ret = s->cmos_data[s->cmos_index];
>              break;
>          case RTC_REG_A:
> @@ -507,13 +441,6 @@ void rtc_set_memory(ISADevice *dev, int addr, int val)
>          s->cmos_data[addr] = val;
>  }
> 
> -void rtc_set_date(ISADevice *dev, const struct tm *tm)
> -{
> -    RTCState *s = DO_UPCAST(RTCState, dev, dev);
> -    s->current_tm = *tm;
> -    rtc_copy_date(s);
> -}
> -
>  /* PC cmos mappings */
>  #define REG_IBM_CENTURY_BYTE        0x32
>  #define REG_IBM_PS2_CENTURY_BYTE    0x37
> @@ -524,9 +451,14 @@ static void rtc_set_date_from_host(ISADevice *dev)
>      struct tm tm;
>      int val;
> 
> -    /* set the CMOS date */
>      qemu_get_timedate(&tm, 0);
> -    rtc_set_date(dev, &tm);
> +
> +    s->offset_sec = mktimegm(&tm) - time(NULL);
> +    s->offset_usec = 0;
> +
> +    /* set the CMOS date */
> +    s->current_tm = tm;
> +    rtc_set_cmos(s);
> 
>      val = rtc_to_bcd(s, (tm.tm_year / 100) + 19);
>      rtc_set_memory(dev, REG_IBM_CENTURY_BYTE, val);
> @@ -563,11 +495,10 @@ static const VMStateDescription vmstate_rtc = {
>          VMSTATE_INT32(current_tm.tm_mday, RTCState),
>          VMSTATE_INT32(current_tm.tm_mon, RTCState),
>          VMSTATE_INT32(current_tm.tm_year, RTCState),
> +        VMSTATE_INT64(offset_sec, RTCState),
> +        VMSTATE_INT32(offset_usec, RTCState),
>          VMSTATE_TIMER(periodic_timer, RTCState),
>          VMSTATE_INT64(next_periodic_time, RTCState),
> -        VMSTATE_INT64(next_second_time, RTCState),
> -        VMSTATE_TIMER(second_timer, RTCState),
> -        VMSTATE_TIMER(second_timer2, RTCState),
>          VMSTATE_UINT32_V(irq_coalesced, RTCState, 2),
>          VMSTATE_UINT32_V(period, RTCState, 2),
>          VMSTATE_END_OF_LIST()
> @@ -580,8 +511,6 @@ static void rtc_notify_clock_reset(Notifier *notifier, 
> void                                                                          
>              *data)
>      int64_t now = *(int64_t *)data;
> 
>      rtc_set_date_from_host(&s->dev);
> -    s->next_second_time = now + (get_ticks_per_sec() * 99) / 100;
> -    qemu_mod_timer(s->second_timer2, s->next_second_time);
>      rtc_timer_update(s, now);
>  #ifdef TARGET_I386
>      if (s->lost_tick_policy == LOST_TICK_SLEW) {
> @@ -636,6 +565,8 @@ static void rtc_get_date(Object *obj, Visitor *v, void 
> *opaq                                                                         
>              ue,
>      ISADevice *isa = ISA_DEVICE(obj);
>      RTCState *s = DO_UPCAST(RTCState, dev, isa);
> 
> +    rtc_calibrate_time(s);
> +    rtc_set_cmos(s);
>      visit_start_struct(v, NULL, "struct tm", name, 0, errp);
>      visit_type_int32(v, &s->current_tm.tm_year, "tm_year", errp);
>      visit_type_int32(v, &s->current_tm.tm_mon, "tm_mon", errp);
> @@ -672,8 +603,6 @@ static int rtc_initfn(ISADevice *dev)
>  #endif
> 
>      s->periodic_timer = qemu_new_timer_ns(rtc_clock, rtc_periodic_timer, s);
> -    s->second_timer = qemu_new_timer_ns(rtc_clock, rtc_update_second, s);
> -    s->second_timer2 = qemu_new_timer_ns(rtc_clock, rtc_update_second2, s);
> 
>      s->clock_reset_notifier.notify = rtc_notify_clock_reset;
>      qemu_register_clock_reset_notifier(rtc_clock, &s->clock_reset_notifier);
> @@ -681,10 +610,6 @@ static int rtc_initfn(ISADevice *dev)
>      s->suspend_notifier.notify = rtc_notify_suspend;
>      qemu_register_suspend_notifier(&s->suspend_notifier);
> 
> -    s->next_second_time =
> -        qemu_get_clock_ns(rtc_clock) + (get_ticks_per_sec() * 99) / 100;
> -    qemu_mod_timer(s->second_timer2, s->next_second_time);
> -
>      memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
>      isa_register_ioport(dev, &s->io, base);
> 
> --
> 1.7.1
>