On Sat, Nov 05, 2022 at 09:27:07AM +1100, Richard Henderson wrote: > On 11/4/22 00:42, Ilya Leoshkevich wrote: > > On Wed, Oct 05, 2022 at 08:44:07PM -0700, Richard Henderson wrote: > > > Masking after the fact in s390x_tr_init_disas_context > > > provides incorrect information to tb_lookup. > > > > > > Signed-off-by: Richard Henderson <richard.hender...@linaro.org> > > > --- > > > target/s390x/cpu.h | 13 +++++++------ > > > target/s390x/tcg/translate.c | 6 ------ > > > 2 files changed, 7 insertions(+), 12 deletions(-) > > > > How can we end up in a situation where this matters? E.g. if we are in > > 64-bit mode and execute > > > > 0xa12345678: sam31 > > > > we will get a specification exception, and cpu_get_tb_cpu_state() will > > not run. And for valid 31-bit addresses masking should be a no-op. > > Ah, true. I was mislead by the presence of the code in > s390x_tr_init_disas_context. Perhaps a tcg_debug_assert or just a comment?
An assert sounds good to me. I tried the following diff with the attached test and it worked: --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -390,7 +390,12 @@ static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc, } *pflags = flags; *cs_base = env->ex_value; - *pc = (flags & FLAG_MASK_64 ? env->psw.addr : env->psw.addr & 0x7fffffff); + if (!(flags & FLAG_MASK_32)) { + g_assert(env->psw.addr <= 0xffffff); + } else if (!(flags & FLAG_MASK_64)) { + g_assert(env->psw.addr <= 0x7fffffff); + } + *pc = env->psw.addr; } /* PER bits from control register 9 */ diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c index 24dc57a8816..a50453dd0d4 100644 --- a/target/s390x/tcg/translate.c +++ b/target/s390x/tcg/translate.c @@ -6464,6 +6464,12 @@ static void s390x_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) { DisasContext *dc = container_of(dcbase, DisasContext, base); + if (!(dc->base.tb->flags & FLAG_MASK_32)) { + tcg_debug_assert(dc->base.pc_first <= 0xffffff); + } else if (!(dc->base.tb->flags & FLAG_MASK_64)) { + tcg_debug_assert(dc->base.pc_first <= 0x7fffffff); + } + dc->pc_save = dc->base.pc_first; dc->cc_op = CC_OP_DYNAMIC; dc->ex_value = dc->base.tb->cs_base;