[PATCH 00/26] accel/tcg: Introduce translator_use_goto_tb

[PATCH 00/26] accel/tcg: Introduce translator_use_goto_tb

[Richard Henderson]

There are a number of inconsistencies with goto_tb usage, and I
plan to make changes in order to better support breakpoints.

(1) Testing CF_LAST_IO is a hold-over from since before ba3e7926691
    ("icount: clean up cpu_can_io at the entry to the block").
    Several targets still have this test.

(2) Testing singlestep is superfluous, as it doesn't mean anything
    besides limiting max_insns to 1.

(3) Not testing page crossing for CONFIG_USER_ONLY is wrong, because
    mmap and mprotect can change page permissions.  It's a very
    uncommon case wrt executables, but it's still wrong.

(4) Not testing page crossing for non-mmu targets (where page
    permissions literally cannot change) is not currently wrong,
    but will be after the breakpoint changes.

(5) When the TB does cross two pages, considering non-page crossing
    from the second page is not currently wrong, but will be after
    the breakpoint changes.

Based on my recent avr, cris and nios2 patch sets.  The full tree is

    https://gitlab.com/rth7680/qemu/-/tree/tcg-goto-tb


r~


Richard Henderson (26):
  accel/tcg: Introduce translator_use_goto_tb
  target/alpha: Remove use_exit_tb
  target/alpha: Remove in_superpage
  target/alpha: Use translator_use_goto_tb
  target/arm: Use translator_use_goto_tb
  target/avr: Use translator_use_goto_tb
  target/avr: Mark some helpers noreturn
  target/cris: Use translator_use_goto_tb
  target/hppa: Use translator_use_goto_tb
  target/i386: Use translator_use_goto_tb
  target/m68k: Use translator_use_goto_tb
  target/microblaze: Use translator_use_goto_tb
  target/mips: Use translator_use_goto_tb
  target/mips: Fix missing else in gen_goto_tb
  target/nios2: Use translator_use_goto_tb
  target/openrisc: Use translator_use_goto_tb
  target/ppc: Use translator_use_goto_tb
  target/riscv: Use translator_use_goto_tb
  target/rx: Use translator_use_goto_tb
  target/s390x: Use translator_use_goto_tb
  target/s390x: Remove use_exit_tb
  target/sh4: Use translator_use_goto_tb
  target/sparc: Use translator_use_goto_tb
  target/tricore: Use translator_use_goto_tb
  target/tricore: Use tcg_gen_lookup_and_goto_ptr
  target/xtensa: Use translator_use_goto_tb

 include/exec/translator.h     | 10 ++++++++
 target/arm/translate.h        | 13 ++++++++++
 target/avr/helper.h           |  8 +++---
 accel/tcg/translator.c        | 11 +++++++++
 target/alpha/translate.c      | 46 ++++-------------------------------
 target/arm/translate-a64.c    | 22 +----------------
 target/arm/translate.c        | 10 --------
 target/avr/translate.c        |  9 ++++---
 target/cris/translate.c       |  5 ++--
 target/hppa/translate.c       |  5 +---
 target/i386/tcg/translate.c   | 14 ++---------
 target/m68k/translate.c       | 12 +--------
 target/microblaze/translate.c | 11 +--------
 target/mips/tcg/translate.c   | 20 +++------------
 target/nios2/translate.c      | 15 +-----------
 target/openrisc/translate.c   | 15 ++++++------
 target/ppc/translate.c        | 10 +-------
 target/riscv/translate.c      | 20 +--------------
 target/rx/translate.c         | 11 +--------
 target/s390x/translate.c      | 18 +++-----------
 target/sh4/translate.c        | 11 +++------
 target/sparc/translate.c      | 19 ++++-----------
 target/tricore/translate.c    | 20 +++------------
 target/xtensa/translate.c     |  6 +----
 24 files changed, 89 insertions(+), 252 deletions(-)

-- 
2.25.1

[PATCH 01/26] accel/tcg: Introduce translator_use_goto_tb

[Richard Henderson]

Add a generic version of the common use_goto_tb test.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/translator.h | 10 ++++++++++
 accel/tcg/translator.c    | 11 +++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/exec/translator.h b/include/exec/translator.h
index 24232ead41..dd9c06d40d 100644
--- a/include/exec/translator.h
+++ b/include/exec/translator.h
@@ -145,6 +145,16 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
 
 void translator_loop_temp_check(DisasContextBase *db);
 
+/**
+ * translator_use_goto_tb
+ * @db: Disassembly context
+ * @dest: target pc of the goto
+ *
+ * Return true if goto_tb is allowed between the current TB
+ * and the destination PC.
+ */
+bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest);
+
 /*
  * Translator Load Functions
  *
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 1d32732198..59804af37b 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -31,6 +31,17 @@ void translator_loop_temp_check(DisasContextBase *db)
     }
 }
 
+bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
+{
+    /* Suppress goto_tb in the case of single-steping.  */
+    if (db->singlestep_enabled || singlestep) {
+        return false;
+    }
+
+    /* Check for the dest on the same page as the start of the TB.  */
+    return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0;
+}
+
 void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
                      CPUState *cpu, TranslationBlock *tb, int max_insns)
 {
-- 
2.25.1

Re: [PATCH 01/26] accel/tcg: Introduce translator_use_goto_tb

[Max Filippov]

On Sun, Jun 20, 2021 at 6:36 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Add a generic version of the common use_goto_tb test.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/translator.h | 10 ++++++++++
>  accel/tcg/translator.c    | 11 +++++++++++
>  2 files changed, 21 insertions(+)

Reviewed-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

Re: [PATCH 01/26] accel/tcg: Introduce translator_use_goto_tb

[Philippe Mathieu-Daudé]

Hi Richard,

On 6/21/21 3:34 AM, Richard Henderson wrote:
> Add a generic version of the common use_goto_tb test.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/translator.h | 10 ++++++++++
>  accel/tcg/translator.c    | 11 +++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index 24232ead41..dd9c06d40d 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -145,6 +145,16 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>  
>  void translator_loop_temp_check(DisasContextBase *db);
>  
> +/**
> + * translator_use_goto_tb
> + * @db: Disassembly context
> + * @dest: target pc of the goto
> + *
> + * Return true if goto_tb is allowed between the current TB
> + * and the destination PC.
> + */
> +bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest);
> +
>  /*
>   * Translator Load Functions
>   *
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 1d32732198..59804af37b 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -31,6 +31,17 @@ void translator_loop_temp_check(DisasContextBase *db)
>      }
>  }
>  
> +bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
> +{
> +    /* Suppress goto_tb in the case of single-steping.  */
> +    if (db->singlestep_enabled || singlestep) {
> +        return false;
> +    }
> +

