Re: [PATCH 01/12] target/s390x: Handle branching to odd addresses

[Richard Henderson]

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.