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

[Michael Tokarev]

08.05.2023 01:21, Alistair Francis wrote:

On Fri, May 5, 2023 at 11:08 AM Alistair Francis  wrote:


From: Bin Meng 

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 
Signed-off-by: Bin Meng 
Reviewed-by: Daniel Henrique Barboza 
Reviewed-by: Weiwei Li 
Reviewed-by: Alistair Francis 
Reviewed-by: LIU Zhiwei 
Message-Id: <20230417043054.3125614-1-bm...@tinylab.org>
Signed-off-by: Alistair Francis 


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

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

[Alistair Francis]

On Fri, May 5, 2023 at 11:08 AM Alistair Francis  wrote:
>
> From: Bin Meng 
>
> 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 
> Signed-off-by: Bin Meng 
> Reviewed-by: Daniel Henrique Barboza 
> Reviewed-by: Weiwei Li 
> Reviewed-by: Alistair Francis 
> Reviewed-by: LIU Zhiwei 
> Message-Id: <20230417043054.3125614-1-bm...@tinylab.org>
> Signed-off-by: Alistair Francis 

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
>

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

[Alistair Francis]

From: Bin Meng 

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 
Signed-off-by: Bin Meng 
Reviewed-by: Daniel Henrique Barboza 
Reviewed-by: Weiwei Li 
Reviewed-by: Alistair Francis 
Reviewed-by: LIU Zhiwei 
Message-Id: <20230417043054.3125614-1-bm...@tinylab.org>
Signed-off-by: Alistair Francis 
---
 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