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

[weiwei]

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 <bm...@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;