| Message ID | 20230808090914.17634-1-nikita.shubin@maquefel.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] target/riscv: don't read CSR in riscv_csrrw_do64 | expand |
On Tue, Aug 8, 2023 at 7:10 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > From: Nikita Shubin <n.shubin@yadro.com> > > As per ISA: > > "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." > > trans_csrrwi() and trans_csrrw() call do_csrw() if rd=x0, do_csrw() calls > riscv_csrrw_do64(), via helper_csrw() passing NULL as *ret_value. > > Signed-off-by: Nikita Shubin <n.shubin@yadro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > Changelog v2: > - fixed uninitialized old_value > > Changelog v3: > - reword comment and commit message as Deniel suggested > > --- > target/riscv/csr.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index ea7585329e..c5564d6d53 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -3908,21 +3908,27 @@ 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; > + /* > + * 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. > + */ > + 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 > >
On Tue, Aug 8, 2023 at 7:10 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > From: Nikita Shubin <n.shubin@yadro.com> > > As per ISA: > > "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." > > trans_csrrwi() and trans_csrrw() call do_csrw() if rd=x0, do_csrw() calls > riscv_csrrw_do64(), via helper_csrw() passing NULL as *ret_value. > > Signed-off-by: Nikita Shubin <n.shubin@yadro.com> Thanks! Applied to riscv-to-apply.next Alistair > --- > Changelog v2: > - fixed uninitialized old_value > > Changelog v3: > - reword comment and commit message as Deniel suggested > > --- > target/riscv/csr.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index ea7585329e..c5564d6d53 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -3908,21 +3908,27 @@ 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; > + /* > + * 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. > + */ > + 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 > >
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index ea7585329e..c5564d6d53 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -3908,21 +3908,27 @@ 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; + /* + * 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. + */ + 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 */