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

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

[Alex Bennée]

Hi,

Hopefully this is the final iteration after:

  https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg02452.html

The only major change is fixing up gen_goto_tb for arm and then using
that in both a32 and a64 for the ISB changes. Other than that I've
applied the review tags.

Hopefully there isn't anything else that needs fixing so I guess the
next question is who's tree should this go through?

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     | 22 ++++++++++++++--------
 target/arm/translate.h     |  5 ++++-
 4 files changed, 54 insertions(+), 21 deletions(-)

-- 
2.13.0

[Qemu-devel] [PATCH v3 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

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

[Richard Henderson]

On 07/11/2017 07:59 AM, Alex Bennée wrote:
> 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(-)

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


r~

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

[Emilio G. Cota]

On Tue, Jul 11, 2017 at 18:59:32 +0100, Alex Bennée wrote:
> 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.
(snip)
> + * 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.

'o' x 3

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

s/necessities/necessitates/

Reviewed-by: Emilio G. Cota <cota@braap.org>

		E.

[Qemu-devel] [PATCH v3 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

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

[Emilio G. Cota]

On Tue, Jul 11, 2017 at 18:59:33 +0100, Alex Bennée 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

s/therefor/therefore/

> and return to the main execution loop rather assuming DISAS_JUMP would

Rather *than* assuming ?

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

Reviewed-by: Emilio G. Cota <cota@braap.org>

		E.

[Qemu-devel] [PATCH v3 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

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

[Emilio G. Cota]

On Tue, Jul 11, 2017 at 18:59:34 +0100, 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>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Reviewed-by: Emilio G. Cota <cota@braap.org>

		E.

[Qemu-devel] [PATCH v3 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>
---
 target/arm/translate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 5f2cea8641..82341ee709 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4158,15 +4158,20 @@ 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)) {
         tcg_gen_goto_tb(n);
         gen_set_pc_im(s, dest);
         tcg_gen_exit_tb((uintptr_t)s->tb + n);
+        s->is_jmp = DISAS_TB_JUMP;
     } else {
         gen_set_pc_im(s, dest);
         gen_goto_ptr();
+        s->is_jmp = DISAS_JUMP;
     }
 }
 
@@ -4179,7 +4184,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

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

[Richard Henderson]

On 07/11/2017 07:59 AM, Alex Bennée wrote:
>       if (use_goto_tb(s, dest)) {
>           tcg_gen_goto_tb(n);
>           gen_set_pc_im(s, dest);
>           tcg_gen_exit_tb((uintptr_t)s->tb + n);
> +        s->is_jmp = DISAS_TB_JUMP;
>       } else {
>           gen_set_pc_im(s, dest);
>           gen_goto_ptr();
> +        s->is_jmp = DISAS_JUMP;
>       }

I think DISAS_TB_JUMP is appropriate for both cases.  When not using goto_tb, 
the jump is still static and we still chain to the next TB via goto_ptr.

Otherwise,

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


r~

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

[Alex Bennée]

Richard Henderson <rth@twiddle.net> writes:

> On 07/11/2017 07:59 AM, Alex Bennée wrote:
>>       if (use_goto_tb(s, dest)) {
>>           tcg_gen_goto_tb(n);
>>           gen_set_pc_im(s, dest);
>>           tcg_gen_exit_tb((uintptr_t)s->tb + n);
>> +        s->is_jmp = DISAS_TB_JUMP;
>>       } else {
>>           gen_set_pc_im(s, dest);
>>           gen_goto_ptr();
>> +        s->is_jmp = DISAS_JUMP;
>>       }
>
> I think DISAS_TB_JUMP is appropriate for both cases.  When not using
> goto_tb, the jump is still static and we still chain to the next TB
> via goto_ptr.

OK - I guess we need to nail down what the essential difference is
between the two. I understood DISAS_TB_JUMP as a static known PC which
can be patched in the generated code because we know the two addresses
are in the same page - whereas DISAS_JUMP is a "computed" jump although
in this case the PC is already known.

Does making a distinction between computed and non-computer inter-page
jumps make any sense anyway?

>
> Otherwise,
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>
>
>
> r~


--
Alex Bennée

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

[Richard Henderson]

On 07/11/2017 09:20 AM, Alex Bennée wrote:
> 
> Richard Henderson <rth@twiddle.net> writes:
> 
>> On 07/11/2017 07:59 AM, Alex Bennée wrote:
>>>        if (use_goto_tb(s, dest)) {
>>>            tcg_gen_goto_tb(n);
>>>            gen_set_pc_im(s, dest);
>>>            tcg_gen_exit_tb((uintptr_t)s->tb + n);
>>> +        s->is_jmp = DISAS_TB_JUMP;
>>>        } else {
>>>            gen_set_pc_im(s, dest);
>>>            gen_goto_ptr();
>>> +        s->is_jmp = DISAS_JUMP;
>>>        }
>>
>> I think DISAS_TB_JUMP is appropriate for both cases.  When not using
>> goto_tb, the jump is still static and we still chain to the next TB
>> via goto_ptr.
> 
> OK - I guess we need to nail down what the essential difference is
> between the two. I understood DISAS_TB_JUMP as a static known PC which
> can be patched in the generated code because we know the two addresses
> are in the same page - whereas DISAS_JUMP is a "computed" jump although
> in this case the PC is already known.
> 
> Does making a distinction between computed and non-computer inter-page
> jumps make any sense anyway?

*shrug* probably not.

Honestly, the only thing that's really interesting here is that is_jmp indicate 
that we have already exited the TB and that all following code is dead.

As a response to one of Lluis' threads, I suggested that the generic term for 
this be DISAS_NORETURN.  Which also covers exiting the TB via calling a helper 
that does not return, e.g. throwing an illegal opcode exception, aka DISAS_EXC 
in the current target/arm sources.


r~

[Qemu-devel] [PATCH v3 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>
---
 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 82341ee709..dbf919cce3 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8169,7 +8169,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;
@@ -10562,7 +10562,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 v3 5/6] target/arm: use gen_goto_tb for ISB handling

[Richard Henderson]

On 07/11/2017 07:59 AM, Alex Bennée 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>
> ---
>   target/arm/translate-a64.c | 2 +-
>   target/arm/translate.c     | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)

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


r~

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

[Emilio G. Cota]

On Tue, Jul 11, 2017 at 18:59:36 +0100, Alex Bennée 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

s/therefor/therefore/

> 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: Emilio G. Cota <cota@braap.org>

		E.

[Qemu-devel] [PATCH v3 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: 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 dbf919cce3..f1023d5263 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4479,7 +4479,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. */
@@ -9523,7 +9524,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 v3 6/6] target/arm: use DISAS_EXIT for eret handling

[Emilio G. Cota]

On Tue, Jul 11, 2017 at 18:59:37 +0100, 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.
> 
> 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>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Reviewed-by: Emilio G. Cota <cota@braap.org>

		E.

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

[Richard Henderson]

On 07/11/2017 07:59 AM, Alex Bennée wrote:
> Hopefully there isn't anything else that needs fixing so I guess the
> next question is who's tree should this go through?

I'm happy with this going through the arm tree.


r~