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.
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.
> 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.
> --- 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.
> 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.
> + case OPC_VMULU:
> + case OPC_V3MULU:
These two are large enough that they should be done as out-of-line
helpers.
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.
> /* 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?
> + 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.
> +#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
