Move delayed branch handling to tb_stop, where we can re-use other
end-of-tb code, e.g. the evaluation of flags. Honor single stepping.
Validate that we aren't losing state by overwriting is_jmp.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/cris/translate.c | 96 ++++++++++++++++++++++++-----------------
1 file changed, 56 insertions(+), 40 deletions(-)
diff --git a/target/cris/translate.c b/target/cris/translate.c
index c9822eae4c..f58f6f2e5e 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -61,6 +61,8 @@
#define DISAS_UPDATE DISAS_TARGET_1
/* Cpu state was modified dynamically, excluding pc -- use npc */
#define DISAS_UPDATE_NEXT DISAS_TARGET_2
+/* PC update for delayed branch, see cpustate_changed otherwise */
+#define DISAS_DBRANCH DISAS_TARGET_3
/* Used by the decoder. */
#define EXTRACT_FIELD(src, start, end) \
@@ -3228,50 +3230,22 @@ static void cris_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
dc->cpustate_changed |= dc->flags_x != (dc->base.tb->flags & X_FLAG);
/*
- * Check for delayed branches here. If we do it before
- * actually generating any host code, the simulator will just
- * loop doing nothing for on this program location.
+ * All branches are delayed branches, handled immediately below.
+ * We don't expect to see odd combinations of exit conditions.
*/
+ assert(dc->base.is_jmp == DISAS_NEXT || dc->cpustate_changed);
+
if (dc->delayed_branch && --dc->delayed_branch == 0) {
- if (dc->base.tb->flags & 7) {
- t_gen_movi_env_TN(dslot, 0);
- }
+ dc->base.is_jmp = DISAS_DBRANCH;
+ return;
+ }
- if (dc->cpustate_changed) {
- cris_store_direct_jmp(dc);
- }
-
- if (dc->clear_locked_irq) {
- dc->clear_locked_irq = 0;
- t_gen_movi_env_TN(locked_irq, 0);
- }
-
- if (dc->jmp == JMP_DIRECT_CC) {
- TCGLabel *l1 = gen_new_label();
- cris_evaluate_flags(dc);
-
- /* Conditional jmp. */
- tcg_gen_brcondi_tl(TCG_COND_EQ, env_btaken, 0, l1);
- gen_goto_tb(dc, 1, dc->jmp_pc);
- gen_set_label(l1);
- gen_goto_tb(dc, 0, dc->pc);
- dc->base.is_jmp = DISAS_NORETURN;
- dc->jmp = JMP_NOJMP;
- } else if (dc->jmp == JMP_DIRECT) {
- cris_evaluate_flags(dc);
- gen_goto_tb(dc, 0, dc->jmp_pc);
- dc->base.is_jmp = DISAS_NORETURN;
- dc->jmp = JMP_NOJMP;
- } else {
- TCGv c = tcg_const_tl(dc->pc);
- t_gen_cc_jmp(env_btarget, c);
- tcg_temp_free(c);
- dc->base.is_jmp = DISAS_JUMP;
- }
+ if (dc->base.is_jmp != DISAS_NEXT) {
+ return;
}
/* Force an update if the per-tb cpu state has changed. */
- if (dc->base.is_jmp == DISAS_NEXT && dc->cpustate_changed) {
+ if (dc->cpustate_changed) {
dc->base.is_jmp = DISAS_UPDATE_NEXT;
return;
}
@@ -3281,8 +3255,7 @@ static void cris_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
* If we can detect the length of the next insn easily, we should.
* In the meantime, simply stop when we do cross.
*/
- if (dc->base.is_jmp == DISAS_NEXT
- && ((dc->pc ^ dc->base.pc_first) & TARGET_PAGE_MASK) != 0) {
+ if ((dc->pc ^ dc->base.pc_first) & TARGET_PAGE_MASK) {
dc->base.is_jmp = DISAS_TOO_MANY;
}
}
@@ -3312,6 +3285,49 @@ static void cris_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
cris_evaluate_flags(dc);
+ /* Evaluate delayed branch destination and fold to another is_jmp case. */
+ if (is_jmp == DISAS_DBRANCH) {
+ if (dc->base.tb->flags & 7) {
+ t_gen_movi_env_TN(dslot, 0);
+ }
+
+ switch (dc->jmp) {
+ case JMP_DIRECT:
+ npc = dc->jmp_pc;
+ is_jmp = dc->cpustate_changed ? DISAS_UPDATE_NEXT : DISAS_TOO_MANY;
+ break;
+
+ case JMP_DIRECT_CC:
+ /*
+ * Use a conditional branch if either taken or not-taken path
+ * can use goto_tb. If neither can, then treat it as indirect.
+ */
+ if (likely(!dc->base.singlestep_enabled)
+ && likely(!dc->cpustate_changed)
+ && (use_goto_tb(dc, dc->jmp_pc) || use_goto_tb(dc, npc))) {
+ TCGLabel *not_taken = gen_new_label();
+
+ tcg_gen_brcondi_tl(TCG_COND_EQ, env_btaken, 0, not_taken);
+ gen_goto_tb(dc, 1, dc->jmp_pc);
+ gen_set_label(not_taken);
+
+ /* not-taken case handled below. */
+ is_jmp = DISAS_TOO_MANY;
+ break;
+ }
+ tcg_gen_movi_tl(env_btarget, dc->jmp_pc);
+ /* fall through */
+
+ case JMP_INDIRECT:
+ t_gen_cc_jmp(env_btarget, tcg_constant_tl(npc));
+ is_jmp = dc->cpustate_changed ? DISAS_UPDATE : DISAS_JUMP;
+ break;
+
+ default:
+ g_assert_not_reached();
+ }
+ }
+
if (unlikely(dc->base.singlestep_enabled)) {
switch (is_jmp) {
case DISAS_TOO_MANY:
--
2.25.1
On Tue, Jun 22, 2021 at 08:48:16AM -0700, Richard Henderson wrote:
> Move delayed branch handling to tb_stop, where we can re-use other
> end-of-tb code, e.g. the evaluation of flags. Honor single stepping.
> Validate that we aren't losing state by overwriting is_jmp.
Hi Richard,
This patch breaks my kernel boot test:
edgar@zapote:cris-axisdev88$ ./qemu-run.sh
+ MACH=-M axis-dev88
+ QEMU_BUILD_PATH=/home/edgar/src/c/qemu/build-qemu/
+ QEMU=/home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris
+ KERNEL=-kernel kimage
+ NIC0=-netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0
+ /home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris -M axis-dev88 -netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0 -serial stdio -display none -kernel kimage
Linux version 2.6.33 (edgar@edde) (gcc version 4.3.1 20080521 (prerelease) [gcc-4_3-branch revision 135713] (GCC 4.3.1 Axis release R93/1.93) ) #4 Thu Jan 13 15:11:20 CET 2011
bootconsole [early0] enabled
ROM fs in RAM, size 6946816 bytes
Setting up paging and the MMU.
Linux/CRISv32 port on ETRAX FS (C) 2003, 2004 Axis Communications AB
Built 1 zonelists in Zone order, mobility grouping on. Total pages: 4080
Kernel command line: root=/dev/mtdblock3 init=/linuxrc rootfstype=jffs2 mmc_core.use_spi_crc=0 mmc_spi.spi_mode=3
PID hash table entries: 128 (order: -4, 512 bytes)
Dentry cache hash table entries: 4096 (order: 1, 16384 bytes)
Inode-cache hash table entries: 2048 (order: 0, 8192 bytes)
Memory: 22864k/32768k available (2260k kernel code, 9904k reserved, 504k data, 80k init)
Hierarchical RCU implementation.
NR_IRQS:80
Enabling watchdog...
Calibrating delay loop... qemu-system-cris: ../qemu/target/cris/translate.c:3236: cris_tr_translate_insn: Assertion `dc->base.is_jmp == DISAS_NEXT || dc->cpustate_changed' failed.
Aborted (core dumped)
I can share the image if you like.
Best regards,
Edgar
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/cris/translate.c | 96 ++++++++++++++++++++++++-----------------
> 1 file changed, 56 insertions(+), 40 deletions(-)
>
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index c9822eae4c..f58f6f2e5e 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -61,6 +61,8 @@
> #define DISAS_UPDATE DISAS_TARGET_1
> /* Cpu state was modified dynamically, excluding pc -- use npc */
> #define DISAS_UPDATE_NEXT DISAS_TARGET_2
> +/* PC update for delayed branch, see cpustate_changed otherwise */
> +#define DISAS_DBRANCH DISAS_TARGET_3
>
> /* Used by the decoder. */
> #define EXTRACT_FIELD(src, start, end) \
> @@ -3228,50 +3230,22 @@ static void cris_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
> dc->cpustate_changed |= dc->flags_x != (dc->base.tb->flags & X_FLAG);
>
> /*
> - * Check for delayed branches here. If we do it before
> - * actually generating any host code, the simulator will just
> - * loop doing nothing for on this program location.
> + * All branches are delayed branches, handled immediately below.
> + * We don't expect to see odd combinations of exit conditions.
> */
> + assert(dc->base.is_jmp == DISAS_NEXT || dc->cpustate_changed);
> +
> if (dc->delayed_branch && --dc->delayed_branch == 0) {
> - if (dc->base.tb->flags & 7) {
> - t_gen_movi_env_TN(dslot, 0);
> - }
> + dc->base.is_jmp = DISAS_DBRANCH;
> + return;
> + }
>
> - if (dc->cpustate_changed) {
> - cris_store_direct_jmp(dc);
> - }
> -
> - if (dc->clear_locked_irq) {
> - dc->clear_locked_irq = 0;
> - t_gen_movi_env_TN(locked_irq, 0);
> - }
> -
> - if (dc->jmp == JMP_DIRECT_CC) {
> - TCGLabel *l1 = gen_new_label();
> - cris_evaluate_flags(dc);
> -
> - /* Conditional jmp. */
> - tcg_gen_brcondi_tl(TCG_COND_EQ, env_btaken, 0, l1);
> - gen_goto_tb(dc, 1, dc->jmp_pc);
> - gen_set_label(l1);
> - gen_goto_tb(dc, 0, dc->pc);
> - dc->base.is_jmp = DISAS_NORETURN;
> - dc->jmp = JMP_NOJMP;
> - } else if (dc->jmp == JMP_DIRECT) {
> - cris_evaluate_flags(dc);
> - gen_goto_tb(dc, 0, dc->jmp_pc);
> - dc->base.is_jmp = DISAS_NORETURN;
> - dc->jmp = JMP_NOJMP;
> - } else {
> - TCGv c = tcg_const_tl(dc->pc);
> - t_gen_cc_jmp(env_btarget, c);
> - tcg_temp_free(c);
> - dc->base.is_jmp = DISAS_JUMP;
> - }
> + if (dc->base.is_jmp != DISAS_NEXT) {
> + return;
> }
>
> /* Force an update if the per-tb cpu state has changed. */
> - if (dc->base.is_jmp == DISAS_NEXT && dc->cpustate_changed) {
> + if (dc->cpustate_changed) {
> dc->base.is_jmp = DISAS_UPDATE_NEXT;
> return;
> }
> @@ -3281,8 +3255,7 @@ static void cris_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
> * If we can detect the length of the next insn easily, we should.
> * In the meantime, simply stop when we do cross.
> */
> - if (dc->base.is_jmp == DISAS_NEXT
> - && ((dc->pc ^ dc->base.pc_first) & TARGET_PAGE_MASK) != 0) {
> + if ((dc->pc ^ dc->base.pc_first) & TARGET_PAGE_MASK) {
> dc->base.is_jmp = DISAS_TOO_MANY;
> }
> }
> @@ -3312,6 +3285,49 @@ static void cris_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>
> cris_evaluate_flags(dc);
>
> + /* Evaluate delayed branch destination and fold to another is_jmp case. */
> + if (is_jmp == DISAS_DBRANCH) {
> + if (dc->base.tb->flags & 7) {
> + t_gen_movi_env_TN(dslot, 0);
> + }
> +
> + switch (dc->jmp) {
> + case JMP_DIRECT:
> + npc = dc->jmp_pc;
> + is_jmp = dc->cpustate_changed ? DISAS_UPDATE_NEXT : DISAS_TOO_MANY;
> + break;
> +
> + case JMP_DIRECT_CC:
> + /*
> + * Use a conditional branch if either taken or not-taken path
> + * can use goto_tb. If neither can, then treat it as indirect.
> + */
> + if (likely(!dc->base.singlestep_enabled)
> + && likely(!dc->cpustate_changed)
> + && (use_goto_tb(dc, dc->jmp_pc) || use_goto_tb(dc, npc))) {
> + TCGLabel *not_taken = gen_new_label();
> +
> + tcg_gen_brcondi_tl(TCG_COND_EQ, env_btaken, 0, not_taken);
> + gen_goto_tb(dc, 1, dc->jmp_pc);
> + gen_set_label(not_taken);
> +
> + /* not-taken case handled below. */
> + is_jmp = DISAS_TOO_MANY;
> + break;
> + }
> + tcg_gen_movi_tl(env_btarget, dc->jmp_pc);
> + /* fall through */
> +
> + case JMP_INDIRECT:
> + t_gen_cc_jmp(env_btarget, tcg_constant_tl(npc));
> + is_jmp = dc->cpustate_changed ? DISAS_UPDATE : DISAS_JUMP;
> + break;
> +
> + default:
> + g_assert_not_reached();
> + }
> + }
> +
> if (unlikely(dc->base.singlestep_enabled)) {
> switch (is_jmp) {
> case DISAS_TOO_MANY:
> --
> 2.25.1
>
On 6/23/21 6:43 AM, Edgar E. Iglesias wrote: > On Tue, Jun 22, 2021 at 08:48:16AM -0700, Richard Henderson wrote: >> Move delayed branch handling to tb_stop, where we can re-use other >> end-of-tb code, e.g. the evaluation of flags. Honor single stepping. >> Validate that we aren't losing state by overwriting is_jmp. > > Hi Richard, > > This patch breaks my kernel boot test: > > edgar@zapote:cris-axisdev88$ ./qemu-run.sh > + MACH=-M axis-dev88 > + QEMU_BUILD_PATH=/home/edgar/src/c/qemu/build-qemu/ > + QEMU=/home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris > + KERNEL=-kernel kimage > + NIC0=-netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0 > + /home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris -M axis-dev88 -netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0 -serial stdio -display none -kernel kimage > Linux version 2.6.33 (edgar@edde) (gcc version 4.3.1 20080521 (prerelease) [gcc-4_3-branch revision 135713] (GCC 4.3.1 Axis release R93/1.93) ) #4 Thu Jan 13 15:11:20 CET 2011 > bootconsole [early0] enabled > ROM fs in RAM, size 6946816 bytes > Setting up paging and the MMU. > Linux/CRISv32 port on ETRAX FS (C) 2003, 2004 Axis Communications AB > Built 1 zonelists in Zone order, mobility grouping on. Total pages: 4080 > Kernel command line: root=/dev/mtdblock3 init=/linuxrc rootfstype=jffs2 mmc_core.use_spi_crc=0 mmc_spi.spi_mode=3 > PID hash table entries: 128 (order: -4, 512 bytes) > Dentry cache hash table entries: 4096 (order: 1, 16384 bytes) > Inode-cache hash table entries: 2048 (order: 0, 8192 bytes) > Memory: 22864k/32768k available (2260k kernel code, 9904k reserved, 504k data, 80k init) > Hierarchical RCU implementation. > NR_IRQS:80 > Enabling watchdog... > Calibrating delay loop... qemu-system-cris: ../qemu/target/cris/translate.c:3236: cris_tr_translate_insn: Assertion `dc->base.is_jmp == DISAS_NEXT || dc->cpustate_changed' failed. > Aborted (core dumped) Ach, I see it -- rfe and rfn set DISAS_UPDATE without cpustate_changed. Could you try adding dc->cpustate_changed = 1 in dec_rfe_etc, please. r~
On Wed, Jun 23, 2021 at 06:55:49AM -0700, Richard Henderson wrote: > On 6/23/21 6:43 AM, Edgar E. Iglesias wrote: > > On Tue, Jun 22, 2021 at 08:48:16AM -0700, Richard Henderson wrote: > > > Move delayed branch handling to tb_stop, where we can re-use other > > > end-of-tb code, e.g. the evaluation of flags. Honor single stepping. > > > Validate that we aren't losing state by overwriting is_jmp. > > > > Hi Richard, > > > > This patch breaks my kernel boot test: > > > > edgar@zapote:cris-axisdev88$ ./qemu-run.sh > > + MACH=-M axis-dev88 > > + QEMU_BUILD_PATH=/home/edgar/src/c/qemu/build-qemu/ > > + QEMU=/home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris > > + KERNEL=-kernel kimage > > + NIC0=-netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0 > > + /home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris -M axis-dev88 -netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0 -serial stdio -display none -kernel kimage > > Linux version 2.6.33 (edgar@edde) (gcc version 4.3.1 20080521 (prerelease) [gcc-4_3-branch revision 135713] (GCC 4.3.1 Axis release R93/1.93) ) #4 Thu Jan 13 15:11:20 CET 2011 > > bootconsole [early0] enabled > > ROM fs in RAM, size 6946816 bytes > > Setting up paging and the MMU. > > Linux/CRISv32 port on ETRAX FS (C) 2003, 2004 Axis Communications AB > > Built 1 zonelists in Zone order, mobility grouping on. Total pages: 4080 > > Kernel command line: root=/dev/mtdblock3 init=/linuxrc rootfstype=jffs2 mmc_core.use_spi_crc=0 mmc_spi.spi_mode=3 > > PID hash table entries: 128 (order: -4, 512 bytes) > > Dentry cache hash table entries: 4096 (order: 1, 16384 bytes) > > Inode-cache hash table entries: 2048 (order: 0, 8192 bytes) > > Memory: 22864k/32768k available (2260k kernel code, 9904k reserved, 504k data, 80k init) > > Hierarchical RCU implementation. > > NR_IRQS:80 > > Enabling watchdog... > > Calibrating delay loop... qemu-system-cris: ../qemu/target/cris/translate.c:3236: cris_tr_translate_insn: Assertion `dc->base.is_jmp == DISAS_NEXT || dc->cpustate_changed' failed. > > Aborted (core dumped) > > Ach, I see it -- rfe and rfn set DISAS_UPDATE without cpustate_changed. > Could you try adding dc->cpustate_changed = 1 in dec_rfe_etc, please. > Great, that fixes it! This now passes all my tests. With that fix, on the full series: Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> Cheers, Edgar
On 6/23/21 7:17 AM, Edgar E. Iglesias wrote: > On Wed, Jun 23, 2021 at 06:55:49AM -0700, Richard Henderson wrote: >> On 6/23/21 6:43 AM, Edgar E. Iglesias wrote: >>> On Tue, Jun 22, 2021 at 08:48:16AM -0700, Richard Henderson wrote: >>>> Move delayed branch handling to tb_stop, where we can re-use other >>>> end-of-tb code, e.g. the evaluation of flags. Honor single stepping. >>>> Validate that we aren't losing state by overwriting is_jmp. >>> >>> Hi Richard, >>> >>> This patch breaks my kernel boot test: >>> >>> edgar@zapote:cris-axisdev88$ ./qemu-run.sh >>> + MACH=-M axis-dev88 >>> + QEMU_BUILD_PATH=/home/edgar/src/c/qemu/build-qemu/ >>> + QEMU=/home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris >>> + KERNEL=-kernel kimage >>> + NIC0=-netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0 >>> + /home/edgar/src/c/qemu/build-qemu//cris-softmmu/qemu-system-cris -M axis-dev88 -netdev user,id=net0,hostfwd=tcp::2256-10.0.2.15:21 -net nic,netdev=net0 -serial stdio -display none -kernel kimage >>> Linux version 2.6.33 (edgar@edde) (gcc version 4.3.1 20080521 (prerelease) [gcc-4_3-branch revision 135713] (GCC 4.3.1 Axis release R93/1.93) ) #4 Thu Jan 13 15:11:20 CET 2011 >>> bootconsole [early0] enabled >>> ROM fs in RAM, size 6946816 bytes >>> Setting up paging and the MMU. >>> Linux/CRISv32 port on ETRAX FS (C) 2003, 2004 Axis Communications AB >>> Built 1 zonelists in Zone order, mobility grouping on. Total pages: 4080 >>> Kernel command line: root=/dev/mtdblock3 init=/linuxrc rootfstype=jffs2 mmc_core.use_spi_crc=0 mmc_spi.spi_mode=3 >>> PID hash table entries: 128 (order: -4, 512 bytes) >>> Dentry cache hash table entries: 4096 (order: 1, 16384 bytes) >>> Inode-cache hash table entries: 2048 (order: 0, 8192 bytes) >>> Memory: 22864k/32768k available (2260k kernel code, 9904k reserved, 504k data, 80k init) >>> Hierarchical RCU implementation. >>> NR_IRQS:80 >>> Enabling watchdog... >>> Calibrating delay loop... qemu-system-cris: ../qemu/target/cris/translate.c:3236: cris_tr_translate_insn: Assertion `dc->base.is_jmp == DISAS_NEXT || dc->cpustate_changed' failed. >>> Aborted (core dumped) >> >> Ach, I see it -- rfe and rfn set DISAS_UPDATE without cpustate_changed. >> Could you try adding dc->cpustate_changed = 1 in dec_rfe_etc, please. >> > > Great, that fixes it! This now passes all my tests. > > With that fix, on the full series: > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> Thanks. Based on private email with Edgar, I'll queue this to tcg-next. r~
© 2016 - 2025 Red Hat, Inc.