Re: [Qemu-devel] [PATCH v6] timer: a9gtimer: remove loop to auto-increment comparator

[Peter Maydell]

On 14 October 2016 at 04:43, P J P <address@hidden> wrote:
> From: Prasad J Pandit <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  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 <address@hidden>

> +                    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