[RFC qemu legoater/aspeed-3.1 0/4] Handle short timer periods

[RFC qemu legoater/aspeed-3.1 0/4] Handle short timer periods

[Andrew Jeffery]

Hello,

We've had an issue for a while, since the introduction of 4451d3f59f2a
("clocksource/drivers/fttmr010: Fix set_next_event handler") in Linux, where
our qemu instances have a "sticky" behaviour. The VM becomes unresponsive at
unexpected times for unpredicable but often long periods of time.

This series, along with the associated patch to Linux's fttmr010 driver[0],
aims to resolve it.

[0] http://patchwork.ozlabs.org/patch/1023363/

The series an RFC series because it doesn't fix all the cases, just
demonstrates a potential way forward. The approach is to provide back-pressure
- to test if the reload value is set to a value that's smaller than some
experimentally determined value (in this case, 20us), and if so, configure a
period of at least 20us. The MAX() of the requested and minimum values is then
set in the reload register rather than the requested value, which can then be
read back by Linux. The fttmr010 driver, upon observing the greater value,
starts pushing back on the clock event subsystem, which then iteratively
determines a minimum acceptible event period before proceeding with the boot
process.

The implementation does not take care of the match register cases, but at the
moment they are unused by Linux on Aspeed SoCs. The match register case is not
taken care of because I'm not sure if this implementation is what we want to
use going forward, or whether we want to do something that's completely hidden
in the qemu model. However taking advantage of Linux's existing support for
determining minimum timer periods seemed like easy solution.

The stickiness turns out to be a consequence of Linux configuring a very short
timer period (1us, mostly due to the timerio_rng driver), which causes qemu to
spend a large chunk of its timeslice handling the host timer interrupt rather
than executing guest code.

Please critique!

Andrew

Andrew Jeffery (4):
  timer: aspeed: Fire interrupt on failure to meet deadline
  timer: aspeed: Status register contains reload for stopped timer
  timer: aspeed: Fix match calculations
  timer: aspeed: Provide back-pressure information for short periods

 hw/misc/aspeed_scu.c            |  6 ++++++
 hw/timer/aspeed_timer.c         | 37 ++++++++++++++++++++++++++-------
 include/hw/timer/aspeed_timer.h |  1 +
 3 files changed, 36 insertions(+), 8 deletions(-)

-- 
2.19.1

[RFC qemu legoater/aspeed-3.1 1/4] timer: aspeed: Fire interrupt on failure to meet deadline

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/timer/aspeed_timer.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 1a54d85e9d49..54c75bf4f322 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -128,10 +128,16 @@ static uint64_t calculate_next(struct AspeedTimer *t)
     if (now < next)
         return next;
 
-    /* We've missed all of the deadlines. Set a timer in the future to let
-     * execution catch up */
-    t->start = now;
-    return next + 10000;
+    /* We've missed all deadlines, fire interrupt and try again */
+    timer_del(&t->timer);
+
+    if (timer_overflow_interrupt(t)) {
+        t->level = !t->level;
+        qemu_set_irq(t->irq, t->level);
+    }
+
+    t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
 }
 
 static void aspeed_timer_mod(AspeedTimer *t)
-- 
2.19.1

Re: [RFC qemu legoater/aspeed-3.1 1/4] timer: aspeed: Fire interrupt on failure to meet deadline

[Cédric Le Goater]

On 1/11/19 4:56 AM, Andrew Jeffery wrote:
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  hw/timer/aspeed_timer.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 1a54d85e9d49..54c75bf4f322 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -128,10 +128,16 @@ static uint64_t calculate_next(struct AspeedTimer *t)
>      if (now < next)
>          return next;
>  
> -    /* We've missed all of the deadlines. Set a timer in the future to let
> -     * execution catch up */
> -    t->start = now;
> -    return next + 10000;
> +    /* We've missed all deadlines, fire interrupt and try again */
> +    timer_del(&t->timer);
> +
> +    if (timer_overflow_interrupt(t)) {
> +        t->level = !t->level;
> +        qemu_set_irq(t->irq, t->level);
> +    }
> +
> +    t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));

MAX(MAX()) ? 

Is that necessary ?

C.

