From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52693) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwBvO-00036u-1S for qemu-devel@nongnu.org; Mon, 17 Oct 2016 13:42:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwBvM-0008Vd-RV for qemu-devel@nongnu.org; Mon, 17 Oct 2016 13:42:10 -0400 Received: from mail-vk0-x232.google.com ([2607:f8b0:400c:c05::232]:34571) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1bwBvM-0008VL-Ft for qemu-devel@nongnu.org; Mon, 17 Oct 2016 13:42:08 -0400 Received: by mail-vk0-x232.google.com with SMTP id b186so186026501vkb.1 for ; Mon, 17 Oct 2016 10:42:08 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1476416631-2870-1-git-send-email-ppandit@redhat.com> References: <1476416631-2870-1-git-send-email-ppandit@redhat.com> From: Peter Maydell Date: Mon, 17 Oct 2016 18:41:47 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v6] timer: a9gtimer: remove loop to auto-increment comparator List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P Cc: Qemu Developers , Li Qiang , qemu-arm , Prasad J Pandit On 14 October 2016 at 04:43, P J P wrote: > From: Prasad J Pandit > > ARM A9MP processor has a peripheral timer with an auto-increment > register, which holds an increment step value. A user could set > this value to zero. When auto-increment control bit is enabled, > it leads to an infinite loop in 'a9_gtimer_update' while > updating comparator value. Remove this loop incrementing the > comparator value. > > Reported-by: Li Qiang > Signed-off-by: Prasad J Pandit > --- > hw/timer/a9gtimer.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > Update per > -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg02891.html > > diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c > index 772f85f..03dfaf2 100644 > --- a/hw/timer/a9gtimer.c > +++ b/hw/timer/a9gtimer.c > @@ -73,6 +73,7 @@ static void a9_gtimer_update(A9GTimerState *s, bool sync) > > A9GTimerUpdate update = a9_gtimer_get_update(s); > int i; > + uint64_t inc; > int64_t next_cdiff = 0; > > for (i = 0; i < s->num_cpu; ++i) { > @@ -82,15 +83,15 @@ static void a9_gtimer_update(A9GTimerState *s, bool sync) > if ((s->control & R_CONTROL_TIMER_ENABLE) && > (gtb->control & R_CONTROL_COMP_ENABLE)) { > /* R2p0+, where the compare function is >= */ > - while (gtb->compare < update.new) { > + if (gtb->compare < update.new) { > DB_PRINT("Compare event happened for CPU %d\n", i); > gtb->status = 1; > - if (gtb->control & R_CONTROL_AUTO_INCREMENT) { > - DB_PRINT("Auto incrementing timer compare by %" PRId32 "\n", > - gtb->inc); > - gtb->compare += gtb->inc; > - } else { > - break; > + if (gtb->control & R_CONTROL_AUTO_INCREMENT && gtb->inc) { > + inc = update.new + gtb->inc - gtb->compare - 1; > + inc = QEMU_ALIGN_DOWN(inc, gtb->inc); Isn't this pair of lines equivalent to uint64_t inc = QEMU_ALIGN_UP(update.new - gtb->compare, gtb->inc); ? I think using that macro makes it clearer that the code is correct. If you replace the above two lines (and the unnecessarily widely scoped declaration of inc) with that line then you can have Reviewed-by: Peter Maydell > + DB_PRINT("Auto incrementing timer compare by %" > + PRId64 "\n", inc); > + gtb->compare += inc; > } > } > cdiff = (int64_t)gtb->compare - (int64_t)update.new + 1; > -- > 2.5.5 thanks -- PMM