On Fri, May 13, 2022 at 11:58 PM Atish Kumar Patra wrote: > On Thu, May 12, 2022 at 11:29 PM Frank Chang > wrote: > > > > On Thu, May 12, 2022 at 6:01 AM Atish Patra wrote: > >> > >> From: Atish Patra > >> > >> 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 > >> Signed-off-by: Atish Patra > >> Signed-off-by: Atish Patra > >> --- > >> 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 . > >> + */ > >> + > >> +#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 . > >> + */ > >> + > >> +#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 > >> > >> >