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

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

[Alex Bennée]

Hi,

This is a quick iteration from the previous series:

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

Which was born out of the fix:

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

Following feedback I reverted the attempt to use DISAS_UPDATE for
everything and just made it match the semantics of not-chaining. So
patches 1 & 3 are purely documentation.

Patch 2 should prevent any system change prompted by writing to a
system register not exiting the loop.

Patch 4 is a minor optimisation to ISB handling
Patch 5 fixes the eret regression from the DISAS_JUMP optimisation

I think now all users of DISAS_JUMP should be safe with respect to TB
chaining.

Please review.

Alex Bennée (5):
  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: use DISAS_JUMP for ISB handling
  target/arm: use DISAS_EXIT for eret handling

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

-- 
2.13.0

[Qemu-devel] [PATCH v2 1/5] 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 v2 2/5] 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>
---
 target/arm/translate-a64.c | 14 +++++++-------
 target/arm/translate.c     |  5 ++---
 2 files changed, 9 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..ccc4768b2e 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12095,12 +12095,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:
+            gen_set_pc_im(dc, dc->pc);
         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 v2 2/5] target/arm/translate: make DISAS_UPDATE match declared semantics

[Richard Henderson]

On 07/10/2017 09:21 AM, Alex Bennée wrote:
> -        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);
>           default:

Modulo missing fallthru markup,

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[Qemu-devel] [PATCH v2 3/5] 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>
---
 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

Re: [Qemu-devel] [PATCH v2 3/5] target/arm/translate.h: expand comment on DISAS_EXIT

[Richard Henderson]

On 07/10/2017 09:21 AM, Alex Bennée wrote:
> 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>
> ---
>   target/arm/translate.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[Qemu-devel] [PATCH v2 4/5] 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 |  3 ++-
 target/arm/translate.c     | 13 +++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 66139b6046..ad46d84efb 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1393,7 +1393,8 @@ 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_a64_set_pc_im(s->pc);
+        s->is_jmp = DISAS_JUMP;
         return;
     default:
         unallocated_encoding(s);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index ccc4768b2e..94aa4bbb4d 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1201,6 +1201,15 @@ static inline void gen_lookup_tb(DisasContext *s)
     s->is_jmp = DISAS_EXIT;
 }
 
+/* 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.
@@ -8165,7 +8174,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;
@@ -10558,7 +10567,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

Re: [Qemu-devel] [PATCH v2 4/5] target/arm: use DISAS_JUMP for ISB handling

[Richard Henderson]

On 07/10/2017 09:21 AM, Alex Bennée wrote:
> -        s->is_jmp = DISAS_UPDATE;
> +        gen_a64_set_pc_im(s->pc);
> +        s->is_jmp = DISAS_JUMP;

Better would be gen_goto_tb.  The destination is known, so there's no need to 
go through lookup_and_goto_ptr.  You still get the icount_decr check at the 
start of the linked TB, which is what you're looking for.

Interesting that a64's gen_goto_tb sets is_jmp, but a32 does not...


r~

Re: [Qemu-devel] [PATCH v2 4/5] target/arm: use DISAS_JUMP for ISB handling

[Alex Bennée]

Richard Henderson <rth@twiddle.net> writes:

> On 07/10/2017 09:21 AM, Alex Bennée wrote:
>> -        s->is_jmp = DISAS_UPDATE;
>> +        gen_a64_set_pc_im(s->pc);
>> +        s->is_jmp = DISAS_JUMP;
>
> Better would be gen_goto_tb.  The destination is known, so there's no
> need to go through lookup_and_goto_ptr.  You still get the icount_decr
> check at the start of the linked TB, which is what you're looking for.
>
> Interesting that a64's gen_goto_tb sets is_jmp, but a32 does not...

Hmm the only caller that is not already in the exit path sets it. Maybe
I should push the s->is_jmp to the a32 gen_goto_tb? I can then do the
same in both.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 4/5] target/arm: use DISAS_JUMP for ISB handling

[Richard Henderson]

On 07/10/2017 10:27 PM, Alex Bennée wrote:
> 
> Richard Henderson <rth@twiddle.net> writes:
> 
>> On 07/10/2017 09:21 AM, Alex Bennée wrote:
>>> -        s->is_jmp = DISAS_UPDATE;
>>> +        gen_a64_set_pc_im(s->pc);
>>> +        s->is_jmp = DISAS_JUMP;
>>
>> Better would be gen_goto_tb.  The destination is known, so there's no
>> need to go through lookup_and_goto_ptr.  You still get the icount_decr
>> check at the start of the linked TB, which is what you're looking for.
>>
>> Interesting that a64's gen_goto_tb sets is_jmp, but a32 does not...
> 
> Hmm the only caller that is not already in the exit path sets it. Maybe
> I should push the s->is_jmp to the a32 gen_goto_tb? I can then do the
> same in both.

Sounds reasonable.


r~

[Qemu-devel] [PATCH v2 5/5] 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.

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 ad46d84efb..48825f5722 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1789,7 +1789,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 94aa4bbb4d..c67a4f90d4 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4484,7 +4484,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. */
@@ -9528,7 +9529,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 v2 5/5] target/arm: use DISAS_EXIT for eret handling

[Richard Henderson]

On 07/10/2017 09:21 AM, Alex Bennée wrote:
> 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.
> 
> 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(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>

As an aside, I presume we don't have support for armv7ve?  I was expecting 
there to be an eret insn in the aa32 translator and had to dig up previous 
manuals to see when that insn was introduced.


r~

Re: [Qemu-devel] [PATCH v2 5/5] target/arm: use DISAS_EXIT for eret handling

[Peter Maydell]

On 10 July 2017 at 20:58, Richard Henderson <rth@twiddle.net> wrote:
> As an aside, I presume we don't have support for armv7ve?  I was expecting
> there to be an eret insn in the aa32 translator and had to dig up previous
> manuals to see when that insn was introduced.

We don't yet fully support 32-bit EL2, no. We've been adding bits
and pieces but still haven't filled it all in. Edgar might have
a patch to add ARM A32 ERET to the decoder in the xilinx tree?

thanks
-- PMM