Re: [Qemu-devel] [PATCH v3 25/25] tcg: Check for overflow via highwater mark

[Peter Maydell]

On 22 September 2015 at 13:25, Richard Henderson <address@hidden> wrote:
> We currently pre-compute an worst case code size for any TB, which
> works out to be 122kB.  Since the average TB size is near 1kB, this
> wastes quite a lot of storage.
>
> Instead, check for overflow in between generating code for each opcode.
> The overhead of the check isn't measurable and wastage is minimized.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  include/exec/exec-all.h |  6 ------
>  tcg/tcg.c               | 16 ++++++++++++----
>  tcg/tcg.h               |  6 ++++--
>  translate-all.c         | 31 ++++++++++++++++++++++++++-----
>  4 files changed, 42 insertions(+), 17 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 6871e78..71c9d85 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -62,12 +62,6 @@ typedef struct TranslationBlock TranslationBlock;
>  #define OPC_BUF_SIZE 640
>  #define OPC_MAX_SIZE (OPC_BUF_SIZE - MAX_OP_PER_INSTR)
>
> -/* Maximum size a TCG op can expand to.  This is complicated because a
> -   single op may require several host instructions and register reloads.
> -   For now take a wild guess at 192 bytes, which should allow at least
> -   a couple of fixup instructions per argument.  */
> -#define TCG_MAX_OP_SIZE 192
> -
>  #define OPPARAM_BUF_SIZE (OPC_BUF_SIZE * MAX_OPC_PARAM)
>
>  #include "qemu/log.h"
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index db4032a..750b977 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -375,11 +375,12 @@ void tcg_prologue_init(TCGContext *s)
>      /* Deduct the prologue from the buffer.  */
>      prologue_size = tcg_current_code_size(s);
>      s->code_gen_ptr = s->code_gen_buffer = s->code_buf = s->code_ptr;
> -
> -    /* Compute a high-water mark, at which we voluntarily flush the
> -       buffer and start over.  */
>      total_size = s->code_gen_buffer_size -= prologue_size;
> -    s->code_gen_buffer_max_size = total_size - TCG_MAX_OP_SIZE * 
> OPC_BUF_SIZE;
> +
> +    /* 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 - 1024);
>
>      tcg_register_jit(s->code_gen_buffer, total_size);
>
> @@ -2430,6 +2431,13 @@ int tcg_gen_code(TCGContext *s, tcg_insn_unit 
> *gen_code_buf)
>  #ifndef NDEBUG
>          check_regs(s);
>  #endif
> +        /* Test for (pending) buffer overflow.  The assumption is that any
> +           one operation beginning below the high water mark cannot overrun
> +           the buffer completely.  Thus we can test for overfow after
> +           generating code without having to check during generation.  */

"overflow"


