On Sat, 20 Jul 2024 at 16:19, Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Thu, 18 Jul 2024 at 03:15, Alistair Francis <alistai...@gmail.com> wrote: > > > > From: Atish Patra <ati...@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 <dbarb...@ventanamicro.com> > > Signed-off-by: Atish Patra <ati...@rivosinc.com> > > Message-ID: <20240711-smcntrpmf_v7-v8-12-b7c38ae7b...@rivosinc.com> > > Signed-off-by: Alistair Francis <alistair.fran...@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