On Fri, Nov 10, 2023 at 1:24 AM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Tue, 7 Nov 2023 at 02:36, Alistair Francis <alistai...@gmail.com> wrote:
> >
> > From: Rob Bradford <rbradf...@rivosinc.com>
> >
> > Check the PMU available bitmask when checking if a counter is valid
> > rather than comparing the index against the number of PMUs.
> >
> > Signed-off-by: Rob Bradford <rbradf...@rivosinc.com>
> > Reviewed-by: LIU Zhiwei <zhiwei_...@linux.alibaba.com>
> > Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
> > Reviewed-by: Atish Patra <ati...@rivosinc.com>
> > Message-ID: <20231031154000.18134-3-rbradf...@rivosinc.com>
> > Signed-off-by: Alistair Francis <alistair.fran...@wdc.com>
> > ---
> > target/riscv/csr.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index fc26b52c88..fde7ce1a53 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -188,7 +188,8 @@ static RISCVException zcmt(CPURISCVState *env, int
> > csrno)
> > #if !defined(CONFIG_USER_ONLY)
> > static RISCVException mctr(CPURISCVState *env, int csrno)
> > {
> > - int pmu_num = riscv_cpu_cfg(env)->pmu_num;
> > + RISCVCPU *cpu = env_archcpu(env);
> > + uint32_t pmu_avail_ctrs = cpu->pmu_avail_ctrs;
> > int ctr_index;
> > int base_csrno = CSR_MHPMCOUNTER3;
> >
> > @@ -197,7 +198,7 @@ static RISCVException mctr(CPURISCVState *env, int
> > csrno)
> > base_csrno += 0x80;
> > }
> > ctr_index = csrno - base_csrno;
> > - if (!pmu_num || ctr_index >= pmu_num) {
> > + if ((BIT(ctr_index) & pmu_avail_ctrs >> 3) == 0) {
> > /* The PMU is not enabled or counter is out of range */
> > return RISCV_EXCP_ILLEGAL_INST;
> > }
>
> Hi; Coverity is not convinced that ctr_index is necessarily
> guaranteed to be within the valid range to be an argument
> to BIT() (eg that it won't be negative). Looking at the
> code as a human I'm pretty unsure too. Could somebody have
> a look at this and maybe improve the readability / add an
> assertion / fix a bug if any ? (CID 1523910)
The code looks ok to me. I have a patch to add an assert to keep Coverity happy.
>
> More generally there are about half a dozen other riscv
> issues in Coverity at the moment, so if somebody who knows
> the riscv code could have a look at them that would be great.
I am happy to look at it. I didn't realise we could all see the
Coverity data. I just requested permission to see the results
Alistair
>
> thanks
> -- PMM
