[PATCH 0/3] Assorted fixes for PMU

[PATCH 0/3] Assorted fixes for PMU

[Atish Patra]

This series contains few miscallenous fixes related to hpmcounters
and related code. The first patch fixes an issue with cycle/instret
counters overcouting while the remaining two are more for specification
compliance.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Atish Patra (3):
      target/riscv: Save counter values during countinhibit update
      target/riscv: Enforce WARL behavior for scounteren/hcounteren
      target/riscv: Fix the predicate functions for mhpmeventhX CSRs

 target/riscv/cpu.h     |   1 -
 target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
 target/riscv/machine.c |   1 -
 3 files changed, 68 insertions(+), 45 deletions(-)
---
base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
change-id: 20240428-countinhibit_fix-c6a1c11f4375
--
Regards,
Atish patra

[PATCH 1/3] target/riscv: Save counter values during countinhibit update

[Atish Patra]

Currently, if a counter monitoring cycle/instret is stopped via
mcountinhibit we just update the state while the value is saved
during the next read. This is not accurate as the read may happen
many cycles after the counter is stopped. Ideally, the read should
return the value saved when the counter is stopped.

Thus, save the value of the counter during the inhibit update
operation and return that value during the read if corresponding bit
in mcountihibit is set.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu.h     |  1 -
 target/riscv/csr.c     | 32 ++++++++++++++++++++------------
 target/riscv/machine.c |  1 -
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 3b1a02b9449a..09bbf7ce9880 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -153,7 +153,6 @@ typedef struct PMUCTRState {
     target_ulong mhpmcounter_prev;
     /* Snapshort value of a counter in RV32 */
     target_ulong mhpmcounterh_prev;
-    bool started;
     /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
     target_ulong irq_overflow_left;
 } PMUCTRState;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 726096444fae..68ca31aff47d 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -929,17 +929,11 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *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.
+         * Counter should not increment if inhibit bit is set. Just return the
+         * current counter value.
          */
-        if (!counter->started) {
-            *val = ctr_val;
-            return RISCV_EXCP_NONE;
-        } else {
-            /* Mark that the counter has been stopped */
-            counter->started = false;
-        }
+         *val = ctr_val;
+         return RISCV_EXCP_NONE;
     }
 
     /*
@@ -1973,9 +1967,23 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
 
     /* 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;
+        if (get_field(env->mcountinhibit, BIT(cidx)) && (val & BIT(cidx))) {
+	    /*
+             * Update the counter value for cycle/instret as we can't stop the
+             * host ticks. But we should show the current value at this moment.
+             */
+            if (riscv_pmu_ctr_monitor_cycles(env, cidx) ||
+                riscv_pmu_ctr_monitor_instructions(env, cidx)) {
+                counter->mhpmcounter_val = get_ticks(false) -
+                                           counter->mhpmcounter_prev +
+                                           counter->mhpmcounter_val;
+                if (riscv_cpu_mxl(env) == MXL_RV32) {
+                    counter->mhpmcounterh_val = get_ticks(false) -
+                                                counter->mhpmcounterh_prev +
+                                                counter->mhpmcounterh_val;
+		}
+            }
         }
     }
 
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 76f2150f78b5..3e0f2dd2ce2a 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -328,7 +328,6 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
         VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
         VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
         VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
-        VMSTATE_BOOL(started, PMUCTRState),
         VMSTATE_END_OF_LIST()
     }
 };

-- 
2.34.1

[PATCH 2/3] target/riscv: Enforce WARL behavior for scounteren/hcounteren

[Atish Patra]

