* [PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64
@ 2023-08-08 9:09 Nikita Shubin
2023-09-07 3:01 ` Alistair Francis
2023-09-07 3:06 ` Alistair Francis
0 siblings, 2 replies; 3+ messages in thread
From: Nikita Shubin @ 2023-08-08 9:09 UTC (permalink / raw)
To: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei
Cc: Nikita Shubin, qemu-riscv, qemu-devel
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>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64
2023-08-08 9:09 [PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64 Nikita Shubin
@ 2023-09-07 3:01 ` Alistair Francis
2023-09-07 3:06 ` Alistair Francis
1 sibling, 0 replies; 3+ messages in thread
From: Alistair Francis @ 2023-09-07 3:01 UTC (permalink / raw)
To: Nikita Shubin
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, Nikita Shubin, qemu-riscv,
qemu-devel
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64
2023-08-08 9:09 [PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64 Nikita Shubin
2023-09-07 3:01 ` Alistair Francis
@ 2023-09-07 3:06 ` Alistair Francis
1 sibling, 0 replies; 3+ messages in thread
From: Alistair Francis @ 2023-09-07 3:06 UTC (permalink / raw)
To: Nikita Shubin
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, Nikita Shubin, qemu-riscv,
qemu-devel
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-07 3:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-08 9:09 [PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64 Nikita Shubin
2023-09-07 3:01 ` Alistair Francis
2023-09-07 3:06 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).