Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model

[Peter Crosthwaite]

Heres the diff for proposed comments:

@@ -129,6 +129,8 @@ static uint64_t
cadence_timer_get_steps(CadenceTimerState *s, uint64_t ns)
     return r;
 }

+/* determine if x is inbetween a and b, exclusive of a, inclusive of b */
+
 static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
 {
     if (a < b) {
@@ -188,12 +190,18 @@ static void cadence_timer_sync(CadenceTimerState *s)

     r = (int64_t)cadence_timer_get_steps(s, s->cpu_time - old_time);
     x = (int64_t)s->reg_value + ((s->reg_count & COUNTER_CTRL_DEC) ? -r : r);
+    if (x > 2 * interval || x < -interval) {
+        hw_error("Timer wrapped around twice in one I/O event "
+                "interval (wrap interval set too low / frequency too high)\n");
+    }

     for (i = 0; i < 3; ++i) {
         int64_t m = (int64_t)s->reg_match[i] << 16;
         if (m > interval) {
             continue;
         }
+        /* check to see if match event has occured. check m +/- interval
+         * to account for match events in wrap around cases */
         if (is_between(m, s->reg_value, x) ||
             is_between(m + interval, s->reg_value, x) ||
             is_between(m - interval, s->reg_value, x)) {


On Tue, Feb 28, 2012 at 11:04 AM, Peter Crosthwaite
<address@hidden> wrote:
> On Tue, Feb 28, 2012 at 1:45 AM, Paul Brook <address@hidden> wrote:
>>> On Tue, Feb 21, 2012 at 11:04 PM, Paul Brook <address@hidden> wrote:
>>> >> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
>>> >> > +{
>>> >> > +    if (a < b) {
>>> >> > +        return x > a && x <= b;
>>> >> > +    }
>>> >> > +    return x < a && x >= b;
>>> >> > +}
>>> >>
>>> >> This looks slightly odd -- should the boundary condition for whether
>>> >> a value equal to the max/min really change depending on :whether a
>>> >> or b is greater?
>>>
>>> The function determines whether x is in-between a and b exclusive of
>>> a, inclusive of b, so it is consistent with itself in that regard.
>>>
>>> > This is a ugly hack.  Instead of figuring out whether we have a count-up
>>> > or count-down timer the code checks for both, and have the "in_between"
>>> > function magically DTRT.  I haven't followed the paths through in enough
>>> > detail to figure out whether it gets all the corner cases right.
>>>
>>> Is it really a "hack"?? For count up b will always be greater than a,
>>> and for count down the reverse. I suppose I could assert these
>>> conditions at the call site for peace of mind? The invocation from
>>> cadence_timer_run doesn't care whether it is count up of count down,
>>> it really does just only care if the match value is in-between the
>>> current timer value and the next timer value, which is exactly what
>>> this function determines.
>>
>> When you explain it like this, it makes a more sense.  But this isn't
>> immediately obvious from the code.  It took me at least a couple of readings
>> to figure out what was going on. This is exactly the sort of thing that 
>> should
>> be described in comments.
>
> Ok, ill be a little more descriptive :)
>
> A function with a very generic name
>
> Perhaps clarify the whole inclusive a exclusive b in comment?
>
> is used in a
>> way that has fairly subtle implications.  There's a good chance someone[1]
>> will come along in a few months/years, reuse this function and "fix" the
>> wierdness at the same time.
>>
>> Annother non-obvious detail is the way you handle overflow.  Specifically you
>> check a range both plus and minus the wrap value before wrapping the final
>> count.  This is certainly confusing/surprising when you first encounter it.
>> Very large steps result in overlapping ranges, which triggers [in this case
>> harmless] warning bells.
>>
>> Thinking about that, I realised why I don't like the following line:
>>
>>> +    s->reg_value = (uint32_t)((x + interval) % interval);
>>
>> This assumes x > -interval, which is not always true.
>
> This would mean you have wrapped twice or more in one time step, which
> I am assuming is a fatal error condition, as It means your software
> has missed interrupts and all sort of race conditions would occur. I
> would personally prefer to assert !(x < -interval) and have qemu
> hw_error or something, as in these cases QEMU can just not handle your
> super quick timer wrap around.
>
>>
>> Paul
>>
>> [1] "someone" includes me.  After I've forgotten this obscure detail.