Patch looks correct IMHO. I have a few cosmetic suggestions:
On 7/27/23 05:17, Nikita Shubin wrote:
From: Nikita Shubin <n.shu...@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.shu...@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 */
