Re: [PATCH 12/26] target/s390x: Move masking of psw.addr to cpu_get_tb_cpu_state

[Ilya Leoshkevich]

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;