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

[Khansa Butt <khansa@kics.edu.pk>]

[-- Attachment #1: Type: text/plain, Size: 10383 bytes --]

Please see the online comments highlighted in red.
I'll be sending corrected Patches to the mailing list.

On Wed, Mar 30, 2011 at 9:38 PM, Nathan Froyd <froydnj@codesourcery.com>wrote:

> 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.
>


A variable is declared in image_info to set a flag in CPUMIPSState and
discarded a global variable
@@ -51,6 +51,7 @@ struct image_info {
         abi_ulong       arg_start;
         abi_ulong       arg_end;
  int personality;
+ int elf_arch;


>
> 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.
>


The above line has been changed with following code snippet
+#if defined(TARGET_MIPS64)
+                ret = do_syscall(env, env->active_tc.gpr[2],
+                                 env->active_tc.gpr[4],
+                                 env->active_tc.gpr[5],
+                                 env->active_tc.gpr[6],
+                                 env->active_tc.gpr[7],
+                                 env->active_tc.gpr[8],
+                                 env->active_tc.gpr[9]);
+#else
                 ret = do_syscall(env, env->active_tc.gpr[2],
                                  env->active_tc.gpr[4],
                                  env->active_tc.gpr[5],
                                  env->active_tc.gpr[6],
                                  env->active_tc.gpr[7],
                                  arg5, arg6/*, arg7, arg8*/);
+#endif



> > 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.
>
> The above thing has been made octeon specific


> > --- 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.
>

Octeon register in TCState as follows
@@ -171,6 +176,15 @@ 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;
+    /* Octeon specific Coprocessor 0 register */
+    target_ulong cvmctl;
 };




> > 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.
>


The above behavior when  ld instruction came was due to env->hflags had
 not been 'or' ed with mips64 user mode flag(MIPS_HFLAG_UX) so now following
line is added in translate.c: cpu_rest()
#ifdef TARGET_MIPS64
+    env->hflags |=  MIPS_HFLAG_UX;
     if (env->active_fpu.fcr0 & (1 << FCR0_F64)) {
         env->hflags |= MIPS_HFLAG_F64;
     }



> > +        case OPC_VMULU:
> > +        case OPC_V3MULU:
>
> These two are large enough that they should be done as out-of-line
> helpers.
>

Out-of-line helper function has been implemented


>
> 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.
>

check for octeon specific instructions
static inline void check_octeon(DisasContext *ctx, CPUState *env)
+{
+    if (!env->TARGET_OCTEON)
+        generate_exception(ctx, EXCP_RI);
+}




>
> >  /* 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?
>


t0 is actually compared with 1. if t0 <1 then rd =1
for reference check slti instruction of mips64r2
switch (opc) {
    case OPC_SLTI:
        tcg_gen_setcondi_tl(TCG_COND_LT, cpu_gpr[rt], t0, uimm);
        opn = "slti";



> > +    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.
>


This is set if not equal instruction. if rs != rt, rd has some value greater
than 1 (because of
tcg_gen_xor_tl) so if t1 < t0(in  tcg_gen_setcond_tl) rd will be 1. that's
wat we needed "set if not
equal"



> > +#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
>

[-- Attachment #2: Type: text/html, Size: 18549 bytes --]