| Message ID | 1534789014-8310-9-git-send-email-aleksandar.markovic@rt-rk.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add nanoMIPS support - core functionality and system mode | expand |
On Mon, Aug 20, 2018 at 8:17 PM Aleksandar Markovic <aleksandar.markovic@rt-rk.com> wrote: > > From: Stefan Markovic <smarkovic@wavecomp.com> > > Add emulation of nanoMIPS 16-bit branch instructions. > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Yongbok Kim <yongbok.kim@mips.com> > Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com> > Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com> > --- > target/mips/translate.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 158 insertions(+) > > diff --git a/target/mips/translate.c b/target/mips/translate.c > index 261680e..b0bbf4c 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -4564,6 +4564,128 @@ static void gen_compute_branch (DisasContext *ctx, uint32_t opc, > tcg_temp_free(t1); > } > > + > +/* nanoMIPS Branches */ > +static void gen_compute_branch_nm(DisasContext *ctx, uint32_t opc, > + int insn_bytes, > + int rs, int rt, int32_t offset) > +{ > + target_ulong btgt = -1; > + int bcond_compute = 0; > + TCGv t0 = tcg_temp_new(); > + TCGv t1 = tcg_temp_new(); > + > + /* Load needed operands */ > + switch (opc) { > + case OPC_BEQ: > + case OPC_BNE: > + /* Compare two registers */ > + if (rs != rt) { > + gen_load_gpr(t0, rs); > + gen_load_gpr(t1, rt); > + bcond_compute = 1; > + } > + btgt = ctx->base.pc_next + insn_bytes + offset; > + break; > + case OPC_BGEZAL: > + /* Compare to zero */ > + if (rs != 0) { > + gen_load_gpr(t0, rs); > + bcond_compute = 1; > + } > + btgt = ctx->base.pc_next + insn_bytes + offset; > + break; > + case OPC_BPOSGE32: > + tcg_gen_andi_tl(t0, cpu_dspctrl, 0x3F); > + bcond_compute = 1; > + btgt = ctx->base.pc_next + insn_bytes + offset; > + break; > + case OPC_JR: > + case OPC_JALR: > + /* Jump to register */ > + if (offset != 0 && offset != 16) { > + /* Hint = 0 is JR/JALR, hint 16 is JR.HB/JALR.HB, the > + others are reserved. */ > + MIPS_INVAL("jump hint"); > + generate_exception_end(ctx, EXCP_RI); > + goto out; > + } > + gen_load_gpr(btarget, rs); > + break; > + default: > + MIPS_INVAL("branch/jump"); > + generate_exception_end(ctx, EXCP_RI); > + goto out; > + } > + if (bcond_compute == 0) { > + /* No condition to be computed */ > + switch (opc) { > + case OPC_BEQ: /* rx == rx */ > + /* Always take */ > + ctx->hflags |= MIPS_HFLAG_B; > + break; > + case OPC_BGEZAL: /* 0 >= 0 */ > + /* Always take and link */ > + tcg_gen_movi_tl(cpu_gpr[31], > + ctx->base.pc_next + insn_bytes); > + ctx->hflags |= MIPS_HFLAG_B; > + break; > + case OPC_BNE: /* rx != rx */ > + tcg_gen_movi_tl(cpu_gpr[31], ctx->base.pc_next + 8); > + /* Skip the instruction in the delay slot */ > + ctx->base.pc_next += 4; > + goto out; > + case OPC_JR: > + ctx->hflags |= MIPS_HFLAG_BR; > + break; > + case OPC_JALR: > + if (rt > 0) { > + tcg_gen_movi_tl(cpu_gpr[rt], > + ctx->base.pc_next + insn_bytes); > + } > + ctx->hflags |= MIPS_HFLAG_BR; > + break; > + default: > + MIPS_INVAL("branch/jump"); > + generate_exception_end(ctx, EXCP_RI); > + goto out; > + } > + } else { > + switch (opc) { > + case OPC_BEQ: > + tcg_gen_setcond_tl(TCG_COND_EQ, bcond, t0, t1); > + goto not_likely; > + case OPC_BNE: > + tcg_gen_setcond_tl(TCG_COND_NE, bcond, t0, t1); > + goto not_likely; > + case OPC_BGEZAL: > + tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 0); > + tcg_gen_movi_tl(cpu_gpr[31], > + ctx->base.pc_next + insn_bytes); > + goto not_likely; > + case OPC_BPOSGE32: > + tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 32); This opcode implementation seems incomplete, per the ISA manual: If a control transfer instruction (CTI) is executed in the forbidden slot of a branch or jump, Release 6 implementations are required to signal a Reserved Instruction Exception. A CTI is considered to be one of the following instructions: branch, jump, NAL (Release 6), ERET, ERETNC (Release 5), DERET, WAIT, or PAUSE (Release 2). An instruction is in the forbidden slot if it is the instruction following the branch. > + not_likely: > + ctx->hflags |= MIPS_HFLAG_BC; > + break; > + default: > + MIPS_INVAL("conditional branch/jump"); > + generate_exception_end(ctx, EXCP_RI); > + goto out; > + } > + } > + > + ctx->btarget = btgt; > + > + out: > + if (insn_bytes == 2) { > + ctx->hflags |= MIPS_HFLAG_B16; > + } > + tcg_temp_free(t0); > + tcg_temp_free(t1); > +}
On 5/29/21 3:52 PM, Philippe Mathieu-Daudé wrote: > On Mon, Aug 20, 2018 at 8:17 PM Aleksandar Markovic > <aleksandar.markovic@rt-rk.com> wrote: >> >> From: Stefan Markovic <smarkovic@wavecomp.com> >> >> Add emulation of nanoMIPS 16-bit branch instructions. >> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Yongbok Kim <yongbok.kim@mips.com> >> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com> >> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com> >> --- >> target/mips/translate.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 158 insertions(+) >> >> diff --git a/target/mips/translate.c b/target/mips/translate.c >> index 261680e..b0bbf4c 100644 >> --- a/target/mips/translate.c >> +++ b/target/mips/translate.c >> @@ -4564,6 +4564,128 @@ static void gen_compute_branch (DisasContext *ctx, uint32_t opc, >> tcg_temp_free(t1); >> } >> >> + >> +/* nanoMIPS Branches */ >> +static void gen_compute_branch_nm(DisasContext *ctx, uint32_t opc, >> + int insn_bytes, >> + int rs, int rt, int32_t offset) >> +{ >> + target_ulong btgt = -1; >> + int bcond_compute = 0; >> + TCGv t0 = tcg_temp_new(); >> + TCGv t1 = tcg_temp_new(); >> + >> + /* Load needed operands */ >> + switch (opc) { >> + case OPC_BEQ: >> + case OPC_BNE: >> + /* Compare two registers */ >> + if (rs != rt) { >> + gen_load_gpr(t0, rs); >> + gen_load_gpr(t1, rt); >> + bcond_compute = 1; >> + } >> + btgt = ctx->base.pc_next + insn_bytes + offset; >> + break; >> + case OPC_BGEZAL: >> + /* Compare to zero */ >> + if (rs != 0) { >> + gen_load_gpr(t0, rs); >> + bcond_compute = 1; >> + } >> + btgt = ctx->base.pc_next + insn_bytes + offset; >> + break; >> + case OPC_BPOSGE32: >> + tcg_gen_andi_tl(t0, cpu_dspctrl, 0x3F); >> + bcond_compute = 1; >> + btgt = ctx->base.pc_next + insn_bytes + offset; I think this opcode never worked correctly. Per the "MIPS® Architecture Extension: nanoMIPS32 DSP Technical Reference Manual — Revision 0.04" p. 88 "BPOSGE32C": "First, the offset argument is left-shifted by one bit to form a 17-bit signed integer value." The caller, decode_nanomips_32_48_opc(), doesn't shift the offset: case NM_BPOSGE32C: check_dsp_r3(ctx); { int32_t imm = extract32(ctx->opcode, 1, 13) | extract32(ctx->opcode, 0, 1) << 13; gen_compute_branch_nm(ctx, OPC_BPOSGE32, 4, -1, -2, imm); } break; >> + break; >> + case OPC_JR: >> + case OPC_JALR: >> + /* Jump to register */ >> + if (offset != 0 && offset != 16) { >> + /* Hint = 0 is JR/JALR, hint 16 is JR.HB/JALR.HB, the >> + others are reserved. */ >> + MIPS_INVAL("jump hint"); >> + generate_exception_end(ctx, EXCP_RI); >> + goto out; >> + } >> + gen_load_gpr(btarget, rs); >> + break; >> + default: >> + MIPS_INVAL("branch/jump"); >> + generate_exception_end(ctx, EXCP_RI); >> + goto out; >> + } >> + if (bcond_compute == 0) { >> + /* No condition to be computed */ >> + switch (opc) { >> + case OPC_BEQ: /* rx == rx */ >> + /* Always take */ >> + ctx->hflags |= MIPS_HFLAG_B; >> + break; >> + case OPC_BGEZAL: /* 0 >= 0 */ >> + /* Always take and link */ >> + tcg_gen_movi_tl(cpu_gpr[31], >> + ctx->base.pc_next + insn_bytes); >> + ctx->hflags |= MIPS_HFLAG_B; >> + break; >> + case OPC_BNE: /* rx != rx */ >> + tcg_gen_movi_tl(cpu_gpr[31], ctx->base.pc_next + 8); >> + /* Skip the instruction in the delay slot */ >> + ctx->base.pc_next += 4; >> + goto out; >> + case OPC_JR: >> + ctx->hflags |= MIPS_HFLAG_BR; >> + break; >> + case OPC_JALR: >> + if (rt > 0) { >> + tcg_gen_movi_tl(cpu_gpr[rt], >> + ctx->base.pc_next + insn_bytes); >> + } >> + ctx->hflags |= MIPS_HFLAG_BR; >> + break; >> + default: >> + MIPS_INVAL("branch/jump"); >> + generate_exception_end(ctx, EXCP_RI); >> + goto out; >> + } >> + } else { >> + switch (opc) { >> + case OPC_BEQ: >> + tcg_gen_setcond_tl(TCG_COND_EQ, bcond, t0, t1); >> + goto not_likely; >> + case OPC_BNE: >> + tcg_gen_setcond_tl(TCG_COND_NE, bcond, t0, t1); >> + goto not_likely; >> + case OPC_BGEZAL: >> + tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 0); >> + tcg_gen_movi_tl(cpu_gpr[31], >> + ctx->base.pc_next + insn_bytes); >> + goto not_likely; >> + case OPC_BPOSGE32: >> + tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 32); > > This opcode implementation seems incomplete, per the ISA manual: > > If a control transfer instruction (CTI) is executed in the forbidden > slot of a branch or jump, Release 6 implementations are required to > signal a Reserved Instruction Exception. A CTI is considered to be one > of the following instructions: branch, jump, NAL (Release 6), ERET, > ERETNC (Release 5), DERET, WAIT, or PAUSE (Release 2). An instruction > is in the forbidden slot if it is the instruction following the > branch. > >> + not_likely: >> + ctx->hflags |= MIPS_HFLAG_BC; >> + break; >> + default: >> + MIPS_INVAL("conditional branch/jump"); >> + generate_exception_end(ctx, EXCP_RI); >> + goto out; >> + } >> + } >> + >> + ctx->btarget = btgt; >> + >> + out: >> + if (insn_bytes == 2) { >> + ctx->hflags |= MIPS_HFLAG_B16; >> + } >> + tcg_temp_free(t0); >> + tcg_temp_free(t1); >> +} >
Hi Philippe, On 5/29/21 3:52 PM, Philippe Mathieu-Daudé wrote: > I think this opcode never worked correctly. > > Per the "MIPS® Architecture Extension: nanoMIPS32 DSP Technical > Reference Manual — Revision 0.04" p. 88 "BPOSGE32C": > > "First, the offset argument is left-shifted by one bit to form > a 17-bit signed integer value." > > The caller, decode_nanomips_32_48_opc(), doesn't shift the offset: > > case NM_BPOSGE32C: > check_dsp_r3(ctx); > { > int32_t imm = extract32(ctx->opcode, 1, 13) | > extract32(ctx->opcode, 0, 1) << 13; > > gen_compute_branch_nm(ctx, OPC_BPOSGE32, 4, -1, -2, > imm); > } > break; You're right. We will prepare a patch very soon. -- Aleksandar R.
Hi Philippe, On 5/29/21 7:50 PM, Philippe Mathieu-Daudé wrote: > On 5/29/21 3:52 PM, Philippe Mathieu-Daudé wrote: >> On Mon, Aug 20, 2018 at 8:17 PM Aleksandar Markovic >> <aleksandar.markovic@rt-rk.com> wrote: >>> >>> From: Stefan Markovic <smarkovic@wavecomp.com> >>> + case OPC_BPOSGE32: >>> + tcg_gen_andi_tl(t0, cpu_dspctrl, 0x3F); >>> + bcond_compute = 1; >>> + btgt = ctx->base.pc_next + insn_bytes + offset; > > I think this opcode never worked correctly. > > Per the "MIPS® Architecture Extension: nanoMIPS32 DSP Technical > Reference Manual — Revision 0.04" p. 88 "BPOSGE32C": > > "First, the offset argument is left-shifted by one bit to form > a 17-bit signed integer value." > > The caller, decode_nanomips_32_48_opc(), doesn't shift the offset: > > case NM_BPOSGE32C: > check_dsp_r3(ctx); > { > int32_t imm = extract32(ctx->opcode, 1, 13) | > extract32(ctx->opcode, 0, 1) << 13; > > gen_compute_branch_nm(ctx, OPC_BPOSGE32, 4, -1, -2, > imm); > } > break; I agree that the left-shift is missing. One must also note that the text in the nanoMIPS32 DSP manual is incorrect. This was most probably copy-pasted from the microMIPS DSP manual. The effective offset for nanoMIPS DSP is supposed to be 15-bit signed, not 17-bit. - farazS
On 5/29/21 7:22 PM, Philippe Mathieu-Daudé wrote: > On Mon, Aug 20, 2018 at 8:17 PM Aleksandar Markovic > <aleksandar.markovic@rt-rk.com> wrote: >> >> From: Stefan Markovic <smarkovic@wavecomp.com> >> >> Add emulation of nanoMIPS 16-bit branch instructions. >> ... >> + /* Compare two registers */ >> + case OPC_BPOSGE32: >> + tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 32); > > This opcode implementation seems incomplete, per the ISA manual: > > If a control transfer instruction (CTI) is executed in the forbidden > slot of a branch or jump, Release 6 implementations are required to > signal a Reserved Instruction Exception. A CTI is considered to be one > of the following instructions: branch, jump, NAL (Release 6), ERET, > ERETNC (Release 5), DERET, WAIT, or PAUSE (Release 2). An instruction > is in the forbidden slot if it is the instruction following the > branch. > This also stems from mistakes in the DSP instruction manual. The description text for BPOSGE32C in the nanoMIPS32 DSP manual has been blindly copy pasted from microMIPS32 DSP manual. As per the nanoMIPS32 Instruction Set Technical Reference Manual, Revision 1.01, Chapter 1, Introduction, p12 [1] : "branch delay slots and forbidden slots have been removed." [1] https://s3-eu-west-1.amazonaws.com/downloads-mips/I7200/I7200+product+launch/MIPS_nanomips32_ISA_TRM_01_01_MD01247.pdf The nanoMIPS architecture is not a revision of MIPS release 6, but an entirely new architecture that happens to have a lot in common with MIPS release 6. Using the same target sources to implement both means we get quite a bit of code reuse, but also that we have to be cognizant of such differences. - farazS
diff --git a/target/mips/translate.c b/target/mips/translate.c index 261680e..b0bbf4c 100644 --- a/target/mips/translate.c +++ b/target/mips/translate.c @@ -4564,6 +4564,128 @@ static void gen_compute_branch (DisasContext *ctx, uint32_t opc, tcg_temp_free(t1); } + +/* nanoMIPS Branches */ +static void gen_compute_branch_nm(DisasContext *ctx, uint32_t opc, + int insn_bytes, + int rs, int rt, int32_t offset) +{ + target_ulong btgt = -1; + int bcond_compute = 0; + TCGv t0 = tcg_temp_new(); + TCGv t1 = tcg_temp_new(); + + /* Load needed operands */ + switch (opc) { + case OPC_BEQ: + case OPC_BNE: + /* Compare two registers */ + if (rs != rt) { + gen_load_gpr(t0, rs); + gen_load_gpr(t1, rt); + bcond_compute = 1; + } + btgt = ctx->base.pc_next + insn_bytes + offset; + break; + case OPC_BGEZAL: + /* Compare to zero */ + if (rs != 0) { + gen_load_gpr(t0, rs); + bcond_compute = 1; + } + btgt = ctx->base.pc_next + insn_bytes + offset; + break; + case OPC_BPOSGE32: + tcg_gen_andi_tl(t0, cpu_dspctrl, 0x3F); + bcond_compute = 1; + btgt = ctx->base.pc_next + insn_bytes + offset; + break; + case OPC_JR: + case OPC_JALR: + /* Jump to register */ + if (offset != 0 && offset != 16) { + /* Hint = 0 is JR/JALR, hint 16 is JR.HB/JALR.HB, the + others are reserved. */ + MIPS_INVAL("jump hint"); + generate_exception_end(ctx, EXCP_RI); + goto out; + } + gen_load_gpr(btarget, rs); + break; + default: + MIPS_INVAL("branch/jump"); + generate_exception_end(ctx, EXCP_RI); + goto out; + } + if (bcond_compute == 0) { + /* No condition to be computed */ + switch (opc) { + case OPC_BEQ: /* rx == rx */ + /* Always take */ + ctx->hflags |= MIPS_HFLAG_B; + break; + case OPC_BGEZAL: /* 0 >= 0 */ + /* Always take and link */ + tcg_gen_movi_tl(cpu_gpr[31], + ctx->base.pc_next + insn_bytes); + ctx->hflags |= MIPS_HFLAG_B; + break; + case OPC_BNE: /* rx != rx */ + tcg_gen_movi_tl(cpu_gpr[31], ctx->base.pc_next + 8); + /* Skip the instruction in the delay slot */ + ctx->base.pc_next += 4; + goto out; + case OPC_JR: + ctx->hflags |= MIPS_HFLAG_BR; + break; + case OPC_JALR: + if (rt > 0) { + tcg_gen_movi_tl(cpu_gpr[rt], + ctx->base.pc_next + insn_bytes); + } + ctx->hflags |= MIPS_HFLAG_BR; + break; + default: + MIPS_INVAL("branch/jump"); + generate_exception_end(ctx, EXCP_RI); + goto out; + } + } else { + switch (opc) { + case OPC_BEQ: + tcg_gen_setcond_tl(TCG_COND_EQ, bcond, t0, t1); + goto not_likely; + case OPC_BNE: + tcg_gen_setcond_tl(TCG_COND_NE, bcond, t0, t1); + goto not_likely; + case OPC_BGEZAL: + tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 0); + tcg_gen_movi_tl(cpu_gpr[31], + ctx->base.pc_next + insn_bytes); + goto not_likely; + case OPC_BPOSGE32: + tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 32); + not_likely: + ctx->hflags |= MIPS_HFLAG_BC; + break; + default: + MIPS_INVAL("conditional branch/jump"); + generate_exception_end(ctx, EXCP_RI); + goto out; + } + } + + ctx->btarget = btgt; + + out: + if (insn_bytes == 2) { + ctx->hflags |= MIPS_HFLAG_B16; + } + tcg_temp_free(t0); + tcg_temp_free(t1); +} + + /* special3 bitfield operations */ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt, int rs, int lsb, int msb) @@ -16729,14 +16851,50 @@ static int decode_nanomips_opc(CPUMIPSState *env, DisasContext *ctx) case NM_SWGP16: break; case NM_BC16: + gen_compute_branch_nm(ctx, OPC_BEQ, 2, 0, 0, + (sextract32(ctx->opcode, 0, 1) << 10) | + (extract32(ctx->opcode, 1, 9) << 1)); break; case NM_BALC16: + gen_compute_branch_nm(ctx, OPC_BGEZAL, 2, 0, 0, + (sextract32(ctx->opcode, 0, 1) << 10) | + (extract32(ctx->opcode, 1, 9) << 1)); break; case NM_BEQZC16: + gen_compute_branch_nm(ctx, OPC_BEQ, 2, rt, 0, + (sextract32(ctx->opcode, 0, 1) << 7) | + (extract32(ctx->opcode, 1, 6) << 1)); break; case NM_BNEZC16: + gen_compute_branch_nm(ctx, OPC_BNE, 2, rt, 0, + (sextract32(ctx->opcode, 0, 1) << 7) | + (extract32(ctx->opcode, 1, 6) << 1)); break; case NM_P16_BR: + switch (ctx->opcode & 0xf) { + case 0: + /* P16.JRC */ + switch (extract32(ctx->opcode, 4, 1)) { + case NM_JRC: + gen_compute_branch_nm(ctx, OPC_JR, 2, + extract32(ctx->opcode, 5, 5), 0, 0); + break; + case NM_JALRC16: + gen_compute_branch_nm(ctx, OPC_JALR, 2, + extract32(ctx->opcode, 5, 5), 31, 0); + break; + } + break; + default: + { + /* P16.BRI */ + uint32_t opc = extract32(ctx->opcode, 4, 3) < + extract32(ctx->opcode, 7, 3) ? OPC_BEQ : OPC_BNE; + gen_compute_branch_nm(ctx, opc, 2, rs, rt, + extract32(ctx->opcode, 0, 4) << 1); + } + break; + } break; case NM_P16_SR: break;