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
