diff mbox series

[for,4.1,v3] target/riscv: Expose time CSRs when allowed by [m|s]counteren

Message ID 20190528183020.17915-1-jonathan@fintelia.io
State New
Headers show
Series [for,4.1,v3] target/riscv: Expose time CSRs when allowed by [m|s]counteren | expand

Commit Message

Jonathan Behrens May 28, 2019, 6:30 p.m. UTC 
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.

The frequency of the time register is stored in the time_freq field of each hart so that it is accessible during CSR reads, but must be the same across all harts. Each board can initialize it to a custom value, although all currently use a 10 MHz frequency.

Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
---
 hw/riscv/riscv_hart.c           |  4 ++++
 hw/riscv/sifive_clint.c         | 30 ++++++++++++++++++++++--------
 hw/riscv/sifive_e.c             |  2 ++
 hw/riscv/sifive_u.c             |  4 +++-
 hw/riscv/spike.c                |  6 +++++-
 hw/riscv/virt.c                 |  4 +++-
 include/hw/riscv/riscv_hart.h   |  1 +
 include/hw/riscv/sifive_clint.h |  4 ----
 include/hw/riscv/sifive_e.h     |  4 ++++
 include/hw/riscv/sifive_u.h     |  1 +
 include/hw/riscv/spike.h        |  1 +
 include/hw/riscv/virt.h         |  1 +
 target/riscv/cpu.h              |  2 ++
 target/riscv/csr.c              | 17 +++++++++++------
 14 files changed, 60 insertions(+), 21 deletions(-)

Comments

Palmer Dabbelt June 14, 2019, 11:52 a.m. UTC | #1 
On Tue, 28 May 2019 11:30:20 PDT (-0700), jonathan@fintelia.io 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.
>
> The frequency of the time register is stored in the time_freq field of each hart so that it is accessible during CSR reads, but must be the same across all harts. Each board can initialize it to a custom value, although all currently use a 10 MHz frequency.
>
> Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> ---
>  hw/riscv/riscv_hart.c           |  4 ++++
>  hw/riscv/sifive_clint.c         | 30 ++++++++++++++++++++++--------
>  hw/riscv/sifive_e.c             |  2 ++
>  hw/riscv/sifive_u.c             |  4 +++-
>  hw/riscv/spike.c                |  6 +++++-
>  hw/riscv/virt.c                 |  4 +++-
>  include/hw/riscv/riscv_hart.h   |  1 +
>  include/hw/riscv/sifive_clint.h |  4 ----
>  include/hw/riscv/sifive_e.h     |  4 ++++
>  include/hw/riscv/sifive_u.h     |  1 +
>  include/hw/riscv/spike.h        |  1 +
>  include/hw/riscv/virt.h         |  1 +
>  target/riscv/cpu.h              |  2 ++
>  target/riscv/csr.c              | 17 +++++++++++------
>  14 files changed, 60 insertions(+), 21 deletions(-)
>
> diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
> index e34a26a0ef..c39cd55330 100644
> --- a/hw/riscv/riscv_hart.c
> +++ b/hw/riscv/riscv_hart.c
> @@ -19,6 +19,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/timer.h"
>  #include "qapi/error.h"
>  #include "hw/sysbus.h"
>  #include "target/riscv/cpu.h"
> @@ -27,6 +28,8 @@
>  static Property riscv_harts_props[] = {
>      DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1),
>      DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
> +    DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState, time_freq,
> +                       NANOSECONDS_PER_SECOND),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -49,6 +52,7 @@ static void riscv_harts_realize(DeviceState *dev, Error **errp)
>                                  sizeof(RISCVCPU), s->cpu_type,
>                                  &error_abort, NULL);
>          s->harts[n].env.mhartid = n;
> +        s->harts[n].env.time_freq = s->time_freq;
>          qemu_register_reset(riscv_harts_cpu_reset, &s->harts[n]);
>          object_property_set_bool(OBJECT(&s->harts[n]), true,
>                                   "realized", &err);
> diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> index d4c159e937..71edf4dcc6 100644
> --- a/hw/riscv/sifive_clint.c
> +++ b/hw/riscv/sifive_clint.c
> @@ -26,10 +26,10 @@
>  #include "hw/riscv/sifive_clint.h"
>  #include "qemu/timer.h"
>
> -static uint64_t cpu_riscv_read_rtc(void)
> +static uint64_t cpu_riscv_read_rtc(CPURISCVState *env)
>  {
>      return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> -        SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
> +        env->time_freq, NANOSECONDS_PER_SECOND);
>  }
>
>  /*
> @@ -41,7 +41,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value)
>      uint64_t next;
>      uint64_t diff;
>
> -    uint64_t rtc_r = cpu_riscv_read_rtc();
> +    uint64_t rtc_r = cpu_riscv_read_rtc(&cpu->env);
>
>      cpu->env.timecmp = value;
>      if (cpu->env.timecmp <= rtc_r) {
> @@ -56,7 +56,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value)
>      diff = cpu->env.timecmp - rtc_r;
>      /* back to ns (note args switched in muldiv64) */
>      next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -        muldiv64(diff, NANOSECONDS_PER_SECOND, SIFIVE_CLINT_TIMEBASE_FREQ);
> +        muldiv64(diff, NANOSECONDS_PER_SECOND, cpu->env.time_freq);
>      timer_mod(cpu->env.timer, next);
>  }
>
> @@ -107,11 +107,25 @@ static uint64_t sifive_clint_read(void *opaque, hwaddr addr, unsigned size)
>              return 0;
>          }
>      } else if (addr == clint->time_base) {
> -        /* time_lo */
> -        return cpu_riscv_read_rtc() & 0xFFFFFFFF;
> +        /* All harts must have the same time frequency, so just use hart 0 */
> +        CPUState *cpu = qemu_get_cpu(0);
> +        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
> +        if (!env) {
> +            error_report("clint: hart 0 not valid?!");
> +        } else {
> +            /* time_lo */
> +            return cpu_riscv_read_rtc(env) & 0xFFFFFFFF;
> +        }
>      } else if (addr == clint->time_base + 4) {
> -        /* time_hi */
> -        return (cpu_riscv_read_rtc() >> 32) & 0xFFFFFFFF;
> +        /* All harts must have the same time frequency, so just use hart 0 */
> +        CPUState *cpu = qemu_get_cpu(0);
> +        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
> +        if (!env) {
> +            error_report("clint: hart 0 not valid?!");
> +        } else {
> +            /* time_hi */
> +            return (cpu_riscv_read_rtc(env) >> 32) & 0xFFFFFFFF;
> +        }
>      }
>
>      error_report("clint: invalid read: %08x", (uint32_t)addr);
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 80ac56fa7d..2d6f768ff1 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -146,6 +146,8 @@ static void riscv_sifive_e_soc_init(Object *obj)
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
>                              &error_abort);
> +    object_property_set_int(OBJECT(&s->cpus), SIFIVE_E_TIMEBASE_FREQ,
> +                            "timebase-frequency", &error_abort);
>      sysbus_init_child_obj(obj, "riscv.sifive.e.gpio0",
>                            &s->gpio, sizeof(s->gpio),
>                            TYPE_SIFIVE_GPIO);
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 5ecc47cea3..ab2b00106b 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -116,7 +116,7 @@ static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
>
>      qemu_fdt_add_subnode(fdt, "/cpus");
>      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
> -        SIFIVE_CLINT_TIMEBASE_FREQ);
> +        SIFIVE_U_TIMEBASE_FREQ);
>      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
>      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
>
> @@ -329,6 +329,8 @@ static void riscv_sifive_u_soc_init(Object *obj)
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
>                              &error_abort);
> +    object_property_set_int(OBJECT(&s->cpus), SIFIVE_U_TIMEBASE_FREQ,
> +                            "timebase-frequency", &error_abort);
>
>      sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
>                            TYPE_CADENCE_GEM);
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 5b33d4be3b..7c1872aad0 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -106,7 +106,7 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
>
>      qemu_fdt_add_subnode(fdt, "/cpus");
>      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
> -        SIFIVE_CLINT_TIMEBASE_FREQ);
> +        SPIKE_TIMEBASE_FREQ);
>      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
>      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
>
> @@ -180,6 +180,8 @@ static void spike_board_init(MachineState *machine)
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>                              &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
> +                            "timebase-frequency", &error_abort);
>      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>                              &error_abort);
>
> @@ -268,6 +270,8 @@ static void spike_v1_10_0_board_init(MachineState *machine)
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>                              &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
> +                            "timebase-frequency", &error_abort);
>      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>                              &error_abort);
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 84d94d0c42..e3a2474cea 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -185,7 +185,7 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>
>      qemu_fdt_add_subnode(fdt, "/cpus");
>      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
> -                          SIFIVE_CLINT_TIMEBASE_FREQ);
> +                          VIRT_TIMEBASE_FREQ);
>      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
>      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
>
> @@ -403,6 +403,8 @@ static void riscv_virt_board_init(MachineState *machine)
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>                              &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), VIRT_TIMEBASE_FREQ,
> +                            "timebase-frequency", &error_abort);
>      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>                              &error_abort);
>
> diff --git a/include/hw/riscv/riscv_hart.h b/include/hw/riscv/riscv_hart.h
> index 0671d88a44..2e82e85b2b 100644
> --- a/include/hw/riscv/riscv_hart.h
> +++ b/include/hw/riscv/riscv_hart.h
> @@ -33,6 +33,7 @@ typedef struct RISCVHartArrayState {
>      /*< public >*/
>      uint32_t num_harts;
>      char *cpu_type;
> +    uint64_t time_freq;
>      RISCVCPU *harts;
>  } RISCVHartArrayState;
>
> diff --git a/include/hw/riscv/sifive_clint.h b/include/hw/riscv/sifive_clint.h
> index e2865be1d1..aaa2a58c6e 100644
> --- a/include/hw/riscv/sifive_clint.h
> +++ b/include/hw/riscv/sifive_clint.h
> @@ -47,8 +47,4 @@ enum {
>      SIFIVE_TIME_BASE    = 0xBFF8
>  };
>
> -enum {
> -    SIFIVE_CLINT_TIMEBASE_FREQ = 10000000
> -};
> -
>  #endif
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 3b14eb7462..95bd33d872 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -71,6 +71,10 @@ enum {
>      SIFIVE_E_GPIO0_IRQ0 = 8
>  };
>
> +enum {
> +    SIFIVE_E_TIMEBASE_FREQ = 10000000,
> +};
> +
>  #define SIFIVE_E_PLIC_HART_CONFIG "M"
>  #define SIFIVE_E_PLIC_NUM_SOURCES 127
>  #define SIFIVE_E_PLIC_NUM_PRIORITIES 7
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 892f0eee21..85ee1eca78 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -63,6 +63,7 @@ enum {
>  };
>
>  enum {
> +    SIFIVE_U_TIMEBASE_FREQ = 10000000,
>      SIFIVE_U_CLOCK_FREQ = 1000000000,
>      SIFIVE_U_GEM_CLOCK_FREQ = 125000000
>  };
> diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
> index 641b70da67..2d391d1559 100644
> --- a/include/hw/riscv/spike.h
> +++ b/include/hw/riscv/spike.h
> @@ -36,6 +36,7 @@ enum {
>  };
>
>  enum {
> +    SPIKE_TIMEBASE_FREQ = 10000000,
>      SPIKE_CLOCK_FREQ = 1000000000
>  };
>
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index d01a1a85c4..53d4318f76 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -53,6 +53,7 @@ enum {
>  };
>
>  enum {
> +    VIRT_TIMEBASE_FREQ = 10000000,
>      VIRT_CLOCK_FREQ = 1000000000
>  };
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 74e726c1c9..9c09bbbffd 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -175,7 +175,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 f9e2910643..303c362782 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 */
>
> @@ -856,14 +865,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

