[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 2/6] target/arm/translate.c: make DISAS_UPDAT
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PATCH v1 2/6] target/arm/translate.c: make DISAS_UPDATE match declared semantics |
Date: |
Mon, 10 Jul 2017 19:35:21 +0100 |
User-agent: |
mu4e 0.9.19; emacs 25.2.50.3 |
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
[Qemu-devel] [PATCH v1 6/6] target/arm: ensure eret exits the run-loop via DISAS_UPDATE, Alex Bennée, 2017/07/10
[Qemu-devel] [PATCH v1 3/6] target/arm/translate-a64: make DISAS_UPDATE match declared semantics, Alex Bennée, 2017/07/10