Re: [Qemu-devel] [PATCH 2/3] target-mips:Adding Octeon cpu definitions

[Peter Maydell]

I'm not a MIPS expert, but some attempt at review anyway...

On 5 July 2011 10:19,  <address@hidden> 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