Re: [RFC qemu legoater/aspeed-3.1 1/4] timer: aspeed: Fire interrupt on failure to meet deadline

[Andrew Jeffery]

On Fri, 11 Jan 2019, at 19:14, Cédric Le Goater wrote:
> On 1/11/19 4:56 AM, Andrew Jeffery wrote:
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  hw/timer/aspeed_timer.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> > index 1a54d85e9d49..54c75bf4f322 100644
> > --- a/hw/timer/aspeed_timer.c
> > +++ b/hw/timer/aspeed_timer.c
> > @@ -128,10 +128,16 @@ static uint64_t calculate_next(struct AspeedTimer *t)
> >      if (now < next)
> >          return next;
> >  
> > -    /* We've missed all of the deadlines. Set a timer in the future to let
> > -     * execution catch up */
> > -    t->start = now;
> > -    return next + 10000;
> > +    /* We've missed all deadlines, fire interrupt and try again */
> > +    timer_del(&t->timer);
> > +
> > +    if (timer_overflow_interrupt(t)) {
> > +        t->level = !t->level;
> > +        qemu_set_irq(t->irq, t->level);
> > +    }
> > +
> > +    t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +    return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
> 
> MAX(MAX()) ? 
> 
> Is that necessary ?

No, just my wip internal reasoning getting exposed externally :)

> 
> C.

[RFC qemu legoater/aspeed-3.1 2/4] timer: aspeed: Status register contains reload for stopped timer

[Andrew Jeffery]

From the datasheet:

> This register stores the current status of counter #N. When timer
> enable bit TMC30[N * b] is disabled, the reload register will be
> loaded into this counter. When timer bit TMC30[N * b] is set, the
> counter will start to decrement. CPU can update this register value
> when enable bit is set.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/timer/aspeed_timer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 54c75bf4f322..6adc14d62034 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -182,7 +182,11 @@ static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
 
     switch (reg) {
     case TIMER_REG_STATUS:
-        value = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        if (timer_enabled(t)) {
+            value = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        } else {
+            value = t->reload;
+        }
         break;
     case TIMER_REG_RELOAD:
         value = t->reload;
-- 
2.19.1

Re: [RFC qemu legoater/aspeed-3.1 2/4] timer: aspeed: Status register contains reload for stopped timer

[Cédric Le Goater]

On 1/11/19 4:56 AM, Andrew Jeffery wrote:
> From the datasheet:
> 
>> This register stores the current status of counter #N. When timer
>> enable bit TMC30[N * b] is disabled, the reload register will be
>> loaded into this counter. When timer bit TMC30[N * b] is set, the
>> counter will start to decrement. CPU can update this register value
>> when enable bit is set.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/timer/aspeed_timer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 54c75bf4f322..6adc14d62034 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -182,7 +182,11 @@ static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
>  
>      switch (reg) {
>      case TIMER_REG_STATUS:
> -        value = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> +        if (timer_enabled(t)) {
> +            value = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> +        } else {
> +            value = t->reload;
> +        }
>          break;
>      case TIMER_REG_RELOAD:
>          value = t->reload;
> 

[RFC qemu legoater/aspeed-3.1 3/4] timer: aspeed: Fix match calculations

[Andrew Jeffery]

If the match value exceeds reload then we don't want to include it in
calculations for the next event.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/timer/aspeed_timer.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 6adc14d62034..35b40a7c4010 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -107,6 +107,11 @@ static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks)
     return t->start + delta_ns;
 }
 
+static inline uint32_t calculate_match(struct AspeedTimer *t, int i)
+{
+    return t->match[i] < t->reload ? t->match[i] : 0;
+}
+
 static uint64_t calculate_next(struct AspeedTimer *t)
 {
     uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -116,11 +121,11 @@ static uint64_t calculate_next(struct AspeedTimer *t)
      * registers, so sort using MAX/MIN/zero. We sort in that order as the
      * timer counts down to zero. */
 
-    next = calculate_time(t, MAX(t->match[0], t->match[1]));
+    next = calculate_time(t, MAX(calculate_match(t, 0), calculate_match(t, 1)));
     if (now < next)
         return next;
 
-    next = calculate_time(t, MIN(t->match[0], t->match[1]));
+    next = calculate_time(t, MIN(calculate_match(t, 0), calculate_match(t, 1)));
     if (now < next)
         return next;
 
@@ -136,8 +141,10 @@ static uint64_t calculate_next(struct AspeedTimer *t)
         qemu_set_irq(t->irq, t->level);
     }
 
