From: Nikita Shubin <n.shubin@yadro.com>

In case of write-only CSR don't return illegal inst error when CSR is
written and lacks read op.

Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
Changelog:
- fixed uninitialized old_value

Anyway it not might be a good idea to read CSR when we are not asked
for, during CSRRW or CSRRWI:

"For CSRRWI, if rd=x0, then the instruction shall not read the CSR and
shall not cause any of the side effects that might occur on a CSR read."

May be i am missing something of course.
---
 target/riscv/csr.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ea7585329e..3f0b3277e4 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3908,21 +3908,24 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
                                        target_ulong write_mask)
 {
     RISCVException ret;
-    target_ulong old_value;
+    target_ulong old_value = 0;
 
     /* execute combined read/write operation if it exists */
     if (csr_ops[csrno].op) {
         return csr_ops[csrno].op(env, csrno, ret_value, new_value, write_mask);
     }
 
-    /* if no accessor exists then return failure */
-    if (!csr_ops[csrno].read) {
-        return RISCV_EXCP_ILLEGAL_INST;
-    }
-    /* read old value */
-    ret = csr_ops[csrno].read(env, csrno, &old_value);
-    if (ret != RISCV_EXCP_NONE) {
-        return ret;
+    /* don't read if ret_value==NULL */
+    if (ret_value) {
+        /* if no accessor exists then return failure */
+        if (!csr_ops[csrno].read) {
+            return RISCV_EXCP_ILLEGAL_INST;
+        }
+        /* read old value */
+        ret = csr_ops[csrno].read(env, csrno, &old_value);
+        if (ret != RISCV_EXCP_NONE) {
+            return ret;
+        }
     }
 
     /* write value if writable and write mask set, otherwise drop writes */
-- 
2.39.2

Patch looks correct IMHO. I have a few cosmetic suggestions:

On 7/27/23 05:17, Nikita Shubin wrote:
> From: Nikita Shubin <n.shubin@yadro.com>
> 
> In case of write-only CSR don't return illegal inst error when CSR is
> written and lacks read op.


It's worth citing the ISA bits you put down below in the commit msg.

It would also be nice to explain why checking for 'ret_value != NULL' is the
right fix. In this case, both trans_csrrw() and trans_csrrwi() will check if
rd=x0 and, in this case, will redirect to do_csrw(). do_csrw() will call
riscv_csrrw_do64(), via helper_csrw(), with ret_val = NULL.

And ...

> 
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
> Changelog:
> - fixed uninitialized old_value
> 
> Anyway it not might be a good idea to read CSR when we are not asked
> for, during CSRRW or CSRRWI:
> 
> "For CSRRWI, if rd=x0, then the instruction shall not read the CSR and
> shall not cause any of the side effects that might occur on a CSR read."
> 
> May be i am missing something of course.
> ---
>   target/riscv/csr.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index ea7585329e..3f0b3277e4 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3908,21 +3908,24 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
>                                          target_ulong write_mask)
>   {
>       RISCVException ret;
> -    target_ulong old_value;
> +    target_ulong old_value = 0;
>   
>       /* execute combined read/write operation if it exists */
>       if (csr_ops[csrno].op) {
>           return csr_ops[csrno].op(env, csrno, ret_value, new_value, write_mask);
>       }
>   
> -    /* if no accessor exists then return failure */
> -    if (!csr_ops[csrno].read) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    }
> -    /* read old value */
> -    ret = csr_ops[csrno].read(env, csrno, &old_value);
> -    if (ret != RISCV_EXCP_NONE) {
> -        return ret;
> +    /* don't read if ret_value==NULL */

I would add in this comment that ret_value == NULL means that rd=x0 and we're coming
from helper_csrw() and we can't throw side effects caused by CSR reads.


Thanks,

Daniel

> +    if (ret_value) {
> +        /* if no accessor exists then return failure */
> +        if (!csr_ops[csrno].read) {
> +            return RISCV_EXCP_ILLEGAL_INST;
> +        }
> +        /* read old value */
> +        ret = csr_ops[csrno].read(env, csrno, &old_value);
> +        if (ret != RISCV_EXCP_NONE) {
> +            return ret;
> +        }
>       }
>   
>       /* write value if writable and write mask set, otherwise drop writes */