scounteren/hcountern are also WARL registers similar to mcountern.
Only set the bits for the available counters during the write to
preserve the WARL behavior.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/csr.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 68ca31aff47d..a01911541d67 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2843,7 +2843,11 @@ static RISCVException read_scounteren(CPURISCVState *env, int csrno,
 static RISCVException write_scounteren(CPURISCVState *env, int csrno,
                                        target_ulong val)
 {
-    env->scounteren = val;
+    RISCVCPU *cpu = env_archcpu(env);
+
+    /* WARL register - disable unavailable counters */
+    env->scounteren = val & (cpu->pmu_avail_ctrs | COUNTEREN_CY | COUNTEREN_TM |
+                             COUNTEREN_IR);
     return RISCV_EXCP_NONE;
 }
 
@@ -3475,7 +3479,11 @@ static RISCVException read_hcounteren(CPURISCVState *env, int csrno,
 static RISCVException write_hcounteren(CPURISCVState *env, int csrno,
                                        target_ulong val)
 {
-    env->hcounteren = val;
+    RISCVCPU *cpu = env_archcpu(env);
+
+    /* WARL register - disable unavailable counters */
+    env->hcounteren = val & (cpu->pmu_avail_ctrs | COUNTEREN_CY | COUNTEREN_TM |
+                             COUNTEREN_IR);
     return RISCV_EXCP_NONE;
 }
 

-- 
2.34.1

[PATCH 3/3] target/riscv: Fix the predicate functions for mhpmeventhX CSRs

[Atish Patra]

mhpmeventhX CSRs are available for RV32. The predicate function
should check that first before checking sscofpmf extension.

Fixes: 14664483457b ("target/riscv: Add sscofpmf extension support")
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/csr.c | 67 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index a01911541d67..29d3a1296539 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -227,6 +227,15 @@ static RISCVException sscofpmf(CPURISCVState *env, int csrno)
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException sscofpmf_32(CPURISCVState *env, int csrno)
+{
+    if (riscv_cpu_mxl(env) != MXL_RV32) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    return sscofpmf(env, csrno);
+}
+
 static RISCVException any(CPURISCVState *env, int csrno)
 {
     return RISCV_EXCP_NONE;
@@ -5057,91 +5066,91 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_MHPMEVENT31]    = { "mhpmevent31",    any,    read_mhpmevent,
                              write_mhpmevent                           },
 
-    [CSR_MHPMEVENT3H]    = { "mhpmevent3h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT3H]    = { "mhpmevent3h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT4H]    = { "mhpmevent4h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT4H]    = { "mhpmevent4h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT5H]    = { "mhpmevent5h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT5H]    = { "mhpmevent5h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT6H]    = { "mhpmevent6h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT6H]    = { "mhpmevent6h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT7H]    = { "mhpmevent7h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT7H]    = { "mhpmevent7h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT8H]    = { "mhpmevent8h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT8H]    = { "mhpmevent8h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT9H]    = { "mhpmevent9h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT9H]    = { "mhpmevent9h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT10H]   = { "mhpmevent10h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT10H]   = { "mhpmevent10h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT11H]   = { "mhpmevent11h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT11H]   = { "mhpmevent11h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT12H]   = { "mhpmevent12h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT12H]   = { "mhpmevent12h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT13H]   = { "mhpmevent13h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT13H]   = { "mhpmevent13h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT14H]   = { "mhpmevent14h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT14H]   = { "mhpmevent14h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT15H]   = { "mhpmevent15h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT15H]   = { "mhpmevent15h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT16H]   = { "mhpmevent16h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT16H]   = { "mhpmevent16h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT17H]   = { "mhpmevent17h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT17H]   = { "mhpmevent17h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT18H]   = { "mhpmevent18h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT18H]   = { "mhpmevent18h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT19H]   = { "mhpmevent19h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT19H]   = { "mhpmevent19h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT20H]   = { "mhpmevent20h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT20H]   = { "mhpmevent20h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT21H]   = { "mhpmevent21h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT21H]   = { "mhpmevent21h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT22H]   = { "mhpmevent22h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT22H]   = { "mhpmevent22h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT23H]   = { "mhpmevent23h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT23H]   = { "mhpmevent23h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT24H]   = { "mhpmevent24h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT24H]   = { "mhpmevent24h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT25H]   = { "mhpmevent25h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT25H]   = { "mhpmevent25h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT26H]   = { "mhpmevent26h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT26H]   = { "mhpmevent26h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT27H]   = { "mhpmevent27h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT27H]   = { "mhpmevent27h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT28H]   = { "mhpmevent28h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT28H]   = { "mhpmevent28h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT29H]   = { "mhpmevent29h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT29H]   = { "mhpmevent29h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT30H]   = { "mhpmevent30h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT30H]   = { "mhpmevent30h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
-    [CSR_MHPMEVENT31H]   = { "mhpmevent31h",    sscofpmf,  read_mhpmeventh,
+    [CSR_MHPMEVENT31H]   = { "mhpmevent31h",    sscofpmf_32,  read_mhpmeventh,
                              write_mhpmeventh,
                              .min_priv_ver = PRIV_VERSION_1_12_0        },
 

-- 
2.34.1

Re: [PATCH 1/3] target/riscv: Save counter values during countinhibit update

[Daniel Henrique Barboza]

On 4/29/24 16:28, Atish Patra wrote:
> Currently, if a counter monitoring cycle/instret is stopped via
> mcountinhibit we just update the state while the value is saved
> during the next read. This is not accurate as the read may happen
> many cycles after the counter is stopped. Ideally, the read should
> return the value saved when the counter is stopped.
> 
> Thus, save the value of the counter during the inhibit update
> operation and return that value during the read if corresponding bit
> in mcountihibit is set.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>   target/riscv/cpu.h     |  1 -
>   target/riscv/csr.c     | 32 ++++++++++++++++++++------------
>   target/riscv/machine.c |  1 -
>   3 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 3b1a02b9449a..09bbf7ce9880 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -153,7 +153,6 @@ typedef struct PMUCTRState {
>       target_ulong mhpmcounter_prev;
>       /* Snapshort value of a counter in RV32 */
>       target_ulong mhpmcounterh_prev;
> -    bool started;
>       /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
>       target_ulong irq_overflow_left;
>   } PMUCTRState;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 726096444fae..68ca31aff47d 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -929,17 +929,11 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *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.
> +         * Counter should not increment if inhibit bit is set. Just return the
> +         * current counter value.
>            */
> -        if (!counter->started) {
> -            *val = ctr_val;
> -            return RISCV_EXCP_NONE;
> -        } else {
> -            /* Mark that the counter has been stopped */
> -            counter->started = false;
> -        }
> +         *val = ctr_val;
> +         return RISCV_EXCP_NONE;
>       }
>   
>       /*
> @@ -1973,9 +1967,23 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
>   
>       /* 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;
> +        if (get_field(env->mcountinhibit, BIT(cidx)) && (val & BIT(cidx))) {
> +	    /*
> +             * Update the counter value for cycle/instret as we can't stop the
> +             * host ticks. But we should show the current value at this moment.
> +             */
> +            if (riscv_pmu_ctr_monitor_cycles(env, cidx) ||
> +                riscv_pmu_ctr_monitor_instructions(env, cidx)) {
> +                counter->mhpmcounter_val = get_ticks(false) -
> +                                           counter->mhpmcounter_prev +
> +                                           counter->mhpmcounter_val;
> +                if (riscv_cpu_mxl(env) == MXL_RV32) {
> +                    counter->mhpmcounterh_val = get_ticks(false) -
> +                                                counter->mhpmcounterh_prev +
> +                                                counter->mhpmcounterh_val;
> +		}
> +            }
>           }
>       }
>   
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 76f2150f78b5..3e0f2dd2ce2a 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -328,7 +328,6 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
>           VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
>           VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
>           VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
> -        VMSTATE_BOOL(started, PMUCTRState),

Unfortunately we can't remove fields from the VMStateDescription without breaking
migration backward compatibility. Older QEMUs will attempt to read a field that
doesn't exist and migration will fail.

I'm assuming that we care about backward compat. If we're not up to this point yet
then we can just bump the version_id of vmstate_pmu_ctr_state and be done with it.
This is fine to do unless someone jumps in and complains that we broke a migration
case for the 'virt' board. Granted, we don't have versioned boards yet so I'm unsure
if someone would actually have a base to complain. Alistair, Drew, care to comment?


Now, if we care about backward migration compat, we'll need to do as described in
devel/migration/main.rst, section "Not sending existing elements". An example on
how we need to proceed can also be seen in commit 6cc88d6bf9. But in short we
would need to:

- add a dummy property, e.g. a 'mig_started' bool

- use a slightly different macro in vmstate:

> -        VMSTATE_BOOL(started, PMUCTRState),
> +        VMSTATE_BOOL_TEST(mig_started, PMUCTRState),

- add hooks in pre_load()/post_load() to load the 'started' field



Thanks,

Daniel

>           VMSTATE_END_OF_LIST()
>       }
>   };
> 

