From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52709 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q8UvF-0006Vs-7d for qemu-devel@nongnu.org; Sat, 09 Apr 2011 05:57:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q8UvB-0007p7-0k for qemu-devel@nongnu.org; Sat, 09 Apr 2011 05:57:39 -0400 Received: from mail-qy0-f173.google.com ([209.85.216.173]:43742) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q8UvA-0007ot-Me for qemu-devel@nongnu.org; Sat, 09 Apr 2011 05:57:36 -0400 Received: by qyk36 with SMTP id 36so378787qyk.4 for ; Sat, 09 Apr 2011 02:57:35 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20110330163850.GA23480@codesourcery.com> References: <20110330163850.GA23480@codesourcery.com> Date: Sat, 9 Apr 2011 14:57:35 +0500 Message-ID: Subject: Re: [Qemu-devel] MIPS64 user mode emulation Patch From: Khansa Butt Content-Type: multipart/alternative; boundary=90e6ba308c6a2c8e0404a0795fd6 List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nathan Froyd Cc: qemu-devel@nongnu.org --90e6ba308c6a2c8e0404a0795fd6 Content-Type: text/plain; charset=ISO-8859-1 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 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 > --90e6ba308c6a2c8e0404a0795fd6 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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 1= 1:58:37AM +0500, Khansa Butt wrote:
> Subject: [PATCH] MIPS64 user mode emulation in= QEMU
> =A0This patch adds support for Cavium Network's
> =A0Octeon 57XX user mode instructions. =A0Octeon
> =A057xx is based on MIPS64. =A0So this patch is
> =A0the first MIPS64 User Mode Emulation in QEMU
> =A0This is the team(Khansa Butt, Ehsan-ul-Haq, Abdul Qadeer, Abdul Wah= eed)
> =A0work of HPCNL Lab at KICS-UET Lahore.

Thanks for doing this. =A0As already noted, this patch should be spli= t
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. =A0Perhaps 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 variab= le is declared in image_info to set a flag in CPUMIPSState and discarded a = global variable
@@ -51,6 +51,7= @@ struct image_info {
=A0=A0 =A0 =A0 =A0 abi_ulong =A0 =A0 =A0 arg_start;=
=A0=A0 =A0 = =A0 =A0 abi_ulong =A0 =A0 =A0 arg_end;
=A0 int personality;
+ int elf_arch;
=A0

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)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 en= v->active_tc.gpr[5],
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 en= v->active_tc.gpr[6],
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 en= v->active_tc.gpr[7],
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 arg5= , arg6/*, arg7, arg8*/);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 env-= >active_tc.gpr[8],
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 env-= >active_tc.gpr[9]/*, arg7, arg8*/);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ret =3D=3D -TARGET_QEMU_ESIGRETURN) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Returning from a successful sigr= eturn syscall.

This change breaks O32 binaries; it needs to be done in a different w= ay.


The above line has been changed with followi= ng code snippet
+#if defined(TARGET= _MIPS64)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D do_syscall(env, env->active_t= c.gpr[2],
+ =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 env->active_tc.gpr[4],
+ =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 env->active_= tc.gpr[5],
+ =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 env->active_tc.gpr[6],
+ =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 env->active_= tc.gpr[7],
+ =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 env->active_tc.gpr[8],
+ =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 env->active_= tc.gpr[9]);
+#else
=
=A0=A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 ret =3D do_syscall(env, env->active_tc.gpr[2],
=A0=A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0env->active_tc.gp= r[4],
=A0=A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0env->active_tc.gpr[5]= ,
=A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0env->= active_tc.gpr[6],
=A0=A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0env->active_tc.gpr[7]= ,
=A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0arg5, ar= g6/*, arg7, arg8*/);
+#endif=A0