I would prefer to avoid making the timer CSRs availiable to S-mode and below,
as that doesn't match what implementations actually do.

>  #if !defined(CONFIG_USER_ONLY)
>      /* Machine Timers and Counters */
Jonathan Behrens June 24, 2019, 11:03 p.m. UTC | #2 
Apparently my previous message didn't make it out onto the list (sorry
about all these email glitches!). I've included the message again below.
Hopefully either a patch like this one or something simpler that just hard
codes mcounteren.TM to zero (so QEMU is at least conformant) can be merged
in time for 4.1.

On Fri, Jun 14, 2019 at 8:55 AM Jonathan Behrens <jonathan@fintelia.io>
wrote:

> I'm not sure that is accurate. Based on the discussion here
> <https://forums.sifive.com/t/possible-bug-in-counteren-csrs/2123> the
> HiFive Unleashed actually does support reading the timer CSR from
> unprivileged modes (from that discussion it does so a little too well...
> but it should presumably be fixed in later iterations of the processor).
> And even if no real hardware supported this capability, it still might make
> sense to provide it in QEMU as an optimization.
>
> On Fri, Jun 14, 2019 at 7:52 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
>> On Tue, 28 May 2019 11:30:20 PDT (-0700), jonathan@fintelia.io 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.
>> >
>> > The frequency of the time register is stored in the time_freq field of
>> each hart so that it is accessible during CSR reads, but must be the same
>> across all harts. Each board can initialize it to a custom value, although
>> all currently use a 10 MHz frequency.
>> >
>> > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
>> > ---
>> >  hw/riscv/riscv_hart.c           |  4 ++++
>> >  hw/riscv/sifive_clint.c         | 30 ++++++++++++++++++++++--------
>> >  hw/riscv/sifive_e.c             |  2 ++
>> >  hw/riscv/sifive_u.c             |  4 +++-
>> >  hw/riscv/spike.c                |  6 +++++-
>> >  hw/riscv/virt.c                 |  4 +++-
>> >  include/hw/riscv/riscv_hart.h   |  1 +
>> >  include/hw/riscv/sifive_clint.h |  4 ----
>> >  include/hw/riscv/sifive_e.h     |  4 ++++
>> >  include/hw/riscv/sifive_u.h     |  1 +
>> >  include/hw/riscv/spike.h        |  1 +
>> >  include/hw/riscv/virt.h         |  1 +
>> >  target/riscv/cpu.h              |  2 ++
>> >  target/riscv/csr.c              | 17 +++++++++++------
>> >  14 files changed, 60 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
>> > index e34a26a0ef..c39cd55330 100644
>> > --- a/hw/riscv/riscv_hart.c
>> > +++ b/hw/riscv/riscv_hart.c
>> > @@ -19,6 +19,7 @@
>> >   */
>> >
>> >  #include "qemu/osdep.h"
>> > +#include "qemu/timer.h"
>> >  #include "qapi/error.h"
>> >  #include "hw/sysbus.h"
>> >  #include "target/riscv/cpu.h"
>> > @@ -27,6 +28,8 @@
>> >  static Property riscv_harts_props[] = {
>> >      DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1),
>> >      DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
>> > +    DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState,
>> time_freq,
>> > +                       NANOSECONDS_PER_SECOND),
>> >      DEFINE_PROP_END_OF_LIST(),
>> >  };
>> >
>> > @@ -49,6 +52,7 @@ static void riscv_harts_realize(DeviceState *dev,
>> Error **errp)
>> >                                  sizeof(RISCVCPU), s->cpu_type,
>> >                                  &error_abort, NULL);
>> >          s->harts[n].env.mhartid = n;
>> > +        s->harts[n].env.time_freq = s->time_freq;
>> >          qemu_register_reset(riscv_harts_cpu_reset, &s->harts[n]);
>> >          object_property_set_bool(OBJECT(&s->harts[n]), true,
>> >                                   "realized", &err);
>> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
>> > index d4c159e937..71edf4dcc6 100644
>> > --- a/hw/riscv/sifive_clint.c
>> > +++ b/hw/riscv/sifive_clint.c
>> > @@ -26,10 +26,10 @@
>> >  #include "hw/riscv/sifive_clint.h"
>> >  #include "qemu/timer.h"
>> >
>> > -static uint64_t cpu_riscv_read_rtc(void)
>> > +static uint64_t cpu_riscv_read_rtc(CPURISCVState *env)
>> >  {
>> >      return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>> > -        SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
>> > +        env->time_freq, NANOSECONDS_PER_SECOND);
>> >  }
>> >
>> >  /*
>> > @@ -41,7 +41,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
>> uint64_t value)
>> >      uint64_t next;
>> >      uint64_t diff;
>> >
>> > -    uint64_t rtc_r = cpu_riscv_read_rtc();
>> > +    uint64_t rtc_r = cpu_riscv_read_rtc(&cpu->env);
>> >
>> >      cpu->env.timecmp = value;
>> >      if (cpu->env.timecmp <= rtc_r) {
>> > @@ -56,7 +56,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
>> uint64_t value)
>> >      diff = cpu->env.timecmp - rtc_r;
>> >      /* back to ns (note args switched in muldiv64) */
>> >      next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>> > -        muldiv64(diff, NANOSECONDS_PER_SECOND,
>> SIFIVE_CLINT_TIMEBASE_FREQ);
>> > +        muldiv64(diff, NANOSECONDS_PER_SECOND, cpu->env.time_freq);
>> >      timer_mod(cpu->env.timer, next);
>> >  }
>> >
>> > @@ -107,11 +107,25 @@ static uint64_t sifive_clint_read(void *opaque,
>> hwaddr addr, unsigned size)
>> >              return 0;
>> >          }
>> >      } else if (addr == clint->time_base) {
>> > -        /* time_lo */
>> > -        return cpu_riscv_read_rtc() & 0xFFFFFFFF;
>> > +        /* All harts must have the same time frequency, so just use
>> hart 0 */
>> > +        CPUState *cpu = qemu_get_cpu(0);
>> > +        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
>> > +        if (!env) {
>> > +            error_report("clint: hart 0 not valid?!");
>> > +        } else {
>> > +            /* time_lo */
>> > +            return cpu_riscv_read_rtc(env) & 0xFFFFFFFF;
>> > +        }
>> >      } else if (addr == clint->time_base + 4) {
>> > -        /* time_hi */
>> > -        return (cpu_riscv_read_rtc() >> 32) & 0xFFFFFFFF;
>> > +        /* All harts must have the same time frequency, so just use
>> hart 0 */
>> > +        CPUState *cpu = qemu_get_cpu(0);
>> > +        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
>> > +        if (!env) {
>> > +            error_report("clint: hart 0 not valid?!");
>> > +        } else {
>> > +            /* time_hi */
>> > +            return (cpu_riscv_read_rtc(env) >> 32) & 0xFFFFFFFF;
>> > +        }
>> >      }
>> >
>> >      error_report("clint: invalid read: %08x", (uint32_t)addr);
>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> > index 80ac56fa7d..2d6f768ff1 100644
>> > --- a/hw/riscv/sifive_e.c
>> > +++ b/hw/riscv/sifive_e.c
>> > @@ -146,6 +146,8 @@ static void riscv_sifive_e_soc_init(Object *obj)
>> >                              &error_abort);
>> >      object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
>> >                              &error_abort);
>> > +    object_property_set_int(OBJECT(&s->cpus), SIFIVE_E_TIMEBASE_FREQ,
>> > +                            "timebase-frequency", &error_abort);
>> >      sysbus_init_child_obj(obj, "riscv.sifive.e.gpio0",
>> >                            &s->gpio, sizeof(s->gpio),
>> >                            TYPE_SIFIVE_GPIO);
>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>> > index 5ecc47cea3..ab2b00106b 100644
>> > --- a/hw/riscv/sifive_u.c
>> > +++ b/hw/riscv/sifive_u.c
>> > @@ -116,7 +116,7 @@ static void create_fdt(SiFiveUState *s, const
>> struct MemmapEntry *memmap,
>> >
>> >      qemu_fdt_add_subnode(fdt, "/cpus");
>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
>> > -        SIFIVE_CLINT_TIMEBASE_FREQ);
>> > +        SIFIVE_U_TIMEBASE_FREQ);
>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
>> >
>> > @@ -329,6 +329,8 @@ static void riscv_sifive_u_soc_init(Object *obj)
>> >                              &error_abort);
>> >      object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
>> >                              &error_abort);
>> > +    object_property_set_int(OBJECT(&s->cpus), SIFIVE_U_TIMEBASE_FREQ,
>> > +                            "timebase-frequency", &error_abort);
>> >
>> >      sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
>> >                            TYPE_CADENCE_GEM);
>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> > index 5b33d4be3b..7c1872aad0 100644
>> > --- a/hw/riscv/spike.c
>> > +++ b/hw/riscv/spike.c
>> > @@ -106,7 +106,7 @@ static void create_fdt(SpikeState *s, const struct
>> MemmapEntry *memmap,
>> >
>> >      qemu_fdt_add_subnode(fdt, "/cpus");
>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
>> > -        SIFIVE_CLINT_TIMEBASE_FREQ);
>> > +        SPIKE_TIMEBASE_FREQ);
>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
>> >
>> > @@ -180,6 +180,8 @@ static void spike_board_init(MachineState *machine)
>> >                              &error_abort);
>> >      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>> >                              &error_abort);
>> > +    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
>> > +                            "timebase-frequency", &error_abort);
>> >      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>> >                              &error_abort);
>> >
>> > @@ -268,6 +270,8 @@ static void spike_v1_10_0_board_init(MachineState
>> *machine)
>> >                              &error_abort);
>> >      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>> >                              &error_abort);
>> > +    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
>> > +                            "timebase-frequency", &error_abort);
>> >      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>> >                              &error_abort);
>> >
>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> > index 84d94d0c42..e3a2474cea 100644
>> > --- a/hw/riscv/virt.c
>> > +++ b/hw/riscv/virt.c
>> > @@ -185,7 +185,7 @@ static void *create_fdt(RISCVVirtState *s, const
>> struct MemmapEntry *memmap,
>> >
>> >      qemu_fdt_add_subnode(fdt, "/cpus");
>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
>> > -                          SIFIVE_CLINT_TIMEBASE_FREQ);
>> > +                          VIRT_TIMEBASE_FREQ);
>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
>> >
>> > @@ -403,6 +403,8 @@ static void riscv_virt_board_init(MachineState
>> *machine)
>> >                              &error_abort);
>> >      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>> >                              &error_abort);
>> > +    object_property_set_int(OBJECT(&s->soc), VIRT_TIMEBASE_FREQ,
>> > +                            "timebase-frequency", &error_abort);
>> >      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>> >                              &error_abort);
>> >
>> > diff --git a/include/hw/riscv/riscv_hart.h
>> b/include/hw/riscv/riscv_hart.h
>> > index 0671d88a44..2e82e85b2b 100644
>> > --- a/include/hw/riscv/riscv_hart.h
>> > +++ b/include/hw/riscv/riscv_hart.h
>> > @@ -33,6 +33,7 @@ typedef struct RISCVHartArrayState {
>> >      /*< public >*/
>> >      uint32_t num_harts;
>> >      char *cpu_type;
>> > +    uint64_t time_freq;
>> >      RISCVCPU *harts;
>> >  } RISCVHartArrayState;
>> >
>> > diff --git a/include/hw/riscv/sifive_clint.h
>> b/include/hw/riscv/sifive_clint.h
>> > index e2865be1d1..aaa2a58c6e 100644
>> > --- a/include/hw/riscv/sifive_clint.h
>> > +++ b/include/hw/riscv/sifive_clint.h
>> > @@ -47,8 +47,4 @@ enum {
>> >      SIFIVE_TIME_BASE    = 0xBFF8
>> >  };
>> >
>> > -enum {
>> > -    SIFIVE_CLINT_TIMEBASE_FREQ = 10000000
>> > -};
>> > -
>> >  #endif
>> > diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
>> > index 3b14eb7462..95bd33d872 100644
>> > --- a/include/hw/riscv/sifive_e.h
>> > +++ b/include/hw/riscv/sifive_e.h
>> > @@ -71,6 +71,10 @@ enum {
>> >      SIFIVE_E_GPIO0_IRQ0 = 8
>> >  };
>> >
>> > +enum {
>> > +    SIFIVE_E_TIMEBASE_FREQ = 10000000,
>> > +};
>> > +
>> >  #define SIFIVE_E_PLIC_HART_CONFIG "M"
>> >  #define SIFIVE_E_PLIC_NUM_SOURCES 127
>> >  #define SIFIVE_E_PLIC_NUM_PRIORITIES 7
>> > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
>> > index 892f0eee21..85ee1eca78 100644
>> > --- a/include/hw/riscv/sifive_u.h
>> > +++ b/include/hw/riscv/sifive_u.h
>> > @@ -63,6 +63,7 @@ enum {
>> >  };
>> >
>> >  enum {
>> > +    SIFIVE_U_TIMEBASE_FREQ = 10000000,
>> >      SIFIVE_U_CLOCK_FREQ = 1000000000,
>> >      SIFIVE_U_GEM_CLOCK_FREQ = 125000000
>> >  };
>> > diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
>> > index 641b70da67..2d391d1559 100644
>> > --- a/include/hw/riscv/spike.h
>> > +++ b/include/hw/riscv/spike.h
>> > @@ -36,6 +36,7 @@ enum {
>> >  };
>> >
>> >  enum {
>> > +    SPIKE_TIMEBASE_FREQ = 10000000,
>> >      SPIKE_CLOCK_FREQ = 1000000000
>> >  };
>> >
>> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
>> > index d01a1a85c4..53d4318f76 100644
>> > --- a/include/hw/riscv/virt.h
>> > +++ b/include/hw/riscv/virt.h
>> > @@ -53,6 +53,7 @@ enum {
>> >  };
>> >
>> >  enum {
>> > +    VIRT_TIMEBASE_FREQ = 10000000,
>> >      VIRT_CLOCK_FREQ = 1000000000
>> >  };
>> >
>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> > index 74e726c1c9..9c09bbbffd 100644
>> > --- a/target/riscv/cpu.h
>> > +++ b/target/riscv/cpu.h
>> > @@ -175,7 +175,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 f9e2910643..303c362782 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 */
>> >
>> > @@ -856,14 +865,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
>>
>> I would prefer to avoid making the timer CSRs availiable to S-mode and
>> below,
>> as that doesn't match what implementations actually do.
>>
>> >  #if !defined(CONFIG_USER_ONLY)
>> >      /* Machine Timers and Counters */
>>
>
Palmer Dabbelt June 25, 2019, 9:56 a.m. UTC | #3 
On Mon, 24 Jun 2019 16:03:20 PDT (-0700), fintelia@gmail.com wrote:
> Apparently my previous message didn't make it out onto the list (sorry
> about all these email glitches!). I've included the message again below.
> Hopefully either a patch like this one or something simpler that just hard
> codes mcounteren.TM to zero (so QEMU is at least conformant) can be merged
> in time for 4.1.
>
> On Fri, Jun 14, 2019 at 8:55 AM Jonathan Behrens <jonathan@fintelia.io>
> wrote:
>
>> I'm not sure that is accurate. Based on the discussion here
>> <https://forums.sifive.com/t/possible-bug-in-counteren-csrs/2123> the
>> HiFive Unleashed actually does support reading the timer CSR from
>> unprivileged modes (from that discussion it does so a little too well...
>> but it should presumably be fixed in later iterations of the processor).
>> And even if no real hardware supported this capability, it still might make
>> sense to provide it in QEMU as an optimization.