Re: [PATCH 2/3] target/riscv: Enforce WARL behavior for scounteren/hcounteren

[Daniel Henrique Barboza]

On 4/29/24 16:28, Atish Patra wrote:
> scounteren/hcountern are also WARL registers similar to mcountern.
> Only set the bits for the available counters during the write to
> preserve the WARL behavior.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   target/riscv/csr.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 68ca31aff47d..a01911541d67 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2843,7 +2843,11 @@ static RISCVException read_scounteren(CPURISCVState *env, int csrno,
>   static RISCVException write_scounteren(CPURISCVState *env, int csrno,
>                                          target_ulong val)
>   {
> -    env->scounteren = val;
> +    RISCVCPU *cpu = env_archcpu(env);
> +
> +    /* WARL register - disable unavailable counters */
> +    env->scounteren = val & (cpu->pmu_avail_ctrs | COUNTEREN_CY | COUNTEREN_TM |
> +                             COUNTEREN_IR);
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -3475,7 +3479,11 @@ static RISCVException read_hcounteren(CPURISCVState *env, int csrno,
>   static RISCVException write_hcounteren(CPURISCVState *env, int csrno,
>                                          target_ulong val)
>   {
> -    env->hcounteren = val;
> +    RISCVCPU *cpu = env_archcpu(env);
> +
> +    /* WARL register - disable unavailable counters */
> +    env->hcounteren = val & (cpu->pmu_avail_ctrs | COUNTEREN_CY | COUNTEREN_TM |
> +                             COUNTEREN_IR);
>       return RISCV_EXCP_NONE;
>   }
>   
> 

Re: [PATCH 1/3] target/riscv: Save counter values during countinhibit update

[Andrew Jones]

On Tue, Apr 30, 2024 at 03:00:45PM GMT, Daniel Henrique Barboza wrote:
> 
> 
> On 4/29/24 16:28, Atish Patra wrote:
> > Currently, if a counter monitoring cycle/instret is stopped via
> > mcountinhibit we just update the state while the value is saved
> > during the next read. This is not accurate as the read may happen
> > many cycles after the counter is stopped. Ideally, the read should
> > return the value saved when the counter is stopped.
> > 
> > Thus, save the value of the counter during the inhibit update
> > operation and return that value during the read if corresponding bit
> > in mcountihibit is set.
> > 
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >   target/riscv/cpu.h     |  1 -
> >   target/riscv/csr.c     | 32 ++++++++++++++++++++------------
> >   target/riscv/machine.c |  1 -
> >   3 files changed, 20 insertions(+), 14 deletions(-)
> > 
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 3b1a02b9449a..09bbf7ce9880 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -153,7 +153,6 @@ typedef struct PMUCTRState {
> >       target_ulong mhpmcounter_prev;
> >       /* Snapshort value of a counter in RV32 */
> >       target_ulong mhpmcounterh_prev;
> > -    bool started;
> >       /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> >       target_ulong irq_overflow_left;
> >   } PMUCTRState;
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 726096444fae..68ca31aff47d 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -929,17 +929,11 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *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.
> > +         * Counter should not increment if inhibit bit is set. Just return the
> > +         * current counter value.
> >            */
> > -        if (!counter->started) {
> > -            *val = ctr_val;
> > -            return RISCV_EXCP_NONE;
> > -        } else {
> > -            /* Mark that the counter has been stopped */
> > -            counter->started = false;
> > -        }
> > +         *val = ctr_val;
> > +         return RISCV_EXCP_NONE;
> >       }
> >       /*
> > @@ -1973,9 +1967,23 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
> >       /* 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;
> > +        if (get_field(env->mcountinhibit, BIT(cidx)) && (val & BIT(cidx))) {
> > +	    /*
> > +             * Update the counter value for cycle/instret as we can't stop the
> > +             * host ticks. But we should show the current value at this moment.
> > +             */
> > +            if (riscv_pmu_ctr_monitor_cycles(env, cidx) ||
> > +                riscv_pmu_ctr_monitor_instructions(env, cidx)) {
> > +                counter->mhpmcounter_val = get_ticks(false) -
> > +                                           counter->mhpmcounter_prev +
> > +                                           counter->mhpmcounter_val;
> > +                if (riscv_cpu_mxl(env) == MXL_RV32) {
> > +                    counter->mhpmcounterh_val = get_ticks(false) -
> > +                                                counter->mhpmcounterh_prev +
> > +                                                counter->mhpmcounterh_val;
> > +		}
> > +            }
> >           }
> >       }
> > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > index 76f2150f78b5..3e0f2dd2ce2a 100644
> > --- a/target/riscv/machine.c
> > +++ b/target/riscv/machine.c
> > @@ -328,7 +328,6 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
> >           VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
> >           VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
> >           VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
> > -        VMSTATE_BOOL(started, PMUCTRState),
> 
> Unfortunately we can't remove fields from the VMStateDescription without breaking
> migration backward compatibility. Older QEMUs will attempt to read a field that
> doesn't exist and migration will fail.
> 
> I'm assuming that we care about backward compat. If we're not up to this point yet
> then we can just bump the version_id of vmstate_pmu_ctr_state and be done with it.
> This is fine to do unless someone jumps in and complains that we broke a migration
> case for the 'virt' board. Granted, we don't have versioned boards yet so I'm unsure
> if someone would actually have a base to complain. Alistair, Drew, care to comment?

