Re: [PATCH v2] target/riscv: don't read write-only CSR

[Daniel Henrique Barboza]

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 */