On 23/10/26 10:07, Philippe Mathieu-Daudé wrote:
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.
Noticed because the coccinelle conversion now triggers:
../target/hppa/translate.c:2367:5: error: controlling expression type
'uint32_t' (aka 'unsigned int') not compatible with any generic
association type
2367 | tcg_gen_ld_env_i64(tmp, psw);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:30:14: note:
expanded from macro 'tcg_gen_ld_env_i64'
30 | _Generic(TCG_ENV_FIELD_(f), \
| ^~~~~~~~~~~~~~~~~
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:15:29: note:
expanded from macro 'TCG_ENV_FIELD_'
15 | #define TCG_ENV_FIELD_(f) (((struct CPUArchState *)0)->f)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../target/hppa/translate.c:2387:5: error: controlling expression type
'uint32_t' (aka 'unsigned int') not compatible with any generic
association type
2387 | tcg_gen_ld_env_i64(tmp, psw);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:30:14: note:
expanded from macro 'tcg_gen_ld_env_i64'
30 | _Generic(TCG_ENV_FIELD_(f), \
| ^~~~~~~~~~~~~~~~~
/Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:15:29: note:
expanded from macro 'TCG_ENV_FIELD_'
15 | #define TCG_ENV_FIELD_(f) (((struct CPUArchState *)0)->f)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
