[Qemu-devel] [PATCH v7 0/2] PTimer fix and ARM MPTimer conversion

[Qemu-devel] [PATCH v7 0/2] PTimer fix and ARM MPTimer conversion

[Dmitry Osipenko]

Changelog for ARM MPTimer QEMUTimer to ptimer conversion:

    V2: Fixed changing periodic timer counter value "on the fly". I added a
        test to the gist to cover that issue.

    V3: Fixed starting the timer with load = 0 and counter != 0, added tests
        to the gist for this issue. Changed vmstate version for all VMSD's,
        since loadvm doesn't check version of nested VMSD.

    V4: Fixed spurious IT bit set for the timer starting in the periodic mode
        with counter = 0. Test added.

    V5: Code cleanup, now depends on ptimer_set_limit() fix.

    V6: No code change, added test to check ptimer_get_count() with corrected
        .limit value.

    V7: No change.

ARM MPTimer tests: https://gist.github.com/digetx/dbd46109503b1a91941a


Patch for ptimer is introduced since V5 of ARM MPTimer conversion.

Changelog for ptimer patch:

    V5: Only fixed ptimer_set_limit() for the disabled timer.

    V6: As was pointed by Peter Maydell, there are other issues beyond
        ptimer_set_limit(), so V6 supposed to cover all those issues.

    V7: Added accidentally removed !use_icount check.
        Added missed "else" statement, thanks to checkpatch :)

Dmitry Osipenko (2):
  hw/ptimer: Fix issues caused by artificially limited timer timeout
  arm_mptimer: Convert to use ptimer

 hw/core/ptimer.c               |  37 ++++++++------
 hw/timer/arm_mptimer.c         | 110 ++++++++++++++++++-----------------------
 include/hw/timer/arm_mptimer.h |   4 +-
 3 files changed, 72 insertions(+), 79 deletions(-)

-- 
2.6.1

[Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Dmitry Osipenko]

Multiple issues here related to the timer with a corrected .limit value:

1) ptimer_get_count() returns incorrect value for the disabled timer after
loading the counter with a small value, because corrected limit value
is used instead of the original.

For instance:
    1) ptimer_stop(t)
    2) ptimer_set_period(t, 1)
    3) ptimer_set_limit(t, 0, 1)
    4) ptimer_get_count(t) <-- would return 10000 instead of 0

2) ptimer_get_count() might return incorrect value for the timer running
with a corrected limit value.

For instance:
    1) ptimer_stop(t)
    2) ptimer_set_period(t, 1)
    3) ptimer_set_limit(t, 10, 1)
    4) ptimer_run(t)
    5) ptimer_get_count(t) <-- might return value > 10

3) Neither ptimer_set_period() nor ptimer_set_freq() are correcting the
limit value, so it is still possible to make timer timeout value
arbitrary small.

For instance:
    1) ptimer_set_period(t, 10000)
    2) ptimer_set_limit(t, 1, 0)
    3) ptimer_set_period(t, 1) <-- bypass limit correction

Fix all of the above issues by moving timeout value correction to the
ptimer_reload(). Instead of changing limit value, set new ptimer struct
member "next_event_corrected" when next_event was corrected and make
ptimer_get_count() always return 1 for the timer running with corrected
next_event value. Bump VMSD version since ptimer VM state is changed, but
keep .minimum_version_id as it is backward compatible.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 hw/core/ptimer.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index 8437bd6..2441943 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -19,6 +19,7 @@ struct ptimer_state
     int64_t period;
     int64_t last_event;
     int64_t next_event;
+    uint8_t next_event_corrected;
     QEMUBH *bh;
     QEMUTimer *timer;
 };
@@ -48,6 +49,23 @@ static void ptimer_reload(ptimer_state *s)
     if (s->period_frac) {
         s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
     }
+
+    /*
+     * Artificially limit timeout 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.
+     */
+
+    s->next_event_corrected = 0;
+
+    if (!use_icount && (s->next_event - s->last_event < 10000)) {
+        s->next_event = s->last_event + 10000;
+        s->next_event_corrected = 1;
+    }
+
     timer_mod(s->timer, s->next_event);
 }
 
@@ -76,6 +94,9 @@ uint64_t ptimer_get_count(ptimer_state *s)
             /* Prevent timer underflowing if it should already have
                triggered.  */
             counter = 0;
+        } else if (s->next_event_corrected) {
+            /* Always return 1 when timer expire value was corrected.  */
+            counter = 1;
         } else {
             uint64_t rem;
             uint64_t div;
@@ -180,19 +201,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) {
-        limit = 10000 / s->period;
-    }
-
     s->limit = limit;
     if (reload)
         s->delta = limit;
@@ -204,7 +212,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
 
 const VMStateDescription vmstate_ptimer = {
     .name = "ptimer",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(enabled, ptimer_state),
@@ -214,6 +222,7 @@ const VMStateDescription vmstate_ptimer = {
         VMSTATE_INT64(period, ptimer_state),
         VMSTATE_INT64(last_event, ptimer_state),
         VMSTATE_INT64(next_event, ptimer_state),
+        VMSTATE_UINT8(next_event_corrected, ptimer_state),
         VMSTATE_TIMER_PTR(timer, ptimer_state),
         VMSTATE_END_OF_LIST()
     }
-- 
2.6.1

Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Peter Crosthwaite]

On Sat, Oct 24, 2015 at 5:21 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> Multiple issues here related to the timer with a corrected .limit value:
>
> 1) ptimer_get_count() returns incorrect value for the disabled timer after
> loading the counter with a small value, because corrected limit value
> is used instead of the original.
>
> For instance:
>     1) ptimer_stop(t)
>     2) ptimer_set_period(t, 1)
>     3) ptimer_set_limit(t, 0, 1)
>     4) ptimer_get_count(t) <-- would return 10000 instead of 0
>
> 2) ptimer_get_count() might return incorrect value for the timer running
> with a corrected limit value.
>
> For instance:
>     1) ptimer_stop(t)
>     2) ptimer_set_period(t, 1)
>     3) ptimer_set_limit(t, 10, 1)
>     4) ptimer_run(t)
>     5) ptimer_get_count(t) <-- might return value > 10
>
> 3) Neither ptimer_set_period() nor ptimer_set_freq() are correcting the
> limit value, so it is still possible to make timer timeout value
> arbitrary small.
>
> For instance:
>     1) ptimer_set_period(t, 10000)
>     2) ptimer_set_limit(t, 1, 0)
>     3) ptimer_set_period(t, 1) <-- bypass limit correction
>
> Fix all of the above issues by moving timeout value correction to the
> ptimer_reload(). Instead of changing limit value, set new ptimer struct
> member "next_event_corrected" when next_event was corrected and make
> ptimer_get_count() always return 1 for the timer running with corrected
> next_event value. Bump VMSD version since ptimer VM state is changed, but
> keep .minimum_version_id as it is backward compatible.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/core/ptimer.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 8437bd6..2441943 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -19,6 +19,7 @@ struct ptimer_state
>      int64_t period;
>      int64_t last_event;
>      int64_t next_event;
> +    uint8_t next_event_corrected;
>      QEMUBH *bh;
>      QEMUTimer *timer;
>  };
> @@ -48,6 +49,23 @@ static void ptimer_reload(ptimer_state *s)
>      if (s->period_frac) {
>          s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
>      }
> +
> +    /*
> +     * Artificially limit timeout 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.
> +     */
> +
> +    s->next_event_corrected = 0;
> +
> +    if (!use_icount && (s->next_event - s->last_event < 10000)) {
> +        s->next_event = s->last_event + 10000;
> +        s->next_event_corrected = 1;
> +    }
> +
>      timer_mod(s->timer, s->next_event);
>  }
>
> @@ -76,6 +94,9 @@ uint64_t ptimer_get_count(ptimer_state *s)
>              /* Prevent timer underflowing if it should already have
>                 triggered.  */
>              counter = 0;
> +        } else if (s->next_event_corrected) {
> +            /* Always return 1 when timer expire value was corrected.  */
> +            counter = 1;

This looks like a give-up without trying to get the correct value. If
the calculated value (using the normal-path logic below) is sane, you
should just use it. If it comes out bad then you should clamp to 1.

I am wondering whether this clamping policy (as in original code as
well) is correct at all though. The value of a free-running
short-interval periodic timer (poor mans random number generator)
without any actual interrupt generation will be affected by QEMUs
asynchronous handling of timer events. So if I set limit to 100, then
sample this timer every user keyboard stroke, I should get a uniform
distribution on [0,100]. Instead in am going to get lots of 1s. This
is more broken in the original code (as you state), as I will get >
100, but I think we have traded broken for slightly less broken. I
think the correct semantic is to completely ignoring rate limiting
except for the scheduling on event callbacks. That is, the timer
interval is not rate limited, instead the timer_mod interval
(next_event -last_event) just has a 10us clamp.

The means the original code semantic of returning counter = 0 for an
already triggered timer is wrong. It should handle in-the-past
wrap-arounds as wraparounds by triggering the timer and redoing the
math with the new interval values. So instead the logic would be
something like:

timer_val = -1;

for(;;) {

 if (!enabled) {
    return delta;
 }

 timer_val = (next-event - now) *  scaling();
 if (timer_val >=  0) {
     return timer_val;
 }
 /* Timer has actually expired but we missed it, reload it and try again */
 ptimer_tick();
}

ptimer_reload() then needs to be patched to make sure it always
timer_mod()s in the future, otherwise this loop could iterate a large
number of times.

This means that when the qemu_timer() actually ticks, a large number
or cycles may have occured, but we can justify that in that callback
event latency (usually interrupts) is undefined anyway and coalescing
of multiples may have happened as part of that. This usually matches
expectations of real guests where interrupt latency is ultimately
undefined.

Regards,
Peter

>          } else {
>              uint64_t rem;
>              uint64_t div;
> @@ -180,19 +201,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) {
> -        limit = 10000 / s->period;
> -    }
> -
>      s->limit = limit;
>      if (reload)
>          s->delta = limit;
> @@ -204,7 +212,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>
>  const VMStateDescription vmstate_ptimer = {
>      .name = "ptimer",
> -    .version_id = 1,
> +    .version_id = 2,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(enabled, ptimer_state),
> @@ -214,6 +222,7 @@ const VMStateDescription vmstate_ptimer = {
>          VMSTATE_INT64(period, ptimer_state),
>          VMSTATE_INT64(last_event, ptimer_state),
>          VMSTATE_INT64(next_event, ptimer_state),
> +        VMSTATE_UINT8(next_event_corrected, ptimer_state),
>          VMSTATE_TIMER_PTR(timer, ptimer_state),
>          VMSTATE_END_OF_LIST()
>      }
> --
> 2.6.1
>

Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Dmitry Osipenko]

