Let branching happen and try to generate a new translation block with
an odd address. Generate a specification exception in
cpu_get_tb_cpu_state().

Reported-by: Harold Grovesteen <h.grovsteen@tx.rr.com>
Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/cpu.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b2..0a76e96e078 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -29,6 +29,7 @@
 #include "cpu_models.h"
 #include "exec/cpu-defs.h"
 #include "qemu/cpu-float.h"
+#include "tcg/tcg_s390x.h"
 
 #define ELF_MACHINE_UNAME "S390X"
 
@@ -381,6 +382,14 @@ 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)
 {
+    if (env->psw.addr & 1) {
+        /*
+         * Instructions must be at even addresses.
+         * This needs to be checked before address translation.
+         */
+        env->int_pgm_ilen = 2; /* see s390_cpu_tlb_fill() */
+        tcg_s390_program_interrupt(env, PGM_SPECIFICATION, 0);
+    }
     *pc = env->psw.addr;
     *cs_base = env->ex_value;
     *flags = (env->psw.mask >> FLAG_MASK_PSW_SHIFT) & FLAG_MASK_PSW;
-- 
2.39.2

On 3/10/23 09:42, Ilya Leoshkevich wrote:
> @@ -381,6 +382,14 @@ 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)
>   {
> +    if (env->psw.addr & 1) {
> +        /*
> +         * Instructions must be at even addresses.
> +         * This needs to be checked before address translation.
> +         */
> +        env->int_pgm_ilen = 2; /* see s390_cpu_tlb_fill() */
> +        tcg_s390_program_interrupt(env, PGM_SPECIFICATION, 0);
> +    }

This is incorrect placement.  You can't raise an exception from all of the places from 
which this is called.

You need to do this at the start of s390x_tr_translate_insn.
Compare aarch64_tr_translate_insn and the test for (pc & 3).


r~

On Fri, 2023-03-10 at 11:24 -0800, Richard Henderson wrote:
> On 3/10/23 09:42, Ilya Leoshkevich wrote:
> > @@ -381,6 +382,14 @@ 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)
> >   {
> > +    if (env->psw.addr & 1) {
> > +        /*
> > +         * Instructions must be at even addresses.
> > +         * This needs to be checked before address translation.
> > +         */
> > +        env->int_pgm_ilen = 2; /* see s390_cpu_tlb_fill() */
> > +        tcg_s390_program_interrupt(env, PGM_SPECIFICATION, 0);
> > +    }
> 
> This is incorrect placement.  You can't raise an exception from all
> of the places from 
> which this is called.
> 
> You need to do this at the start of s390x_tr_translate_insn.
> Compare aarch64_tr_translate_insn and the test for (pc & 3).
> 
> 
> r~

The problem is that it's too late - for non-mapped memory we would get
a translation exception instead of a specification exception.

I see the following call sites:

- HELPER(lookup_tb_ptr) - for helpers the exceptions should work;
- cpu_exec_loop(), cpu_exec_step_atomic - these are wrapped in setjmp,
  so it should be ok too?
- tb_check_watchpoint() - is this the problematic one?

Am I missing something?

On 3/10/23 11:34, Ilya Leoshkevich wrote:
> On Fri, 2023-03-10 at 11:24 -0800, Richard Henderson wrote:
>> On 3/10/23 09:42, Ilya Leoshkevich wrote:
>>> @@ -381,6 +382,14 @@ 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)
>>>    {
>>> +    if (env->psw.addr & 1) {
>>> +        /*
>>> +         * Instructions must be at even addresses.
>>> +         * This needs to be checked before address translation.
>>> +         */
>>> +        env->int_pgm_ilen = 2; /* see s390_cpu_tlb_fill() */
>>> +        tcg_s390_program_interrupt(env, PGM_SPECIFICATION, 0);
>>> +    }
>>
>> This is incorrect placement.  You can't raise an exception from all
>> of the places from
>> which this is called.
>>
>> You need to do this at the start of s390x_tr_translate_insn.
>> Compare aarch64_tr_translate_insn and the test for (pc & 3).
>>
>>
>> r~
> 
> The problem is that it's too late - for non-mapped memory we would get
> a translation exception instead of a specification exception.

Ah.  I wonder if I've got the placement right for arm.


> I see the following call sites:
> 
> - HELPER(lookup_tb_ptr) - for helpers the exceptions should work;
> - cpu_exec_loop(), cpu_exec_step_atomic - these are wrapped in setjmp,
>    so it should be ok too?
> - tb_check_watchpoint() - is this the problematic one?
> 
> Am I missing something?

Apparently not.  I thought the ones except for lookup_tb_ptr would be outside the setjmp, 
but I was wrong.