[v2] target/riscv: Various fixes to gdbstub and CSR access

[PATCH v2 03/18] target/riscv: Use g_assert() for the predicate() NULL check

Posted by Bin Meng 2 years, 11 months ago

At present riscv_csrrw_check() checks the CSR predicate() against
NULL and throws RISCV_EXCP_ILLEGAL_INST if it is NULL. But this is
a pure software check, and has nothing to do with the emulation of
the hardware behavior, thus it is inappropriate to return illegal
instruction exception when software forgets to install the hook.

Change to use g_assert() instead.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

Changes in v2:
- new patch: Use assert() for the predicate() NULL check

 target/riscv/csr.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 4cc2c6370f..cfd7ffc5c2 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3786,11 +3786,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
-    /* check predicate */
-    if (!csr_ops[csrno].predicate) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
     /* read / write check */
     if (write_mask && read_only) {
         return RISCV_EXCP_ILLEGAL_INST;
@@ -3803,6 +3798,7 @@ 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;
-- 
2.25.1

Re: [PATCH v2 03/18] target/riscv: Use g_assert() for the predicate() NULL check

Posted by LIU Zhiwei 2 years, 11 months ago

On 2023/2/28 18:40, Bin Meng wrote:
> At present riscv_csrrw_check() checks the CSR predicate() against
> NULL and throws RISCV_EXCP_ILLEGAL_INST if it is NULL. But this is
> a pure software check, and has nothing to do with the emulation of
> the hardware behavior, thus it is inappropriate to return illegal
> instruction exception when software forgets to install the hook.
>
> Change to use g_assert() instead.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
> Changes in v2:
> - new patch: Use assert() for the predicate() NULL check
>
>   target/riscv/csr.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 4cc2c6370f..cfd7ffc5c2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3786,11 +3786,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> -    /* check predicate */
> -    if (!csr_ops[csrno].predicate) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
>       /* read / write check */
>       if (write_mask && read_only) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -3803,6 +3798,7 @@ 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);

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       RISCVException ret = csr_ops[csrno].predicate(env, csrno);
>       if (ret != RISCV_EXCP_NONE) {
>           return ret;

Re: [PATCH v2 03/18] target/riscv: Use g_assert() for the predicate() NULL check

Posted by liweiwei 2 years, 11 months ago

On 2023/2/28 18:40, Bin Meng wrote:
> At present riscv_csrrw_check() checks the CSR predicate() against
> NULL and throws RISCV_EXCP_ILLEGAL_INST if it is NULL. But this is
> a pure software check, and has nothing to do with the emulation of
> the hardware behavior, thus it is inappropriate to return illegal
> instruction exception when software forgets to install the hook.
>
> Change to use g_assert() instead.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li<liweiwei@iscas.ac.cn>

Weiwei Li
> ---
>
> Changes in v2:
> - new patch: Use assert() for the predicate() NULL check
>
>   target/riscv/csr.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 4cc2c6370f..cfd7ffc5c2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3786,11 +3786,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> -    /* check predicate */
> -    if (!csr_ops[csrno].predicate) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
>       /* read / write check */
>       if (write_mask && read_only) {
>           return RISCV_EXCP_ILLEGAL_INST;
> @@ -3803,6 +3798,7 @@ 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;