24.10.2015 22:45, Peter Crosthwaite пишет:
>
> This looks like a give-up without trying to get the correct value. If
> the calculated value (using the normal-path logic below) is sane, you
> should just use it. If it comes out bad then you should clamp to 1.
>
> I am wondering whether this clamping policy (as in original code as
> well) is correct at all though. The value of a free-running
> short-interval periodic timer (poor mans random number generator)
> without any actual interrupt generation will be affected by QEMUs
> asynchronous handling of timer events. So if I set limit to 100, then
> sample this timer every user keyboard stroke, I should get a uniform
> distribution on [0,100]. Instead in am going to get lots of 1s. This

Right, that's a good example. What about to scale ptimer period to match 
adjusted timer_mod interval?

> is more broken in the original code (as you state), as I will get >
> 100, but I think we have traded broken for slightly less broken. I
> think the correct semantic is to completely ignoring rate limitin
> except for the scheduling on event callbacks. That is, the timer

I'm missing you here. What event callbacks?

> interval is not rate limited, instead the timer_mod interval
> (next_event -last_event) just has a 10us clamp.
>
> The means the original code semantic of returning counter = 0 for an
> already triggered timer is wrong. It should handle in-the-past
> wrap-arounds as wraparounds by triggering the timer and redoing the
> math with the new interval values. So instead the logic would be
> something like:
>
> timer_val = -1;
>
> for(;;) {
>
>   if (!enabled) {
>      return delta;
>   }
>
>   timer_val = (next-event - now) *  scaling();
>   if (timer_val >=  0) {
>       return timer_val;
>   }
>   /* Timer has actually expired but we missed it, reload it and try again */
>   ptimer_tick();
> }

Why do you think that ptimer_get_count() == 0 in case of the running periodic 
timer that was expired while QEMU was "on the way" to ptimer code is bad and 
wrong? From the guest point of view it's okay (no?), do we really need to 
overengineer that corner case?

>
> ptimer_reload() then needs to be patched to make sure it always
> timer_mod()s in the future, otherwise this loop could iterate a large
> number of times.
>
> This means that when the qemu_timer() actually ticks, a large number
> or cycles may have occured, but we can justify that in that callback
> event latency (usually interrupts) is undefined anyway and coalescing
> of multiples may have happened as part of that. This usually matches
> expectations of real guests where interrupt latency is ultimately
> undefined.

ptimer_tick() is re-arm'ing the qemu_timer() in case of periodic mode. Hope I 
haven't missed your point here.

>
> Regards,
> Peter
>

-- 
Dmitry

Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Peter Crosthwaite]

On Sat, Oct 24, 2015 at 3:22 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
> 24.10.2015 22:45, Peter Crosthwaite пишет:
>>
>>
>> This looks like a give-up without trying to get the correct value. If
>> the calculated value (using the normal-path logic below) is sane, you
>> should just use it. If it comes out bad then you should clamp to 1.
>>
>> I am wondering whether this clamping policy (as in original code as
>> well) is correct at all though. The value of a free-running
>> short-interval periodic timer (poor mans random number generator)
>> without any actual interrupt generation will be affected by QEMUs
>> asynchronous handling of timer events. So if I set limit to 100, then
>> sample this timer every user keyboard stroke, I should get a uniform
>> distribution on [0,100]. Instead in am going to get lots of 1s. This
>
>
> Right, that's a good example. What about to scale ptimer period to match
> adjusted timer_mod interval?
>

Thats just as incorrect as changing the limit IMO. The guest could get
confused with a timer running at the wrong frequency.

>> is more broken in the original code (as you state), as I will get >
>> 100, but I think we have traded broken for slightly less broken. I
>> think the correct semantic is to completely ignoring rate limitin
>> except for the scheduling on event callbacks. That is, the timer
>
>
> I'm missing you here. What event callbacks?
>

when timer_mod() hits, and it turn triggers some device specific event
(usually an interrupt).

There are two basic interactions for any QEMU timer. There are
synchronous events, i.e. the guest reading (polling) the counter which
is what this patch tries to fix. The second is the common case of
periodic interrupt generation. My proposal is that rate limiting does
not affect synchronous operation, only asynchronous (so my keystroke
RNG case works). In the current code, if ptimer_get_count() is called
when the event has passed it returns 0 under the assumption that the
timer_mod callback is about to happen. With rate-limiting that may be
well in the future.

>> interval is not rate limited, instead the timer_mod interval
>> (next_event -last_event) just has a 10us clamp.
>>
>> The means the original code semantic of returning counter = 0 for an
>> already triggered timer is wrong. It should handle in-the-past
>> wrap-arounds as wraparounds by triggering the timer and redoing the
>> math with the new interval values. So instead the logic would be
>> something like:
>>
>> timer_val = -1;
>>
>> for(;;) {
>>
>>   if (!enabled) {
>>      return delta;
>>   }
>>
>>   timer_val = (next-event - now) *  scaling();
>>   if (timer_val >=  0) {
>>       return timer_val;
>>   }
>>   /* Timer has actually expired but we missed it, reload it and try again
>> */
>>   ptimer_tick();
>> }
>
>
> Why do you think that ptimer_get_count() == 0 in case of the running
> periodic timer that was expired while QEMU was "on the way" to ptimer code
> is bad and wrong?

Because you may have gone past the time when it was actually zero and
reloaded and started counting again. It should return the real
(reloaded and resumed) value. This is made worse by rate limiting as
the timer will spend a long time at the clamp value waiting for the
rate-limited tick to fix it.

Following on from before, we don't want the async stuff to affect
sync. As the async callbacks are heavily affected by rate limiting we
don't want the polled timer having to rely on the callbacks (async
path) at all for correct operation. That's what the current
implementation of ptimer_get_count assumes with that 0-clamp.

> From the guest point of view it's okay (no?), do we really
> need to overengineer that corner case?
>

Depends on your use case. Your bug report is correct in that the timer
should never be outside the bounds of the limit. But you are fixing
that very specifically with a minimal change rather than correcting
the larger ptimer_get_count() logic which I think is more broken than
you suggest it is.

>>
>> ptimer_reload() then needs to be patched to make sure it always
>> timer_mod()s in the future, otherwise this loop could iterate a large
>> number of times.
>>
>> This means that when the qemu_timer() actually ticks, a large number
>> or cycles may have occured, but we can justify that in that callback
>> event latency (usually interrupts) is undefined anyway and coalescing
>> of multiples may have happened as part of that. This usually matches
>> expectations of real guests where interrupt latency is ultimately
>> undefined.
>
>
> ptimer_tick() is re-arm'ing the qemu_timer() in case of periodic mode. Hope
> I haven't missed your point here.
>

Yes. But it is also capable of doing the heavy lifting for our already
expired case. Basic idea is, if the timer is in a bad state (should
have hit) but hasn't, do the hit to put the timer into a good state
(by calling ptimer_tick) then just do the right thing. That's what the
loop does. It should also work for an in-the-past one-shot.

Regards,
Peter

>>
>> Regards,
>> Peter
>>
>
> --
> Dmitry

Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Dmitry Osipenko]

25.10.2015 02:55, Peter Crosthwaite пишет:
> On Sat, Oct 24, 2015 at 3:22 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> 24.10.2015 22:45, Peter Crosthwaite пишет:
>>>
>>>
>>> This looks like a give-up without trying to get the correct value. If
>>> the calculated value (using the normal-path logic below) is sane, you
>>> should just use it. If it comes out bad then you should clamp to 1.
>>>
>>> I am wondering whether this clamping policy (as in original code as
>>> well) is correct at all though. The value of a free-running
>>> short-interval periodic timer (poor mans random number generator)
>>> without any actual interrupt generation will be affected by QEMUs
>>> asynchronous handling of timer events. So if I set limit to 100, then
>>> sample this timer every user keyboard stroke, I should get a uniform
>>> distribution on [0,100]. Instead in am going to get lots of 1s. This
>>
>>
>> Right, that's a good example. What about to scale ptimer period to match
>> adjusted timer_mod interval?
>>
>
> Thats just as incorrect as changing the limit IMO. The guest could get
> confused with a timer running at the wrong frequency.
>
>>> is more broken in the original code (as you state), as I will get >
>>> 100, but I think we have traded broken for slightly less broken. I
>>> think the correct semantic is to completely ignoring rate limitin
>>> except for the scheduling on event callbacks. That is, the timer
>>
>>
>> I'm missing you here. What event callbacks?
>>
>
> when timer_mod() hits, and it turn triggers some device specific event
> (usually an interrupt).
>
> There are two basic interactions for any QEMU timer. There are
> synchronous events, i.e. the guest reading (polling) the counter which
> is what this patch tries to fix. The second is the common case of
> periodic interrupt generation. My proposal is that rate limiting does
> not affect synchronous operation, only asynchronous (so my keystroke
> RNG case works). In the current code, if ptimer_get_count() is called
> when the event has passed it returns 0 under the assumption that the
> timer_mod callback is about to happen. With rate-limiting that may be
> well in the future.
>

ptimer_tick() would happen on the next QEMU loop cycle, so it might be more 
reasonable to return counter = 1 here, wouldn't it?

>>> interval is not rate limited, instead the timer_mod interval
>>> (next_event -last_event) just has a 10us clamp.
>>>
>>> The means the original code semantic of returning counter = 0 for an
>>> already triggered timer is wrong. It should handle in-the-past
>>> wrap-arounds as wraparounds by triggering the timer and redoing the
>>> math with the new interval values. So instead the logic would be
>>> something like:
>>>
>>> timer_val = -1;
>>>
>>> for(;;) {
>>>
>>>    if (!enabled) {
>>>       return delta;
>>>    }
>>>
>>>    timer_val = (next-event - now) *  scaling();
>>>    if (timer_val >=  0) {
>>>        return timer_val;
>>>    }
>>>    /* Timer has actually expired but we missed it, reload it and try again
>>> */
>>>    ptimer_tick();
>>> }
>>
>>
>> Why do you think that ptimer_get_count() == 0 in case of the running
>> periodic timer that was expired while QEMU was "on the way" to ptimer code
>> is bad and wrong?
>
> Because you may have gone past the time when it was actually zero and
> reloaded and started counting again. It should return the real
> (reloaded and resumed) value. This is made worse by rate limiting as
> the timer will spend a long time at the clamp value waiting for the
> rate-limited tick to fix it.
>
> Following on from before, we don't want the async stuff to affect
> sync. As the async callbacks are heavily affected by rate limiting we
> don't want the polled timer having to rely on the callbacks (async
> path) at all for correct operation. That's what the current
> implementation of ptimer_get_count assumes with that 0-clamp.
>

Alright, that make sense now. Thanks for clarifying.

>>  From the guest point of view it's okay (no?), do we really
>> need to overengineer that corner case?
>>
>
> Depends on your use case. Your bug report is correct in that the timer
> should never be outside the bounds of the limit. But you are fixing
> that very specifically with a minimal change rather than correcting
> the larger ptimer_get_count() logic which I think is more broken than
> you suggest it is.
>
>>>
>>> ptimer_reload() then needs to be patched to make sure it always
>>> timer_mod()s in the future, otherwise this loop could iterate a large
>>> number of times.
>>>
>>> This means that when the qemu_timer() actually ticks, a large number
>>> or cycles may have occured, but we can justify that in that callback
>>> event latency (usually interrupts) is undefined anyway and coalescing
>>> of multiples may have happened as part of that. This usually matches
>>> expectations of real guests where interrupt latency is ultimately
>>> undefined.
>>
>>
>> ptimer_tick() is re-arm'ing the qemu_timer() in case of periodic mode. Hope
>> I haven't missed your point here.
>>
>
> Yes. But it is also capable of doing the heavy lifting for our already
> expired case. Basic idea is, if the timer is in a bad state (should
> have hit) but hasn't, do the hit to put the timer into a good state
> (by calling ptimer_tick) then just do the right thing. That's what the
> loop does. It should also work for an in-the-past one-shot.
>

Summarizing what we have now:

There are two issues with ptimer:

1) ptimer_get_count() return wrong values with adjusted .limit

Patch V7 doesn't solve that issue, but makes it slightly better by clamping 
returned value to [0, 1]. That's not what we want, we need to return counter 
value in it's valid range [0, limit].

You are rejecting variant of scaling ptimer period, saying that it might affect 
software behavior inside the guest. But by adjusting the timer, we might already 
causing same misbehavior in case of blazing fast host machine. I'll scratch my 
head a bit more on it. If you have any better idea, please share.

2) ptimer_get_count() return fake 0 value in case of the expired qemu_timer() 
without triggering ptimer_tick()

You're suggesting to solve it by running ptimer_tick(). So if emulated device 
uses ptimer tick event (scheduled qemu bh) to raise interrupt, it would do it by 
one QEMU loop cycle earlier.

My question here: is it always legal for the guest software to be able to get 
counter = 0 on poll while CPU interrupt on timer expire hasn't happened yet 
(would happen after one QEMU cycle). I guess it might cause software misbehavior 
if it assumes that the real hardware has CPU and timer running in the same clock 
domain, i.e. such situation might be not possible. So I'm suggesting to return 
counter = 1 and allow ptimer_tick() happen on it's own.

-- 
Dmitry

Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Peter Crosthwaite]

On Sun, Oct 25, 2015 at 6:23 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> 25.10.2015 02:55, Peter Crosthwaite пишет:
>
>> On Sat, Oct 24, 2015 at 3:22 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>
>>> 24.10.2015 22:45, Peter Crosthwaite пишет:
>>>>
>>>>
>>>>
>>>> This looks like a give-up without trying to get the correct value. If
>>>> the calculated value (using the normal-path logic below) is sane, you
>>>> should just use it. If it comes out bad then you should clamp to 1.
>>>>
>>>> I am wondering whether this clamping policy (as in original code as
>>>> well) is correct at all though. The value of a free-running
>>>> short-interval periodic timer (poor mans random number generator)
>>>> without any actual interrupt generation will be affected by QEMUs
>>>> asynchronous handling of timer events. So if I set limit to 100, then
>>>> sample this timer every user keyboard stroke, I should get a uniform
>>>> distribution on [0,100]. Instead in am going to get lots of 1s. This
>>>
>>>
>>>
>>> Right, that's a good example. What about to scale ptimer period to match
>>> adjusted timer_mod interval?
>>>
>>
>> Thats just as incorrect as changing the limit IMO. The guest could get
>> confused with a timer running at the wrong frequency.
>>
>>>> is more broken in the original code (as you state), as I will get >
>>>> 100, but I think we have traded broken for slightly less broken. I
>>>> think the correct semantic is to completely ignoring rate limitin
>>>> except for the scheduling on event callbacks. That is, the timer
>>>
>>>
>>>
>>> I'm missing you here. What event callbacks?
>>>
>>
>> when timer_mod() hits, and it turn triggers some device specific event
>> (usually an interrupt).
>>
>> There are two basic interactions for any QEMU timer. There are
>> synchronous events, i.e. the guest reading (polling) the counter which
>> is what this patch tries to fix. The second is the common case of
>> periodic interrupt generation. My proposal is that rate limiting does
>> not affect synchronous operation, only asynchronous (so my keystroke
>> RNG case works). In the current code, if ptimer_get_count() is called
>> when the event has passed it returns 0 under the assumption that the
>> timer_mod callback is about to happen. With rate-limiting that may be
>> well in the future.
>>
>
> ptimer_tick() would happen on the next QEMU loop cycle, so it might be more
> reasonable to return counter = 1 here, wouldn't it?
>
>
>>>> interval is not rate limited, instead the timer_mod interval
>>>> (next_event -last_event) just has a 10us clamp.
>>>>
>>>> The means the original code semantic of returning counter = 0 for an
>>>> already triggered timer is wrong. It should handle in-the-past
>>>> wrap-arounds as wraparounds by triggering the timer and redoing the
>>>> math with the new interval values. So instead the logic would be
>>>> something like:
>>>>
>>>> timer_val = -1;
>>>>
>>>> for(;;) {
>>>>
>>>>    if (!enabled) {
>>>>       return delta;
>>>>    }
>>>>
>>>>    timer_val = (next-event - now) *  scaling();
>>>>    if (timer_val >=  0) {
>>>>        return timer_val;
>>>>    }
>>>>    /* Timer has actually expired but we missed it, reload it and try
>>>> again
>>>> */
>>>>    ptimer_tick();
>>>> }
>>>
>>>
>>>
>>> Why do you think that ptimer_get_count() == 0 in case of the running
>>> periodic timer that was expired while QEMU was "on the way" to ptimer
>>> code
>>> is bad and wrong?
>>
>>
>> Because you may have gone past the time when it was actually zero and
>> reloaded and started counting again. It should return the real
>> (reloaded and resumed) value. This is made worse by rate limiting as
>> the timer will spend a long time at the clamp value waiting for the
>> rate-limited tick to fix it.
>>
>> Following on from before, we don't want the async stuff to affect
>> sync. As the async callbacks are heavily affected by rate limiting we
>> don't want the polled timer having to rely on the callbacks (async
>> path) at all for correct operation. That's what the current
>> implementation of ptimer_get_count assumes with that 0-clamp.
>>
>
> Alright, that make sense now. Thanks for clarifying.
>
>>>  From the guest point of view it's okay (no?), do we really
>>> need to overengineer that corner case?
>>>
>>
>> Depends on your use case. Your bug report is correct in that the timer
>> should never be outside the bounds of the limit. But you are fixing
>> that very specifically with a minimal change rather than correcting
>> the larger ptimer_get_count() logic which I think is more broken than
>> you suggest it is.
>>
>>>>
>>>> ptimer_reload() then needs to be patched to make sure it always
>>>> timer_mod()s in the future, otherwise this loop could iterate a large
>>>> number of times.
>>>>
>>>> This means that when the qemu_timer() actually ticks, a large number
>>>> or cycles may have occured, but we can justify that in that callback
>>>> event latency (usually interrupts) is undefined anyway and coalescing
>>>> of multiples may have happened as part of that. This usually matches
>>>> expectations of real guests where interrupt latency is ultimately
>>>> undefined.
>>>
>>>
>>>
>>> ptimer_tick() is re-arm'ing the qemu_timer() in case of periodic mode.
>>> Hope
>>> I haven't missed your point here.
>>>
>>
>> Yes. But it is also capable of doing the heavy lifting for our already
>> expired case. Basic idea is, if the timer is in a bad state (should
>> have hit) but hasn't, do the hit to put the timer into a good state
>> (by calling ptimer_tick) then just do the right thing. That's what the
>> loop does. It should also work for an in-the-past one-shot.
>>
>
> Summarizing what we have now:
>
> There are two issues with ptimer:
>
> 1) ptimer_get_count() return wrong values with adjusted .limit
>
> Patch V7 doesn't solve that issue, but makes it slightly better by clamping
> returned value to [0, 1]. That's not what we want, we need to return counter
> value in it's valid range [0, limit].
>
> You are rejecting variant of scaling ptimer period, saying that it might
> affect software behavior inside the guest. But by adjusting the timer, we
> might already causing same misbehavior in case of blazing fast host machine.

