Message ID | 20231107022946.1055027-47-alistair.francis@wdc.com |
---|---|
State | New |
Headers | show |
Series | [PULL,01/49] target/riscv: rename ext_ifencei to ext_zifencei | expand |
On Tue, 7 Nov 2023 at 02:36, Alistair Francis <alistair23@gmail.com> wrote: > > From: Rob Bradford <rbradford@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 <rbradford@rivosinc.com> > Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > Reviewed-by: Atish Patra <atishp@rivosinc.com> > Message-ID: <20231031154000.18134-3-rbradford@rivosinc.com> > Signed-off-by: Alistair Francis <alistair.francis@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) 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. thanks -- PMM
On Fri, Nov 10, 2023 at 1:24 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 7 Nov 2023 at 02:36, Alistair Francis <alistair23@gmail.com> wrote: > > > > From: Rob Bradford <rbradford@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 <rbradford@rivosinc.com> > > Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > Reviewed-by: Atish Patra <atishp@rivosinc.com> > > Message-ID: <20231031154000.18134-3-rbradford@rivosinc.com> > > Signed-off-by: Alistair Francis <alistair.francis@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
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; }