On 14 October 2016 at 04:43, P J P <[email protected]> wrote: > From: Prasad J Pandit <[email protected]> > > 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 <[email protected]> > Signed-off-by: Prasad J Pandit <[email protected]> > --- > 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 <[email protected]> > + 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
