diff mbox series

[v1,2/6] target/arm/translate.c: make DISAS_UPDATE match declared semantics

Message ID 20170710154749.13624-3-alex.bennee@linaro.org
State New
Headers show
Series DISAS_UPDATE fixes for eret | expand

Commit Message

Alex Bennée July 10, 2017, 3:47 p.m. UTC 
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 <alex.bennee@linaro.org>

---
 target/arm/translate.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
2.13.0

Comments

Peter Maydell July 10, 2017, 4:13 p.m. UTC | #1 
On 10 July 2017 at 16:47, Alex Bennée <alex.bennee@linaro.org> 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 <alex.bennee@linaro.org>

> ---

>  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.

Also, now the "synchronize the CPU state which we only write at the
end of the TB" code is split up -- we write the PC in the places
that set s->is_jmp, but we write the condexec bits in the end
of TB common code.

thanks
-- PMM
Richard Henderson July 10, 2017, 4:31 p.m. UTC | #2 
On 07/10/2017 06:13 AM, Peter Maydell wrote:
> On 10 July 2017 at 16:47, Alex Bennée <alex.bennee@linaro.org> 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 <alex.bennee@linaro.org>

>> ---

>>   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.


r~
Alex Bennée July 10, 2017, 6:35 p.m. UTC | #3 
Richard Henderson <rth@twiddle.net> writes:

> On 07/10/2017 06:13 AM, Peter Maydell wrote:

>> On 10 July 2017 at 16:47, Alex Bennée <alex.bennee@linaro.org> 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 <alex.bennee@linaro.org>

>>> ---

>>>   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
Richard Henderson July 10, 2017, 6:55 p.m. UTC | #4 
On 07/10/2017 08:35 AM, Alex Bennée wrote:
> 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.


I hope we can do that within the context of (or after) LLuis' changes.
Doing it beforehand is just going to confuse the issue.

> 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?


Yes, that seems like the best short-term solution.


r~
diff mbox series

Patch

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);