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

[P J P]

  Hello Peter,

+-- On Fri, 30 Sep 2016, Peter Maydell wrote --+
| > -    int i;
| > +    uint32_t i, inc;
| 
| Why have you changed the type of i when you haven't changed
| any of the code that works with it?

  Actually I declared 'inc' on the same line, then changed type to 'uint32' as 
'gtb->inc' is of the same type.
 
| 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.

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

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