From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51287) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bqiEa-0002zO-1A for qemu-devel@nongnu.org; Sun, 02 Oct 2016 10:59:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bqiEX-0002zE-Rt for qemu-devel@nongnu.org; Sun, 02 Oct 2016 10:59:19 -0400 Received: from mail-vk0-x22a.google.com ([2607:f8b0:400c:c05::22a]:35508) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bqiEX-0002yg-EK for qemu-devel@nongnu.org; Sun, 02 Oct 2016 10:59:17 -0400 Received: by mail-vk0-x22a.google.com with SMTP id 192so141454743vkl.2 for ; Sun, 02 Oct 2016 07:59:16 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1475054208-20288-1-git-send-email-ppandit@redhat.com> From: Peter Maydell Date: Sun, 2 Oct 2016 07:58:55 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v2] timer: a9gtimer: check auto-increment register value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P Cc: Qemu Developers , Li Qiang , qemu-arm On 1 October 2016 at 23:15, P J P wrote: > Hello Peter, > > +-- On Fri, 30 Sep 2016, Peter Maydell wrote --+ > | Since the gtb->compare field is 64 bit your inc variable should > | probably be uint64_t. > > Okay. I was wondering about its values, as gtb->inc is 'uint32_t' type. The timer can't autoincrement by more than a 32 bit value, but the timer value is 64 bits, and since this code may be doing more than one autoincrement it's better to use 64 bits. > | > + DB_PRINT("Compare event happened for CPU %d\n", i); > | > | This is wrong -- this debug print should only happen when the > | comparator matches (ie when gtb->compare is less than update.new), > | not every time. > | > | > + gtb->status = 1; > | > | Similarly the status flag should only be set when the comparator > | matches. > | > | > + if (gtb->control & R_CONTROL_AUTO_INCREMENT > | > + && gtb->compare < update.new) { > | > + inc = ((update.new - gtb->compare - 1) / gtb->inc) * gtb->inc; > | > | I suggest using either QEMU_ALIGN_UP or QEMU_ALIGN_DOWN here, > | depending on which way round you're trying for. > > Okay. > > | Also this will divide by zero if gtb->inc is 0 (as for instance > | it will be on reset). > | > | > + gtb->inc = value ? value : 1; > | > | Where has this change come from ?? > > This was to avoid the possible divide by zero. Couldn't find if zero could > be valid value for 'gtb->inc' and a non-zero default value if not. If in doubt, you need to check the datasheet/technical reference manual for the hardware, in this case http://infocenter.arm.com/help/topic/com.arm.doc.100486_0401_10_en/ada1443777319205.html Zero is a valid value for the auto-increment register, and in fact it is the documented reset value. > If > 'gtb->inc' could be zero, should the 'gtb->compare += inc' > only happen when > it's non-zero ? Ie gtb->status would be set if gtb->inc > 0. If gtb->inc is zero, then when the timer fires the compare register should increment by the increment value (which will be zero, so it won't change). This is not a particularly useful setting for the guest to put the timer into, but the behaviour is well-defined. (Effectively, setting the increment to 0 is equivalent to disabling auto-increment.) gtb->status should be set whenever the timer comparator fires, whether we are auto-incrementing or not, and whatever the value of the auto-increment register is. It is the "timer matched" interrupt output. (Your current patch is wrong in that it has moved setting gtb->status into the wrong place, as I mentioned above.) thanks -- PMM