Re: [Qemu-devel] [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed

[Paolo Bonzini]

On 12/04/2017 11:51, address@hidden wrote:
> From: Xiao Guangrong <address@hidden>
> 
> Currently, the timer is updated whenever RegA or RegB is written
> even if the periodic timer related configuration is not changed
> 
> This patch optimizes it slightly to make the update happen only
> if its period or enable-status is changed, also later patches are
> depend on this optimization
> 
> Signed-off-by: Xiao Guangrong <address@hidden>
> ---
>  hw/timer/mc146818rtc.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 4165450..749e206 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -387,10 +387,25 @@ static void rtc_update_timer(void *opaque)
>      check_update_timer(s);
>  }
>  
> +static bool rtc_periodic_timer_updated_by_regA(RTCState *s, uint64_t data)
> +{
> +    uint8_t orig = s->cmos_data[RTC_REG_A];
> +
> +    return (orig & 0x0f) != (data & 0x0f);
> +}
> +
> +static bool rtc_periodic_timer_updated_by_regB(RTCState *s, uint64_t data)
> +{
> +    uint8_t orig = s->cmos_data[RTC_REG_B];
> +
> +    return (orig & REG_B_PIE) != (data & REG_B_PIE);
> +}
> +
>  static void cmos_ioport_write(void *opaque, hwaddr addr,
>                                uint64_t data, unsigned size)
>  {
>      RTCState *s = opaque;
> +    bool update_periodic_timer;
>  
>      if ((addr & 1) == 0) {
>          s->cmos_index = data & 0x7f;
> @@ -423,6 +438,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>              }
>              break;
>          case RTC_REG_A:
> +            update_periodic_timer = rtc_periodic_timer_updated_by_regA(s, 
> data);

I think you can change it to a...

>              if ((data & 0x60) == 0x60) {
>                  if (rtc_running(s)) {
>                      rtc_update_time(s);
> @@ -445,10 +462,16 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>              /* UIP bit is read only */

... inlined update_periodic_timer assignment here:

    update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_A_PERIOD;

>              s->cmos_data[RTC_REG_A] = (data & ~REG_A_UIP) |
>                  (s->cmos_data[RTC_REG_A] & REG_A_UIP);
> -            periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> +
> +            if (update_periodic_timer) {
> +                periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> +            }
> +
>              check_update_timer(s);
>              break;
>          case RTC_REG_B:
> +            update_periodic_timer = rtc_periodic_timer_updated_by_regB(s, 
> data);
> +
>              if (data & REG_B_SET) {
>                  /* update cmos to when the rtc was stopping */
>                  if (rtc_running(s)) {
> @@ -475,7 +498,11 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>                  qemu_irq_lower(s->irq);
>              }

Same here:

    update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_B_PIE;

Thanks,

Paolo

>              s->cmos_data[RTC_REG_B] = data;
> -            periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> +
> +            if (update_periodic_timer) {
> +                periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> +            }
> +
>              check_update_timer(s);
>              break;
>          case RTC_REG_C:
>