Re: [PULL 26/30] target/riscv: Do not setup pmu timer if OF is disabled

[Peter Maydell]

On Thu, 18 Jul 2024 at 03:15, Alistair Francis <alistair23@gmail.com> wrote:
>
> From: Atish Patra <atishp@rivosinc.com>
>
> The timer is setup function is invoked in both hpmcounter
> write and mcountinhibit write path. If the OF bit set, the
> LCOFI interrupt is disabled. There is no benefitting in
> setting up the qemu timer until LCOFI is cleared to indicate
> that interrupts can be fired again.
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> Message-ID: <20240711-smcntrpmf_v7-v8-12-b7c38ae7b263@rivosinc.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/pmu.c | 56 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 12 deletions(-)
>
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index a4729f6c53..3cc0b3648c 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -416,14 +416,49 @@ int riscv_pmu_update_event_map(CPURISCVState *env, 
> uint64_t value,
>      return 0;
>  }

Hi; I was looking at an issue Coverity flagged up with this code (CID
1558461, 1558463):

> +static bool pmu_hpmevent_is_of_set(CPURISCVState *env, uint32_t ctr_idx)
> +{
> +    target_ulong mhpmevent_val;
> +    uint64_t of_bit_mask;
> +
> +    if (riscv_cpu_mxl(env) == MXL_RV32) {
> +        mhpmevent_val = env->mhpmeventh_val[ctr_idx];
> +        of_bit_mask = MHPMEVENTH_BIT_OF;
> +     } else {
> +        mhpmevent_val = env->mhpmevent_val[ctr_idx];
> +        of_bit_mask = MHPMEVENT_BIT_OF;

MHPMEVENT_BIT_OF is defined as BIT_ULL(63)...

> +    }
> +
> +    return get_field(mhpmevent_val, of_bit_mask);

...but we pass it to get_field(), whose definition is:

#define get_field(reg, mask) (((reg) & \
                 (uint64_t)(mask)) / ((mask) & ~((mask) << 1)))

Notice that part of this expression is "(mask) << 1". So Coverity complains
that we took a constant value and shifted it right off the top.

I think this is probably a false positive, but why is target/riscv
using its own ad-hoc macros for extracting bitfields? We have
a standard set of extract/deposit macros in bitops.h, and not
using them makes the riscv code harder to read for people who
are used to the rest of the codebase (e.g. to figure out if this
Coverity issue is a false positive I would need to look at these
macros to figure out what exactly they're doing).

thanks
-- PMM