[Qemu-devel] [PATCH v1 0/6] DISAS_UPDATE fixes for eret

[Qemu-devel] [PATCH v1 0/6] DISAS_UPDATE fixes for eret

[Alex Bennée]

Hi,

This is a follow-up to the eret patch I posted on Friday:

  https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01831.html

It fixes the same bug but does it in a slightly more holistic way. We
start by expanding the commentary on the common DISAS_n exit flags in
exec-all.h. I then update ARMs usage of DISAS_UPDATE to:

  - explicitly exit the run-loop (and not fall-through to DISAS_JUMP)
  - make setting the PC optional (as helpers may set it)

I then get rid of the architecture specific hack DISAS_EXIT and use
DISAS_UPDATE for all the eret paths.

Please review.

Alex Bennée (6):
  include/exec/exec-all: document common exit conditions
  target/arm/translate.c: make DISAS_UPDATE match declared semantics
  target/arm/translate-a64: make DISAS_UPDATE match declared semantics
  target/arm/translate-a64: get rid of DISAS_EXIT
  target/arm: use DISAS_JUMP for ISB handling
  target/arm: ensure eret exits the run-loop via DISAS_UPDATE

 include/exec/exec-all.h    | 29 ++++++++++++++++++++++++++---
 target/arm/translate-a64.c | 21 +++++++++++----------
 target/arm/translate.c     | 36 ++++++++++++++++++++++++++----------
 target/arm/translate.h     |  4 ----
 4 files changed, 63 insertions(+), 27 deletions(-)

-- 
2.13.0

[Qemu-devel] [PATCH v1 1/6] include/exec/exec-all: document common exit conditions

[Alex Bennée]

As a precursor to later patches attempt to come up with a more
concrete wording for what each of the common exit cases would be.

CC: Emilio G. Cota <cota@braap.org>
CC: Richard Henderson <rth@twiddle.net>
CC: Lluís Vilanova <vilanova@ac.upc.edu>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/exec-all.h | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 8096d64a1d..a23894f687 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -35,11 +35,34 @@ typedef abi_ulong tb_page_addr_t;
 typedef ram_addr_t tb_page_addr_t;
 #endif
 
-/* is_jmp field values */
+/* DisasContext is_jmp field values
+ *
+ * is_jmp starts as DISAS_NEXT. The translator will keep processing
+ * instructions until an exit condition is reached. If we reach the
+ * exit condition and is_jmp is still DISAS_NEXT (because of some
+ * other condition) we simply "jump" to the next address.
+ * The remaining exit cases are:
+ *
+ *   DISAS_JUMP    - Only the PC was modified dynamically (e.g computed)
+ *   DISAS_TB_JUMP - Only the PC was modified statically (e.g. branch)
+ *
+ * In these cases as long as the PC is updated we can chain to the
+ * next TB either by exiting the loop or looking up the next TB via
+ * the loookup helper.
+ *
+ *   DISAS_UPDATE  - CPU State was modified dynamically
+ *
+ * This covers any other CPU state which necessities us exiting the
+ * TCG code to the main run-loop. Typically this includes anything
+ * that might change the interrupt state.
+ *
+ * Individual translators may define additional exit cases to deal
+ * with per-target special conditions.
+ */
 #define DISAS_NEXT    0 /* next instruction can be analyzed */
 #define DISAS_JUMP    1 /* only pc was modified dynamically */
-#define DISAS_UPDATE  2 /* cpu state was modified dynamically */
-#define DISAS_TB_JUMP 3 /* only pc was modified statically */
+#define DISAS_TB_JUMP 2 /* only pc was modified statically */
+#define DISAS_UPDATE  3 /* cpu state was modified dynamically */
 
 #include "qemu/log.h"
 
-- 
2.13.0

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

[Alex Bennée]

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

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

[Peter Maydell]

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

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

[Richard Henderson]

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~

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

[Alex Bennée]

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

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

[Richard Henderson]

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~

[Qemu-devel] [PATCH v1 3/6] target/arm/translate-a64: make DISAS_UPDATE match declared semantics

