Re: [Qemu-devel] [PATCH v2 1/3] target-mips: Copy restrictions from ext/ins to dext/dins

[Aurelien Jarno <aurelien@aurel32.net>]

On 2015-08-03 12:35, Richard Henderson wrote:
> The checks in dins is required to avoid triggering an assertion
> in tcg_gen_deposit_tl.  The check in dext is just for completeness.
> Fold the other D cases in via fallthru.
> 
> In this case the errant dins appears to be data, not code, as
> translation failed to stop after a break insn.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target-mips/translate.c | 45 +++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index d1de35a..1cba415 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -4750,48 +4750,53 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt,
>      gen_load_gpr(t1, rs);
>      switch (opc) {
>      case OPC_EXT:
> -        if (lsb + msb > 31)
> +        if (lsb + msb > 31) {
>              goto fail;
> +        }
>          tcg_gen_shri_tl(t0, t1, lsb);
>          if (msb != 31) {
> -            tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1);
> +            tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1);

Is this change really needed?

>          } else {
>              tcg_gen_ext32s_tl(t0, t0);
>          }
>          break;
>  #if defined(TARGET_MIPS64)
> -    case OPC_DEXTM:
> -        tcg_gen_shri_tl(t0, t1, lsb);
> -        if (msb != 31) {
> -            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1 + 32)) - 1);
> -        }
> -        break;
>      case OPC_DEXTU:
> -        tcg_gen_shri_tl(t0, t1, lsb + 32);
> -        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> -        break;
> +        lsb += 32;
> +        goto do_dext;
> +    case OPC_DEXTM:
> +        msb += 32;
> +        goto do_dext;
>      case OPC_DEXT:
> +    do_dext:
> +        if (lsb + msb > 63) {
> +            goto fail;
> +        }

Note that DEXT can't fail as both lsb and msb are in the range 0..31.
DEXTU and DEXTM can.

>          tcg_gen_shri_tl(t0, t1, lsb);
> -        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> +        if (msb != 63) {
> +            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> +        }
>          break;
>  #endif
>      case OPC_INS:
> -        if (lsb > msb)
> +        if (lsb > msb) {
>              goto fail;
> +        }
>          gen_load_gpr(t0, rt);
>          tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
>          tcg_gen_ext32s_tl(t0, t0);
>          break;
>  #if defined(TARGET_MIPS64)
> -    case OPC_DINSM:
> -        gen_load_gpr(t0, rt);
> -        tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1);
> -        break;
>      case OPC_DINSU:
> -        gen_load_gpr(t0, rt);
> -        tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1);
> -        break;
> +        lsb += 32;
> +        /* FALLTHRU */
> +    case OPC_DINSM:
> +        msb += 32;

The same way DINSM can't fail.

> +        /* FALLTHRU */
>      case OPC_DINS:
> +        if (lsb > msb) {
> +            goto fail;
> +        }
>          gen_load_gpr(t0, rt);
>          tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
>          break;

Despite the above comments:

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

Should we try to get this one into 2.4, if not already too late?

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net