Without versioning boards, then we shouldn't expect migrations to work for
anything other than between QEMUs of the same version. We're delaying the
versioning until it's reasonable to expect users to prefer to migrate
their guests, rather than reboot them, when updating the QEMU the guests
are running on. I'm not sure how we'll know when that is, but I think we
can wait until somebody shouts or at least until we see that the tooling
which makes migration easy (libvirt, etc.) is present.

Regarding this patch, I'm curious what the current status is of migration.
If we can currently migrate from a QEMU with the latest released version
to a QEMU built from the current upstream, and then back again, then I
think this patch should be written in a way to preserve that. If we
already fail that ping-pong migration, then, as this patch doesn't make
things worse, we might as well save ourselves from the burden of the
compat code.

Thanks,
drew

Re: [PATCH 1/3] target/riscv: Save counter values during countinhibit update

[Atish Kumar Patra]

On Thu, May 2, 2024 at 5:39 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Tue, Apr 30, 2024 at 03:00:45PM GMT, Daniel Henrique Barboza wrote:
> >
> >
> > On 4/29/24 16:28, Atish Patra wrote:
> > > Currently, if a counter monitoring cycle/instret is stopped via
> > > mcountinhibit we just update the state while the value is saved
> > > during the next read. This is not accurate as the read may happen
> > > many cycles after the counter is stopped. Ideally, the read should
> > > return the value saved when the counter is stopped.
> > >
> > > Thus, save the value of the counter during the inhibit update
> > > operation and return that value during the read if corresponding bit
> > > in mcountihibit is set.
> > >
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > ---
> > >   target/riscv/cpu.h     |  1 -
> > >   target/riscv/csr.c     | 32 ++++++++++++++++++++------------
> > >   target/riscv/machine.c |  1 -
> > >   3 files changed, 20 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 3b1a02b9449a..09bbf7ce9880 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -153,7 +153,6 @@ typedef struct PMUCTRState {
> > >       target_ulong mhpmcounter_prev;
> > >       /* Snapshort value of a counter in RV32 */
> > >       target_ulong mhpmcounterh_prev;
> > > -    bool started;
> > >       /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> > >       target_ulong irq_overflow_left;
> > >   } PMUCTRState;
> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > index 726096444fae..68ca31aff47d 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> > > @@ -929,17 +929,11 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *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.
> > > +         * Counter should not increment if inhibit bit is set. Just return the
> > > +         * current counter value.
> > >            */
> > > -        if (!counter->started) {
> > > -            *val = ctr_val;
> > > -            return RISCV_EXCP_NONE;
> > > -        } else {
> > > -            /* Mark that the counter has been stopped */
> > > -            counter->started = false;
> > > -        }
> > > +         *val = ctr_val;
> > > +         return RISCV_EXCP_NONE;
> > >       }
> > >       /*
> > > @@ -1973,9 +1967,23 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
> > >       /* 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;
> > > +        if (get_field(env->mcountinhibit, BIT(cidx)) && (val & BIT(cidx))) {
> > > +       /*
> > > +             * Update the counter value for cycle/instret as we can't stop the
> > > +             * host ticks. But we should show the current value at this moment.
> > > +             */
> > > +            if (riscv_pmu_ctr_monitor_cycles(env, cidx) ||
> > > +                riscv_pmu_ctr_monitor_instructions(env, cidx)) {
> > > +                counter->mhpmcounter_val = get_ticks(false) -
> > > +                                           counter->mhpmcounter_prev +
> > > +                                           counter->mhpmcounter_val;
> > > +                if (riscv_cpu_mxl(env) == MXL_RV32) {
> > > +                    counter->mhpmcounterh_val = get_ticks(false) -
> > > +                                                counter->mhpmcounterh_prev +
> > > +                                                counter->mhpmcounterh_val;
> > > +           }
> > > +            }
> > >           }
> > >       }
> > > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > > index 76f2150f78b5..3e0f2dd2ce2a 100644
> > > --- a/target/riscv/machine.c
> > > +++ b/target/riscv/machine.c
> > > @@ -328,7 +328,6 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
> > >           VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
> > >           VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
> > >           VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
> > > -        VMSTATE_BOOL(started, PMUCTRState),
> >
> > Unfortunately we can't remove fields from the VMStateDescription without breaking
> > migration backward compatibility. Older QEMUs will attempt to read a field that
> > doesn't exist and migration will fail.
> >
> > I'm assuming that we care about backward compat. If we're not up to this point yet
> > then we can just bump the version_id of vmstate_pmu_ctr_state and be done with it.
> > This is fine to do unless someone jumps in and complains that we broke a migration
> > case for the 'virt' board. Granted, we don't have versioned boards yet so I'm unsure
> > if someone would actually have a base to complain. Alistair, Drew, care to comment?
>
> Without versioning boards, then we shouldn't expect migrations to work for
> anything other than between QEMUs of the same version. We're delaying the
> versioning until it's reasonable to expect users to prefer to migrate
> their guests, rather than reboot them, when updating the QEMU the guests
> are running on. I'm not sure how we'll know when that is, but I think we
> can wait until somebody shouts or at least until we see that the tooling
> which makes migration easy (libvirt, etc.) is present.
>
> Regarding this patch, I'm curious what the current status is of migration.
> If we can currently migrate from a QEMU with the latest released version
> to a QEMU built from the current upstream, and then back again, then I