I notice various targets do:

#ifdef CONFIG_USER_ONLY
       return true;
#else

> +    /* Check for the dest on the same page as the start of the TB.  */
> +    return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0;

#endif

> +}

Is that OK to remove this case? If so, it might be worth a comment
somewhere.

Regards,

Phil.

Re: [PATCH 01/26] accel/tcg: Introduce translator_use_goto_tb

[Richard Henderson]

On 6/21/21 5:50 AM, Philippe Mathieu-Daudé wrote:
> I notice various targets do:
> 
> #ifdef CONFIG_USER_ONLY
>         return true;
> #else
> 
>> +    /* Check for the dest on the same page as the start of the TB.  */
>> +    return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0;
> #endif
> 
>> +}
> Is that OK to remove this case? If so, it might be worth a comment
> somewhere.

I mentioned it in the cover letter.


r~

Re: [PATCH 01/26] accel/tcg: Introduce translator_use_goto_tb

[Philippe Mathieu-Daudé]

On 6/21/21 3:47 PM, Richard Henderson wrote:
> On 6/21/21 5:50 AM, Philippe Mathieu-Daudé wrote:
>> I notice various targets do:
>>
>> #ifdef CONFIG_USER_ONLY
>>         return true;
>> #else
>>
>>> +    /* Check for the dest on the same page as the start of the TB.  */
>>> +    return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0;
>> #endif
>>
>>> +}
>> Is that OK to remove this case? If so, it might be worth a comment
>> somewhere.
> 
> I mentioned it in the cover letter.

But the commit letter will vanish, so preferably mentioning
the change in the commit description (copying the cover letter
comment could work too):

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

RE: [PATCH 01/26] accel/tcg: Introduce translator_use_goto_tb

[Luis Fernando Fujita Pires]

From: Richard Henderson <richard.henderson@linaro.org>
> Add a generic version of the common use_goto_tb test.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/translator.h | 10 ++++++++++
>  accel/tcg/translator.c    | 11 +++++++++++
>  2 files changed, 21 insertions(+)

Reviewed-by: Luis Pires <luis.pires@eldorado.org.br>

--
Luis Pires
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[PATCH 02/26] target/alpha: Remove use_exit_tb

[Richard Henderson]

We have not needed to end a TB for I/O since ba3e7926691
("icount: clean up cpu_can_io at the entry to the block").
We do not need to use exit_tb for singlestep, which only
means generate one insn per TB.

Which leaves only singlestep_enabled, which means raise a
debug trap after every TB, which does not use exit_tb,
which would leave the function mis-named.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/alpha/translate.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/target/alpha/translate.c b/target/alpha/translate.c
index f454adea5e..70ba4a67c7 100644
--- a/target/alpha/translate.c
+++ b/target/alpha/translate.c
@@ -450,19 +450,8 @@ static bool in_superpage(DisasContext *ctx, int64_t addr)
 #endif
 }
 
