Re: [PATCH v6 1/3] hw/intc: Move mtimer/mtimecmp to aclint

[Andrew Jones]

On Thu, Jul 21, 2022 at 06:00:44PM -0700, Atish Patra wrote:
> Historically, The mtime/mtimecmp has been part of the CPU because
> they are per hart entities. However, they actually belong to aclint
> which is a MMIO device.
> 
> Move them to the ACLINT device. This also emulates the real hardware
> more closely.
> 
> Reviewed-by: Anup Patel <anup@brainfault.org>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  hw/intc/riscv_aclint.c         | 41 ++++++++++++++++++++++++----------
>  hw/timer/ibex_timer.c          | 18 ++++++---------
>  include/hw/intc/riscv_aclint.h |  2 ++
>  include/hw/timer/ibex_timer.h  |  2 ++
>  target/riscv/cpu.h             |  2 --
>  target/riscv/machine.c         |  5 ++---
>  6 files changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index e7942c4e5a32..47f355224612 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -32,6 +32,7 @@
>  #include "hw/intc/riscv_aclint.h"
>  #include "qemu/timer.h"
>  #include "hw/irq.h"
> +#include "migration/vmstate.h"
>  
>  typedef struct riscv_aclint_mtimer_callback {
>      RISCVAclintMTimerState *s;
> @@ -65,8 +66,8 @@ static void 
> riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>  
>      uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
>  
> -    cpu->env.timecmp = value;
> -    if (cpu->env.timecmp <= rtc_r) {
> +    mtimer->timecmp[hartid] = value;
> +    if (mtimer->timecmp[hartid] <= rtc_r) {
>          /*
>           * If we're setting an MTIMECMP value in the "past",
>           * immediately raise the timer interrupt
> @@ -77,7 +78,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 = mtimer->timecmp[hartid] - rtc_r;
>      /* back to ns (note args switched in muldiv64) */
>      uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
>  
> @@ -102,7 +103,7 @@ static void 
> riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>          next = MIN(next, INT64_MAX);
>      }
>  
> -    timer_mod(cpu->env.timer, next);
> +    timer_mod(mtimer->timers[hartid], next);
>  }
>  
>  /*
> @@ -133,11 +134,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 for RV32/RV64 or timecmp for RV64 */
> -            uint64_t timecmp = env->timecmp;
> +            uint64_t timecmp = mtimer->timecmp[hartid];
>              return (size == 4) ? (timecmp & 0xFFFFFFFF) : timecmp;
>          } else if ((addr & 0x7) == 4) {
>              /* timecmp_hi */
> -            uint64_t timecmp = env->timecmp;
> +            uint64_t timecmp = mtimer->timecmp[hartid];
>              return (timecmp >> 32) & 0xFFFFFFFF;
>          } else {
>              qemu_log_mask(LOG_UNIMP,
> @@ -177,7 +178,7 @@ static void riscv_aclint_mtimer_write(void *opaque, 
> hwaddr addr,
>          } else if ((addr & 0x7) == 0) {
>              if (size == 4) {
>                  /* timecmp_lo for RV32/RV64 */
> -                uint64_t timecmp_hi = env->timecmp >> 32;
> +                uint64_t timecmp_hi = mtimer->timecmp[hartid] >> 32;
>                  riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> hartid,
>                      timecmp_hi << 32 | (value & 0xFFFFFFFF));
>              } else {
> @@ -188,7 +189,7 @@ static void riscv_aclint_mtimer_write(void *opaque, 
> hwaddr addr,
>          } else if ((addr & 0x7) == 4) {
>              if (size == 4) {
>                  /* timecmp_hi for RV32/RV64 */
> -                uint64_t timecmp_lo = env->timecmp;
> +                uint64_t timecmp_lo = mtimer->timecmp[hartid];
>                  riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> hartid,
>                      value << 32 | (timecmp_lo & 0xFFFFFFFF));
>              } else {
> @@ -234,7 +235,7 @@ static void riscv_aclint_mtimer_write(void *opaque, 
> hwaddr addr,
>              }
>              riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu),
>                                                mtimer->hartid_base + i,
> -                                              env->timecmp);
> +                                              mtimer->timecmp[i]);
>          }
>          return;
>      }
> @@ -284,6 +285,8 @@ static void riscv_aclint_mtimer_realize(DeviceState *dev, 
> Error **errp)
>      s->timer_irqs = g_new(qemu_irq, s->num_harts);
>      qdev_init_gpio_out(dev, s->timer_irqs, s->num_harts);
>  
> +    s->timers = g_malloc0(s->num_harts * sizeof(QEMUTimer));

It looks like we're overallocating the space here, since we want an
array of QEMUTimer pointers, not QEMUTimer objects. Also, QEMU
prefers g_new to g_malloc (see docs/devel/style.rst).

Thanks,
drew