| Message ID | 20230213180215.1524938-3-bmeng@tinylab.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Various fixes to gdbstub and CSR access | expand |
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; >
On 2023/2/14 2: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. > > 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; > + } > + Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei > #if !defined(CONFIG_USER_ONLY) > int csr_priv, effective_priv = env->priv; >
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;
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. 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(-)