No, I've still been meaning to send it. After thinking about this some more I realized that it didn't actually make sense for the CLINT to decide the timer frequency and that it should instead be a property of the board itself. I got a bit sidetracked in the process of making those changes, but I should have a new version out in the next few days.
On Thu, Apr 25, 2019 at 5:44 PM Palmer Dabbelt <pal...@sifive.com> wrote: > On Fri, 19 Apr 2019 16:05:35 PDT (-0700), alistai...@gmail.com wrote: > > On Mon, Apr 15, 2019 at 5:46 PM Jonathan Behrens <finte...@gmail.com> > wrote: > >> > >> For any chip that has a CLINT, we want the frequency of the time > register and the frequency of the CLINT to match. That frequency, > SIFIVE_CLINT_TIMEBASE_FREQ (=10MHz) is currently defined in > hw/riscv/sifive_clint.h and so isn't visible to target/riscv/cpu.c where > the CPURISCVState is first created. Instead, I first initialize the > frequency to a reasonable default (1GHz) and then let the CLINT override > the value if one is attached. Phrased differently, the values produced by > the `sifive_clint.c: cpu_riscv_read_rtc()` and `csr.c: read_time()` must > match, and this is one way of doing that. > > > > Ah that seems fine. Can you add a comment in the code to indicate that > > it will be overwritten later? > > I don't see a v2, did I miss something? > > > > > Alistair > > > >> > >> I'd be open to other suggestions. > >> > >> Jonathan > >> > >> On Mon, Apr 15, 2019 at 8:23 PM Alistair Francis <alistai...@gmail.com> > wrote: > >>> > >>> On Fri, Apr 12, 2019 at 12:04 PM Jonathan Behrens <finte...@gmail.com> > wrote: > >>> > > >>> > Currently mcounteren.TM acts as though it is hardwired to zero, even > though > >>> > QEMU > >>> > allows it to be set. This change resolves the issue by allowing > reads to the > >>> > time and timeh control registers when running in a privileged mode > where > >>> > such > >>> > accesses are allowed. > >>> > > >>> > Signed-off-by: Jonathan Behrens <finte...@gmail.com> > >>> > --- > >>> > hw/riscv/sifive_clint.c | 1 + > >>> > target/riscv/cpu.c | 14 ++++++++++++++ > >>> > target/riscv/cpu.h | 2 ++ > >>> > target/riscv/csr.c | 17 +++++++++++------ > >>> > 4 files changed, 28 insertions(+), 6 deletions(-) > >>> > > >>> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c > >>> > index d4c159e937..3ad4fe6139 100644 > >>> > --- a/hw/riscv/sifive_clint.c > >>> > +++ b/hw/riscv/sifive_clint.c > >>> > @@ -237,6 +237,7 @@ DeviceState *sifive_clint_create(hwaddr addr, > hwaddr > >>> > size, uint32_t num_harts, > >>> > env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, > >>> > &sifive_clint_timer_cb, cpu); > >>> > env->timecmp = 0; > >>> > + env->time_freq = SIFIVE_CLINT_TIMEBASE_FREQ; > >>> > >>> Why do you need to set this here? > >>> > >>> Alistair > >>> > >>> > } > >>> > > >>> > DeviceState *dev = qdev_create(NULL, TYPE_SIFIVE_CLINT); > >>> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > >>> > index d61bce6d55..ff17d54691 100644 > >>> > --- a/target/riscv/cpu.c > >>> > +++ b/target/riscv/cpu.c > >>> > @@ -103,12 +103,20 @@ static void set_resetvec(CPURISCVState *env, > int > >>> > resetvec) > >>> > #endif > >>> > } > >>> > > >>> > +static void set_time_freq(CPURISCVState *env, uint64_t freq) > >>> > +{ > >>> > +#ifndef CONFIG_USER_ONLY > >>> > + env->time_freq = freq; > >>> > +#endif > >>> > +} > >>> > + > >>> > static void riscv_any_cpu_init(Object *obj) > >>> > { > >>> > CPURISCVState *env = &RISCV_CPU(obj)->env; > >>> > set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU); > >>> > set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0); > >>> > set_resetvec(env, DEFAULT_RSTVEC); > >>> > + set_time_freq(env, NANOSECONDS_PER_SECOND); > >>> > } > >>> > > >>> > #if defined(TARGET_RISCV32) > >>> > @@ -121,6 +129,7 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object > *obj) > >>> > set_resetvec(env, DEFAULT_RSTVEC); > >>> > set_feature(env, RISCV_FEATURE_MMU); > >>> > set_feature(env, RISCV_FEATURE_PMP); > >>> > + set_time_freq(env, NANOSECONDS_PER_SECOND); > >>> > } > >>> > > >>> > static void rv32gcsu_priv1_10_0_cpu_init(Object *obj) > >>> > @@ -131,6 +140,7 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object > *obj) > >>> > set_resetvec(env, DEFAULT_RSTVEC); > >>> > set_feature(env, RISCV_FEATURE_MMU); > >>> > set_feature(env, RISCV_FEATURE_PMP); > >>> > + set_time_freq(env, NANOSECONDS_PER_SECOND); > >>> > } > >>> > > >>> > static void rv32imacu_nommu_cpu_init(Object *obj) > >>> > @@ -140,6 +150,7 @@ static void rv32imacu_nommu_cpu_init(Object *obj) > >>> > set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0); > >>> > set_resetvec(env, DEFAULT_RSTVEC); > >>> > set_feature(env, RISCV_FEATURE_PMP); > >>> > + set_time_freq(env, NANOSECONDS_PER_SECOND); > >>> > } > >>> > > >>> > #elif defined(TARGET_RISCV64) > >>> > @@ -152,6 +163,7 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object > *obj) > >>> > set_resetvec(env, DEFAULT_RSTVEC); > >>> > set_feature(env, RISCV_FEATURE_MMU); > >>> > set_feature(env, RISCV_FEATURE_PMP); > >>> > + set_time_freq(env, NANOSECONDS_PER_SECOND); > >>> > } > >>> > > >>> > static void rv64gcsu_priv1_10_0_cpu_init(Object *obj) > >>> > @@ -162,6 +174,7 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object > *obj) > >>> > set_resetvec(env, DEFAULT_RSTVEC); > >>> > set_feature(env, RISCV_FEATURE_MMU); > >>> > set_feature(env, RISCV_FEATURE_PMP); > >>> > + set_time_freq(env, NANOSECONDS_PER_SECOND); > >>> > } > >>> > > >>> > static void rv64imacu_nommu_cpu_init(Object *obj) > >>> > @@ -171,6 +184,7 @@ static void rv64imacu_nommu_cpu_init(Object *obj) > >>> > set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0); > >>> > set_resetvec(env, DEFAULT_RSTVEC); > >>> > set_feature(env, RISCV_FEATURE_PMP); > >>> > + set_time_freq(env, NANOSECONDS_PER_SECOND); > >>> > } > >>> > > >>> > #endif > >>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > >>> > index 20bce8742e..67b1769ad3 100644 > >>> > --- a/target/riscv/cpu.h > >>> > +++ b/target/riscv/cpu.h > >>> > @@ -173,7 +173,9 @@ struct CPURISCVState { > >>> > /* temporary htif regs */ > >>> > uint64_t mfromhost; > >>> > uint64_t mtohost; > >>> > + > >>> > uint64_t timecmp; > >>> > + uint64_t time_freq; > >>> > > >>> > /* physical memory protection */ > >>> > pmp_table_t pmp_state; > >>> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > >>> > index e1d91b6c60..6083c782a1 100644 > >>> > --- a/target/riscv/csr.c > >>> > +++ b/target/riscv/csr.c > >>> > @@ -191,22 +191,31 @@ static int read_instreth(CPURISCVState *env, > int > >>> > csrno, target_ulong *val) > >>> > } > >>> > #endif /* TARGET_RISCV32 */ > >>> > > >>> > -#if defined(CONFIG_USER_ONLY) > >>> > static int read_time(CPURISCVState *env, int csrno, target_ulong > *val) > >>> > { > >>> > +#if !defined(CONFIG_USER_ONLY) > >>> > + *val = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > >>> > + env->time_freq, NANOSECONDS_PER_SECOND); > >>> > +#else > >>> > *val = cpu_get_host_ticks(); > >>> > +#endif > >>> > return 0; > >>> > } > >>> > > >>> > #if defined(TARGET_RISCV32) > >>> > static int read_timeh(CPURISCVState *env, int csrno, target_ulong > *val) > >>> > { > >>> > +#if !defined(CONFIG_USER_ONLY) > >>> > + *val = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > >>> > + env->time_freq, NANOSECONDS_PER_SECOND) >> 32; > >>> > +#else > >>> > *val = cpu_get_host_ticks() >> 32; > >>> > +#endif > >>> > return 0; > >>> > } > >>> > #endif > >>> > > >>> > -#else /* CONFIG_USER_ONLY */ > >>> > +#if !defined(CONFIG_USER_ONLY) > >>> > > >>> > /* Machine constants */ > >>> > > >>> > @@ -854,14 +863,10 @@ static riscv_csr_operations > csr_ops[CSR_TABLE_SIZE] = > >>> > { > >>> > [CSR_INSTRETH] = { ctr, > >>> > read_instreth }, > >>> > #endif > >>> > > >>> > - /* User-level time CSRs are only available in linux-user > >>> > - * In privileged mode, the monitor emulates these CSRs */ > >>> > -#if defined(CONFIG_USER_ONLY) > >>> > [CSR_TIME] = { ctr, > >>> > read_time }, > >>> > #if defined(TARGET_RISCV32) > >>> > [CSR_TIMEH] = { ctr, > >>> > read_timeh }, > >>> > #endif > >>> > -#endif > >>> > > >>> > #if !defined(CONFIG_USER_ONLY) > >>> > /* Machine Timers and Counters */ > >>> > -- > >>> > 2.20.1 >