+    next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
     t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
+
+    return calculate_time(t, next);
 }
 
 static void aspeed_timer_mod(AspeedTimer *t)
-- 
2.19.1

Re: [RFC qemu legoater/aspeed-3.1 3/4] timer: aspeed: Fix match calculations

[Cédric Le Goater]

On 1/11/19 4:56 AM, Andrew Jeffery wrote:
> If the match value exceeds reload then we don't want to include it in
> calculations for the next event.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Looks correct apart from the MAX(MAX())

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/timer/aspeed_timer.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 6adc14d62034..35b40a7c4010 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -107,6 +107,11 @@ static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks)
>      return t->start + delta_ns;
>  }
>  
> +static inline uint32_t calculate_match(struct AspeedTimer *t, int i)
> +{
> +    return t->match[i] < t->reload ? t->match[i] : 0;
> +}
> +
>  static uint64_t calculate_next(struct AspeedTimer *t)
>  {
>      uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> @@ -116,11 +121,11 @@ static uint64_t calculate_next(struct AspeedTimer *t)
>       * registers, so sort using MAX/MIN/zero. We sort in that order as the
>       * timer counts down to zero. */
>  
> -    next = calculate_time(t, MAX(t->match[0], t->match[1]));
> +    next = calculate_time(t, MAX(calculate_match(t, 0), calculate_match(t, 1)));
>      if (now < next)
>          return next;
>  
> -    next = calculate_time(t, MIN(t->match[0], t->match[1]));
> +    next = calculate_time(t, MIN(calculate_match(t, 0), calculate_match(t, 1)));
>      if (now < next)
>          return next;
>  
> @@ -136,8 +141,10 @@ static uint64_t calculate_next(struct AspeedTimer *t)
>          qemu_set_irq(t->irq, t->level);
>      }
>  
> +    next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
>      t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -    return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
> +
> +    return calculate_time(t, next);
>  }
>  
>  static void aspeed_timer_mod(AspeedTimer *t)
> 

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

[Andrew Jeffery]

First up: This is not the way the hardware behaves.

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")

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@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);
 }
 
 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);
 }
 
 static void aspeed_timer_ctrl_overflow_interrupt(AspeedTimer *t, bool enable)
diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
index 1fb949e16710..10c851ebb6d7 100644
--- a/include/hw/timer/aspeed_timer.h
+++ b/include/hw/timer/aspeed_timer.h
@@ -41,6 +41,7 @@ typedef struct AspeedTimer {
      * interrupts, signalling with both the rising and falling edge.
      */
     int32_t level;
+    uint32_t min_ticks;
     uint32_t reload;
     uint32_t match[2];
     uint64_t start;
-- 
2.19.1

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

[Cédric Le Goater]

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.

> 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@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 ?

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

C.

>  }
>  
>  static void aspeed_timer_ctrl_overflow_interrupt(AspeedTimer *t, bool enable)
> diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
> index 1fb949e16710..10c851ebb6d7 100644
> --- a/include/hw/timer/aspeed_timer.h
> +++ b/include/hw/timer/aspeed_timer.h
> @@ -41,6 +41,7 @@ typedef struct AspeedTimer {
>       * interrupts, signalling with both the rising and falling edge.
>       */
>      int32_t level;
> +    uint32_t min_ticks;
>      uint32_t reload;
>      uint32_t match[2];
>      uint64_t start;
> 

Re: [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@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

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

[Cédric Le Goater]

On 1/14/19 12:07 AM, Andrew Jeffery wrote:
> 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@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?

This is really a reset order problem. SCU reset should be called before all 
others models. I don't know if we can order the calls but calling reset in 
a realize routine is a problem also. 

The APB clock depends on the HPLL_PARAM register and on the CLK_SEL register,
and these can be set a runtime by FW.

May be we can introduce a reset method at the machine level forcing SCU
first. That  would be one way to ask how it should be done.

C.



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

Re: [RFC qemu legoater/aspeed-3.1 0/4] Handle short timer periods

[Cédric Le Goater]

Hello Andrew,

On 1/11/19 4:56 AM, Andrew Jeffery wrote:
> Hello,
> 
> We've had an issue for a while, since the introduction of 4451d3f59f2a
> ("clocksource/drivers/fttmr010: Fix set_next_event handler") in Linux, where
> our qemu instances have a "sticky" behaviour. The VM becomes unresponsive at
> unexpected times for unpredicable but often long periods of time.
> 
> This series, along with the associated patch to Linux's fttmr010 driver[0],
> aims to resolve it.
> 
> [0] http://patchwork.ozlabs.org/patch/1023363/

I gave the whole a try and on a x86 host, QEMU reaches :

[    8.282333] systemd[1]: Set hostname to <romulus>.

on a ppc64el :

[   11.497910] systemd[1]: Set hostname to <romulus>.

which is much better than before.

As the QEMU patchset does not seem to impact support for older images, 
I have updated the aspeed-3.1 branch and also created a new branch for 
dev : aspeed-4.0.

> The series an RFC series because it doesn't fix all the cases, just
> demonstrates a potential way forward. The approach is to provide back-pressure
> - to test if the reload value is set to a value that's smaller than some
> experimentally determined value (in this case, 20us), and if so, configure a
> period of at least 20us. The MAX() of the requested and minimum values is then
> set in the reload register rather than the requested value, which can then be
> read back by Linux. The fttmr010 driver, upon observing the greater value,
> starts pushing back on the clock event subsystem, which then iteratively
> determines a minimum acceptible event period before proceeding with the boot
> process.
> 
> The implementation does not take care of the match register cases, but at the
> moment they are unused by Linux on Aspeed SoCs. The match register case is not
> taken care of because I'm not sure if this implementation is what we want to
> use going forward, or whether we want to do something that's completely hidden
> in the qemu model. However taking advantage of Linux's existing support for
> determining minimum timer periods seemed like easy solution.
> 
> The stickiness turns out to be a consequence of Linux configuring a very short
> timer period (1us, mostly due to the timerio_rng driver), which causes qemu to
> spend a large chunk of its timeslice handling the host timer interrupt rather
> than executing guest code.
> 
> Please critique!

We are adding another QEMU HW tweak in Linux. We need feedback from the
Linux maintainers I would say.

Thanks,

C. 
 
> 
> Andrew
> 
> Andrew Jeffery (4):
>   timer: aspeed: Fire interrupt on failure to meet deadline
>   timer: aspeed: Status register contains reload for stopped timer
>   timer: aspeed: Fix match calculations
>   timer: aspeed: Provide back-pressure information for short periods
> 
>  hw/misc/aspeed_scu.c            |  6 ++++++
>  hw/timer/aspeed_timer.c         | 37 ++++++++++++++++++++++++++-------
>  include/hw/timer/aspeed_timer.h |  1 +
>  3 files changed, 36 insertions(+), 8 deletions(-)
> 

Re: [RFC qemu legoater/aspeed-3.1 0/4] Handle short timer periods

[Andrew Jeffery]

On Fri, 11 Jan 2019, at 20:44, Cédric Le Goater wrote:
> Hello Andrew,
> 
> On 1/11/19 4:56 AM, Andrew Jeffery wrote:
> > Hello,
> > 
> > We've had an issue for a while, since the introduction of 4451d3f59f2a
> > ("clocksource/drivers/fttmr010: Fix set_next_event handler") in Linux, where
> > our qemu instances have a "sticky" behaviour. The VM becomes unresponsive at
> > unexpected times for unpredicable but often long periods of time.
> > 
> > This series, along with the associated patch to Linux's fttmr010 driver[0],
> > aims to resolve it.
> > 
> > [0] http://patchwork.ozlabs.org/patch/1023363/
> 
> I gave the whole a try and on a x86 host, QEMU reaches :
> 
> [    8.282333] systemd[1]: Set hostname to <romulus>.
> 
> on a ppc64el :
> 
> [   11.497910] systemd[1]: Set hostname to <romulus>.
> 
> which is much better than before.
> 
> As the QEMU patchset does not seem to impact support for older images, 
> I have updated the aspeed-3.1 branch and also created a new branch for 
> dev : aspeed-4.0.
> 
> > The series an RFC series because it doesn't fix all the cases, just
> > demonstrates a potential way forward. The approach is to provide back-pressure
> > - to test if the reload value is set to a value that's smaller than some
> > experimentally determined value (in this case, 20us), and if so, configure a
> > period of at least 20us. The MAX() of the requested and minimum values is then
> > set in the reload register rather than the requested value, which can then be
> > read back by Linux. The fttmr010 driver, upon observing the greater value,
> > starts pushing back on the clock event subsystem, which then iteratively
> > determines a minimum acceptible event period before proceeding with the boot
> > process.
> > 
> > The implementation does not take care of the match register cases, but at the
> > moment they are unused by Linux on Aspeed SoCs. The match register case is not
> > taken care of because I'm not sure if this implementation is what we want to
> > use going forward, or whether we want to do something that's completely hidden
> > in the qemu model. However taking advantage of Linux's existing support for
> > determining minimum timer periods seemed like easy solution.

Do you mind helping me hash out the full behaviour?

The key requirement is that any modelled timer period not be shorter than e.g. the
20us I picked. So there are a number cases:

MIN_PERIOD = ticks(20us)

1. RELOAD < MIN_PERIOD
2. RELOAD >= (2 * MIN_PERIOD), MATCH{1,2} < MIN_PERIOD
3. RELOAD >= (2 * MIN_PERIOD), (RELOAD - (RELOAD - MATCH{1,2})) < MIN_PERIOD
4. MIN_PERIOD <= RELOAD < (2 * MIN_PERIOD), MATCH{1,2} < RELOAD
5. RELOAD > (2 * MIN_PERIOD), MIN_PERIOD < MATCH1 <= (RELOAD - MIN_PERIOD), 0 <= (MATCH1 - MATCH2) < MIN_PERIOD
6. RELOAD > (2 * MIN_PERIOD), 0 <= (MATCH2 - MATCH1) < MIN_PERIOD, MIN_PERIOD < MATCH2 <= (RELOAD - MIN_PERIOD)
7. RELOAD < (3 * MIN_PERIOD), 0 < MATCH1 < RELOAD, 0 < MATCH2 < RELOAD

The provided patches only solve case 1 (with the assumption that the match
registers contain an irrelevant value).

For the rest, I think we need to implement the following:

Each time RELOAD, MATCH1, or MATCH2 are modified, check the constraints
above, and expand all values as necessary to accommodate the minimum period
constraint of 20us. With the constraint of "not-before" on a timer event,
conceptually the algorithm looks like the following for the most general case of
two valid match register values:

* Define registers RELOAD, MATCH1, MATCH2

* Sort the values of RELOAD, MATCH1 and MATCH2 such that we have values
R > X > Y > 0 where R maps to RELOAD, and X and Y to MATCH1 and MATCH2 as
appropriate

* Set dY = MAX(MIN_PERIOD - (Y - 0), 0);
* Set Y = Y + dY
* Set X = X + dY
* Set R = R + dY
* Set dX = MAX(MIN_PERIOD - (X - Y), 0);
* Set X = X + dX
* Set R = R + dX
* Set dR = MAX(MIN_PERIOD - (R - X), 0);
* Set R = R + dR

* Assign R, X and Y back to RELOAD, MATCH1 and MATCH2 as appropriate

With this we require R <= (UINT32_MAX - (2 * (MIN_PERIOD - 1))) to
allow for any adjustments.

The downfall of this approach is it only works for a stopped timer. I can't see
how we could expand time on a running timer to account for a short match period
*and* keep consistency with the monotonic decrease of the status register.

Unless we use shadow registers or track things like ticks-per-tick? Currently the
way the timer is implemented is it derives the ticks and time periods directly from
the configuration of the model. If we implement a set of shadow registers that
contain the expanded times above and we translate between the configured values
and the dilated time it might be possible?

That's a half-baked thought, so if it doesn't make sense please ask questions and
I'll try to untangle the idea from the wool in my head.

It seems this needs some deep consideration :/

Andrew