time and cycle are different registers: rdtime should trap, but rdcycle should
work.  The hardware traps rdtime into machine mode and emulates it via a memory
mapped register.  The bug in the FU540-C000 appears to be related to the
counter enables, not the presence of the counters.

>>
>> On Fri, Jun 14, 2019 at 7:52 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>>> On Tue, 28 May 2019 11:30:20 PDT (-0700), jonathan@fintelia.io 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.
>>> >
>>> > The frequency of the time register is stored in the time_freq field of
>>> each hart so that it is accessible during CSR reads, but must be the same
>>> across all harts. Each board can initialize it to a custom value, although
>>> all currently use a 10 MHz frequency.
>>> >
>>> > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
>>> > ---
>>> >  hw/riscv/riscv_hart.c           |  4 ++++
>>> >  hw/riscv/sifive_clint.c         | 30 ++++++++++++++++++++++--------
>>> >  hw/riscv/sifive_e.c             |  2 ++
>>> >  hw/riscv/sifive_u.c             |  4 +++-
>>> >  hw/riscv/spike.c                |  6 +++++-
>>> >  hw/riscv/virt.c                 |  4 +++-
>>> >  include/hw/riscv/riscv_hart.h   |  1 +
>>> >  include/hw/riscv/sifive_clint.h |  4 ----
>>> >  include/hw/riscv/sifive_e.h     |  4 ++++
>>> >  include/hw/riscv/sifive_u.h     |  1 +
>>> >  include/hw/riscv/spike.h        |  1 +
>>> >  include/hw/riscv/virt.h         |  1 +
>>> >  target/riscv/cpu.h              |  2 ++
>>> >  target/riscv/csr.c              | 17 +++++++++++------
>>> >  14 files changed, 60 insertions(+), 21 deletions(-)
>>> >
>>> > diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
>>> > index e34a26a0ef..c39cd55330 100644
>>> > --- a/hw/riscv/riscv_hart.c
>>> > +++ b/hw/riscv/riscv_hart.c
>>> > @@ -19,6 +19,7 @@
>>> >   */
>>> >
>>> >  #include "qemu/osdep.h"
>>> > +#include "qemu/timer.h"
>>> >  #include "qapi/error.h"
>>> >  #include "hw/sysbus.h"
>>> >  #include "target/riscv/cpu.h"
>>> > @@ -27,6 +28,8 @@
>>> >  static Property riscv_harts_props[] = {
>>> >      DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1),
>>> >      DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
>>> > +    DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState,
>>> time_freq,
>>> > +                       NANOSECONDS_PER_SECOND),
>>> >      DEFINE_PROP_END_OF_LIST(),
>>> >  };
>>> >
>>> > @@ -49,6 +52,7 @@ static void riscv_harts_realize(DeviceState *dev,
>>> Error **errp)
>>> >                                  sizeof(RISCVCPU), s->cpu_type,
>>> >                                  &error_abort, NULL);
>>> >          s->harts[n].env.mhartid = n;
>>> > +        s->harts[n].env.time_freq = s->time_freq;
>>> >          qemu_register_reset(riscv_harts_cpu_reset, &s->harts[n]);
>>> >          object_property_set_bool(OBJECT(&s->harts[n]), true,
>>> >                                   "realized", &err);
>>> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
>>> > index d4c159e937..71edf4dcc6 100644
>>> > --- a/hw/riscv/sifive_clint.c
>>> > +++ b/hw/riscv/sifive_clint.c
>>> > @@ -26,10 +26,10 @@
>>> >  #include "hw/riscv/sifive_clint.h"
>>> >  #include "qemu/timer.h"
>>> >
>>> > -static uint64_t cpu_riscv_read_rtc(void)
>>> > +static uint64_t cpu_riscv_read_rtc(CPURISCVState *env)
>>> >  {
>>> >      return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>>> > -        SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
>>> > +        env->time_freq, NANOSECONDS_PER_SECOND);
>>> >  }
>>> >
>>> >  /*
>>> > @@ -41,7 +41,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
>>> uint64_t value)
>>> >      uint64_t next;
>>> >      uint64_t diff;
>>> >
>>> > -    uint64_t rtc_r = cpu_riscv_read_rtc();
>>> > +    uint64_t rtc_r = cpu_riscv_read_rtc(&cpu->env);
>>> >
>>> >      cpu->env.timecmp = value;
>>> >      if (cpu->env.timecmp <= rtc_r) {
>>> > @@ -56,7 +56,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
>>> uint64_t value)
>>> >      diff = cpu->env.timecmp - rtc_r;
>>> >      /* back to ns (note args switched in muldiv64) */
>>> >      next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>>> > -        muldiv64(diff, NANOSECONDS_PER_SECOND,
>>> SIFIVE_CLINT_TIMEBASE_FREQ);
>>> > +        muldiv64(diff, NANOSECONDS_PER_SECOND, cpu->env.time_freq);
>>> >      timer_mod(cpu->env.timer, next);
>>> >  }
>>> >
>>> > @@ -107,11 +107,25 @@ static uint64_t sifive_clint_read(void *opaque,
>>> hwaddr addr, unsigned size)
>>> >              return 0;
>>> >          }
>>> >      } else if (addr == clint->time_base) {
>>> > -        /* time_lo */
>>> > -        return cpu_riscv_read_rtc() & 0xFFFFFFFF;
>>> > +        /* All harts must have the same time frequency, so just use
>>> hart 0 */
>>> > +        CPUState *cpu = qemu_get_cpu(0);
>>> > +        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
>>> > +        if (!env) {
>>> > +            error_report("clint: hart 0 not valid?!");
>>> > +        } else {
>>> > +            /* time_lo */
>>> > +            return cpu_riscv_read_rtc(env) & 0xFFFFFFFF;
>>> > +        }
>>> >      } else if (addr == clint->time_base + 4) {
>>> > -        /* time_hi */
>>> > -        return (cpu_riscv_read_rtc() >> 32) & 0xFFFFFFFF;
>>> > +        /* All harts must have the same time frequency, so just use
>>> hart 0 */
>>> > +        CPUState *cpu = qemu_get_cpu(0);
>>> > +        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
>>> > +        if (!env) {
>>> > +            error_report("clint: hart 0 not valid?!");
>>> > +        } else {
>>> > +            /* time_hi */
>>> > +            return (cpu_riscv_read_rtc(env) >> 32) & 0xFFFFFFFF;
>>> > +        }
>>> >      }
>>> >
>>> >      error_report("clint: invalid read: %08x", (uint32_t)addr);
>>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>>> > index 80ac56fa7d..2d6f768ff1 100644
>>> > --- a/hw/riscv/sifive_e.c
>>> > +++ b/hw/riscv/sifive_e.c
>>> > @@ -146,6 +146,8 @@ static void riscv_sifive_e_soc_init(Object *obj)
>>> >                              &error_abort);
>>> >      object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
>>> >                              &error_abort);
>>> > +    object_property_set_int(OBJECT(&s->cpus), SIFIVE_E_TIMEBASE_FREQ,
>>> > +                            "timebase-frequency", &error_abort);
>>> >      sysbus_init_child_obj(obj, "riscv.sifive.e.gpio0",
>>> >                            &s->gpio, sizeof(s->gpio),
>>> >                            TYPE_SIFIVE_GPIO);
>>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>>> > index 5ecc47cea3..ab2b00106b 100644
>>> > --- a/hw/riscv/sifive_u.c
>>> > +++ b/hw/riscv/sifive_u.c
>>> > @@ -116,7 +116,7 @@ static void create_fdt(SiFiveUState *s, const
>>> struct MemmapEntry *memmap,
>>> >
>>> >      qemu_fdt_add_subnode(fdt, "/cpus");
>>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
>>> > -        SIFIVE_CLINT_TIMEBASE_FREQ);
>>> > +        SIFIVE_U_TIMEBASE_FREQ);
>>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
>>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
>>> >
>>> > @@ -329,6 +329,8 @@ static void riscv_sifive_u_soc_init(Object *obj)
>>> >                              &error_abort);
>>> >      object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
>>> >                              &error_abort);
>>> > +    object_property_set_int(OBJECT(&s->cpus), SIFIVE_U_TIMEBASE_FREQ,
>>> > +                            "timebase-frequency", &error_abort);
>>> >
>>> >      sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
>>> >                            TYPE_CADENCE_GEM);
>>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>>> > index 5b33d4be3b..7c1872aad0 100644
>>> > --- a/hw/riscv/spike.c
>>> > +++ b/hw/riscv/spike.c
>>> > @@ -106,7 +106,7 @@ static void create_fdt(SpikeState *s, const struct
>>> MemmapEntry *memmap,
>>> >
>>> >      qemu_fdt_add_subnode(fdt, "/cpus");
>>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
>>> > -        SIFIVE_CLINT_TIMEBASE_FREQ);
>>> > +        SPIKE_TIMEBASE_FREQ);
>>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
>>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
>>> >
>>> > @@ -180,6 +180,8 @@ static void spike_board_init(MachineState *machine)
>>> >                              &error_abort);
>>> >      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>>> >                              &error_abort);
>>> > +    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
>>> > +                            "timebase-frequency", &error_abort);
>>> >      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>>> >                              &error_abort);
>>> >
>>> > @@ -268,6 +270,8 @@ static void spike_v1_10_0_board_init(MachineState
>>> *machine)
>>> >                              &error_abort);
>>> >      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>>> >                              &error_abort);
>>> > +    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
>>> > +                            "timebase-frequency", &error_abort);
>>> >      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>>> >                              &error_abort);
>>> >
>>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>> > index 84d94d0c42..e3a2474cea 100644
>>> > --- a/hw/riscv/virt.c
>>> > +++ b/hw/riscv/virt.c
>>> > @@ -185,7 +185,7 @@ static void *create_fdt(RISCVVirtState *s, const
>>> struct MemmapEntry *memmap,
>>> >
>>> >      qemu_fdt_add_subnode(fdt, "/cpus");
>>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
>>> > -                          SIFIVE_CLINT_TIMEBASE_FREQ);
>>> > +                          VIRT_TIMEBASE_FREQ);
>>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
>>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
>>> >
>>> > @@ -403,6 +403,8 @@ static void riscv_virt_board_init(MachineState
>>> *machine)
>>> >                              &error_abort);
>>> >      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>>> >                              &error_abort);
>>> > +    object_property_set_int(OBJECT(&s->soc), VIRT_TIMEBASE_FREQ,
>>> > +                            "timebase-frequency", &error_abort);
>>> >      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>>> >                              &error_abort);
>>> >
>>> > diff --git a/include/hw/riscv/riscv_hart.h
>>> b/include/hw/riscv/riscv_hart.h
>>> > index 0671d88a44..2e82e85b2b 100644
>>> > --- a/include/hw/riscv/riscv_hart.h
>>> > +++ b/include/hw/riscv/riscv_hart.h
>>> > @@ -33,6 +33,7 @@ typedef struct RISCVHartArrayState {
>>> >      /*< public >*/
>>> >      uint32_t num_harts;
>>> >      char *cpu_type;
>>> > +    uint64_t time_freq;
>>> >      RISCVCPU *harts;
>>> >  } RISCVHartArrayState;
>>> >
>>> > diff --git a/include/hw/riscv/sifive_clint.h
>>> b/include/hw/riscv/sifive_clint.h
>>> > index e2865be1d1..aaa2a58c6e 100644
>>> > --- a/include/hw/riscv/sifive_clint.h
>>> > +++ b/include/hw/riscv/sifive_clint.h
>>> > @@ -47,8 +47,4 @@ enum {
>>> >      SIFIVE_TIME_BASE    = 0xBFF8
>>> >  };
>>> >
>>> > -enum {
>>> > -    SIFIVE_CLINT_TIMEBASE_FREQ = 10000000
>>> > -};
>>> > -
>>> >  #endif
>>> > diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
>>> > index 3b14eb7462..95bd33d872 100644
>>> > --- a/include/hw/riscv/sifive_e.h
>>> > +++ b/include/hw/riscv/sifive_e.h
>>> > @@ -71,6 +71,10 @@ enum {
>>> >      SIFIVE_E_GPIO0_IRQ0 = 8
>>> >  };
>>> >
>>> > +enum {
>>> > +    SIFIVE_E_TIMEBASE_FREQ = 10000000,
>>> > +};
>>> > +
>>> >  #define SIFIVE_E_PLIC_HART_CONFIG "M"
>>> >  #define SIFIVE_E_PLIC_NUM_SOURCES 127
>>> >  #define SIFIVE_E_PLIC_NUM_PRIORITIES 7
>>> > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
>>> > index 892f0eee21..85ee1eca78 100644
>>> > --- a/include/hw/riscv/sifive_u.h
>>> > +++ b/include/hw/riscv/sifive_u.h
>>> > @@ -63,6 +63,7 @@ enum {
>>> >  };
>>> >
>>> >  enum {
>>> > +    SIFIVE_U_TIMEBASE_FREQ = 10000000,
>>> >      SIFIVE_U_CLOCK_FREQ = 1000000000,
>>> >      SIFIVE_U_GEM_CLOCK_FREQ = 125000000
>>> >  };
>>> > diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
>>> > index 641b70da67..2d391d1559 100644
>>> > --- a/include/hw/riscv/spike.h
>>> > +++ b/include/hw/riscv/spike.h
>>> > @@ -36,6 +36,7 @@ enum {
>>> >  };
>>> >
>>> >  enum {
>>> > +    SPIKE_TIMEBASE_FREQ = 10000000,
>>> >      SPIKE_CLOCK_FREQ = 1000000000
>>> >  };
>>> >
>>> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
>>> > index d01a1a85c4..53d4318f76 100644
>>> > --- a/include/hw/riscv/virt.h
>>> > +++ b/include/hw/riscv/virt.h
>>> > @@ -53,6 +53,7 @@ enum {
>>> >  };
>>> >
>>> >  enum {
>>> > +    VIRT_TIMEBASE_FREQ = 10000000,
>>> >      VIRT_CLOCK_FREQ = 1000000000
>>> >  };
>>> >
>>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>> > index 74e726c1c9..9c09bbbffd 100644
>>> > --- a/target/riscv/cpu.h
>>> > +++ b/target/riscv/cpu.h
>>> > @@ -175,7 +175,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 f9e2910643..303c362782 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 */
>>> >
>>> > @@ -856,14 +865,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
>>>
>>> I would prefer to avoid making the timer CSRs availiable to S-mode and
>>> below,
>>> as that doesn't match what implementations actually do.
>>>
>>> >  #if !defined(CONFIG_USER_ONLY)
>>> >      /* Machine Timers and Counters */
>>>
>>
Jonathan Behrens June 25, 2019, 8:22 p.m. UTC | #4 
I just did some testing on a HiFive Unleashed board and can confirm what
you are saying. The low 5 bits of both mcounteren and scounteren are
writable (if you try to write 0xFFFFFFFF to them, they'll take on the value
0x1F) but even with the TM bit set in both mcounteren and scounteren the
rdtime instruction always generates an illegal instruction exception.

