[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH v13 6/8] hw/ptimer: Support running with counter =

From: Dmitry Osipenko
Subject: Re: [Qemu-arm] [PATCH v13 6/8] hw/ptimer: Support running with counter = 0 by introducing new policy feature
Date: Mon, 6 Jun 2016 20:24:37 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0

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.


+enum ptimer_policy {

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.


 /* 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!


reply via email to

[Prev in Thread] Current Thread [Next in Thread]