diff mbox series

[PULL,86/89] target/riscv: Restore the predicate() NULL check behavior

Message ID 20230505010241.21812-87-alistair.francis@wdc.com (mailing list archive)
State New, archived
Headers show
Series [PULL,01/89] target/riscv: Avoid env_archcpu() when reading RISCVCPUConfig | expand

Commit Message

Alistair Francis May 5, 2023, 1:02 a.m. UTC 
From: Bin Meng <bmeng@tinylab.org>

When reading a non-existent CSR QEMU should raise illegal instruction
exception, but currently it just exits due to the g_assert() check.

This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617.
Some comments are also added to indicate that predicate() must be
provided for an implemented CSR.

Reported-by: Fei Wu <fei2.wu@intel.com>
Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Message-Id: <20230417043054.3125614-1-bmeng@tinylab.org>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/csr.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Alistair Francis May 7, 2023, 10:21 p.m. UTC | #1 
On Fri, May 5, 2023 at 11:08 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> From: Bin Meng <bmeng@tinylab.org>
>
> When reading a non-existent CSR QEMU should raise illegal instruction
> exception, but currently it just exits due to the g_assert() check.
>
> This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617.
> Some comments are also added to indicate that predicate() must be
> provided for an implemented CSR.
>
> Reported-by: Fei Wu <fei2.wu@intel.com>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> Message-Id: <20230417043054.3125614-1-bmeng@tinylab.org>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Sorry, I didn't realise I should have done this with the PR, but this
is a good candidate for going into 8.0.1

Alistair

> ---
>  target/riscv/csr.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 865ee9efda..4451bd1263 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3826,6 +3826,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>
> +    /* ensure CSR is implemented by checking predicate */
> +    if (!csr_ops[csrno].predicate) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
>      /* privileged spec version check */
>      if (env->priv_ver < csr_min_priv) {
>          return RISCV_EXCP_ILLEGAL_INST;
> @@ -3843,7 +3848,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>       * illegal instruction exception should be triggered instead of virtual
>       * instruction exception. Hence this comes after the read / write check.
>       */
> -    g_assert(csr_ops[csrno].predicate != NULL);
>      RISCVException ret = csr_ops[csrno].predicate(env, csrno);
>      if (ret != RISCV_EXCP_NONE) {
>          return ret;
> @@ -4032,7 +4036,10 @@ static RISCVException write_jvt(CPURISCVState *env, int csrno,
>      return RISCV_EXCP_NONE;
>  }
>
> -/* Control and Status Register function table */
> +/*
> + * Control and Status Register function table
> + * riscv_csr_operations::predicate() must be provided for an implemented CSR
> + */
>  riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>      /* User Floating-Point CSRs */
>      [CSR_FFLAGS]   = { "fflags",   fs,     read_fflags,  write_fflags },
> --
> 2.40.0
>
Michael Tokarev May 10, 2023, 5:18 p.m. UTC | #2 
08.05.2023 01:21, Alistair Francis wrote:
> On Fri, May 5, 2023 at 11:08 AM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> From: Bin Meng <bmeng@tinylab.org>
>>
>> When reading a non-existent CSR QEMU should raise illegal instruction
>> exception, but currently it just exits due to the g_assert() check.
>>
>> This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617.
>> Some comments are also added to indicate that predicate() must be
>> provided for an implemented CSR.
>>
>> Reported-by: Fei Wu <fei2.wu@intel.com>
>> Signed-off-by: Bin Meng <bmeng@tinylab.org>
>> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> Message-Id: <20230417043054.3125614-1-bmeng@tinylab.org>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> 
> Sorry, I didn't realise I should have done this with the PR, but this
> is a good candidate for going into 8.0.1

Queued for 8.0, with minor context tweak. Thank you!

/mjt
diff mbox series

Patch

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 865ee9efda..4451bd1263 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3826,6 +3826,11 @@  static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
+    /* ensure CSR is implemented by checking predicate */
+    if (!csr_ops[csrno].predicate) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
     /* privileged spec version check */
     if (env->priv_ver < csr_min_priv) {
         return RISCV_EXCP_ILLEGAL_INST;
@@ -3843,7 +3848,6 @@  static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
      * illegal instruction exception should be triggered instead of virtual
      * instruction exception. Hence this comes after the read / write check.
      */
-    g_assert(csr_ops[csrno].predicate != NULL);
     RISCVException ret = csr_ops[csrno].predicate(env, csrno);
     if (ret != RISCV_EXCP_NONE) {
         return ret;
@@ -4032,7 +4036,10 @@  static RISCVException write_jvt(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
-/* Control and Status Register function table */
+/*
+ * Control and Status Register function table
+ * riscv_csr_operations::predicate() must be provided for an implemented CSR
+ */
 riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     /* User Floating-Point CSRs */
     [CSR_FFLAGS]   = { "fflags",   fs,     read_fflags,  write_fflags },