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 <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;
>