Re: [PATCH v3 3/4] hw/intc: Make RISC-V ACLINT mtime MMIO register writable

[Alistair Francis <alistair23@gmail.com>]

On Tue, Apr 19, 2022 at 7:10 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> RISC-V privilege spec defines that mtime is exposed as a memory-mapped
> machine-mode read-write register. However, as QEMU uses host monotonic
> timer as timer source, this makes mtime to be read-only in RISC-V
> ACLINT.
>
> This patch makes mtime to be writable by recording the time delta value
> between the mtime value to be written and the timer value at the time
> mtime is written. Time delta value is then added back whenever the timer
> value is retrieved.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>
> ---
>  hw/intc/riscv_aclint.c         | 71 ++++++++++++++++++++++++----------
>  include/hw/intc/riscv_aclint.h |  1 +
>  target/riscv/cpu.h             |  8 ++--
>  target/riscv/cpu_helper.c      |  4 +-
>  4 files changed, 57 insertions(+), 27 deletions(-)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index ad3c49706f..ad7ccf96cd 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -38,12 +38,18 @@ typedef struct riscv_aclint_mtimer_callback {
>      int num;
>  } riscv_aclint_mtimer_callback;
>
> -static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
> +static uint64_t cpu_riscv_read_rtc_raw(uint32_t timebase_freq)
>  {
>      return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>          timebase_freq, NANOSECONDS_PER_SECOND);
>  }
>
> +static uint64_t cpu_riscv_read_rtc(void *opaque)
> +{
> +    RISCVAclintMTimerState *mtimer = opaque;
> +    return cpu_riscv_read_rtc_raw(mtimer->timebase_freq) + mtimer->time_delta;
> +}
> +
>  /*
>   * Called when timecmp is written to update the QEMU timer or immediately
>   * trigger timer interrupt if mtimecmp <= current timer value.
> @@ -51,13 +57,13 @@ static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
>  static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>                                                RISCVCPU *cpu,
>                                                int hartid,
> -                                              uint64_t value,
> -                                              uint32_t timebase_freq)
> +                                              uint64_t value)
>  {
> +    uint32_t timebase_freq = mtimer->timebase_freq;
>      uint64_t next;
>      uint64_t diff;
>
> -    uint64_t rtc_r = cpu_riscv_read_rtc(timebase_freq);
> +    uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
>
>      cpu->env.timecmp = value;
>      if (cpu->env.timecmp <= rtc_r) {
> @@ -140,11 +146,11 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque, hwaddr addr,
>          }
>      } else if (addr == mtimer->time_base) {
>          /* time_lo for RV32/RV64 or timecmp for RV64 */
> -        uint64_t rtc = cpu_riscv_read_rtc(mtimer->timebase_freq);
> +        uint64_t rtc = cpu_riscv_read_rtc(mtimer);
>          return (size == 4) ? (rtc & 0xFFFFFFFF) : rtc;
>      } else if (addr == mtimer->time_base + 4) {
>          /* time_hi */
> -        return (cpu_riscv_read_rtc(mtimer->timebase_freq) >> 32) & 0xFFFFFFFF;
> +        return (cpu_riscv_read_rtc(mtimer) >> 32) & 0xFFFFFFFF;
>      }
>
>      qemu_log_mask(LOG_UNIMP,
> @@ -157,6 +163,7 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
>      uint64_t value, unsigned size)
>  {
>      RISCVAclintMTimerState *mtimer = opaque;
> +    int i;
>
>      if (addr >= mtimer->timecmp_base &&
>          addr < (mtimer->timecmp_base + (mtimer->num_harts << 3))) {
> @@ -172,20 +179,18 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
>                  /* timecmp_lo for RV32/RV64 */
>                  uint64_t timecmp_hi = env->timecmp >> 32;
>                  riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), hartid,
> -                    timecmp_hi << 32 | (value & 0xFFFFFFFF),
> -                    mtimer->timebase_freq);
> +                    timecmp_hi << 32 | (value & 0xFFFFFFFF));
>              } else {
>                  /* timecmp for RV64 */
>                  riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), hartid,
> -                                                  value, mtimer->timebase_freq);
> +                                                  value);
>              }
>          } else if ((addr & 0x7) == 4) {
>              if (size == 4) {
>                  /* timecmp_hi for RV32/RV64 */
>                  uint64_t timecmp_lo = env->timecmp;
>                  riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), hartid,
> -                    value << 32 | (timecmp_lo & 0xFFFFFFFF),
> -                    mtimer->timebase_freq);
> +                    value << 32 | (timecmp_lo & 0xFFFFFFFF));
>              } else {
>                  qemu_log_mask(LOG_UNIMP,
>                                "aclint-mtimer: invalid timecmp_hi write: %08x",
> @@ -197,15 +202,39 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
>                            (uint32_t)addr);
>          }
>          return;
> -    } else if (addr == mtimer->time_base) {
> -        /* time_lo */
> -        qemu_log_mask(LOG_UNIMP,
> -                      "aclint-mtimer: time_lo write not implemented");
> -        return;
> -    } else if (addr == mtimer->time_base + 4) {
> -        /* time_hi */
> -        qemu_log_mask(LOG_UNIMP,
> -                      "aclint-mtimer: time_hi write not implemented");
> +    } else if (addr == mtimer->time_base || addr == mtimer->time_base + 4) {
> +        uint64_t rtc_r = cpu_riscv_read_rtc_raw(mtimer->timebase_freq);
> +
> +        if (addr == mtimer->time_base) {
> +            if (size == 4) {
> +                /* time_lo for RV32/RV64 */
> +                mtimer->time_delta = ((rtc_r & ~0xFFFFFFFFULL) | value) - rtc_r;
> +            } else {
> +                /* time for RV64 */
> +                mtimer->time_delta = value - rtc_r;
> +            }
> +        } else {
> +            if (size == 4) {
> +                /* time_hi for RV32/RV64 */
> +                mtimer->time_delta = (value << 32 | (rtc_r & 0xFFFFFFFF)) - rtc_r;
> +            } else {
> +                qemu_log_mask(LOG_UNIMP,

This should be a guest error instead

Otherwise:

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> +                              "aclint-mtimer: invalid time_hi write: %08x",
> +                              (uint32_t)addr);
> +                return;
> +            }
> +        }
> +
> +        /* Check if timer interrupt is triggered for each hart. */
> +        for (i = 0; i < mtimer->num_harts; i++) {
> +            CPUState *cpu = qemu_get_cpu(mtimer->hartid_base + i);
> +            CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
> +            if (!env) {
> +                continue;
> +            }
> +            riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu),
> +                                              i, env->timecmp);
> +        }
>          return;
>      }
>
> @@ -315,7 +344,7 @@ DeviceState *riscv_aclint_mtimer_create(hwaddr addr, hwaddr size,
>              continue;
>          }
>          if (provide_rdtime) {
> -            riscv_cpu_set_rdtime_fn(env, cpu_riscv_read_rtc, timebase_freq);
> +            riscv_cpu_set_rdtime_fn(env, cpu_riscv_read_rtc, dev);
>          }
>
>          cb->s = RISCV_ACLINT_MTIMER(dev);
> diff --git a/include/hw/intc/riscv_aclint.h b/include/hw/intc/riscv_aclint.h
> index 229bd08d25..26d4048687 100644
> --- a/include/hw/intc/riscv_aclint.h
> +++ b/include/hw/intc/riscv_aclint.h
> @@ -31,6 +31,7 @@
>  typedef struct RISCVAclintMTimerState {
>      /*< private >*/
>      SysBusDevice parent_obj;
> +    uint64_t time_delta;
>
>      /*< public >*/
>      MemoryRegion mmio;
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 249fce4c3b..0cb7c2a05a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -286,8 +286,8 @@ struct CPUArchState {
>      type2_trigger_t type2_trig[TRIGGER_TYPE2_NUM];
>
>      /* machine specific rdtime callback */
> -    uint64_t (*rdtime_fn)(uint32_t);
> -    uint32_t rdtime_fn_arg;
> +    uint64_t (*rdtime_fn)(void *);
> +    void *rdtime_fn_arg;
>
>      /* machine specific AIA ireg read-modify-write callback */
>  #define AIA_MAKE_IREG(__isel, __priv, __virt, __vgein, __xlen) \
> @@ -505,8 +505,8 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env);
>  int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint64_t interrupts);
>  uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, uint64_t mask, uint64_t value);
>  #define BOOL_TO_MASK(x) (-!!(x)) /* helper for riscv_cpu_update_mip value */
> -void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(uint32_t),
> -                             uint32_t arg);
> +void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(void *),
> +                             void *arg);
>  void riscv_cpu_set_aia_ireg_rmw_fn(CPURISCVState *env, uint32_t priv,
>                                     int (*rmw_fn)(void *arg,
>                                                   target_ulong reg,
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index b5bbe6fc39..51a3d96a66 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -632,8 +632,8 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, uint64_t mask, uint64_t value)
>      return old;
>  }
>
> -void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(uint32_t),
> -                             uint32_t arg)
> +void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(void *),
> +                             void *arg)
>  {
>      env->rdtime_fn = fn;
>      env->rdtime_fn_arg = arg;
> --
> 2.35.1
>
>