[Qemu-devel] [PATCH v4 0/6] arm: fixes for eret, isb and DISAS_UPDATE handling

[Qemu-devel] [PATCH v4 0/6] arm: fixes for eret, isb and DISAS_UPDATE handling

[Alex Bennée]

Hi Peter,

All reviews are now in and I've tested against AArch64 OPTEE xtest
suite and it fixes the hang. Apart from review tags there was one
minor tweak to gen_goto_tb so it always sets s->is_jmp =
DISAS_TB_JUMP;

I reckon these are ready to be taken into the ARM tree now.

Regards,

Alex Bennée (6):
  include/exec/exec-all: document common exit conditions
  target/arm/translate: make DISAS_UPDATE match declared semantics
  target/arm/translate.h: expand comment on DISAS_EXIT
  target/arm/translate: ensure gen_goto_tb sets exit flags
  target/arm: use gen_goto_tb for ISB handling
  target/arm: use DISAS_EXIT for eret handling

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

-- 
2.13.0

[Qemu-devel] [PATCH v4 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>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 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 v4 2/6] target/arm/translate: 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.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 target/arm/translate-a64.c | 14 +++++++-------
 target/arm/translate.c     |  6 +++---
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index e55547d95d..66139b6046 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -11364,16 +11364,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 +11390,13 @@ void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb)
              */
             tcg_gen_exit_tb(0);
             break;
+        case DISAS_UPDATE:
+            gen_a64_set_pc_im(dc->pc);
+            /* fall through */
+        case DISAS_EXIT:
+        default:
+            tcg_gen_exit_tb(0);
+            break;
         }
     }
 
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 0862f9e4aa..5f2cea8641 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12095,12 +12095,12 @@ 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:
+            gen_set_pc_im(dc, dc->pc);
+            /* fall through */
         default:
             /* indicate that the hash table must be used to find the next TB */
             tcg_gen_exit_tb(0);
-- 
2.13.0

[Qemu-devel] [PATCH v4 3/6] target/arm/translate.h: expand comment on DISAS_EXIT

[Alex Bennée]

We already have an exit condition, DISAS_UPDATE which will exit the
run-loop. Expand on the difference with DISAS_EXIT in the comments.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 target/arm/translate.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate.h b/target/arm/translate.h
index 15d383d9af..12fd79ba8e 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -140,7 +140,10 @@ static void disas_set_insn_syndrome(DisasContext *s, uint32_t syn)
  */
 #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.
+ * as opposed to attempting to use lookup_and_goto_ptr. Unlike
+ * DISAS_UPDATE this doesn't write the PC on exiting the translation
+ * loop so you need to ensure something (gen_a64_set_pc_im or runtime
+ * helper) has done so before we reach return from cpu_tb_exec.
  */
 #define DISAS_EXIT 12
 
-- 
2.13.0

[Qemu-devel] [PATCH v4 4/6] target/arm/translate: ensure gen_goto_tb sets exit flags

[Alex Bennée]

As the gen_goto_tb function can do both static and dynamic jumps it
should also set the is_jmp field. This matches the behaviour of the
a64 code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>

--
v4
  - set s->is_jmp = DISAS_TB_JUMP in both cases
---
 target/arm/translate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 5f2cea8641..493a7b424a 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4158,6 +4158,9 @@ static void gen_goto_ptr(void)
     tcg_temp_free(addr);
 }
 
+/* This will end the TB but doesn't guarantee we'll return to
+ * cpu_loop_exec. Any live exit_requests will be processed as we
+ * enter the next TB. */
 static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)
 {
     if (use_goto_tb(s, dest)) {
@@ -4168,6 +4171,7 @@ static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)
         gen_set_pc_im(s, dest);
         gen_goto_ptr();
     }
+    s->is_jmp = DISAS_TB_JUMP;
 }
 
 static inline void gen_jmp (DisasContext *s, uint32_t dest)
@@ -4179,7 +4183,6 @@ static inline void gen_jmp (DisasContext *s, uint32_t dest)
         gen_bx_im(s, dest);
     } else {
         gen_goto_tb(s, 0, dest);
-        s->is_jmp = DISAS_TB_JUMP;
     }
 }
 
-- 
2.13.0

