| Message ID | 20180830193019.20104-4-jancraig@amazon.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add limited MXU instruction support | expand |
Hi, Craig, > From: Craig Janeczek <jancraig@amazon.com> > Sent: Thursday, August 30, 2018 9:30 PM > To: qemu-devel@nongnu.org > Cc: Aleksandar Markovic; aurelien@aurel32.net; Craig Janeczek > Subject: [PATCH v4 3/9] target/mips: Split mips instruction handling > > Splits the instruction handling switch statement from the original > legacy code. > > Signed-off-by: Craig Janeczek <jancraig@amazon.com> > --- > v1 > - NA > v2 > - NA > v3 > - NA > v4 > - Initial patch > > target/mips/mips-defs.h | 1 + > target/mips/translate.c | 28 +++++++++++++++++++++++++++- > 2 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h > index d239069975..5a409757f0 100644 > --- a/target/mips/mips-defs.h > +++ b/target/mips/mips-defs.h > @@ -50,6 +50,7 @@ > #define ASE_SMARTMIPS 0x00400000 > #define ASE_MICROMIPS 0x00800000 > #define ASE_MSA 0x01000000 > +#define ASE_MXU 0x02000000 > > /* Chip specific instructions. */ > #define INSN_LOONGSON2E 0x20000000 > diff --git a/target/mips/translate.c b/target/mips/translate.c > index a598f45558..53d896ebf9 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -17855,6 +17855,28 @@ static void decode_opc_special(CPUMIPSState *env, DisasContext *ctx) > } > } > > +static void decode_opc_special2_mxu(CPUMIPSState *env, DisasContext *ctx) > +{ > + int rs, rt, rd; > + uint32_t op1; > + > + rs = (ctx->opcode >> 21) & 0x1f; > + rt = (ctx->opcode >> 16) & 0x1f; > + rd = (ctx->opcode >> 11) & 0x1f; > + > + op1 = MASK_SPECIAL2(ctx->opcode); > + > + switch (op1) { > + case OPC_MUL: > + gen_arith(ctx, op1, rd, rs, rt); > + break; > + default: /* Invalid */ > + MIPS_INVAL("special2_mxu"); > + generate_exception_end(ctx, EXCP_RI); > + break; > + } > +} > + This (case OPC_MUL) just looks very odd to me. Why would OPC_MUL somehow be supposed to be included here? Is there any documentation to support this? For example of other kind: OPC_MADD is not included in this switch, but there is an OPC_MADD equivalent in MXU. At the same time, there is an OPC_MUL equivalent in MXU too. This looks to me as a very unclear opcode organization. Too bad the MXU documentation that you linked to doesn't have opcode specifications. Xburst base set documentation would be very helpful, but there is no such doc to my knowledge. Sincerely, Aleksandar > static void decode_opc_special2_legacy(CPUMIPSState *env, DisasContext *ctx) > { > int rs, rt, rd; > @@ -19836,7 +19858,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > decode_opc_special(env, ctx); > break; > case OPC_SPECIAL2: > - decode_opc_special2_legacy(env, ctx); > + if (ctx->insn_flags & ASE_MXU) { > + decode_opc_special2_mxu(env, ctx); > + } else { > + decode_opc_special2_legacy(env, ctx); > + } > break; > case OPC_SPECIAL3: > decode_opc_special3(env, ctx); > -- > 2.18.0 >
To clarify the OPC_MUL here is not an MXU instruction, this is the original OPC_MUL that was in the special2 instruction set. The inclusion of this instruction in this switch statement is due to the suggested method of splitting up the mxu commands instruction handling switch statement from the original special2 commands. Since there is no MXU command with the opcode suffix of 0x02 there was not an instruction collision. Your other example is not correct as there is an MXU instruction sharing the opcode suffix 0x00 (OPC_MXU_S32MADD) therefore the original OPC_MUL would not be used. Remember that I did not arbitrarily make this instruction mapping, I just implemented the list of MXU opcodes. The confusion stems from the fact that these opcodes overlap with pre-existing instructions and do not consistently map original instruction to MXU instruction. I have not been able to find a document to back this up. The only evidence I have is the existence of the OPC_MUL instruction in an MXU compiled binary. -----Original Message----- From: Aleksandar Markovic <amarkovic@wavecomp.com> Sent: Friday, August 31, 2018 2:40 PM To: Janeczek, Craig <jancraig@amazon.com>; qemu-devel@nongnu.org Cc: aurelien@aurel32.net; Petar Jovanovic <pjovanovic@wavecomp.com>; Richard Henderson <richard.henderson@linaro.org> Subject: Re: [PATCH v4 3/9] target/mips: Split mips instruction handling Hi, Craig, > From: Craig Janeczek <jancraig@amazon.com> > Sent: Thursday, August 30, 2018 9:30 PM > To: qemu-devel@nongnu.org > Cc: Aleksandar Markovic; aurelien@aurel32.net; Craig Janeczek > Subject: [PATCH v4 3/9] target/mips: Split mips instruction handling > > Splits the instruction handling switch statement from the original > legacy code. > > Signed-off-by: Craig Janeczek <jancraig@amazon.com> > --- > v1 > - NA > v2 > - NA > v3 > - NA > v4 > - Initial patch > > target/mips/mips-defs.h | 1 + > target/mips/translate.c | 28 +++++++++++++++++++++++++++- > 2 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h index > d239069975..5a409757f0 100644 > --- a/target/mips/mips-defs.h > +++ b/target/mips/mips-defs.h > @@ -50,6 +50,7 @@ > #define ASE_SMARTMIPS 0x00400000 > #define ASE_MICROMIPS 0x00800000 > #define ASE_MSA 0x01000000 > +#define ASE_MXU 0x02000000 > > /* Chip specific instructions. */ > #define INSN_LOONGSON2E 0x20000000 > diff --git a/target/mips/translate.c b/target/mips/translate.c index > a598f45558..53d896ebf9 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -17855,6 +17855,28 @@ static void decode_opc_special(CPUMIPSState *env, DisasContext *ctx) > } > } > > +static void decode_opc_special2_mxu(CPUMIPSState *env, DisasContext > +*ctx) { > + int rs, rt, rd; > + uint32_t op1; > + > + rs = (ctx->opcode >> 21) & 0x1f; > + rt = (ctx->opcode >> 16) & 0x1f; > + rd = (ctx->opcode >> 11) & 0x1f; > + > + op1 = MASK_SPECIAL2(ctx->opcode); > + > + switch (op1) { > + case OPC_MUL: > + gen_arith(ctx, op1, rd, rs, rt); > + break; > + default: /* Invalid */ > + MIPS_INVAL("special2_mxu"); > + generate_exception_end(ctx, EXCP_RI); > + break; > + } > +} > + This (case OPC_MUL) just looks very odd to me. Why would OPC_MUL somehow be supposed to be included here? Is there any documentation to support this? For example of other kind: OPC_MADD is not included in this switch, but there is an OPC_MADD equivalent in MXU. At the same time, there is an OPC_MUL equivalent in MXU too. This looks to me as a very unclear opcode organization. Too bad the MXU documentation that you linked to doesn't have opcode specifications. Xburst base set documentation would be very helpful, but there is no such doc to my knowledge. Sincerely, Aleksandar > static void decode_opc_special2_legacy(CPUMIPSState *env, > DisasContext *ctx) { > int rs, rt, rd; > @@ -19836,7 +19858,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > decode_opc_special(env, ctx); > break; > case OPC_SPECIAL2: > - decode_opc_special2_legacy(env, ctx); > + if (ctx->insn_flags & ASE_MXU) { > + decode_opc_special2_mxu(env, ctx); > + } else { > + decode_opc_special2_legacy(env, ctx); > + } > break; > case OPC_SPECIAL3: > decode_opc_special3(env, ctx); > -- > 2.18.0 >
> From: Janeczek, Craig <jancraig@amazon.com> > Sent: Tuesday, September 4, 2018 4:44 PM > > Subject: RE: [PATCH v4 3/9] target/mips: Split mips instruction handling > > To clarify the OPC_MUL here is not an MXU instruction, this is the original OPC_MUL that was in the special2 instruction set. The inclusion of this instruction in this switch statement is due to the suggested method of splitting up the mxu commands instruction handling switch statement from the original special2 commands. There are five more cases where current SPECIAL2 instructions occupy free slots in MXU opcode scheme: /* Loongson 2F */ OPC_MODU_G_2F = 0x1e | OPC_SPECIAL2, OPC_DMODU_G_2F = 0x1f | OPC_SPECIAL2, /* Misc */ OPC_CLZ = 0x20 | OPC_SPECIAL2, OPC_CLO = 0x21 | OPC_SPECIAL2, /* Special */ OPC_SDBBP = 0x3F | OPC_SPECIAL2, What to do with them? Should they be treated like OPC_MUL? Can you do the same binary check as for OPC_MUL? Is there a confirmation in Ingenic gcc/asm source for all these cases? Thanks, Aleksandar
> From: Janeczek, Craig <jancraig@amazon.com> > Sent: Tuesday, September 4, 2018 4:44 PM > > Subject: RE: [PATCH v4 3/9] target/mips: Split mips instruction handling > > To clarify the OPC_MUL here is not an MXU instruction, this is the original OPC_MUL that was in the special2 instruction set. The inclusion of this instruction in this switch statement is due to the suggested method of splitting up the mxu commands instruction handling switch statement from the original special2 commands. In any case, handling OPC_MUL (and others similar cases if it turns out to be needed) in such way should be in a separate patch in this series, and backed by, at the least, references to the Ingenic source code of gcc/asm or similar utilities. Thanks, Aleksandar
diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h index d239069975..5a409757f0 100644 --- a/target/mips/mips-defs.h +++ b/target/mips/mips-defs.h @@ -50,6 +50,7 @@ #define ASE_SMARTMIPS 0x00400000 #define ASE_MICROMIPS 0x00800000 #define ASE_MSA 0x01000000 +#define ASE_MXU 0x02000000 /* Chip specific instructions. */ #define INSN_LOONGSON2E 0x20000000 diff --git a/target/mips/translate.c b/target/mips/translate.c index a598f45558..53d896ebf9 100644 --- a/target/mips/translate.c +++ b/target/mips/translate.c @@ -17855,6 +17855,28 @@ static void decode_opc_special(CPUMIPSState *env, DisasContext *ctx) } } +static void decode_opc_special2_mxu(CPUMIPSState *env, DisasContext *ctx) +{ + int rs, rt, rd; + uint32_t op1; + + rs = (ctx->opcode >> 21) & 0x1f; + rt = (ctx->opcode >> 16) & 0x1f; + rd = (ctx->opcode >> 11) & 0x1f; + + op1 = MASK_SPECIAL2(ctx->opcode); + + switch (op1) { + case OPC_MUL: + gen_arith(ctx, op1, rd, rs, rt); + break; + default: /* Invalid */ + MIPS_INVAL("special2_mxu"); + generate_exception_end(ctx, EXCP_RI); + break; + } +} + static void decode_opc_special2_legacy(CPUMIPSState *env, DisasContext *ctx) { int rs, rt, rd; @@ -19836,7 +19858,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) decode_opc_special(env, ctx); break; case OPC_SPECIAL2: - decode_opc_special2_legacy(env, ctx); + if (ctx->insn_flags & ASE_MXU) { + decode_opc_special2_mxu(env, ctx); + } else { + decode_opc_special2_legacy(env, ctx); + } break; case OPC_SPECIAL3: decode_opc_special3(env, ctx);
Splits the instruction handling switch statement from the original legacy code. Signed-off-by: Craig Janeczek <jancraig@amazon.com> --- v1 - NA v2 - NA v3 - NA v4 - Initial patch target/mips/mips-defs.h | 1 + target/mips/translate.c | 28 +++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-)