=A0
> 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,
> =A0 =A0 =A0case TARGET_NR_set_thread_area:
> =A0#if defined(TARGET_MIPS)
> =A0 =A0 =A0 =A0((CPUMIPSState *) cpu_env)->tls_value =3D arg1;
> + =A0 =A0 =A0 /*tls entry is moved to k0 so that this can be used late= r*/
> + =A0 =A0 =A0((CPUMIPSState *) cpu_env)->active_tc.gpr[26] =3D arg1= ;
> =A0 =A0 =A0 =A0ret =3D 0;
> =A0 =A0 =A0 =A0break;
> =A0#elif defined(TARGET_CRIS)

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

The above thing has been made octeon specific=A0
=A0
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -140,6 +140,20 @@ typedef struct mips_def_t mips_def_t;
> =A0#define MIPS_FPU_MAX 1
> =A0#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. =A0I don't think there= 9;s any
reason why these need to be defined outside TCState, either.

Oc= teon register in TCState as follows
@@ -171,6 +176,15 @@ struct TCState {
=A0=A0 =A0 target_u= long CP0_TCSchedule;
=A0=A0 =A0 target_ulong CP0_TCScheFBack;
=A0=A0 =A0 int32_t CP0_Debu= g_tcstatus;
+ =A0 =A0/* Multipl= ier registers for Octeon */
+ =A0 =A0target_ulong MPL0;
+ =A0 =A0target_ulong MPL1;
+ =A0 =A0target_ulo= ng MPL2;
+ =A0 =A0target_ulong P0;
+ =A0 =A0target_ulong P1;
+ =A0 =A0target_ulo= ng P2;
= + =A0 =A0/* Octeon specific Coprocessor 0 register */
+ =A0 =A0target_ulong cvmctl= ;
=A0};
<= /div>
=A0



> 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 @@
> =A0#define GEN_HELPER 1
> =A0#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<= br> 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
> =A0 =A0 =A0 =A0 See the MIPS64 PRA manual, section 4.10. */
> =A0 =A0 =A0if (((ctx->hflags & MIPS_HFLAG_KSU) =3D=3D MIPS_HFLA= G_UM) &&
> =A0 =A0 =A0 =A0 =A0!(ctx->hflags & MIPS_HFLAG_UX)) {
> - =A0 =A0 =A0 =A0tcg_gen_ext32s_i64(ret, ret);
> + =A0 =A0 =A0 =A0/*This function sign extend 32 bit value to 64 bit, w= as causing
> error
> + =A0 =A0 =A0 =A0 =A0when ld instruction came.Thats why it is commment= ed out*/
> + =A0 =A0 =A0 /* tcg_gen_ext32s_i64(ret, ret);*/
> =A0 =A0 =A0}
> =A0#endif
> =A0}

Um, no. =A0If you needed to comment this out, you have a bug someplac= e
else. =A0Don't paper over the bug here.

=

The = above=A0behavior when =A0ld instruction came was due to env->hflags had<= /font>
=A0not been 'or= ' ed with mips64 user mode flag(MIPS_HFLAG_UX) so now following<= /div>
line is added = in translate.c: cpu_rest()
#ifdef TARGET_= MIPS64
= + =A0 =A0env->hflags |=3D =A0MIPS_HFLAG_UX;
=A0=A0 =A0 if (env->active_fpu.f= cr0 & (1 << FCR0_F64)) {
=A0=A0 =A0 =A0 =A0 = env->hflags |=3D MIPS_HFLAG_F64;
=A0=A0 =A0 }
=A0=A0=A0


> + =A0 =A0 =A0 =A0case OPC_VMULU:
> + =A0 =A0 =A0 =A0case 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
=A0

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 inli= ne void check_octeon(DisasContext *ctx, CPUState *env)
+{
+ =A0 =A0if (!env->T= ARGET_OCTEON)
+ =A0 =A0 =A0 =A0generate_exception(ctx, EXCP_RI);
+}


=A0

