On Fri, May 13, 2022 at 11:58 PM Atish Kumar Patra <ati...@rivosinc.com>
wrote:
> On Thu, May 12, 2022 at 11:29 PM Frank Chang <frank.ch...@sifive.com>
> wrote:
> >
> > On Thu, May 12, 2022 at 6:01 AM Atish Patra <ati...@rivosinc.com> wrote:
> >>
> >> From: Atish Patra <atish.pa...@wdc.com>
> >>
> >> mcycle/minstret are actually WARL registers and can be written with any
> >> given value. With SBI PMU extension, it will be used to store a initial
> >> value provided from supervisor OS. The Qemu also need prohibit the
> counter
> >> increment if mcountinhibit is set.
> >>
> >> Support mcycle/minstret through generic counter infrastructure.
> >>
> >> Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
> >> Signed-off-by: Atish Patra <atish.pa...@wdc.com>
> >> Signed-off-by: Atish Patra <ati...@rivosinc.com>
> >> ---
> >> target/riscv/cpu.h | 23 ++++--
> >> target/riscv/csr.c | 157 ++++++++++++++++++++++++++++-----------
> >> target/riscv/machine.c | 25 ++++++-
> >> target/riscv/meson.build | 3 +-
> >> target/riscv/pmu.c | 32 ++++++++
> >> target/riscv/pmu.h | 28 +++++++
> >> 6 files changed, 214 insertions(+), 54 deletions(-)
> >> create mode 100644 target/riscv/pmu.c
> >> create mode 100644 target/riscv/pmu.h
> >>
> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> index 32cdd9070be5..f60072e0fd3d 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -111,7 +111,7 @@ typedef struct CPUArchState CPURISCVState;
> >> #endif
> >>
> >> #define RV_VLEN_MAX 1024
> >> -#define RV_MAX_MHPMEVENTS 29
> >> +#define RV_MAX_MHPMEVENTS 32
> >> #define RV_MAX_MHPMCOUNTERS 32
> >>
> >> FIELD(VTYPE, VLMUL, 0, 3)
> >> @@ -121,6 +121,18 @@ FIELD(VTYPE, VMA, 7, 1)
> >> FIELD(VTYPE, VEDIV, 8, 2)
> >> FIELD(VTYPE, RESERVED, 10, sizeof(target_ulong) * 8 - 11)
> >>
> >> +typedef struct PMUCTRState {
> >> + /* Current value of a counter */
> >> + target_ulong mhpmcounter_val;
> >> + /* Current value of a counter in RV32*/
> >> + target_ulong mhpmcounterh_val;
> >> + /* Snapshot values of counter */
> >> + target_ulong mhpmcounter_prev;
> >> + /* Snapshort value of a counter in RV32 */
> >> + target_ulong mhpmcounterh_prev;
> >> + bool started;
> >> +} PMUCTRState;
> >> +
> >> struct CPUArchState {
> >> target_ulong gpr[32];
> >> target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> >> @@ -273,13 +285,10 @@ struct CPUArchState {
> >>
> >> target_ulong mcountinhibit;
> >>
> >> - /* PMU counter configured values */
> >> - target_ulong mhpmcounter_val[RV_MAX_MHPMCOUNTERS];
> >> -
> >> - /* for RV32 */
> >> - target_ulong mhpmcounterh_val[RV_MAX_MHPMCOUNTERS];
> >> + /* PMU counter state */
> >> + PMUCTRState pmu_ctrs[RV_MAX_MHPMCOUNTERS];
> >>
> >> - /* PMU event selector configured values */
> >> + /* PMU event selector configured values. First three are unused*/
> >> target_ulong mhpmevent_val[RV_MAX_MHPMEVENTS];
> >>
> >> target_ulong sscratch;
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index 87aa601e5ddb..c050ed2e2c1b 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -21,6 +21,7 @@
> >> #include "qemu/log.h"
> >> #include "qemu/timer.h"
> >> #include "cpu.h"
> >> +#include "pmu.h"
> >> #include "qemu/main-loop.h"
> >> #include "exec/exec-all.h"
> >> #include "sysemu/cpu-timers.h"
> >> @@ -597,34 +598,28 @@ static int write_vcsr(CPURISCVState *env, int
> csrno, target_ulong val)
> >> }
> >>
> >> /* User Timers and Counters */
> >> -static RISCVException read_instret(CPURISCVState *env, int csrno,
> >> - target_ulong *val)
> >> +static target_ulong get_ticks(bool shift)
> >> {
> >> + int64_t val;
> >> + target_ulong result;
> >> +
> >> #if !defined(CONFIG_USER_ONLY)
> >> if (icount_enabled()) {
> >> - *val = icount_get();
> >> + val = icount_get();
> >> } else {
> >> - *val = cpu_get_host_ticks();
> >> + val = cpu_get_host_ticks();
> >> }
> >> #else
> >> - *val = cpu_get_host_ticks();
> >> + val = cpu_get_host_ticks();
> >> #endif
> >> - return RISCV_EXCP_NONE;
> >> -}
> >>
> >> -static RISCVException read_instreth(CPURISCVState *env, int csrno,
> >> - target_ulong *val)
> >> -{
> >> -#if !defined(CONFIG_USER_ONLY)
> >> - if (icount_enabled()) {
> >> - *val = icount_get() >> 32;
> >> + if (shift) {
> >> + result = val >> 32;
> >> } else {
> >> - *val = cpu_get_host_ticks() >> 32;
> >> + result = val;
> >> }
> >> -#else
> >> - *val = cpu_get_host_ticks() >> 32;
> >> -#endif
> >> - return RISCV_EXCP_NONE;
> >> +
> >> + return result;
> >> }
> >>
> >> #if defined(CONFIG_USER_ONLY)
> >> @@ -642,11 +637,23 @@ static RISCVException read_timeh(CPURISCVState
> *env, int csrno,
> >> return RISCV_EXCP_NONE;
> >> }
> >>
> >> +static int read_hpmcounter(CPURISCVState *env, int csrno, target_ulong
> *val)
> >> +{
> >> + *val = get_ticks(false);
> >> + return RISCV_EXCP_NONE;
> >> +}
> >> +
> >> +static int read_hpmcounterh(CPURISCVState *env, int csrno,
> target_ulong *val)
> >> +{
> >> + *val = get_ticks(true);
> >> + return RISCV_EXCP_NONE;
> >> +}
> >> +
> >> #else /* CONFIG_USER_ONLY */
> >>
> >> static int read_mhpmevent(CPURISCVState *env, int csrno, target_ulong
> *val)
> >> {
> >> - int evt_index = csrno - CSR_MHPMEVENT3;
> >> + int evt_index = csrno - CSR_MCOUNTINHIBIT;
> >>
> >> *val = env->mhpmevent_val[evt_index];
> >>
> >> @@ -655,7 +662,7 @@ static int read_mhpmevent(CPURISCVState *env, int
> csrno, target_ulong *val)
> >>
> >> static int write_mhpmevent(CPURISCVState *env, int csrno, target_ulong
> val)
> >> {
> >> - int evt_index = csrno - CSR_MHPMEVENT3;
> >> + int evt_index = csrno - CSR_MCOUNTINHIBIT;
> >>
> >> env->mhpmevent_val[evt_index] = val;
> >>
> >> @@ -664,52 +671,102 @@ static int write_mhpmevent(CPURISCVState *env,
> int csrno, target_ulong val)
> >>
> >> static int write_mhpmcounter(CPURISCVState *env, int csrno,
> target_ulong val)
> >> {
> >> - int ctr_index = csrno - CSR_MHPMCOUNTER3 + 3;
> >> + int ctr_idx = csrno - CSR_MCYCLE;
> >> + PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> >>
> >> - env->mhpmcounter_val[ctr_index] = val;
> >> + counter->mhpmcounter_val = val;
> >> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> >> + riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> >> + counter->mhpmcounter_prev = get_ticks(false);
> >> + } else {
> >> + /* Other counters can keep incrementing from the given value */
> >> + counter->mhpmcounter_prev = val;
> >> + }
> >>
> >> return RISCV_EXCP_NONE;
> >> }
> >>
> >> static int write_mhpmcounterh(CPURISCVState *env, int csrno,
> target_ulong val)
> >> {
> >> - int ctr_index = csrno - CSR_MHPMCOUNTER3H + 3;
> >> + int ctr_idx = csrno - CSR_MCYCLEH;
> >> + PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> >>
> >> - env->mhpmcounterh_val[ctr_index] = val;
> >> + counter->mhpmcounterh_val = val;
> >> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> >> + riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> >> + counter->mhpmcounterh_prev = get_ticks(true);
> >> + } else {
> >> + counter->mhpmcounterh_prev = val;
> >> + }
> >> +
> >> + return RISCV_EXCP_NONE;
> >> +}
> >> +
> >> +static RISCVException riscv_pmu_read_ctr(CPURISCVState *env,
> target_ulong *val,
> >> + bool upper_half, uint32_t
> ctr_idx)
> >> +{
> >> + PMUCTRState counter = env->pmu_ctrs[ctr_idx];
> >> + target_ulong ctr_prev = upper_half ? counter.mhpmcounterh_prev :
> >> + counter.mhpmcounter_prev;
> >> + target_ulong ctr_val = upper_half ? counter.mhpmcounterh_val :
> >> + counter.mhpmcounter_val;
> >> +
> >> + if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
> >> + /**
> >> + * Counter should not increment if inhibit bit is set. We
> can't really
> >> + * stop the icount counting. Just return the counter value
> written by
> >> + * the supervisor to indicate that counter was not incremented.
> >> + */
> >> + if (!counter.started) {
> >> + *val = ctr_val;
> >> + return RISCV_EXCP_NONE;
> >> + } else {
> >> + /* Mark that the counter has been stopped */
> >> + counter.started = false;
> >> + }
> >> + }
> >> +
> >> +
> >> + /**
> >> + * The kernel computes the perf delta by subtracting the current
> value from
> >> + * the value it initialized previously (ctr_val).
> >> + */
> >> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> >> + riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> >> + *val = get_ticks(upper_half) - ctr_prev + ctr_val;
> >> + } else {
> >> + *val = ctr_val;
> >> + }
> >>
> >> return RISCV_EXCP_NONE;
> >> }
> >>
> >> static int read_hpmcounter(CPURISCVState *env, int csrno, target_ulong
> *val)
> >> {
> >> - int ctr_index;
> >> + uint16_t ctr_index;
> >>
> >> if (env->priv == PRV_M) {
> >> - ctr_index = csrno - CSR_MHPMCOUNTER3 + 3;
> >> + ctr_index = csrno - CSR_MCYCLE;
> >>
> >> } else {
> >> - ctr_index = csrno - CSR_HPMCOUNTER3 + 3;
> >> + ctr_index = csrno - CSR_CYCLE;
> >> }
> >
> >
> > Hi Atish,
> >
> > According to spec:
> > "The RDCYCLE pseudoinstruction reads the low XLEN bits of the cycle CSR
> > which holds a count of the number of clock cycles executed by the
> > processor core on which the hart is running from an arbitrary start time
> in the past."
> >
> > However, the counter index calculation here would have the issue.
> > For example, if RDCYCLE instruction is executed in M-mode:
> > CYCLE csrno is: 0xc00, but MCYCLE csrno is: 0xb00
> > "csrno - CSR_MCYCLE" would result in the invalidate value: 0x100,
> > which is out of the counter array's bound.
> >
>
> Why does somebody want to invoke RDCYCLE in M-mode ? IIRC, it's not
> used anywhere
> currently. I understand rdtime use case but don't know why do you want
> to run RDCYCLE.
>
Hi Atish,
That's just a random program and I think it tries to be portable in both
U-mode and M-mode.
Does RISC-V spec have any limitation on executing RDCYCLE instruction in
M-mode?
Regards,
Frank Chang
> > Will it be easier to just check against csrno instead of the current
> privileged mode?
> >
>
> Anyways, we can move to csrno range check to calculate the offset
> instead of the priv mode.
> I am fine with that. I just want to understand the use case. If it is
> just for completeness and cover
> edge cases, that's fine too.
>
> > And I think the same issue also occurs for:
> > HPMCOUNTER3 ~ HPMCOUNTER31.
>
> Again what is the use case for reading HPMCOUNTER3-HPMCOUNTER31 in M-mode
> when you MHPMCOUNTERX available ?
>
> >
> > Regards,
> > Frank Chang
> >
> >>
> >> - *val = env->mhpmcounter_val[ctr_index];
> >>
> >> - return RISCV_EXCP_NONE;
> >> + return riscv_pmu_read_ctr(env, val, false, ctr_index);
> >> }
> >>
> >> static int read_hpmcounterh(CPURISCVState *env, int csrno,
> target_ulong *val)
> >> {
> >> - int ctr_index;
> >> + uint16_t ctr_index;
> >>
> >> if (env->priv == PRV_M) {
> >> - ctr_index = csrno - CSR_MHPMCOUNTER3H + 3;
> >> + ctr_index = csrno - CSR_MCYCLEH;
> >> } else {
> >> - ctr_index = csrno - CSR_HPMCOUNTER3H + 3;
> >> + ctr_index = csrno - CSR_CYCLEH;
> >> }
> >>
> >> - *val = env->mhpmcounterh_val[ctr_index];
> >> -
> >> - return RISCV_EXCP_NONE;
> >> + return riscv_pmu_read_ctr(env, val, true, ctr_index);
> >> }
> >>
> >> -
> >> static RISCVException read_time(CPURISCVState *env, int csrno,
> >> target_ulong *val)
> >> {
> >> @@ -1564,11 +1621,23 @@ static RISCVException
> read_mcountinhibit(CPURISCVState *env, int csrno,
> >> static RISCVException write_mcountinhibit(CPURISCVState *env, int
> csrno,
> >> target_ulong val)
> >> {
> >> + int cidx;
> >> + PMUCTRState *counter;
> >> +
> >> if (env->priv_ver < PRIV_VERSION_1_11_0) {
> >> return RISCV_EXCP_ILLEGAL_INST;
> >> }
> >>
> >> env->mcountinhibit = val;
> >> +
> >> + /* Check if any other counter is also monitoring
> cycles/instructions */
> >> + for (cidx = 0; cidx < RV_MAX_MHPMCOUNTERS; cidx++) {
> >> + if (!get_field(env->mcountinhibit, BIT(cidx))) {
> >> + counter = &env->pmu_ctrs[cidx];
> >> + counter->started = true;
> >> + }
> >> + }
> >> +
> >> return RISCV_EXCP_NONE;
> >> }
> >>
> >> @@ -3526,10 +3595,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >> [CSR_VLENB] = { "vlenb", vs, read_vlenb,
> >> .min_priv_ver =
> PRIV_VERSION_1_12_0 },
> >> /* User Timers and Counters */
> >> - [CSR_CYCLE] = { "cycle", ctr, read_instret },
> >> - [CSR_INSTRET] = { "instret", ctr, read_instret },
> >> - [CSR_CYCLEH] = { "cycleh", ctr32, read_instreth },
> >> - [CSR_INSTRETH] = { "instreth", ctr32, read_instreth },
> >> + [CSR_CYCLE] = { "cycle", ctr, read_hpmcounter },
> >> + [CSR_INSTRET] = { "instret", ctr, read_hpmcounter },
> >> + [CSR_CYCLEH] = { "cycleh", ctr32, read_hpmcounterh },
> >> + [CSR_INSTRETH] = { "instreth", ctr32, read_hpmcounterh },
> >>
> >> /*
> >> * In privileged mode, the monitor will have to emulate TIME CSRs
> only if
> >> @@ -3543,10 +3612,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >>
> >> #if !defined(CONFIG_USER_ONLY)
> >> /* Machine Timers and Counters */
> >> - [CSR_MCYCLE] = { "mcycle", any, read_instret },
> >> - [CSR_MINSTRET] = { "minstret", any, read_instret },
> >> - [CSR_MCYCLEH] = { "mcycleh", any32, read_instreth },
> >> - [CSR_MINSTRETH] = { "minstreth", any32, read_instreth },
> >> + [CSR_MCYCLE] = { "mcycle", any, read_hpmcounter,
> write_mhpmcounter},
> >> + [CSR_MINSTRET] = { "minstret", any, read_hpmcounter,
> write_mhpmcounter},
> >> + [CSR_MCYCLEH] = { "mcycleh", any32, read_hpmcounterh,
> write_mhpmcounterh},
> >> + [CSR_MINSTRETH] = { "minstreth", any32, read_hpmcounterh,
> write_mhpmcounterh},
> >>
> >> /* Machine Information Registers */
> >> [CSR_MVENDORID] = { "mvendorid", any, read_mvendorid },
> >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> >> index 99193c85bb97..dc182ca81119 100644
> >> --- a/target/riscv/machine.c
> >> +++ b/target/riscv/machine.c
> >> @@ -279,7 +279,28 @@ static const VMStateDescription vmstate_envcfg = {
> >> VMSTATE_UINT64(env.menvcfg, RISCVCPU),
> >> VMSTATE_UINTTL(env.senvcfg, RISCVCPU),
> >> VMSTATE_UINT64(env.henvcfg, RISCVCPU),
> >> + VMSTATE_END_OF_LIST()
> >> + }
> >> +};
> >> +
> >> +static bool pmu_needed(void *opaque)
> >> +{
> >> + RISCVCPU *cpu = opaque;
> >>
> >> + return cpu->cfg.pmu_num;
> >> +}
> >> +
> >> +static const VMStateDescription vmstate_pmu_ctr_state = {
> >> + .name = "cpu/pmu",
> >> + .version_id = 1,
> >> + .minimum_version_id = 1,
> >> + .needed = pmu_needed,
> >> + .fields = (VMStateField[]) {
> >> + VMSTATE_UINTTL(mhpmcounter_val, PMUCTRState),
> >> + VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
> >> + VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
> >> + VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
> >> + VMSTATE_BOOL(started, PMUCTRState),
> >> VMSTATE_END_OF_LIST()
> >> }
> >> };
> >> @@ -331,8 +352,8 @@ const VMStateDescription vmstate_riscv_cpu = {
> >> VMSTATE_UINTTL(env.scounteren, RISCVCPU),
> >> VMSTATE_UINTTL(env.mcounteren, RISCVCPU),
> >> VMSTATE_UINTTL(env.mcountinhibit, RISCVCPU),
> >> - VMSTATE_UINTTL_ARRAY(env.mhpmcounter_val, RISCVCPU,
> RV_MAX_MHPMCOUNTERS),
> >> - VMSTATE_UINTTL_ARRAY(env.mhpmcounterh_val, RISCVCPU,
> RV_MAX_MHPMCOUNTERS),
> >> + VMSTATE_STRUCT_ARRAY(env.pmu_ctrs, RISCVCPU,
> RV_MAX_MHPMCOUNTERS, 0,
> >> + vmstate_pmu_ctr_state, PMUCTRState),
> >> VMSTATE_UINTTL_ARRAY(env.mhpmevent_val, RISCVCPU,
> RV_MAX_MHPMEVENTS),
> >> VMSTATE_UINTTL(env.sscratch, RISCVCPU),
> >> VMSTATE_UINTTL(env.mscratch, RISCVCPU),
> >> diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> >> index 096249f3a30f..2c1975e72c4e 100644
> >> --- a/target/riscv/meson.build
> >> +++ b/target/riscv/meson.build
> >> @@ -30,7 +30,8 @@ riscv_softmmu_ss.add(files(
> >> 'pmp.c',
> >> 'debug.c',
> >> 'monitor.c',
> >> - 'machine.c'
> >> + 'machine.c',
> >> + 'pmu.c'
> >> ))
> >>
> >> target_arch += {'riscv': riscv_ss}
> >> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> >> new file mode 100644
> >> index 000000000000..000fe8da45ef
> >> --- /dev/null
> >> +++ b/target/riscv/pmu.c
> >> @@ -0,0 +1,32 @@
> >> +/*
> >> + * RISC-V PMU file.
> >> + *
> >> + * Copyright (c) 2021 Western Digital Corporation or its affiliates.
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> modify it
> >> + * under the terms and conditions of the GNU General Public License,
> >> + * version 2 or later, as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> or
> >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> License for
> >> + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> along with
> >> + * this program. If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "cpu.h"
> >> +#include "pmu.h"
> >> +
> >> +bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
> >> + uint32_t target_ctr)
> >> +{
> >> + return (target_ctr == 0) ? true : false;
> >> +}
> >> +
> >> +bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, uint32_t
> target_ctr)
> >> +{
> >> + return (target_ctr == 2) ? true : false;
> >> +}
> >> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> >> new file mode 100644
> >> index 000000000000..58a5bc3a4089
> >> --- /dev/null
> >> +++ b/target/riscv/pmu.h
> >> @@ -0,0 +1,28 @@
> >> +/*
> >> + * RISC-V PMU header file.
> >> + *
> >> + * Copyright (c) 2021 Western Digital Corporation or its affiliates.
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> modify it
> >> + * under the terms and conditions of the GNU General Public License,
> >> + * version 2 or later, as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> or
> >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> License for
> >> + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> along with
> >> + * this program. If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "qemu/log.h"
> >> +#include "cpu.h"
> >> +#include "qemu/main-loop.h"
> >> +#include "exec/exec-all.h"
> >> +
> >> +bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
> >> + uint32_t target_ctr);
> >> +bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env,
> >> + uint32_t target_ctr);
> >> --
> >> 2.25.1
> >>
> >>
>