[Alex Bennée]

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 the call sites which set
->is_jmp to DISAS_UPDATE.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/translate-a64.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index e55547d95d..fe1c49b565 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1393,6 +1393,7 @@ static void handle_sync(DisasContext *s, uint32_t insn,
          * a self-modified code correctly and also to take
          * any pending interrupts immediately.
          */
+        gen_a64_set_pc_im(s->pc);
         s->is_jmp = DISAS_UPDATE;
         return;
     default:
@@ -1593,12 +1594,14 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
     if ((s->tb->cflags & CF_USE_ICOUNT) && (ri->type & ARM_CP_IO)) {
         /* I/O operations must end the TB here (whether read or write) */
         gen_io_end();
+        gen_a64_set_pc_im(s->pc);
         s->is_jmp = DISAS_UPDATE;
     } else if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
         /* We default to ending the TB on a coprocessor register write,
          * but allow this to be suppressed by the register definition
          * (usually only necessary to work around guest bugs).
          */
+        gen_a64_set_pc_im(s->pc);
         s->is_jmp = DISAS_UPDATE;
     }
 }
@@ -11364,16 +11367,9 @@ void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb)
         case DISAS_NEXT:
             gen_goto_tb(dc, 1, dc->pc);
             break;
-        default:
-        case DISAS_UPDATE:
-            gen_a64_set_pc_im(dc->pc);
-            /* fall through */
         case DISAS_JUMP:
             tcg_gen_lookup_and_goto_ptr(cpu_pc);
             break;
-        case DISAS_EXIT:
-            tcg_gen_exit_tb(0);
-            break;
         case DISAS_TB_JUMP:
         case DISAS_EXC:
         case DISAS_SWI:
@@ -11397,6 +11393,11 @@ void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb)
              */
             tcg_gen_exit_tb(0);
             break;
+        case DISAS_UPDATE:
+        case DISAS_EXIT:
+        default:
+            tcg_gen_exit_tb(0);
+            break;
         }
     }
 
-- 
2.13.0

[Qemu-devel] [PATCH v1 4/6] target/arm/translate-a64: get rid of DISAS_EXIT

[Alex Bennée]

We already have an exit condition that declares we should exit to the
run-loop because wider CPU state changes have been made. Use
DISAS_UPDATE and kill the architecture specific DISAS_EXIT.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/translate-a64.c | 3 +--
 target/arm/translate.c     | 6 ++++--
 target/arm/translate.h     | 4 ----
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index fe1c49b565..bde6ca934e 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1425,7 +1425,7 @@ static void handle_msr_i(DisasContext *s, uint32_t insn,
         tcg_temp_free_i32(tcg_op);
         /* For DAIFClear, exit the cpu loop to re-evaluate pending IRQs.  */
         gen_a64_set_pc_im(s->pc);
-        s->is_jmp = (op == 0x1f ? DISAS_EXIT : DISAS_JUMP);
+        s->is_jmp = (op == 0x1f ? DISAS_UPDATE : DISAS_JUMP);
         break;
     }
     default:
@@ -11394,7 +11394,6 @@ void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb)
             tcg_gen_exit_tb(0);
             break;
         case DISAS_UPDATE:
-        case DISAS_EXIT:
         default:
             tcg_gen_exit_tb(0);
             break;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index f9c4aee1b6..e840499c6f 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1194,11 +1194,13 @@ static void gen_exception_insn(DisasContext *s, int offset, int excp,
     s->is_jmp = DISAS_EXC;
 }
 
-/* Force a TB lookup after an instruction that changes the CPU state.  */
+/* Force a TB lookup after an instruction that changes the CPU state.
+ * (other than just the PC)
+ */
 static inline void gen_lookup_tb(DisasContext *s)
 {
     tcg_gen_movi_i32(cpu_R[15], s->pc & ~1);
-    s->is_jmp = DISAS_EXIT;
+    s->is_jmp = DISAS_UPDATE;
 }
 
 static inline void gen_hlt(DisasContext *s, int imm)
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 15d383d9af..6b2cc34c33 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -139,10 +139,6 @@ static void disas_set_insn_syndrome(DisasContext *s, uint32_t syn)
  * custom end-of-TB code)
  */
 #define DISAS_BX_EXCRET 11
-/* For instructions which want an immediate exit to the main loop,
- * as opposed to attempting to use lookup_and_goto_ptr.
- */
-#define DISAS_EXIT 12
 
 #ifdef TARGET_AARCH64
 void a64_translate_init(void);
-- 
2.13.0

Re: [Qemu-devel] [PATCH v1 4/6] target/arm/translate-a64: get rid of DISAS_EXIT

[Richard Henderson]

On 07/10/2017 05:47 AM, Alex Bennée wrote:
> We already have an exit condition that declares we should exit to the
> run-loop because wider CPU state changes have been made. Use
> DISAS_UPDATE and kill the architecture specific DISAS_EXIT.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   target/arm/translate-a64.c | 3 +--
>   target/arm/translate.c     | 6 ++++--
>   target/arm/translate.h     | 4 ----
>   3 files changed, 5 insertions(+), 8 deletions(-)