[Qemu-devel] [PATCH v4 5/6] target/arm: use gen_goto_tb 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 a simple tb exit 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>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 target/arm/translate-a64.c | 2 +-
 target/arm/translate.c     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 66139b6046..2ac565eb10 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1393,7 +1393,7 @@ static void handle_sync(DisasContext *s, uint32_t insn,
          * a self-modified code correctly and also to take
          * any pending interrupts immediately.
          */
-        s->is_jmp = DISAS_UPDATE;
+        gen_goto_tb(s, 0, s->pc);
         return;
     default:
         unallocated_encoding(s);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 493a7b424a..d8892d9ba5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8168,7 +8168,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_goto_tb(s, 0, s->pc & ~1);
                 return;
             default:
                 goto illegal_op;
@@ -10561,7 +10561,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_goto_tb(s, 0, s->pc & ~1);
                             break;
                         default:
                             goto illegal_op;
-- 
2.13.0

Re: [Qemu-devel] [PATCH v4 5/6] target/arm: use gen_goto_tb for ISB handling

[Peter Maydell]

On 13 July 2017 at 15:19, Alex Bennée <alex.bennee@linaro.org> wrote:
> 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 a simple tb exit 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>
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> ---
>  target/arm/translate-a64.c | 2 +-
>  target/arm/translate.c     | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 66139b6046..2ac565eb10 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1393,7 +1393,7 @@ static void handle_sync(DisasContext *s, uint32_t insn,
>           * a self-modified code correctly and also to take
>           * any pending interrupts immediately.
>           */
> -        s->is_jmp = DISAS_UPDATE;
> +        gen_goto_tb(s, 0, s->pc);
>          return;
>      default:
>          unallocated_encoding(s);
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 493a7b424a..d8892d9ba5 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8168,7 +8168,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_goto_tb(s, 0, s->pc & ~1);
>                  return;
>              default:
>                  goto illegal_op;
> @@ -10561,7 +10561,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_goto_tb(s, 0, s->pc & ~1);
>                              break;
>                          default:
>                              goto illegal_op;

Why do we need to clear the low bit of s->pc for ISB?
s->pc is the actual PC, not the "PC and low bit indicates
Thumb mode" form that jump addresses have.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 5/6] target/arm: use gen_goto_tb for ISB handling

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 13 July 2017 at 15:19, Alex Bennée <alex.bennee@linaro.org> wrote:
>> 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 a simple tb exit 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>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>> ---
>>  target/arm/translate-a64.c | 2 +-
>>  target/arm/translate.c     | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>> index 66139b6046..2ac565eb10 100644
>> --- a/target/arm/translate-a64.c
>> +++ b/target/arm/translate-a64.c
>> @@ -1393,7 +1393,7 @@ static void handle_sync(DisasContext *s, uint32_t insn,
>>           * a self-modified code correctly and also to take
>>           * any pending interrupts immediately.
>>           */
>> -        s->is_jmp = DISAS_UPDATE;
>> +        gen_goto_tb(s, 0, s->pc);
>>          return;
>>      default:
>>          unallocated_encoding(s);
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index 493a7b424a..d8892d9ba5 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -8168,7 +8168,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_goto_tb(s, 0, s->pc & ~1);
>>                  return;
>>              default:
>>                  goto illegal_op;
>> @@ -10561,7 +10561,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_goto_tb(s, 0, s->pc & ~1);
>>                              break;
>>                          default:
>>                              goto illegal_op;
>
> Why do we need to clear the low bit of s->pc for ISB?
> s->pc is the actual PC, not the "PC and low bit indicates
> Thumb mode" form that jump addresses have.

It's what gen_lookup_tb does to it's PC before the calculated jump. If
it can never happen I can get rid of it.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v4 5/6] target/arm: use gen_goto_tb for ISB handling

[Peter Maydell]

On 14 July 2017 at 15:12, Alex Bennée <alex.bennee@linaro.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> Why do we need to clear the low bit of s->pc for ISB?
>> s->pc is the actual PC, not the "PC and low bit indicates
>> Thumb mode" form that jump addresses have.
>
> It's what gen_lookup_tb does to it's PC before the calculated jump. If
> it can never happen I can get rid of it.

Hmm, I think that it's unnecessary, but since we were doing this before
I guess better to make cleaning it up be a separate patch.

thanks
-- PMM

[Qemu-devel] [PATCH v4 6/6] target/arm: use DISAS_EXIT for eret handling

[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_EXIT which will always exit the run-loop.

CC: Etienne Carriere <etienne.carriere@linaro.org>
CC: Joakim Bech <joakim.bech@linaro.org>
CC: Jaroslaw Pelczar <j.pelczar@samsung.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Emilio G. Cota <cota@braap.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: 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 2ac565eb10..3fa39023ca 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1788,7 +1788,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_EXIT;
         return;
     case 5: /* DRPS */
         if (rn != 0x1f) {
diff --git a/target/arm/translate.c b/target/arm/translate.c
index d8892d9ba5..2d2b3f772c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4478,7 +4478,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_EXIT;
 }
 
 /* Generate an old-style exception return. Marks pc as dead. */
@@ -9522,7 +9523,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_EXIT;
                 }
             }
             break;
-- 
2.13.0

Re: [Qemu-devel] [PATCH v4 0/6] arm: fixes for eret, isb and DISAS_UPDATE handling

[Peter Maydell]

On 13 July 2017 at 15:19, Alex Bennée <alex.bennee@linaro.org> wrote:
> Hi Peter,
>
> All reviews are now in and I've tested against AArch64 OPTEE xtest
> suite and it fixes the hang. Apart from review tags there was one
> minor tweak to gen_goto_tb so it always sets s->is_jmp =
> DISAS_TB_JUMP;
>
> I reckon these are ready to be taken into the ARM tree now.
>

Applied to target-arm.next, thanks. We can fix up the long
standing oddity of masking bit 0 of s->pc in a different patch.

-- PMM