1. Patchew
  2. QEMU
  3. View series

[PATCH v2] target/riscv: fix instructions count handling in icount mode

Clément Léger posted 1 patch 5 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240618112649.76683-1-cleger@rivosinc.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bmeng.cn@gmail.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
target/riscv/csr.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
[PATCH v2] target/riscv: fix instructions count handling in icount mode
Posted by Clément Léger 5 months, 1 week ago 
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>

---

v2:
 - Apply checkpatch and fixed missing braces

---
 target/riscv/csr.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 58ef7079dc..b8915e32a2 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -762,14 +762,18 @@ 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 +808,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 +879,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 +906,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 +930,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 +951,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.45.2
Re: [PATCH v2] target/riscv: fix instructions count handling in icount mode
Posted by Alistair Francis 5 months ago 
On Tue, Jun 18, 2024 at 9:28 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

>
> ---
>
> v2:
>  - Apply checkpatch and fixed missing braces
>
> ---
>  target/riscv/csr.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 58ef7079dc..b8915e32a2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -762,14 +762,18 @@ 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 +808,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 +879,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 +906,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 +930,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 +951,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.45.2
>
>
Re: [PATCH v2] target/riscv: fix instructions count handling in icount mode
Posted by Atish Kumar Patra 5 months, 1 week ago 
On Tue, Jun 18, 2024 at 4:27 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>
>
> ---
>
> v2:
>  - Apply checkpatch and fixed missing braces
>

As the changes were minor, you can keep the RB tag. Anyways,

Reviewed-by: Atish Patra <atishp@rivosinc.com>

> ---
>  target/riscv/csr.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 58ef7079dc..b8915e32a2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -762,14 +762,18 @@ 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 +808,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 +879,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 +906,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 +930,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 +951,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.45.2
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.