It is a different misbehaviour. We are modelling the polled timer
perfectly but limiting the frequency of callbacks (interrupts). I
think this is the lesser of two evils.

> I'll scratch my head a bit more on it. If you have any better idea, please
> share.
>
> 2) ptimer_get_count() return fake 0 value in case of the expired
> qemu_timer() without triggering ptimer_tick()
>
> You're suggesting to solve it by running ptimer_tick(). So if emulated
> device uses ptimer tick event (scheduled qemu bh) to raise interrupt, it
> would do it by one QEMU loop cycle earlier.
>

Yes, this is ok, as even in a rate limited scenario there is no reason
to absolutely force the rate limit. If a poll happens it should just
flush the waiting interrupt.

> My question here: is it always legal for the guest software to be able to
> get counter = 0 on poll while CPU interrupt on timer expire hasn't happened
> yet (would happen after one QEMU cycle).

Yes. And I am going a step further by saying it is ok for the guest
software to see the timer value wrapped around before the expire too.

>I guess it might cause software
> misbehavior if it assumes that the real hardware has CPU and timer running
> in the same clock domain, i.e. such situation might be not possible.

Assumptions about the CPU clocking only make sense in icount mode,
where the rate limiter is disable anyway.

> So I'm
> suggesting to return counter = 1 and allow ptimer_tick() happen on it's own.
>

My alternate suggestion is, if you detect that the tick should have
already happened, just make it happen. I don't see the need to rate
limit a polled timer.

Regards,
Peter

> --
> Dmitry

Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Dmitry Osipenko]

25.10.2015 20:39, Peter Crosthwaite пишет:
> On Sun, Oct 25, 2015 at 6:23 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> 25.10.2015 02:55, Peter Crosthwaite пишет:
>>
>>> On Sat, Oct 24, 2015 at 3:22 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> 24.10.2015 22:45, Peter Crosthwaite пишет:
>>>>>
>>>>>
>>>>>
>>>>> This looks like a give-up without trying to get the correct value. If
>>>>> the calculated value (using the normal-path logic below) is sane, you
>>>>> should just use it. If it comes out bad then you should clamp to 1.
>>>>>
>>>>> I am wondering whether this clamping policy (as in original code as
>>>>> well) is correct at all though. The value of a free-running
>>>>> short-interval periodic timer (poor mans random number generator)
>>>>> without any actual interrupt generation will be affected by QEMUs
>>>>> asynchronous handling of timer events. So if I set limit to 100, then
>>>>> sample this timer every user keyboard stroke, I should get a uniform
>>>>> distribution on [0,100]. Instead in am going to get lots of 1s. This
>>>>
>>>>
>>>>
>>>> Right, that's a good example. What about to scale ptimer period to match
>>>> adjusted timer_mod interval?
>>>>
>>>
>>> Thats just as incorrect as changing the limit IMO. The guest could get
>>> confused with a timer running at the wrong frequency.
>>>
>>>>> is more broken in the original code (as you state), as I will get >
>>>>> 100, but I think we have traded broken for slightly less broken. I
>>>>> think the correct semantic is to completely ignoring rate limitin
>>>>> except for the scheduling on event callbacks. That is, the timer
>>>>
>>>>
>>>>
>>>> I'm missing you here. What event callbacks?
>>>>
>>>
>>> when timer_mod() hits, and it turn triggers some device specific event
>>> (usually an interrupt).
>>>
>>> There are two basic interactions for any QEMU timer. There are
>>> synchronous events, i.e. the guest reading (polling) the counter which
>>> is what this patch tries to fix. The second is the common case of
>>> periodic interrupt generation. My proposal is that rate limiting does
>>> not affect synchronous operation, only asynchronous (so my keystroke
>>> RNG case works). In the current code, if ptimer_get_count() is called
>>> when the event has passed it returns 0 under the assumption that the
>>> timer_mod callback is about to happen. With rate-limiting that may be
>>> well in the future.
>>>
>>
>> ptimer_tick() would happen on the next QEMU loop cycle, so it might be more
>> reasonable to return counter = 1 here, wouldn't it?
>>
>>
>>>>> interval is not rate limited, instead the timer_mod interval
>>>>> (next_event -last_event) just has a 10us clamp.
>>>>>
>>>>> The means the original code semantic of returning counter = 0 for an
>>>>> already triggered timer is wrong. It should handle in-the-past
>>>>> wrap-arounds as wraparounds by triggering the timer and redoing the
>>>>> math with the new interval values. So instead the logic would be
>>>>> something like:
>>>>>
>>>>> timer_val = -1;
>>>>>
>>>>> for(;;) {
>>>>>
>>>>>     if (!enabled) {
>>>>>        return delta;
>>>>>     }
>>>>>
>>>>>     timer_val = (next-event - now) *  scaling();
>>>>>     if (timer_val >=  0) {
>>>>>         return timer_val;
>>>>>     }
>>>>>     /* Timer has actually expired but we missed it, reload it and try
>>>>> again
>>>>> */
>>>>>     ptimer_tick();
>>>>> }
>>>>
>>>>
>>>>
>>>> Why do you think that ptimer_get_count() == 0 in case of the running
>>>> periodic timer that was expired while QEMU was "on the way" to ptimer
>>>> code
>>>> is bad and wrong?
>>>
>>>
>>> Because you may have gone past the time when it was actually zero and
>>> reloaded and started counting again. It should return the real
>>> (reloaded and resumed) value. This is made worse by rate limiting as
>>> the timer will spend a long time at the clamp value waiting for the
>>> rate-limited tick to fix it.
>>>
>>> Following on from before, we don't want the async stuff to affect
>>> sync. As the async callbacks are heavily affected by rate limiting we
>>> don't want the polled timer having to rely on the callbacks (async
>>> path) at all for correct operation. That's what the current
>>> implementation of ptimer_get_count assumes with that 0-clamp.
>>>
>>
>> Alright, that make sense now. Thanks for clarifying.
>>
>>>>   From the guest point of view it's okay (no?), do we really
>>>> need to overengineer that corner case?
>>>>
>>>
>>> Depends on your use case. Your bug report is correct in that the timer
>>> should never be outside the bounds of the limit. But you are fixing
>>> that very specifically with a minimal change rather than correcting
>>> the larger ptimer_get_count() logic which I think is more broken than
>>> you suggest it is.
>>>
>>>>>
>>>>> ptimer_reload() then needs to be patched to make sure it always
>>>>> timer_mod()s in the future, otherwise this loop could iterate a large
>>>>> number of times.
>>>>>
>>>>> This means that when the qemu_timer() actually ticks, a large number
>>>>> or cycles may have occured, but we can justify that in that callback
>>>>> event latency (usually interrupts) is undefined anyway and coalescing
>>>>> of multiples may have happened as part of that. This usually matches
>>>>> expectations of real guests where interrupt latency is ultimately
>>>>> undefined.
>>>>
>>>>
>>>>
>>>> ptimer_tick() is re-arm'ing the qemu_timer() in case of periodic mode.
>>>> Hope
>>>> I haven't missed your point here.
>>>>
>>>
>>> Yes. But it is also capable of doing the heavy lifting for our already
>>> expired case. Basic idea is, if the timer is in a bad state (should
>>> have hit) but hasn't, do the hit to put the timer into a good state
>>> (by calling ptimer_tick) then just do the right thing. That's what the
>>> loop does. It should also work for an in-the-past one-shot.
>>>
>>
>> Summarizing what we have now:
>>
>> There are two issues with ptimer:
>>
>> 1) ptimer_get_count() return wrong values with adjusted .limit
>>
>> Patch V7 doesn't solve that issue, but makes it slightly better by clamping
>> returned value to [0, 1]. That's not what we want, we need to return counter
>> value in it's valid range [0, limit].
>>
>> You are rejecting variant of scaling ptimer period, saying that it might
>> affect software behavior inside the guest. But by adjusting the timer, we
>> might already causing same misbehavior in case of blazing fast host machine.
>
> It is a different misbehaviour. We are modelling the polled timer
> perfectly but limiting the frequency of callbacks (interrupts). I
> think this is the lesser of two evils.
>
>> I'll scratch my head a bit more on it. If you have any better idea, please
>> share.
>>
>> 2) ptimer_get_count() return fake 0 value in case of the expired
>> qemu_timer() without triggering ptimer_tick()
>>
>> You're suggesting to solve it by running ptimer_tick(). So if emulated
>> device uses ptimer tick event (scheduled qemu bh) to raise interrupt, it
>> would do it by one QEMU loop cycle earlier.
>>
>
> Yes, this is ok, as even in a rate limited scenario there is no reason
> to absolutely force the rate limit. If a poll happens it should just
> flush the waiting interrupt.
>
>> My question here: is it always legal for the guest software to be able to
>> get counter = 0 on poll while CPU interrupt on timer expire hasn't happened
>> yet (would happen after one QEMU cycle).
>
> Yes. And I am going a step further by saying it is ok for the guest
> software to see the timer value wrapped around before the expire too.
>

