Re: [PATCH v1 2/5] hw/intc: sifive_clint: Use RISC-V CPU GPIO lines

[Anup Patel]

On Wed, Jul 14, 2021 at 11:41 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Jul 13, 2021 at 2:06 PM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Fri, Jul 9, 2021 at 9:01 AM Alistair Francis
> > <alistair.francis@wdc.com> wrote:
> > >
> > > Instead of using riscv_cpu_update_mip() let's instead use the new RISC-V
> > > CPU GPIO lines to set the timer and soft MIP bits.
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >  include/hw/intc/sifive_clint.h |  2 +
> > >  hw/intc/sifive_clint.c         | 72 ++++++++++++++++++++++++----------
> > >  2 files changed, 54 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/include/hw/intc/sifive_clint.h 
> > > b/include/hw/intc/sifive_clint.h
> > > index a30be0f3d6..921b1561dd 100644
> > > --- a/include/hw/intc/sifive_clint.h
> > > +++ b/include/hw/intc/sifive_clint.h
> > > @@ -40,6 +40,8 @@ typedef struct SiFiveCLINTState {
> > >      uint32_t time_base;
> > >      uint32_t aperture_size;
> > >      uint32_t timebase_freq;
> > > +    qemu_irq *timer_irqs;
> > > +    qemu_irq *soft_irqs;
> > >  } SiFiveCLINTState;
> > >
> > >  DeviceState *sifive_clint_create(hwaddr addr, hwaddr size,
> > > diff --git a/hw/intc/sifive_clint.c b/hw/intc/sifive_clint.c
> > > index 0f41e5ea1c..c635a47507 100644
> > > --- a/hw/intc/sifive_clint.c
> > > +++ b/hw/intc/sifive_clint.c
> > > @@ -28,6 +28,12 @@
> > >  #include "hw/qdev-properties.h"
> > >  #include "hw/intc/sifive_clint.h"
> > >  #include "qemu/timer.h"
> > > +#include "hw/irq.h"
> > > +
> > > +typedef struct sifive_clint_callback {
> > > +    SiFiveCLINTState *s;
> > > +    int num;
> > > +} sifive_clint_callback;
> > >
> > >  static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
> > >  {
> > > @@ -39,7 +45,9 @@ static uint64_t cpu_riscv_read_rtc(uint32_t 
> > > timebase_freq)
> > >   * Called when timecmp is written to update the QEMU timer or immediately
> > >   * trigger timer interrupt if mtimecmp <= current timer value.
> > >   */
> > > -static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value,
> > > +static void sifive_clint_write_timecmp(SiFiveCLINTState *s, RISCVCPU 
> > > *cpu,
> > > +                                       int hartid,
> > > +                                       uint64_t value,
> > >                                         uint32_t timebase_freq)
> > >  {
> > >      uint64_t next;
> > > @@ -51,12 +59,12 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, 
> > > uint64_t value,
> > >      if (cpu->env.timecmp <= rtc_r) {
> > >          /* if we're setting an MTIMECMP value in the "past",
> > >             immediately raise the timer interrupt */
> > > -        riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(1));
> > > +        qemu_irq_raise(s->timer_irqs[hartid]);
> >
> > This breaks multi-socket support.
> >
> > Please use "hartid - s->hartid_base" as an index.
> >
> > >          return;
> > >      }
> > >
> > >      /* otherwise, set up the future timer interrupt */
> > > -    riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
> > > +    qemu_irq_lower(s->timer_irqs[hartid]);
> > >      diff = cpu->env.timecmp - rtc_r;
> > >      /* back to ns (note args switched in muldiv64) */
> > >      next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> > > @@ -70,8 +78,9 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, 
> > > uint64_t value,
> > >   */
> > >  static void sifive_clint_timer_cb(void *opaque)
> > >  {
> > > -    RISCVCPU *cpu = opaque;
> > > -    riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(1));
> > > +    sifive_clint_callback *state = opaque;
> > > +
> > > +    qemu_irq_raise(state->s->timer_irqs[state->num]);
> > >  }
> > >
> > >  /* CPU wants to read rtc or timecmp register */
> > > @@ -137,7 +146,11 @@ static void sifive_clint_write(void *opaque, hwaddr 
> > > addr, uint64_t value,
> > >          if (!env) {
> > >              error_report("clint: invalid timecmp hartid: %zu", hartid);
> > >          } else if ((addr & 0x3) == 0) {
> > > -            riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_MSIP, 
> > > BOOL_TO_MASK(value));
> > > +            if (value) {
> > > +                qemu_irq_raise(clint->soft_irqs[hartid]);
> > > +            } else {
> > > +                qemu_irq_lower(clint->soft_irqs[hartid]);
> > > +            }
> >
> > Eventthis is broken for multi-socket.
> >
> > Use "hartid - clint->hartid_base" as index.
>
> Thanks for testing this. I have fixed this and will send a v2. I also
> added multi-socket tests to my automated tests.

Thanks, I will rebase the ACLINT series on your v2 series.

Regards,
Anup

>
> Alistair
>
> >
> > >          } else {
> > >              error_report("clint: invalid sip write: %08x", 
> > > (uint32_t)addr);
> > >          }
> > > @@ -153,13 +166,13 @@ static void sifive_clint_write(void *opaque, hwaddr 
> > > addr, uint64_t value,
> > >          } else if ((addr & 0x7) == 0) {
> > >              /* timecmp_lo */
> > >              uint64_t timecmp_hi = env->timecmp >> 32;
> > > -            sifive_clint_write_timecmp(RISCV_CPU(cpu),
> > > +            sifive_clint_write_timecmp(clint, RISCV_CPU(cpu), hartid,
> > >                  timecmp_hi << 32 | (value & 0xFFFFFFFF), 
> > > clint->timebase_freq);
> > >              return;
> > >          } else if ((addr & 0x7) == 4) {
> > >              /* timecmp_hi */
> > >              uint64_t timecmp_lo = env->timecmp;
> > > -            sifive_clint_write_timecmp(RISCV_CPU(cpu),
> > > +            sifive_clint_write_timecmp(clint, RISCV_CPU(cpu), hartid,
> > >                  value << 32 | (timecmp_lo & 0xFFFFFFFF), 
> > > clint->timebase_freq);
> > >          } else {
> > >              error_report("clint: invalid timecmp write: %08x", 
> > > (uint32_t)addr);
> > > @@ -205,6 +218,12 @@ static void sifive_clint_realize(DeviceState *dev, 
> > > Error **errp)
> > >      memory_region_init_io(&s->mmio, OBJECT(dev), &sifive_clint_ops, s,
> > >                            TYPE_SIFIVE_CLINT, s->aperture_size);
> > >      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
> > > +
> > > +    s->timer_irqs = g_malloc(sizeof(qemu_irq) * s->num_harts);
> > > +    qdev_init_gpio_out(dev, s->timer_irqs, s->num_harts);
> > > +
> > > +    s->soft_irqs = g_malloc(sizeof(qemu_irq) * s->num_harts);
> > > +    qdev_init_gpio_out(dev, s->soft_irqs, s->num_harts);
> > >  }
> > >
> > >  static void sifive_clint_class_init(ObjectClass *klass, void *data)
> > > @@ -228,7 +247,6 @@ static void sifive_clint_register_types(void)
> > >
> > >  type_init(sifive_clint_register_types)
> > >
> > > -
> > >  /*
> > >   * Create CLINT device.
> > >   */
> > > @@ -238,29 +256,43 @@ DeviceState *sifive_clint_create(hwaddr addr, 
> > > hwaddr size,
> > >      bool provide_rdtime)
> > >  {
> > >      int i;
> > > +
> > > +    DeviceState *dev = qdev_new(TYPE_SIFIVE_CLINT);
> > > +    qdev_prop_set_uint32(dev, "hartid-base", hartid_base);
> > > +    qdev_prop_set_uint32(dev, "num-harts", num_harts);
> > > +    qdev_prop_set_uint32(dev, "sip-base", sip_base);
> > > +    qdev_prop_set_uint32(dev, "timecmp-base", timecmp_base);
> > > +    qdev_prop_set_uint32(dev, "time-base", time_base);
> > > +    qdev_prop_set_uint32(dev, "aperture-size", size);
> > > +    qdev_prop_set_uint32(dev, "timebase-freq", timebase_freq);
> > > +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
> > > +
> > >      for (i = 0; i < num_harts; i++) {
> > >          CPUState *cpu = qemu_get_cpu(hartid_base + i);
> > > +        RISCVCPU *rvcpu = RISCV_CPU(cpu);
> > >          CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
> > > +        sifive_clint_callback *cb = 
> > > g_malloc0(sizeof(sifive_clint_callback));
> > > +
> > >          if (!env) {
> > > +            g_free(cb);
> > >              continue;
> > >          }
> > >          if (provide_rdtime) {
> > >              riscv_cpu_set_rdtime_fn(env, cpu_riscv_read_rtc, 
> > > timebase_freq);
> > >          }
> > > +
> > > +        cb->s = SIFIVE_CLINT(dev);
> > > +        cb->num = i;
> > >          env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > -                                  &sifive_clint_timer_cb, cpu);
> > > +                                  &sifive_clint_timer_cb, cb);
> > >          env->timecmp = 0;
> > > +
> > > +        qdev_connect_gpio_out_named(dev, NULL, i,
> > > +                           qdev_get_gpio_in(DEVICE(rvcpu), IRQ_M_TIMER));
> > > +        qdev_connect_gpio_out_named(dev, NULL, num_harts + i,
> > > +                           qdev_get_gpio_in(DEVICE(rvcpu), IRQ_M_SOFT));
> > >      }
> > >
> > > -    DeviceState *dev = qdev_new(TYPE_SIFIVE_CLINT);
> > > -    qdev_prop_set_uint32(dev, "hartid-base", hartid_base);
> > > -    qdev_prop_set_uint32(dev, "num-harts", num_harts);
> > > -    qdev_prop_set_uint32(dev, "sip-base", sip_base);
> > > -    qdev_prop_set_uint32(dev, "timecmp-base", timecmp_base);
> > > -    qdev_prop_set_uint32(dev, "time-base", time_base);
> > > -    qdev_prop_set_uint32(dev, "aperture-size", size);
> > > -    qdev_prop_set_uint32(dev, "timebase-freq", timebase_freq);
> > > -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > > -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
> > >      return dev;
> > >  }
> > > --
> > > 2.31.1
> > >
> > >
> >
> > Regards,
> > Anup