Re: [Qemu-arm] [PATCH v8 1/4] hw/ptimer: Fix issues caused by the adjusted timer limit value

[Peter Crosthwaite]

On Wed, Jan 6, 2016 at 5:25 AM, Dmitry Osipenko <address@hidden> wrote:
> 06.01.2016 15:15, Peter Crosthwaite пишет:
>>>
>>> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
>>> index edf077c..035af97 100644
>>> --- a/hw/core/ptimer.c
>>> +++ b/hw/core/ptimer.c
>>> @@ -34,20 +34,39 @@ static void ptimer_trigger(ptimer_state *s)
>>>
>>>   static void ptimer_reload(ptimer_state *s)
>>>   {
>>> -    if (s->delta == 0) {
>>> +    uint32_t period_frac = s->period_frac;
>>> +    uint64_t period = s->period;
>>> +    uint64_t delta = s->delta;
>>> +    uint64_t limit = s->limit;
>>> +
>>
>>
>> Localising these variables is out of scope of the change. I think you can
>> just use s->foo and if you want to cleanup, it should be a separate
>> patch.
>>
>
> Okay
>
>>> +    if (delta == 0) {
>>>           ptimer_trigger(s);
>>> -        s->delta = s->limit;
>>> +        delta = limit;
>>>       }
>>> -    if (s->delta == 0 || s->period == 0) {
>>> +    if (delta == 0 || period == 0) {
>>>           fprintf(stderr, "Timer with period zero, disabling\n");
>>>           s->enabled = 0;
>>>           return;
>>>       }
>>>
>>> +    /*
>>> +     * Artificially limit timeout rate to something
>>> +     * achievable under QEMU.  Otherwise, QEMU spends all
>>> +     * its time generating timer interrupts, and there
>>> +     * is no forward progress.
>>> +     * About ten microseconds is the fastest that really works
>>> +     * on the current generation of host machines.
>>> +     */
>>> +
>>> +    if ((s->enabled == 1) && (limit * period < 10000)) {
>>> +        period = 10000 / limit;
>>> +        period_frac = 0;
>>> +    }
>>> +
>>
>>
>> I think it should be ok to just update s->period and s->period_frac ...
>>
>
> No, then it would be irreversibly lost. What if'd decide to change the limit
> to some large value?
>

Ok makes sense.

>
>>>       s->last_event = s->next_event;
>>> -    s->next_event = s->last_event + s->delta * s->period;
>>> -    if (s->period_frac) {
>>> -        s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
>>> +    s->next_event = s->last_event + delta * period;
>>> +    if (period_frac) {
>>> +        s->next_event += ((int64_t)period_frac * delta) >> 32;
>>>       }
>>>       timer_mod(s->timer, s->next_event);
>>>   }
>>> @@ -82,6 +101,8 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>>               uint64_t div;
>>>               int clz1, clz2;
>>>               int shift;
>>> +            uint32_t period_frac = s->period_frac;
>>> +            uint64_t period = s->period;
>>>
>>>               /* We need to divide time by period, where time is stored
>>> in
>>>                  rem (64-bit integer) and period is stored in
>>> period/period_frac
>>> @@ -93,8 +114,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>>                  backwards.
>>>               */
>>>
>>> +            if ((s->enabled == 1) && (s->limit * period < 10000)) {
>>> +                period = 10000 / s->limit;
>>> +                period_frac = 0;
>>> +            }
>>> +

As this now needs to be kept, another note I had is it should probably
go before the block comment as the comment relates specifically to the
math below.

Regards,
Peter

>>
>>
>> ... and then this (and the local variables) become obsolete. Can
>> get_count()
>> blindly use the period and period_frac as used by ptimer_reload?
>>
>>>               rem = s->next_event - now;
>>> -            div = s->period;
>>> +            div = period;
>>>
>>>               clz1 = clz64(rem);
>>>               clz2 = clz64(div);
>>> @@ -103,13 +129,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>>               rem <<= shift;
>>>               div <<= shift;
>>>               if (shift >= 32) {
>>> -                div |= ((uint64_t)s->period_frac << (shift - 32));
>>> +                div |= ((uint64_t)period_frac << (shift - 32));
>>>               } else {
>>>                   if (shift != 0)
>>> -                    div |= (s->period_frac >> (32 - shift));
>>> +                    div |= (period_frac >> (32 - shift));
>>>                   /* Look at remaining bits of period_frac and round div
>>> up if
>>>                      necessary.  */
>>> -                if ((uint32_t)(s->period_frac << shift))
>>> +                if ((uint32_t)(period_frac << shift))
>>>                       div += 1;
>>>               }
>>>               counter = rem / div;
>>> @@ -181,19 +207,6 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>>>      count = limit.  */
>>>   void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>>>   {
>>> -    /*
>>> -     * Artificially limit timeout rate to something
>>> -     * achievable under QEMU.  Otherwise, QEMU spends all
>>> -     * its time generating timer interrupts, and there
>>> -     * is no forward progress.
>>> -     * About ten microseconds is the fastest that really works
>>> -     * on the current generation of host machines.
>>> -     */
>>> -
>>> -    if (!use_icount && limit * s->period < 10000 && s->period) {
>>
>>
>> This original rate limiting code is gated on icount, so I think then
>> new way should be the same.
>>
>
> Shoot :) That's second time I'm missing it. Good catch!
>
>
>> Regards,
>> Peter
>>
>>> -        limit = 10000 / s->period;
>>> -    }
>>> -
>>>       s->limit = limit;
>>>       if (reload)
>>>           s->delta = limit;
>>> --
>>> 2.6.4
>>>
>
>
> --
> Dmitry