On 23/10/26, Philippe Mathieu-Daudé wrote: > On 23/10/26 10:10, Philippe Mathieu-Daudé wrote: > > 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. > > > > > > And using these macros on tcg_global_mem_*, another issue on Hexagon > (slot_cancelled is u8 but registered to be accessed as u32): > > FAILED: [code=1] libqemu-hexagon-softmmu.a.p/target_hexagon_translate.c.o > ../target/hexagon/translate.c:1472:26: error: controlling expression type > 'uint8_t' (aka 'unsigned char') not compatible with any generic association > type > 1472 | hex_slot_cancelled = tcg_global_mem_new_env_tl(slot_cancelled, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1473 | "slot_cancelled"); > | ~~~~~~~~~~~~~~~~~ > /Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:160:37: note: > expanded from macro 'tcg_global_mem_new_env_tl' > 160 | # define tcg_global_mem_new_env_tl tcg_global_mem_new_env_i32 > | ^ > /Users/philmd/worktree/qemu/excp/include/tcg/tcg-op-env.h:52:14: note: > expanded from macro 'tcg_global_mem_new_env_i32' > 52 | _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) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1 error generated. > >
Ah this is nice, let's drop my patch and go for this instead IMO! :)
