[PATCH] target/riscv: write back unmodified value for csrrc/csrrs with rs1 is not x0 but holding zero

[Weiwei Li]

     For csrrs and csrrc, if rs1 specifies a register other than x0, holding
     a zero value, the instruction will still attempt to write the unmodified
     value back to the csr and will cause side effects

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/csr.c       | 46 ++++++++++++++++++++++++++++------------
 target/riscv/op_helper.c |  7 +++++-
 2 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index aea82dff4a..f4774ca07b 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2872,7 +2872,7 @@ static RISCVException write_upmbase(CPURISCVState *env, 
int csrno,
 
 static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
                                                int csrno,
-                                               bool write_mask,
+                                               bool write_csr,
                                                RISCVCPU *cpu)
 {
     /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
@@ -2895,7 +2895,7 @@ static inline RISCVException 
riscv_csrrw_check(CPURISCVState *env,
         return RISCV_EXCP_ILLEGAL_INST;
     }
 #endif
-    if (write_mask && read_only) {
+    if (write_csr && read_only) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
@@ -2915,7 +2915,8 @@ static inline RISCVException 
riscv_csrrw_check(CPURISCVState *env,
 static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
                                        target_ulong *ret_value,
                                        target_ulong new_value,
-                                       target_ulong write_mask)
+                                       target_ulong write_mask,
+                                       bool write_csr)
 {
     RISCVException ret;
     target_ulong old_value;
@@ -2935,8 +2936,8 @@ static RISCVException riscv_csrrw_do64(CPURISCVState 
*env, int csrno,
         return ret;
     }
 
-    /* write value if writable and write mask set, otherwise drop writes */
-    if (write_mask) {
+    /* write value if needed, otherwise drop writes */
+    if (write_csr) {
         new_value = (old_value & ~write_mask) | (new_value & write_mask);
         if (csr_ops[csrno].write) {
             ret = csr_ops[csrno].write(env, csrno, new_value);
@@ -2960,18 +2961,27 @@ RISCVException riscv_csrrw(CPURISCVState *env, int 
csrno,
 {
     RISCVCPU *cpu = env_archcpu(env);
 
-    RISCVException ret = riscv_csrrw_check(env, csrno, write_mask, cpu);
+    /*
+     * write value when write_mask is set or rs1 is not x0 but holding zero
+     * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
+     */
+    bool write_csr = write_mask || ((write_mask == 0) &&
+                                    ((new_value == 0) ||
+                                     (new_value == (target_ulong)-1)));
+
+    RISCVException ret = riscv_csrrw_check(env, csrno, write_csr, cpu);
     if (ret != RISCV_EXCP_NONE) {
         return ret;
     }
 
-    return riscv_csrrw_do64(env, csrno, ret_value, new_value, write_mask);
+    return riscv_csrrw_do64(env, csrno, ret_value, new_value, write_mask,
+                            write_csr);
 }
 
 static RISCVException riscv_csrrw_do128(CPURISCVState *env, int csrno,
                                         Int128 *ret_value,
                                         Int128 new_value,
-                                        Int128 write_mask)
+                                        Int128 write_mask, bool write_csr)
 {
     RISCVException ret;
     Int128 old_value;
@@ -2982,8 +2992,8 @@ static RISCVException riscv_csrrw_do128(CPURISCVState 
*env, int csrno,
         return ret;
     }
 
-    /* write value if writable and write mask set, otherwise drop writes */
-    if (int128_nz(write_mask)) {
+    /* write value if needed, otherwise drop writes */
+    if (write_csr) {
         new_value = int128_or(int128_and(old_value, int128_not(write_mask)),
                               int128_and(new_value, write_mask));
         if (csr_ops[csrno].write128) {
@@ -3015,13 +3025,22 @@ RISCVException riscv_csrrw_i128(CPURISCVState *env, int 
csrno,
     RISCVException ret;
     RISCVCPU *cpu = env_archcpu(env);
 
-    ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask), cpu);
+    /*
+     * write value when write_mask is set or rs1 is not x0 but holding zero
+     * value for csrrc(new_value is zero) and csrrs(new_value is all-ones)
+     */
+    bool write_csr = write_mask || ((write_mask == 0) &&
+                                    ((new_value == 0) ||
+                                     (new_value == UINT128_MAX)));
+
+    ret = riscv_csrrw_check(env, csrno, write_csr, cpu);
     if (ret != RISCV_EXCP_NONE) {
         return ret;
     }
 
     if (csr_ops[csrno].read128) {
-        return riscv_csrrw_do128(env, csrno, ret_value, new_value, write_mask);
+        return riscv_csrrw_do128(env, csrno, ret_value, new_value, write_mask,
+                                 write_csr);
     }
 
     /*
@@ -3033,7 +3052,8 @@ RISCVException riscv_csrrw_i128(CPURISCVState *env, int 
csrno,
     target_ulong old_value;
     ret = riscv_csrrw_do64(env, csrno, &old_value,
                            int128_getlo(new_value),
-                           int128_getlo(write_mask));
+                           int128_getlo(write_mask),
+                           write_csr);
     if (ret == RISCV_EXCP_NONE && ret_value) {
         *ret_value = int128_make64(old_value);
     }
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 1a75ba11e6..b2ad465533 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -40,7 +40,12 @@ void helper_raise_exception(CPURISCVState *env, uint32_t 
exception)
 target_ulong helper_csrr(CPURISCVState *env, int csr)
 {
     target_ulong val = 0;
-    RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
+
+    /*
+     * new_value here should be none-zero or none-all-ones here to
+     * distinguish with csrrc/csrrs with rs1 is not x0 but holding zero value
+     */
+    RISCVException ret = riscv_csrrw(env, csr, &val, 1, 0);
 
     if (ret != RISCV_EXCP_NONE) {
         riscv_raise_exception(env, ret, GETPC());
-- 
2.17.1