[v10,5/7] hw/ptimer: Legalize running with delta = load = 0

Currently ptimer would print error message and clear enable flag for an
arming timer that has delta = load = 0. That actually could be a valid case
for some hardware, like instant IRQ trigger for oneshot timer or continuous
in periodic mode. Support those cases by printing error message only when
period = 0.

In addition, don't load one-shot timer when delta = 0 and actually stop the
timer by timer_del().

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

On Sat, Jan 09, 2016 at 08:39:53PM +0300, Dmitry Osipenko wrote:
> Currently ptimer would print error message and clear enable flag for an
> arming timer that has delta = load = 0. That actually could be a valid case
> for some hardware, like instant IRQ trigger for oneshot timer or continuous
> in periodic mode. Support those cases by printing error message only when
> period = 0.
> 

Isn't the continuous-periodic the same as period = 0, so if we were to really
support this, there should be no error message. This would simplify as we
can remove the conditionals of 0 period completely and rely only on the
too-fast clamps you add in previous patches.

Regards,
Peter

> In addition, don't load one-shot timer when delta = 0 and actually stop the
> timer by timer_del().
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/core/ptimer.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 6960738..42e44f9 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -36,13 +36,20 @@ static void ptimer_reload(ptimer_state *s)
>  {
>      uint32_t period_frac = s->period_frac;
>      uint64_t period = s->period;
> +    int periodic = (s->enabled == 1);
>  
> -    if (s->delta == 0) {
> +    if (s->delta == 0 && period != 0) {
>          ptimer_trigger(s);
> -        s->delta = s->limit;
> +        if (periodic) {
> +            s->delta = s->limit;
> +        }
>      }
> -    if (s->delta == 0 || s->period == 0) {
> -        fprintf(stderr, "Timer with period zero, disabling\n");
> +    if (s->delta == 0 || period == 0) {
> +        if (period == 0) {
> +            fprintf(stderr, "Timer with period zero, disabling\n");
> +            s->delta = 0;
> +        }
> +        timer_del(s->timer);
>          s->enabled = 0;
>          return;
>      }
> @@ -56,7 +63,7 @@ static void ptimer_reload(ptimer_state *s)
>       * on the current generation of host machines.
>       */
>  
> -    if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
> +    if (periodic && !use_icount && (s->delta * period < 10000)) {
>          period = 10000 / s->delta;
>          period_frac = 0;
>      }
> @@ -86,14 +93,14 @@ uint64_t ptimer_get_count(ptimer_state *s)
>      int enabled = s->enabled;
>      uint64_t counter;
>  
> -    if (enabled) {
> +    if (enabled && s->delta != 0) {
>          int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>          int64_t next = s->next_event;
>          int expired = (now - next >= 0);
>          int oneshot = (enabled == 2);
>  
>          /* Figure out the current counter value.  */
> -        if (s->period == 0 || (expired && (use_icount || oneshot))) {
> +        if (expired && (use_icount || oneshot)) {
>              /* Prevent timer underflowing if it should already have
>                 triggered.  */
>              counter = 0;
> -- 
> 2.6.4
>

12.01.2016 06:58, Peter Crosthwaite пишет:
> On Sat, Jan 09, 2016 at 08:39:53PM +0300, Dmitry Osipenko wrote:
>> Currently ptimer would print error message and clear enable flag for an
>> arming timer that has delta = load = 0. That actually could be a valid case
>> for some hardware, like instant IRQ trigger for oneshot timer or continuous
>> in periodic mode. Support those cases by printing error message only when
>> period = 0.
>>
>
> Isn't the continuous-periodic the same as period = 0, so if we were to really
> support this, there should be no error message. This would simplify as we
> can remove the conditionals of 0 period completely and rely only on the
> too-fast clamps you add in previous patches.
>

I don't think that clamping is needed. Instead doing the ptimer_tick might be 
necessary, so ptimer user could handle that case on it own.

BTW, that printf isn't quite reliable, hw_error or other way of execution abort 
should been used instead.

Thanks for the comment.

12.01.2016 21:12, Dmitry Osipenko пишет:
> 12.01.2016 06:58, Peter Crosthwaite пишет:
>> On Sat, Jan 09, 2016 at 08:39:53PM +0300, Dmitry Osipenko wrote:
>>> Currently ptimer would print error message and clear enable flag for an
>>> arming timer that has delta = load = 0. That actually could be a valid case
>>> for some hardware, like instant IRQ trigger for oneshot timer or continuous
>>> in periodic mode. Support those cases by printing error message only when
>>> period = 0.
>>>
>>
>> Isn't the continuous-periodic the same as period = 0, so if we were to really
>> support this, there should be no error message. This would simplify as we
>> can remove the conditionals of 0 period completely and rely only on the
>> too-fast clamps you add in previous patches.
>>
>
> I don't think that clamping is needed. Instead doing the ptimer_tick might be
> necessary, so ptimer user could handle that case on it own.
>
> BTW, that printf isn't quite reliable, hw_error or other way of execution abort
> should been used instead.
>
> Thanks for the comment.
>

Looking more at it, I think we should keep period = 0 forbidden. So it's treated 
as undefined behaviour and ptimer user should take care of it. If we'd want to 
handle period = 0 within ptimer, then we should also handle freq = 0, which 
implies potential ptimer VMSD version bump. Also it is uncertain what default 
behaviour should be chosen for period = 0, my guess is that pausing 
(_not_disabling_) the timer (like in freq = 0 case) should be more common among 
various hardware. If you have any thoughts on it, please let me know.

12.01.2016 21:12, Dmitry Osipenko пишет:
> 12.01.2016 06:58, Peter Crosthwaite пишет:
>> On Sat, Jan 09, 2016 at 08:39:53PM +0300, Dmitry Osipenko wrote:
>>> Currently ptimer would print error message and clear enable flag for an
>>> arming timer that has delta = load = 0. That actually could be a valid case
>>> for some hardware, like instant IRQ trigger for oneshot timer or continuous
>>> in periodic mode. Support those cases by printing error message only when
>>> period = 0.
>>>
>>
>> Isn't the continuous-periodic the same as period = 0, so if we were to really
>> support this, there should be no error message. This would simplify as we
>> can remove the conditionals of 0 period completely and rely only on the
>> too-fast clamps you add in previous patches.
>>
>
> I don't think that clamping is needed. Instead doing the ptimer_tick might be
> necessary, so ptimer user could handle that case on it own.
>
> BTW, that printf isn't quite reliable, hw_error or other way of execution abort
> should been used instead.
>
> Thanks for the comment.
>

Just tried with the clamping. It works, but what clamping should be chosen for 
icount mode? delta * period = 1ns is damn slow.

However, I'm still not sure what benefit has clamping over unclearable IRQ... 
only disadvantages in form of slowdown in icount mode, +2 lines of ptimer code 
and possible misbehaviour of blazing fast host machine in non-icount mode.