I haven't heard of anyone who actually uses migration in Qemu.
There is only one way to know about it when somebody complains.

I think we should just keep it simple and bump up the version  of
vmstate_pmu_ctr_state.
If somebody complains about backward compatibility, we can implement
compat code.
Otherwise, I don't see the point.

> think this patch should be written in a way to preserve that. If we
> already fail that ping-pong migration, then, as this patch doesn't make
> things worse, we might as well save ourselves from the burden of the
> compat code.
>
> Thanks,
> drew

Re: [PATCH 1/3] target/riscv: Save counter values during countinhibit update

[Andrew Jones]

On Thu, May 09, 2024 at 01:26:56PM GMT, Atish Kumar Patra wrote:
> On Thu, May 2, 2024 at 5:39 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Tue, Apr 30, 2024 at 03:00:45PM GMT, Daniel Henrique Barboza wrote:
> > >
> > >
> > > On 4/29/24 16:28, Atish Patra wrote:
> > > > Currently, if a counter monitoring cycle/instret is stopped via
> > > > mcountinhibit we just update the state while the value is saved
> > > > during the next read. This is not accurate as the read may happen
> > > > many cycles after the counter is stopped. Ideally, the read should
> > > > return the value saved when the counter is stopped.
> > > >
> > > > Thus, save the value of the counter during the inhibit update
> > > > operation and return that value during the read if corresponding bit
> > > > in mcountihibit is set.
> > > >
> > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > ---
> > > >   target/riscv/cpu.h     |  1 -
> > > >   target/riscv/csr.c     | 32 ++++++++++++++++++++------------
> > > >   target/riscv/machine.c |  1 -
> > > >   3 files changed, 20 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > index 3b1a02b9449a..09bbf7ce9880 100644
> > > > --- a/target/riscv/cpu.h
> > > > +++ b/target/riscv/cpu.h
> > > > @@ -153,7 +153,6 @@ typedef struct PMUCTRState {
> > > >       target_ulong mhpmcounter_prev;
> > > >       /* Snapshort value of a counter in RV32 */
> > > >       target_ulong mhpmcounterh_prev;
> > > > -    bool started;
> > > >       /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> > > >       target_ulong irq_overflow_left;
> > > >   } PMUCTRState;
> > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > index 726096444fae..68ca31aff47d 100644
> > > > --- a/target/riscv/csr.c
> > > > +++ b/target/riscv/csr.c
> > > > @@ -929,17 +929,11 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *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.
> > > > +         * Counter should not increment if inhibit bit is set. Just return the
> > > > +         * current counter value.
> > > >            */
> > > > -        if (!counter->started) {
> > > > -            *val = ctr_val;
> > > > -            return RISCV_EXCP_NONE;
> > > > -        } else {
> > > > -            /* Mark that the counter has been stopped */
> > > > -            counter->started = false;
> > > > -        }
> > > > +         *val = ctr_val;
> > > > +         return RISCV_EXCP_NONE;
> > > >       }
> > > >       /*
> > > > @@ -1973,9 +1967,23 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
> > > >       /* 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;
> > > > +        if (get_field(env->mcountinhibit, BIT(cidx)) && (val & BIT(cidx))) {
> > > > +       /*
> > > > +             * Update the counter value for cycle/instret as we can't stop the
> > > > +             * host ticks. But we should show the current value at this moment.
> > > > +             */
> > > > +            if (riscv_pmu_ctr_monitor_cycles(env, cidx) ||
> > > > +                riscv_pmu_ctr_monitor_instructions(env, cidx)) {
> > > > +                counter->mhpmcounter_val = get_ticks(false) -
> > > > +                                           counter->mhpmcounter_prev +
> > > > +                                           counter->mhpmcounter_val;
> > > > +                if (riscv_cpu_mxl(env) == MXL_RV32) {
> > > > +                    counter->mhpmcounterh_val = get_ticks(false) -
> > > > +                                                counter->mhpmcounterh_prev +
> > > > +                                                counter->mhpmcounterh_val;
> > > > +           }
> > > > +            }
> > > >           }
> > > >       }
> > > > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > > > index 76f2150f78b5..3e0f2dd2ce2a 100644
> > > > --- a/target/riscv/machine.c
> > > > +++ b/target/riscv/machine.c
> > > > @@ -328,7 +328,6 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
> > > >           VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
> > > >           VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
> > > >           VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
> > > > -        VMSTATE_BOOL(started, PMUCTRState),
> > >
> > > Unfortunately we can't remove fields from the VMStateDescription without breaking
> > > migration backward compatibility. Older QEMUs will attempt to read a field that
> > > doesn't exist and migration will fail.
> > >
> > > I'm assuming that we care about backward compat. If we're not up to this point yet
> > > then we can just bump the version_id of vmstate_pmu_ctr_state and be done with it.
> > > This is fine to do unless someone jumps in and complains that we broke a migration
> > > case for the 'virt' board. Granted, we don't have versioned boards yet so I'm unsure
> > > if someone would actually have a base to complain. Alistair, Drew, care to comment?
> >
> > Without versioning boards, then we shouldn't expect migrations to work for
> > anything other than between QEMUs of the same version. We're delaying the
> > versioning until it's reasonable to expect users to prefer to migrate
> > their guests, rather than reboot them, when updating the QEMU the guests
> > are running on. I'm not sure how we'll know when that is, but I think we
> > can wait until somebody shouts or at least until we see that the tooling
> > which makes migration easy (libvirt, etc.) is present.
> >
> > Regarding this patch, I'm curious what the current status is of migration.
> > If we can currently migrate from a QEMU with the latest released version
> > to a QEMU built from the current upstream, and then back again, then I
> 
> I haven't heard of anyone who actually uses migration in Qemu.
> There is only one way to know about it when somebody complains.
> 
> I think we should just keep it simple and bump up the version  of
> vmstate_pmu_ctr_state.
> If somebody complains about backward compatibility, we can implement
> compat code.
> Otherwise, I don't see the point.

