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

[Peter Maydell]

On Sat, 20 Jul 2024 at 16:19, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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

Having worked through some examples I'm happy that this is
a false positive and the expression used here does the
right thing even when the mask goes right up to bit 63.
So I've closed the Coverity issues as false positives;
whether we decide it's worth converting the riscv target to
use QEMU's more standard accessor macros is a separate issue.

If we do want to keep these get_field/set_field macros,
could we add a documentation comment that says what they
do, including description of what the arguments are?

thanks
-- PMM