Re: [Qemu-devel] [PATCH v2 05/33] target/mips: Add emulation of nanoMIPS 16-bit save and restore instructions

[Aleksandar Markovic]

> Subject: [PATCH v2 05/33] target/mips: Add emulation of nanoMIPS 16-bit save 
> and restore > instructions
>
> From: Matthew Fortune <address@hidden>
>
> Add emulation of SAVE16 and RESTORE.JRC16 instructions. Routines
> gen_save(), gen_restore(), and gen_adjust_sp() are provided for this
> purpose.
>
> Signed-off-by: Yongbok Kim <address@hidden>
> Signed-off-by: Aleksandar Markovic <address@hidden>
> Signed-off-by: Stefan Markovic <address@hidden>
> ---
>  target/mips/translate.c | 73 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 9b87fed..379b6ba 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -16367,6 +16367,65 @@ static int decode_gpr_gpr4_zero(int r)
>      return map[r & 0xf];
>  }
>
> +static void gen_adjust_sp(DisasContext *ctx, int u)
> +{
> +    TCGv tsp = tcg_temp_new();
> +    gen_base_offset_addr(ctx, tsp, 29, u);
> +    gen_store_gpr(tsp, 29);

What is the meaning and origin of these two instances of the constant 29?
Shouldn't it be defined as a preprocessor constant?
Is this unique for nanoMIPS?

> +    tcg_temp_free(tsp);
> +}
> +
> +static void gen_save(DisasContext *ctx, uint8_t rt, uint8_t count,
> +                     uint8_t gp, uint16_t u)
> +{
> +    int counter = 0;
> +    TCGv va = tcg_temp_new();
> +    TCGv t0 = tcg_temp_new();
> +
> +    while (counter != count) {
> +        bool use_gp = gp && (counter == count - 1);
> +        int this_rt = use_gp ? 28 : (rt & 0x10) | ((rt + counter) & 0x1f);
> +        int this_offset = -((counter + 1) << 2);
> +        gen_base_offset_addr(ctx, va, 29, this_offset);
> +        gen_load_gpr(t0, this_rt);
> +        tcg_gen_qemu_st_tl(t0, va, ctx->mem_idx,
> +                           (MO_TEUL | ctx->default_tcg_memop_mask));
> +        counter++;
> +    }
> +
> +    /* adjust stack pointer */
> +    gen_adjust_sp(ctx, -u);
> +
> +    tcg_temp_free(t0);
> +    tcg_temp_free(va);
> +}
> +
> +static void gen_restore(DisasContext *ctx, uint8_t rt, uint8_t count,
> +                        uint8_t gp, uint16_t u)
> +{
> +    int counter = 0;
> +    TCGv va = tcg_temp_new();
> +    TCGv t0 = tcg_temp_new();
> +
> +    while (counter != count) {
> +        bool use_gp = gp && (counter == count - 1);
> +        int this_rt = use_gp ? 28 : (rt & 0x10) | ((rt + counter) & 0x1f);

Now we have even 28...

> +        int this_offset = u - ((counter + 1) << 2);
> +        gen_base_offset_addr(ctx, va, 29, this_offset);

29 again... too unclear...

> +        tcg_gen_qemu_ld_tl(t0, va, ctx->mem_idx, MO_TESL |
> +                        ctx->default_tcg_memop_mask);
> +        tcg_gen_ext32s_tl(t0, t0);
> +        gen_store_gpr(t0, this_rt);
> +        counter++;
> +    }
> +
> +    /* adjust stack pointer */
> +    gen_adjust_sp(ctx, u);
> +
> +    tcg_temp_free(t0);
> +    tcg_temp_free(va);
> +}
> +
>  static void gen_pool16c_nanomips_insn(DisasContext *ctx)
>  {
>      int rt = decode_gpr_gpr3(NANOMIPS_EXTRACT_RD(ctx->opcode));
> @@ -16715,6 +16774,20 @@ static int decode_nanomips_opc(CPUMIPSState *env, 
> DisasContext > *ctx)
>          }
>          break;
>      case NM_P16_SR:
> +    {
> +        int count = extract32(ctx->opcode, 0, 4);
> +        int u = extract32(ctx->opcode, 4, 4) << 4;
> +        int rt = 30 + ((ctx->opcode >> 9) & 1);

What is the meaning and origin of constant 30?
Shouldn't it be defined as a preprocessor constant?
Is this unique for nanoMIPS?

> +        switch ((ctx->opcode >> 8) & 1) {
> +        case NM_SAVE16:
> +            gen_save(ctx, rt, count, 0, u);
> +            break;
> +        case NM_RESTORE_JRC16:
> +            gen_restore(ctx, rt, count, 0, u);
> +            gen_compute_branch(ctx, OPC_JR, 2, 31, 0, 0, 0);
> +            break;
> +        }
> +    }
>          break;
>      case NM_MOVEP:
>      case NM_MOVEPREV:
> --
> 2.7.4