-static bool use_exit_tb(DisasContext *ctx)
-{
-    return ((tb_cflags(ctx->base.tb) & CF_LAST_IO)
-            || ctx->base.singlestep_enabled
-            || singlestep);
-}
-
 static bool use_goto_tb(DisasContext *ctx, uint64_t dest)
 {
-    /* Suppress goto_tb in the case of single-steping and IO.  */
-    if (unlikely(use_exit_tb(ctx))) {
-        return false;
-    }
 #ifndef CONFIG_USER_ONLY
     /* If the destination is in the superpage, the page perms can't change.  */
     if (in_superpage(ctx, dest)) {
@@ -1271,7 +1260,7 @@ static DisasJumpType gen_call_pal(DisasContext *ctx, int palcode)
            need the page permissions check.  We'll see the existence of
            the page when we create the TB, and we'll flush all TBs if
            we change the PAL base register.  */
-        if (!use_exit_tb(ctx)) {
+        if (!ctx->base.singlestep_enabled) {
             tcg_gen_goto_tb(0);
             tcg_gen_movi_i64(cpu_pc, entry);
             tcg_gen_exit_tb(ctx->base.tb, 0);
@@ -3020,7 +3009,7 @@ static void alpha_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
         tcg_gen_movi_i64(cpu_pc, ctx->base.pc_next);
         /* FALLTHRU */
     case DISAS_PC_UPDATED:
-        if (!use_exit_tb(ctx)) {
+        if (!ctx->base.singlestep_enabled) {
             tcg_gen_lookup_and_goto_ptr();
             break;
         }
-- 
2.25.1

[PATCH 03/26] target/alpha: Remove in_superpage

[Richard Henderson]

The number of links across (normal) pages using this is low,
and it will shortly violate the contract for breakpoints.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/alpha/translate.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/target/alpha/translate.c b/target/alpha/translate.c
index 70ba4a67c7..6ea19a1d4c 100644
--- a/target/alpha/translate.c
+++ b/target/alpha/translate.c
@@ -439,24 +439,9 @@ static DisasJumpType gen_store_conditional(DisasContext *ctx, int ra, int rb,
     return DISAS_NEXT;
 }
 
-static bool in_superpage(DisasContext *ctx, int64_t addr)
-{
-#ifndef CONFIG_USER_ONLY
-    return ((ctx->tbflags & ENV_FLAG_PS_USER) == 0
-            && addr >> TARGET_VIRT_ADDR_SPACE_BITS == -1
-            && ((addr >> 41) & 3) == 2);
-#else
-    return false;
-#endif
-}
-
 static bool use_goto_tb(DisasContext *ctx, uint64_t dest)
 {
 #ifndef CONFIG_USER_ONLY
-    /* If the destination is in the superpage, the page perms can't change.  */
-    if (in_superpage(ctx, dest)) {
-        return true;
-    }
     /* Check for the dest on the same page as the start of the TB.  */
     return ((ctx->base.tb->pc ^ dest) & TARGET_PAGE_MASK) == 0;
 #else
@@ -2916,7 +2901,7 @@ static void alpha_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPUAlphaState *env = cpu->env_ptr;
-    int64_t bound, mask;
+    int64_t bound;
 
     ctx->tbflags = ctx->base.tb->flags;
     ctx->mem_idx = cpu_mmu_index(env, false);
@@ -2945,12 +2930,7 @@ static void alpha_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu)
     ctx->lit = NULL;
 
     /* Bound the number of insns to execute to those left on the page.  */
-    if (in_superpage(ctx, ctx->base.pc_first)) {
-        mask = -1ULL << 41;
-    } else {
-        mask = TARGET_PAGE_MASK;
-    }
-    bound = -(ctx->base.pc_first | mask) / 4;
+    bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;
     ctx->base.max_insns = MIN(ctx->base.max_insns, bound);
 }
 
-- 
2.25.1

[PATCH 04/26] target/alpha: Use translator_use_goto_tb

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/alpha/translate.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/target/alpha/translate.c b/target/alpha/translate.c
index 6ea19a1d4c..1aa6d24d83 100644
--- a/target/alpha/translate.c
+++ b/target/alpha/translate.c
@@ -441,12 +441,7 @@ static DisasJumpType gen_store_conditional(DisasContext *ctx, int ra, int rb,
 
 static bool use_goto_tb(DisasContext *ctx, uint64_t dest)
 {
-#ifndef CONFIG_USER_ONLY
-    /* Check for the dest on the same page as the start of the TB.  */
-    return ((ctx->base.tb->pc ^ dest) & TARGET_PAGE_MASK) == 0;
-#else
-    return true;
-#endif
+    return translator_use_goto_tb(&ctx->base, dest);
 }
 
 static DisasJumpType gen_bdirect(DisasContext *ctx, int ra, int32_t disp)
-- 
2.25.1

[PATCH 05/26] target/arm: Use translator_use_goto_tb

[Richard Henderson]

Put a wrapper in translate.h, which also checks for ss_active.
The ss_active test was incorrectly missing from the a32 version.

Cc: qemu-arm@nongnu.org
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate.h     | 13 +++++++++++++
 target/arm/translate-a64.c | 22 +---------------------
 target/arm/translate.c     | 10 ----------
 3 files changed, 14 insertions(+), 31 deletions(-)

diff --git a/target/arm/translate.h b/target/arm/translate.h
index 2821b325e3..3a62f50faf 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -116,6 +116,19 @@ typedef struct DisasContext {
     TCGv_i64 tmp_a64[TMP_A64_MAX];
 } DisasContext;
 
+static inline bool use_goto_tb(DisasContext *s, target_ulong dest)
+{
+    /*
+     * No direct tb linking with singlestep.
+     * This handles the ARM debug architecture kind; the QEMU kind
+     * is handled inside translator_use_goto_tb.
+     */
+    if (s->ss_active) {
+        return false;
+    }
+    return translator_use_goto_tb(&s->base, dest);
+}
+
 typedef struct DisasCompare {
     TCGCond cond;
     TCGv_i32 value;
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 7f74d0e81a..ac58a86e59 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -386,32 +386,12 @@ static void gen_step_complete_exception(DisasContext *s)
     s->base.is_jmp = DISAS_NORETURN;
 }
 
-static inline bool use_goto_tb(DisasContext *s, int n, uint64_t dest)
-{
-    /* No direct tb linking with singlestep (either QEMU's or the ARM
-     * debug architecture kind) or deterministic io
-     */
-    if (s->base.singlestep_enabled || s->ss_active ||
-        (tb_cflags(s->base.tb) & CF_LAST_IO)) {
-        return false;
-    }
-
-#ifndef CONFIG_USER_ONLY
-    /* Only link tbs from inside the same guest page */
-    if ((s->base.tb->pc & TARGET_PAGE_MASK) != (dest & TARGET_PAGE_MASK)) {
-        return false;
-    }
-#endif
-
-    return true;
-}
-
 static inline void gen_goto_tb(DisasContext *s, int n, uint64_t dest)
 {
     const TranslationBlock *tb;
 
     tb = s->base.tb;
-    if (use_goto_tb(s, n, dest)) {
+    if (use_goto_tb(s, dest)) {
         tcg_gen_goto_tb(n);
         gen_a64_set_pc_im(dest);
         tcg_gen_exit_tb(tb, n);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 9e2cca7707..3ac7943d86 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -2511,16 +2511,6 @@ static int disas_dsp_insn(DisasContext *s, uint32_t insn)
     return 1;
 }
 
-static inline bool use_goto_tb(DisasContext *s, target_ulong dest)
-{
-#ifndef CONFIG_USER_ONLY
-    return (s->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) ||
-           ((s->base.pc_next - 1) & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
-}
-
 static void gen_goto_ptr(void)
 {
     tcg_gen_lookup_and_goto_ptr();
-- 
2.25.1

Re: [PATCH 05/26] target/arm: Use translator_use_goto_tb

[Peter Maydell]

On Mon, 21 Jun 2021 at 02:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Put a wrapper in translate.h, which also checks for ss_active.
> The ss_active test was incorrectly missing from the a32 version.

True, but we almost never call gen_goto_tb() when ss_active
(which makes sense because we generate code to raise a singlestep
exception instead). The only cases I think where we do call
gen_goto_tb() are ISB and SB; everything else calls gen_jmp_tb()
or otherwise handles the is_singlestepping() case.

thanks
-- PMM

[PATCH 06/26] target/avr: Use translator_use_goto_tb

[Richard Henderson]

Single stepping is not the only reason not to use goto_tb.
If goto_tb is disallowed, and single-stepping is not enabled,
then use tcg_gen_lookup_and_goto_tb to indirectly chain.

Cc: Michael Rolnik <mrolnik@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/translate.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index c06ce45bc7..8237a03c23 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -1083,14 +1083,17 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
 {
     const TranslationBlock *tb = ctx->base.tb;
 
-    if (!ctx->base.singlestep_enabled) {
+    if (translator_use_goto_tb(&ctx->base, dest)) {
         tcg_gen_goto_tb(n);
         tcg_gen_movi_i32(cpu_pc, dest);
         tcg_gen_exit_tb(tb, n);
     } else {
         tcg_gen_movi_i32(cpu_pc, dest);
-        gen_helper_debug(cpu_env);
-        tcg_gen_exit_tb(NULL, 0);
+        if (ctx->base.singlestep_enabled) {
+            gen_helper_debug(cpu_env);
+        } else {
+            tcg_gen_lookup_and_goto_ptr();
+        }
     }
     ctx->base.is_jmp = DISAS_NORETURN;
 }
-- 
2.25.1

[PATCH 07/26] target/avr: Mark some helpers noreturn

[Richard Henderson]

All of these helpers end with cpu_loop_exit.

Cc: Michael Rolnik <mrolnik@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/helper.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/avr/helper.h b/target/avr/helper.h
index 8e1ae7fda0..4d02e648fa 100644
--- a/target/avr/helper.h
+++ b/target/avr/helper.h
@@ -19,10 +19,10 @@
  */
 
 DEF_HELPER_1(wdr, void, env)
-DEF_HELPER_1(debug, void, env)
-DEF_HELPER_1(break, void, env)
-DEF_HELPER_1(sleep, void, env)
-DEF_HELPER_1(unsupported, void, env)
+DEF_HELPER_1(debug, noreturn, env)
+DEF_HELPER_1(break, noreturn, env)
+DEF_HELPER_1(sleep, noreturn, env)
+DEF_HELPER_1(unsupported, noreturn, env)
 DEF_HELPER_3(outb, void, env, i32, i32)
 DEF_HELPER_2(inb, tl, env, i32)
 DEF_HELPER_3(fullwr, void, env, i32, i32)
-- 
2.25.1

Re: [PATCH 07/26] target/avr: Mark some helpers noreturn

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

Reviewed-by: Michael Rolnik <mrolnik@gmail.com>


On Mon, Jun 21, 2021 at 4:34 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> All of these helpers end with cpu_loop_exit.
>
> Cc: Michael Rolnik <mrolnik@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/helper.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/avr/helper.h b/target/avr/helper.h
> index 8e1ae7fda0..4d02e648fa 100644
> --- a/target/avr/helper.h
> +++ b/target/avr/helper.h
> @@ -19,10 +19,10 @@
>   */
>
>  DEF_HELPER_1(wdr, void, env)
> -DEF_HELPER_1(debug, void, env)
> -DEF_HELPER_1(break, void, env)
> -DEF_HELPER_1(sleep, void, env)
> -DEF_HELPER_1(unsupported, void, env)
> +DEF_HELPER_1(debug, noreturn, env)
> +DEF_HELPER_1(break, noreturn, env)
> +DEF_HELPER_1(sleep, noreturn, env)
> +DEF_HELPER_1(unsupported, noreturn, env)
>  DEF_HELPER_3(outb, void, env, i32, i32)
>  DEF_HELPER_2(inb, tl, env, i32)
>  DEF_HELPER_3(fullwr, void, env, i32, i32)
> --
> 2.25.1
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 1739 bytes --]

Re: [PATCH 07/26] target/avr: Mark some helpers noreturn

[Philippe Mathieu-Daudé]

On 6/21/21 3:34 AM, Richard Henderson wrote:
> All of these helpers end with cpu_loop_exit.
> 
> Cc: Michael Rolnik <mrolnik@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/helper.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 08/26] target/cris: Use translator_use_goto_tb

[Richard Henderson]

The test for singlestepping is done in translator_use_goto_tb,
so we may elide it from cris_tr_tb_stop.

Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/cris/translate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/target/cris/translate.c b/target/cris/translate.c
index 3200819f32..33ad76c0a8 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -517,7 +517,7 @@ static void t_gen_swapr(TCGv d, TCGv s)
 
 static bool use_goto_tb(DisasContext *dc, target_ulong dest)
 {
-    return ((dest ^ dc->base.pc_first) & TARGET_PAGE_MASK) == 0;
+    return translator_use_goto_tb(&dc->base, dest);
 }
 
 static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
@@ -3275,8 +3275,7 @@ static void cris_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
              * Use a conditional branch if either taken or not-taken path
              * can use goto_tb.  If neither can, then treat it as indirect.
              */
-            if (likely(!dc->base.singlestep_enabled)
-                && (use_goto_tb(dc, dc->jmp_pc) || use_goto_tb(dc, npc))) {
+            if (use_goto_tb(dc, dc->jmp_pc) || use_goto_tb(dc, npc)) {
                 TCGLabel *not_taken = gen_new_label();
 
                 tcg_gen_brcondi_tl(TCG_COND_EQ, env_btaken, 0, not_taken);
-- 
2.25.1

[PATCH 09/26] target/hppa: Use translator_use_goto_tb

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/hppa/translate.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 64af1e0d5c..952cfe09a6 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -817,10 +817,7 @@ static bool gen_illegal(DisasContext *ctx)
 
 static bool use_goto_tb(DisasContext *ctx, target_ureg dest)
 {
-    /* Suppress goto_tb for page crossing, IO, or single-steping.  */
-    return !(((ctx->base.pc_first ^ dest) & TARGET_PAGE_MASK)
-             || (tb_cflags(ctx->base.tb) & CF_LAST_IO)
-             || ctx->base.singlestep_enabled);
+    return translator_use_goto_tb(&ctx->base, dest);
 }
 
 /* If the next insn is to be nullified, and it's on the same page,
-- 
2.25.1

[PATCH 10/26] target/i386: Use translator_use_goto_tb

[Richard Henderson]

Just use translator_use_goto_tb directly at the one call site,
rather than maintaining a local wrapper.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/translate.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index a7f5c0c8f2..e47b220ce7 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -2314,21 +2314,11 @@ static inline int insn_const_size(MemOp ot)
     }
 }
 
-static inline bool use_goto_tb(DisasContext *s, target_ulong pc)
-{
-#ifndef CONFIG_USER_ONLY
-    return (pc & TARGET_PAGE_MASK) == (s->base.tb->pc & TARGET_PAGE_MASK) ||
-           (pc & TARGET_PAGE_MASK) == (s->pc_start & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
-}
-
-static inline void gen_goto_tb(DisasContext *s, int tb_num, target_ulong eip)
+static void gen_goto_tb(DisasContext *s, int tb_num, target_ulong eip)
 {
     target_ulong pc = s->cs_base + eip;
 
-    if (use_goto_tb(s, pc))  {
+    if (translator_use_goto_tb(&s->base, pc))  {
         /* jump to same page: we can use a direct jump */
         tcg_gen_goto_tb(tb_num);
         gen_jmp_im(s, eip);
-- 
2.25.1

[PATCH 11/26] target/m68k: Use translator_use_goto_tb

[Richard Henderson]

Just use translator_use_goto_tb directly at the one call site,
rather than maintaining a local wrapper.

Cc: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/m68k/translate.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index f0c5bf9154..05b96fdda7 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -1520,16 +1520,6 @@ static void gen_exit_tb(DisasContext *s)
         }                                                               \
     } while (0)
 
-static inline bool use_goto_tb(DisasContext *s, uint32_t dest)
-{
-#ifndef CONFIG_USER_ONLY
-    return (s->base.pc_first & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)
-        || (s->base.pc_next & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
-}
-
 /* Generate a jump to an immediate address.  */
 static void gen_jmp_tb(DisasContext *s, int n, uint32_t dest)
 {
@@ -1537,7 +1527,7 @@ static void gen_jmp_tb(DisasContext *s, int n, uint32_t dest)
         update_cc_op(s);
         tcg_gen_movi_i32(QREG_PC, dest);
         gen_singlestep_exception(s);
-    } else if (use_goto_tb(s, dest)) {
+    } else if (translator_use_goto_tb(&s->base, dest)) {
         tcg_gen_goto_tb(n);
         tcg_gen_movi_i32(QREG_PC, dest);
         tcg_gen_exit_tb(s->base.tb, n);
-- 
2.25.1

Re: [PATCH 11/26] target/m68k: Use translator_use_goto_tb

[Laurent Vivier]

Le 21/06/2021 à 03:34, Richard Henderson a écrit :
> Just use translator_use_goto_tb directly at the one call site,
> rather than maintaining a local wrapper.
> 
> Cc: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/m68k/translate.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index f0c5bf9154..05b96fdda7 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -1520,16 +1520,6 @@ static void gen_exit_tb(DisasContext *s)
>          }                                                               \
>      } while (0)
>  
> -static inline bool use_goto_tb(DisasContext *s, uint32_t dest)
> -{
> -#ifndef CONFIG_USER_ONLY
> -    return (s->base.pc_first & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)
> -        || (s->base.pc_next & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
> -#else
> -    return true;
> -#endif
> -}
> -
>  /* Generate a jump to an immediate address.  */
>  static void gen_jmp_tb(DisasContext *s, int n, uint32_t dest)
>  {
> @@ -1537,7 +1527,7 @@ static void gen_jmp_tb(DisasContext *s, int n, uint32_t dest)
>          update_cc_op(s);
>          tcg_gen_movi_i32(QREG_PC, dest);
>          gen_singlestep_exception(s);
> -    } else if (use_goto_tb(s, dest)) {
> +    } else if (translator_use_goto_tb(&s->base, dest)) {
>          tcg_gen_goto_tb(n);
>          tcg_gen_movi_i32(QREG_PC, dest);
>          tcg_gen_exit_tb(s->base.tb, n);
> 

Acked-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 12/26] target/microblaze: Use translator_use_goto_tb

[Richard Henderson]

Just use translator_use_goto_tb directly at the one call site,
rather than maintaining a local wrapper.

Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/microblaze/translate.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index c1b13f4c7d..b753f080e7 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -125,15 +125,6 @@ static void gen_raise_hw_excp(DisasContext *dc, uint32_t esr_ec)
     gen_raise_exception_sync(dc, EXCP_HW_EXCP);
 }
 
-static inline bool use_goto_tb(DisasContext *dc, target_ulong dest)
-{
-#ifndef CONFIG_USER_ONLY
-    return (dc->base.pc_first & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
-}
-
 static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
 {
     if (dc->base.singlestep_enabled) {
@@ -141,7 +132,7 @@ static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
         tcg_gen_movi_i32(cpu_pc, dest);
         gen_helper_raise_exception(cpu_env, tmp);
         tcg_temp_free_i32(tmp);
-    } else if (use_goto_tb(dc, dest)) {
+    } else if (translator_use_goto_tb(&dc->base, dest)) {
         tcg_gen_goto_tb(n);
         tcg_gen_movi_i32(cpu_pc, dest);
         tcg_gen_exit_tb(dc->base.tb, n);
-- 
2.25.1

[PATCH 13/26] target/mips: Use translator_use_goto_tb

[Richard Henderson]

Just use translator_use_goto_tb directly at the one call site,
rather than maintaining a local wrapper.

Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/mips/tcg/translate.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 797eba4434..d59986b340 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -5019,22 +5019,9 @@ static void gen_trap(DisasContext *ctx, uint32_t opc,
     tcg_temp_free(t1);
 }
 
-static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
+static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
 {
-    if (unlikely(ctx->base.singlestep_enabled)) {
-        return false;
-    }
-
-#ifndef CONFIG_USER_ONLY
-    return (ctx->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
-}
-
-static inline void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
-{
-    if (use_goto_tb(ctx, dest)) {
+    if (translator_use_goto_tb(&ctx->base, dest)) {
         tcg_gen_goto_tb(n);
         gen_save_pc(dest);
         tcg_gen_exit_tb(ctx->base.tb, n);
-- 
2.25.1

[PATCH 14/26] target/mips: Fix missing else in gen_goto_tb

[Richard Henderson]

Do not emit dead code for the singlestep_enabled case,
after having exited the TB with a debug exception.

Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/mips/tcg/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index d59986b340..99827ee740 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -5030,8 +5030,9 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
         if (ctx->base.singlestep_enabled) {
             save_cpu_state(ctx, 0);
             gen_helper_raise_exception_debug(cpu_env);
+        } else {
+            tcg_gen_lookup_and_goto_ptr();
         }
-        tcg_gen_lookup_and_goto_ptr();
     }
 }
 
-- 
2.25.1

Re: [PATCH 14/26] target/mips: Fix missing else in gen_goto_tb

[Philippe Mathieu-Daudé]

On 6/21/21 3:34 AM, Richard Henderson wrote:
> Do not emit dead code for the singlestep_enabled case,
> after having exited the TB with a debug exception.
> 
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/mips/tcg/translate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 15/26] target/nios2: Use translator_use_goto_tb

[Richard Henderson]

Just use translator_use_goto_tb directly at the one call site,
rather than maintaining a local wrapper.

Cc: Chris Wulff <crwulff@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/nios2/translate.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 276643cee0..2e2c83f3ad 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -150,24 +150,11 @@ static void t_gen_helper_raise_exception(DisasContext *dc,
     dc->base.is_jmp = DISAS_NORETURN;
 }
 
-static bool use_goto_tb(DisasContext *dc, uint32_t dest)
-{
-    if (unlikely(dc->base.singlestep_enabled)) {
-        return false;
-    }
-
-#ifndef CONFIG_USER_ONLY
-    return (dc->base.pc_first & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
-}
-
 static void gen_goto_tb(DisasContext *dc, int n, uint32_t dest)
 {
     const TranslationBlock *tb = dc->base.tb;
 
-    if (use_goto_tb(dc, dest)) {
+    if (translator_use_goto_tb(&dc->base, dest)) {
         tcg_gen_goto_tb(n);
         tcg_gen_movi_tl(cpu_R[R_PC], dest);
         tcg_gen_exit_tb(tb, n);
-- 
2.25.1

[PATCH 16/26] target/openrisc: Use translator_use_goto_tb

[Richard Henderson]

Reorder the cases in openrisc_tr_tb_stop to make this easier to read.

Cc: Stafford Horne <shorne@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/openrisc/translate.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index a9c81f8bd5..2d142d8577 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -1720,16 +1720,17 @@ static void openrisc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
         /* fallthru */
 
     case DISAS_TOO_MANY:
-        if (unlikely(dc->base.singlestep_enabled)) {
-            tcg_gen_movi_tl(cpu_pc, jmp_dest);
-            gen_exception(dc, EXCP_DEBUG);
-        } else if ((dc->base.pc_first ^ jmp_dest) & TARGET_PAGE_MASK) {
-            tcg_gen_movi_tl(cpu_pc, jmp_dest);
-            tcg_gen_lookup_and_goto_ptr();
-        } else {
+        if (translator_use_goto_tb(&dc->base, jmp_dest)) {
             tcg_gen_goto_tb(0);
             tcg_gen_movi_tl(cpu_pc, jmp_dest);
             tcg_gen_exit_tb(dc->base.tb, 0);
+            break;
+        }
+        tcg_gen_movi_tl(cpu_pc, jmp_dest);
+        if (unlikely(dc->base.singlestep_enabled)) {
+            gen_exception(dc, EXCP_DEBUG);
+        } else {
+            tcg_gen_lookup_and_goto_ptr();
         }
         break;
 
-- 
2.25.1

[PATCH 17/26] target/ppc: Use translator_use_goto_tb

[Richard Henderson]

Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/translate.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index f65d1e81ea..0fb09f2301 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4302,15 +4302,7 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
 
 static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
 {
-    if (unlikely(ctx->singlestep_enabled)) {
-        return false;
-    }
-
-#ifndef CONFIG_USER_ONLY
-    return (ctx->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
+    return translator_use_goto_tb(&ctx->base, dest);
 }
 
 static void gen_lookup_and_goto_ptr(DisasContext *ctx)
-- 
2.25.1

RE: [PATCH 17/26] target/ppc: Use translator_use_goto_tb

[Luis Fernando Fujita Pires]

From: Richard Henderson <richard.henderson@linaro.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/ppc/translate.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)

Reviewed-by: Luis Pires <luis.pires@eldorado.org.br>

--
Luis Pires
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[PATCH 18/26] target/riscv: Use translator_use_goto_tb

[Richard Henderson]

Just use translator_use_goto_tb directly at the one call site,
rather than maintaining a local wrapper.

Cc: qemu-riscv@nongnu.org
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/translate.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index c6e8739614..ecd3764338 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -168,29 +168,11 @@ static void gen_exception_inst_addr_mis(DisasContext *ctx)
     generate_exception_mtval(ctx, RISCV_EXCP_INST_ADDR_MIS);
 }
 
-static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
-{
-    if (unlikely(ctx->base.singlestep_enabled)) {
-        return false;
-    }
-
-#ifndef CONFIG_USER_ONLY
-    return (ctx->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
-}
-
 static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
 {
-    if (use_goto_tb(ctx, dest)) {
-        /* chaining is only allowed when the jump is to the same page */
+    if (translator_use_goto_tb(&ctx->base, dest)) {
         tcg_gen_goto_tb(n);
         tcg_gen_movi_tl(cpu_pc, dest);
-
-        /* No need to check for single stepping here as use_goto_tb() will
-         * return false in case of single stepping.
-         */
         tcg_gen_exit_tb(ctx->base.tb, n);
     } else {
         tcg_gen_movi_tl(cpu_pc, dest);
-- 
2.25.1

Re: [PATCH 18/26] target/riscv: Use translator_use_goto_tb

[Alistair Francis]

On Mon, Jun 21, 2021 at 11:47 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Just use translator_use_goto_tb directly at the one call site,
> rather than maintaining a local wrapper.
>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/translate.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index c6e8739614..ecd3764338 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -168,29 +168,11 @@ static void gen_exception_inst_addr_mis(DisasContext *ctx)
>      generate_exception_mtval(ctx, RISCV_EXCP_INST_ADDR_MIS);
>  }
>
> -static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
> -{
> -    if (unlikely(ctx->base.singlestep_enabled)) {
> -        return false;
> -    }
> -
> -#ifndef CONFIG_USER_ONLY
> -    return (ctx->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
> -#else
> -    return true;
> -#endif
> -}
> -
>  static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>  {
> -    if (use_goto_tb(ctx, dest)) {
> -        /* chaining is only allowed when the jump is to the same page */
> +    if (translator_use_goto_tb(&ctx->base, dest)) {
>          tcg_gen_goto_tb(n);
>          tcg_gen_movi_tl(cpu_pc, dest);
> -
> -        /* No need to check for single stepping here as use_goto_tb() will
> -         * return false in case of single stepping.
> -         */
>          tcg_gen_exit_tb(ctx->base.tb, n);
>      } else {
>          tcg_gen_movi_tl(cpu_pc, dest);
> --
> 2.25.1
>
>

Re: [PATCH 18/26] target/riscv: Use translator_use_goto_tb

[Alistair Francis]

On Mon, Jun 21, 2021 at 11:47 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Just use translator_use_goto_tb directly at the one call site,
> rather than maintaining a local wrapper.
>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/translate.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index c6e8739614..ecd3764338 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -168,29 +168,11 @@ static void gen_exception_inst_addr_mis(DisasContext *ctx)
>      generate_exception_mtval(ctx, RISCV_EXCP_INST_ADDR_MIS);
>  }
>
> -static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
> -{
> -    if (unlikely(ctx->base.singlestep_enabled)) {
> -        return false;
> -    }
> -
> -#ifndef CONFIG_USER_ONLY
> -    return (ctx->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
> -#else
> -    return true;
> -#endif
> -}
> -
>  static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>  {
> -    if (use_goto_tb(ctx, dest)) {
> -        /* chaining is only allowed when the jump is to the same page */
> +    if (translator_use_goto_tb(&ctx->base, dest)) {
>          tcg_gen_goto_tb(n);
>          tcg_gen_movi_tl(cpu_pc, dest);
> -
> -        /* No need to check for single stepping here as use_goto_tb() will
> -         * return false in case of single stepping.
> -         */
>          tcg_gen_exit_tb(ctx->base.tb, n);
>      } else {
>          tcg_gen_movi_tl(cpu_pc, dest);
> --
> 2.25.1
>
>

[PATCH 19/26] target/rx: Use translator_use_goto_tb

[Richard Henderson]

Just use translator_use_goto_tb directly at the one call site,
rather than maintaining a local wrapper.

Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/rx/translate.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/target/rx/translate.c b/target/rx/translate.c
index 9ea941c630..2443406de5 100644
--- a/target/rx/translate.c
+++ b/target/rx/translate.c
@@ -143,18 +143,9 @@ void rx_cpu_dump_state(CPUState *cs, FILE *f, int flags)
     }
 }
 
-static bool use_goto_tb(DisasContext *dc, target_ulong dest)
-{
-    if (unlikely(dc->base.singlestep_enabled)) {
-        return false;
-    } else {
-        return true;
-    }
-}
-
 static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
 {
-    if (use_goto_tb(dc, dest)) {
+    if (translator_use_goto_tb(&dc->base, dest)) {
         tcg_gen_goto_tb(n);
         tcg_gen_movi_i32(cpu_pc, dest);
         tcg_gen_exit_tb(dc->base.tb, n);
-- 
2.25.1

[PATCH 20/26] target/s390x: Use translator_use_goto_tb

[Richard Henderson]

Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/translate.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index e243624d2a..4bb5d82a37 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -697,12 +697,7 @@ static bool use_goto_tb(DisasContext *s, uint64_t dest)
     if (unlikely(use_exit_tb(s))) {
         return false;
     }
-#ifndef CONFIG_USER_ONLY
-    return (dest & TARGET_PAGE_MASK) == (s->base.tb->pc & TARGET_PAGE_MASK) ||
-           (dest & TARGET_PAGE_MASK) == (s->base.pc_next & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
+    return translator_use_goto_tb(&s->base, dest);
 }
 
 static void account_noninline_branch(DisasContext *s, int cc_op)
-- 
2.25.1

[PATCH 21/26] target/s390x: Remove use_exit_tb

[Richard Henderson]

We have not needed to end a TB for I/O since ba3e7926691
("icount: clean up cpu_can_io at the entry to the block").

In use_goto_tb, the check for singlestep_enabled is in the
generic translator_use_goto_tb.  In s390x_tr_tb_stop, the
check for singlestep_enabled is in the preceeding do_debug test.

Which leaves only FLAG_MASK_PER: fold that test alone into
the two callers of use_exit tb.

Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/translate.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 4bb5d82a37..de7d846461 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -685,16 +685,9 @@ static void gen_op_calc_cc(DisasContext *s)
     set_cc_static(s);
 }
 
-static bool use_exit_tb(DisasContext *s)
-{
-    return s->base.singlestep_enabled ||
-            (tb_cflags(s->base.tb) & CF_LAST_IO) ||
-            (s->base.tb->flags & FLAG_MASK_PER);
-}
-
 static bool use_goto_tb(DisasContext *s, uint64_t dest)
 {
-    if (unlikely(use_exit_tb(s))) {
+    if (unlikely(s->base.tb->flags & FLAG_MASK_PER)) {
         return false;
     }
     return translator_use_goto_tb(&s->base, dest);
@@ -6634,7 +6627,7 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
         /* Exit the TB, either by raising a debug exception or by return.  */
         if (dc->do_debug) {
             gen_exception(EXCP_DEBUG);
-        } else if (use_exit_tb(dc) ||
+        } else if ((dc->base.tb->flags & FLAG_MASK_PER) ||
                    dc->base.is_jmp == DISAS_PC_STALE_NOCHAIN) {
             tcg_gen_exit_tb(NULL, 0);
         } else {
-- 
2.25.1

[PATCH 22/26] target/sh4: Use translator_use_goto_tb

[Richard Henderson]

Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/sh4/translate.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index 9312790623..9ac9bc722e 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -225,17 +225,12 @@ static inline bool use_exit_tb(DisasContext *ctx)
     return (ctx->tbflags & GUSA_EXCLUSIVE) != 0;
 }
 
-static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
+static bool use_goto_tb(DisasContext *ctx, target_ulong dest)
 {
-    /* Use a direct jump if in same page and singlestep not enabled */
-    if (unlikely(ctx->base.singlestep_enabled || use_exit_tb(ctx))) {
+    if (use_exit_tb(ctx)) {
         return false;
     }
-#ifndef CONFIG_USER_ONLY
-    return (ctx->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
+    return translator_use_goto_tb(&ctx->base, dest);
 }
 
 static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
-- 
2.25.1

[PATCH 23/26] target/sparc: Use translator_use_goto_tb

[Richard Henderson]

Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/sparc/translate.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 4bfa3179f8..fb0c242606 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -339,23 +339,14 @@ static inline TCGv gen_dest_gpr(DisasContext *dc, int reg)
     }
 }
 
-static inline bool use_goto_tb(DisasContext *s, target_ulong pc,
-                               target_ulong npc)
+static bool use_goto_tb(DisasContext *s, target_ulong pc, target_ulong npc)
 {
-    if (unlikely(s->base.singlestep_enabled || singlestep)) {
-        return false;
-    }
-
-#ifndef CONFIG_USER_ONLY
-    return (pc & TARGET_PAGE_MASK) == (s->base.tb->pc & TARGET_PAGE_MASK) &&
-           (npc & TARGET_PAGE_MASK) == (s->base.tb->pc & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
+    return translator_use_goto_tb(&s->base, pc) &&
+           translator_use_goto_tb(&s->base, npc);
 }
 
-static inline void gen_goto_tb(DisasContext *s, int tb_num,
-                               target_ulong pc, target_ulong npc)
+static void gen_goto_tb(DisasContext *s, int tb_num,
+                        target_ulong pc, target_ulong npc)
 {
     if (use_goto_tb(s, pc, npc))  {
         /* jump to same page: we can use a direct jump */
-- 
2.25.1

Re: [PATCH 23/26] target/sparc: Use translator_use_goto_tb

[Mark Cave-Ayland]

On 21/06/2021 02:34, Richard Henderson wrote:

> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/sparc/translate.c | 19 +++++--------------
>   1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/target/sparc/translate.c b/target/sparc/translate.c
> index 4bfa3179f8..fb0c242606 100644
> --- a/target/sparc/translate.c
> +++ b/target/sparc/translate.c
> @@ -339,23 +339,14 @@ static inline TCGv gen_dest_gpr(DisasContext *dc, int reg)
>       }
>   }
>   
> -static inline bool use_goto_tb(DisasContext *s, target_ulong pc,
> -                               target_ulong npc)
> +static bool use_goto_tb(DisasContext *s, target_ulong pc, target_ulong npc)
>   {
> -    if (unlikely(s->base.singlestep_enabled || singlestep)) {
> -        return false;
> -    }
> -
> -#ifndef CONFIG_USER_ONLY
> -    return (pc & TARGET_PAGE_MASK) == (s->base.tb->pc & TARGET_PAGE_MASK) &&
> -           (npc & TARGET_PAGE_MASK) == (s->base.tb->pc & TARGET_PAGE_MASK);
> -#else
> -    return true;
> -#endif
> +    return translator_use_goto_tb(&s->base, pc) &&
> +           translator_use_goto_tb(&s->base, npc);
>   }
>   
> -static inline void gen_goto_tb(DisasContext *s, int tb_num,
> -                               target_ulong pc, target_ulong npc)
> +static void gen_goto_tb(DisasContext *s, int tb_num,
> +                        target_ulong pc, target_ulong npc)
>   {
>       if (use_goto_tb(s, pc, npc))  {
>           /* jump to same page: we can use a direct jump */

Is it still worth keeping the unlikely() hint around the single-step check? I see it 
isn't included in your translator_use_goto_tb() implementation in patch 1.

I'll trust that you know better than me the effect of removing the inline, so:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 24/26] target/tricore: Use translator_use_goto_tb

[Richard Henderson]

Just use translator_use_goto_tb directly at the one call site,
rather than maintaining a local wrapper.

Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/tricore/translate.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 2a814263de..09465ea013 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -3225,19 +3225,6 @@ static inline void gen_save_pc(target_ulong pc)
     tcg_gen_movi_tl(cpu_PC, pc);
 }
 
-static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
-{
-    if (unlikely(ctx->base.singlestep_enabled)) {
-        return false;
-    }
-
-#ifndef CONFIG_USER_ONLY
-    return (ctx->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
-#else
-    return true;
-#endif
-}
-
 static void generate_qemu_excp(DisasContext *ctx, int excp)
 {
     TCGv_i32 tmp = tcg_const_i32(excp);
@@ -3246,9 +3233,9 @@ static void generate_qemu_excp(DisasContext *ctx, int excp)
     tcg_temp_free(tmp);
 }
 
-static inline void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
+static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
 {
-    if (use_goto_tb(ctx, dest)) {
+    if (translator_use_goto_tb(&ctx->base, dest)) {
         tcg_gen_goto_tb(n);
         gen_save_pc(dest);
         tcg_gen_exit_tb(ctx->base.tb, n);
-- 
2.25.1

Re: [PATCH 24/26] target/tricore: Use translator_use_goto_tb

[Bastian Koppelmann]

On Sun, Jun 20, 2021 at 06:34:37PM -0700, Richard Henderson wrote:
> Just use translator_use_goto_tb directly at the one call site,
> rather than maintaining a local wrapper.
> 
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/tricore/translate.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)

Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

Cheers,
Bastian

[PATCH 25/26] target/tricore: Use tcg_gen_lookup_and_goto_ptr

[Richard Henderson]

The non-single-step case of gen_goto_tb may use
tcg_gen_lookup_and_goto_ptr to indirectly chain.

Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/tricore/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 09465ea013..865020754d 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -3243,8 +3243,9 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
         gen_save_pc(dest);
         if (ctx->base.singlestep_enabled) {
             generate_qemu_excp(ctx, EXCP_DEBUG);
+        } else {
+            tcg_gen_lookup_and_goto_ptr();
         }
-        tcg_gen_exit_tb(NULL, 0);
     }
 }
 
-- 
2.25.1

Re: [PATCH 25/26] target/tricore: Use tcg_gen_lookup_and_goto_ptr

[Bastian Koppelmann]

On Sun, Jun 20, 2021 at 06:34:38PM -0700, Richard Henderson wrote:
> The non-single-step case of gen_goto_tb may use
> tcg_gen_lookup_and_goto_ptr to indirectly chain.
> 
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/tricore/translate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

Cheers,
Bastian

[PATCH 26/26] target/xtensa: Use translator_use_goto_tb

[Richard Henderson]

Cc: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/xtensa/translate.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 14028d307d..ac42f5efdc 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -406,11 +406,7 @@ static void gen_jump(DisasContext *dc, TCGv dest)
 
 static int adjust_jump_slot(DisasContext *dc, uint32_t dest, int slot)
 {
-    if (((dc->base.pc_first ^ dest) & TARGET_PAGE_MASK) != 0) {
-        return -1;
-    } else {
-        return slot;
-    }
+    return translator_use_goto_tb(&dc->base, dest) ? slot : -1;
 }
 
 static void gen_jumpi(DisasContext *dc, uint32_t dest, int slot)
-- 
2.25.1

Re: [PATCH 26/26] target/xtensa: Use translator_use_goto_tb

[Max Filippov]

On Sun, Jun 20, 2021 at 6:34 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/xtensa/translate.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Max Filippov <jcmvbkbc@gmail.com>


--
Thanks.
-- Max