Re: [Qemu-devel] [PATCH 3/3] target-mips:Support for Cavium specific instructions

[Peter Maydell]

On 5 July 2011 10:19,  <address@hidden> wrote:
> ---
>  host-utils.c            |    1 +
>  target-mips/cpu.h       |    7 +
>  target-mips/helper.h    |    5 +
>  target-mips/op_helper.c |   67 +++++++
>  target-mips/translate.c |  443 
> ++++++++++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 514 insertions(+), 9 deletions(-)

Don't you also need to add support for the new instructions
to the disassembler in mips-dis.c ?

> diff --git a/host-utils.c b/host-utils.c
> index dc96123..1128698 100644
> --- a/host-utils.c
> +++ b/host-utils.c
> @@ -102,4 +102,5 @@ void muls64 (uint64_t *plow, uint64_t *phigh, int64_t a, 
> int64_t b)
>            a, b, *phigh, *plow);
>  #endif
>  }
> +
>  #endif /* !defined(__x86_64__) */

Stray random whitespace change to an unrelated file:
please drop this from the patch.

> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index b0ac4da..8e75e9b 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -171,6 +171,13 @@ struct TCState {
>     target_ulong CP0_TCSchedule;
>     target_ulong CP0_TCScheFBack;
>     int32_t CP0_Debug_tcstatus;
> +    /* Multiplier registers for Octeon */
> +    target_ulong MPL0;
> +    target_ulong MPL1;
> +    target_ulong MPL2;
> +    target_ulong P0;
> +    target_ulong P1;
> +    target_ulong P2;
>  };

If you add new fields to the CPU struct then you must also
add code to save/restore them in target-mips/machine.c.

> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 6b966b1..a1893d1 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -266,7 +266,74 @@ void helper_dmultu (target_ulong arg1, target_ulong arg2)
>  {
>     mulu64(&(env->active_tc.LO[0]), &(env->active_tc.HI[0]), arg1, arg2);
>  }
> +static void addc(uint64_t res[], uint64_t a, int i)

Can you leave blank lines between function definitions, please?
(here and also in a few places below)

> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index eb108bc..b480665 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -69,6 +69,11 @@ enum {
>     OPC_JAL      = (0x03 << 26),
>     OPC_JALS     = OPC_JAL | 0x5,
>     OPC_BEQ      = (0x04 << 26),  /* Unconditional if rs = rt = 0 (B) */
> +    /* Cavium Specific */
> +    OPC_BBIT1    = (0x3a << 26),  /* jump on bit set, cavium specific */
> +    OPC_BBIT132  = (0x3e << 26),  /* jump on bit set(for upper 32 bits) */

Space before the '(' in the comment, please.

> +    OPC_BBIT0    = (0x32 << 26),  /* jump on bit clear, cavium specific */
> +    OPC_BBIT032  = (0x36 << 26),  /* jump on bit clear(for upper 32 bits) */

Ditto.

> @@ -482,7 +512,7 @@ enum {
>  static TCGv_ptr cpu_env;
>  static TCGv cpu_gpr[32], cpu_PC;
>  static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC], 
> cpu_ACX[MIPS_DSP_ACC];
> -static TCGv cpu_dspctrl, btarget, bcond;
> +static TCGv cpu_dspctrl, btarget, bcond, mpl0, mpl1, mpl2, p0, p1, p2;

p0/p1/p2 are awful names for global variables -- far too short.
mpl0/mpl1/mpl2 aren't a great deal better.

Also it looks like at least some of these aren't really used very
often -- you should consider just doing loads/stores to env+offset
the way we do for most other cpu fields.

> +/* set on equal/not equal immidiate */
> +static void gen_set_imm(CPUState *env, uint32_t opc,
> +                        int rt, int rs, int16_t imm)
> +{
> +    target_ulong uimm = (target_long)imm;
> +    TCGv t0;
> +    const char *opn = "imm set";
> +    if (rt == 0) {
> +        /* If no destination, treat it as a NOP. */
> +        MIPS_DEBUG("NOP");
> +        return;
> +    }
> +    t0 = tcg_temp_new();
> +    gen_load_gpr(t0, rs);
> +    switch (opc) {
> +    case OPC_SEQI:
> +        tcg_gen_xori_tl(t0, t0, uimm);
> +        tcg_gen_setcondi_tl(TCG_COND_LT, cpu_gpr[rt], t0, 1);
> +        opn = "seqi";
> +        break;
> +    case OPC_SNEI:
> +        tcg_gen_xori_tl(t0, t0, uimm);
> +        tcg_gen_setcondi_tl(TCG_COND_GT, cpu_gpr[rt], t0, 0);
> +        opn = "snei";
> +        break;
> +    }
> +    tcg_temp_free(t0);
> +}

The tcg_gen_xori_tl() is the same in both cases of the switch, you
could pull it out of the switch.

> @@ -1636,6 +1881,30 @@ static void gen_arith (CPUState *env, DisasContext 
> *ctx, uint32_t opc,
>         }
>         opn = "addu";
>         break;
> +    case OPC_BADDU:
> +        {
> +            TCGv t0 = tcg_temp_new();
> +            TCGv t1 = tcg_temp_new();
> +            gen_load_gpr(t0, rs);
> +            gen_load_gpr(t1, rt);
> +            tcg_gen_add_tl(t0, t1, t0);
> +            tcg_gen_ext8u_tl(t0, t0);
> +            gen_store_gpr(t0, rd);
> +            tcg_temp_free(t0);
> +            tcg_temp_free(t1);
> +        }
> +       opn = "baddu";
> +       break;

These should go inside the braces, please [ditto for other cases below]

> +    case OPC_DMUL:
> +        {
> +            TCGv t0 = tcg_temp_new();
> +            TCGv t1 = tcg_temp_new();
> +            gen_load_gpr(t0, rs);
> +            gen_load_gpr(t1, rt);
> +            tcg_gen_mul_i64(cpu_gpr[rd], t0, t1);
> +        }
> +            opn = "dmul";
> +            break;

Missing tcg_temp_free()s ?


>     case OPC_SUB:
>         {
>             TCGv t0 = tcg_temp_local_new();
> @@ -2704,6 +2973,7 @@ static void gen_compute_branch (DisasContext *ctx, 
> uint32_t opc,
>     target_ulong btgt = -1;
>     int blink = 0;
>     int bcond_compute = 0;
> +    target_ulong maskb; /* Used in BBIT0 and BBIT1 */

Add braces to the relevant cases and use variables local to that
restricted scope -- there's no need for this to be visible for the
whole function.

> +        case OPC_BBIT1:
> +        case OPC_BBIT132:
> +            tcg_gen_setcondi_tl(TCG_COND_NE, bcond, t0, 0);
> +            goto not_likely;
> +            case OPC_BBIT0:
> +            case OPC_BBIT032:
> +            tcg_gen_setcondi_tl(TCG_COND_EQ, bcond, t0, 0);
> +            goto not_likely;

Indentation on the OPC_BBIT0, OPC_BBIT032 cases is wrong.

> @@ -11637,6 +11946,9 @@ static void decode_opc (CPUState *env, DisasContext 
> *ctx, int *is_branch)
>     rd = (ctx->opcode >> 11) & 0x1f;
>     sa = (ctx->opcode >> 6) & 0x1f;
>     imm = (int16_t)ctx->opcode;
> +    /* 10 bit Immediate value For SEQI,SNEI */
> +    imm10 = (ctx->opcode >> 6) & 0x3ff;
> +

Only used inside a single case, so just make it a variable scoped
only to that case and decode it from opcode at that point.

> @@ -11862,6 +12174,58 @@ static void decode_opc (CPUState *env, DisasContext 
> *ctx, int *is_branch)
>         case OPC_MUL:
>             gen_arith(env, ctx, op1, rd, rs, rt);
>             break;
> +#if defined(TARGET_MIPS64)
> +
> +        case OPC_DMUL:

This blank line is unnecessary.

> @@ -11881,13 +12245,24 @@ static void decode_opc (CPUState *env, DisasContext 
> *ctx, int *is_branch)
>             break;
>         case OPC_DIV_G_2F:
>         case OPC_DIVU_G_2F:
> -        case OPC_MULT_G_2F:
>         case OPC_MULTU_G_2F:
>         case OPC_MOD_G_2F:
>         case OPC_MODU_G_2F:
>             check_insn(env, ctx, INSN_LOONGSON2F);
>             gen_loongson_integer(ctx, op1, rd, rs, rt);
>             break;
> +        case OPC_MULT_G_2F:
> +            if (!(env->insn_flags & CPU_OCTEON)) {
> +                check_insn(env, ctx, INSN_LOONGSON2F);
> +                gen_loongson_integer(ctx, op1, rd, rs, rt);
> +            } else {
> +#if defined(TARGET_MIPS64)
> +                /* Cavium Specific vmm0 */
> +                check_mips_64(ctx);
> +                gen_LMI(env, ctx, op1, rs, rt, rd);
> +#endif
> +            }
> +            break;

You could rearrange this to:
  case OPC_MULT_G_2F:
#if defined(TARGET_MIPS64)
     if (OCTEON) {
         check_mips_64(ctx);
         gen_LMI(...);
     }
#endif
     /* Otherwise fall through, this is also a Loongson insn */
  case OPC_DIV_G_2F:
  case OPC_DIVU_G_2F:
  [etc]
         check_insn(env, ctx, INSN_LOONGSON2F);
         gen_loongson_integer(ctx, op1, rd, rs, rt);

...which lets you avoid duplicating the loongson code.
Ditto in the DMULT/DDIV case.

-- PMM