Re: [PATCH] target/riscv: Fix CPU state access on big-endian hosts

Fri, 23 Oct 2026 01:08:42 -0700 

On 23/10/26 09:53, Philippe Mathieu-Daudé wrote:

On 23/10/26 09:37, Philippe Mathieu-Daudé wrote:

On 22/10/26 15:06, Anton Johansson via qemu development wrote:

mstatus and mstatus_hs are uint64_t but accessed through

   tcg_gen_[ld|st]_tl(..., offsetof(mstatus))

which on a big-endian host running riscv32 incorrectly accesses the
top 32 bits rather than the lower bits.


   mstatus and mstatus_hs are uint64_t, but mark_fs_dirty() and
   mark_vs_dirty() update them with tcg_gen_[ld|st]_tl().
   On big-endian hosts with 32-bit target_ulong, this modifies
   the upper half instead of the lower one, so the FS/VS dirty
   bits are never set. Use 64-bit accesses instead.

Cc: [email protected]
Fixes: 284d697c74e ("target/riscv: Merge m/vsstatus and m/vsstatush into one uint64_t unit") Fixes: 8e1ee1fb571 ("target/riscv: rvv-1.0: add translation-time vector context status") 
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>


Widen the operations to 64 bit so the lower bits are modified.

Signed-off-by: Anton Johansson <[email protected]>
---
  target/riscv/tcg/translate.c | 32 ++++++++++++++++----------------
  1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/target/riscv/tcg/translate.c b/target/riscv/tcg/translate.c
index ede9c2563f..79b0f444ef 100644
--- a/target/riscv/tcg/translate.c
+++ b/target/riscv/tcg/translate.c
@@ -693,7 +693,7 @@ static TCGv get_address_indexed(DisasContext *ctx, int rs1, TCGv offs) 
   */
  static void mark_fs_dirty(DisasContext *ctx)
  {
-    TCGv tmp;
+    TCGv_i64 tmp;
      if (!has_ext(ctx, RVF)) {
          return;
@@ -703,15 +703,15 @@ static void mark_fs_dirty(DisasContext *ctx)
          /* Remember the state change for the rest of the TB. */
          ctx->mstatus_fs = EXT_STATUS_DIRTY;
-        tmp = tcg_temp_new();
-        tcg_gen_ld_tl(tmp, tcg_env, offsetof(CPURISCVState, mstatus));
-        tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS);
-        tcg_gen_st_tl(tmp, tcg_env, offsetof(CPURISCVState, mstatus));
+        tmp = tcg_temp_new_i64();
+        tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPURISCVState, mstatus));
+        tcg_gen_ori_i64(tmp, tmp, MSTATUS_FS);
+        tcg_gen_st_i64(tmp, tcg_env, offsetof(CPURISCVState, mstatus));
          if (ctx->virt_enabled) {
-            tcg_gen_ld_tl(tmp, tcg_env, offsetof(CPURISCVState, mstatus_hs)); 
-            tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS);
-            tcg_gen_st_tl(tmp, tcg_env, offsetof(CPURISCVState, mstatus_hs)); +            tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPURISCVState, mstatus_hs)); 
+            tcg_gen_ori_i64(tmp, tmp, MSTATUS_FS);
+            tcg_gen_st_i64(tmp, tcg_env, offsetof(CPURISCVState, mstatus_hs)); 
          }
      }
  }


Wondering about being safe (or catching other bad uses)

#define TCG_ENV_OFS_(f)     offsetof(CPUArchState, f)
#define TCG_ENV_SIZE_(f)    sizeof_field(CPUArchState, f)

#define tcg_gen_ld_env_tl(ret, f)                               \
     do {                                                        \
         QEMU_BUILD_BUG_ON(TCG_ENV_SIZE_(f) !=                   \
                           sizeof(target_ulong));                \
         tcg_gen_ld_tl(ret, tcg_env, TCG_ENV_OFS_(f));           \
     } while (0)

