Re: [Qemu-devel] MIPS64 user mode emulation Patch

[Nathan Froyd <froydnj@codesourcery.com>]

On Sat, Mar 26, 2011 at 11:58:37AM +0500, Khansa Butt wrote:
> Subject: [PATCH] MIPS64 user mode emulation in QEMU
>  This patch adds support for Cavium Network's
>  Octeon 57XX user mode instructions.  Octeon
>  57xx is based on MIPS64.  So this patch is
>  the first MIPS64 User Mode Emulation in QEMU
>  This is the team(Khansa Butt, Ehsan-ul-Haq, Abdul Qadeer, Abdul Waheed)
>  work of HPCNL Lab at KICS-UET Lahore.

Thanks for doing this.  As already noted, this patch should be split
into at least two patches: one to add Octeon-specific instructions in
target-mips/ and one adding the necessary linux-user bits.

> +extern int TARGET_OCTEON;

I don't think a global like this is the right way to go.  Perhaps the
elfload.c code should set a flag in image_info , which can then be used
to set a flag in CPUMIPSState later on.

If we must use a global variable, it should be declared in
target-mips/cpu.h.

> @@ -2013,7 +2024,8 @@ void cpu_loop(CPUMIPSState *env)
>                                   env->active_tc.gpr[5],
>                                   env->active_tc.gpr[6],
>                                   env->active_tc.gpr[7],
> -                                 arg5, arg6/*, arg7, arg8*/);
> +                                 env->active_tc.gpr[8],
> +                                 env->active_tc.gpr[9]/*, arg7, arg8*/);
>              }
>              if (ret == -TARGET_QEMU_ESIGRETURN) {
>                  /* Returning from a successful sigreturn syscall.

This change breaks O32 binaries; it needs to be done in a different way.

> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 499c4d7..47fef05 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7195,6 +7195,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
> arg1,
>      case TARGET_NR_set_thread_area:
>  #if defined(TARGET_MIPS)
>        ((CPUMIPSState *) cpu_env)->tls_value = arg1;
> +       /*tls entry is moved to k0 so that this can be used later*/
> +      ((CPUMIPSState *) cpu_env)->active_tc.gpr[26] = arg1;
>        ret = 0;
>        break;
>  #elif defined(TARGET_CRIS)

I believe this is only correct for Octeon binaries; it's not how the
rest of the MIPS world works.  It therefore needs to be conditional on
Octeon-ness.

> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -140,6 +140,20 @@ typedef struct mips_def_t mips_def_t;
>  #define MIPS_FPU_MAX 1
>  #define MIPS_DSP_ACC 4
> 
> +typedef struct cavium_mul cavium_mul;
> +struct cavium_mul {
> + target_ulong MPL0;
> + target_ulong MPL1;
> + target_ulong MPL2;
> + target_ulong P0;
> + target_ulong P1;
> + target_ulong P2;
> +};
> +typedef struct cvmctl_register cvmctl_register;
> +struct cvmctl_register {
> + target_ulong cvmctl;
> +};

The indentation here needs to be fixed.  I don't think there's any
reason why these need to be defined outside TCState, either.

> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index cce77be..9c3d772 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -36,6 +36,15 @@
>  #define GEN_HELPER 1
>  #include "helper.h"
> 
> +int TARGET_OCTEON;
> +#if defined(TARGET_MIPS64)
> +/*Macros for setting values of cvmctl registers*/
> +#define FUSE_START_BIT(cvmctl)(cvmctl | 0x80000000)
> +#define KASUMI(cvmctl)(cvmctl | 0x20000000)
> +#define IPPCI(cvmctl)(cvmctl | 0x380)
> +#define IPTI(cvmctl)(cvmctl | 0x70)
> +#endif

Please follow existing style; spaces between the comment and comment
markers (many examples in translate.c, not just here) and spaces between
the macro argument list and definition.

> @@ -779,7 +818,9 @@ static inline void gen_op_addr_add (DisasContext *ctx,
> TCGv ret, TCGv arg0, TCGv
>         See the MIPS64 PRA manual, section 4.10. */
>      if (((ctx->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_UM) &&
>          !(ctx->hflags & MIPS_HFLAG_UX)) {
> -        tcg_gen_ext32s_i64(ret, ret);
> +        /*This function sign extend 32 bit value to 64 bit, was causing
> error
> +          when ld instruction came.Thats why it is commmented out*/
> +       /* tcg_gen_ext32s_i64(ret, ret);*/
>      }
>  #endif
>  }

Um, no.  If you needed to comment this out, you have a bug someplace
else.  Don't paper over the bug here.

> +        case OPC_VMULU:
> +        case OPC_V3MULU:

These two are large enough that they should be done as out-of-line
helpers.

Also, since all these new instructions are Octeon-specific, there should
be checks emitted to ensure that they produce appropriate errors when
non-Octeon hardware is being simulated, similar in style to
check_mips_64.

>  /* Arithmetic */
>  static void gen_arith (CPUState *env, DisasContext *ctx, uint32_t opc,
>                         int rd, int rs, int rt)
>  {
>      const char *opn = "arith";
> 
> +    target_ulong mask = 0xFF;

I don't think it's really necessary to have this, but if you feel it's
necessary, please move the declaration closer to the point of use.

> +#if defined(TARGET_MIPS64)
> +static void gen_seqsne (DisasContext *ctx, uint32_t opc,
> +                        int rd, int rs, int rt)
> +{
> +    const char *opn = "seq/sne";
> +    TCGv t0, t1;
> +    t0 = tcg_temp_new();
> +    t1 = tcg_temp_new();
> +    switch (opc) {
> +    case OPC_SEQ:
> +        {
> +            tcg_gen_xor_tl(cpu_gpr[rd], cpu_gpr[rs], cpu_gpr[rt]);
> +            gen_load_gpr(t0, rd);
> +            tcg_gen_setcondi_tl(TCG_COND_LTU, cpu_gpr[rd], t0, 1);
> +        }
> +        opn = "seq";
> +        break;

This looks like you are comparing rd with itself?

> +    case OPC_SNE:
> +        {
> +            tcg_gen_xor_tl(cpu_gpr[rd], cpu_gpr[rs], cpu_gpr[rt]);
> +            gen_load_gpr(t0, rd);
> +            gen_load_gpr(t1, 0);
> +            tcg_gen_setcond_tl(TCG_COND_LTU, cpu_gpr[rd], t1, t0);

You could use setcondi here by reversing the condition.

> +#if defined(TARGET_MIPS64)
> +static void insn_opc_pop (DisasContext *ctx, CPUState *env, uint32_t opc,
> +                          int rd, int rs, int rt)
> +static void insn_opc_dpop (DisasContext *ctx, CPUState *env, uint32_t opc,
> +                           int rd, int rs, int rt)

These should also be done as out-of-line helpers.

> @@ -2983,7 +3408,44 @@ static void gen_compute_branch (DisasContext *ctx,
> uint32_t opc,
>      tcg_temp_free(t0);
>      tcg_temp_free(t1);
>  }
> +/*For cavium specific extract instructions*/
> +#if defined(TARGET_MIPS64)
> +static void gen_exts (CPUState *env,DisasContext *ctx, uint32_t opc, int
> rt,
> +                      int rs, int lsb, int msb)
> +{
> +    TCGv t0 = tcg_temp_new();
> +    TCGv t1 = tcg_temp_new();
> +    target_ulong mask;
> +    gen_load_gpr(t1, rs);
> +    switch (opc) {
> +    case OPC_EXTS:
> +        tcg_gen_shri_tl(t0, t1, lsb);
> +        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> +        /* To sign extened the remaining bits according to
> +           the msb of the bit field */
> +        mask = 1ULL << msb;
> +        tcg_gen_andi_tl(t1, t0, mask);
> +        tcg_gen_addi_tl(t1, t1, -1);
> +        tcg_gen_not_i64(t1, t1);
> +        tcg_gen_or_tl(t0, t0, t1);

You can use tcg_gen_orc_tl here and elsewhere.

-Nathan