diff mbox series

[1/3] target/riscv: Assert that the CSR numbers will be correct

Message ID 20240108001328.280222-2-alistair.francis@wdc.com (mailing list archive)
State New, archived
Headers show
Series target/riscv: A few bug fixes and Coverity fix | expand

Commit Message

Alistair Francis Jan. 8, 2024, 12:13 a.m. UTC 
The CSRs will always be between either CSR_MHPMCOUNTER3 and
CSR_MHPMCOUNTER31 or CSR_MHPMCOUNTER3H and CSR_MHPMCOUNTER31H.

So although ctr_index can't be negative, Coverity doesn't know this and
it isn't obvious to human readers either. Let's add an assert to ensure
that Coverity knows the values will be within range.

To simplify the code let's also change the RV32 adjustment.

Fixes: Coverity CID 1523910
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/csr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Daniel Henrique Barboza Jan. 8, 2024, 4:34 p.m. UTC | #1 
On 1/7/24 21:13, Alistair Francis wrote:
> The CSRs will always be between either CSR_MHPMCOUNTER3 and
> CSR_MHPMCOUNTER31 or CSR_MHPMCOUNTER3H and CSR_MHPMCOUNTER31H.
> 
> So although ctr_index can't be negative, Coverity doesn't know this and
> it isn't obvious to human readers either. Let's add an assert to ensure
> that Coverity knows the values will be within range.
> 
> To simplify the code let's also change the RV32 adjustment.
> 
> Fixes: Coverity CID 1523910
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/csr.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fde7ce1a53..336ec7eda7 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -195,8 +195,11 @@ static RISCVException mctr(CPURISCVState *env, int csrno)
>   
>       if ((riscv_cpu_mxl(env) == MXL_RV32) && csrno >= CSR_MCYCLEH) {
>           /* Offset for RV32 mhpmcounternh counters */
> -        base_csrno += 0x80;
> +        csrno -= 0x80;
>       }
> +
> +    g_assert(csrno >= CSR_MHPMCOUNTER3 && csrno <= CSR_MHPMCOUNTER31);
> +
>       ctr_index = csrno - base_csrno;


After this change 'base_csrno' is now used to store CSR_MHPMCOUNTER3 and nothing
else. You can get rid of it, although having the var is arguably more readable.

Regardless, this LGTM:

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


>       if ((BIT(ctr_index) & pmu_avail_ctrs >> 3) == 0) {
>           /* The PMU is not enabled or counter is out of range */
diff mbox series

Patch

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index fde7ce1a53..336ec7eda7 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -195,8 +195,11 @@  static RISCVException mctr(CPURISCVState *env, int csrno)
 
     if ((riscv_cpu_mxl(env) == MXL_RV32) && csrno >= CSR_MCYCLEH) {
         /* Offset for RV32 mhpmcounternh counters */
-        base_csrno += 0x80;
+        csrno -= 0x80;
     }
+
+    g_assert(csrno >= CSR_MHPMCOUNTER3 && csrno <= CSR_MHPMCOUNTER31);
+
     ctr_index = csrno - base_csrno;
     if ((BIT(ctr_index) & pmu_avail_ctrs >> 3) == 0) {
         /* The PMU is not enabled or counter is out of range */