Let's imagine a hardware with a such restriction: timer interrupt has highest 
priority and CPU immediately switches to the interrupt handler in a such way 
that it won't ever could see counter = 0 / wraparound (with interrupt enabled) 
before entering the handler.

Is it unrealistic?

For instance (on QEMU):

              CPU                  |               Timer
---------------------------------------------------------------------
   start_periodic_timer            |       timer starts ticking
                                 .....
   QEMU starts to execute          |
   translated block                |
                                   |       QEMU timer expires
                                   |
   CPU reads the timer register,   |       ptimer_get_count() return
   ptimer_get_count() called       |       wrapped around value
                                 .....
   CPU interrupt handler kicks in  |       timer continue ticking, so
                                   |       any value is valid here
   CPU stops the timer and sets    |
   counter to 0, returns from the  |
   handler                         |
                                 .....
   Now, for some reason, software  |
   sees that timer is stopped      |
   and do something using read     |
   value                           |

Program code sketch:

timer_interrupt_handler()
{
	write32(1, TIMER_STOP);
	write32(0, TIMER_COUNTER);
	write32(TIMER_IRQ_CLEAR, TIMER_STATUS);

	return IRQ_HANDLED;
}

program()
{
         .....

	..... <--- timer expired here
	..... <--- interrupt handler executed here on real HW

	var1 = read32(TIMER_COUNTER); <--- Emulated got wrapped,
	                                   real got 0

	..... <--- interrupt handler executed here on QEMU

	if (read32(TIMER_STATUS) & TIMER_RUNNING) {
		.....
	} else {
                 .....

		write(var1 >> 16, SOME_DEV_REGISTER);
	}

         .....
}

Might emulated program behave differently from the real HW after it? Probably.

I want to mention that not only beefy generic CPU's are the ptimer users.

However, it seems that no one of the current ptimer users has a such restriction 
since it would already return 0 on expire and ptimer_tick() would happen after 
it. We can agree on keeping ptimer less universal in favor of
the expire optimization, so somebody may improve it later if it would be needed.

Do we agree?

>> I guess it might cause software
>> misbehavior if it assumes that the real hardware has CPU and timer running
>> in the same clock domain, i.e. such situation might be not possible.
>
> Assumptions about the CPU clocking only make sense in icount mode,
> where the rate limiter is disable anyway.
>

Timer limiter has nothing to do with a returned value for the expired timer. 
Clock cycle accurate execution isn't relevant to upstream QEMU, I meant clocking 
in general. Emulated behavior shouldn't diverge from the real HW.

>> So I'm
>> suggesting to return counter = 1 and allow ptimer_tick() happen on it's own.
>>
>
> My alternate suggestion is, if you detect that the tick should have
> already happened, just make it happen. I don't see the need to rate
> limit a polled timer.
>

Yes, I got your idea and it is absolutely correct if we agree on the above 
tradeoff (if that tradeoff exists).

-- 
Dmitry

Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 12218 bytes --]

On Tue, Oct 27, 2015 at 2:26 PM, Dmitry Osipenko <digetx@gmail.com> wrote:

> 25.10.2015 20:39, Peter Crosthwaite пишет:
>
> On Sun, Oct 25, 2015 at 6:23 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>>> 25.10.2015 02:55, Peter Crosthwaite пишет:
>>>
>>> On Sat, Oct 24, 2015 at 3:22 PM, Dmitry Osipenko <digetx@gmail.com>
>>>> wrote:
>>>>
>>>>>
>>>>> 24.10.2015 22:45, Peter Crosthwaite пишет:
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> This looks like a give-up without trying to get the correct value. If
>>>>>> the calculated value (using the normal-path logic below) is sane, you
>>>>>> should just use it. If it comes out bad then you should clamp to 1.
>>>>>>
>>>>>> I am wondering whether this clamping policy (as in original code as
>>>>>> well) is correct at all though. The value of a free-running
>>>>>> short-interval periodic timer (poor mans random number generator)
>>>>>> without any actual interrupt generation will be affected by QEMUs
>>>>>> asynchronous handling of timer events. So if I set limit to 100, then
>>>>>> sample this timer every user keyboard stroke, I should get a uniform
>>>>>> distribution on [0,100]. Instead in am going to get lots of 1s. This
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Right, that's a good example. What about to scale ptimer period to
>>>>> match
>>>>> adjusted timer_mod interval?
>>>>>
>>>>>
>>>> Thats just as incorrect as changing the limit IMO. The guest could get
>>>> confused with a timer running at the wrong frequency.
>>>>
>>>> is more broken in the original code (as you state), as I will get >
>>>>>> 100, but I think we have traded broken for slightly less broken. I
>>>>>> think the correct semantic is to completely ignoring rate limitin
>>>>>> except for the scheduling on event callbacks. That is, the timer
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I'm missing you here. What event callbacks?
>>>>>
>>>>>
>>>> when timer_mod() hits, and it turn triggers some device specific event
>>>> (usually an interrupt).
>>>>
>>>> There are two basic interactions for any QEMU timer. There are
>>>> synchronous events, i.e. the guest reading (polling) the counter which
>>>> is what this patch tries to fix. The second is the common case of
>>>> periodic interrupt generation. My proposal is that rate limiting does
>>>> not affect synchronous operation, only asynchronous (so my keystroke
>>>> RNG case works). In the current code, if ptimer_get_count() is called
>>>> when the event has passed it returns 0 under the assumption that the
>>>> timer_mod callback is about to happen. With rate-limiting that may be
>>>> well in the future.
>>>>
>>>>
>>> ptimer_tick() would happen on the next QEMU loop cycle, so it might be
>>> more
>>> reasonable to return counter = 1 here, wouldn't it?
>>>
>>>
>>> interval is not rate limited, instead the timer_mod interval
>>>>>> (next_event -last_event) just has a 10us clamp.
>>>>>>
>>>>>> The means the original code semantic of returning counter = 0 for an
>>>>>> already triggered timer is wrong. It should handle in-the-past
>>>>>> wrap-arounds as wraparounds by triggering the timer and redoing the
>>>>>> math with the new interval values. So instead the logic would be
>>>>>> something like:
>>>>>>
>>>>>> timer_val = -1;
>>>>>>
>>>>>> for(;;) {
>>>>>>
>>>>>>     if (!enabled) {
>>>>>>        return delta;
>>>>>>     }
>>>>>>
>>>>>>     timer_val = (next-event - now) *  scaling();
>>>>>>     if (timer_val >=  0) {
>>>>>>         return timer_val;
>>>>>>     }
>>>>>>     /* Timer has actually expired but we missed it, reload it and try
>>>>>> again
>>>>>> */
>>>>>>     ptimer_tick();
>>>>>> }
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Why do you think that ptimer_get_count() == 0 in case of the running
>>>>> periodic timer that was expired while QEMU was "on the way" to ptimer
>>>>> code
>>>>> is bad and wrong?
>>>>>
>>>>
>>>>
>>>> Because you may have gone past the time when it was actually zero and
>>>> reloaded and started counting again. It should return the real
>>>> (reloaded and resumed) value. This is made worse by rate limiting as
>>>> the timer will spend a long time at the clamp value waiting for the
>>>> rate-limited tick to fix it.
>>>>
>>>> Following on from before, we don't want the async stuff to affect
>>>> sync. As the async callbacks are heavily affected by rate limiting we
>>>> don't want the polled timer having to rely on the callbacks (async
>>>> path) at all for correct operation. That's what the current
>>>> implementation of ptimer_get_count assumes with that 0-clamp.
>>>>
>>>>
>>> Alright, that make sense now. Thanks for clarifying.
>>>
>>>   From the guest point of view it's okay (no?), do we really
>>>>> need to overengineer that corner case?
>>>>>
>>>>>
>>>> Depends on your use case. Your bug report is correct in that the timer
>>>> should never be outside the bounds of the limit. But you are fixing
>>>> that very specifically with a minimal change rather than correcting
>>>> the larger ptimer_get_count() logic which I think is more broken than
>>>> you suggest it is.
>>>>
>>>>
>>>>>> ptimer_reload() then needs to be patched to make sure it always
>>>>>> timer_mod()s in the future, otherwise this loop could iterate a large
>>>>>> number of times.
>>>>>>
>>>>>> This means that when the qemu_timer() actually ticks, a large number
>>>>>> or cycles may have occured, but we can justify that in that callback
>>>>>> event latency (usually interrupts) is undefined anyway and coalescing
>>>>>> of multiples may have happened as part of that. This usually matches
>>>>>> expectations of real guests where interrupt latency is ultimately
>>>>>> undefined.
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> ptimer_tick() is re-arm'ing the qemu_timer() in case of periodic mode.
>>>>> Hope
>>>>> I haven't missed your point here.
>>>>>
>>>>>
>>>> Yes. But it is also capable of doing the heavy lifting for our already
>>>> expired case. Basic idea is, if the timer is in a bad state (should
>>>> have hit) but hasn't, do the hit to put the timer into a good state
>>>> (by calling ptimer_tick) then just do the right thing. That's what the
>>>> loop does. It should also work for an in-the-past one-shot.
>>>>
>>>>
>>> Summarizing what we have now:
>>>
>>> There are two issues with ptimer:
>>>
>>> 1) ptimer_get_count() return wrong values with adjusted .limit
>>>
>>> Patch V7 doesn't solve that issue, but makes it slightly better by
>>> clamping
>>> returned value to [0, 1]. That's not what we want, we need to return
>>> counter
>>> value in it's valid range [0, limit].
>>>
>>> You are rejecting variant of scaling ptimer period, saying that it might
>>> affect software behavior inside the guest. But by adjusting the timer, we
>>> might already causing same misbehavior in case of blazing fast host
>>> machine.
>>>
>>
>> It is a different misbehaviour. We are modelling the polled timer
>> perfectly but limiting the frequency of callbacks (interrupts). I
>> think this is the lesser of two evils.
>>
>> I'll scratch my head a bit more on it. If you have any better idea, please
>>> share.
>>>
>>> 2) ptimer_get_count() return fake 0 value in case of the expired
>>> qemu_timer() without triggering ptimer_tick()
>>>
>>> You're suggesting to solve it by running ptimer_tick(). So if emulated
>>> device uses ptimer tick event (scheduled qemu bh) to raise interrupt, it
>>> would do it by one QEMU loop cycle earlier.
>>>
>>>
>> Yes, this is ok, as even in a rate limited scenario there is no reason
>> to absolutely force the rate limit. If a poll happens it should just
>> flush the waiting interrupt.
>>
>> My question here: is it always legal for the guest software to be able to
>>> get counter = 0 on poll while CPU interrupt on timer expire hasn't
>>> happened
>>> yet (would happen after one QEMU cycle).
>>>
>>
>> Yes. And I am going a step further by saying it is ok for the guest
>> software to see the timer value wrapped around before the expire too.
>>
>>
> Let's imagine a hardware with a such restriction: timer interrupt has
> highest priority and CPU immediately switches to the interrupt handler in a
> such way that it won't ever could see counter = 0 / wraparound (with
> interrupt enabled) before entering the handler.
>
> Is it unrealistic?
>
>
Yes. And if it is possible in real HW, I don't think this is valid for QEMU
outside of icount mode.



