[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v14 1/3] hw/ptimer: Support running with counter
From: |
Dmitry Osipenko |
Subject: |
Re: [Qemu-devel] [PATCH v14 1/3] hw/ptimer: Support running with counter = 0 by introducing new policy feature |
Date: |
Thu, 23 Jun 2016 19:32:24 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
On 23.06.2016 16:49, Peter Maydell wrote:
> On 17 June 2016 at 14:17, Dmitry Osipenko <address@hidden> wrote:
>> Currently ptimer prints error message and stops the running timer that has
>> delta (counter) = 0, this is an incorrect behaviour for some of the ptimer
>> users. There are different variants of how particular timer could handle that
>> case besides stopping the timer, like immediate or deferred IRQ trigger.
>> Introduce policy feature that provides ptimer with an information about the
>> correct behaviour.
>>
>> Implement the "counter = 0 triggers IRQ after one period" policy, as it is
>> known to be used by the ARM MPTimer and set "default" policy to all ptimer
>> users, maintaining old behaviour till they get fixed.
>
> Could you split this into:
> (1) a patch which just adds the new argument to ptimer_init() and
> updates all its callers
> (2) a patch which adds support for setting the policy option to
> something other than the default value
>
> and also make sure that we only do one change per patch -- there
> seem to be several different behaviour changes tangled up in
> one patch here.
>
> I think that will be easier to review.
>
This patch isn't supposed to change behaviour for any of the current timers. I
think it is clearly expressed in the last sentence of the commit message. There
is one unintended behaviour change in this patch, it's my overlook [see below].
>> Signed-off-by: Dmitry Osipenko <address@hidden>
>
>> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
>> index 05b0c27..289e23e 100644
>> --- a/hw/core/ptimer.c
>> +++ b/hw/core/ptimer.c
>> @@ -21,6 +21,7 @@ struct ptimer_state
>> int64_t period;
>> int64_t last_event;
>> int64_t next_event;
>> + uint8_t policy_mask;
>> QEMUBH *bh;
>> QEMUTimer *timer;
>> };
>> @@ -35,14 +36,15 @@ static void ptimer_trigger(ptimer_state *s)
>>
>> static void ptimer_reload(ptimer_state *s)
>> {
>> - uint32_t period_frac = s->period_frac;
>> + int64_t period_frac = s->period_frac;
>
> Why does this variable change type?
>
I removed the in-place type conversion further:
>> if (period_frac) {
>> - s->next_event += ((int64_t)period_frac * s->delta) >> 32;
>> + s->next_event += (period_frac * delta) >> 32;
>> }
>> timer_mod(s->timer, s->next_event);
>> }
>> @@ -73,27 +75,30 @@ static void ptimer_reload(ptimer_state *s)
>> static void ptimer_tick(void *opaque)
>> {
>> ptimer_state *s = (ptimer_state *)opaque;
>> - ptimer_trigger(s);
>> - s->delta = 0;
>> - if (s->enabled == 2) {
>> +
>> + s->delta = (s->enabled == 1) ? s->limit : 0;
>> +
>> + if (s->delta == 0) {
>
> This seems to be a change not guarded by a check of a policy bit?
>
That's a good question. In the current ARM MPTimer conversion patch, I assume
that delta = 0 would stop the timer regardless of it's mode and ptimer user
would itself re-arm timer in a tick() handler if needed. However, after your
question, I think it's a bit unintuitive and needs to be changed to continuous
*ptimer* trigger in periodic mode in case of the deferred trigger policy.
>> s->enabled = 0;
>> } else {
>> ptimer_reload(s);
>> }
>> +
>> + ptimer_trigger(s);
>> }
>>
>> uint64_t ptimer_get_count(ptimer_state *s)
>> {
>> uint64_t counter;
>>
>> - if (s->enabled) {
>> + if (s->enabled && s->delta != 0) {
>> int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> int64_t next = s->next_event;
>> bool expired = (now - next >= 0);
>> bool oneshot = (s->enabled == 2);
>>
>> /* Figure out the current counter value. */
>> - if (s->period == 0 || (expired && (oneshot || use_icount))) {
>> + if (expired && (oneshot || use_icount)) {
>> /* Prevent timer underflowing if it should already have
>> triggered. */
>> counter = 0;
>> @@ -165,10 +170,6 @@ 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;
>> - }
>
> If the default policy value was provided, we shouldn't change behaviour.
>
Right, now I see that ptimer_trigger() is missed in case of running timer with a
default policy and delta = 0. Thanks for the note, will fix it. I'll craft
QTests for the ptimer, shouldn't take a lot of effort.
>> s->enabled = oneshot ? 2 : 1;
>> if (was_disabled) {
>> s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> @@ -203,6 +204,7 @@ void ptimer_set_period(ptimer_state *s, int64_t period)
>> /* Set counter frequency in Hz. */
>> void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>> {
>> + g_assert(freq != 0 && freq <= 1000000000ll);
>
> This seems to be an unrelated change.
>
Agree :) I have hit it couple times during the work on the ARM MPTimer patch in
the past, decided that it may not worth a whole patch for a such simple
one-liner.
>> s->delta = ptimer_get_count(s);
>> s->period = 1000000000ll / freq;
>> s->period_frac = (1000000000ll << 32) / freq;
>> @@ -232,8 +234,8 @@ uint64_t ptimer_get_limit(ptimer_state *s)
>>
>> const VMStateDescription vmstate_ptimer = {
>> .name = "ptimer",
>> - .version_id = 1,
>> - .minimum_version_id = 1,
>> + .version_id = 2,
>> + .minimum_version_id = 2,
>
> Why are we bumping the version ID here?
>
Ooops, good catch!
Thanks for the review!
--
Dmitry