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

[Alistair Francis]

On Tue, Jul 23, 2024 at 9:33 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Sat, Jul 20, 2024 at 8:19 AM 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, 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
>
> Thanks for pointing those out. I checked the get_field usage from the
> beginning of riscv support 6 years back.
> There are tons of users of get_field in a bunch of riscv sources. I
> guess it was just added once and everybody kept using it
> without switching to generic functions.

I think you are right about that

>
> @Alistair Francis : Are there any other reasons ?

Not that I know of

>
> If not, I can take a stab at fixing those if nobody is looking at them 
> already.

That would be great!

Alistair

>
> > 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