Agreed.

Thanks,
drew

> 
> > think this patch should be written in a way to preserve that. If we
> > already fail that ping-pong migration, then, as this patch doesn't make
> > things worse, we might as well save ourselves from the burden of the
> > compat code.
> >
> > Thanks,
> > drew

Re: [PATCH 1/3] target/riscv: Save counter values during countinhibit update

[Alistair Francis]

On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> Currently, if a counter monitoring cycle/instret is stopped via
> mcountinhibit we just update the state while the value is saved
> during the next read. This is not accurate as the read may happen
> many cycles after the counter is stopped. Ideally, the read should
> return the value saved when the counter is stopped.
>
> Thus, save the value of the counter during the inhibit update
> operation and return that value during the read if corresponding bit
> in mcountihibit is set.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.h     |  1 -
>  target/riscv/csr.c     | 32 ++++++++++++++++++++------------
>  target/riscv/machine.c |  1 -
>  3 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 3b1a02b9449a..09bbf7ce9880 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -153,7 +153,6 @@ typedef struct PMUCTRState {
>      target_ulong mhpmcounter_prev;
>      /* Snapshort value of a counter in RV32 */
>      target_ulong mhpmcounterh_prev;
> -    bool started;
>      /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
>      target_ulong irq_overflow_left;
>  } PMUCTRState;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 726096444fae..68ca31aff47d 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -929,17 +929,11 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *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.
> +         * Counter should not increment if inhibit bit is set. Just return the
> +         * current counter value.
>           */
> -        if (!counter->started) {
> -            *val = ctr_val;
> -            return RISCV_EXCP_NONE;
> -        } else {
> -            /* Mark that the counter has been stopped */
> -            counter->started = false;
> -        }
> +         *val = ctr_val;
> +         return RISCV_EXCP_NONE;
>      }
>
>      /*
> @@ -1973,9 +1967,23 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
>
>      /* 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;
> +        if (get_field(env->mcountinhibit, BIT(cidx)) && (val & BIT(cidx))) {
> +           /*
> +             * Update the counter value for cycle/instret as we can't stop the
> +             * host ticks. But we should show the current value at this moment.
> +             */
> +            if (riscv_pmu_ctr_monitor_cycles(env, cidx) ||
> +                riscv_pmu_ctr_monitor_instructions(env, cidx)) {
> +                counter->mhpmcounter_val = get_ticks(false) -
> +                                           counter->mhpmcounter_prev +
> +                                           counter->mhpmcounter_val;
> +                if (riscv_cpu_mxl(env) == MXL_RV32) {
> +                    counter->mhpmcounterh_val = get_ticks(false) -
> +                                                counter->mhpmcounterh_prev +
> +                                                counter->mhpmcounterh_val;
> +               }
> +            }
>          }
>      }
>
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 76f2150f78b5..3e0f2dd2ce2a 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -328,7 +328,6 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
>          VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
>          VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
>          VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
> -        VMSTATE_BOOL(started, PMUCTRState),
>          VMSTATE_END_OF_LIST()
>      }
>  };
>
> --
> 2.34.1
>
>

Re: [PATCH 2/3] target/riscv: Enforce WARL behavior for scounteren/hcounteren

[Alistair Francis]

