From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed Date: Wed, 3 May 2017 17:42:01 +0200 Message-ID: <65524a48-e39a-1db4-b173-2ba488a762f8@redhat.com> References: <20170412095111.11728-1-xiaoguangrong@tencent.com> <20170412095111.11728-2-xiaoguangrong@tencent.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, yunfangtai@tencent.com, Xiao Guangrong To: guangrong.xiao@gmail.com, mst@redhat.com, mtosatti@redhat.com Return-path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:36322 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751509AbdECPmG (ORCPT ); Wed, 3 May 2017 11:42:06 -0400 Received: by mail-wm0-f66.google.com with SMTP id u65so13693172wmu.3 for ; Wed, 03 May 2017 08:42:06 -0700 (PDT) In-Reply-To: <20170412095111.11728-2-xiaoguangrong@tencent.com> Sender: kvm-owner@vger.kernel.org List-ID: On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote: > From: Xiao Guangrong > > 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 > --- > 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: > From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d5wPr-00016k-FW for qemu-devel@nongnu.org; Wed, 03 May 2017 11:42:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d5wPm-0000ZN-Jv for qemu-devel@nongnu.org; Wed, 03 May 2017 11:42:11 -0400 Received: from mail-wm0-x243.google.com ([2a00:1450:400c:c09::243]:34383) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1d5wPm-0000Yz-Cz for qemu-devel@nongnu.org; Wed, 03 May 2017 11:42:06 -0400 Received: by mail-wm0-x243.google.com with SMTP id z129so13787270wmb.1 for ; Wed, 03 May 2017 08:42:06 -0700 (PDT) Sender: Paolo Bonzini References: <20170412095111.11728-1-xiaoguangrong@tencent.com> <20170412095111.11728-2-xiaoguangrong@tencent.com> From: Paolo Bonzini Message-ID: <65524a48-e39a-1db4-b173-2ba488a762f8@redhat.com> Date: Wed, 3 May 2017 17:42:01 +0200 MIME-Version: 1.0 In-Reply-To: <20170412095111.11728-2-xiaoguangrong@tencent.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: guangrong.xiao@gmail.com, mst@redhat.com, mtosatti@redhat.com Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, yunfangtai@tencent.com, Xiao Guangrong On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote: > From: Xiao Guangrong > > 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 > --- > 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: >