Re: [PATCH qemu.git v2 9/9] hw/timer/imx_epit: fix compare timer handling

[Axel Heider]

Peter,

> If you're correcting behaviour of the timer use here,
> you should start by fixing the way the timers are currently
> created with PTIMER_POLICY_LEGACY. That setting is basically
> "bug-for-bug-compatibility with very old QEMU, for devices
> where nobody really knows what the hardware behaviour should
> be". Where we do know what the hardware's supposed to do and
> we have some way of testing we're not breaking guest code,
> the right thing is to set the correct policy flags for
> the desired behaviour. These are documented in a comment
> near the top of include/hw/ptimer.h.

I would prefer to postpone changing PTIMER_POLICY_LEGACY to a
separate patchset, which is on top of the current one, as this
seems not to be an issue at the moment. Fixing the general isses
on access and ensure the flags are correct seem more pressing,
and this seem unrelated to the timer policy.


> It is modestly harmful because the sequence
>     counter = ptimer_get_count(s->timer_reload);
>     ...
>     ptimer_set_count(s->timer_cmp, counter);
>
> will cause the counter to lose or gain time. This happens because
> when you call "get" the ptimer code will look at the current exact
> time in nanoseconds and tell you the counter value at that point.
> That is probably somewhere in the middle of a timer-clock period
> (which runs at whatever frequency you tell the ptimer to use):
> for argument's sake, suppose the timer-clock counts every 1000ns.
> Suppose at the point of the 'get' the next tick will be in 300ns time.
> When you do a "set" that is assumed to be the result of a guest
> register write of some kind, and will effectively start a new
> timer-clock period. This means the next tick will not be for
> a full 1000ns, and we just lost 300ns (or gained 700ns perhaps).
> So it's better to avoid this kind of "get-and-then-set" code.

I see you point. The "get-and-then-set" was already in the code, I
did not really change this. I have tried to find a better way to
implement this, but could not come up with something so far. Any
suggestions here that is non trivial? Othereise I would prefer to
look into this in a new patch-set, together with replacing the
PTIMER_POLICY_LEGACY.


Axel