Reading through the relevant chapter of the spec, I still think that having
mcounteren.TM be writable but making rdtime unconditionally trap is
non-conformant. If other people feel strongly that rdtime should always
require trapping into firmware then the natural change would be to simply
hardwire mcounteren.TM to zero (the value in scounteren wouldn't matter in
that case so it could be left writable). My own (biased) personal feeling
is that this full implementation makes sense at least for the `virt`
machine type because it represents a clear case where deviating from
current hardware enables a performance boost, and would not break
compatibility with any current software: both OpenSBI and BBL try to enable
hardware handling of rdtime when the platform claims to support it.

On Tue, Jun 25, 2019 at 5:56 AM Palmer Dabbelt <palmer@sifive.com> wrote:

> On Mon, 24 Jun 2019 16:03:20 PDT (-0700), fintelia@gmail.com wrote:
> > Apparently my previous message didn't make it out onto the list (sorry
> > about all these email glitches!). I've included the message again below.
> > Hopefully either a patch like this one or something simpler that just
> hard
> > codes mcounteren.TM to zero (so QEMU is at least conformant) can be
> merged
> > in time for 4.1.
> >
> > On Fri, Jun 14, 2019 at 8:55 AM Jonathan Behrens <jonathan@fintelia.io>
> > wrote:
> >
> >> I'm not sure that is accurate. Based on the discussion here
> >> <https://forums.sifive.com/t/possible-bug-in-counteren-csrs/2123> the
> >> HiFive Unleashed actually does support reading the timer CSR from
> >> unprivileged modes (from that discussion it does so a little too well...
> >> but it should presumably be fixed in later iterations of the processor).
> >> And even if no real hardware supported this capability, it still might
> make
> >> sense to provide it in QEMU as an optimization.
>
> time and cycle are different registers: rdtime should trap, but rdcycle
> should
> work.  The hardware traps rdtime into machine mode and emulates it via a
> memory
> mapped register.  The bug in the FU540-C000 appears to be related to the
> counter enables, not the presence of the counters.
>
> >>
> >> On Fri, Jun 14, 2019 at 7:52 AM Palmer Dabbelt <palmer@sifive.com>
> wrote:
> >>
> >>> On Tue, 28 May 2019 11:30:20 PDT (-0700), jonathan@fintelia.io 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.
> >>> >
> >>> > The frequency of the time register is stored in the time_freq field
> of
> >>> each hart so that it is accessible during CSR reads, but must be the
> same
> >>> across all harts. Each board can initialize it to a custom value,
> although
> >>> all currently use a 10 MHz frequency.
> >>> >
> >>> > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> >>> > ---
> >>> >  hw/riscv/riscv_hart.c           |  4 ++++
> >>> >  hw/riscv/sifive_clint.c         | 30 ++++++++++++++++++++++--------
> >>> >  hw/riscv/sifive_e.c             |  2 ++
> >>> >  hw/riscv/sifive_u.c             |  4 +++-
> >>> >  hw/riscv/spike.c                |  6 +++++-
> >>> >  hw/riscv/virt.c                 |  4 +++-
> >>> >  include/hw/riscv/riscv_hart.h   |  1 +
> >>> >  include/hw/riscv/sifive_clint.h |  4 ----
> >>> >  include/hw/riscv/sifive_e.h     |  4 ++++
> >>> >  include/hw/riscv/sifive_u.h     |  1 +
> >>> >  include/hw/riscv/spike.h        |  1 +
> >>> >  include/hw/riscv/virt.h         |  1 +
> >>> >  target/riscv/cpu.h              |  2 ++
> >>> >  target/riscv/csr.c              | 17 +++++++++++------
> >>> >  14 files changed, 60 insertions(+), 21 deletions(-)
> >>> >
> >>> > diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
> >>> > index e34a26a0ef..c39cd55330 100644
> >>> > --- a/hw/riscv/riscv_hart.c
> >>> > +++ b/hw/riscv/riscv_hart.c
> >>> > @@ -19,6 +19,7 @@
> >>> >   */
> >>> >
> >>> >  #include "qemu/osdep.h"
> >>> > +#include "qemu/timer.h"
> >>> >  #include "qapi/error.h"
> >>> >  #include "hw/sysbus.h"
> >>> >  #include "target/riscv/cpu.h"
> >>> > @@ -27,6 +28,8 @@
> >>> >  static Property riscv_harts_props[] = {
> >>> >      DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts,
> 1),
> >>> >      DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
> >>> > +    DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState,
> >>> time_freq,
> >>> > +                       NANOSECONDS_PER_SECOND),
> >>> >      DEFINE_PROP_END_OF_LIST(),
> >>> >  };
> >>> >
> >>> > @@ -49,6 +52,7 @@ static void riscv_harts_realize(DeviceState *dev,
> >>> Error **errp)
> >>> >                                  sizeof(RISCVCPU), s->cpu_type,
> >>> >                                  &error_abort, NULL);
> >>> >          s->harts[n].env.mhartid = n;
> >>> > +        s->harts[n].env.time_freq = s->time_freq;
> >>> >          qemu_register_reset(riscv_harts_cpu_reset, &s->harts[n]);
> >>> >          object_property_set_bool(OBJECT(&s->harts[n]), true,
> >>> >                                   "realized", &err);
> >>> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> >>> > index d4c159e937..71edf4dcc6 100644
> >>> > --- a/hw/riscv/sifive_clint.c
> >>> > +++ b/hw/riscv/sifive_clint.c
> >>> > @@ -26,10 +26,10 @@
> >>> >  #include "hw/riscv/sifive_clint.h"
> >>> >  #include "qemu/timer.h"
> >>> >
> >>> > -static uint64_t cpu_riscv_read_rtc(void)
> >>> > +static uint64_t cpu_riscv_read_rtc(CPURISCVState *env)
> >>> >  {
> >>> >      return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> >>> > -        SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
> >>> > +        env->time_freq, NANOSECONDS_PER_SECOND);
> >>> >  }
> >>> >
> >>> >  /*
> >>> > @@ -41,7 +41,7 @@ static void sifive_clint_write_timecmp(RISCVCPU
> *cpu,
> >>> uint64_t value)
> >>> >      uint64_t next;
> >>> >      uint64_t diff;
> >>> >
> >>> > -    uint64_t rtc_r = cpu_riscv_read_rtc();
> >>> > +    uint64_t rtc_r = cpu_riscv_read_rtc(&cpu->env);
> >>> >
> >>> >      cpu->env.timecmp = value;
> >>> >      if (cpu->env.timecmp <= rtc_r) {
> >>> > @@ -56,7 +56,7 @@ static void sifive_clint_write_timecmp(RISCVCPU
> *cpu,
> >>> uint64_t value)
> >>> >      diff = cpu->env.timecmp - rtc_r;
> >>> >      /* back to ns (note args switched in muldiv64) */
> >>> >      next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> >>> > -        muldiv64(diff, NANOSECONDS_PER_SECOND,
> >>> SIFIVE_CLINT_TIMEBASE_FREQ);
> >>> > +        muldiv64(diff, NANOSECONDS_PER_SECOND, cpu->env.time_freq);
> >>> >      timer_mod(cpu->env.timer, next);
> >>> >  }
> >>> >
> >>> > @@ -107,11 +107,25 @@ static uint64_t sifive_clint_read(void *opaque,
> >>> hwaddr addr, unsigned size)
> >>> >              return 0;
> >>> >          }
> >>> >      } else if (addr == clint->time_base) {
> >>> > -        /* time_lo */
> >>> > -        return cpu_riscv_read_rtc() & 0xFFFFFFFF;
> >>> > +        /* All harts must have the same time frequency, so just use
> >>> hart 0 */
> >>> > +        CPUState *cpu = qemu_get_cpu(0);
> >>> > +        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
> >>> > +        if (!env) {
> >>> > +            error_report("clint: hart 0 not valid?!");
> >>> > +        } else {
> >>> > +            /* time_lo */
> >>> > +            return cpu_riscv_read_rtc(env) & 0xFFFFFFFF;
> >>> > +        }
> >>> >      } else if (addr == clint->time_base + 4) {
> >>> > -        /* time_hi */
> >>> > -        return (cpu_riscv_read_rtc() >> 32) & 0xFFFFFFFF;
> >>> > +        /* All harts must have the same time frequency, so just use
> >>> hart 0 */
> >>> > +        CPUState *cpu = qemu_get_cpu(0);
> >>> > +        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
> >>> > +        if (!env) {
> >>> > +            error_report("clint: hart 0 not valid?!");
> >>> > +        } else {
> >>> > +            /* time_hi */
> >>> > +            return (cpu_riscv_read_rtc(env) >> 32) & 0xFFFFFFFF;
> >>> > +        }
> >>> >      }
> >>> >
> >>> >      error_report("clint: invalid read: %08x", (uint32_t)addr);
> >>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> >>> > index 80ac56fa7d..2d6f768ff1 100644
> >>> > --- a/hw/riscv/sifive_e.c
> >>> > +++ b/hw/riscv/sifive_e.c
> >>> > @@ -146,6 +146,8 @@ static void riscv_sifive_e_soc_init(Object *obj)
> >>> >                              &error_abort);
> >>> >      object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
> >>> >                              &error_abort);
> >>> > +    object_property_set_int(OBJECT(&s->cpus),
> SIFIVE_E_TIMEBASE_FREQ,
> >>> > +                            "timebase-frequency", &error_abort);
> >>> >      sysbus_init_child_obj(obj, "riscv.sifive.e.gpio0",
> >>> >                            &s->gpio, sizeof(s->gpio),
> >>> >                            TYPE_SIFIVE_GPIO);
> >>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> >>> > index 5ecc47cea3..ab2b00106b 100644
> >>> > --- a/hw/riscv/sifive_u.c
> >>> > +++ b/hw/riscv/sifive_u.c
> >>> > @@ -116,7 +116,7 @@ static void create_fdt(SiFiveUState *s, const
> >>> struct MemmapEntry *memmap,
> >>> >
> >>> >      qemu_fdt_add_subnode(fdt, "/cpus");
> >>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
> >>> > -        SIFIVE_CLINT_TIMEBASE_FREQ);
> >>> > +        SIFIVE_U_TIMEBASE_FREQ);
> >>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
> >>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
> >>> >
> >>> > @@ -329,6 +329,8 @@ static void riscv_sifive_u_soc_init(Object *obj)
> >>> >                              &error_abort);
> >>> >      object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
> >>> >                              &error_abort);
> >>> > +    object_property_set_int(OBJECT(&s->cpus),
> SIFIVE_U_TIMEBASE_FREQ,
> >>> > +                            "timebase-frequency", &error_abort);
> >>> >
> >>> >      sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
> >>> >                            TYPE_CADENCE_GEM);
> >>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> >>> > index 5b33d4be3b..7c1872aad0 100644
> >>> > --- a/hw/riscv/spike.c
> >>> > +++ b/hw/riscv/spike.c
> >>> > @@ -106,7 +106,7 @@ static void create_fdt(SpikeState *s, const
> struct
> >>> MemmapEntry *memmap,
> >>> >
> >>> >      qemu_fdt_add_subnode(fdt, "/cpus");
> >>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
> >>> > -        SIFIVE_CLINT_TIMEBASE_FREQ);
> >>> > +        SPIKE_TIMEBASE_FREQ);
> >>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
> >>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
> >>> >
> >>> > @@ -180,6 +180,8 @@ static void spike_board_init(MachineState
> *machine)
> >>> >                              &error_abort);
> >>> >      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> >>> >                              &error_abort);
> >>> > +    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
> >>> > +                            "timebase-frequency", &error_abort);
> >>> >      object_property_set_bool(OBJECT(&s->soc), true, "realized",
> >>> >                              &error_abort);
> >>> >
> >>> > @@ -268,6 +270,8 @@ static void spike_v1_10_0_board_init(MachineState
> >>> *machine)
> >>> >                              &error_abort);
> >>> >      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> >>> >                              &error_abort);
> >>> > +    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
> >>> > +                            "timebase-frequency", &error_abort);
> >>> >      object_property_set_bool(OBJECT(&s->soc), true, "realized",
> >>> >                              &error_abort);
> >>> >
> >>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >>> > index 84d94d0c42..e3a2474cea 100644
> >>> > --- a/hw/riscv/virt.c
> >>> > +++ b/hw/riscv/virt.c
> >>> > @@ -185,7 +185,7 @@ static void *create_fdt(RISCVVirtState *s, const
> >>> struct MemmapEntry *memmap,
> >>> >
> >>> >      qemu_fdt_add_subnode(fdt, "/cpus");
> >>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
> >>> > -                          SIFIVE_CLINT_TIMEBASE_FREQ);
> >>> > +                          VIRT_TIMEBASE_FREQ);
> >>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
> >>> >      qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
> >>> >
> >>> > @@ -403,6 +403,8 @@ static void riscv_virt_board_init(MachineState
> >>> *machine)
> >>> >                              &error_abort);
> >>> >      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> >>> >                              &error_abort);
> >>> > +    object_property_set_int(OBJECT(&s->soc), VIRT_TIMEBASE_FREQ,
> >>> > +                            "timebase-frequency", &error_abort);
> >>> >      object_property_set_bool(OBJECT(&s->soc), true, "realized",
> >>> >                              &error_abort);
> >>> >
> >>> > diff --git a/include/hw/riscv/riscv_hart.h
> >>> b/include/hw/riscv/riscv_hart.h
> >>> > index 0671d88a44..2e82e85b2b 100644
> >>> > --- a/include/hw/riscv/riscv_hart.h
> >>> > +++ b/include/hw/riscv/riscv_hart.h
> >>> > @@ -33,6 +33,7 @@ typedef struct RISCVHartArrayState {
> >>> >      /*< public >*/
> >>> >      uint32_t num_harts;
> >>> >      char *cpu_type;
> >>> > +    uint64_t time_freq;
> >>> >      RISCVCPU *harts;
> >>> >  } RISCVHartArrayState;
> >>> >
> >>> > diff --git a/include/hw/riscv/sifive_clint.h
> >>> b/include/hw/riscv/sifive_clint.h
> >>> > index e2865be1d1..aaa2a58c6e 100644
> >>> > --- a/include/hw/riscv/sifive_clint.h
> >>> > +++ b/include/hw/riscv/sifive_clint.h
> >>> > @@ -47,8 +47,4 @@ enum {
> >>> >      SIFIVE_TIME_BASE    = 0xBFF8
> >>> >  };
> >>> >
> >>> > -enum {
> >>> > -    SIFIVE_CLINT_TIMEBASE_FREQ = 10000000
> >>> > -};
> >>> > -
> >>> >  #endif
> >>> > diff --git a/include/hw/riscv/sifive_e.h
> b/include/hw/riscv/sifive_e.h
> >>> > index 3b14eb7462..95bd33d872 100644
> >>> > --- a/include/hw/riscv/sifive_e.h
> >>> > +++ b/include/hw/riscv/sifive_e.h
> >>> > @@ -71,6 +71,10 @@ enum {
> >>> >      SIFIVE_E_GPIO0_IRQ0 = 8
> >>> >  };
> >>> >
> >>> > +enum {
> >>> > +    SIFIVE_E_TIMEBASE_FREQ = 10000000,
> >>> > +};
> >>> > +
> >>> >  #define SIFIVE_E_PLIC_HART_CONFIG "M"
> >>> >  #define SIFIVE_E_PLIC_NUM_SOURCES 127
> >>> >  #define SIFIVE_E_PLIC_NUM_PRIORITIES 7
> >>> > diff --git a/include/hw/riscv/sifive_u.h
> b/include/hw/riscv/sifive_u.h
> >>> > index 892f0eee21..85ee1eca78 100644
> >>> > --- a/include/hw/riscv/sifive_u.h
> >>> > +++ b/include/hw/riscv/sifive_u.h
> >>> > @@ -63,6 +63,7 @@ enum {
> >>> >  };
> >>> >
> >>> >  enum {
> >>> > +    SIFIVE_U_TIMEBASE_FREQ = 10000000,
> >>> >      SIFIVE_U_CLOCK_FREQ = 1000000000,
> >>> >      SIFIVE_U_GEM_CLOCK_FREQ = 125000000
> >>> >  };
> >>> > diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
> >>> > index 641b70da67..2d391d1559 100644
> >>> > --- a/include/hw/riscv/spike.h
> >>> > +++ b/include/hw/riscv/spike.h
> >>> > @@ -36,6 +36,7 @@ enum {
> >>> >  };
> >>> >
> >>> >  enum {
> >>> > +    SPIKE_TIMEBASE_FREQ = 10000000,
> >>> >      SPIKE_CLOCK_FREQ = 1000000000
> >>> >  };
> >>> >
> >>> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> >>> > index d01a1a85c4..53d4318f76 100644
> >>> > --- a/include/hw/riscv/virt.h
> >>> > +++ b/include/hw/riscv/virt.h
> >>> > @@ -53,6 +53,7 @@ enum {
> >>> >  };
> >>> >
> >>> >  enum {
> >>> > +    VIRT_TIMEBASE_FREQ = 10000000,
> >>> >      VIRT_CLOCK_FREQ = 1000000000
> >>> >  };
> >>> >
> >>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >>> > index 74e726c1c9..9c09bbbffd 100644
> >>> > --- a/target/riscv/cpu.h
> >>> > +++ b/target/riscv/cpu.h
> >>> > @@ -175,7 +175,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 f9e2910643..303c362782 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 */
> >>> >
> >>> > @@ -856,14 +865,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
> >>>
> >>> I would prefer to avoid making the timer CSRs availiable to S-mode and
> >>> below,
> >>> as that doesn't match what implementations actually do.
> >>>
> >>> >  #if !defined(CONFIG_USER_ONLY)
> >>> >      /* Machine Timers and Counters */
> >>>
> >>
>
Bin Meng June 26, 2019, 6:54 a.m. UTC | #5 
Hi Palmer,

