Re: [Qemu-devel] [PATCH] hw/timer: Add value matching support to aspeed_timer

[Peter Maydell]

On 10 June 2016 at 01:59, Andrew Jeffery <address@hidden> wrote:
> On Thu, 2016-06-09 at 19:15 +0100, Peter Maydell wrote:
>> On 27 May 2016 at 06:08, Andrew Jeffery <address@hidden> wrote:
>> >
>> > Value matching allows Linux to boot with CONFIG_NO_HZ_IDLE=y on the
>> > palmetto-bmc machine. Two match registers are provided for each timer.
>> >
>> > Signed-off-by: Andrew Jeffery <address@hidden>
>> > ---
>> >
>> > The change pulls out ptimer in favour of the regular timer infrastructure. 
>> > As a
>> > consequence it implements the conversions between ticks and time which 
>> > feels a
>> > little tedious. Any comments there would be appreciated.
>> Couple of minor comments below.
>>
>> >
>> >  hw/timer/aspeed_timer.c         | 135 
>> > ++++++++++++++++++++++++++++++----------
>> >  include/hw/timer/aspeed_timer.h |   6 +-
>> >  2 files changed, 105 insertions(+), 36 deletions(-)
>> >
>> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>> > index 4b94808821b4..905de0f788b2 100644
>> > --- a/hw/timer/aspeed_timer.c
>> > +++ b/hw/timer/aspeed_timer.c
>> > @@ -9,13 +9,12 @@
>> >   * the COPYING file in the top-level directory.
>> >   */
>> >
>> > +#include
>> >  #include "qemu/osdep.h"
>> osdep.h must always be the first include.
>
> Thanks for picking that up.

If you like you can run scripts/clean-includes file ...
and it will automatically make any necessary changes like this to
your files.


>> > +static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t 
>> > now_ns)
>> > +{
>> > +    uint64_t delta_ns = now_ns - MIN(now_ns, t->start);
>> > +    uint32_t ticks = (uint32_t) floor(delta_ns / calculate_period(t));
>> Do we really need to do this with floating point ?
>
> I went with floating point to avoid accumulating errors from truncation
> by integer division. At eg 24MHz truncation increases the tick rate by
> approximately 1 in 63. We're using floor() here, but that only
> truncates at the end of the calculation, so the fractional current
> tick.

Right, but you only have a risk of truncation because of the way
you've structured the calculation. You have

floor(delta_ns / calculate_period())
== floor(delta_ns / (calculate_rate() / NS_PER_SEC))
== floor((delta_ns * NS_PER_SEC) / calculate_rate())

and calculate_rate() returns either 1000000 or 24000000.

So you have a 64 bit delta_ns, a 32 bit NANOSECONDS_PER_SECOND
and a 32 bit frequency. We have a function for doing this
accurately with integer arithmetic: muldiv64() (which takes
a 64 bit a, 32 bit b and 32 bit c and returns (a*b)/c using
an intermediate 96 bit precision to avoid overflow).

Doing ticks-to-ns and ns-to-ticks with muldiv64() is pretty
standard (grep the codebase and you'll see a lot of it).

> Having said that, grep'ing around under {,include/}hw/ doesn't show a
> lot of use of floating point. It looks like we are explicitly avoiding
> it, however with a quick search I didn't find it mentioned in any of
> the docs. What's the reasoning? Should I use a fixed-point approach
> like ptimer?

My view is that hardware doesn't generally use floating point
so it's odd to see it in a software model of hardware. Double
doesn't actually get you any more bits than uint64_t anyway...

>> > +    return calculate_next(t);

>> Why is this recursing ?
>
> The common case is that we return during iterating over seq array i.e.
> we're anticipating another match event (either from the match values or
> the timer reaching zero). If not then we've overrun, so we reinitialise
> the timer by resetting the 'start' reference point then searching again
> for the next event (iterating over seq). As the search for the next
> event is encoded in the function, I've recursed to reuse it.
>
> Would you prefer a loop here?
>
> Considering the two approaches (recursion vs loop), if TCO doesn't
> apply we could blow the stack, and with loops or TCO it's possible we
> could spin here indefinitely if the timer period is shorter than the
> time it takes to recalculate. Arguably, not crashing is better so I'm
> happy to rework it.

Yes, I'd prefer a loop -- TCO is an optimization, not a guarantee
by the compiler.

> As an aside, the approach for reinitialising start skips countdown
> periods that were completely missed through high service latency, and
> there will be no interrupts issued for the missing events. Is that
> reasonable?

If the countdown period is so short that we can't service it
in time then the system is probably not going to be functional
anyway, so I don't think it matters very much. (In ptimer we
impose an artificial limit on the timeout rate for a periodic timer
and just refuse to fire any faster than that: see ptimer_reload().)

thanks
-- PMM