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