Re: [Qemu-devel] [PATCH v4 5/7] RTC:Add RTC update-ended interrupt support

[Stefano Stabellini]

On Mon, 19 Mar 2012, Zhang, Yang Z wrote:
> Use a timer to emulate update cycle. When update cycle ended and UIE is 
> setting, then raise an interrupt. The timer runs only when UF or AF is 
> cleared.

The idea is that if the user requests the update-ended interrupt (UIE)
we setup a timer to inject it at the right time into the guest and
another timer to update the UIP bit, correct?
But do we actually need the second timer? Why can't we just update the
UIP bit whenever the user tries to read reg A, as we do when UIE is not set?


Also I am not sure it is a great idea to rename rtc_timer_update into
periodic_timer_update in the same patch where you introduce this new
logic.


> Signed-off-by: Yang Zhang <address@hidden>
> ---
>  hw/mc146818rtc.c |   86 
> ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 80 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 5e7fbb5..fae049e 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -97,6 +97,11 @@ typedef struct RTCState {
>      /* periodic timer */
>      QEMUTimer *periodic_timer;
>      int64_t next_periodic_time;
> +    /* update-ended timer */
> +    QEMUTimer *update_timer;
> +    QEMUTimer *update_timer2;
> +    uint64_t next_update_time;
> +    uint32_t use_timer;
>      uint16_t irq_reinject_on_ack_count;
>      uint32_t irq_coalesced;
>      uint32_t period;
> @@ -157,7 +162,8 @@ static void rtc_coalesced_timer(void *opaque)
>  }
>  #endif
> 
> -static void rtc_timer_update(RTCState *s, int64_t current_time)
> +/* handle periodic timer */
> +static void periodic_timer_update(RTCState *s, int64_t current_time)
>  {
>      int period_code, period;
>      int64_t cur_clock, next_irq_clock;
> @@ -195,7 +201,7 @@ static void rtc_periodic_timer(void *opaque)
>  {
>      RTCState *s = opaque;
> 
> -    rtc_timer_update(s, s->next_periodic_time);
> +    periodic_timer_update(s, s->next_periodic_time);
>      s->cmos_data[RTC_REG_C] |= REG_C_PF;
>      if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
>          s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
> @@ -222,6 +228,58 @@ static void rtc_periodic_timer(void *opaque)
>      }
>  }
> 
> +/* handle update-ended timer */
> +static void check_update_timer(RTCState *s)
> +{
> +    uint64_t next_update_time, expire_time;
> +    uint64_t guest_usec;
> +    qemu_del_timer(s->update_timer);
> +    qemu_del_timer(s->update_timer2);
> +
> +    if (!((s->cmos_data[RTC_REG_C] & (REG_C_UF | REG_C_AF)) ==
> +            (REG_C_UF | REG_C_AF)) && !(s->cmos_data[RTC_REG_B] & 
> REG_B_SET)) {
> +        s->use_timer = 1;
> +        guest_usec = get_guest_rtc_us(s) % USEC_PER_SEC;
> +        if (guest_usec >= (USEC_PER_SEC - 244)) {
> +            /* RTC is in update cycle when enabling UIE */
> +            s->cmos_data[RTC_REG_A] |= REG_A_UIP;
> +            next_update_time = (USEC_PER_SEC - guest_usec) * NS_PER_USEC;
> +            expire_time = qemu_get_clock_ns(rtc_clock) + next_update_time;
> +            qemu_mod_timer(s->update_timer2, expire_time);
> +        } else {
> +            next_update_time = (USEC_PER_SEC - guest_usec - 244) * 
> NS_PER_USEC;
> +            expire_time = qemu_get_clock_ns(rtc_clock) + next_update_time;
> +            s->next_update_time = expire_time;
> +            qemu_mod_timer(s->update_timer, expire_time);
> +        }
> +    } else {
> +        s->use_timer = 0;
> +    }
> +}
> +static void rtc_update_timer(void *opaque)
> +{
> +    RTCState *s = opaque;
> +
> +    if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
> +        s->cmos_data[RTC_REG_A] |= REG_A_UIP;
> +        qemu_mod_timer(s->update_timer2, s->next_update_time + 244000UL);
> +    }
> +}
> +
> +static void rtc_update_timer2(void *opaque)
> +{
> +    RTCState *s = opaque;
> +
> +    if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
> +        s->cmos_data[RTC_REG_C] |= REG_C_UF;
> +        s->cmos_data[RTC_REG_A] &= ~REG_A_UIP;
> +        s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
> +        qemu_irq_raise(s->irq);
> +    }
> +    check_update_timer(s);
> +}
> +
>  static void rtc_set_offset(RTCState *s, int32_t start_usec)
>  {
>      struct tm *tm = &s->current_tm;
> @@ -283,13 +341,14 @@ static void cmos_ioport_write(void *opaque, uint32_t 
> addr, uint32_t data)
>                      rtc_calibrate_time(s);
>                      rtc_set_offset(s, 500000);
>                      s->cmos_data[RTC_REG_A] &= ~REG_A_UIP;
> +                    check_update_timer(s);
>                      divider_reset = 0;
>                  }
>              }
>              /* UIP bit is read only */
>              s->cmos_data[RTC_REG_A] = (data & ~REG_A_UIP) |
>                  (s->cmos_data[RTC_REG_A] & REG_A_UIP);
> -            rtc_timer_update(s, qemu_get_clock_ns(rtc_clock));
> +            periodic_timer_update(s, qemu_get_clock_ns(rtc_clock));
>              break;
>          case RTC_REG_B:
>              if (data & REG_B_SET) {
> @@ -315,7 +374,8 @@ static void cmos_ioport_write(void *opaque, uint32_t 
> addr, uint32_t data)
>                  }
>              }
>              s->cmos_data[RTC_REG_B] = data;
> -            rtc_timer_update(s, qemu_get_clock_ns(rtc_clock));
> +            periodic_timer_update(s, qemu_get_clock_ns(rtc_clock));
> +            check_update_timer(s);
>              break;
>          case RTC_REG_C:
>          case RTC_REG_D:
> @@ -445,7 +505,7 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t 
> addr)
>              break;
>          case RTC_REG_A:
>              ret = s->cmos_data[s->cmos_index];
> -            if (update_in_progress(s)) {
> +            if ((s->use_timer == 0) && update_in_progress(s)) {
>                  ret |= REG_A_UIP;
>              }
>              break;
> @@ -453,6 +513,12 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t 
> addr)
>              ret = s->cmos_data[s->cmos_index];
>              qemu_irq_lower(s->irq);
>              s->cmos_data[RTC_REG_C] = 0x00;
> +            if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) {
> +                rtc_calibrate_time(s);
> +            }
> +            if (ret & (REG_C_UF | REG_C_AF)) {
> +                check_update_timer(s);
> +            }
>  #ifdef TARGET_I386
>              if(s->irq_coalesced &&
>                      (s->cmos_data[RTC_REG_B] & REG_B_PIE) &&
> @@ -545,6 +611,9 @@ static const VMStateDescription vmstate_rtc = {
>          VMSTATE_INT32(offset_usec, RTCState),
>          VMSTATE_TIMER(periodic_timer, RTCState),
>          VMSTATE_INT64(next_periodic_time, RTCState),
> +        VMSTATE_TIMER(update_timer, RTCState),
> +        VMSTATE_TIMER(update_timer2, RTCState),
> +        VMSTATE_UINT64(next_update_time, RTCState),
>          VMSTATE_UINT32_V(irq_coalesced, RTCState, 2),
>          VMSTATE_UINT32_V(period, RTCState, 2),
>          VMSTATE_END_OF_LIST()
> @@ -557,7 +626,8 @@ static void rtc_notify_clock_reset(Notifier *notifier, 
> void *data)
>      int64_t now = *(int64_t *)data;
> 
>      rtc_set_date_from_host(&s->dev);
> -    rtc_timer_update(s, now);
> +    periodic_timer_update(s, now);
> +    check_update_timer(s);
>  #ifdef TARGET_I386
>      if (s->lost_tick_policy == LOST_TICK_SLEW) {
>          rtc_coalesced_timer_update(s);
> @@ -579,6 +649,7 @@ static void rtc_reset(void *opaque)
> 
>      s->cmos_data[RTC_REG_B] &= ~(REG_B_PIE | REG_B_AIE | REG_B_SQWE);
>      s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF | REG_C_PF | 
> REG_C_AF);
> +    check_update_timer(s);
> 
>      qemu_irq_lower(s->irq);
> 
> @@ -649,6 +720,9 @@ static int rtc_initfn(ISADevice *dev)
>  #endif
> 
>      s->periodic_timer = qemu_new_timer_ns(rtc_clock, rtc_periodic_timer, s);
> +    s->update_timer = qemu_new_timer_ns(rtc_clock, rtc_update_timer, s);
> +    s->update_timer2 = qemu_new_timer_ns(rtc_clock, rtc_update_timer2, s);
> +    check_update_timer(s);
> 
>      s->clock_reset_notifier.notify = rtc_notify_clock_reset;
>      qemu_register_clock_reset_notifier(rtc_clock, &s->clock_reset_notifier);
> --
> 1.7.1
>