Re: [Qemu-devel] [PATCH] ARM - Remove fixed map code buffer restriction

[Peter Maydell]

CC'ing Andrzej, who is the tcg/arm maintainer.

On 12 December 2011 15:37, Dr. David Alan Gilbert
<address@hidden> wrote:
> On ARM, don't map the code buffer at a fixed location, and fix up the
> call/goto tcg routines to let it do long jumps.
>
> Mapping the code buffer at a fixed address could sometimes result in it being
> mapped over the top of the heap with pretty random results.
>
> This diff is against v1.0.

Patches should always be against master, although we're still
close enough to 1.0 that this will probably apply anyway.

Comments like "This diff is against v1.0." go below the '---' so they
don't appear in the final commit log.

Code generally looks OK to me.

> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  exec.c               |    4 +---
>  tcg/arm/tcg-target.c |   31 ++++++++++++-------------------
>  2 files changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 6b92198..ef83da1 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -497,9 +497,7 @@ static void code_gen_alloc(unsigned long tb_size)
>         if (code_gen_buffer_size > (512 * 1024 * 1024))
>             code_gen_buffer_size = (512 * 1024 * 1024);
>  #elif defined(__arm__)
> -        /* Map the buffer below 32M, so we can use direct calls and branches 
> */
> -        flags |= MAP_FIXED;
> -        start = (void *) 0x01000000UL;
> +        /* Keep the buffer no bigger than 16GB to branch between blocks */
>         if (code_gen_buffer_size > 16 * 1024 * 1024)
>             code_gen_buffer_size = 16 * 1024 * 1024;
>  #elif defined(__s390x__)
> diff --git a/tcg/arm/tcg-target.c b/tcg/arm/tcg-target.c
> index e05a64f..730d913 100644
> --- a/tcg/arm/tcg-target.c
> +++ b/tcg/arm/tcg-target.c
> @@ -842,6 +842,12 @@ static inline void tcg_out_st8(TCGContext *s, int cond,
>         tcg_out_st8_12(s, cond, rd, rn, offset);
>  }
>
> +/* The _goto case is normally between TBs within the same code buffer,
> +   and with the code buffer limited to 16GB we shouldn't need the long
> +   case.
> +
> +   .... except to the prologue that is in its own buffer.
> +     */
>  static inline void tcg_out_goto(TCGContext *s, int cond, uint32_t addr)
>  {
>     int32_t val;
> @@ -855,22 +861,20 @@ static inline void tcg_out_goto(TCGContext *s, int 
> cond, uint32_t addr)
>     if (val - 8 < 0x01fffffd && val - 8 > -0x01fffffd)
>         tcg_out_b(s, cond, val);
>     else {
> -#if 1
> -        tcg_abort();
> -#else
>         if (cond == COND_AL) {
>             tcg_out_ld32_12(s, COND_AL, TCG_REG_PC, TCG_REG_PC, -4);
> -            tcg_out32(s, addr); /* XXX: This is l->u.value, can we use it? */
> +            tcg_out32(s, addr);

I see these XXXs have been there since the ARM tcg target was
introduced. Does anybody know what they were referring to? Andrzej?

>         } else {
>             tcg_out_movi32(s, cond, TCG_REG_R8, val - 8);
>             tcg_out_dat_reg(s, cond, ARITH_ADD,
>                             TCG_REG_PC, TCG_REG_PC,
>                             TCG_REG_R8, SHIFT_IMM_LSL(0));
>         }
> -#endif
>     }
>  }
>
> +/* The call case is mostly used for helpers - so it's not unreasonable
> +   for them to be beyond branch range */
>  static inline void tcg_out_call(TCGContext *s, uint32_t addr)
>  {
>     int32_t val;
> @@ -887,20 +891,9 @@ static inline void tcg_out_call(TCGContext *s, uint32_t 
> addr)
>             tcg_out_bl(s, COND_AL, val);
>         }
>     } else {
> -#if 1
> -        tcg_abort();
> -#else
> -        if (cond == COND_AL) {
> -            tcg_out_dat_imm(s, cond, ARITH_ADD, TCG_REG_R14, TCG_REG_PC, 4);
> -            tcg_out_ld32_12(s, COND_AL, TCG_REG_PC, TCG_REG_PC, -4);
> -            tcg_out32(s, addr); /* XXX: This is l->u.value, can we use it? */
> -        } else {
> -            tcg_out_movi32(s, cond, TCG_REG_R9, addr);
> -            tcg_out_dat_reg(s, cond, ARITH_MOV, TCG_REG_R14, 0,
> -                            TCG_REG_PC, SHIFT_IMM_LSL(0));
> -            tcg_out_bx(s, cond, TCG_REG_R9);
> -        }
> -#endif
> +        tcg_out_dat_imm(s, COND_AL, ARITH_ADD, TCG_REG_R14, TCG_REG_PC, 4);
> +        tcg_out_ld32_12(s, COND_AL, TCG_REG_PC, TCG_REG_PC, -4);
> +        tcg_out32(s, addr);
>     }
>  }
>

-- PMM