| Message ID | 20250501125544.727038-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Don't assert() for ISB/SB inside IT block | expand |
On 5/1/25 05:55, Peter Maydell wrote: > (NB: the TCG optimizer doesn't optimize out the jump-to-next, but > we can't really avoid emitting it because we don't know at the > point we're emitting the handling for the condexec check whether > this insn is going to happen to be a nop for us or not.) Heh. For some reason I only fold unconditional branch-to-next. I'll fix this. Anyway, > > Cc: qemu-stable@nongnu.org > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2942 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/tcg/translate.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c > index 88df9c482ab..e773ab72685 100644 > --- a/target/arm/tcg/translate.c > +++ b/target/arm/tcg/translate.c > @@ -7760,7 +7760,8 @@ static bool arm_check_ss_active(DisasContext *dc) > > static void arm_post_translate_insn(DisasContext *dc) > { > - if (dc->condjmp && dc->base.is_jmp == DISAS_NEXT) { > + if (dc->condjmp && > + (dc->base.is_jmp == DISAS_NEXT || dc->base.is_jmp == DISAS_TOO_MANY)) { > if (dc->pc_save != dc->condlabel.pc_save) { > gen_update_pc(dc, dc->condlabel.pc_save - dc->pc_save); > } Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 01.05.2025 15:55, Peter Maydell wrote: > If the guest code has an ISB or SB insn inside an IT block, we > generate incorrect code which trips a TCG assertion: > > qemu-system-arm: ../tcg/tcg-op.c:3343: void tcg_gen_goto_tb(unsigned int): Assertion `(tcg_ctx->goto_tb_issue_mask & (1 << idx)) == 0' failed. > > This is because we call gen_goto_tb(dc, 1, ...) twice: > > brcond_i32 ZF,$0x0,ne,$L1 > add_i32 pc,pc,$0x4 > goto_tb $0x1 > exit_tb $0x73d948001b81 > set_label $L1 > add_i32 pc,pc,$0x4 > goto_tb $0x1 > exit_tb $0x73d948001b81 > > Both calls are in arm_tr_tb_stop(), one for the > DISAS_NEXT/DISAS_TOO_MANY handling, and one for the dc->condjump > condition-failed codepath. The DISAS_NEXT handling doesn't have this > problem because arm_post_translate_insn() does the handling of "emit > the label for the condition-failed conditional execution" and so > arm_tr_tb_stop() doesn't have dc->condjump set. But for > DISAS_TOO_MANY we don't do that. > > Fix the bug by making arm_post_translate_insn() handle the > DISAS_TOO_MANY case. This only affects the SB and ISB insns when > used in Thumb mode inside an IT block: only these insns specifically > set is_jmp to TOO_MANY, and their A32 encodings are unconditional. > > For the major TOO_MANY case (breaking the TB because it would cross a > page boundary) we do that check and set is_jmp to TOO_MANY only after > the call to arm_post_translate_insn(); so arm_post_translate_insn() > sees is_jmp == DISAS_NEXT, and we emit the correct code for that > situation. > > With this fix we generate the somewhat more sensible set of TCG ops: > brcond_i32 ZF,$0x0,ne,$L1 > set_label $L1 > add_i32 pc,pc,$0x4 > goto_tb $0x1 > exit_tb $0x7c5434001b81 > > (NB: the TCG optimizer doesn't optimize out the jump-to-next, but > we can't really avoid emitting it because we don't know at the > point we're emitting the handling for the condexec check whether > this insn is going to happen to be a nop for us or not.) > > Cc: qemu-stable@nongnu.org > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2942 Hi! Is this change applicable for older stable releases, besides 10.0 (currently 9.2 and 7.2)? It applies cleanly but I wonder if it is actually needed.. Thanks, /mjt
On Thu, 8 May 2025 at 08:31, Michael Tokarev <mjt@tls.msk.ru> wrote: > > On 01.05.2025 15:55, Peter Maydell wrote: > > If the guest code has an ISB or SB insn inside an IT block, we > > generate incorrect code which trips a TCG assertion: > Is this change applicable for older stable releases, besides 10.0 > (currently 9.2 and 7.2)? It applies cleanly but I wonder if it is > actually needed.. I think any branch with commit 73fce314dbbf2d ("target/arm: Use DISAS_TOO_MANY for ISB and SB") needs the fix. That would include both 9.2 and 7.2, unless I got my git rune wrong. thanks -- PMM
diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c index 88df9c482ab..e773ab72685 100644 --- a/target/arm/tcg/translate.c +++ b/target/arm/tcg/translate.c @@ -7760,7 +7760,8 @@ static bool arm_check_ss_active(DisasContext *dc) static void arm_post_translate_insn(DisasContext *dc) { - if (dc->condjmp && dc->base.is_jmp == DISAS_NEXT) { + if (dc->condjmp && + (dc->base.is_jmp == DISAS_NEXT || dc->base.is_jmp == DISAS_TOO_MANY)) { if (dc->pc_save != dc->condlabel.pc_save) { gen_update_pc(dc, dc->condlabel.pc_save - dc->pc_save); }
If the guest code has an ISB or SB insn inside an IT block, we generate incorrect code which trips a TCG assertion: qemu-system-arm: ../tcg/tcg-op.c:3343: void tcg_gen_goto_tb(unsigned int): Assertion `(tcg_ctx->goto_tb_issue_mask & (1 << idx)) == 0' failed. This is because we call gen_goto_tb(dc, 1, ...) twice: brcond_i32 ZF,$0x0,ne,$L1 add_i32 pc,pc,$0x4 goto_tb $0x1 exit_tb $0x73d948001b81 set_label $L1 add_i32 pc,pc,$0x4 goto_tb $0x1 exit_tb $0x73d948001b81 Both calls are in arm_tr_tb_stop(), one for the DISAS_NEXT/DISAS_TOO_MANY handling, and one for the dc->condjump condition-failed codepath. The DISAS_NEXT handling doesn't have this problem because arm_post_translate_insn() does the handling of "emit the label for the condition-failed conditional execution" and so arm_tr_tb_stop() doesn't have dc->condjump set. But for DISAS_TOO_MANY we don't do that. Fix the bug by making arm_post_translate_insn() handle the DISAS_TOO_MANY case. This only affects the SB and ISB insns when used in Thumb mode inside an IT block: only these insns specifically set is_jmp to TOO_MANY, and their A32 encodings are unconditional. For the major TOO_MANY case (breaking the TB because it would cross a page boundary) we do that check and set is_jmp to TOO_MANY only after the call to arm_post_translate_insn(); so arm_post_translate_insn() sees is_jmp == DISAS_NEXT, and we emit the correct code for that situation. With this fix we generate the somewhat more sensible set of TCG ops: brcond_i32 ZF,$0x0,ne,$L1 set_label $L1 add_i32 pc,pc,$0x4 goto_tb $0x1 exit_tb $0x7c5434001b81 (NB: the TCG optimizer doesn't optimize out the jump-to-next, but we can't really avoid emitting it because we don't know at the point we're emitting the handling for the condexec check whether this insn is going to happen to be a nop for us or not.) Cc: qemu-stable@nongnu.org Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2942 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/tcg/translate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)