[v4,5/6] target/arm: use gen_goto_tb for ISB handling

Message ID 20170713141928.25419-6-alex.bennee@linaro.org
State Superseded
Headers show
Series
  • arm: fixes for eret, isb and DISAS_UPDATE handling
Related show

Commit Message

Alex Bennée July 13, 2017, 2:19 p.m.
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(-)

-- 
2.13.0

Comments

Peter Maydell July 14, 2017, 12:49 p.m. | #1
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
Alex Bennée July 14, 2017, 2:12 p.m. | #2
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
Peter Maydell July 14, 2017, 2:20 p.m. | #3
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

Patch

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;