target/riscv: Fix MEPC/SEPC bit masking

[PATCH 1/2] target/riscv: Fix MEPC/SEPC bit masking for IALIGN

Posted by Charalampos Mitrodimas 9 months, 3 weeks ago

According to the RISC-V Privileged Architecture specification, the low
bit of MEPC/SEPC must always be zero. When IALIGN=32, the two low bits
must be zero.

This commit fixes the behavior of MEPC/SEPC CSR reads and writes, and
the implicit reads by MRET/SRET instructions to properly mask the
lowest bit(s) based on whether the C extension is enabled:
- When C extension is enabled (IALIGN=16): mask bit 0
- When C extension is disabled (IALIGN=32): mask bits [1:0]

Previously, when vectored mode bits from STVEC (which sets bit 0 for
vectored mode) were written to MEPC, the bits would not be cleared
correctly, causing incorrect behavior on MRET.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2855
Signed-off-by: Charalampos Mitrodimas <charmitro@posteo.net>
---
 target/riscv/csr.c       |  8 ++++----
 target/riscv/internals.h | 11 +++++++++++
 target/riscv/op_helper.c |  4 ++--
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index fb14972169..c33a6e86d2 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3126,14 +3126,14 @@ static RISCVException write_mscratch(CPURISCVState *env, int csrno,
 static RISCVException read_mepc(CPURISCVState *env, int csrno,
                                 target_ulong *val)
 {
-    *val = env->mepc;
+    *val = env->mepc & get_xepc_mask(env);
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException write_mepc(CPURISCVState *env, int csrno,
                                  target_ulong val, uintptr_t ra)
 {
-    env->mepc = val;
+    env->mepc = val & get_xepc_mask(env);
     return RISCV_EXCP_NONE;
 }
 
@@ -4113,14 +4113,14 @@ static RISCVException write_sscratch(CPURISCVState *env, int csrno,
 static RISCVException read_sepc(CPURISCVState *env, int csrno,
                                 target_ulong *val)
 {
-    *val = env->sepc;
+    *val = env->sepc & get_xepc_mask(env);
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException write_sepc(CPURISCVState *env, int csrno,
                                  target_ulong val, uintptr_t ra)
 {
-    env->sepc = val;
+    env->sepc = val & get_xepc_mask(env);
     return RISCV_EXCP_NONE;
 }
 
diff --git a/target/riscv/internals.h b/target/riscv/internals.h
index 4570bd50be..89ac6a160f 100644
--- a/target/riscv/internals.h
+++ b/target/riscv/internals.h
@@ -142,6 +142,17 @@ static inline float16 check_nanbox_h(CPURISCVState *env, uint64_t f)
     }
 }
 
+static inline target_ulong get_xepc_mask(CPURISCVState *env)
+{
+    /* When IALIGN=32, both low bits must be zero.
+     * When IALIGN=16 (has C extension), only bit 0 must be zero. */
+    if (riscv_has_ext(env, RVC)) {
+        return ~(target_ulong)1;
+    } else {
+        return ~(target_ulong)3;
+    }
+}
+
 #ifndef CONFIG_USER_ONLY
 /* Our implementation of SysemuCPUOps::has_work */
 bool riscv_cpu_has_work(CPUState *cs);
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 557807ba4b..15460bf84b 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -280,7 +280,7 @@ target_ulong helper_sret(CPURISCVState *env)
         riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
     }
 
-    target_ulong retpc = env->sepc;
+    target_ulong retpc = env->sepc & get_xepc_mask(env);
     if (!riscv_cpu_allow_16bit_insn(&env_archcpu(env)->cfg,
                                     env->priv_ver,
                                     env->misa_ext) && (retpc & 0x3)) {
@@ -391,7 +391,7 @@ static target_ulong ssdbltrp_mxret(CPURISCVState *env, target_ulong mstatus,
 
 target_ulong helper_mret(CPURISCVState *env)
 {
-    target_ulong retpc = env->mepc;
+    target_ulong retpc = env->mepc & get_xepc_mask(env);
     uint64_t mstatus = env->mstatus;
     target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
 
-- 
2.47.2

Re: [PATCH 1/2] target/riscv: Fix MEPC/SEPC bit masking for IALIGN

Posted by Alistair Francis 9 months, 3 weeks ago

On Fri, Jul 4, 2025 at 4:25 AM Charalampos Mitrodimas
<charmitro@posteo.net> wrote:
>
> According to the RISC-V Privileged Architecture specification, the low
> bit of MEPC/SEPC must always be zero. When IALIGN=32, the two low bits
> must be zero.
>
> This commit fixes the behavior of MEPC/SEPC CSR reads and writes, and
> the implicit reads by MRET/SRET instructions to properly mask the
> lowest bit(s) based on whether the C extension is enabled:
> - When C extension is enabled (IALIGN=16): mask bit 0
> - When C extension is disabled (IALIGN=32): mask bits [1:0]
>
> Previously, when vectored mode bits from STVEC (which sets bit 0 for
> vectored mode) were written to MEPC, the bits would not be cleared
> correctly, causing incorrect behavior on MRET.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2855
> Signed-off-by: Charalampos Mitrodimas <charmitro@posteo.net>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/csr.c       |  8 ++++----
>  target/riscv/internals.h | 11 +++++++++++
>  target/riscv/op_helper.c |  4 ++--
>  3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fb14972169..c33a6e86d2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3126,14 +3126,14 @@ static RISCVException write_mscratch(CPURISCVState *env, int csrno,
>  static RISCVException read_mepc(CPURISCVState *env, int csrno,
>                                  target_ulong *val)
>  {
> -    *val = env->mepc;
> +    *val = env->mepc & get_xepc_mask(env);
>      return RISCV_EXCP_NONE;
>  }
>
>  static RISCVException write_mepc(CPURISCVState *env, int csrno,
>                                   target_ulong val, uintptr_t ra)
>  {
> -    env->mepc = val;
> +    env->mepc = val & get_xepc_mask(env);
>      return RISCV_EXCP_NONE;
>  }
>
> @@ -4113,14 +4113,14 @@ static RISCVException write_sscratch(CPURISCVState *env, int csrno,
>  static RISCVException read_sepc(CPURISCVState *env, int csrno,
>                                  target_ulong *val)
>  {
> -    *val = env->sepc;
> +    *val = env->sepc & get_xepc_mask(env);
>      return RISCV_EXCP_NONE;
>  }
>
>  static RISCVException write_sepc(CPURISCVState *env, int csrno,
>                                   target_ulong val, uintptr_t ra)
>  {
> -    env->sepc = val;
> +    env->sepc = val & get_xepc_mask(env);
>      return RISCV_EXCP_NONE;
>  }
>
> diff --git a/target/riscv/internals.h b/target/riscv/internals.h
> index 4570bd50be..89ac6a160f 100644
> --- a/target/riscv/internals.h
> +++ b/target/riscv/internals.h
> @@ -142,6 +142,17 @@ static inline float16 check_nanbox_h(CPURISCVState *env, uint64_t f)
>      }
>  }
>
> +static inline target_ulong get_xepc_mask(CPURISCVState *env)
> +{
> +    /* When IALIGN=32, both low bits must be zero.
> +     * When IALIGN=16 (has C extension), only bit 0 must be zero. */
> +    if (riscv_has_ext(env, RVC)) {
> +        return ~(target_ulong)1;
> +    } else {
> +        return ~(target_ulong)3;
> +    }
> +}
> +
>  #ifndef CONFIG_USER_ONLY
>  /* Our implementation of SysemuCPUOps::has_work */
>  bool riscv_cpu_has_work(CPUState *cs);
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 557807ba4b..15460bf84b 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -280,7 +280,7 @@ target_ulong helper_sret(CPURISCVState *env)
>          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>      }
>
> -    target_ulong retpc = env->sepc;
> +    target_ulong retpc = env->sepc & get_xepc_mask(env);
>      if (!riscv_cpu_allow_16bit_insn(&env_archcpu(env)->cfg,
>                                      env->priv_ver,
>                                      env->misa_ext) && (retpc & 0x3)) {
> @@ -391,7 +391,7 @@ static target_ulong ssdbltrp_mxret(CPURISCVState *env, target_ulong mstatus,
>
>  target_ulong helper_mret(CPURISCVState *env)
>  {
> -    target_ulong retpc = env->mepc;
> +    target_ulong retpc = env->mepc & get_xepc_mask(env);
>      uint64_t mstatus = env->mstatus;
>      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
>
> --
> 2.47.2
>
>

Re: [PATCH 1/2] target/riscv: Fix MEPC/SEPC bit masking for IALIGN

Posted by Daniel Henrique Barboza 9 months, 3 weeks ago


On 7/3/25 3:21 PM, Charalampos Mitrodimas wrote:
> According to the RISC-V Privileged Architecture specification, the low
> bit of MEPC/SEPC must always be zero. When IALIGN=32, the two low bits
> must be zero.
> 
> This commit fixes the behavior of MEPC/SEPC CSR reads and writes, and
> the implicit reads by MRET/SRET instructions to properly mask the
> lowest bit(s) based on whether the C extension is enabled:
> - When C extension is enabled (IALIGN=16): mask bit 0
> - When C extension is disabled (IALIGN=32): mask bits [1:0]
> 
> Previously, when vectored mode bits from STVEC (which sets bit 0 for
> vectored mode) were written to MEPC, the bits would not be cleared
> correctly, causing incorrect behavior on MRET.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2855
> Signed-off-by: Charalampos Mitrodimas <charmitro@posteo.net>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   target/riscv/csr.c       |  8 ++++----
>   target/riscv/internals.h | 11 +++++++++++
>   target/riscv/op_helper.c |  4 ++--
>   3 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fb14972169..c33a6e86d2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3126,14 +3126,14 @@ static RISCVException write_mscratch(CPURISCVState *env, int csrno,
>   static RISCVException read_mepc(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>   {
> -    *val = env->mepc;
> +    *val = env->mepc & get_xepc_mask(env);
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_mepc(CPURISCVState *env, int csrno,
>                                    target_ulong val, uintptr_t ra)
>   {
> -    env->mepc = val;
> +    env->mepc = val & get_xepc_mask(env);
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -4113,14 +4113,14 @@ static RISCVException write_sscratch(CPURISCVState *env, int csrno,
>   static RISCVException read_sepc(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>   {
> -    *val = env->sepc;
> +    *val = env->sepc & get_xepc_mask(env);
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_sepc(CPURISCVState *env, int csrno,
>                                    target_ulong val, uintptr_t ra)
>   {
> -    env->sepc = val;
> +    env->sepc = val & get_xepc_mask(env);
>       return RISCV_EXCP_NONE;
>   }
>   
> diff --git a/target/riscv/internals.h b/target/riscv/internals.h
> index 4570bd50be..89ac6a160f 100644
> --- a/target/riscv/internals.h
> +++ b/target/riscv/internals.h
> @@ -142,6 +142,17 @@ static inline float16 check_nanbox_h(CPURISCVState *env, uint64_t f)
>       }
>   }
>   
> +static inline target_ulong get_xepc_mask(CPURISCVState *env)
> +{
> +    /* When IALIGN=32, both low bits must be zero.
> +     * When IALIGN=16 (has C extension), only bit 0 must be zero. */
> +    if (riscv_has_ext(env, RVC)) {
> +        return ~(target_ulong)1;
> +    } else {
> +        return ~(target_ulong)3;
> +    }
> +}
> +
>   #ifndef CONFIG_USER_ONLY
>   /* Our implementation of SysemuCPUOps::has_work */
>   bool riscv_cpu_has_work(CPUState *cs);
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 557807ba4b..15460bf84b 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -280,7 +280,7 @@ target_ulong helper_sret(CPURISCVState *env)
>           riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>       }
>   
> -    target_ulong retpc = env->sepc;
> +    target_ulong retpc = env->sepc & get_xepc_mask(env);
>       if (!riscv_cpu_allow_16bit_insn(&env_archcpu(env)->cfg,
>                                       env->priv_ver,
>                                       env->misa_ext) && (retpc & 0x3)) {
> @@ -391,7 +391,7 @@ static target_ulong ssdbltrp_mxret(CPURISCVState *env, target_ulong mstatus,
>   
>   target_ulong helper_mret(CPURISCVState *env)
>   {
> -    target_ulong retpc = env->mepc;
> +    target_ulong retpc = env->mepc & get_xepc_mask(env);
>       uint64_t mstatus = env->mstatus;
>       target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
>

[PATCH 1/2] target/riscv: Fix MEPC/SEPC bit masking for IALIGN
[PATCH 2/2] tests/tcg/riscv64: Add test for MEPC bit masking