* Re: [PATCH] target/riscv: fix instructions count handling in icount mode
2024-04-11 11:34 [PATCH] target/riscv: fix instructions count handling in icount mode Clément Léger
@ 2024-04-11 17:22 ` Atish Kumar Patra
2024-04-29 2:37 ` Alistair Francis
2024-05-27 23:47 ` Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Atish Kumar Patra @ 2024-04-11 17:22 UTC (permalink / raw)
To: Clément Léger
Cc: qemu-riscv, Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, qemu-devel
On Thu, Apr 11, 2024 at 4:34 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> When icount is enabled, rather than returning the virtual CPU time, we
> should return the instruction count itself. Add an instructions bool
> parameter to get_ticks() to correctly return icount_get_raw() when
> icount_enabled() == 1 and instruction count is queried. This will modify
> the existing behavior which was returning an instructions count close to
> the number of cycles (CPI ~= 1).
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>
> ---
> target/riscv/csr.c | 29 ++++++++++++++++-------------
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 726096444f..5f1dcee102 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -762,14 +762,17 @@ static RISCVException write_vcsr(CPURISCVState *env, int csrno,
> }
>
> /* User Timers and Counters */
> -static target_ulong get_ticks(bool shift)
> +static target_ulong get_ticks(bool shift, bool instructions)
> {
> int64_t val;
> target_ulong result;
>
> #if !defined(CONFIG_USER_ONLY)
> if (icount_enabled()) {
> - val = icount_get();
> + if (instructions)
> + val = icount_get_raw();
> + else
> + val = icount_get();
> } else {
> val = cpu_get_host_ticks();
> }
> @@ -804,14 +807,14 @@ static RISCVException read_timeh(CPURISCVState *env, int csrno,
> static RISCVException read_hpmcounter(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> - *val = get_ticks(false);
> + *val = get_ticks(false, (csrno == CSR_INSTRET));
> return RISCV_EXCP_NONE;
> }
>
> static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> - *val = get_ticks(true);
> + *val = get_ticks(true, (csrno == CSR_INSTRETH));
> return RISCV_EXCP_NONE;
> }
>
> @@ -875,11 +878,11 @@ static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> int ctr_idx = csrno - CSR_MCYCLE;
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = val;
> + bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> 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);
> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> + counter->mhpmcounter_prev = get_ticks(false, instr);
> if (ctr_idx > 2) {
> if (riscv_cpu_mxl(env) == MXL_RV32) {
> mhpmctr_val = mhpmctr_val |
> @@ -902,12 +905,12 @@ static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = counter->mhpmcounter_val;
> uint64_t mhpmctrh_val = val;
> + bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> counter->mhpmcounterh_val = val;
> mhpmctr_val = mhpmctr_val | (mhpmctrh_val << 32);
> - if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> - riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> - counter->mhpmcounterh_prev = get_ticks(true);
> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> + counter->mhpmcounterh_prev = get_ticks(true, instr);
> if (ctr_idx > 2) {
> riscv_pmu_setup_timer(env, mhpmctr_val, ctr_idx);
> }
> @@ -926,6 +929,7 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> counter->mhpmcounter_prev;
> target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
> counter->mhpmcounter_val;
> + bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
> /*
> @@ -946,9 +950,8 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> * 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;
> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> + *val = get_ticks(upper_half, instr) - ctr_prev + ctr_val;
> } else {
> *val = ctr_val;
> }
> --
> 2.43.0
>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] target/riscv: fix instructions count handling in icount mode
2024-04-11 11:34 [PATCH] target/riscv: fix instructions count handling in icount mode Clément Léger
2024-04-11 17:22 ` Atish Kumar Patra
@ 2024-04-29 2:37 ` Alistair Francis
2024-05-27 23:47 ` Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2024-04-29 2:37 UTC (permalink / raw)
To: Clément Léger
Cc: qemu-riscv, Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, qemu-devel, Atish Patra
On Thu, Apr 11, 2024 at 9:34 PM Clément Léger <cleger@rivosinc.com> wrote:
>
> When icount is enabled, rather than returning the virtual CPU time, we
> should return the instruction count itself. Add an instructions bool
> parameter to get_ticks() to correctly return icount_get_raw() when
> icount_enabled() == 1 and instruction count is queried. This will modify
> the existing behavior which was returning an instructions count close to
> the number of cycles (CPI ~= 1).
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
Thanks!
Applied to riscv-to-apply.next
Alistair
>
> ---
> target/riscv/csr.c | 29 ++++++++++++++++-------------
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 726096444f..5f1dcee102 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -762,14 +762,17 @@ static RISCVException write_vcsr(CPURISCVState *env, int csrno,
> }
>
> /* User Timers and Counters */
> -static target_ulong get_ticks(bool shift)
> +static target_ulong get_ticks(bool shift, bool instructions)
> {
> int64_t val;
> target_ulong result;
>
> #if !defined(CONFIG_USER_ONLY)
> if (icount_enabled()) {
> - val = icount_get();
> + if (instructions)
> + val = icount_get_raw();
> + else
> + val = icount_get();
> } else {
> val = cpu_get_host_ticks();
> }
> @@ -804,14 +807,14 @@ static RISCVException read_timeh(CPURISCVState *env, int csrno,
> static RISCVException read_hpmcounter(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> - *val = get_ticks(false);
> + *val = get_ticks(false, (csrno == CSR_INSTRET));
> return RISCV_EXCP_NONE;
> }
>
> static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> - *val = get_ticks(true);
> + *val = get_ticks(true, (csrno == CSR_INSTRETH));
> return RISCV_EXCP_NONE;
> }
>
> @@ -875,11 +878,11 @@ static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> int ctr_idx = csrno - CSR_MCYCLE;
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = val;
> + bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> 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);
> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> + counter->mhpmcounter_prev = get_ticks(false, instr);
> if (ctr_idx > 2) {
> if (riscv_cpu_mxl(env) == MXL_RV32) {
> mhpmctr_val = mhpmctr_val |
> @@ -902,12 +905,12 @@ static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = counter->mhpmcounter_val;
> uint64_t mhpmctrh_val = val;
> + bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> counter->mhpmcounterh_val = val;
> mhpmctr_val = mhpmctr_val | (mhpmctrh_val << 32);
> - if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> - riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> - counter->mhpmcounterh_prev = get_ticks(true);
> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> + counter->mhpmcounterh_prev = get_ticks(true, instr);
> if (ctr_idx > 2) {
> riscv_pmu_setup_timer(env, mhpmctr_val, ctr_idx);
> }
> @@ -926,6 +929,7 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> counter->mhpmcounter_prev;
> target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
> counter->mhpmcounter_val;
> + bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
> /*
> @@ -946,9 +950,8 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> * 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;
> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> + *val = get_ticks(upper_half, instr) - ctr_prev + ctr_val;
> } else {
> *val = ctr_val;
> }
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] target/riscv: fix instructions count handling in icount mode
2024-04-11 11:34 [PATCH] target/riscv: fix instructions count handling in icount mode Clément Léger
2024-04-11 17:22 ` Atish Kumar Patra
2024-04-29 2:37 ` Alistair Francis
@ 2024-05-27 23:47 ` Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2024-05-27 23:47 UTC (permalink / raw)
To: Clément Léger
Cc: qemu-riscv, Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, qemu-devel, Atish Patra
On Thu, Apr 11, 2024 at 9:34 PM Clément Léger <cleger@rivosinc.com> wrote:
>
> When icount is enabled, rather than returning the virtual CPU time, we
> should return the instruction count itself. Add an instructions bool
> parameter to get_ticks() to correctly return icount_get_raw() when
> icount_enabled() == 1 and instruction count is queried. This will modify
> the existing behavior which was returning an instructions count close to
> the number of cycles (CPI ~= 1).
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
This patch fails checkpatch
Have a look at https://www.qemu.org/docs/master/devel/submitting-a-patch.html#id32
for details on what tests to run before submitting patches
Alistair
>
> ---
> target/riscv/csr.c | 29 ++++++++++++++++-------------
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 726096444f..5f1dcee102 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -762,14 +762,17 @@ static RISCVException write_vcsr(CPURISCVState *env, int csrno,
> }
>
> /* User Timers and Counters */
> -static target_ulong get_ticks(bool shift)
> +static target_ulong get_ticks(bool shift, bool instructions)
> {
> int64_t val;
> target_ulong result;
>
> #if !defined(CONFIG_USER_ONLY)
> if (icount_enabled()) {
> - val = icount_get();
> + if (instructions)
> + val = icount_get_raw();
> + else
> + val = icount_get();
> } else {
> val = cpu_get_host_ticks();
> }
> @@ -804,14 +807,14 @@ static RISCVException read_timeh(CPURISCVState *env, int csrno,
> static RISCVException read_hpmcounter(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> - *val = get_ticks(false);
> + *val = get_ticks(false, (csrno == CSR_INSTRET));
> return RISCV_EXCP_NONE;
> }
>
> static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> - *val = get_ticks(true);
> + *val = get_ticks(true, (csrno == CSR_INSTRETH));
> return RISCV_EXCP_NONE;
> }
>
> @@ -875,11 +878,11 @@ static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> int ctr_idx = csrno - CSR_MCYCLE;
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = val;
> + bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> 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);
> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> + counter->mhpmcounter_prev = get_ticks(false, instr);
> if (ctr_idx > 2) {
> if (riscv_cpu_mxl(env) == MXL_RV32) {
> mhpmctr_val = mhpmctr_val |
> @@ -902,12 +905,12 @@ static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = counter->mhpmcounter_val;
> uint64_t mhpmctrh_val = val;
> + bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> counter->mhpmcounterh_val = val;
> mhpmctr_val = mhpmctr_val | (mhpmctrh_val << 32);
> - if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> - riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> - counter->mhpmcounterh_prev = get_ticks(true);
> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> + counter->mhpmcounterh_prev = get_ticks(true, instr);
> if (ctr_idx > 2) {
> riscv_pmu_setup_timer(env, mhpmctr_val, ctr_idx);
> }
> @@ -926,6 +929,7 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> counter->mhpmcounter_prev;
> target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
> counter->mhpmcounter_val;
> + bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
> /*
> @@ -946,9 +950,8 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> * 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;
> + if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> + *val = get_ticks(upper_half, instr) - ctr_prev + ctr_val;
> } else {
> *val = ctr_val;
> }
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread