[PATCH for-6.1 v6 00/17] tcg: breakpoint reorg

Richard Henderson posted 17 patches 2 years, 9 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210720195439.626594-1-richard.henderson@linaro.org
Maintainers: Chris Wulff <crwulff@gmail.com>, Marek Vasut <marex@denx.de>, Stafford Horne <shorne@gmail.com>, Max Filippov <jcmvbkbc@gmail.com>, Richard Henderson <richard.henderson@linaro.org>, Thomas Huth <thuth@redhat.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, David Gibson <david@gibson.dropbear.id.au>, Laurent Vivier <laurent@vivier.eu>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Bin Meng <bin.meng@windriver.com>, Palmer Dabbelt <palmer@dabbelt.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Artyom Tarasenko <atar4qemu@gmail.com>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aurelien Jarno <aurelien@aurel32.net>, Alistair Francis <alistair.francis@wdc.com>, Cornelia Huck <cohuck@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Michael Rolnik <mrolnik@gmail.com>, Taylor Simpson <tsimpson@quicinc.com>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, David Hildenbrand <david@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Greg Kurz <groug@kaod.org>
include/exec/exec-all.h       |  24 ++--
include/exec/translator.h     |  11 --
include/hw/core/cpu.h         |   4 +
include/hw/core/tcg-cpu-ops.h |   6 +
target/arm/helper.h           |   2 -
target/arm/internals.h        |   3 +
target/avr/cpu.h              |   1 +
accel/tcg/cpu-exec.c          | 205 ++++++++++++++++++++++++++--------
accel/tcg/translate-all.c     |   7 +-
accel/tcg/translator.c        |  39 ++-----
cpu.c                         |  34 ++----
target/alpha/translate.c      |  31 +----
target/arm/cpu.c              |   1 +
target/arm/cpu_tcg.c          |   1 +
target/arm/debug_helper.c     |  12 +-
target/arm/translate-a64.c    |  25 -----
target/arm/translate.c        |  29 -----
target/avr/cpu.c              |   1 +
target/avr/gdbstub.c          |  13 +++
target/avr/translate.c        |  32 ------
target/cris/translate.c       |  20 ----
target/hexagon/translate.c    |  17 ---
target/hppa/translate.c       |  11 --
target/i386/tcg/tcg-cpu.c     |  12 ++
target/i386/tcg/translate.c   |  28 -----
target/m68k/translate.c       |  18 ---
target/microblaze/translate.c |  18 ---
target/mips/tcg/translate.c   |  19 ----
target/nios2/translate.c      |  27 -----
target/openrisc/translate.c   |  17 ---
target/ppc/translate.c        |  18 ---
target/riscv/translate.c      |  17 ---
target/rx/translate.c         |  14 ---
target/s390x/tcg/translate.c  |  24 ----
target/sh4/translate.c        |  18 ---
target/sparc/translate.c      |  17 ---
target/tricore/translate.c    |  16 ---
target/xtensa/translate.c     |  17 ---
tcg/tcg-op.c                  |  28 ++---
39 files changed, 248 insertions(+), 589 deletions(-)
[PATCH for-6.1 v6 00/17] tcg: breakpoint reorg
Posted by Richard Henderson 2 years, 9 months ago
This is fixing #404 ("windows xp boot takes much longer...")
and several other similar reports.

Changes for v6:
  * Reinstate accidental loss of singlestep overriding breakpoint check.
    Shows up in the record-replay avocado tests failing to make progress.
  * Add CPUState.gdb_adjust_breakpoint hook for AVR weirdness.

Changes for v5:
  * Include missing hunk in tb_gen_code, as noted in reply to v4.
  * Remove helper_check_breakpoints from target/arm/.
  * Reorg cflags_for_breakpoints into check_for_breakpoints;
    reorg cpu_exec to use a break instead of a longjmp.
  * Move singlestep_enabled check from cflags_for_breakpoints
    to curr_cflags, which makes cpu_exec_step_atomic cleaner.

Changes for v4:
  * Issue breakpoints directly from cflags_for_breakpoints.
    Do not generate code for a TB beginning with a BP at all.
  * Drop the problematic TranslatorOps.breakpoint_check hook entirely.

Changes for v3:
  * Map CF_COUNT_MASK == 0 -> TCG_MAX_INSNS.
  * Split out *_breakpoint_check fixes for avr, mips, riscv.

Changes for v2:
  * All prerequisites and 7 of the patches from v1 with are merged.

Patches lacking review:
  11-hw-core-Introduce-CPUClass.gdb_adjust_breakpoint.patch
  12-target-avr-Implement-gdb_adjust_breakpoint.patch
  15-accel-tcg-Remove-TranslatorOps.breakpoint_check.patch
  17-accel-tcg-Record-singlestep_enabled-in-tb-cflags.patch