On Tue, Jun 25, 2019 at 5:57 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Mon, 24 Jun 2019 16:03:20 PDT (-0700), fintelia@gmail.com wrote:
> > Apparently my previous message didn't make it out onto the list (sorry
> > about all these email glitches!). I've included the message again below.
> > Hopefully either a patch like this one or something simpler that just hard
> > codes mcounteren.TM to zero (so QEMU is at least conformant) can be merged
> > in time for 4.1.
> >
> > On Fri, Jun 14, 2019 at 8:55 AM Jonathan Behrens <jonathan@fintelia.io>
> > wrote:
> >
> >> I'm not sure that is accurate. Based on the discussion here
> >> <https://forums.sifive.com/t/possible-bug-in-counteren-csrs/2123> the
> >> HiFive Unleashed actually does support reading the timer CSR from
> >> unprivileged modes (from that discussion it does so a little too well...
> >> but it should presumably be fixed in later iterations of the processor).
> >> And even if no real hardware supported this capability, it still might make
> >> sense to provide it in QEMU as an optimization.
>
> time and cycle are different registers: rdtime should trap, but rdcycle should
> work.  The hardware traps rdtime into machine mode and emulates it via a memory
> mapped register.  The bug in the FU540-C000 appears to be related to the
> counter enables, not the presence of the counters.

