Re: [Qemu-devel] [PATCH v2] timer: a9gtimer: check auto-increment register value

[Peter Maydell <peter.maydell@linaro.org>]

On 1 October 2016 at 23:15, P J P <ppandit@redhat.com> 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