> For instance (on QEMU):
>
>              CPU                  |               Timer
> ---------------------------------------------------------------------
>   start_periodic_timer            |       timer starts ticking
>                                 .....
>   QEMU starts to execute          |
>   translated block                |
>                                   |       QEMU timer expires
>                                   |
>   CPU reads the timer register,   |       ptimer_get_count() return
>   ptimer_get_count() called       |       wrapped around value
>                                 .....
>   CPU interrupt handler kicks in  |       timer continue ticking, so
>                                   |       any value is valid here
>   CPU stops the timer and sets    |
>   counter to 0, returns from the  |
>   handler                         |
>                                 .....
>   Now, for some reason, software  |
>   sees that timer is stopped      |
>   and do something using read     |
>   value                           |
>
> Program code sketch:
>
> timer_interrupt_handler()
> {
>         write32(1, TIMER_STOP);
>         write32(0, TIMER_COUNTER);
>         write32(TIMER_IRQ_CLEAR, TIMER_STATUS);
>
>         return IRQ_HANDLED;
> }
>
> program()
> {
>         .....
>
>         ..... <--- timer expired here
>         ..... <--- interrupt handler executed here on real HW
>
>         var1 = read32(TIMER_COUNTER); <--- Emulated got wrapped,
>                                            real got 0
>
>         ..... <--- interrupt handler executed here on QEMU
>
>         if (read32(TIMER_STATUS) & TIMER_RUNNING) {
>                 .....
>         } else {
>                 .....
>
>                 write(var1 >> 16, SOME_DEV_REGISTER);
>         }
>
>         .....
> }
>
> Might emulated program behave differently from the real HW after it?
> Probably.
>
> I want to mention that not only beefy generic CPU's are the ptimer users.
>
> However, it seems that no one of the current ptimer users has a such
> restriction since it would already return 0 on expire and ptimer_tick()
> would happen after it. We can agree on keeping ptimer less universal in
> favor of
> the expire optimization, so somebody may improve it later if it would be
> needed.
>
>
I think removing the rate limiter's and clamping-affect on the read value
makes it more universal if anything.


> Do we agree?
>
>
I'm not sure, what are you referring to as the "expire optimizsation"?


> I guess it might cause software
>>> misbehavior if it assumes that the real hardware has CPU and timer
>>> running
>>> in the same clock domain, i.e. such situation might be not possible.
>>>
>>
>> Assumptions about the CPU clocking only make sense in icount mode,
>> where the rate limiter is disable anyway.
>>
>>
> Timer limiter has nothing to do with a returned value for the expired
> timer. Clock cycle accurate execution isn't relevant to upstream QEMU, I
> meant clocking in general. Emulated behaviour shouldn't diverge from the
> real HW.
>
>
But when the rate limiter is on for a short interval, it massively distorts
this.

Regards,
Peter


> So I'm
>>> suggesting to return counter = 1 and allow ptimer_tick() happen on it's
>>> own.
>>>
>>>
>> My alternate suggestion is, if you detect that the tick should have
>> already happened, just make it happen. I don't see the need to rate
>> limit a polled timer.
>>
>>
> Yes, I got your idea and it is absolutely correct if we agree on the above
> tradeoff (if that tradeoff exists).
>
> --
> Dmitry
>

[-- Attachment #2: Type: text/html, Size: 16165 bytes --]

Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Dmitry Osipenko]

29.10.2015 04:39, Peter Crosthwaite пишет:
>
>
> On Tue, Oct 27, 2015 at 2:26 PM, Dmitry Osipenko <digetx@gmail.com
> <mailto:digetx@gmail.com>> wrote:
>
>     25.10.2015 20:39, Peter Crosthwaite пишет:
>
>         On Sun, Oct 25, 2015 at 6:23 AM, Dmitry Osipenko <digetx@gmail.com
>         <mailto:digetx@gmail.com>> wrote:
>
>             25.10.2015 02:55, Peter Crosthwaite пишет:
>
>                 On Sat, Oct 24, 2015 at 3:22 PM, Dmitry Osipenko
>                 <digetx@gmail.com <mailto:digetx@gmail.com>> wrote:
>
>
>                     24.10.2015 22:45, Peter Crosthwaite пишет:
>
>
>
>
>                         This looks like a give-up without trying to get the
>                         correct value. If
>                         the calculated value (using the normal-path logic below)
>                         is sane, you
>                         should just use it. If it comes out bad then you should
>                         clamp to 1.
>
>                         I am wondering whether this clamping policy (as in
>                         original code as
>                         well) is correct at all though. The value of a free-running
>                         short-interval periodic timer (poor mans random number
>                         generator)
>                         without any actual interrupt generation will be affected
>                         by QEMUs
>                         asynchronous handling of timer events. So if I set limit
>                         to 100, then
>                         sample this timer every user keyboard stroke, I should
>                         get a uniform
>                         distribution on [0,100]. Instead in am going to get lots
>                         of 1s. This
>
>
>
>
>                     Right, that's a good example. What about to scale ptimer
>                     period to match
>                     adjusted timer_mod interval?
>
>
>                 Thats just as incorrect as changing the limit IMO. The guest
>                 could get
>                 confused with a timer running at the wrong frequency.
>
>                         is more broken in the original code (as you state), as I
>                         will get >
>                         100, but I think we have traded broken for slightly less
>                         broken. I
>                         think the correct semantic is to completely ignoring
>                         rate limitin
>                         except for the scheduling on event callbacks. That is,
>                         the timer
>
>
>
>
>                     I'm missing you here. What event callbacks?
>
>
>                 when timer_mod() hits, and it turn triggers some device specific
>                 event
>                 (usually an interrupt).
>
>                 There are two basic interactions for any QEMU timer. There are
>                 synchronous events, i.e. the guest reading (polling) the counter
>                 which
>                 is what this patch tries to fix. The second is the common case of
>                 periodic interrupt generation. My proposal is that rate limiting
>                 does
>                 not affect synchronous operation, only asynchronous (so my keystroke
>                 RNG case works). In the current code, if ptimer_get_count() is
>                 called
>                 when the event has passed it returns 0 under the assumption that the
>                 timer_mod callback is about to happen. With rate-limiting that
>                 may be
>                 well in the future.
>
>
>             ptimer_tick() would happen on the next QEMU loop cycle, so it might
>             be more
>             reasonable to return counter = 1 here, wouldn't it?
>
>
>                         interval is not rate limited, instead the timer_mod interval
>                         (next_event -last_event) just has a 10us clamp.
>
>                         The means the original code semantic of returning
>                         counter = 0 for an
>                         already triggered timer is wrong. It should handle
>                         in-the-past
>                         wrap-arounds as wraparounds by triggering the timer and
>                         redoing the
>                         math with the new interval values. So instead the logic
>                         would be
>                         something like:
>
>                         timer_val = -1;
>
>                         for(;;) {
>
>                              if (!enabled) {
>                                 return delta;
>                              }
>
>                              timer_val = (next-event - now) *  scaling();
>                              if (timer_val >=  0) {
>                                  return timer_val;
>                              }
>                              /* Timer has actually expired but we missed it,
>                         reload it and try
>                         again
>                         */
>                              ptimer_tick();
>                         }
>
>
>
>
>                     Why do you think that ptimer_get_count() == 0 in case of the
>                     running
>                     periodic timer that was expired while QEMU was "on the way"
>                     to ptimer
>                     code
>                     is bad and wrong?
>
>
>
>                 Because you may have gone past the time when it was actually
>                 zero and
>                 reloaded and started counting again. It should return the real
>                 (reloaded and resumed) value. This is made worse by rate limiting as
>                 the timer will spend a long time at the clamp value waiting for the
>                 rate-limited tick to fix it.
>
>                 Following on from before, we don't want the async stuff to affect
>                 sync. As the async callbacks are heavily affected by rate
>                 limiting we
>                 don't want the polled timer having to rely on the callbacks (async
>                 path) at all for correct operation. That's what the current
>                 implementation of ptimer_get_count assumes with that 0-clamp.
>
>
>             Alright, that make sense now. Thanks for clarifying.
>
>                        From the guest point of view it's okay (no?), do we really
>                     need to overengineer that corner case?
>
>
>                 Depends on your use case. Your bug report is correct in that the
>                 timer
>                 should never be outside the bounds of the limit. But you are fixing
>                 that very specifically with a minimal change rather than correcting
>                 the larger ptimer_get_count() logic which I think is more broken
>                 than
>                 you suggest it is.
>
>
>                         ptimer_reload() then needs to be patched to make sure it
>                         always
>                         timer_mod()s in the future, otherwise this loop could
>                         iterate a large
>                         number of times.
>
>                         This means that when the qemu_timer() actually ticks, a
>                         large number
>                         or cycles may have occured, but we can justify that in
>                         that callback
>                         event latency (usually interrupts) is undefined anyway
>                         and coalescing
>                         of multiples may have happened as part of that. This
>                         usually matches
>                         expectations of real guests where interrupt latency is
>                         ultimately
>                         undefined.
>
>
>
>
>                     ptimer_tick() is re-arm'ing the qemu_timer() in case of
>                     periodic mode.
>                     Hope
>                     I haven't missed your point here.
>
>
>                 Yes. But it is also capable of doing the heavy lifting for our
>                 already
>                 expired case. Basic idea is, if the timer is in a bad state (should
>                 have hit) but hasn't, do the hit to put the timer into a good state
>                 (by calling ptimer_tick) then just do the right thing. That's
>                 what the
>                 loop does. It should also work for an in-the-past one-shot.
>
>
>             Summarizing what we have now:
>
>             There are two issues with ptimer:
>
>             1) ptimer_get_count() return wrong values with adjusted .limit
>
>             Patch V7 doesn't solve that issue, but makes it slightly better by
>             clamping
>             returned value to [0, 1]. That's not what we want, we need to return
>             counter
>             value in it's valid range [0, limit].
>
>             You are rejecting variant of scaling ptimer period, saying that it might
>             affect software behavior inside the guest. But by adjusting the
>             timer, we
>             might already causing same misbehavior in case of blazing fast host
>             machine.
>
>
>         It is a different misbehaviour. We are modelling the polled timer
>         perfectly but limiting the frequency of callbacks (interrupts). I
>         think this is the lesser of two evils.
>
>             I'll scratch my head a bit more on it. If you have any better idea,
>             please
>             share.
>
>             2) ptimer_get_count() return fake 0 value in case of the expired
>             qemu_timer() without triggering ptimer_tick()
>
>             You're suggesting to solve it by running ptimer_tick(). So if emulated
>             device uses ptimer tick event (scheduled qemu bh) to raise interrupt, it
>             would do it by one QEMU loop cycle earlier.
>
>
>         Yes, this is ok, as even in a rate limited scenario there is no reason
>         to absolutely force the rate limit. If a poll happens it should just
>         flush the waiting interrupt.
>
>             My question here: is it always legal for the guest software to be
>             able to
>             get counter = 0 on poll while CPU interrupt on timer expire hasn't
>             happened
>             yet (would happen after one QEMU cycle).
>
>
>         Yes. And I am going a step further by saying it is ok for the guest
>         software to see the timer value wrapped around before the expire too.
>
>
>     Let's imagine a hardware with a such restriction: timer interrupt has
>     highest priority and CPU immediately switches to the interrupt handler in a
>     such way that it won't ever could see counter = 0 / wraparound (with
>     interrupt enabled) before entering the handler.
>
>     Is it unrealistic?
>
>
> Yes. And if it is possible in real HW, I don't think this is valid for QEMU
> outside of icount mode.
>

