Re: [PATCH v2 04/33] target/riscv: Bugfix riscv_pmu_ctr_get_fixed_counters_val()

[Alistair Francis <alistair23@gmail.com>]

On Wed, Oct 1, 2025 at 5:43 PM Anton Johansson via
<qemu-devel@nongnu.org> wrote:
>
> From my understanding the upper_half argument only indicates whether the
> upper or lower 32 bits should be returned, and upper_half will only ever
> be set when MXLEN == 32.  However, the function also uses upper_half to
> determine whether the inhibit flags are located in mcyclecfgh or
> mcyclecfg, but this misses the case where MXLEN == 32, upper_half == false
> for TARGET_RISCV32 where we would also need to read the upper half field.

If MXLEN == 32, upper_half == false then we want to read mcyclecfg,
which the code today seems to be doing correctly.

>
> Minor simplifications are also made along with some formatting fixes.
>
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> ---
>  target/riscv/csr.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 3c8989f522..859f89aedd 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -17,6 +17,7 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>
> +#include "cpu_bits.h"
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
>  #include "qemu/timer.h"
> @@ -1241,18 +1242,21 @@ static target_ulong riscv_pmu_ctr_get_fixed_counters_val(CPURISCVState *env,
>      int inst = riscv_pmu_ctr_monitor_instructions(env, counter_idx);
>      uint64_t *counter_arr_virt = env->pmu_fixed_ctrs[inst].counter_virt;
>      uint64_t *counter_arr = env->pmu_fixed_ctrs[inst].counter;
> -    target_ulong result = 0;
>      uint64_t curr_val = 0;
>      uint64_t cfg_val = 0;
> +    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32;
> +
> +    /* Ensure upper_half is only set for MXL_RV32 */
> +    g_assert(rv32 || !upper_half);
>
>      if (counter_idx == 0) {
> -        cfg_val = upper_half ? ((uint64_t)env->mcyclecfgh << 32) :
> +        cfg_val = rv32 ? ((uint64_t)env->mcyclecfgh << 32) :

This doesn't look right.

RV32 will want to access both mcyclecfgh and mcyclecfg, but this
change restricts access to mcyclecfg as rv32 will always be true.

I don't think there is anything wrong with the current code.

Alistair

>                    env->mcyclecfg;
>      } else if (counter_idx == 2) {
> -        cfg_val = upper_half ? ((uint64_t)env->minstretcfgh << 32) :
> +        cfg_val = rv32 ? ((uint64_t)env->minstretcfgh << 32) :
>                    env->minstretcfg;
>      } else {
> -        cfg_val = upper_half ?
> +        cfg_val = rv32 ?
>                    ((uint64_t)env->mhpmeventh_val[counter_idx] << 32) :
>                    env->mhpmevent_val[counter_idx];
>          cfg_val &= MHPMEVENT_FILTER_MASK;
> @@ -1260,7 +1264,7 @@ static target_ulong riscv_pmu_ctr_get_fixed_counters_val(CPURISCVState *env,
>
>      if (!cfg_val) {
>          if (icount_enabled()) {
> -                curr_val = inst ? icount_get_raw() : icount_get();
> +            curr_val = inst ? icount_get_raw() : icount_get();
>          } else {
>              curr_val = cpu_get_host_ticks();
>          }
> @@ -1292,13 +1296,7 @@ static target_ulong riscv_pmu_ctr_get_fixed_counters_val(CPURISCVState *env,
>      }
>
>  done:
> -    if (riscv_cpu_mxl(env) == MXL_RV32) {
> -        result = upper_half ? curr_val >> 32 : curr_val;
> -    } else {
> -        result = curr_val;
> -    }
> -
> -    return result;
> +    return upper_half ? curr_val >> 32 : curr_val;
>  }
>
>  static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong val,
> --
> 2.51.0
>
>