On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> scounteren/hcountern are also WARL registers similar to mcountern.
> Only set the bits for the available counters during the write to
> preserve the WARL behavior.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/csr.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 68ca31aff47d..a01911541d67 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2843,7 +2843,11 @@ static RISCVException read_scounteren(CPURISCVState *env, int csrno,
>  static RISCVException write_scounteren(CPURISCVState *env, int csrno,
>                                         target_ulong val)
>  {
> -    env->scounteren = val;
> +    RISCVCPU *cpu = env_archcpu(env);
> +
> +    /* WARL register - disable unavailable counters */
> +    env->scounteren = val & (cpu->pmu_avail_ctrs | COUNTEREN_CY | COUNTEREN_TM |
> +                             COUNTEREN_IR);
>      return RISCV_EXCP_NONE;
>  }
>
> @@ -3475,7 +3479,11 @@ static RISCVException read_hcounteren(CPURISCVState *env, int csrno,
>  static RISCVException write_hcounteren(CPURISCVState *env, int csrno,
>                                         target_ulong val)
>  {
> -    env->hcounteren = val;
> +    RISCVCPU *cpu = env_archcpu(env);
> +
> +    /* WARL register - disable unavailable counters */
> +    env->hcounteren = val & (cpu->pmu_avail_ctrs | COUNTEREN_CY | COUNTEREN_TM |
> +                             COUNTEREN_IR);
>      return RISCV_EXCP_NONE;
>  }
>
>
> --
> 2.34.1
>
>

Re: [PATCH 0/3] Assorted fixes for PMU

[Alistair Francis]

On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> This series contains few miscallenous fixes related to hpmcounters
> and related code. The first patch fixes an issue with cycle/instret
> counters overcouting while the remaining two are more for specification
> compliance.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
> Atish Patra (3):
>       target/riscv: Save counter values during countinhibit update
>       target/riscv: Enforce WARL behavior for scounteren/hcounteren
>       target/riscv: Fix the predicate functions for mhpmeventhX CSRs

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/cpu.h     |   1 -
>  target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
>  target/riscv/machine.c |   1 -
>  3 files changed, 68 insertions(+), 45 deletions(-)
> ---
> base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
> change-id: 20240428-countinhibit_fix-c6a1c11f4375
> --
> Regards,
> Atish patra
>
>

Re: [PATCH 0/3] Assorted fixes for PMU

[Atish Kumar Patra]

On Mon, May 13, 2024 at 11:29 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
> >
> > This series contains few miscallenous fixes related to hpmcounters
> > and related code. The first patch fixes an issue with cycle/instret
> > counters overcouting while the remaining two are more for specification
> > compliance.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> > Atish Patra (3):
> >       target/riscv: Save counter values during countinhibit update
> >       target/riscv: Enforce WARL behavior for scounteren/hcounteren
> >       target/riscv: Fix the predicate functions for mhpmeventhX CSRs
>
> Thanks!
>
> Applied to riscv-to-apply.next
>

Hi Alistair,
Thanks for your review. But the patch 1 had some comments about
vmstate which needs updating.
We also found a few more fixes that I was planning to include in v2.

I can send a separate fixes series on top riscv-to-apply.next or this
series can be dropped for the time being.
You can queue it v2 later. Let me know what you prefer.


> Alistair
>
> >
> >  target/riscv/cpu.h     |   1 -
> >  target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
> >  target/riscv/machine.c |   1 -
> >  3 files changed, 68 insertions(+), 45 deletions(-)
> > ---
> > base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
> > change-id: 20240428-countinhibit_fix-c6a1c11f4375
> > --
> > Regards,
> > Atish patra
> >
> >

Re: [PATCH 0/3] Assorted fixes for PMU

[Peter Maydell]

On Mon, 29 Apr 2024 at 20:29, Atish Patra <atishp@rivosinc.com> wrote:
>
> This series contains few miscallenous fixes related to hpmcounters
> and related code. The first patch fixes an issue with cycle/instret
> counters overcouting while the remaining two are more for specification
> compliance.

I've noticed a number of riscv patchsets from various people
recently where the subject line for the cover letter doesn't
include any indication that it's a riscv related patchset.
For instance this one is just "Assorted fixes for PMU", which
could easily be an Arm PMU series. Could you all try to make sure
that cover letter subject lines indicate the architecture or
other subcomponent they affect, please? This helps people who
are skimming over the mailing list looking for patches relevant
to them.

thanks
-- PMM

Re: [PATCH 0/3] Assorted fixes for PMU

[Alistair Francis]

On Tue, May 14, 2024 at 5:15 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Mon, May 13, 2024 at 11:29 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
> > >
> > > This series contains few miscallenous fixes related to hpmcounters
> > > and related code. The first patch fixes an issue with cycle/instret
> > > counters overcouting while the remaining two are more for specification
> > > compliance.
> > >
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > ---
> > > Atish Patra (3):
> > >       target/riscv: Save counter values during countinhibit update
> > >       target/riscv: Enforce WARL behavior for scounteren/hcounteren
> > >       target/riscv: Fix the predicate functions for mhpmeventhX CSRs
> >
> > Thanks!
> >
> > Applied to riscv-to-apply.next
> >
>
> Hi Alistair,
> Thanks for your review. But the patch 1 had some comments about
> vmstate which needs updating.

Ah, I did read the comments but forgot that you were going to bump the version.

> We also found a few more fixes that I was planning to include in v2.

I found that patch `target/riscv: Save counter values during
countinhibit update` gives me this error as well

../target/riscv/csr.c: In function ‘write_mcountinhibit’:
../target/riscv/csr.c:1981:44: error: too few arguments to function ‘get_ticks’
1981 |                 counter->mhpmcounter_val = get_ticks(false) -
     |                                            ^~~~~~~~~
../target/riscv/csr.c:765:21: note: declared here
 765 | static target_ulong get_ticks(bool shift, bool instructions)
     |                     ^~~~~~~~~
../target/riscv/csr.c:1985:49: error: too few arguments to function ‘get_ticks’
1985 |                     counter->mhpmcounterh_val = get_ticks(false) -
     |                                                 ^~~~~~~~~
../target/riscv/csr.c:765:21: note: declared here
 765 | static target_ulong get_ticks(bool shift, bool instructions)
     |                     ^~~~~~~~~



>
> I can send a separate fixes series on top riscv-to-apply.next or this
> series can be dropped for the time being.

I'm going to drop it due to the build error above

Alistair

> You can queue it v2 later. Let me know what you prefer.
>
>
> > Alistair
> >
> > >
> > >  target/riscv/cpu.h     |   1 -
> > >  target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
> > >  target/riscv/machine.c |   1 -
> > >  3 files changed, 68 insertions(+), 45 deletions(-)
> > > ---
> > > base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
> > > change-id: 20240428-countinhibit_fix-c6a1c11f4375
> > > --
> > > Regards,
> > > Atish patra
> > >
> > >

Re: [PATCH 0/3] Assorted fixes for PMU

[Atish Patra]

On Tue, May 14, 2024 at 3:18 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, May 14, 2024 at 5:15 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >
> > On Mon, May 13, 2024 at 11:29 PM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
> > > >
> > > > This series contains few miscallenous fixes related to hpmcounters
> > > > and related code. The first patch fixes an issue with cycle/instret
> > > > counters overcouting while the remaining two are more for specification
> > > > compliance.
> > > >
> > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > ---
> > > > Atish Patra (3):
> > > >       target/riscv: Save counter values during countinhibit update
> > > >       target/riscv: Enforce WARL behavior for scounteren/hcounteren
> > > >       target/riscv: Fix the predicate functions for mhpmeventhX CSRs
> > >
> > > Thanks!
> > >
> > > Applied to riscv-to-apply.next
> > >
> >
> > Hi Alistair,
> > Thanks for your review. But the patch 1 had some comments about
> > vmstate which needs updating.
>
> Ah, I did read the comments but forgot that you were going to bump the version.
>
> > We also found a few more fixes that I was planning to include in v2.
>
> I found that patch `target/riscv: Save counter values during
> countinhibit update` gives me this error as well
>
> ../target/riscv/csr.c: In function ‘write_mcountinhibit’:
> ../target/riscv/csr.c:1981:44: error: too few arguments to function ‘get_ticks’
> 1981 |                 counter->mhpmcounter_val = get_ticks(false) -
>      |                                            ^~~~~~~~~
> ../target/riscv/csr.c:765:21: note: declared here
>  765 | static target_ulong get_ticks(bool shift, bool instructions)
>      |                     ^~~~~~~~~
> ../target/riscv/csr.c:1985:49: error: too few arguments to function ‘get_ticks’
> 1985 |                     counter->mhpmcounterh_val = get_ticks(false) -
>      |                                                 ^~~~~~~~~
> ../target/riscv/csr.c:765:21: note: declared here
>  765 | static target_ulong get_ticks(bool shift, bool instructions)
>      |                     ^~~~~~~~~
>

Yeah. Clement's patch got in. I will rebase and update the series
based on the riscv-to-apply.next.

>
>
> >
> > I can send a separate fixes series on top riscv-to-apply.next or this
> > series can be dropped for the time being.
>
> I'm going to drop it due to the build error above
>
> Alistair
>
> > You can queue it v2 later. Let me know what you prefer.
> >
> >
> > > Alistair
> > >
> > > >
> > > >  target/riscv/cpu.h     |   1 -
> > > >  target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
> > > >  target/riscv/machine.c |   1 -
> > > >  3 files changed, 68 insertions(+), 45 deletions(-)
> > > > ---
> > > > base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
> > > > change-id: 20240428-countinhibit_fix-c6a1c11f4375
> > > > --
> > > > Regards,
> > > > Atish patra
> > > >
> > > >
>


-- 
Regards,
Atish

Re: [PATCH 0/3] Assorted fixes for PMU

[Atish Patra]

On Tue, May 14, 2024 at 2:16 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 29 Apr 2024 at 20:29, Atish Patra <atishp@rivosinc.com> wrote:
> >
> > This series contains few miscallenous fixes related to hpmcounters
> > and related code. The first patch fixes an issue with cycle/instret
> > counters overcouting while the remaining two are more for specification
> > compliance.
>
> I've noticed a number of riscv patchsets from various people
> recently where the subject line for the cover letter doesn't
> include any indication that it's a riscv related patchset.
> For instance this one is just "Assorted fixes for PMU", which
> could easily be an Arm PMU series. Could you all try to make sure
> that cover letter subject lines indicate the architecture or
> other subcomponent they affect, please? This helps people who
> are skimming over the mailing list looking for patches relevant
> to them.
>

Makes sense. I will include RISC-V in the series cover letter as well.
Thanks for the feedback.

> thanks
> -- PMM
>


-- 
Regards,
Atish