Okay, fair enough. So QEMU doesn't guarantee proper behavior outside of icount 
mode. That's not what I expected :(

>     For instance (on QEMU):
>
>                   CPU                  |               Timer
>     ---------------------------------------------------------------------
>        start_periodic_timer            |       timer starts ticking
>                                      .....
>        QEMU starts to execute          |
>        translated block                |
>                                        |       QEMU timer expires
>                                        |
>        CPU reads the timer register,   |       ptimer_get_count() return
>        ptimer_get_count() called       |       wrapped around value
>                                      .....
>        CPU interrupt handler kicks in  |       timer continue ticking, so
>                                        |       any value is valid here
>        CPU stops the timer and sets    |
>        counter to 0, returns from the  |
>        handler                         |
>                                      .....
>        Now, for some reason, software  |
>        sees that timer is stopped      |
>        and do something using read     |
>        value                           |
>
>     Program code sketch:
>
>     timer_interrupt_handler()
>     {
>              write32(1, TIMER_STOP);
>              write32(0, TIMER_COUNTER);
>              write32(TIMER_IRQ_CLEAR, TIMER_STATUS);
>
>              return IRQ_HANDLED;
>     }
>
>     program()
>     {
>              .....
>
>              ..... <--- timer expired here
>              ..... <--- interrupt handler executed here on real HW
>
>              var1 = read32(TIMER_COUNTER); <--- Emulated got wrapped,
>                                                 real got 0
>
>              ..... <--- interrupt handler executed here on QEMU
>
>              if (read32(TIMER_STATUS) & TIMER_RUNNING) {
>                      .....
>              } else {
>                      .....
>
>                      write(var1 >> 16, SOME_DEV_REGISTER);
>              }
>
>              .....
>     }
>
>     Might emulated program behave differently from the real HW after it? Probably.
>
>     I want to mention that not only beefy generic CPU's are the ptimer users.
>
>     However, it seems that no one of the current ptimer users has a such
>     restriction since it would already return 0 on expire and ptimer_tick()
>     would happen after it. We can agree on keeping ptimer less universal in favor of
>     the expire optimization, so somebody may improve it later if it would be needed.
>
>
> I think removing the rate limiter's and clamping-affect on the read value makes
> it more universal if anything.
>
>     Do we agree?
>
>
> I'm not sure, what are you referring to as the "expire optimizsation"?
>

By calling ptimer_tick() from ptimer_get_count() and returning wrapped around 
value (see the "future"), we would cause qemu_bh schedule happen earlier and 
might break expected IRQ ordering. That's what I meant by "expire optimization".

>             I guess it might cause software
>             misbehavior if it assumes that the real hardware has CPU and timer
>             running
>             in the same clock domain, i.e. such situation might be not possible.
>
>
>         Assumptions about the CPU clocking only make sense in icount mode,
>         where the rate limiter is disable anyway.
>
>
>     Timer limiter has nothing to do with a returned value for the expired timer.
>     Clock cycle accurate execution isn't relevant to upstream QEMU, I meant
>     clocking in general. Emulated behaviour shouldn't diverge from the real HW.
>
>
> But when the rate limiter is on for a short interval, it massively distorts this.

Two corner cases for the limited polled periodic ptimer:

1) QEMU timer expires during poll:

Like you are suggesting, in that case we are calculating wrapped around counter 
value, trigger ptimer_tick() and return wrapped value. Good.

2) Host machine is blazing fast, so poor RNG might constantly face "counter > 
limit":

How should we handle it?

- Period scaling is rejected, okay.

- Clamp counter to limit and pretend that it won't happen?

- Disable limiting for that host machine?

- Tune timer limit?

- Scale counter when we hit "counter > limit" in ptimer_get_count()? But now 
we'll have to book fact that we have hit that case and should scale counter on 
every next poll till timer expires to avoid jumping into the "past", when 
counter become less or equal the limit.

>
> Regards,
> Peter
>
>             So I'm
>             suggesting to return counter = 1 and allow ptimer_tick() happen on
>             it's own.
>
>
>         My alternate suggestion is, if you detect that the tick should have
>         already happened, just make it happen. I don't see the need to rate
>         limit a polled timer.
>
>
>     Yes, I got your idea and it is absolutely correct if we agree on the above
>     tradeoff (if that tradeoff exists).
>

-- 
Dmitry

Re: [Qemu-devel] [PATCH v7 1/2] hw/ptimer: Fix issues caused by artificially limited timer timeout

[Dmitry Osipenko]

29.10.2015 17:28, Dmitry Osipenko пишет:
> - Disable limiting for that host machine?
>
> - Tune timer limit?

These tho variants seem too farfetched, since we don't distinguish the guest 
software timer poll and some device internal ptimer_get_count() call. Just a note.

-- 
Dmitry

[Qemu-devel] [PATCH v7 2/2] arm_mptimer: Convert to use ptimer

[Dmitry Osipenko]

Current ARM MPTimer implementation uses QEMUTimer for the actual timer,
this implementation isn't complete and mostly tries to duplicate of what
generic ptimer is already doing fine.

Conversion to ptimer brings the following benefits and fixes:
	- Simple timer pausing implementation
	- Fixes counter value preservation after stopping the timer
	- Code simplification and reduction

Bump VMSD to version 3, since VMState is changed and is not compatible
with the previous implementation.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 hw/timer/arm_mptimer.c         | 110 ++++++++++++++++++-----------------------
 include/hw/timer/arm_mptimer.h |   4 +-
 2 files changed, 49 insertions(+), 65 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 3e59c2a..c06da5e 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -19,8 +19,9 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "hw/ptimer.h"
 #include "hw/timer/arm_mptimer.h"
