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
Then replacing (a cheap replacement that can be done with coccinelle
spatch):
@@ -704,14 +757,14 @@ static void mark_fs_dirty(DisasContext *ctx)
ctx->mstatus_fs = EXT_STATUS_DIRTY;
tmp = tcg_temp_new();
- tcg_gen_ld_tl(tmp, tcg_env, offsetof(CPURISCVState, mstatus));
+ tcg_gen_ld_env_tl(tmp, mstatus);
tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS);
- tcg_gen_st_tl(tmp, tcg_env, offsetof(CPURISCVState, mstatus));
+ tcg_gen_st_env_tl(tmp, mstatus);
if (ctx->virt_enabled) {
- tcg_gen_ld_tl(tmp, tcg_env, offsetof(CPURISCVState,
mstatus_hs));
+ tcg_gen_ld_env_tl(tmp, mstatus_hs);
tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS);
- tcg_gen_st_tl(tmp, tcg_env, offsetof(CPURISCVState,
mstatus_hs));
+ tcg_gen_st_env_tl(tmp, mstatus_hs);
}
}
triggers:
FAILED: [code=1] libqemu-riscv32-softmmu.a.p/target_riscv_tcg_translate.c.o
../target/riscv/tcg/translate.c:760:9: error: controlling expression
type 'uint64_t' (aka 'unsigned long long') not compatible with any
generic association type
760 | tcg_gen_ld_env_tl(tmp, mstatus);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:86:29: note: expanded from macro
'tcg_gen_ld_env_tl'
86 | # define tcg_gen_ld_env_tl tcg_gen_ld_env_i32
| ^
../target/riscv/tcg/translate.c:45:14: note: expanded from macro
'tcg_gen_ld_env_i32'
45 | _Generic(TCG_ENV_FIELD_(f), \
| ^~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:41:29: note: expanded from macro
'TCG_ENV_FIELD_'
41 | #define TCG_ENV_FIELD_(f) (((CPURISCVState *)0)->f)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:762:9: error: controlling expression
type 'uint64_t' (aka 'unsigned long long') not compatible with any
generic association type
762 | tcg_gen_st_env_tl(tmp, mstatus);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:87:29: note: expanded from macro
'tcg_gen_st_env_tl'
87 | # define tcg_gen_st_env_tl tcg_gen_st_env_i32
| ^
../target/riscv/tcg/translate.c:69:14: note: expanded from macro
'tcg_gen_st_env_i32'
69 | _Generic(TCG_ENV_FIELD_(f), \
| ^~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:41:29: note: expanded from macro
'TCG_ENV_FIELD_'
41 | #define TCG_ENV_FIELD_(f) (((CPURISCVState *)0)->f)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:765:13: error: controlling expression
type 'uint64_t' (aka 'unsigned long long') not compatible with any
generic association type
765 | tcg_gen_ld_env_tl(tmp, mstatus_hs);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:86:29: note: expanded from macro
'tcg_gen_ld_env_tl'
86 | # define tcg_gen_ld_env_tl tcg_gen_ld_env_i32
| ^
../target/riscv/tcg/translate.c:45:14: note: expanded from macro
'tcg_gen_ld_env_i32'
45 | _Generic(TCG_ENV_FIELD_(f), \
| ^~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:41:29: note: expanded from macro
'TCG_ENV_FIELD_'
41 | #define TCG_ENV_FIELD_(f) (((CPURISCVState *)0)->f)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:767:13: error: controlling expression
type 'uint64_t' (aka 'unsigned long long') not compatible with any
generic association type
767 | tcg_gen_st_env_tl(tmp, mstatus_hs);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:87:29: note: expanded from macro
'tcg_gen_st_env_tl'
87 | # define tcg_gen_st_env_tl tcg_gen_st_env_i32
| ^
../target/riscv/tcg/translate.c:69:14: note: expanded from macro
'tcg_gen_st_env_i32'
69 | _Generic(TCG_ENV_FIELD_(f), \
| ^~~~~~~~~~~~~~~~~
../target/riscv/tcg/translate.c:41:29: note: expanded from macro
'TCG_ENV_FIELD_'
41 | #define TCG_ENV_FIELD_(f) (((CPURISCVState *)0)->f)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
4 errors generated.
