target/riscv/csr.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
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 */
© 2016 - 2024 Red Hat, Inc.