Re: [RFC PATCH 1/3] target/riscv: Rename timer & timecmp to mtimer and mtimecmp

[Alistair Francis]

On Fri, Mar 4, 2022 at 2:08 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Fri, Mar 4, 2022 at 8:50 AM Atish Patra <atishp@rivosinc.com> wrote:
> >
> > Currently, the aclint and ibex timer devices uses the "timer" &
> > "timecmp" to generate the m-mode timer interrupt. In future,
> > we will have timer interrupt injected to S/VS mode directly.
> > No functionality change introduced in this patch.
>
> s/have timer/have a timer/
>
> >
> > Add a prefix "m" these enviornment variables to indicate its
> > true purpose.
>
> s/enviornment/environment/
>
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
>
> I suggest we should remove mtimecmp and mtimer from
> target/riscv because mtimer is a memory mapped device.

I agree. I guess this is in the CPU as it's per hart, and it's easy to
store here instead of in the timer, but we could convert aclint to use
an array of timers (Ibex only ever has one hart).

That would probably be a closer match to hardware, as the timer is
external to the CPU

Alistair

>
> Regards,
> Anup
>
> > ---
> >  hw/intc/riscv_aclint.c | 20 ++++++++++----------
> >  hw/timer/ibex_timer.c  | 14 +++++++-------
> >  target/riscv/cpu.h     |  4 ++--
> >  target/riscv/machine.c |  2 +-
> >  4 files changed, 20 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> > index f1a5d3d284fd..642794a06865 100644
> > --- a/hw/intc/riscv_aclint.c
> > +++ b/hw/intc/riscv_aclint.c
> > @@ -59,8 +59,8 @@ static void 
> > riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> >
> >      uint64_t rtc_r = cpu_riscv_read_rtc(timebase_freq);
> >
> > -    cpu->env.timecmp = value;
> > -    if (cpu->env.timecmp <= rtc_r) {
> > +    cpu->env.mtimecmp = value;
> > +    if (cpu->env.mtimecmp <= rtc_r) {
> >          /*
> >           * If we're setting an MTIMECMP value in the "past",
> >           * immediately raise the timer interrupt
> > @@ -71,7 +71,7 @@ static void 
> > riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> >
> >      /* otherwise, set up the future timer interrupt */
> >      qemu_irq_lower(mtimer->timer_irqs[hartid - mtimer->hartid_base]);
> > -    diff = cpu->env.timecmp - rtc_r;
> > +    diff = cpu->env.mtimecmp - rtc_r;
> >      /* back to ns (note args switched in muldiv64) */
> >      uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND, 
> > timebase_freq);
> >
> > @@ -96,7 +96,7 @@ static void 
> > riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> >          next = MIN(next, INT64_MAX);
> >      }
> >
> > -    timer_mod(cpu->env.timer, next);
> > +    timer_mod(cpu->env.mtimer, next);
> >  }
> >
> >  /*
> > @@ -127,11 +127,11 @@ static uint64_t riscv_aclint_mtimer_read(void 
> > *opaque, hwaddr addr,
> >                            "aclint-mtimer: invalid hartid: %zu", hartid);
> >          } else if ((addr & 0x7) == 0) {
> >              /* timecmp_lo */
> > -            uint64_t timecmp = env->timecmp;
> > +            uint64_t timecmp = env->mtimecmp;
> >              return timecmp & 0xFFFFFFFF;
> >          } else if ((addr & 0x7) == 4) {
> >              /* timecmp_hi */
> > -            uint64_t timecmp = env->timecmp;
> > +            uint64_t timecmp = env->mtimecmp;
> >              return (timecmp >> 32) & 0xFFFFFFFF;
> >          } else {
> >              qemu_log_mask(LOG_UNIMP,
> > @@ -168,14 +168,14 @@ static void riscv_aclint_mtimer_write(void *opaque, 
> > hwaddr addr,
> >                            "aclint-mtimer: invalid hartid: %zu", hartid);
> >          } else if ((addr & 0x7) == 0) {
> >              /* timecmp_lo */
> > -            uint64_t timecmp_hi = env->timecmp >> 32;
> > +            uint64_t timecmp_hi = env->mtimecmp >> 32;
> >              riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> > hartid,
> >                  timecmp_hi << 32 | (value & 0xFFFFFFFF),
> >                  mtimer->timebase_freq);
> >              return;
> >          } else if ((addr & 0x7) == 4) {
> >              /* timecmp_hi */
> > -            uint64_t timecmp_lo = env->timecmp;
> > +            uint64_t timecmp_lo = env->mtimecmp;
> >              riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> > hartid,
> >                  value << 32 | (timecmp_lo & 0xFFFFFFFF),
> >                  mtimer->timebase_freq);
> > @@ -304,9 +304,9 @@ DeviceState *riscv_aclint_mtimer_create(hwaddr addr, 
> > hwaddr size,
> >
> >          cb->s = RISCV_ACLINT_MTIMER(dev);
> >          cb->num = i;
> > -        env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > +        env->mtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> >                                    &riscv_aclint_mtimer_cb, cb);
> > -        env->timecmp = 0;
> > +        env->mtimecmp = 0;
> >
> >          qdev_connect_gpio_out(dev, i,
> >                                qdev_get_gpio_in(DEVICE(rvcpu), 
> > IRQ_M_TIMER));
> > diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c
> > index 8c2ca364daab..4c34f9e08282 100644
> > --- a/hw/timer/ibex_timer.c
> > +++ b/hw/timer/ibex_timer.c
> > @@ -73,9 +73,9 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
> >      }
> >
> >      /* Update the CPUs mtimecmp */
> > -    cpu->env.timecmp = value;
> > +    cpu->env.mtimecmp = value;
> >
> > -    if (cpu->env.timecmp <= now) {
> > +    if (cpu->env.mtimecmp <= now) {
> >          /*
> >           * If the mtimecmp was in the past raise the interrupt now.
> >           */
> > @@ -91,7 +91,7 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
> >      qemu_irq_lower(s->m_timer_irq);
> >      qemu_set_irq(s->irq, false);
> >
> > -    diff = cpu->env.timecmp - now;
> > +    diff = cpu->env.mtimecmp - now;
> >      next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> >                                   muldiv64(diff,
> >                                            NANOSECONDS_PER_SECOND,
> > @@ -99,9 +99,9 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
> >
> >      if (next < qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
> >          /* We overflowed the timer, just set it as large as we can */
> > -        timer_mod(cpu->env.timer, 0x7FFFFFFFFFFFFFFF);
> > +        timer_mod(cpu->env.mtimer, 0x7FFFFFFFFFFFFFFF);
> >      } else {
> > -        timer_mod(cpu->env.timer, next);
> > +        timer_mod(cpu->env.mtimer, next);
> >      }
> >  }
> >
> > @@ -122,9 +122,9 @@ static void ibex_timer_reset(DeviceState *dev)
> >
> >      CPUState *cpu = qemu_get_cpu(0);
> >      CPURISCVState *env = cpu->env_ptr;
> > -    env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > +    env->mtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> >                                &ibex_timer_cb, s);
> > -    env->timecmp = 0;
> > +    env->mtimecmp = 0;
> >
> >      s->timer_ctrl = 0x00000000;
> >      s->timer_cfg0 = 0x00010000;
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 5d914bd34550..94234c59ffa8 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -265,7 +265,7 @@ struct CPURISCVState {
> >      /* temporary htif regs */
> >      uint64_t mfromhost;
> >      uint64_t mtohost;
> > -    uint64_t timecmp;
> > +    uint64_t mtimecmp;
> >
> >      /* physical memory protection */
> >      pmp_table_t pmp_state;
> > @@ -316,7 +316,7 @@ struct CPURISCVState {
> >      float_status fp_status;
> >
> >      /* Fields from here on are preserved across CPU reset. */
> > -    QEMUTimer *timer; /* Internal timer */
> > +    QEMUTimer *mtimer; /* Internal timer for M-mode interrupt */
> >
> >      hwaddr kernel_addr;
> >      hwaddr fdt_addr;
> > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > index ebc33c9e2781..be3022082a46 100644
> > --- a/target/riscv/machine.c
> > +++ b/target/riscv/machine.c
> > @@ -303,7 +303,7 @@ const VMStateDescription vmstate_riscv_cpu = {
> >          VMSTATE_UINTTL(env.mscratch, RISCVCPU),
> >          VMSTATE_UINT64(env.mfromhost, RISCVCPU),
> >          VMSTATE_UINT64(env.mtohost, RISCVCPU),
> > -        VMSTATE_UINT64(env.timecmp, RISCVCPU),
> > +        VMSTATE_UINT64(env.mtimecmp, RISCVCPU),
> >
> >          VMSTATE_END_OF_LIST()
> >      },
> > --
> > 2.30.2
> >
> >
>