#define tcg_gen_ld_env_i64(ret, f)                              \
     do {                                                        \
         QEMU_BUILD_BUG_ON(TCG_ENV_SIZE_(f) != 8);               \
         tcg_gen_ld_i64(ret, tcg_env, TCG_ENV_OFS_(f));          \
     } while (0)

or even using _Generic:

#define TCG_ENV_FIELD_(f)   (((CPUArchState *)0)->f)
#define TCG_ENV_OFS_(f)     offsetof(CPUArchState, f)

#define tcg_gen_st_env_i32(arg, f)                              \
     _Generic(TCG_ENV_FIELD_(f),                                 \
              bool:     tcg_gen_st8_i32,                         \
              uint8_t:  tcg_gen_st8_i32,                         \
              int8_t:   tcg_gen_st8_i32,                         \
              uint16_t: tcg_gen_st16_i32,                        \
              int16_t:  tcg_gen_st16_i32,                        \


Hmm tricky (dangerous?), this also change our current uses of
target-long accesses to smaller fields. This is how I'd expect
the uses but not always what we have now. For example, commit
ebc9401a406 ("target/hppa: Split PSW X and B into their own
field") reduced CPUHPPAState::psw from uint64_t to uint32_t,
but both accessors -- trans_rsm() and trans_ssm() -- keep
accessing with tcg_gen_ld_i64() instead of tcg_gen_ld32u_i64().
Likely buggy wrt endianness.



              uint32_t: tcg_gen_st_i32,                          \
              int32_t:  tcg_gen_st_i32)                          \
         (arg, tcg_env, TCG_ENV_OFS_(f))

#define tcg_gen_st_env_i64(arg, f)                              \
     _Generic(TCG_ENV_FIELD_(f),                                 \
              uint64_t: tcg_gen_st_i64,                          \
              int64_t:  tcg_gen_st_i64)                          \
         (arg, tcg_env, TCG_ENV_OFS_(f))

#if TARGET_LONG_BITS == 32
# define tcg_gen_ld_env_tl  tcg_gen_ld_env_i32
# define tcg_gen_st_env_tl  tcg_gen_st_env_i32
#else
# define tcg_gen_ld_env_tl  tcg_gen_ld_env_i64
# define tcg_gen_st_env_tl  tcg_gen_st_env_i64
#endif


I added that to "tcg/tcg-op-env.h".



Then replacing (a cheap replacement that can be done with coccinelle
spatch):


-- >8 --
@@
expression R;
identifier F;
symbol tcg_env;
type T;
@@
(
- tcg_gen_ld_tl(R, tcg_env, offsetof(T, F))
+ tcg_gen_ld_env_tl(R, F)
|
- tcg_gen_st_tl(R, tcg_env, offsetof(T, F))
+ tcg_gen_st_env_tl(R, F)
)

@@
expression R;
identifier F;
symbol tcg_env;
identifier T;
@@
(
- tcg_gen_ld_tl(R, tcg_env, offsetof(T, F))
+ tcg_gen_ld_env_tl(R, F)
|
- tcg_gen_st_tl(R, tcg_env, offsetof(T, F))
+ tcg_gen_st_env_tl(R, F)
)
---

Converting i386:

-- >8 --
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index dd417cb5341..abdf9b61ad1 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -25,2 +25,3 @@
  #include "tcg/tcg-op.h"
+#include "tcg/tcg-op-env.h"
  #include "tcg/tcg-op-gvec.h"
@@ -706,5 +707,5 @@ static void gen_set_eflags(DisasContext *s, target_ulong mask) 

-    tcg_gen_ld_tl(t, tcg_env, offsetof(CPUX86State, eflags));
+    tcg_gen_ld_env_tl(t, eflags);
      tcg_gen_ori_tl(t, t, mask);
-    tcg_gen_st_tl(t, tcg_env, offsetof(CPUX86State, eflags));
+    tcg_gen_st_env_tl(t, eflags);
  }