r~


Richard Henderson (17):
  accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS
  accel/tcg: Move curr_cflags into cpu-exec.c
  target/alpha: Drop goto_tb path in gen_call_pal
  accel/tcg: Add CF_NO_GOTO_TB and CF_NO_GOTO_PTR
  accel/tcg: Drop CF_NO_GOTO_PTR from -d nochain
  accel/tcg: Handle -singlestep in curr_cflags
  accel/tcg: Use CF_NO_GOTO_{TB, PTR} in cpu_exec_step_atomic
  hw/core: Introduce TCGCPUOps.debug_check_breakpoint
  target/arm: Implement debug_check_breakpoint
  target/i386: Implement debug_check_breakpoint
  hw/core: Introduce CPUClass.gdb_adjust_breakpoint
  target/avr: Implement gdb_adjust_breakpoint
  accel/tcg: Merge tb_find into its only caller
  accel/tcg: Move breakpoint recognition outside translation
  accel/tcg: Remove TranslatorOps.breakpoint_check
  accel/tcg: Hoist tb_cflags to a local in translator_loop
  accel/tcg: Record singlestep_enabled in tb->cflags

 include/exec/exec-all.h       |  24 ++--
 include/exec/translator.h     |  11 --
 include/hw/core/cpu.h         |   4 +
 include/hw/core/tcg-cpu-ops.h |   6 +
 target/arm/helper.h           |   2 -
 target/arm/internals.h        |   3 +
 target/avr/cpu.h              |   1 +
 accel/tcg/cpu-exec.c          | 205 ++++++++++++++++++++++++++--------
 accel/tcg/translate-all.c     |   7 +-
 accel/tcg/translator.c        |  39 ++-----
 cpu.c                         |  34 ++----
 target/alpha/translate.c      |  31 +----
 target/arm/cpu.c              |   1 +
 target/arm/cpu_tcg.c          |   1 +
 target/arm/debug_helper.c     |  12 +-
 target/arm/translate-a64.c    |  25 -----
 target/arm/translate.c        |  29 -----
 target/avr/cpu.c              |   1 +
 target/avr/gdbstub.c          |  13 +++
 target/avr/translate.c        |  32 ------
 target/cris/translate.c       |  20 ----
 target/hexagon/translate.c    |  17 ---
 target/hppa/translate.c       |  11 --
 target/i386/tcg/tcg-cpu.c     |  12 ++
 target/i386/tcg/translate.c   |  28 -----
 target/m68k/translate.c       |  18 ---
 target/microblaze/translate.c |  18 ---
 target/mips/tcg/translate.c   |  19 ----
 target/nios2/translate.c      |  27 -----
 target/openrisc/translate.c   |  17 ---
 target/ppc/translate.c        |  18 ---
 target/riscv/translate.c      |  17 ---
 target/rx/translate.c         |  14 ---
 target/s390x/tcg/translate.c  |  24 ----
 target/sh4/translate.c        |  18 ---
 target/sparc/translate.c      |  17 ---
 target/tricore/translate.c    |  16 ---
 target/xtensa/translate.c     |  17 ---
 tcg/tcg-op.c                  |  28 ++---
 39 files changed, 248 insertions(+), 589 deletions(-)

-- 
2.25.1


Re: [PATCH for-6.1 v6 00/17] tcg: breakpoint reorg
Posted by Mark Cave-Ayland 2 years, 9 months ago
On 20/07/2021 20:54, Richard Henderson wrote:

