[PULL,46/49] target/riscv: Don't assume PMU counters are continuous

Commit Message

Alistair Francis Nov. 7, 2023, 2:29 a.m. UTC

From: Rob Bradford <[e-mail]>

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 <[e-mail]>
Reviewed-by: LIU Zhiwei <[e-mail]>
Reviewed-by: Alistair Francis <[e-mail]>
Reviewed-by: Atish Patra <[e-mail]>
Message-ID: <[e-mail]>
Signed-off-by: Alistair Francis <[e-mail]>
---
 target/riscv/csr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Peter Maydell Nov. 9, 2023, 3:24 p.m. UTC | #1

On Tue, 7 Nov 2023 at 02:36, Alistair Francis <[e-mail]> wrote:
>
> From: Rob Bradford <[e-mail]>
>
> 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 <[e-mail]>
> Reviewed-by: LIU Zhiwei <[e-mail]>
> Reviewed-by: Alistair Francis <[e-mail]>
> Reviewed-by: Atish Patra <[e-mail]>
> Message-ID: <[e-mail]>
> Signed-off-by: Alistair Francis <[e-mail]>
> ---
>  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

Alistair Francis Nov. 22, 2023, 5:52 a.m. UTC | #2

On Fri, Nov 10, 2023 at 1:24 AM Peter Maydell <[e-mail]> wrote:
>
> On Tue, 7 Nov 2023 at 02:36, Alistair Francis <[e-mail]> wrote:
> >
> > From: Rob Bradford <[e-mail]>
> >
> > 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 <[e-mail]>
> > Reviewed-by: LIU Zhiwei <[e-mail]>
> > Reviewed-by: Alistair Francis <[e-mail]>
> > Reviewed-by: Atish Patra <[e-mail]>
> > Message-ID: <[e-mail]>
> > Signed-off-by: Alistair Francis <[e-mail]>
> > ---
> >  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

Patch

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