@@ -715,5 +716,5 @@ static void gen_reset_eflags(DisasContext *s, target_ulong mask) 

-    tcg_gen_ld_tl(t, tcg_env, offsetof(CPUX86State, eflags));
+    tcg_gen_ld_env_tl(t, eflags);
      tcg_gen_andi_tl(t, t, ~mask);
-    tcg_gen_st_tl(t, tcg_env, offsetof(CPUX86State, eflags));
+    tcg_gen_st_env_tl(t, eflags);
  }
@@ -2447,4 +2448,3 @@ static void gen_x87(DisasContext *s, X86DecodedInsn *decode) 
                               offsetof(CPUX86State, fpds));
-            tcg_gen_st_tl(last_addr, tcg_env,
-                          offsetof(CPUX86State, fpdp));
+            tcg_gen_st_env_tl(last_addr, fpdp);
          }
@@ -2756,4 +2756,3 @@ static void gen_x87(DisasContext *s, X86DecodedInsn *decode) 
                           offsetof(CPUX86State, fpcs));
-        tcg_gen_st_tl(eip_cur_tl(s),
-                      tcg_env, offsetof(CPUX86State, fpip));
+        tcg_gen_st_env_tl(eip_cur_tl(s), fpip);
      }
@@ -3200,6 +3199,4 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn *decode) 
                      tcg_gen_mov_tl(s->T0, cpu_seg_base[R_GS]);
-                    tcg_gen_ld_tl(cpu_seg_base[R_GS], tcg_env,
-                                  offsetof(CPUX86State, kernelgsbase));
-                    tcg_gen_st_tl(s->T0, tcg_env,
-                                  offsetof(CPUX86State, kernelgsbase));
+                    tcg_gen_ld_env_tl(cpu_seg_base[R_GS], kernelgsbase);
+                    tcg_gen_st_env_tl(s->T0, kernelgsbase);
                  }
---

triggers:
../target/i386/tcg/translate.c:2449:13: error: controlling expression type 'uint64_t' (aka 'unsigned long long') not compatible with any generic association type 
  2449 |             tcg_gen_st_env_tl(last_addr, fpdp);
       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:65:29: note: expanded from macro 'tcg_gen_st_env_tl' 
    65 | # define tcg_gen_st_env_tl  tcg_gen_st_env_i32
       |                             ^
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:47:14: note: expanded from macro 'tcg_gen_st_env_i32' 
    47 |     _Generic(TCG_ENV_FIELD_(f),                                 \
       |              ^~~~~~~~~~~~~~~~~
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:16:29: note: expanded from macro 'TCG_ENV_FIELD_' 
    16 | #define TCG_ENV_FIELD_(f)   (((CPUArchState *)0)->f)
       |                             ^~~~~~~~~~~~~~~~~~~~~~~~
../target/i386/tcg/translate.c:2757:9: error: controlling expression type 'uint64_t' (aka 'unsigned long long') not compatible with any generic association type 
  2757 |         tcg_gen_st_env_tl(eip_cur_tl(s), fpip);
       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:65:29: note: expanded from macro 'tcg_gen_st_env_tl' 
    65 | # define tcg_gen_st_env_tl  tcg_gen_st_env_i32
       |                             ^
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:47:14: note: expanded from macro 'tcg_gen_st_env_i32' 
    47 |     _Generic(TCG_ENV_FIELD_(f),                                 \
       |              ^~~~~~~~~~~~~~~~~
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:16:29: note: expanded from macro 'TCG_ENV_FIELD_' 
    16 | #define TCG_ENV_FIELD_(f)   (((CPUArchState *)0)->f)
       |                             ^~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

Both CPUX86State::fpip/fpdp fields are uint64_t, so the same class of
bug on big-endian hosts.

FAILED: [code=1] libqemu-i386-softmmu.a.p/target_i386_tcg_translate.c.o

Reply via email to