Re: [Qemu-arm] [PATCH v13 6/8] hw/ptimer: Support running with counter = 0 by introducing new policy feature

[Dmitry Osipenko]

On 06.06.2016 13:09, Peter Maydell wrote:
> On 27 May 2016 at 18:03, Dmitry Osipenko <address@hidden> wrote:
> > Bump VMSD version since a new VMState member is added.
>
> I'm afraid you can't do that -- the ptimer code is used by a lot of
> platforms including some which do not permit migration compatibility
> breaks (at least sparc, for instance).
>
> If you want to add this feature you need to at least make it seamlessly
> support migration from the old version using a vmstate subsection.
>
> Better would be to make the policy a QOM property -- then it is
> constant for the lifetime of the timer and doesn't need to be migrated
> at all. (There's no reason to support timers which change policy at
> runtime, I hope). The default property value should be "same behaviour
> as previously".

QOM property should fit well, thanks for the suggestion.

> This whole change would be more solidly supported if it came with
> a clear description of the bug being fixed (ie what device is currently
> using the default-but-wrong behaviour and is going to be fixed by
> setting a different policy).

I'll extend the commit description, thanks for the note.

> > +    if (s->delta == 0 && s->policy == UNIMPLEMENTED) {
> > +        hw_error("ptimer: Running with counter=0 is unimplemented by " \
> > +                 "this timer, fix it!\n");
>
> hw_error() is effectively a verbose assert. Have you checked that
> all the users of ptimer do the right thing? If not, we can't
> assert() until we've fixed them all. Similarly below.

No, I haven't examined all the users thoroughly, will take look through them for 
this issue.

In my opinion it's better to know that your QEMU machine doesn't behave 
correctly and just fix it.

> Note that what we had previously was just a warning-and-continue

Yes, and it was literally "forever" there: "committed on 23 May 2007". Seems 
just nobody cared to fix/improve it since then.

I'll keep old "warning-and-continue" in this patch and derive "hw_error" change 
into a separate patch.

> > @@ -165,11 +170,8 @@ void ptimer_run(ptimer_state *s, int oneshot)
> >  {
> >      bool was_disabled = !s->enabled;
> >
> > -    if (was_disabled && s->period == 0) {
> > -        fprintf(stderr, "Timer with period zero, disabling\n");
> > -        return;
> > -    }
> >      s->enabled = oneshot ? 2 : 1;
> > +
>
> Stray extra blank line.

Ack.

> > +enum ptimer_policy {
> > +    UNIMPLEMENTED,
> > +    SET_CNT_TO_0_TRIGGERS_IRQ_AFTER_ONE_PERIOD,
>
> Given that enums aren't namespaced, "UNIMPLEMENTED" is too broad;
> you need some PTIMER_ prefixes. Also the second one of these is
> too long a name.

Ack.

> > +};
> > +
> >  /* ptimer.c */
> >  typedef struct ptimer_state ptimer_state;
> >  typedef void (*ptimer_cb)(void *opaque);
> > @@ -25,6 +30,7 @@ uint64_t ptimer_get_count(ptimer_state *s);
> >  void ptimer_set_count(ptimer_state *s, uint64_t count);
> >  void ptimer_run(ptimer_state *s, int oneshot);
> >  void ptimer_stop(ptimer_state *s);
> > +void ptimer_set_policy(ptimer_state *s, enum ptimer_policy policy);
>
> Documentation comment headers for new global functions, please.

Ack. Thanks a lot for the review!

--
Dmitry