Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

[Peter Maydell]

On Tue, 26 Feb 2019 at 11:38, bzt <address@hidden> wrote:
>
> Dear qemu developers,
>
> Honestly SubmitAPatch is a bit complicated for me, so I'm hoping I've
> done everything right. To be sure, you can find my patch here:
> https://github.com/bztsrc/qemu-local-timer and diff against the latest
> github repo here:
> https://github.com/bztsrc/qemu-local-timer/blob/patches/3.1.50.diff
>
> Currently the IRQ_TIMER in bcm2836_control is defined, but not
> implemented. This patch adds the basic functionality to qemu by
> implementing 3 new registers in bcm2836_control. You can route the
> interrupt to one of the cores' IRQ or FIQ line by writing 0x40000024,
> and you can set up a periodic interrupt with 38.4MHz frequency by
> writing the divider into 0x40000034. Prescaler are not taken into
> account. When the IRQ fired, you'll see it in 0x40000040+4*N bit 11
> with the rest of the local IRQs, and you can acknowledge it by writing
> 1<<31 to 0x40000038.
>
> The patch is pretty simple, should be easy to review: it does not
> create new classes, does not delete anything, does not change class
> interface, and only two files modified, the bcm2836_control.c and it's
> header.
>
> Sign-off-by: Zoltán Baldaszti <address@hidden>
> Subject: [PATCH] Added periodic IRQ support for bcm2836_control local timer

OK, here are my substantive review comments.

> diff --git a/hw/intc/bcm2836_control.c b/hw/intc/bcm2836_control.c
> index cfa5bc7365..fbd31de0f1 100644
> --- a/hw/intc/bcm2836_control.c
> +++ b/hw/intc/bcm2836_control.c
> @@ -7,7 +7,11 @@
>   * This code is licensed under the GNU GPLv2 and later.
>   *
>   * At present, only implements interrupt routing, and mailboxes (i.e.,
> - * not local timer, PMU interrupt, or AXI counters).
> + * not PMU interrupt, or AXI counters).
> + *
> + * 64-bit ARM Local Timer Copyright (c) 2019. Zoltán Baldaszti
> + * The IRQ_TIMER support is still very basic, does not handle timer access,
> + * and such there's no point in enabling it without the interrupt flag set.

Can you be more precise about what's missing here? I didn't
see anything in the spec that looked like a register for
reading the current timer value (though it would certainly
be usual to provide one).

