Re: [PATCH v2 18/28] target/openrisc: Use translator_use_goto_tb

From: Stafford Horne <shorne@gmail.com>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH v2 18/28] target/openrisc: Use translator_use_goto_tb
Date: Thu, 8 Jul 2021 05:59:32 +0900	[thread overview]
Message-ID: <YOYVtKsXCwiNvcXp@antec> (raw)
In-Reply-To: <20210630183226.3290849-19-richard.henderson@linaro.org>

On Wed, Jun 30, 2021 at 11:32:16AM -0700, Richard Henderson wrote:
> Reorder the cases in openrisc_tr_tb_stop to make this easier to read.

Hi Richard,

For me the description is a bit misleading.  I don't see it as a simple
reorder for making things easier to read, because we need to understand
what is inside of translator_use_goto_tb now.

From the patch basically translator_use_goto_tb indicates that a jump is in
the same page and we are not single stepping.

The old code was:

  If single step
   DEBUG
  else if not same page
   tcg_gen_lookup_and_goto_ptr()
  else *same page*
   jump same page

Now:

  If translator_use_goto_tb() (same page & not single step)
    jump same page

  If single step
    DEBUG
  else *not same page*
    tcg_gen_lookup_and_goto_ptr()

It would be a bit easier to understand if the description was something like,
Reorder the control statements to allow using the page boundary check from
translator_use_goto_tb().

That said it looks correct so:

Reviewed-by: Stafford Horne <shorne@gmail.com>

> 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
> 