1. Patchew
  2. QEMU
  3. View series

[PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64

Nikita Shubin posted 1 patch 2 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230808090914.17634-1-nikita.shubin@maquefel.me
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liweiwei@iscas.ac.cn>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
target/riscv/csr.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
[PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64
Posted by Nikita Shubin 2 years, 6 months ago 
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
Re: [PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64
Posted by Alistair Francis 2 years, 5 months ago 
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
>
>
Re: [PATCH v3] target/riscv: don't read CSR in riscv_csrrw_do64
Posted by Alistair Francis 2 years, 5 months ago 
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
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.