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

[Richard Henderson <richard.henderson@linaro.org>]

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.