| Message ID | 20221006034421.1179141-13-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/s390x: pc-relative translation blocks | expand |
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.henderson@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.
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.henderson@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? r~
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.henderson@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;
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 7d6d01325b..b5c99bc694 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -379,17 +379,18 @@ static inline int cpu_mmu_index(CPUS390XState *env, bool ifetch) } static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc, - target_ulong *cs_base, uint32_t *flags) + target_ulong *cs_base, uint32_t *pflags) { - *pc = env->psw.addr; - *cs_base = env->ex_value; - *flags = (env->psw.mask >> FLAG_MASK_PSW_SHIFT) & FLAG_MASK_PSW; + int flags = (env->psw.mask >> FLAG_MASK_PSW_SHIFT) & FLAG_MASK_PSW; if (env->cregs[0] & CR0_AFP) { - *flags |= FLAG_MASK_AFP; + flags |= FLAG_MASK_AFP; } if (env->cregs[0] & CR0_VECTOR) { - *flags |= FLAG_MASK_VECTOR; + flags |= FLAG_MASK_VECTOR; } + *pflags = flags; + *cs_base = env->ex_value; + *pc = (flags & FLAG_MASK_64 ? env->psw.addr : env->psw.addr & 0x7fffffff); } /* PER bits from control register 9 */ diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c index 67c86996e9..9ee8146b87 100644 --- a/target/s390x/tcg/translate.c +++ b/target/s390x/tcg/translate.c @@ -6485,12 +6485,6 @@ static void s390x_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) { DisasContext *dc = container_of(dcbase, DisasContext, base); - /* 31-bit mode */ - if (!(dc->base.tb->flags & FLAG_MASK_64)) { - dc->base.pc_first &= 0x7fffffff; - dc->base.pc_next = dc->base.pc_first; - } - dc->cc_op = CC_OP_DYNAMIC; dc->ex_value = dc->base.tb->cs_base; dc->exit_to_mainloop = (dc->base.tb->flags & FLAG_MASK_PER) || dc->ex_value;
Masking after the fact in s390x_tr_init_disas_context provides incorrect information to tb_lookup. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/s390x/cpu.h | 13 +++++++------ target/s390x/tcg/translate.c | 6 ------ 2 files changed, 7 insertions(+), 12 deletions(-)