I'm not a MIPS expert, but some attempt at review anyway... On 5 July 2011 10:19, <kha...@kics.edu.pk> wrote: > diff --git a/target-mips/mips-defs.h b/target-mips/mips-defs.h > index bf094a3..6fec935 100644 > --- a/target-mips/mips-defs.h > +++ b/target-mips/mips-defs.h > @@ -44,6 +44,7 @@ > #define INSN_LOONGSON2E 0x20000000 > #define INSN_LOONGSON2F 0x40000000 > #define INSN_VR54XX 0x80000000 > +#define INSN_OCTEON 0x10000000
...why not put this at the top of this set of INSN defines so they are in numerical order? > /* MIPS CPU defines. */ > #define CPU_MIPS1 (ISA_MIPS1) > @@ -53,6 +54,7 @@ > #define CPU_VR54XX (CPU_MIPS4 | INSN_VR54XX) > #define CPU_LOONGSON2E (CPU_MIPS3 | INSN_LOONGSON2E) > #define CPU_LOONGSON2F (CPU_MIPS3 | INSN_LOONGSON2F) > +#define CPU_OCTEON (CPU_MIPS64R2 | INSN_OCTEON) You're using this define in the linux-user patch, which means you have your patches in the wrong order. The code has to compile at every point after each individual patch, not just after the entire set has been applied. (Also the linux-user patch doesn't apply to master any more so you'll need to rebase it anyway.) > #define CPU_MIPS5 (CPU_MIPS4 | ISA_MIPS5) > > diff --git a/target-mips/translate.c b/target-mips/translate.c > index 2848c6a..eb108bc 100644 > --- a/target-mips/translate.c > +++ b/target-mips/translate.c > @@ -12693,6 +12693,7 @@ void cpu_reset (CPUMIPSState *env) > env->hflags |= MIPS_HFLAG_FPU; > } > #ifdef TARGET_MIPS64 > + env->hflags |= MIPS_HFLAG_UX; > if (env->active_fpu.fcr0 & (1 << FCR0_F64)) { > env->hflags |= MIPS_HFLAG_F64; > } This change looks suspiciously like a lurking bug fix not related to adding the Octeon CPU definition. My guess is that it should probably be in its own patch with a proper justification. -- PMM