I don't think rdtime is required to mandatorily trap.

Per privileged spec v1.10, chapter 3.1.17 Counter-Enable Registers
([m|h|s]counteren), it says:

"When the CY, TM, IR, or HPMn bit in the mcounteren register is clear,
attempts to read the cycle, time, instret, or hpmcountern register
while executing in S-mode or U-mode will cause an illegal instruction
exception."

In the same chapter, it also says:

"Implementations can convert reads of the time CSR into loads to the
memory-mapped mtime register, or hard-wire the TM bits in mxcounteren
to 0 and emulate this functionality in M-mode software."

So per my understanding when mcounteren.TM is set, rdtime should NOT trap.

Regards,
Bin
Bin Meng June 26, 2019, 6:58 a.m. UTC | #6 
On Wed, Jun 26, 2019 at 4:23 AM Jonathan Behrens <fintelia@gmail.com> wrote:
>
> I just did some testing on a HiFive Unleashed board and can confirm what
> you are saying. The low 5 bits of both mcounteren and scounteren are
> writable (if you try to write 0xFFFFFFFF to them, they'll take on the value
> 0x1F) but even with the TM bit set in both mcounteren and scounteren the
> rdtime instruction always generates an illegal instruction exception.
>

Then I would think the FU540 is not spec complaint :)

> Reading through the relevant chapter of the spec, I still think that having
> mcounteren.TM be writable but making rdtime unconditionally trap is
> non-conformant. If other people feel strongly that rdtime should always

Agree. To test hardware (FU540) compatibility in QEMU, maybe we can
add a cpu property to allow hard-wiring mcounteren.TM to zero?

> require trapping into firmware then the natural change would be to simply
> hardwire mcounteren.TM to zero (the value in scounteren wouldn't matter in
> that case so it could be left writable). My own (biased) personal feeling
> is that this full implementation makes sense at least for the `virt`
> machine type because it represents a clear case where deviating from
> current hardware enables a performance boost, and would not break
> compatibility with any current software: both OpenSBI and BBL try to enable
> hardware handling of rdtime when the platform claims to support it.
>

Regards,
Bin
Palmer Dabbelt June 26, 2019, 8:25 a.m. UTC | #7 
On Tue, 25 Jun 2019 23:54:06 PDT (-0700), bmeng.cn@gmail.com wrote:
> Hi Palmer,
>
> On Tue, Jun 25, 2019 at 5:57 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Mon, 24 Jun 2019 16:03:20 PDT (-0700), fintelia@gmail.com wrote:
>> > Apparently my previous message didn't make it out onto the list (sorry
>> > about all these email glitches!). I've included the message again below.
>> > Hopefully either a patch like this one or something simpler that just hard
>> > codes mcounteren.TM to zero (so QEMU is at least conformant) can be merged
>> > in time for 4.1.
>> >
>> > On Fri, Jun 14, 2019 at 8:55 AM Jonathan Behrens <jonathan@fintelia.io>
>> > wrote:
>> >
>> >> I'm not sure that is accurate. Based on the discussion here
>> >> <https://forums.sifive.com/t/possible-bug-in-counteren-csrs/2123> the
>> >> HiFive Unleashed actually does support reading the timer CSR from
>> >> unprivileged modes (from that discussion it does so a little too well...
>> >> but it should presumably be fixed in later iterations of the processor).
>> >> And even if no real hardware supported this capability, it still might make
>> >> sense to provide it in QEMU as an optimization.
>>
>> time and cycle are different registers: rdtime should trap, but rdcycle should
>> work.  The hardware traps rdtime into machine mode and emulates it via a memory
>> mapped register.  The bug in the FU540-C000 appears to be related to the
>> counter enables, not the presence of the counters.
>
> I don't think rdtime is required to mandatorily trap.
>
> Per privileged spec v1.10, chapter 3.1.17 Counter-Enable Registers
> ([m|h|s]counteren), it says:
>
> "When the CY, TM, IR, or HPMn bit in the mcounteren register is clear,
> attempts to read the cycle, time, instret, or hpmcountern register
> while executing in S-mode or U-mode will cause an illegal instruction
> exception."
>
> In the same chapter, it also says:
>
> "Implementations can convert reads of the time CSR into loads to the
> memory-mapped mtime register, or hard-wire the TM bits in mxcounteren
> to 0 and emulate this functionality in M-mode software."
>
> So per my understanding when mcounteren.TM is set, rdtime should NOT trap.

Implementations are allowed to implement the instruction, but all the exsting
implementations trap to M-mode.
Palmer Dabbelt June 26, 2019, 8:25 a.m. UTC | #8 
On Tue, 25 Jun 2019 23:58:34 PDT (-0700), bmeng.cn@gmail.com wrote:
> On Wed, Jun 26, 2019 at 4:23 AM Jonathan Behrens <fintelia@gmail.com> wrote:
>>
>> I just did some testing on a HiFive Unleashed board and can confirm what
>> you are saying. The low 5 bits of both mcounteren and scounteren are
>> writable (if you try to write 0xFFFFFFFF to them, they'll take on the value
>> 0x1F) but even with the TM bit set in both mcounteren and scounteren the
>> rdtime instruction always generates an illegal instruction exception.
>>
>
> Then I would think the FU540 is not spec complaint :)

