Re: [Qemu-arm] [PATCH v1 2/6] target/arm/translate.c: make DISAS_UPDATE match declared semantics

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> On 07/10/2017 06:13 AM, Peter Maydell wrote:
>> On 10 July 2017 at 16:47, Alex Bennée <address@hidden> wrote:
>>> DISAS_UPDATE should be used when the wider CPU state other than just
>>> the PC has been updated and we should therefor exit the TCG runtime
>>> and return to the main execution loop rather assuming DISAS_JUMP would
>>> do that.
>>>
>>> As some DISAS_UPDATE users may update the PC dynamically via a helper
>>> we also push the updating to the PC to hhe call sites which set
>>> ->is_jmp to DISAS_UPDATE.
>>>
>>> Signed-off-by: Alex Bennée <address@hidden>
>>> ---
>>>   target/arm/translate.c | 11 +++++++----
>>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>>> index 0862f9e4aa..f9c4aee1b6 100644
>>> --- a/target/arm/translate.c
>>> +++ b/target/arm/translate.c
>>> @@ -4430,6 +4430,7 @@ static void gen_msr_banked(DisasContext *s, int r, 
>>> int sysm, int rn)
>>>       tcg_temp_free_i32(tcg_tgtmode);
>>>       tcg_temp_free_i32(tcg_regno);
>>>       tcg_temp_free_i32(tcg_reg);
>>> +    gen_set_pc_im(s, s->pc);
>>>       s->is_jmp = DISAS_UPDATE;
>>>   }
>>>
>>> @@ -4452,6 +4453,7 @@ static void gen_mrs_banked(DisasContext *s, int r, 
>>> int sysm, int rn)
>>>       tcg_temp_free_i32(tcg_tgtmode);
>>>       tcg_temp_free_i32(tcg_regno);
>>>       store_reg(s, rn, tcg_reg);
>>> +    gen_set_pc_im(s, s->pc);
>>>       s->is_jmp = DISAS_UPDATE;
>>>   }
>>>
>>> @@ -8058,6 +8060,7 @@ static void gen_srs(DisasContext *s,
>>>           tcg_temp_free_i32(tmp);
>>>       }
>>>       tcg_temp_free_i32(addr);
>>> +    gen_set_pc_im(s, s->pc);
>>>       s->is_jmp = DISAS_UPDATE;
>>>   }
>>>
>>> @@ -8146,6 +8149,7 @@ static void disas_arm_insn(DisasContext *s, unsigned 
>>> int insn)
>>>               /* setend */
>>>               if (((insn >> 9) & 1) != !!(s->be_data == MO_BE)) {
>>>                   gen_helper_setend(cpu_env);
>>> +                gen_set_pc_im(s, s->pc);
>>>                   s->is_jmp = DISAS_UPDATE;
>>>               }
>>>               return;
>>> @@ -11619,6 +11623,7 @@ static void disas_thumb_insn(CPUARMState *env, 
>>> DisasContext *s)
>>>                   ARCH(6);
>>>                   if (((insn >> 3) & 1) != !!(s->be_data == MO_BE)) {
>>>                       gen_helper_setend(cpu_env);
>>> +                    gen_set_pc_im(s, s->pc);
>>>                       s->is_jmp = DISAS_UPDATE;
>>>                   }
>>>                   break;
>>> @@ -12076,7 +12081,6 @@ void gen_intermediate_code(CPUARMState *env, 
>>> TranslationBlock *tb)
>>>               break;
>>>           case DISAS_NEXT:
>>>           case DISAS_UPDATE:
>>> -            gen_set_pc_im(dc, dc->pc);
>>>               /* fall through */
>>>           default:
>>>               /* FIXME: Single stepping a WFI insn will not halt the CPU. */
>>> @@ -12095,12 +12099,11 @@ void gen_intermediate_code(CPUARMState *env, 
>>> TranslationBlock *tb)
>>>           case DISAS_NEXT:
>>>               gen_goto_tb(dc, 1, dc->pc);
>>>               break;
>>> -        case DISAS_UPDATE:
>>> -            gen_set_pc_im(dc, dc->pc);
>>> -            /* fall through */
>>>           case DISAS_JUMP:
>>>               gen_goto_ptr();
>>>               break;
>>> +        case DISAS_UPDATE:
>>> +            /* fall through */
>>>           default:
>>>               /* indicate that the hash table must be used to find the next 
>>> TB */
>>>               tcg_gen_exit_tb(0);
>>
>> I'm not a great fan of this, because we've taken five cases that
>> all want the same behaviour for ending the TB, and we've made
>> them handle part of it themselves rather than just letting
>> the end-of-TB code do it for them.
>
> I agree.
>
> Indeed, the fact that you've found 5 cases that are all the same
> suggests there *should* be common handling for them, and you're
> undoing that.
>
> Enumeratiors are not expensive.  If you found that none of the
> existing cases works for some case, then add another enumerator.

Well this was more in the guise of having well defined semantics across
all the translators. I agree just keeping DISAS_EXIT is cleaner w.r.t
the ARM code.

So without messing with where we do gen_set_pc_im(dc, dc->pc); shall I
just cut this down to not falling through to DISAS_JUMP?

--
Alex Bennée