> This is fixing #404 ("windows xp boot takes much longer...")
> and several other similar reports.
> 
> Changes for v6:
>    * Reinstate accidental loss of singlestep overriding breakpoint check.
>      Shows up in the record-replay avocado tests failing to make progress.
>    * Add CPUState.gdb_adjust_breakpoint hook for AVR weirdness.
> 
> Changes for v5:
>    * Include missing hunk in tb_gen_code, as noted in reply to v4.
>    * Remove helper_check_breakpoints from target/arm/.
>    * Reorg cflags_for_breakpoints into check_for_breakpoints;
>      reorg cpu_exec to use a break instead of a longjmp.
>    * Move singlestep_enabled check from cflags_for_breakpoints
>      to curr_cflags, which makes cpu_exec_step_atomic cleaner.
> 
> Changes for v4:
>    * Issue breakpoints directly from cflags_for_breakpoints.
>      Do not generate code for a TB beginning with a BP at all.
>    * Drop the problematic TranslatorOps.breakpoint_check hook entirely.
> 
> Changes for v3:
>    * Map CF_COUNT_MASK == 0 -> TCG_MAX_INSNS.
>    * Split out *_breakpoint_check fixes for avr, mips, riscv.
> 
> Changes for v2:
>    * All prerequisites and 7 of the patches from v1 with are merged.
> 
> Patches lacking review:
>    11-hw-core-Introduce-CPUClass.gdb_adjust_breakpoint.patch
>    12-target-avr-Implement-gdb_adjust_breakpoint.patch
>    15-accel-tcg-Remove-TranslatorOps.breakpoint_check.patch
>    17-accel-tcg-Record-singlestep_enabled-in-tb-cflags.patch
> 
> 
> r~
> 
> 
> Richard Henderson (17):
>    accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS
>    accel/tcg: Move curr_cflags into cpu-exec.c
>    target/alpha: Drop goto_tb path in gen_call_pal
>    accel/tcg: Add CF_NO_GOTO_TB and CF_NO_GOTO_PTR
>    accel/tcg: Drop CF_NO_GOTO_PTR from -d nochain
>    accel/tcg: Handle -singlestep in curr_cflags
>    accel/tcg: Use CF_NO_GOTO_{TB, PTR} in cpu_exec_step_atomic
>    hw/core: Introduce TCGCPUOps.debug_check_breakpoint
>    target/arm: Implement debug_check_breakpoint
>    target/i386: Implement debug_check_breakpoint
>    hw/core: Introduce CPUClass.gdb_adjust_breakpoint
>    target/avr: Implement gdb_adjust_breakpoint
>    accel/tcg: Merge tb_find into its only caller
>    accel/tcg: Move breakpoint recognition outside translation
>    accel/tcg: Remove TranslatorOps.breakpoint_check
>    accel/tcg: Hoist tb_cflags to a local in translator_loop
>    accel/tcg: Record singlestep_enabled in tb->cflags
> 
>   include/exec/exec-all.h       |  24 ++--
>   include/exec/translator.h     |  11 --
>   include/hw/core/cpu.h         |   4 +
>   include/hw/core/tcg-cpu-ops.h |   6 +
>   target/arm/helper.h           |   2 -
>   target/arm/internals.h        |   3 +
>   target/avr/cpu.h              |   1 +
>   accel/tcg/cpu-exec.c          | 205 ++++++++++++++++++++++++++--------
>   accel/tcg/translate-all.c     |   7 +-
>   accel/tcg/translator.c        |  39 ++-----
>   cpu.c                         |  34 ++----
>   target/alpha/translate.c      |  31 +----
>   target/arm/cpu.c              |   1 +
>   target/arm/cpu_tcg.c          |   1 +
>   target/arm/debug_helper.c     |  12 +-
>   target/arm/translate-a64.c    |  25 -----
>   target/arm/translate.c        |  29 -----
>   target/avr/cpu.c              |   1 +
>   target/avr/gdbstub.c          |  13 +++
>   target/avr/translate.c        |  32 ------
>   target/cris/translate.c       |  20 ----
>   target/hexagon/translate.c    |  17 ---
>   target/hppa/translate.c       |  11 --
>   target/i386/tcg/tcg-cpu.c     |  12 ++
>   target/i386/tcg/translate.c   |  28 -----
>   target/m68k/translate.c       |  18 ---
>   target/microblaze/translate.c |  18 ---
>   target/mips/tcg/translate.c   |  19 ----
>   target/nios2/translate.c      |  27 -----
>   target/openrisc/translate.c   |  17 ---
>   target/ppc/translate.c        |  18 ---
>   target/riscv/translate.c      |  17 ---
>   target/rx/translate.c         |  14 ---
>   target/s390x/tcg/translate.c  |  24 ----
>   target/sh4/translate.c        |  18 ---
>   target/sparc/translate.c      |  17 ---
>   target/tricore/translate.c    |  16 ---
>   target/xtensa/translate.c     |  17 ---
>   tcg/tcg-op.c                  |  28 ++---
>   39 files changed, 248 insertions(+), 589 deletions(-)

I spent a bit of time this evening testing this patchset with some typical OpenBIOS 
debugging patterns across SPARC32, SPARC64 and PPC and didn't spot any regressions, 
and the WinXP image still boots in 25s so:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

I did find the slow-down noticeable when testing a few OpenBIOS breakpoints e.g. a 
breakpoint on OpenBIOS SPARC64's init_memory() upped the boot time to the Forth 
prompt from 9s to 30s even though it hits only once early in boot (presumably due to 
its proximity to a hot routine). Having said that, the changes look like a good 
improvement and patch 14 suggests that there could be further optimisations to be 
made in future, so in general this feels like a net win.


ATB,

Mark.