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, \
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
