[Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren

Jonathan Behrens posted 1 patch 1 week ago
Failed in applying to current master (apply log)
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(-)

[Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren

Posted by Jonathan Behrens 1 week ago
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 <fintelia@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;
     }

     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

Re: [Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren

Posted by Alistair Francis 4 days ago
On Fri, Apr 12, 2019 at 12:04 PM Jonathan Behrens <fintelia@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 <fintelia@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

Re: [Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren

Posted by Jonathan Behrens 4 days ago
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.

I'd be open to other suggestions.

Jonathan

On Mon, Apr 15, 2019 at 8:23 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Fri, Apr 12, 2019 at 12:04 PM Jonathan Behrens <fintelia@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 <fintelia@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
>

Re: [Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren

Posted by Alistair Francis 3 hours ago
On Mon, Apr 15, 2019 at 5:46 PM Jonathan Behrens <fintelia@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?

Alistair

>
> I'd be open to other suggestions.
>
> Jonathan
>
> On Mon, Apr 15, 2019 at 8:23 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Fri, Apr 12, 2019 at 12:04 PM Jonathan Behrens <fintelia@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 <fintelia@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