> +        if (unlikely(s->code_gen_ptr > s->code_gen_highwater)) {
> +            return -1;
> +        }
>      }
>      tcg_debug_assert(num_insns >= 0);
>      s->gen_insn_end_off[num_insns] = tcg_current_code_size(s);
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index 5fbbd15..be95b98 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -559,10 +559,12 @@ struct TCGContext {
>      void *code_gen_prologue;
>      void *code_gen_buffer;
>      size_t code_gen_buffer_size;
> -    /* threshold to flush the translated code buffer */
> -    size_t code_gen_buffer_max_size;
>      void *code_gen_ptr;
>
> +    /* Threshold to flush the translated code buffer, and where to go
> +       upon overflow.  */
> +    void *code_gen_highwater;

I don't understand what the "and where to go upon overflow" part
of this comment means. Can you elaborate?

> +
>      TBContext tb_ctx;
>
>      /* The TCGBackendData structure is private to tcg-target.c.  */
> diff --git a/translate-all.c b/translate-all.c
> index 0049927..5ad0a61 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -222,6 +222,7 @@ static target_long decode_sleb128(uint8_t **pp)
>
>  static int encode_search(TranslationBlock *tb, uint8_t *block)
>  {
> +    uint8_t *highwater = tcg_ctx.code_gen_highwater;
>      uint8_t *p = block;
>      int i, j, n;
>
> @@ -240,6 +241,14 @@ static int encode_search(TranslationBlock *tb, uint8_t 
> *block)
>          }
>          prev = (i == 0 ? 0 : tcg_ctx.gen_insn_end_off[i - 1]);
>          p = encode_sleb128(p, tcg_ctx.gen_insn_end_off[i] - prev);
> +
> +        /* Test for (pending) buffer overflow.  The assumption is that any
> +           one row beginning below the high water mark cannot overrun
> +           the buffer completely.  Thus we can test for overfow after
> +           encoding a row without having to check during encoding.  */

"overflow"

> +        if (unlikely(p > highwater)) {
> +            return -1;
> +        }
>      }
>
>      return p - block;
> @@ -756,9 +765,7 @@ static TranslationBlock *tb_alloc(target_ulong pc)
>  {
>      TranslationBlock *tb;
>
> -    if (tcg_ctx.tb_ctx.nb_tbs >= tcg_ctx.code_gen_max_blocks ||
> -        (tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer) >=
> -         tcg_ctx.code_gen_buffer_max_size) {
> +    if (tcg_ctx.tb_ctx.nb_tbs >= tcg_ctx.code_gen_max_blocks) {
>          return NULL;
>      }
>      tb = &tcg_ctx.tb_ctx.tbs[tcg_ctx.tb_ctx.nb_tbs++];
> @@ -1063,12 +1070,15 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>      if (use_icount) {
>          cflags |= CF_USE_ICOUNT;
>      }
> +
>      tb = tb_alloc(pc);
> -    if (!tb) {
> +    if (unlikely(!tb)) {
> + buffer_overflow:
>          /* flush must be done */
>          tb_flush(cpu);
>          /* cannot fail at this point */
>          tb = tb_alloc(pc);
> +        assert(tb != NULL);
>          /* Don't forget to invalidate previous TB info.  */
>          tcg_ctx.tb_ctx.tb_invalidated_flag = 1;
>      }
> @@ -1109,8 +1119,19 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>      tcg_ctx.code_time -= profile_getclock();
>  #endif
>
> +    /* ??? Overflow could be handled better here.  In particular, we
> +       don't need to re-do gen_intermediate_code, nor should we re-do
> +       the tcg optimization currently hidden inside tcg_gen_code.  All
> +       that should be required is to flush the TBs, allocate a new TB,
> +       re-initialize it per above, and re-do the actual code generation.  */
>      gen_code_size = tcg_gen_code(&tcg_ctx, gen_code_buf);
> +    if (unlikely(gen_code_size < 0)) {
> +        goto buffer_overflow;
> +    }
>      search_size = encode_search(tb, (void *)gen_code_buf + gen_code_size);
> +    if (unlikely(search_size < 0)) {
> +        goto buffer_overflow;
> +    }
>
>  #ifdef CONFIG_PROFILER
>      tcg_ctx.code_time += profile_getclock();
> @@ -1681,7 +1702,7 @@ void dump_exec_info(FILE *f, fprintf_function 
> cpu_fprintf)
>      cpu_fprintf(f, "Translation buffer state:\n");
>      cpu_fprintf(f, "gen code size       %td/%zd\n",
>                  tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer,
> -                tcg_ctx.code_gen_buffer_max_size);
> +                tcg_ctx.code_gen_highwater - tcg_ctx.code_gen_buffer);
>      cpu_fprintf(f, "TB count            %d/%d\n",
>              tcg_ctx.tb_ctx.nb_tbs, tcg_ctx.code_gen_max_blocks);
>      cpu_fprintf(f, "TB avg target size  %d max=%d bytes\n",
> --
> 2.4.3
>

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM