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

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

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

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 <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 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);
+                    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

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 <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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 <liqiang6-s@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  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 <peter.maydell@linaro.org>

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

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

[P J P]

+-- On Mon, 17 Oct 2016, Peter Maydell wrote --+
| > +                    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);
| ?

Yes, sent v7. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F