Ya, it's an errata.  There's a handful of them :)

>> Reading through the relevant chapter of the spec, I still think that having
>> mcounteren.TM be writable but making rdtime unconditionally trap is
>> non-conformant. If other people feel strongly that rdtime should always
>
> Agree. To test hardware (FU540) compatibility in QEMU, maybe we can
> add a cpu property to allow hard-wiring mcounteren.TM to zero?

In theory we should have properties to control the behavior of all WARL fields,
but it's a lot of work.  I'd be happy to take a patch for any of them.

>> require trapping into firmware then the natural change would be to simply
>> hardwire mcounteren.TM to zero (the value in scounteren wouldn't matter in
>> that case so it could be left writable). My own (biased) personal feeling
>> is that this full implementation makes sense at least for the `virt`
>> machine type because it represents a clear case where deviating from
>> current hardware enables a performance boost, and would not break
>> compatibility with any current software: both OpenSBI and BBL try to enable
>> hardware handling of rdtime when the platform claims to support it.
>>
>
> Regards,
> Bin
Alistair Francis June 27, 2019, 7:56 p.m. UTC | #9 
On Wed, Jun 26, 2019 at 1:25 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Tue, 25 Jun 2019 23:58:34 PDT (-0700), bmeng.cn@gmail.com wrote:
> > On Wed, Jun 26, 2019 at 4:23 AM Jonathan Behrens <fintelia@gmail.com> wrote:
> >>
> >> I just did some testing on a HiFive Unleashed board and can confirm what
> >> you are saying. The low 5 bits of both mcounteren and scounteren are
> >> writable (if you try to write 0xFFFFFFFF to them, they'll take on the value
> >> 0x1F) but even with the TM bit set in both mcounteren and scounteren the
> >> rdtime instruction always generates an illegal instruction exception.
> >>
> >
> > Then I would think the FU540 is not spec complaint :)
>
> Ya, it's an errata.  There's a handful of them :)
>
> >> Reading through the relevant chapter of the spec, I still think that having
> >> mcounteren.TM be writable but making rdtime unconditionally trap is
> >> non-conformant. If other people feel strongly that rdtime should always
> >
> > Agree. To test hardware (FU540) compatibility in QEMU, maybe we can
> > add a cpu property to allow hard-wiring mcounteren.TM to zero?
>
> In theory we should have properties to control the behavior of all WARL fields,
> but it's a lot of work.  I'd be happy to take a patch for any of them.

Hmmm... We should avoid taking patches that don't adhere to the spec
just to match some hardware. In the case that core/popular software
doesn't work it probably makes sense, but in general it's probably not
the best move.

Alistair

>
> >> require trapping into firmware then the natural change would be to simply
> >> hardwire mcounteren.TM to zero (the value in scounteren wouldn't matter in
> >> that case so it could be left writable). My own (biased) personal feeling
> >> is that this full implementation makes sense at least for the `virt`
> >> machine type because it represents a clear case where deviating from
> >> current hardware enables a performance boost, and would not break
> >> compatibility with any current software: both OpenSBI and BBL try to enable
> >> hardware handling of rdtime when the platform claims to support it.
> >>
> >
> > Regards,
> > Bin
>
Palmer Dabbelt June 28, 2019, 6:12 a.m. UTC | #10 
On Thu, 27 Jun 2019 12:56:57 PDT (-0700), alistair23@gmail.com wrote:
> On Wed, Jun 26, 2019 at 1:25 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Tue, 25 Jun 2019 23:58:34 PDT (-0700), bmeng.cn@gmail.com wrote:
>> > On Wed, Jun 26, 2019 at 4:23 AM Jonathan Behrens <fintelia@gmail.com> wrote:
>> >>
>> >> I just did some testing on a HiFive Unleashed board and can confirm what
>> >> you are saying. The low 5 bits of both mcounteren and scounteren are
>> >> writable (if you try to write 0xFFFFFFFF to them, they'll take on the value
>> >> 0x1F) but even with the TM bit set in both mcounteren and scounteren the
>> >> rdtime instruction always generates an illegal instruction exception.
>> >>
>> >
>> > Then I would think the FU540 is not spec complaint :)
>>
>> Ya, it's an errata.  There's a handful of them :)
>>
>> >> Reading through the relevant chapter of the spec, I still think that having
>> >> mcounteren.TM be writable but making rdtime unconditionally trap is
>> >> non-conformant. If other people feel strongly that rdtime should always
>> >
>> > Agree. To test hardware (FU540) compatibility in QEMU, maybe we can
>> > add a cpu property to allow hard-wiring mcounteren.TM to zero?
>>
>> In theory we should have properties to control the behavior of all WARL fields,
>> but it's a lot of work.  I'd be happy to take a patch for any of them.
>
> Hmmm... We should avoid taking patches that don't adhere to the spec
> just to match some hardware. In the case that core/popular software
> doesn't work it probably makes sense, but in general it's probably not
> the best move.

WARL is Write Any Read Legal.  Essentially it means that the hardware gets to
decide what sort of behavior that field has, and is the mechanism for optional
features in the ISA.  In this case it'd be entirely within spec, specifically:

    Registers mcounteren and scounteren are WARL registers that must be
    implemented if U-mode and S-mode are implemented. Any of the bits may
    contain a hardwired value of zero, indicating reads to the corresponding
    counter will cause an illegal instruction exception when executing in a
    less-privileged mode.

Taking a patch that matches the out-of-spec FU540 behavior doesn't make any
sense, I don't want to implement errata in QEMU :)

>
> Alistair
>
>>
>> >> require trapping into firmware then the natural change would be to simply
>> >> hardwire mcounteren.TM to zero (the value in scounteren wouldn't matter in
>> >> that case so it could be left writable). My own (biased) personal feeling
>> >> is that this full implementation makes sense at least for the `virt`
>> >> machine type because it represents a clear case where deviating from
>> >> current hardware enables a performance boost, and would not break
>> >> compatibility with any current software: both OpenSBI and BBL try to enable
>> >> hardware handling of rdtime when the platform claims to support it.
>> >>
>> >
>> > Regards,
>> > Bin
>>
Alistair Francis June 28, 2019, 6:23 p.m. UTC | #11 
On Thu, Jun 27, 2019 at 11:12 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Thu, 27 Jun 2019 12:56:57 PDT (-0700), alistair23@gmail.com wrote:
> > On Wed, Jun 26, 2019 at 1:25 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>
> >> On Tue, 25 Jun 2019 23:58:34 PDT (-0700), bmeng.cn@gmail.com wrote:
> >> > On Wed, Jun 26, 2019 at 4:23 AM Jonathan Behrens <fintelia@gmail.com> wrote:
> >> >>
> >> >> I just did some testing on a HiFive Unleashed board and can confirm what
> >> >> you are saying. The low 5 bits of both mcounteren and scounteren are
> >> >> writable (if you try to write 0xFFFFFFFF to them, they'll take on the value
> >> >> 0x1F) but even with the TM bit set in both mcounteren and scounteren the
> >> >> rdtime instruction always generates an illegal instruction exception.
> >> >>
> >> >
> >> > Then I would think the FU540 is not spec complaint :)
> >>
> >> Ya, it's an errata.  There's a handful of them :)
> >>
> >> >> Reading through the relevant chapter of the spec, I still think that having
> >> >> mcounteren.TM be writable but making rdtime unconditionally trap is
> >> >> non-conformant. If other people feel strongly that rdtime should always
> >> >
> >> > Agree. To test hardware (FU540) compatibility in QEMU, maybe we can
> >> > add a cpu property to allow hard-wiring mcounteren.TM to zero?
> >>
> >> In theory we should have properties to control the behavior of all WARL fields,
> >> but it's a lot of work.  I'd be happy to take a patch for any of them.
> >
> > Hmmm... We should avoid taking patches that don't adhere to the spec
> > just to match some hardware. In the case that core/popular software
> > doesn't work it probably makes sense, but in general it's probably not
> > the best move.
>
> WARL is Write Any Read Legal.  Essentially it means that the hardware gets to
> decide what sort of behavior that field has, and is the mechanism for optional
> features in the ISA.  In this case it'd be entirely within spec, specifically:
>
>     Registers mcounteren and scounteren are WARL registers that must be
>     implemented if U-mode and S-mode are implemented. Any of the bits may
>     contain a hardwired value of zero, indicating reads to the corresponding
>     counter will cause an illegal instruction exception when executing in a
>     less-privileged mode.
>
> Taking a patch that matches the out-of-spec FU540 behavior doesn't make any
> sense, I don't want to implement errata in QEMU :)

Ah, I misread your email. That is fine then :)

Alistair

>
> >
> > Alistair
> >
> >>
> >> >> require trapping into firmware then the natural change would be to simply
> >> >> hardwire mcounteren.TM to zero (the value in scounteren wouldn't matter in
> >> >> that case so it could be left writable). My own (biased) personal feeling
> >> >> is that this full implementation makes sense at least for the `virt`
> >> >> machine type because it represents a clear case where deviating from
> >> >> current hardware enables a performance boost, and would not break
> >> >> compatibility with any current software: both OpenSBI and BBL try to enable
> >> >> hardware handling of rdtime when the platform claims to support it.
> >> >>
> >> >
> >> > Regards,
> >> > Bin
> >>
Jonathan Behrens June 28, 2019, 8:14 p.m. UTC | #12 
> Taking a patch that matches the out-of-spec FU540 behavior doesn't make
any
> sense, I don't want to implement errata in QEMU :)

QEMU actually currently matches the out of spec behavior of the FU540, I
just sent out a patch to fix this.

Jonathan

