qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] target/m68k: add a mechanism to automatically f


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-devel] [PATCH] target/m68k: add a mechanism to automatically free TCGv
Date: Mon, 19 Mar 2018 00:27:59 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

Hi Laurent,

On 03/18/2018 05:12 PM, Laurent Vivier wrote:
> SRC_EA() and gen_extend() can return either a temporary
> TCGv or a memory allocated one. Mark them when they are
> allocated, and free them automatically at end of the
> instruction translation.
> 
> We want to free locally allocated TCGv to avoid
> overflow in sequence like:
> 
>   0xc00ae406:  movel %fp@(-132),%fp@(-268)
>   0xc00ae40c:  movel %fp@(-128),%fp@(-264)
>   0xc00ae412:  movel %fp@(-20),%fp@(-212)
>   0xc00ae418:  movel %fp@(-16),%fp@(-208)
>   0xc00ae41e:  movel %fp@(-60),%fp@(-220)
>   0xc00ae424:  movel %fp@(-56),%fp@(-216)
>   0xc00ae42a:  movel %fp@(-124),%fp@(-252)
>   0xc00ae430:  movel %fp@(-120),%fp@(-248)
>   0xc00ae436:  movel %fp@(-12),%fp@(-260)
>   0xc00ae43c:  movel %fp@(-8),%fp@(-256)
>   0xc00ae442:  movel %fp@(-52),%fp@(-276)
>   0xc00ae448:  movel %fp@(-48),%fp@(-272)
>   ...
> 
> That can fill a lot of TCGv entries in a sequence,
> especially since 15fa08f845 ("tcg: Dynamically allocate TCGOps")
> we have no limit to fill the TCGOps cache and we can fill
> the entire TCG variables array and overflow it.
> 
> Suggested-by: Richard Henderson <address@hidden>
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  target/m68k/translate.c | 100 
> +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 65 insertions(+), 35 deletions(-)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index cef6f663ad..a660388054 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -123,8 +123,34 @@ typedef struct DisasContext {
>      int done_mac;
>      int writeback_mask;
>      TCGv writeback[8];
> +#define MAX_TO_RELEASE 8
> +    int release_count;
> +    TCGv release[MAX_TO_RELEASE];
>  } DisasContext;
>  
> +static void init_release_array(DisasContext *s)
> +{
> +#ifdef CONFIG_DEBUG_TCG
> +    memset(s->release, 0, sizeof(s->release));
> +#endif
> +    s->release_count = 0;
> +}
> +
> +static void do_release(DisasContext *s)
> +{
> +    int i;
> +    for (i = 0; i < s->release_count; i++) {
> +        tcg_temp_free(s->release[i]);
> +    }
> +    init_release_array(s);
> +}
> +
> +static TCGv mark_to_release(DisasContext *s, TCGv tmp)
> +{
> +    g_assert(s->release_count < MAX_TO_RELEASE);
> +    return s->release[s->release_count++] = tmp;
> +}
> +
>  static TCGv get_areg(DisasContext *s, unsigned regno)
>  {
>      if (s->writeback_mask & (1 << regno)) {
> @@ -347,7 +373,8 @@ static TCGv gen_ldst(DisasContext *s, int opsize, TCGv 
> addr, TCGv val,
>          gen_store(s, opsize, addr, val, index);
>          return store_dummy;
>      } else {
> -        return gen_load(s, opsize, addr, what == EA_LOADS, index);
> +        return mark_to_release(s, gen_load(s, opsize, addr,
> +                                           what == EA_LOADS, index));
>      }
>  }
>  
> @@ -439,7 +466,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, 
> DisasContext *s, TCGv base)
>          } else {
>              bd = 0;
>          }
> -        tmp = tcg_temp_new();
> +        tmp = mark_to_release(s, tcg_temp_new());
>          if ((ext & 0x44) == 0) {
>              /* pre-index */
>              add = gen_addr_index(s, ext, tmp);
> @@ -449,7 +476,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, 
> DisasContext *s, TCGv base)
>          if ((ext & 0x80) == 0) {
>              /* base not suppressed */
>              if (IS_NULL_QREG(base)) {
> -                base = tcg_const_i32(offset + bd);
> +                base = mark_to_release(s, tcg_const_i32(offset + bd));
>                  bd = 0;
>              }
>              if (!IS_NULL_QREG(add)) {
> @@ -465,7 +492,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, 
> DisasContext *s, TCGv base)
>                  add = tmp;
>              }
>          } else {
> -            add = tcg_const_i32(bd);
> +            add = mark_to_release(s, tcg_const_i32(bd));
>          }
>          if ((ext & 3) != 0) {
>              /* memory indirect */
> @@ -494,7 +521,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, 
> DisasContext *s, TCGv base)
>          }
>      } else {
>          /* brief extension word format */
> -        tmp = tcg_temp_new();
> +        tmp = mark_to_release(s, tcg_temp_new());
>          add = gen_addr_index(s, ext, tmp);
>          if (!IS_NULL_QREG(base)) {
>              tcg_gen_add_i32(tmp, add, base);
> @@ -617,14 +644,14 @@ static void gen_flush_flags(DisasContext *s)
>      s->cc_op = CC_OP_FLAGS;
>  }
>  
> -static inline TCGv gen_extend(TCGv val, int opsize, int sign)
> +static inline TCGv gen_extend(DisasContext *s, TCGv val, int opsize, int 
> sign)

I'd rather see this done in 2 patches, one adding the DisasContext
argument, and another to free the TCGv.

Split in 2 or as it:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>  {
>      TCGv tmp;
>  
>      if (opsize == OS_LONG) {
>          tmp = val;
>      } else {
> -        tmp = tcg_temp_new();
> +        tmp = mark_to_release(s, tcg_temp_new());
>          gen_ext(tmp, val, opsize, sign);
>      }
>  
> @@ -746,7 +773,7 @@ static TCGv gen_lea_mode(CPUM68KState *env, DisasContext 
> *s,
>              return NULL_QREG;
>          }
>          reg = get_areg(s, reg0);
> -        tmp = tcg_temp_new();
> +        tmp = mark_to_release(s, tcg_temp_new());
>          if (reg0 == 7 && opsize == OS_BYTE &&
>              m68k_feature(s->env, M68K_FEATURE_M68000)) {
>              tcg_gen_subi_i32(tmp, reg, 2);
> @@ -756,7 +783,7 @@ static TCGv gen_lea_mode(CPUM68KState *env, DisasContext 
> *s,
>          return tmp;
>      case 5: /* Indirect displacement.  */
>          reg = get_areg(s, reg0);
> -        tmp = tcg_temp_new();
> +        tmp = mark_to_release(s, tcg_temp_new());
>          ext = read_im16(env, s);
>          tcg_gen_addi_i32(tmp, reg, (int16_t)ext);
>          return tmp;
> @@ -767,14 +794,14 @@ static TCGv gen_lea_mode(CPUM68KState *env, 
> DisasContext *s,
>          switch (reg0) {
>          case 0: /* Absolute short.  */
>              offset = (int16_t)read_im16(env, s);
> -            return tcg_const_i32(offset);
> +            return mark_to_release(s, tcg_const_i32(offset));
>          case 1: /* Absolute long.  */
>              offset = read_im32(env, s);
> -            return tcg_const_i32(offset);
> +            return mark_to_release(s, tcg_const_i32(offset));
>          case 2: /* pc displacement  */
>              offset = s->pc;
>              offset += (int16_t)read_im16(env, s);
> -            return tcg_const_i32(offset);
> +            return mark_to_release(s, tcg_const_i32(offset));
>          case 3: /* pc index+displacement.  */
>              return gen_lea_indexed(env, s, NULL_QREG);
>          case 4: /* Immediate.  */
> @@ -811,7 +838,7 @@ static TCGv gen_ea_mode(CPUM68KState *env, DisasContext 
> *s, int mode, int reg0,
>              gen_partset_reg(opsize, reg, val);
>              return store_dummy;
>          } else {
> -            return gen_extend(reg, opsize, what == EA_LOADS);
> +            return gen_extend(s, reg, opsize, what == EA_LOADS);
>          }
>      case 1: /* Address register direct.  */
>          reg = get_areg(s, reg0);
> @@ -819,7 +846,7 @@ static TCGv gen_ea_mode(CPUM68KState *env, DisasContext 
> *s, int mode, int reg0,
>              tcg_gen_mov_i32(reg, val);
>              return store_dummy;
>          } else {
> -            return gen_extend(reg, opsize, what == EA_LOADS);
> +            return gen_extend(s, reg, opsize, what == EA_LOADS);
>          }
>      case 2: /* Indirect register */
>          reg = get_areg(s, reg0);
> @@ -900,7 +927,7 @@ static TCGv gen_ea_mode(CPUM68KState *env, DisasContext 
> *s, int mode, int reg0,
>              default:
>                  g_assert_not_reached();
>              }
> -            return tcg_const_i32(offset);
> +            return mark_to_release(s, tcg_const_i32(offset));
>          default:
>              return NULL_QREG;
>          }
> @@ -1759,8 +1786,8 @@ DISAS_INSN(abcd_reg)
>  
>      gen_flush_flags(s); /* !Z is sticky */
>  
> -    src = gen_extend(DREG(insn, 0), OS_BYTE, 0);
> -    dest = gen_extend(DREG(insn, 9), OS_BYTE, 0);
> +    src = gen_extend(s, DREG(insn, 0), OS_BYTE, 0);
> +    dest = gen_extend(s, DREG(insn, 9), OS_BYTE, 0);
>      bcd_add(dest, src);
>      gen_partset_reg(OS_BYTE, DREG(insn, 9), dest);
>  
> @@ -1794,8 +1821,8 @@ DISAS_INSN(sbcd_reg)
>  
>      gen_flush_flags(s); /* !Z is sticky */
>  
> -    src = gen_extend(DREG(insn, 0), OS_BYTE, 0);
> -    dest = gen_extend(DREG(insn, 9), OS_BYTE, 0);
> +    src = gen_extend(s, DREG(insn, 0), OS_BYTE, 0);
> +    dest = gen_extend(s, DREG(insn, 9), OS_BYTE, 0);
>  
>      bcd_sub(dest, src);
>  
> @@ -1856,7 +1883,7 @@ DISAS_INSN(addsub)
>  
>      add = (insn & 0x4000) != 0;
>      opsize = insn_opsize(insn);
> -    reg = gen_extend(DREG(insn, 9), opsize, 1);
> +    reg = gen_extend(s, DREG(insn, 9), opsize, 1);
>      dest = tcg_temp_new();
>      if (insn & 0x100) {
>          SRC_EA(env, tmp, opsize, 1, &addr);
> @@ -2386,7 +2413,7 @@ DISAS_INSN(cas)
>          return;
>      }
>  
> -    cmp = gen_extend(DREG(ext, 0), opsize, 1);
> +    cmp = gen_extend(s, DREG(ext, 0), opsize, 1);
>  
>      /* if  <EA> == Dc then
>       *     <EA> = Du
> @@ -3055,7 +3082,7 @@ DISAS_INSN(or)
>      int opsize;
>  
>      opsize = insn_opsize(insn);
> -    reg = gen_extend(DREG(insn, 9), opsize, 0);
> +    reg = gen_extend(s, DREG(insn, 9), opsize, 0);
>      dest = tcg_temp_new();
>      if (insn & 0x100) {
>          SRC_EA(env, src, opsize, 0, &addr);
> @@ -3120,8 +3147,8 @@ DISAS_INSN(subx_reg)
>  
>      opsize = insn_opsize(insn);
>  
> -    src = gen_extend(DREG(insn, 0), opsize, 1);
> -    dest = gen_extend(DREG(insn, 9), opsize, 1);
> +    src = gen_extend(s, DREG(insn, 0), opsize, 1);
> +    dest = gen_extend(s, DREG(insn, 9), opsize, 1);
>  
>      gen_subx(s, src, dest, opsize);
>  
> @@ -3176,7 +3203,7 @@ DISAS_INSN(cmp)
>  
>      opsize = insn_opsize(insn);
>      SRC_EA(env, src, opsize, 1, NULL);
> -    reg = gen_extend(DREG(insn, 9), opsize, 1);
> +    reg = gen_extend(s, DREG(insn, 9), opsize, 1);
>      gen_update_cc_cmp(s, reg, src, opsize);
>  }
>  
> @@ -3329,8 +3356,8 @@ DISAS_INSN(addx_reg)
>  
>      opsize = insn_opsize(insn);
>  
> -    dest = gen_extend(DREG(insn, 9), opsize, 1);
> -    src = gen_extend(DREG(insn, 0), opsize, 1);
> +    dest = gen_extend(s, DREG(insn, 9), opsize, 1);
> +    src = gen_extend(s, DREG(insn, 0), opsize, 1);
>  
>      gen_addx(s, src, dest, opsize);
>  
> @@ -3369,7 +3396,7 @@ static inline void shift_im(DisasContext *s, uint16_t 
> insn, int opsize)
>      int logical = insn & 8;
>      int left = insn & 0x100;
>      int bits = opsize_bytes(opsize) * 8;
> -    TCGv reg = gen_extend(DREG(insn, 0), opsize, !logical);
> +    TCGv reg = gen_extend(s, DREG(insn, 0), opsize, !logical);
>  
>      if (count == 0) {
>          count = 8;
> @@ -3419,7 +3446,7 @@ static inline void shift_reg(DisasContext *s, uint16_t 
> insn, int opsize)
>      int logical = insn & 8;
>      int left = insn & 0x100;
>      int bits = opsize_bytes(opsize) * 8;
> -    TCGv reg = gen_extend(DREG(insn, 0), opsize, !logical);
> +    TCGv reg = gen_extend(s, DREG(insn, 0), opsize, !logical);
>      TCGv s32;
>      TCGv_i64 t64, s64;
>  
> @@ -3556,7 +3583,7 @@ DISAS_INSN(shift_mem)
>             while M68000 sets if the most significant bit is changed at
>             any time during the shift operation */
>          if (!logical && m68k_feature(s->env, M68K_FEATURE_M68000)) {
> -            src = gen_extend(src, OS_WORD, 1);
> +            src = gen_extend(s, src, OS_WORD, 1);
>              tcg_gen_xor_i32(QREG_CC_V, QREG_CC_N, src);
>          }
>      } else {
> @@ -3789,7 +3816,7 @@ DISAS_INSN(rotate8_im)
>      TCGv shift;
>      int tmp;
>  
> -    reg = gen_extend(DREG(insn, 0), OS_BYTE, 0);
> +    reg = gen_extend(s, DREG(insn, 0), OS_BYTE, 0);
>  
>      tmp = (insn >> 9) & 7;
>      if (tmp == 0) {
> @@ -3816,7 +3843,7 @@ DISAS_INSN(rotate16_im)
>      TCGv shift;
>      int tmp;
>  
> -    reg = gen_extend(DREG(insn, 0), OS_WORD, 0);
> +    reg = gen_extend(s, DREG(insn, 0), OS_WORD, 0);
>      tmp = (insn >> 9) & 7;
>      if (tmp == 0) {
>          tmp = 8;
> @@ -3876,7 +3903,7 @@ DISAS_INSN(rotate8_reg)
>      TCGv t0, t1;
>      int left = (insn & 0x100);
>  
> -    reg = gen_extend(DREG(insn, 0), OS_BYTE, 0);
> +    reg = gen_extend(s, DREG(insn, 0), OS_BYTE, 0);
>      src = DREG(insn, 9);
>      /* shift in [0..63] */
>      t0 = tcg_temp_new_i32();
> @@ -3911,7 +3938,7 @@ DISAS_INSN(rotate16_reg)
>      TCGv t0, t1;
>      int left = (insn & 0x100);
>  
> -    reg = gen_extend(DREG(insn, 0), OS_WORD, 0);
> +    reg = gen_extend(s, DREG(insn, 0), OS_WORD, 0);
>      src = DREG(insn, 9);
>      /* shift in [0..63] */
>      t0 = tcg_temp_new_i32();
> @@ -4353,7 +4380,7 @@ DISAS_INSN(chk)
>          return;
>      }
>      SRC_EA(env, src, opsize, 1, NULL);
> -    reg = gen_extend(DREG(insn, 9), opsize, 1);
> +    reg = gen_extend(s, DREG(insn, 9), opsize, 1);
>  
>      gen_flush_flags(s);
>      gen_helper_chk(cpu_env, reg, src);
> @@ -6033,6 +6060,7 @@ static void disas_m68k_insn(CPUM68KState * env, 
> DisasContext *s)
>      uint16_t insn = read_im16(env, s);
>      opcode_table[insn](env, s, insn);
>      do_writebacks(s);
> +    do_release(s);
>  }
>  
>  /* generate intermediate code for basic block 'tb'.  */
> @@ -6067,6 +6095,8 @@ void gen_intermediate_code(CPUState *cs, 
> TranslationBlock *tb)
>          max_insns = TCG_MAX_INSNS;
>      }
>  
> +    init_release_array(dc);
> +
>      gen_tb_start(tb);
>      do {
>          pc_offset = dc->pc - pc_start;
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]