Re: [Qemu-devel] [PATCH 010/126] target-s390: Reorg exception handling

[Alexander Graf]

On 19.09.2012, at 13:29, Peter Maydell wrote:

> On 19 September 2012 01:14, Richard Henderson <address@hidden> wrote:
>> On 09/18/2012 01:18 PM, Alexander Graf wrote:
>>>> -    /* remember what pgm exeption this was */
>>>> +    /* Remember what pgm exeption this was.  */
> 
> ...if you're changing this comment then at least fix the typo
> ("exception")...
> 
>>>>      tmp = tcg_const_i32(code);
>>>>      tcg_gen_st_i32(tmp, cpu_env, offsetof(CPUS390XState, int_pgm_code));
>>>>      tcg_temp_free_i32(tmp);
>>>>  -    tmp = tcg_const_i32(ilc);
>>>> +    tmp = tcg_const_i32(s->next_pc - s->pc);
>>> 
>>> Mind to explain this one?
>> 
>> ILC = the size of the insn.  Rather than passing ILC around into
>> gen_program_exception, get it back from the s->next_pc value that
>> we stored into DisasContext.
> 
> ...but isn't (s-next_pc - s->pc) ilc*2, not ilc? Compare this hunk
> from elsewhere in the patch:
> 
> -        tmp32_2 = tcg_const_i32(ilc * 2);
> -        tmp32_3 = tcg_const_i32(EXCP_SVC);
> +        tmp32_2 = tcg_const_i32(s->next_pc - s->pc);

Yes, it is, and hence it's correct. The exception field encodes the real 
instruction length, not the ILC bits in the opcode. Maybe the naming is 
unfortunate...


Alex