On Fri, Jun 28, 2019 at 2:26 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Thu, Jun 27, 2019 at 11:12 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> >
> > On Thu, 27 Jun 2019 12:56:57 PDT (-0700), alistair23@gmail.com wrote:
> > > On Wed, Jun 26, 2019 at 1:25 AM Palmer Dabbelt <palmer@sifive.com>
> wrote:
> > >>
> > >> On Tue, 25 Jun 2019 23:58:34 PDT (-0700), bmeng.cn@gmail.com wrote:
> > >> > On Wed, Jun 26, 2019 at 4:23 AM Jonathan Behrens <
> fintelia@gmail.com> wrote:
> > >> >>
> > >> >> I just did some testing on a HiFive Unleashed board and can
> confirm what
> > >> >> you are saying. The low 5 bits of both mcounteren and scounteren
> are
> > >> >> writable (if you try to write 0xFFFFFFFF to them, they'll take on
> the value
> > >> >> 0x1F) but even with the TM bit set in both mcounteren and
> scounteren the
> > >> >> rdtime instruction always generates an illegal instruction
> exception.
> > >> >>
> > >> >
> > >> > Then I would think the FU540 is not spec complaint :)
> > >>
> > >> Ya, it's an errata.  There's a handful of them :)
> > >>
> > >> >> Reading through the relevant chapter of the spec, I still think
> that having
> > >> >> mcounteren.TM be writable but making rdtime unconditionally trap is
> > >> >> non-conformant. If other people feel strongly that rdtime should
> always
> > >> >
> > >> > Agree. To test hardware (FU540) compatibility in QEMU, maybe we can
> > >> > add a cpu property to allow hard-wiring mcounteren.TM to zero?
> > >>
> > >> In theory we should have properties to control the behavior of all
> WARL fields,
> > >> but it's a lot of work.  I'd be happy to take a patch for any of them.
> > >
> > > Hmmm... We should avoid taking patches that don't adhere to the spec
> > > just to match some hardware. In the case that core/popular software
> > > doesn't work it probably makes sense, but in general it's probably not
> > > the best move.
> >
> > WARL is Write Any Read Legal.  Essentially it means that the hardware
> gets to
> > decide what sort of behavior that field has, and is the mechanism for
> optional
> > features in the ISA.  In this case it'd be entirely within spec,
> specifically:
> >
> >     Registers mcounteren and scounteren are WARL registers that must be
> >     implemented if U-mode and S-mode are implemented. Any of the bits may
> >     contain a hardwired value of zero, indicating reads to the
> corresponding
> >     counter will cause an illegal instruction exception when executing
> in a
> >     less-privileged mode.
> >
> > Taking a patch that matches the out-of-spec FU540 behavior doesn't make
> any
> > sense, I don't want to implement errata in QEMU :)
>
> Ah, I misread your email. That is fine then :)
>
> Alistair
>
> >
> > >
> > > Alistair
> > >
> > >>
> > >> >> require trapping into firmware then the natural change would be to
> simply
> > >> >> hardwire mcounteren.TM to zero (the value in scounteren wouldn't
> matter in
> > >> >> that case so it could be left writable). My own (biased) personal
> feeling
> > >> >> is that this full implementation makes sense at least for the
> `virt`
> > >> >> machine type because it represents a clear case where deviating
> from
> > >> >> current hardware enables a performance boost, and would not break
> > >> >> compatibility with any current software: both OpenSBI and BBL try
> to enable
> > >> >> hardware handling of rdtime when the platform claims to support it.
> > >> >>
> > >> >
> > >> > Regards,
> > >> > Bin
> > >>
>
diff mbox series

Patch

diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index e34a26a0ef..c39cd55330 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -19,6 +19,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "qemu/timer.h"
 #include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "target/riscv/cpu.h"
@@ -27,6 +28,8 @@ 
 static Property riscv_harts_props[] = {
     DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1),
     DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
+    DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState, time_freq,
+                       NANOSECONDS_PER_SECOND),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -49,6 +52,7 @@  static void riscv_harts_realize(DeviceState *dev, Error **errp)
                                 sizeof(RISCVCPU), s->cpu_type,
                                 &error_abort, NULL);
         s->harts[n].env.mhartid = n;
+        s->harts[n].env.time_freq = s->time_freq;
         qemu_register_reset(riscv_harts_cpu_reset, &s->harts[n]);
         object_property_set_bool(OBJECT(&s->harts[n]), true,
                                  "realized", &err);
diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
index d4c159e937..71edf4dcc6 100644
--- a/hw/riscv/sifive_clint.c
+++ b/hw/riscv/sifive_clint.c
@@ -26,10 +26,10 @@ 
 #include "hw/riscv/sifive_clint.h"
 #include "qemu/timer.h"
 
-static uint64_t cpu_riscv_read_rtc(void)
+static uint64_t cpu_riscv_read_rtc(CPURISCVState *env)
 {
     return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-        SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
+        env->time_freq, NANOSECONDS_PER_SECOND);
 }
 
 /*
@@ -41,7 +41,7 @@  static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value)
     uint64_t next;
     uint64_t diff;
 
-    uint64_t rtc_r = cpu_riscv_read_rtc();
+    uint64_t rtc_r = cpu_riscv_read_rtc(&cpu->env);
 
     cpu->env.timecmp = value;
     if (cpu->env.timecmp <= rtc_r) {
@@ -56,7 +56,7 @@  static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value)
     diff = cpu->env.timecmp - rtc_r;
     /* back to ns (note args switched in muldiv64) */
     next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-        muldiv64(diff, NANOSECONDS_PER_SECOND, SIFIVE_CLINT_TIMEBASE_FREQ);
+        muldiv64(diff, NANOSECONDS_PER_SECOND, cpu->env.time_freq);
     timer_mod(cpu->env.timer, next);
 }
 
@@ -107,11 +107,25 @@  static uint64_t sifive_clint_read(void *opaque, hwaddr addr, unsigned size)
             return 0;
         }
     } else if (addr == clint->time_base) {
-        /* time_lo */
-        return cpu_riscv_read_rtc() & 0xFFFFFFFF;
+        /* All harts must have the same time frequency, so just use hart 0 */
+        CPUState *cpu = qemu_get_cpu(0);
+        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
+        if (!env) {
+            error_report("clint: hart 0 not valid?!");
+        } else {
+            /* time_lo */
+            return cpu_riscv_read_rtc(env) & 0xFFFFFFFF;
+        }
     } else if (addr == clint->time_base + 4) {
-        /* time_hi */
-        return (cpu_riscv_read_rtc() >> 32) & 0xFFFFFFFF;
+        /* All harts must have the same time frequency, so just use hart 0 */
+        CPUState *cpu = qemu_get_cpu(0);
+        CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
+        if (!env) {
+            error_report("clint: hart 0 not valid?!");
+        } else {
+            /* time_hi */
+            return (cpu_riscv_read_rtc(env) >> 32) & 0xFFFFFFFF;
+        }
     }
 
     error_report("clint: invalid read: %08x", (uint32_t)addr);
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 80ac56fa7d..2d6f768ff1 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -146,6 +146,8 @@  static void riscv_sifive_e_soc_init(Object *obj)
                             &error_abort);
     object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
                             &error_abort);
+    object_property_set_int(OBJECT(&s->cpus), SIFIVE_E_TIMEBASE_FREQ,
+                            "timebase-frequency", &error_abort);
     sysbus_init_child_obj(obj, "riscv.sifive.e.gpio0",
                           &s->gpio, sizeof(s->gpio),
                           TYPE_SIFIVE_GPIO);
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 5ecc47cea3..ab2b00106b 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -116,7 +116,7 @@  static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
 
     qemu_fdt_add_subnode(fdt, "/cpus");
     qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
-        SIFIVE_CLINT_TIMEBASE_FREQ);
+        SIFIVE_U_TIMEBASE_FREQ);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
 
@@ -329,6 +329,8 @@  static void riscv_sifive_u_soc_init(Object *obj)
                             &error_abort);
     object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
                             &error_abort);
+    object_property_set_int(OBJECT(&s->cpus), SIFIVE_U_TIMEBASE_FREQ,
+                            "timebase-frequency", &error_abort);
 
     sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
                           TYPE_CADENCE_GEM);
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 5b33d4be3b..7c1872aad0 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -106,7 +106,7 @@  static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
 
     qemu_fdt_add_subnode(fdt, "/cpus");
     qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
-        SIFIVE_CLINT_TIMEBASE_FREQ);
+        SPIKE_TIMEBASE_FREQ);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
 
@@ -180,6 +180,8 @@  static void spike_board_init(MachineState *machine)
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
                             &error_abort);
+    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
+                            "timebase-frequency", &error_abort);
     object_property_set_bool(OBJECT(&s->soc), true, "realized",
                             &error_abort);
 
@@ -268,6 +270,8 @@  static void spike_v1_10_0_board_init(MachineState *machine)
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
                             &error_abort);
+    object_property_set_int(OBJECT(&s->soc), SPIKE_TIMEBASE_FREQ,
+                            "timebase-frequency", &error_abort);
     object_property_set_bool(OBJECT(&s->soc), true, "realized",
                             &error_abort);
 
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 84d94d0c42..e3a2474cea 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -185,7 +185,7 @@  static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
 
     qemu_fdt_add_subnode(fdt, "/cpus");
     qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
-                          SIFIVE_CLINT_TIMEBASE_FREQ);
+                          VIRT_TIMEBASE_FREQ);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
 
@@ -403,6 +403,8 @@  static void riscv_virt_board_init(MachineState *machine)
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
                             &error_abort);
+    object_property_set_int(OBJECT(&s->soc), VIRT_TIMEBASE_FREQ,
+                            "timebase-frequency", &error_abort);
     object_property_set_bool(OBJECT(&s->soc), true, "realized",
                             &error_abort);
 
diff --git a/include/hw/riscv/riscv_hart.h b/include/hw/riscv/riscv_hart.h
index 0671d88a44..2e82e85b2b 100644
--- a/include/hw/riscv/riscv_hart.h
+++ b/include/hw/riscv/riscv_hart.h
@@ -33,6 +33,7 @@  typedef struct RISCVHartArrayState {
     /*< public >*/
     uint32_t num_harts;
     char *cpu_type;
+    uint64_t time_freq;
     RISCVCPU *harts;
 } RISCVHartArrayState;
 
diff --git a/include/hw/riscv/sifive_clint.h b/include/hw/riscv/sifive_clint.h
index e2865be1d1..aaa2a58c6e 100644
--- a/include/hw/riscv/sifive_clint.h
+++ b/include/hw/riscv/sifive_clint.h
@@ -47,8 +47,4 @@  enum {
     SIFIVE_TIME_BASE    = 0xBFF8
 };
 
-enum {
-    SIFIVE_CLINT_TIMEBASE_FREQ = 10000000
-};
-
 #endif
diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
index 3b14eb7462..95bd33d872 100644
--- a/include/hw/riscv/sifive_e.h
+++ b/include/hw/riscv/sifive_e.h
@@ -71,6 +71,10 @@  enum {
     SIFIVE_E_GPIO0_IRQ0 = 8
 };
 
+enum {
+    SIFIVE_E_TIMEBASE_FREQ = 10000000,
+};
+
 #define SIFIVE_E_PLIC_HART_CONFIG "M"
 #define SIFIVE_E_PLIC_NUM_SOURCES 127
 #define SIFIVE_E_PLIC_NUM_PRIORITIES 7
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 892f0eee21..85ee1eca78 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -63,6 +63,7 @@  enum {
 };
 
 enum {
+    SIFIVE_U_TIMEBASE_FREQ = 10000000,
     SIFIVE_U_CLOCK_FREQ = 1000000000,
     SIFIVE_U_GEM_CLOCK_FREQ = 125000000
 };
diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
index 641b70da67..2d391d1559 100644
--- a/include/hw/riscv/spike.h
+++ b/include/hw/riscv/spike.h
@@ -36,6 +36,7 @@  enum {
 };
 
 enum {
+    SPIKE_TIMEBASE_FREQ = 10000000,
     SPIKE_CLOCK_FREQ = 1000000000
 };
 
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index d01a1a85c4..53d4318f76 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -53,6 +53,7 @@  enum {
 };
 
 enum {
+    VIRT_TIMEBASE_FREQ = 10000000,
     VIRT_CLOCK_FREQ = 1000000000
 };
 
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 74e726c1c9..9c09bbbffd 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -175,7 +175,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 f9e2910643..303c362782 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 */
 
@@ -856,14 +865,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 */