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

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5568 bytes --]

On Fri, 2016-06-10 at 11:42 +0100, Peter Maydell wrote:
> On 10 June 2016 at 01:59, Andrew Jeffery <andrew@aj.id.au> wrote:
> > 
> > On Thu, 2016-06-09 at 19:15 +0100, Peter Maydell wrote:
> > > 
> > > On 27 May 2016 at 06:08, Andrew Jeffery <andrew@aj.id.au> 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 <andrew@aj.id.au>
> > > > ---
> > > > 
> > > > 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.

Thanks, I'll add that to my submission checklist

> 
> 
> > 
> > > 
> > > > 
> > > > +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).

Right! As I didn't see a concern with floating point prior to sending
the patch I didn't try to rearrange the calculation. I'll rework to use
muldiv64().

> 
> > 
> > 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.

Fair enough.

>  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.

Yes, on reflection I agree. I'll rework it.

> 
> > 
> > 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().)

I'll leave it as is for v2.

Thanks for the feedback!

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]