Re: [PATCH v2 03/29] tcg: Re-order tcg_region_init vs tcg_prologue_init

From: Roman Bolshakov <r.bolshakov@yadro.com>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-devel@nongnu.org, j@getutm.app
Subject: Re: [PATCH v2 03/29] tcg: Re-order tcg_region_init vs tcg_prologue_init
Date: Tue, 16 Mar 2021 02:37:36 +0300	[thread overview]
Message-ID: <YE/vwNCE7EEukXll@SPB-NB-133.local> (raw)
In-Reply-To: <20210314212724.1917075-4-richard.henderson@linaro.org>

On Sun, Mar 14, 2021 at 03:26:58PM -0600, Richard Henderson wrote:
> Instead of delaying tcg_region_init until after tcg_prologue_init
> is complete, do tcg_region_init first and let tcg_prologue_init
> shrink the first region by the size of the generated prologue.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  accel/tcg/tcg-all.c       | 11 ---------
>  accel/tcg/translate-all.c |  3 +++
>  bsd-user/main.c           |  1 -
>  linux-user/main.c         |  1 -
>  tcg/tcg.c                 | 52 ++++++++++++++-------------------------
>  5 files changed, 22 insertions(+), 46 deletions(-)
> 
> diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
> index e378c2db73..f132033999 100644
> --- a/accel/tcg/tcg-all.c
> +++ b/accel/tcg/tcg-all.c
> @@ -111,17 +111,6 @@ static int tcg_init(MachineState *ms)
>  
>      tcg_exec_init(s->tb_size * 1024 * 1024, s->splitwx_enabled);
>      mttcg_enabled = s->mttcg_enabled;
> -
> -    /*
> -     * Initialize TCG regions only for softmmu.
> -     *
> -     * This needs to be done later for user mode, because the prologue
> -     * generation needs to be delayed so that GUEST_BASE is already set.
> -     */
> -#ifndef CONFIG_USER_ONLY
> -    tcg_region_init();

Note that tcg_region_init() invokes tcg_n_regions() that depends on
qemu_tcg_mttcg_enabled() that evaluates mttcg_enabled. Likely you need
to move "mttcg_enabled = s->mttcg_enabled;" before tcg_exec_init() to
keep existing behaviour.

