| Message ID | 1476416631-2870-1-git-send-email-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
+-- 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
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;