Re: [Qemu-devel] [PATCH 10/13] target-mips: microMIPS32 R6 POOL32{I, C} instructions

[Leon Alrae <leon.alrae@imgtec.com>]

On 12/06/2015 15:02, Yongbok Kim wrote:
> add new microMIPS32 Release 6 POOL32I/POOL32C type instructions
> 
> Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
> ---
>  target-mips/translate.c |   36 ++++++++++++++++++++++++++++++++----
>  1 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 3d9145c..5be2a9c 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -14412,8 +14412,16 @@ static void decode_micromips32_opc (CPUMIPSState *env, DisasContext *ctx,
>              mips32_op = OPC_TGEIU;
>              goto do_trapi;
>          case TNEI:
> -            mips32_op = OPC_TNEI;
> -            goto do_trapi;
> +            /* SYNCI */

I think the comment can be improved a bit because I still need to figure out
from the actual code which block implements SYNCI. I would suggest sticking to
existing style we have for single R6 and pre-R6 instructions which share
opcode, for example:

case OPC_BC1EQZ: /* OPC_BC1ANY2 */
    if (ctx->insn_flags & ISA_MIPS32R6) {
        /* OPC_BC1EQZ */
        ...
    } else {
        /* OPC_BC1ANY2 */
        ...
    }
    break;

> +            if (ctx->insn_flags & ISA_MIPS32R6) {
> +                /* Break the TB to be able to sync copied instructions
> +                   immediately */
> +                ctx->bstate = BS_STOP;
> +            } else {
> +                mips32_op = OPC_TNEI;
> +                goto do_trapi;
> +            }
> +            break;
>          case TEQI:
>              check_insn_opc_removed(ctx, ISA_MIPS32R6);
>              mips32_op = OPC_TEQI;
> @@ -14537,10 +14545,18 @@ static void decode_micromips32_opc (CPUMIPSState *env, DisasContext *ctx,
>              check_insn(ctx, ISA_MIPS3);
>              check_mips_64(ctx);
>              mips32_op = OPC_LLD;
> +            if (ctx->insn_flags & ISA_MIPS32R6) {
> +                gen_ld(ctx, mips32_op, rt, rs, SIMM(ctx->opcode, 0, 9));
> +                break;
> +            }
>              goto do_ld_lr;
>  #endif
>          case LL:
>              mips32_op = OPC_LL;
> +            if (ctx->insn_flags & ISA_MIPS32R6) {
> +                gen_ld(ctx, mips32_op, rt, rs, SIMM(ctx->opcode, 0, 9));
> +                break;
> +            }
>              goto do_ld_lr;
>          do_ld_lr:
>              gen_ld(ctx, mips32_op, rt, rs, SIMM(ctx->opcode, 0, 12));
> @@ -14549,17 +14565,29 @@ static void decode_micromips32_opc (CPUMIPSState *env, DisasContext *ctx,
>              gen_st(ctx, mips32_op, rt, rs, SIMM(ctx->opcode, 0, 12));
>              break;
>          case SC:
> -            gen_st_cond(ctx, OPC_SC, rt, rs, SIMM(ctx->opcode, 0, 12));
> +            if (ctx->insn_flags & ISA_MIPS32R6) {
> +                gen_st_cond(ctx, OPC_SC, rt, rs, SIMM(ctx->opcode, 0, 9));
> +            } else {
> +                gen_st_cond(ctx, OPC_SC, rt, rs, SIMM(ctx->opcode, 0, 12));
> +            }
>              break;
>  #if defined(TARGET_MIPS64)
>          case SCD:
>              check_insn(ctx, ISA_MIPS3);
>              check_mips_64(ctx);
> -            gen_st_cond(ctx, OPC_SCD, rt, rs, SIMM(ctx->opcode, 0, 12));
> +            if (ctx->insn_flags & ISA_MIPS32R6) {
> +                gen_st_cond(ctx, OPC_SCD, rt, rs, SIMM(ctx->opcode, 0, 9));
> +            } else {
> +                gen_st_cond(ctx, OPC_SCD, rt, rs, SIMM(ctx->opcode, 0, 12));
> +            }

Instead of adding new "if R6" to each of these instructions and calling the
same gen_st_cond/gen_ld function with just different hardcoded value, I think
it would be cleaner to reuse offset variable and set it just once (9 in R6 or
12 in pre-R6) at the beginning of case POOL32C. Thus here we would have just:

gen_st_cond(ctx, OPC_SCD, rt, rs, offset);

Thanks,
Leon

>              break;
>  #endif
>          case PREF:
>              /* Treat as no-op */
> +            if ((ctx->insn_flags & ISA_MIPS32R6) && (rt >= 24)) {
> +                /* hint codes 24-31 are reserved and signal RI */
> +                generate_exception(ctx, EXCP_RI);
> +            }
>              break;
>          default:
>              MIPS_INVAL("pool32c");
>