[RFC qemu legoater/aspeed-3.1 4/4] timer: aspeed: Provide back-pressure information for short periods

[Andrew Jeffery]

On Fri, 11 Jan 2019, at 20:40, Cédric Le Goater wrote:
> On 1/11/19 4:56 AM, Andrew Jeffery wrote:
> > First up: This is not the way the hardware behaves.
> 
> I think this is OK from the QEMU side. It would be better if Linux was 
> not involved though.

It doesn't have to be, just it may see wonky timer behaviour because
the timer will not be doing exactly what was asked.

> 
> > However, it helps resolve real-world problems with short periods being
> > used under Linux. Commit 4451d3f59f2a ("clocksource/drivers/fttmr010:
> > Fix set_next_event handler") in Linux fixed the timer driver to
> > correctly schedule the next event for the Aspeed controller, and in
> > combination with 5daa8212c08e ("ARM: dts: aspeed: Describe random number
> > device") Linux will now set a timer with a period as low as 1us.
> > 
> > Configuring a qemu timer with such a short period results in spending
> > time handling the interrupt in the model rather than executing guest
> > code, leading to noticeable "sticky" behaviour in the guest.
> > 
> > The behaviour of Linux is correct with respect to the hardware, so we
> > need to improve our handling under emulation. The approach chosen is to
> > provide back-pressure information by calculating an acceptable minimum
> > number of ticks to be set on the model. Under Linux an additional read
> > is added in the timer configuration path to detect back-pressure, which
> > will never occur on hardware. However if back-pressure is observed, the
> > driver alerts the clock event subsystem, which then performs its own
> > next event dilation via a config option - d1748302f70b ("clockevents:
> > Make minimum delay adjustments configurable")
> 
> So, this is approach is not totally unacceptable, QEMU is an hypervisor
> which has its own timing constraints.
>  
> > A minimum period of 5us was experimentally determined on a Lenovo
> > T480s, which I've increased to 20us for "safety".
> > 
> > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > ---
> >  hw/misc/aspeed_scu.c            | 6 ++++++
> >  hw/timer/aspeed_timer.c         | 6 +++++-
> >  include/hw/timer/aspeed_timer.h | 1 +
> >  3 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> > index 257f9a6c6b8a..0410776b456a 100644
> > --- a/hw/misc/aspeed_scu.c
> > +++ b/hw/misc/aspeed_scu.c
> > @@ -432,6 +432,12 @@ static void aspeed_scu_realize(DeviceState *dev, Error **errp)
> >                            TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE);
> >  
> >      sysbus_init_mmio(sbd, &s->iomem);
> > +
> > +    /*
> > +     * Reset on realize to ensure the APB clock value is calculated in time for
> > +     * use by the timer model, which is reset before the SCU.
> > +     */
> > +    aspeed_scu_reset(dev);
> 
> sigh. May be we should call aspeed_scu_set_apb_freq() from the Aspeed timer
> model ?

I dunno - will we run into this in other models as well? If we do, should they also call
aspeed_scu_set_apb_freq()? If they do it probably won't matter (same input, same
output), but it seems a bit messy to distribute the call through the code?

> 
> >  }
> >  
> >  static const VMStateDescription vmstate_aspeed_scu = {
> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> > index 35b40a7c4010..0f3501ac5a5c 100644
> > --- a/hw/timer/aspeed_timer.c
> > +++ b/hw/timer/aspeed_timer.c
> > @@ -254,7 +254,7 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
> >      switch (reg) {
> >      case TIMER_REG_RELOAD:
> >          old_reload = t->reload;
> > -        t->reload = value;
> > +        t->reload = value < t->min_ticks ? t->min_ticks : value;
> >  
> >          /* If the reload value was not previously set, or zero, and
> >           * the current value is valid, try to start the timer if it is
> > @@ -306,7 +306,11 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
> >  
> >  static void aspeed_timer_ctrl_external_clock(AspeedTimer *t, bool enable)
> >  {
> > +    AspeedTimerCtrlState *s = timer_to_ctrl(t);
> > +    uint32_t rate = enable ? TIMER_CLOCK_EXT_HZ : s->scu->apb_freq;
> > +
> >      trace_aspeed_timer_ctrl_external_clock(t->id, enable);
> > +    t->min_ticks = muldiv64(20 * SCALE_US, rate, NANOSECONDS_PER_SECOND);
> 
> That '20' deserves a define I think.

Yes, good call :)

Andrew