On Tue, Feb 1, 2022 at 10:34 AM Alistair Francis <alistai...@gmail.com>
wrote:
> On Wed, Jan 26, 2022 at 7:55 PM <frank.ch...@sifive.com> wrote:
> >
> > From: Frank Chang <frank.ch...@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.ch...@sifive.com>
> > ---
> > hw/intc/riscv_aclint.c | 58 ++++++++++++++++++++++------------
> > include/hw/intc/riscv_aclint.h | 1 +
> > target/riscv/cpu.h | 8 ++---
> > target/riscv/cpu_helper.c | 4 +--
> > 4 files changed, 44 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> > index f1a5d3d284..ffbe211a3d 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,10 +146,10 @@ static uint64_t riscv_aclint_mtimer_read(void
> *opaque, hwaddr addr,
> > }
> > } else if (addr == mtimer->time_base) {
> > /* time_lo */
> > - return cpu_riscv_read_rtc(mtimer->timebase_freq) & 0xFFFFFFFF;
> > + return cpu_riscv_read_rtc(mtimer) & 0xFFFFFFFF;
> > } 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,
> > @@ -156,6 +162,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))) {
> > @@ -170,31 +177,40 @@ static void riscv_aclint_mtimer_write(void
> *opaque, hwaddr addr,
> > /* timecmp_lo */
> > 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));
> > return;
> > } else if ((addr & 0x7) == 4) {
> > /* timecmp_hi */
> > 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 write: %08x",
> > (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");
> > - return;
> > + } 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) {
> > + /* time_lo */
> > + mtimer->time_delta = ((rtc_r & ~0xFFFFFFFFULL) | value) -
> rtc_r;
> > + } else {
> > + /* time_hi */
> > + mtimer->time_delta = (value << 32 | (rtc_r & 0xFFFFFFFF)) -
> rtc_r;
> > + }
>
> We should be checking the size here, for RV64 a 64-bit memory access
> to mtime or mtimecmp can occur.
>
> Alistair
>
>From Privilege v1.12 spec:
In RV32, memory-mapped writes to mtimecmp modify only one 32-bit part of
the register.
For RV64, naturally aligned 64-bit memory accesses to the mtime and
mtimecmp registers are
additionally supported and are atomic.
It seems that both mtimecmp and mtime should check the sizes of 4 and 8-bit
memory accesses.
I will refine that in my next patch.
Regards,
Frank Chang
>
> > +
> > + /* 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);
> > + }
> > }
> >
> > qemu_log_mask(LOG_UNIMP,
> > @@ -299,7 +315,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 55635d68d5..46cac9df76 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -249,8 +249,8 @@ struct CPURISCVState {
> > target_ulong mseccfg;
> >
> > /* machine specific rdtime callback */
> > - uint64_t (*rdtime_fn)(uint32_t);
> > - uint32_t rdtime_fn_arg;
> > + uint64_t (*rdtime_fn)(void *);
> > + void *rdtime_fn_arg;
> >
> > /* True if in debugger mode. */
> > bool debugger;
> > @@ -413,8 +413,8 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState
> *env);
> > int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts);
> > uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_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);
> > #endif
> > void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv);
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 327a2c4f1d..73bf1bafa7 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -345,8 +345,8 @@ uint32_t riscv_cpu_update_mip(RISCVCPU *cpu,
> uint32_t mask, uint32_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.31.1
> >
> >
>