> =A0/* Arithmetic */
> =A0static void gen_arith (CPUState *env, DisasContext *ctx, uint32_t o= pc,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int rd, int rs, int rt= )
> =A0{
> =A0 =A0 =A0const char *opn =3D "arith";
>
> + =A0 =A0target_ulong mask =3D 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,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int rd, int rs, int r= t)
> +{
> + =A0 =A0const char *opn =3D "seq/sne";
> + =A0 =A0TCGv t0, t1;
> + =A0 =A0t0 =3D tcg_temp_new();
> + =A0 =A0t1 =3D tcg_temp_new();
> + =A0 =A0switch (opc) {
> + =A0 =A0case OPC_SEQ:
> + =A0 =A0 =A0 =A0{
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_xor_tl(cpu_gpr[rd], cpu_gpr[rs], cpu_= gpr[rt]);
> + =A0 =A0 =A0 =A0 =A0 =A0gen_load_gpr(t0, rd);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_setcondi_tl(TCG_COND_LTU, cpu_gpr[rd]= , t0, 1);
> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0opn =3D "seq";
> + =A0 =A0 =A0 =A0break;

This looks like you are comparing rd with itself?


t0 is actually compared with 1. if t0 <1 then rd =3D1=A0<= /div>
for reference check= slti instruction of mips64r2
switch (opc) {
=A0=A0 =A0case OPC_SLTI:
=A0=A0 =A0 =A0 =A0t= cg_gen_setcondi_tl(TCG_COND_LT, cpu_gpr[rt], t0, uimm);
=A0=A0 =A0 =A0 =A0opn =3D = "slti";



> + =A0 =A0case OPC_SNE:
> + =A0 =A0 =A0 =A0{
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_xor_tl(cpu_gpr[rd], cpu_gpr[rs], cpu_= gpr[rt]);
> + =A0 =A0 =A0 =A0 =A0 =A0gen_load_gpr(t0, rd);
> + =A0 =A0 =A0 =A0 =A0 =A0gen_load_gpr(t1, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_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 !=3D rt, rd has s= ome value greater than 1 (because of=A0
tcg_gen_xor_tl)=A0so = if t1 < t0(in=A0=A0tcg_gen_setcond_tl) rd will be 1. that's wat we n= eeded "set if not
equal"<= /div>
=A0


> +#if defined(TARGET_MIPS64)
> +static void insn_opc_pop (DisasContext *ctx, CPUState *env, uint32_t = opc,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int rd, int rs, i= nt rt)
> +static void insn_opc_dpop (DisasContext *ctx,= CPUState *env, uint32_t opc,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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 *c= tx,
> uint32_t opc,
> =A0 =A0 =A0tcg_temp_free(t0);
> =A0 =A0 =A0tcg_temp_free(t1);
> =A0}
> +/*For cavium specific extract instructions*/
> +#if defined(TARGET_MIPS64)
> +static void gen_exts (CPUState *env,DisasContext *ctx, uint32_t opc, = int
> rt,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int rs, int lsb, int msb)=
> +{
> + =A0 =A0TCGv t0 =3D tcg_temp_new();
> + =A0 =A0TCGv t1 =3D tcg_temp_new();
> + =A0 =A0target_ulong mask;
> + =A0 =A0gen_load_gpr(t1, rs);
> + =A0 =A0switch (opc) {
> + =A0 =A0case OPC_EXTS:
> + =A0 =A0 =A0 =A0tcg_gen_shri_tl(t0, t1, lsb);
> + =A0 =A0 =A0 =A0tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1= );
> + =A0 =A0 =A0 =A0/* To sign extened the remaining bits according to > + =A0 =A0 =A0 =A0 =A0 the msb of the bit field */
> + =A0 =A0 =A0 =A0mask =3D 1ULL << msb;
> + =A0 =A0 =A0 =A0tcg_gen_andi_tl(t1, t0, mask);
> + =A0 =A0 =A0 =A0tcg_gen_addi_tl(t1, t1, -1);
> + =A0 =A0 =A0 =A0tcg_gen_not_i64(t1, t1);
> + =A0 =A0 =A0 =A0tcg_gen_or_tl(t0, t0, t1);

You can use tcg_gen_orc_tl here and elsewhere.

-Nathan

--90e6ba308c6a2c8e0404a0795fd6--