>   *
>   * Ref:
>   * 
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
> @@ -18,6 +22,9 @@
>  #include "qemu/log.h"
>
>  #define REG_GPU_ROUTE           0x0c
> +#define REG_LOCALTIMERROUTING   0x24
> +#define REG_LOCALTIMERCONTROL   0x34
> +#define REG_LOCALTIMERACK       0x38
>  #define REG_TIMERCONTROL        0x40
>  #define REG_MBOXCONTROL         0x50
>  #define REG_IRQSRC              0x60
> @@ -43,6 +50,13 @@
>  #define IRQ_TIMER       11
>  #define IRQ_MAX         IRQ_TIMER
>
> +#define LOCALTIMER_FREQ      38400000
> +#define LOCALTIMER_INTFLAG   (1 << 31)
> +#define LOCALTIMER_RELOAD    (1 << 30)
> +#define LOCALTIMER_INTENABLE (1 << 29)
> +#define LOCALTIMER_ENABLE    (1 << 28)
> +#define LOCALTIMER_VALUE(x)  ((x) & 0xfffffff)
> +
>  static void deliver_local(BCM2836ControlState *s, uint8_t core, uint8_t irq,
>                            uint32_t controlreg, uint8_t controlidx)
>  {
> @@ -78,6 +92,15 @@ static void bcm2836_control_update(BCM2836ControlState *s)
>          s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
>      }
>
> +    /* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
> +    if (s->triggered) {
> +        if (s->route_localtimer & 4) {
> +            s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
> +        } else {
> +            s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
> +        }
> +    }
> +
>      for (i = 0; i < BCM2836_NCORES; i++) {
>          /* handle local timer interrupts for this core */
>          if (s->timerirqs[i]) {
> @@ -162,6 +185,62 @@ static void bcm2836_control_set_gpu_fiq(void
> *opaque, int irq, int level)
>      bcm2836_control_update(s);
>  }
>
> +static void bcm2836_control_local_timer_set_next(void *opaque)
> +{
> +    BCM2836ControlState *s = opaque;
> +    uint64_t next_event;
> +
> +    assert(s->period > 0);
> +
> +    next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +        (NANOSECONDS_PER_SECOND * s->period / LOCALTIMER_FREQ);

This sort of X * Y / Z calculation for timers should
be done with muldiv64() which uses a larger intermediate
result to avoid overflow problems:

       next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
          muldiv64(s->period, NANOSECONDS_PER_SECOND, LOCALTIMER_FREQ);

> +    timer_mod(s->timer, next_event);
> +}
> +
> +static void bcm2836_control_local_timer_tick(void *opaque)
> +{
> +    BCM2836ControlState *s = opaque;
> +
> +    bcm2836_control_local_timer_set_next(s);
> +
> +    if (!s->triggered) {
> +        s->triggered = 1;
> +        bcm2836_control_update(s);
> +    }
> +}
> +
> +static void bcm2836_control_local_timer_control(void *opaque, uint32_t val)
> +{
> +    BCM2836ControlState *s = opaque;
> +
> +    s->period = LOCALTIMER_VALUE(val);
> +    if (val & LOCALTIMER_INTENABLE) {
> +        if (!s->timer) {
> +            s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                bcm2836_control_local_timer_tick, s);

You should just create the timer once, at device initialization,
not every time the guest enables it. Enabling the timer
can be done with timer_mod().

> +        }
> +        bcm2836_control_local_timer_set_next(s);
> +    } else {
> +        if (s->timer) {
> +            timer_del(s->timer);
> +            s->timer = NULL;

This leaks the timer, because timer_del() is just "turn
off the timer", not "deinitialize and free the timer memory".
You just want to use timer_del() here, and leave s->timer
set so that when it's enabled you can use timer_mod() to
restart it.

> +        }
> +        s->triggered = 0;
> +    }
> +}
> +
> +static void bcm2836_control_local_timer_ack(void *opaque, uint32_t val)
> +{
> +    BCM2836ControlState *s = opaque;
> +
> +    if (val & LOCALTIMER_INTFLAG) {
> +        s->triggered = 0;
> +    }
> +    if (val & LOCALTIMER_RELOAD) {
> +        bcm2836_control_local_timer_set_next(s);
> +    }
> +}
> +
>  static uint64_t bcm2836_control_read(void *opaque, hwaddr offset,
> unsigned size)
>  {
>      BCM2836ControlState *s = opaque;
> @@ -170,6 +249,14 @@ static uint64_t bcm2836_control_read(void
> *opaque, hwaddr offset, unsigned size)
>          assert(s->route_gpu_fiq < BCM2836_NCORES
>                 && s->route_gpu_irq < BCM2836_NCORES);
>          return ((uint32_t)s->route_gpu_fiq << 2) | s->route_gpu_irq;
> +    } else if (offset == REG_LOCALTIMERROUTING) {
> +        return s->route_localtimer;
> +    } else if (offset == REG_LOCALTIMERCONTROL) {
> +        return s->period |
> +            (s->timer ? LOCALTIMER_ENABLE | LOCALTIMER_INTENABLE : 0) |

This looks odd - the timer enable and interrupt enable bits
should be independent, not always either both set or both cleared.

I suspect this register will be simpler to model if you
have the state struct have a
  uint32_t local_timer_control;
which just stores the whole register value (so no
separate s->triggered and s->period -- just pull
the fields out of the s->local_timer_control with
extract32() or masking when you need them).

> +            (s->triggered ? LOCALTIMER_INTFLAG : 0);
> +    } else if (offset == REG_LOCALTIMERACK) {
> +        return 0;
>      } else if (offset >= REG_TIMERCONTROL && offset < REG_MBOXCONTROL) {
>          return s->timercontrol[(offset - REG_TIMERCONTROL) >> 2];
>      } else if (offset >= REG_MBOXCONTROL && offset < REG_IRQSRC) {
> @@ -195,6 +282,12 @@ static void bcm2836_control_write(void *opaque,
> hwaddr offset,
>      if (offset == REG_GPU_ROUTE) {
>          s->route_gpu_irq = val & 0x3;
>          s->route_gpu_fiq = (val >> 2) & 0x3;
> +    } else if (offset == REG_LOCALTIMERROUTING) {
> +        s->route_localtimer = val & 7;
> +    } else if (offset == REG_LOCALTIMERCONTROL) {
> +        bcm2836_control_local_timer_control(s, val);
> +    } else if (offset == REG_LOCALTIMERACK) {
> +        bcm2836_control_local_timer_ack(s, val);
>      } else if (offset >= REG_TIMERCONTROL && offset < REG_MBOXCONTROL) {
>          s->timercontrol[(offset - REG_TIMERCONTROL) >> 2] = val & 0xff;
>      } else if (offset >= REG_MBOXCONTROL && offset < REG_IRQSRC) {
> @@ -227,6 +320,13 @@ static void bcm2836_control_reset(DeviceState *d)
>
>      s->route_gpu_irq = s->route_gpu_fiq = 0;
>
> +    s->route_localtimer = s->triggered = 0;
> +    s->period = 0;
> +    if(s->timer) {
> +        timer_del(s->timer);
> +        s->timer = NULL;

As above, this shouldn't be NULLing out s->timer; you can just
unconditionally call timer_del().

> +    }
> +
>      for (i = 0; i < BCM2836_NCORES; i++) {
>          s->timercontrol[i] = 0;
>          s->mailboxcontrol[i] = 0;
> diff --git a/include/hw/intc/bcm2836_control.h
> b/include/hw/intc/bcm2836_control.h
> index 613f3c4186..873bd52253 100644
> --- a/include/hw/intc/bcm2836_control.h
> +++ b/include/hw/intc/bcm2836_control.h
> @@ -5,6 +5,9 @@
>   * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
>   * Written by Andrew Baumann
>   *
> + * 64-bit ARM Local Timer Copyright (c) 2019. Zoltán Baldaszti
> + * Added basic IRQ_TIMER interrupt support
> + *
>   * This code is licensed under the GNU GPLv2 and later.
>   */
>
> @@ -12,6 +15,7 @@
>  #define BCM2836_CONTROL_H
>
>  #include "hw/sysbus.h"
> +#include "qemu/timer.h"
>
>  /* 4 mailboxes per core, for 16 total */
>  #define BCM2836_NCORES 4
> @@ -39,6 +43,12 @@ typedef struct BCM2836ControlState {
>      bool gpu_irq, gpu_fiq;
>      uint8_t timerirqs[BCM2836_NCORES];
>
> +    /* local timer */
> +    uint8_t route_localtimer;
> +    uint32_t period;
> +    bool triggered;
> +    QEMUTimer *timer;

Slightly nicer to just embed the QEMUTimer struct (and
then initialize it with timer_init_ns() rather than using
timer_new_ns(), which does 'allocate and init'.)

> +
>      /* interrupt source registers, post-routing (also input-derived;
> visible) */
>      uint32_t irqsrc[BCM2836_NCORES];
>      uint32_t fiqsrc[BCM2836_NCORES];

New fields in this struct require new entries in the
vmstate_bcm2836_control struct so the data is migrated.
In this case we don't care about maintaining migration
compatibility between QEMU versions, so the easiest thing
is to bump the .version_id field to 2, and use
VMSTATE_*_V(..., 2) macros to mark the new fields as
being only present in version 2.

If you take my suggestion about using a single uint32_t
local_timer_control and an embedded QEMUTimer struct,
that works out at
   VMSTATE_TIMER_V(timer, BCM2836ControlState, 2),
   VMSTATE_UINT32_V(local_timer_control, BCM2836ControlState, 2),
   VMSTATE_UINT8_V(route_localtimer, BCM2836ControlState, 2),

thanks
-- PMM