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

[PATCH 02/18] target/riscv: Correct the priority policy of riscv_csrrw_check()

Posted by Bin Meng 2 years, 11 months ago

The priority policy of riscv_csrrw_check() was once adjusted in
commit eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
whose commit message says the CSR existence check should come
before the access control check, but the code changes did not
agree with the commit message, that the predicate() check came
after the read / write check.

Fixes: eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 target/riscv/csr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 1b0a0c1693..c2dd9d5af0 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3793,15 +3793,15 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
-    if (write_mask && read_only) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-
     RISCVException ret = csr_ops[csrno].predicate(env, csrno);
     if (ret != RISCV_EXCP_NONE) {
         return ret;
     }
 
+    if (write_mask && read_only) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
 #if !defined(CONFIG_USER_ONLY)
     int csr_priv, effective_priv = env->priv;
 
-- 
2.25.1

Re: [PATCH 02/18] target/riscv: Correct the priority policy of riscv_csrrw_check()

Posted by LIU Zhiwei 2 years, 11 months ago

On 2023/2/14 2:01, Bin Meng wrote:
> The priority policy of riscv_csrrw_check() was once adjusted in
> commit eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
> whose commit message says the CSR existence check should come
> before the access control check, but the code changes did not
> agree with the commit message, that the predicate() check came
> after the read / write check.
>
> Fixes: eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 1b0a0c1693..c2dd9d5af0 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3793,15 +3793,15 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> -    if (write_mask && read_only) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
>       RISCVException ret = csr_ops[csrno].predicate(env, csrno);
>       if (ret != RISCV_EXCP_NONE) {
>           return ret;
>       }
>   
> +    if (write_mask && read_only) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +

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

Zhiwei

>   #if !defined(CONFIG_USER_ONLY)
>       int csr_priv, effective_priv = env->priv;
>

Re: [PATCH 02/18] target/riscv: Correct the priority policy of riscv_csrrw_check()

Posted by weiwei 2 years, 11 months ago

On 2023/2/14 02:01, Bin Meng wrote:
> The priority policy of riscv_csrrw_check() was once adjusted in
> commit eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
> whose commit message says the CSR existence check should come
> before the access control check, but the code changes did not
> agree with the commit message, that the predicate() check came
> after the read / write check.
Hi Bin Meng,

Let me explain why I put read-only check before predicate() check in 
commit eacaf4401956:

*  The predicates don't do existence check only. They also do some 
access-control check , and

     will trigger virtual instruction exception in come cases.  I think 
read-only check should be done

     before these access-control check, and trigger illegal instruction 
exception instead of virtual

    instruction exception when writing to read-only CSRs in these cases.

  *  Read-only  check will trigger ILLEGAL_INST exception which is  also 
the exception triggered when

      CSR is not existed, so put this check before existence check will 
not affect the final exception.

Regards,

Weiwei Li

> Fixes: eacaf4401956 ("target/riscv: Fix priority of csr related check in riscv_csrrw_check")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   target/riscv/csr.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 1b0a0c1693..c2dd9d5af0 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3793,15 +3793,15 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> -    if (write_mask && read_only) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -
>       RISCVException ret = csr_ops[csrno].predicate(env, csrno);
>       if (ret != RISCV_EXCP_NONE) {
>           return ret;
>       }
>   
> +    if (write_mask && read_only) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
>   #if !defined(CONFIG_USER_ONLY)
>       int csr_priv, effective_priv = env->priv;
>