I really thought it was clearer the other way around.


r~

[Qemu-devel] [PATCH v1 5/6] target/arm: use DISAS_JUMP for ISB handling

[Alex Bennée]

While an ISB will ensure any raised IRQs happen on the next
instruction it doesn't cause any to get raised by itself. We can
therefor use DISAS_JUMP for ISB instructions and rely on the
exit_request check at the top of each TB to deal with exiting if
needed.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/translate-a64.c |  2 +-
 target/arm/translate.c     | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index bde6ca934e..fd9724b890 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1394,7 +1394,7 @@ static void handle_sync(DisasContext *s, uint32_t insn,
          * any pending interrupts immediately.
          */
         gen_a64_set_pc_im(s->pc);
-        s->is_jmp = DISAS_UPDATE;
+        s->is_jmp = DISAS_JUMP;
         return;
     default:
         unallocated_encoding(s);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index e840499c6f..f7f5f917c7 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1203,6 +1203,15 @@ static inline void gen_lookup_tb(DisasContext *s)
     s->is_jmp = DISAS_UPDATE;
 }
 
+/* End the current block and force a TB lookup. We may chain to the
+ * next TB but exit_req will be immediately checked so we will exit to
+ * the main loop if we need to */
+static inline void gen_jump_tb(DisasContext *s)
+{
+    tcg_gen_movi_i32(cpu_R[15], s->pc & ~1);
+    s->is_jmp = DISAS_JUMP;
+}
+
 static inline void gen_hlt(DisasContext *s, int imm)
 {
     /* HLT. This has two purposes.
@@ -8171,7 +8180,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
                  * self-modifying code correctly and also to take
                  * any pending interrupts immediately.
                  */
-                gen_lookup_tb(s);
+                gen_jump_tb(s);
                 return;
             default:
                 goto illegal_op;
@@ -10564,7 +10573,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                              * and also to take any pending interrupts
                              * immediately.
                              */
-                            gen_lookup_tb(s);
+                            gen_jump_tb(s);
                             break;
                         default:
                             goto illegal_op;
-- 
2.13.0

[Qemu-devel] [PATCH v1 6/6] target/arm: ensure eret exits the run-loop via DISAS_UPDATE

[Alex Bennée]

Previously DISAS_JUMP did ensure this but with the optimisation of
8a6b28c7 (optimize indirect branches) we might not leave the loop.
This means if any pending interrupts are cleared by changing IRQ flags
we might never get around to servicing them. You usually notice this
by seeing the lookup_tb_ptr() helper gainfully chaining TBs together
while cpu->interrupt_request remains high and the exit_request has not
been set.

This breaks amongst other things the OPTEE test suite which executes
an eret from the secure world after a non-secure world IRQ has gone
pending which then never gets serviced.

Instead of using the previously implied semantics of DISAS_JUMP we use
DISAS_UPDATE which (now) clearly states that the run-loop should be
exited as wider CPU State other than just the PC has changed.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
CC: Etienne Carriere <etienne.carriere@linaro.org>
CC: Joakim Bech <joakim.bech@linaro.org>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Emilio G. Cota <cota@braap.org>
CC: Richard Henderson <rth@twiddle.net>
---
 target/arm/translate-a64.c | 3 ++-
 target/arm/translate.c     | 6 ++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index fd9724b890..9efcba49d6 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1791,7 +1791,8 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
             return;
         }
         gen_helper_exception_return(cpu_env);
-        s->is_jmp = DISAS_JUMP;
+        /* Must exit loop to check un-masked IRQs */
+        s->is_jmp = DISAS_UPDATE;
         return;
     case 5: /* DRPS */
         if (rn != 0x1f) {
diff --git a/target/arm/translate.c b/target/arm/translate.c
index f7f5f917c7..75bdc6c7dd 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4488,7 +4488,8 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
      */
     gen_helper_cpsr_write_eret(cpu_env, cpsr);
     tcg_temp_free_i32(cpsr);
-    s->is_jmp = DISAS_JUMP;
+    /* Must exit loop to check un-masked IRQs */
+    s->is_jmp = DISAS_UPDATE;
 }
 
 /* Generate an old-style exception return. Marks pc as dead. */
@@ -9534,7 +9535,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
                     tmp = load_cpu_field(spsr);
                     gen_helper_cpsr_write_eret(cpu_env, tmp);
                     tcg_temp_free_i32(tmp);
-                    s->is_jmp = DISAS_JUMP;
+                    /* Must exit loop to check un-masked IRQs */
+                    s->is_jmp = DISAS_UPDATE;
                 }
             }
             break;
-- 
2.13.0