| Message ID | 20200825205950.730499-63-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/microblaze improvements | expand |
On Tue, Aug 25, 2020 at 01:59:35PM -0700, Richard Henderson wrote: > If the last insn on a page is imm, or a branch with delay slot, > then end a tb early if this has not begun the tb. If it has > begun the tb, then we can allow the tb to span two pages as if > the imm plus its consumer, or branch plus delay, or imm plus > branch plus delay, are a single insn. > > If the insn in the delay slot faults, then the exception handler > will have reset the PC to the beginning of this sequence anyway, > via the stored D_FLAG and BIMM_FLAG bits. > > Disable all of this when single-stepping. Hi Richard, We've got a Linux boot that fails after applying this patch. It goes from always working to only working like 1 out of 3 times. It fails deep in user-space so I don't have a good log for it. I guess we'll have to review this patch more. I can share images but the machine is (DTB) dynamic so it only runs with Xilinx QEMU 5.1 branch (I've backported your series for testing). Cheers, Edgar > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/microblaze/translate.c | 65 ++++++++++++++++++++++++++++++----- > 1 file changed, 56 insertions(+), 9 deletions(-) > > diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c > index 4675326083..fcfc1ac184 100644 > --- a/target/microblaze/translate.c > +++ b/target/microblaze/translate.c > @@ -530,11 +530,50 @@ static void gen_idivu(TCGv_i32 out, TCGv_i32 ina, TCGv_i32 inb) > DO_TYPEA_CFG(idiv, use_div, true, gen_idiv) > DO_TYPEA_CFG(idivu, use_div, true, gen_idivu) > > +/* > + * Try to keep the current instruction with the one following. > + * So if this insn is the last in the TB, and is not the first > + * in the TB, and we are not singlestepping, then back up and > + * exit the current TB. > + */ > +static bool wait_for_next_tb(DisasContext *dc) > +{ > + if (dc->base.num_insns >= dc->base.max_insns > + && !dc->base.singlestep_enabled) { > + /* Also consider if this insn (e.g. brid) itself uses an imm. */ > + int ninsns = (dc->tb_flags & IMM_FLAG ? 2 : 1); > + > + /* > + * If this is not the first insn in the TB, back up and > + * start again with a new TB. > + */ > + if (dc->base.num_insns > ninsns) { > + dc->base.pc_next -= ninsns * 4; > + dc->base.num_insns -= ninsns; > + dc->base.is_jmp = DISAS_TOO_MANY; > + return true; > + } > + > + /* > + * Correspondingly, if this is the first insn of the TB, > + * then extend the TB as necessary to keep it with the > + * next insn. Do this by *reducing* the number of insns > + * processed by this TB so that icount does fail an assertion. > + */ > + if (dc->base.num_insns == ninsns) { > + dc->base.num_insns = 0; > + } > + } > + return false; > +} > + > static bool trans_imm(DisasContext *dc, arg_imm *arg) > { > - dc->ext_imm = arg->imm << 16; > - tcg_gen_movi_i32(cpu_imm, dc->ext_imm); > - dc->tb_flags_to_set = IMM_FLAG; > + if (!wait_for_next_tb(dc)) { > + dc->ext_imm = arg->imm << 16; > + tcg_gen_movi_i32(cpu_imm, dc->ext_imm); > + dc->tb_flags_to_set = IMM_FLAG; > + } > return true; > } > > @@ -1311,12 +1350,17 @@ static void eval_cond_jmp(DisasContext *dc, TCGv_i32 pc_true, TCGv_i32 pc_false) > tcg_temp_free_i32(zero); > } > > -static void dec_setup_dslot(DisasContext *dc) > +static bool dec_setup_dslot(DisasContext *dc) > { > + if (wait_for_next_tb(dc)) { > + return true; > + } > + > dc->tb_flags_to_set |= D_FLAG; > if (dc->type_b && (dc->tb_flags & IMM_FLAG)) { > dc->tb_flags_to_set |= BIMM_FLAG; > } > + return false; > } > > static void dec_bcc(DisasContext *dc) > @@ -1327,8 +1371,8 @@ static void dec_bcc(DisasContext *dc) > cc = EXTRACT_FIELD(dc->ir, 21, 23); > dslot = dc->ir & (1 << 25); > > - if (dslot) { > - dec_setup_dslot(dc); > + if (dslot && dec_setup_dslot(dc)) { > + return; > } > > if (dc->type_b) { > @@ -1402,9 +1446,10 @@ static void dec_br(DisasContext *dc) > } > } > > - if (dslot) { > - dec_setup_dslot(dc); > + if (dslot && dec_setup_dslot(dc)) { > + return; > } > + > if (link && dc->rd) { > tcg_gen_movi_i32(cpu_R[dc->rd], dc->base.pc_next); > } > @@ -1513,7 +1558,9 @@ static void dec_rts(DisasContext *dc) > return; > } > > - dec_setup_dslot(dc); > + if (dec_setup_dslot(dc)) { > + return; > + } > > if (i_bit) { > dc->tb_flags |= DRTI_FLAG; > -- > 2.25.1 >
On 8/27/20 12:17 PM, Edgar E. Iglesias wrote: > On Tue, Aug 25, 2020 at 01:59:35PM -0700, Richard Henderson wrote: >> If the last insn on a page is imm, or a branch with delay slot, >> then end a tb early if this has not begun the tb. If it has >> begun the tb, then we can allow the tb to span two pages as if >> the imm plus its consumer, or branch plus delay, or imm plus >> branch plus delay, are a single insn. >> >> If the insn in the delay slot faults, then the exception handler >> will have reset the PC to the beginning of this sequence anyway, >> via the stored D_FLAG and BIMM_FLAG bits. >> >> Disable all of this when single-stepping. > > > Hi Richard, > > We've got a Linux boot that fails after applying this patch. > It goes from always working to only working like 1 out of 3 times. > It fails deep in user-space so I don't have a good log for it. Hmm. Let's just put this off for now. I'm not 100% happy with it anyway, since it adjusts the data structures in weird ways. r~
On 8/27/20 12:17 PM, Edgar E. Iglesias wrote: > On Tue, Aug 25, 2020 at 01:59:35PM -0700, Richard Henderson wrote: >> If the last insn on a page is imm, or a branch with delay slot, >> then end a tb early if this has not begun the tb. If it has >> begun the tb, then we can allow the tb to span two pages as if >> the imm plus its consumer, or branch plus delay, or imm plus >> branch plus delay, are a single insn. >> >> If the insn in the delay slot faults, then the exception handler >> will have reset the PC to the beginning of this sequence anyway, >> via the stored D_FLAG and BIMM_FLAG bits. >> >> Disable all of this when single-stepping. > > > Hi Richard, > > We've got a Linux boot that fails after applying this patch. > It goes from always working to only working like 1 out of 3 times. > It fails deep in user-space so I don't have a good log for it. I think I've found it: do_rti, do_rtb, do_rte are acting as normal jumps. They need to set DISAS_UPDATE so that they return to the main loop to re-evaluate interrupts. I'll come back to this after we deal with the other 76 patches. r~
diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c index 4675326083..fcfc1ac184 100644 --- a/target/microblaze/translate.c +++ b/target/microblaze/translate.c @@ -530,11 +530,50 @@ static void gen_idivu(TCGv_i32 out, TCGv_i32 ina, TCGv_i32 inb) DO_TYPEA_CFG(idiv, use_div, true, gen_idiv) DO_TYPEA_CFG(idivu, use_div, true, gen_idivu) +/* + * Try to keep the current instruction with the one following. + * So if this insn is the last in the TB, and is not the first + * in the TB, and we are not singlestepping, then back up and + * exit the current TB. + */ +static bool wait_for_next_tb(DisasContext *dc) +{ + if (dc->base.num_insns >= dc->base.max_insns + && !dc->base.singlestep_enabled) { + /* Also consider if this insn (e.g. brid) itself uses an imm. */ + int ninsns = (dc->tb_flags & IMM_FLAG ? 2 : 1); + + /* + * If this is not the first insn in the TB, back up and + * start again with a new TB. + */ + if (dc->base.num_insns > ninsns) { + dc->base.pc_next -= ninsns * 4; + dc->base.num_insns -= ninsns; + dc->base.is_jmp = DISAS_TOO_MANY; + return true; + } + + /* + * Correspondingly, if this is the first insn of the TB, + * then extend the TB as necessary to keep it with the + * next insn. Do this by *reducing* the number of insns + * processed by this TB so that icount does fail an assertion. + */ + if (dc->base.num_insns == ninsns) { + dc->base.num_insns = 0; + } + } + return false; +} + static bool trans_imm(DisasContext *dc, arg_imm *arg) { - dc->ext_imm = arg->imm << 16; - tcg_gen_movi_i32(cpu_imm, dc->ext_imm); - dc->tb_flags_to_set = IMM_FLAG; + if (!wait_for_next_tb(dc)) { + dc->ext_imm = arg->imm << 16; + tcg_gen_movi_i32(cpu_imm, dc->ext_imm); + dc->tb_flags_to_set = IMM_FLAG; + } return true; } @@ -1311,12 +1350,17 @@ static void eval_cond_jmp(DisasContext *dc, TCGv_i32 pc_true, TCGv_i32 pc_false) tcg_temp_free_i32(zero); } -static void dec_setup_dslot(DisasContext *dc) +static bool dec_setup_dslot(DisasContext *dc) { + if (wait_for_next_tb(dc)) { + return true; + } + dc->tb_flags_to_set |= D_FLAG; if (dc->type_b && (dc->tb_flags & IMM_FLAG)) { dc->tb_flags_to_set |= BIMM_FLAG; } + return false; } static void dec_bcc(DisasContext *dc) @@ -1327,8 +1371,8 @@ static void dec_bcc(DisasContext *dc) cc = EXTRACT_FIELD(dc->ir, 21, 23); dslot = dc->ir & (1 << 25); - if (dslot) { - dec_setup_dslot(dc); + if (dslot && dec_setup_dslot(dc)) { + return; } if (dc->type_b) { @@ -1402,9 +1446,10 @@ static void dec_br(DisasContext *dc) } } - if (dslot) { - dec_setup_dslot(dc); + if (dslot && dec_setup_dslot(dc)) { + return; } + if (link && dc->rd) { tcg_gen_movi_i32(cpu_R[dc->rd], dc->base.pc_next); } @@ -1513,7 +1558,9 @@ static void dec_rts(DisasContext *dc) return; } - dec_setup_dslot(dc); + if (dec_setup_dslot(dc)) { + return; + } if (i_bit) { dc->tb_flags |= DRTI_FLAG;
If the last insn on a page is imm, or a branch with delay slot, then end a tb early if this has not begun the tb. If it has begun the tb, then we can allow the tb to span two pages as if the imm plus its consumer, or branch plus delay, or imm plus branch plus delay, are a single insn. If the insn in the delay slot faults, then the exception handler will have reset the PC to the beginning of this sequence anyway, via the stored D_FLAG and BIMM_FLAG bits. Disable all of this when single-stepping. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/microblaze/translate.c | 65 ++++++++++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 9 deletions(-) -- 2.25.1