Re: [Qemu-devel] [PATCH v12 7/9] hw/ptimer: Fix counter - 1 returned by ptimer_get_count for the active timer

[Dmitry Osipenko <digetx@gmail.com>]

08.03.2016 06:43, Peter Crosthwaite пишет:
> On Tue, Feb 2, 2016 at 7:19 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> 30.01.2016 19:43, Dmitry Osipenko пишет:
>>>
>>> Due to rounding down performed by ptimer_get_count, it returns counter - 1
>>> for
>>> the active timer. That's incorrect because counter should decrement only
>>> after
>>> period been expired, not before. I.e. if running timer has been loaded
>>> with
>>> value X, then timer counter should stay with X until period expired and
>>> decrement after. Fix this by adding 1 to the counter value for the active
>>> and
>>> unexpired timer.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>    hw/core/ptimer.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
>>> index 62f8cb1..b2044fb 100644
>>> --- a/hw/core/ptimer.c
>>> +++ b/hw/core/ptimer.c
>>> @@ -136,7 +136,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>>                    if ((uint32_t)(period_frac << shift))
>>>                        div += 1;
>>>                }
>>> -            counter = rem / div;
>>> +            counter = rem / div + (expired ? 0 : 1);
>>>
>>>                if (expired && counter != 0) {
>>>                    /* Wrap around periodic counter.  */
>>>
>>
>> Noticed one nit here:
>>
>> There is possibility to return timer counter = limit + 1, if the following
>> ptimer calls execute in less than 1ns.
>>
>>          ptimer_run(t, 1);
>>          // counter = 91, if set() count executed in less than 1ns
>>          ptimer_set_count(t, 90);
>>          counter = ptimer_get_count(t);
>>
>> Likely, it would be impossible to trigger that issue on a real current
>> machine.
>> But the fix is trivial, I'll incorporate it in V13 if it looks fine:
>>
>> ---
>> @@ -76,20 +76,20 @@ static void ptimer_tick(void *opaque)
>>   {
>>       ptimer_state *s = (ptimer_state *)opaque;
>>       s->delta = 0;
>>       ptimer_reload(s);
>>   }
>>
>>   uint64_t ptimer_get_count(ptimer_state *s)
>>   {
>> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>       uint64_t counter;
>>
>> -    if (s->enabled && s->delta != 0) {
>> -        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +    if (s->enabled && s->delta != 0 && now != s->last_event) {
>>           int64_t next = s->next_event;
>>           bool expired = (now - next >= 0);
>>           bool oneshot = (s->enabled == 2);
>>
>>           /* Figure out the current counter value.  */
>>           if (expired && (oneshot || use_icount)) {
>>               /* Prevent timer underflowing if it should already have
>>                  triggered.  */
>> @@ -131,17 +131,17 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>               } else {
>>                   if (shift != 0)
>>                       div |= (period_frac >> (32 - shift));
>>                   /* Look at remaining bits of period_frac and round div up
>> if
>>                      necessary.  */
>>                   if ((uint32_t)(period_frac << shift))
>>                       div += 1;
>>               }
>> -            counter = rem / div;
>> +            counter = rem / div + (expired ? 0 : 1);
>>
>
> Sorry about the long delays. Im wondering what this has to do with
> expiration though? the commit message suggests you want to change the
> rounding scheme, so can that be done directly with DIV_ROUND_UP?
>
> Regards,
> Peter
>
>>               if (expired && counter != 0) {
>>                   /* Wrap around periodic counter.  */
>>                   counter = s->limit - (counter - 1) % s->limit;
>>               }
>>           }
>>       } else {
>>           counter = s->delta;
>>
>>
>> --
>> Dmitry

Hi Peter,

The expiration check is needed because timer should stay with 0 for a one period 
before doing wrap around. Using DIV_ROUND_UP directly, timer would wrap around 
immediately once now > next (i.e. after 1 ns) regardless of the period value, 
and DIV_ROUND_UP can't be used because it would cause integer overflow of (n) + 
(d) due to the left shifting of the "rem" and "div".

#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))


No worries, I can wait as much as needed :)

-- 
Dmitry