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(-)

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;
-- 
2.34.1

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;

Add a small test to prevent regressions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.softmmu-target |  1 +
 tests/tcg/s390x/sam.S                   | 67 +++++++++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100644 tests/tcg/s390x/sam.S

diff --git a/tests/tcg/s390x/Makefile.softmmu-target b/tests/tcg/s390x/Makefile.softmmu-target
index a34fa68473e..738b04530fc 100644
--- a/tests/tcg/s390x/Makefile.softmmu-target
+++ b/tests/tcg/s390x/Makefile.softmmu-target
@@ -7,3 +7,4 @@ QEMU_OPTS=-action panic=exit-failure -kernel
 		-Wl,--build-id=none $< -o $@
 
 TESTS += unaligned-lowcore
+TESTS += sam
diff --git a/tests/tcg/s390x/sam.S b/tests/tcg/s390x/sam.S
new file mode 100644
index 00000000000..4cab2dd2007
--- /dev/null
+++ b/tests/tcg/s390x/sam.S
@@ -0,0 +1,67 @@
+/* DAT on, home-space mode, 64-bit mode */
+#define DAT_PSWM 0x400c00180000000
+#define VIRTUAL_BASE 0x123456789abcd000
+
+    .org 0x8e
+program_interruption_code:
+    .org 0x150
+program_old_psw:
+    .org 0x1d0                         /* program new PSW */
+    .quad 0,pgm_handler
+    .org 0x200                         /* lowcore padding */
+
+    .globl _start
+_start:
+    lctlg %c13,%c13,hasce
+    lpswe dat_psw
+start_dat:
+    sam24
+sam24_suppressed:
+    /* sam24 should fail */
+fail:
+    basr %r12,%r0
+    lpswe failure_psw-.(%r12)
+pgm_handler:
+    chhsi program_interruption_code,6  /* specification exception? */
+    jne fail
+    clc suppressed_psw(16),program_old_psw  /* correct location? */
+    jne fail
+    lpswe success_psw
+
+    .align 8
+dat_psw:
+    .quad DAT_PSWM,VIRTUAL_BASE+start_dat
+suppressed_psw:
+    .quad DAT_PSWM,VIRTUAL_BASE+sam24_suppressed
+success_psw:
+    .quad 0x2000000000000,0xfff        /* see is_special_wait_psw() */
+failure_psw:
+    .quad 0x2000000000000,0            /* disabled wait */
+hasce:
+    /* DT = 0b11 (region-first-table), TL = 3 (2k entries) */
+    .quad region_first_table + (3 << 2) + 3
+    .align 0x1000
+region_first_table:
+    .org region_first_table + ((VIRTUAL_BASE >> 53) & 0x7ff) * 8
+    /* TT = 0b11 (region-first-table), TL = 3 (2k entries) */
+    .quad region_second_table + (3 << 2) + 3
+    .org region_first_table + 0x800 * 8
+region_second_table:
+    .org region_second_table + ((VIRTUAL_BASE >> 42) & 0x7ff) * 8
+    /* TT = 0b10 (region-second-table), TL = 3 (2k entries) */
+    .quad region_third_table + (2 << 2) + 3
+    .org region_second_table + 0x800 * 8
+region_third_table:
+    .org region_third_table + ((VIRTUAL_BASE >> 31) & 0x7ff) * 8
+    /* TT = 0b01 (region-third-table), TL = 3 (2k entries) */
+    .quad segment_table + (1 << 2) + 3
+    .org region_third_table + 0x800 * 8
+segment_table:
+    .org segment_table + ((VIRTUAL_BASE >> 20) & 0x7ff) * 8
+    /* TT = 0b00 (segment-table) */
+    .quad page_table
+    .org segment_table + 0x800 * 8
+page_table:
+    .org page_table + ((VIRTUAL_BASE >> 12) & 0xff) * 8
+    .quad 0
+    .org page_table + 0x100 * 8
-- 
2.38.1