-#include "qemu/timer.h"
+#include "qemu/main-loop.h"
 #include "qom/cpu.h"
 
 /* This device implements the per-cpu private timer and watchdog block
@@ -47,28 +48,10 @@ static inline uint32_t timerblock_scale(TimerBlock *tb)
     return (((tb->control >> 8) & 0xff) + 1) * 10;
 }
 
-static void timerblock_reload(TimerBlock *tb, int restart)
-{
-    if (tb->count == 0) {
-        return;
-    }
-    if (restart) {
-        tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    }
-    tb->tick += (int64_t)tb->count * timerblock_scale(tb);
-    timer_mod(tb->timer, tb->tick);
-}
-
 static void timerblock_tick(void *opaque)
 {
     TimerBlock *tb = (TimerBlock *)opaque;
     tb->status = 1;
-    if (tb->control & 2) {
-        tb->count = tb->load;
-        timerblock_reload(tb, 0);
-    } else {
-        tb->count = 0;
-    }
     timerblock_update_irq(tb);
 }
 
@@ -76,21 +59,11 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
                                 unsigned size)
 {
     TimerBlock *tb = (TimerBlock *)opaque;
-    int64_t val;
     switch (addr) {
     case 0: /* Load */
         return tb->load;
     case 4: /* Counter.  */
-        if (((tb->control & 1) == 0) || (tb->count == 0)) {
-            return 0;
-        }
-        /* Slow and ugly, but hopefully won't happen too often.  */
-        val = tb->tick - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        val /= timerblock_scale(tb);
-        if (val < 0) {
-            val = 0;
-        }
-        return val;
+        return ptimer_get_count(tb->timer);
     case 8: /* Control.  */
         return tb->control;
     case 12: /* Interrupt status.  */
@@ -100,6 +73,19 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
     }
 }
 
+static void timerblock_run(TimerBlock *tb, uint64_t count, int set_count)
+{
+    if (set_count) {
+        if (((tb->control & 3) == 3) && (count == 0)) {
+            count = tb->load;
+        }
+        ptimer_set_count(tb->timer, count);
+    }
+    if ((tb->control & 1) && (count != 0)) {
+        ptimer_run(tb->timer, !(tb->control & 2));
+    }
+}
+
 static void timerblock_write(void *opaque, hwaddr addr,
                              uint64_t value, unsigned size)
 {
@@ -108,32 +94,34 @@ static void timerblock_write(void *opaque, hwaddr addr,
     switch (addr) {
     case 0: /* Load */
         tb->load = value;
-        /* Fall through.  */
-    case 4: /* Counter.  */
-        if ((tb->control & 1) && tb->count) {
-            /* Cancel the previous timer.  */
-            timer_del(tb->timer);
+        /* Setting load to 0 stops the timer.  */
+        if (tb->load == 0) {
+            ptimer_stop(tb->timer);
         }
-        tb->count = value;
-        if (tb->control & 1) {
-            timerblock_reload(tb, 1);
+        ptimer_set_limit(tb->timer, tb->load, 1);
+        timerblock_run(tb, tb->load, 0);
+        break;
+    case 4: /* Counter.  */
+        /* Setting counter to 0 stops the one-shot timer.  */
+        if (!(tb->control & 2) && (value == 0)) {
+            ptimer_stop(tb->timer);
         }
+        timerblock_run(tb, value, 1);
         break;
     case 8: /* Control.  */
         old = tb->control;
         tb->control = value;
-        if (value & 1) {
-            if ((old & 1) && (tb->count != 0)) {
-                /* Do nothing if timer is ticking right now.  */
-                break;
-            }
-            if (tb->control & 2) {
-                tb->count = tb->load;
-            }
-            timerblock_reload(tb, 1);
-        } else if (old & 1) {
-            /* Shutdown the timer.  */
-            timer_del(tb->timer);
+        /* Timer mode switch requires ptimer to be stopped.  */
+        if ((old & 3) != (tb->control & 3)) {
+            ptimer_stop(tb->timer);
+        }
+        if (!(tb->control & 1)) {
+            break;
+        }
+        ptimer_set_period(tb->timer, timerblock_scale(tb));
+        if ((old & 3) != (tb->control & 3)) {
+            value = ptimer_get_count(tb->timer);
+            timerblock_run(tb, value, 1);
         }
         break;
     case 12: /* Interrupt status.  */
@@ -184,13 +172,12 @@ static const MemoryRegionOps timerblock_ops = {
 
 static void timerblock_reset(TimerBlock *tb)
 {
-    tb->count = 0;
     tb->load = 0;
     tb->control = 0;
     tb->status = 0;
-    tb->tick = 0;
     if (tb->timer) {
-        timer_del(tb->timer);
+        ptimer_stop(tb->timer);
+        ptimer_set_limit(tb->timer, 0, 1);
     }
 }
 
@@ -235,7 +222,8 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
      */
     for (i = 0; i < s->num_cpu; i++) {
         TimerBlock *tb = &s->timerblock[i];
-        tb->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, timerblock_tick, tb);
+        QEMUBH *bh = qemu_bh_new(timerblock_tick, tb);
+        tb->timer = ptimer_init(bh);
         sysbus_init_irq(sbd, &tb->irq);
         memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
                               "arm_mptimer_timerblock", 0x20);
@@ -245,26 +233,24 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
 
 static const VMStateDescription vmstate_timerblock = {
     .name = "arm_mptimer_timerblock",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(count, TimerBlock),
         VMSTATE_UINT32(load, TimerBlock),
         VMSTATE_UINT32(control, TimerBlock),
         VMSTATE_UINT32(status, TimerBlock),
-        VMSTATE_INT64(tick, TimerBlock),
-        VMSTATE_TIMER_PTR(timer, TimerBlock),
+        VMSTATE_PTIMER(timer, TimerBlock),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static const VMStateDescription vmstate_arm_mptimer = {
     .name = "arm_mptimer",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu,
-                                     2, vmstate_timerblock, TimerBlock),
+                                     3, vmstate_timerblock, TimerBlock),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/timer/arm_mptimer.h b/include/hw/timer/arm_mptimer.h
index b34cba0..93db61b 100644
--- a/include/hw/timer/arm_mptimer.h
+++ b/include/hw/timer/arm_mptimer.h
@@ -27,12 +27,10 @@
 
 /* State of a single timer or watchdog block */
 typedef struct {
-    uint32_t count;
     uint32_t load;
     uint32_t control;
     uint32_t status;
-    int64_t tick;
-    QEMUTimer *timer;
+    struct ptimer_state *timer;
     qemu_irq irq;
     MemoryRegion iomem;
 } TimerBlock;
-- 
2.6.1

Re: [Qemu-devel] [PATCH v7 0/2] PTimer fix and ARM MPTimer conversion

[Peter Maydell]

On 24 October 2015 at 13:21, Dmitry Osipenko <digetx@gmail.com> wrote:
> Changelog for ARM MPTimer QEMUTimer to ptimer conversion:
>
>     V2: Fixed changing periodic timer counter value "on the fly". I added a
>         test to the gist to cover that issue.
>
>     V3: Fixed starting the timer with load = 0 and counter != 0, added tests
>         to the gist for this issue. Changed vmstate version for all VMSD's,
>         since loadvm doesn't check version of nested VMSD.
>
>     V4: Fixed spurious IT bit set for the timer starting in the periodic mode
>         with counter = 0. Test added.
>
>     V5: Code cleanup, now depends on ptimer_set_limit() fix.
>
>     V6: No code change, added test to check ptimer_get_count() with corrected
>         .limit value.
>
>     V7: No change.
>
> ARM MPTimer tests: https://gist.github.com/digetx/dbd46109503b1a91941a

Hi. Given that we're now in the second half of softfreeze, and
the discussions about the design of the changes to ptimer don't
seem to have reached a conclusion yet, I would prefer to defer
this to 2.6. (It feels a bit late in the release cycle to make changes
to a core component like ptimer that affects multiple boards.)
My understanding is that although this patchset fixes a bunch of
edge cases and odd features in the mptimer, there aren't any bugs
that it fixes that cause problems for common guests like Linux, right?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v7 0/2] PTimer fix and ARM MPTimer conversion

[Dmitry Osipenko]

31.10.2015 00:52, Peter Maydell пишет:
> On 24 October 2015 at 13:21, Dmitry Osipenko <digetx@gmail.com> wrote:
>> Changelog for ARM MPTimer QEMUTimer to ptimer conversion:
>>
>>      V2: Fixed changing periodic timer counter value "on the fly". I added a
>>          test to the gist to cover that issue.
>>
>>      V3: Fixed starting the timer with load = 0 and counter != 0, added tests
>>          to the gist for this issue. Changed vmstate version for all VMSD's,
>>          since loadvm doesn't check version of nested VMSD.
>>
>>      V4: Fixed spurious IT bit set for the timer starting in the periodic mode
>>          with counter = 0. Test added.
>>
>>      V5: Code cleanup, now depends on ptimer_set_limit() fix.
>>
>>      V6: No code change, added test to check ptimer_get_count() with corrected
>>          .limit value.
>>
>>      V7: No change.
>>
>> ARM MPTimer tests: https://gist.github.com/digetx/dbd46109503b1a91941a
>
> Hi. Given that we're now in the second half of softfreeze, and
> the discussions about the design of the changes to ptimer don't
> seem to have reached a conclusion yet, I would prefer to defer
> this to 2.6. (It feels a bit late in the release cycle to make changes
> to a core component like ptimer that affects multiple boards.)

Sure, I have no objections.

> My understanding is that although this patchset fixes a bunch of
> edge cases and odd features in the mptimer, there aren't any bugs
> that it fixes that cause problems for common guests like Linux, right?
>

Yes, it should be alright.

-- 
Dmitry