1. Patchew
  2. QEMU
  3. View series

[PATCH v2] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR

Yu-Ming Chang via posted 1 patch 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240311030852.53831-1-yumin686@andestech.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
There is a newer version of this series
target/riscv/cpu.h       |  2 ++
target/riscv/csr.c       | 17 ++++++++++++++---
target/riscv/op_helper.c |  2 +-
3 files changed, 17 insertions(+), 4 deletions(-)
[PATCH v2] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR
Posted by Yu-Ming Chang via 1 month, 2 weeks ago 
Both CSRRS and CSRRC always read the addressed CSR and cause any read side
effects regardless of rs1 and rd fields. Note that if rs1 specifies a register
holding a zero value other than x0, the instruction will still attempt to write
the unmodified value back to the CSR and will cause any attendant side effects.

So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies
a register holding a zero value, an illegal instruction exception should be
raised.

Signed-off-by: Yu-Ming Chang <yumin686@andestech.com>
---
This incorporated the comments from Richard. Thank you.

 target/riscv/cpu.h       |  2 ++
 target/riscv/csr.c       | 17 ++++++++++++++---
 target/riscv/op_helper.c |  2 +-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 5d291a7092..452841ae2f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -710,6 +710,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
 void riscv_cpu_update_mask(CPURISCVState *env);
 bool riscv_cpu_is_32bit(RISCVCPU *cpu);
 
+RISCVException riscv_csrr(CPURISCVState *env, int csrno,
+                          target_ulong *ret_value);
 RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
                            target_ulong *ret_value,
                            target_ulong new_value, target_ulong write_mask);
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d4e8ac13b9..0d14ba2ba5 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -4306,7 +4306,7 @@ static RISCVException rmw_seed(CPURISCVState *env, int csrno,
 
 static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
                                                int csrno,
-                                               bool write_mask)
+                                               bool write)
 {
     /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
     bool read_only = get_field(csrno, 0xC00) == 3;
@@ -4328,7 +4328,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
     }
 
     /* read / write check */
-    if (write_mask && read_only) {
+    if (write && read_only) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
@@ -4415,11 +4415,22 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
+RISCVException riscv_csrr(CPURISCVState *env, int csrno,
+                           target_ulong *ret_value)
+{
+    RISCVException ret = riscv_csrrw_check(env, csrno, false);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
+    return riscv_csrrw_do64(env, csrno, ret_value, 0, 0);
+}
+
 RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
                            target_ulong *ret_value,
                            target_ulong new_value, target_ulong write_mask)
 {
-    RISCVException ret = riscv_csrrw_check(env, csrno, write_mask);
+    RISCVException ret = riscv_csrrw_check(env, csrno, true);
     if (ret != RISCV_EXCP_NONE) {
         return ret;
     }
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index f414aaebdb..f3aa705be8 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -51,7 +51,7 @@ target_ulong helper_csrr(CPURISCVState *env, int csr)
     }
 
     target_ulong val = 0;
-    RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
+    RISCVException ret = riscv_csrr(env, csr, &val);
 
     if (ret != RISCV_EXCP_NONE) {
         riscv_raise_exception(env, ret, GETPC());
-- 
2.34.1
Re: [PATCH v2] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR
Posted by LIU Zhiwei 1 month, 2 weeks ago 
On 2024/3/11 11:08, Yu-Ming Chang wrote:
> Both CSRRS and CSRRC always read the addressed CSR and cause any read side
> effects regardless of rs1 and rd fields. Note that if rs1 specifies a register
> holding a zero value other than x0, the instruction will still attempt to write
> the unmodified value back to the CSR and will cause any attendant side effects.
>
> So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies
> a register holding a zero value, an illegal instruction exception should be
> raised.
>
> Signed-off-by: Yu-Ming Chang <yumin686@andestech.com>
> ---
> This incorporated the comments from Richard. Thank you.
>
>   target/riscv/cpu.h       |  2 ++
>   target/riscv/csr.c       | 17 ++++++++++++++---
>   target/riscv/op_helper.c |  2 +-
>   3 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 5d291a7092..452841ae2f 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -710,6 +710,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>   void riscv_cpu_update_mask(CPURISCVState *env);
>   bool riscv_cpu_is_32bit(RISCVCPU *cpu);
>   
> +RISCVException riscv_csrr(CPURISCVState *env, int csrno,
> +                          target_ulong *ret_value);
>   RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
>                              target_ulong *ret_value,
>                              target_ulong new_value, target_ulong write_mask);
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d4e8ac13b9..0d14ba2ba5 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -4306,7 +4306,7 @@ static RISCVException rmw_seed(CPURISCVState *env, int csrno,
>   
>   static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>                                                  int csrno,
> -                                               bool write_mask)
> +                                               bool write)
>   {
>       /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
>       bool read_only = get_field(csrno, 0xC00) == 3;
> @@ -4328,7 +4328,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>       }
>   
>       /* read / write check */
> -    if (write_mask && read_only) {
> +    if (write && read_only) {
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> @@ -4415,11 +4415,22 @@ static RISCVException riscv_csrrw_do64(CPURISCVState *env, int csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> +RISCVException riscv_csrr(CPURISCVState *env, int csrno,
> +                           target_ulong *ret_value)
> +{
> +    RISCVException ret = riscv_csrrw_check(env, csrno, false);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
> +    return riscv_csrrw_do64(env, csrno, ret_value, 0, 0);
> +}
> +
>   RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
>                              target_ulong *ret_value,
>                              target_ulong new_value, target_ulong write_mask)
>   {
> -    RISCVException ret = riscv_csrrw_check(env, csrno, write_mask);
> +    RISCVException ret = riscv_csrrw_check(env, csrno, true);
>       if (ret != RISCV_EXCP_NONE) {
>           return ret;
>       }
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index f414aaebdb..f3aa705be8 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -51,7 +51,7 @@ target_ulong helper_csrr(CPURISCVState *env, int csr)
>       }
>   
>       target_ulong val = 0;
> -    RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
> +    RISCVException ret = riscv_csrr(env, csr, &val);
>   
>       if (ret != RISCV_EXCP_NONE) {
>           riscv_raise_exception(env, ret, GETPC());

Hi Yu-Ming,

The 128-bit CSR operations have the similar errors. Could you solve the 
similar bug in this patch set?

Otherwise,

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Thanks,
Zhiwei
Re: [PATCH v2] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR
Posted by Richard Henderson 1 month, 2 weeks ago 
On 3/10/24 17:08, Yu-Ming Chang via wrote:
> Both CSRRS and CSRRC always read the addressed CSR and cause any read side
> effects regardless of rs1 and rd fields. Note that if rs1 specifies a register
> holding a zero value other than x0, the instruction will still attempt to write
> the unmodified value back to the CSR and will cause any attendant side effects.
> 
> So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies
> a register holding a zero value, an illegal instruction exception should be
> raised.
> 
> Signed-off-by: Yu-Ming Chang<yumin686@andestech.com>
> ---
> This incorporated the comments from Richard. Thank you.
> 
>   target/riscv/cpu.h       |  2 ++
>   target/riscv/csr.c       | 17 ++++++++++++++---
>   target/riscv/op_helper.c |  2 +-
>   3 files changed, 17 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.