> -#endif /* !CONFIG_USER_ONLY */
> -
>      return 0;
>  }
>  
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index f32df8b240..b9057567f4 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1339,6 +1339,9 @@ void tcg_exec_init(unsigned long tb_size, int splitwx)
>                                 splitwx, &error_fatal);
>      assert(ok);
>  
> +    /* TODO: allocating regions is hand-in-glove with code_gen_buffer. */
> +    tcg_region_init();
> +
>  #if defined(CONFIG_SOFTMMU)
>      /* There's no guest base to take into account, so go ahead and
>         initialize the prologue now.  */
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 798aba512c..3669d2b89e 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -994,7 +994,6 @@ int main(int argc, char **argv)
>         generating the prologue until now so that the prologue can take
>         the real value of GUEST_BASE into account.  */
>      tcg_prologue_init(tcg_ctx);
> -    tcg_region_init();
>  
>      /* build Task State */
>      memset(ts, 0, sizeof(TaskState));
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 4f4746dce8..1bc48ca954 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -850,7 +850,6 @@ int main(int argc, char **argv, char **envp)
>         generating the prologue until now so that the prologue can take
>         the real value of GUEST_BASE into account.  */
>      tcg_prologue_init(tcg_ctx);
> -    tcg_region_init();
>  
>      target_cpu_copy_regs(env, regs);
>  
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 2991112829..0a2e5710de 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -1204,32 +1204,18 @@ TranslationBlock *tcg_tb_alloc(TCGContext *s)
>  
>  void tcg_prologue_init(TCGContext *s)
>  {
> -    size_t prologue_size, total_size;
> -    void *buf0, *buf1;
> +    size_t prologue_size;
>  
>      /* Put the prologue at the beginning of code_gen_buffer.  */
> -    buf0 = s->code_gen_buffer;
> -    total_size = s->code_gen_buffer_size;
> -    s->code_ptr = buf0;
> -    s->code_buf = buf0;
> +    tcg_region_assign(s, 0);
> +    s->code_ptr = s->code_gen_ptr;
> +    s->code_buf = s->code_gen_ptr;

Pardon me for asking a naive question, what's the difference between
s->code_buf and s->code_gen_buf and, respectively, s->code_ptr and
s->code_gen_ptr?

Thanks,
Roman

>      s->data_gen_ptr = NULL;
>  
> -    /*
> -     * The region trees are not yet configured, but tcg_splitwx_to_rx
> -     * needs the bounds for an assert.
> -     */
> -    region.start = buf0;
> -    region.end = buf0 + total_size;
> -
>  #ifndef CONFIG_TCG_INTERPRETER
> -    tcg_qemu_tb_exec = (tcg_prologue_fn *)tcg_splitwx_to_rx(buf0);
> +    tcg_qemu_tb_exec = (tcg_prologue_fn *)tcg_splitwx_to_rx(s->code_ptr);
>  #endif
>  
> -    /* Compute a high-water mark, at which we voluntarily flush the buffer
> -       and start over.  The size here is arbitrary, significantly larger
> -       than we expect the code generation for any one opcode to require.  */
> -    s->code_gen_highwater = s->code_gen_buffer + (total_size - TCG_HIGHWATER);
> -
>  #ifdef TCG_TARGET_NEED_POOL_LABELS
>      s->pool_labels = NULL;
>  #endif
> @@ -1246,32 +1232,32 @@ void tcg_prologue_init(TCGContext *s)
>      }
>  #endif
>  
> -    buf1 = s->code_ptr;
> +    prologue_size = tcg_current_code_size(s);
> +
>  #ifndef CONFIG_TCG_INTERPRETER
> -    flush_idcache_range((uintptr_t)tcg_splitwx_to_rx(buf0), (uintptr_t)buf0,
> -                        tcg_ptr_byte_diff(buf1, buf0));
> +    flush_idcache_range((uintptr_t)tcg_splitwx_to_rx(s->code_buf),
> +                        (uintptr_t)s->code_buf, prologue_size);
>  #endif
>  
> -    /* Deduct the prologue from the buffer.  */
> -    prologue_size = tcg_current_code_size(s);
> -    s->code_gen_ptr = buf1;
> -    s->code_gen_buffer = buf1;
> -    s->code_buf = buf1;
> -    total_size -= prologue_size;
> -    s->code_gen_buffer_size = total_size;
> +    /* Deduct the prologue from the first region.  */
> +    region.start = s->code_ptr;
>  
> -    tcg_register_jit(tcg_splitwx_to_rx(s->code_gen_buffer), total_size);
> +    /* Recompute boundaries of the first region. */
> +    tcg_region_assign(s, 0);
> +
> +    tcg_register_jit(tcg_splitwx_to_rx(region.start),
> +                     region.end - region.start);
>  
>  #ifdef DEBUG_DISAS
>      if (qemu_loglevel_mask(CPU_LOG_TB_OUT_ASM)) {
>          FILE *logfile = qemu_log_lock();
>          qemu_log("PROLOGUE: [size=%zu]\n", prologue_size);
>          if (s->data_gen_ptr) {
> -            size_t code_size = s->data_gen_ptr - buf0;
> +            size_t code_size = s->data_gen_ptr - s->code_gen_ptr;
>              size_t data_size = prologue_size - code_size;
>              size_t i;
>  
> -            log_disas(buf0, code_size);
> +            log_disas(s->code_gen_ptr, code_size);
>  
>              for (i = 0; i < data_size; i += sizeof(tcg_target_ulong)) {
>                  if (sizeof(tcg_target_ulong) == 8) {
> @@ -1285,7 +1271,7 @@ void tcg_prologue_init(TCGContext *s)
>                  }
>              }
>          } else {
> -            log_disas(buf0, prologue_size);
> +            log_disas(s->code_gen_ptr, prologue_size);
>          }
>          qemu_log("\n");
>          qemu_log_flush();
> -- 
> 2.25.1
> 