| Message ID | cover.1626286772.git.bobby.eshleman@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Remove unconditional arch dependency on asm/debugger.h | expand |
On 14/07/2021 21:37, Bobby Eshleman wrote: > Currently, any architecture wishing to use common/ is likely > to be required to implement the functions found in "asm/debugger.h". > Some architectures, however, do not have an actual use for these > functions and so are forced to implement stubs. This patch does the > following: > > * Supplies common stubs if !CONFIG_CRASH_DEBUG for any architecture, > removing the need for all new architectures to have "asm/debugger.h". > * Moves parts of the x86 implementation to "arch/x86/debugger.c". > * Removes the ARM calls to its stubs. > * Centralizes non-inlined x86 code conditionally compiled by CONFIG_CRASH_DEBUG > into arch/x86/debugger.c, which is now conditionally built for > CONFIG_CRASH_DEBUG via Kbuild (i.e., obj-$(CONFIG_CRASH_DEBUG)). > * Tries to improve the x86 implementation by not inlining large > functions (but preserving inlining for those that seemed "small"). My replies from yesterday appear to have got lost. Lets try it again. Jan already picked up on the header file and commit change in patch 1. However, patch 2 actually demonstrates a massive confusion which exists in the x86 code. We have two things called debugger, which are unrelated, but mixed in asm-x86/debugger.h There is gdbstub itself, which is an implementation of the gdb remote debugging protocol over serial. (I've never seen anyone use this in a decade, and the logic isn't remotely SMP-safe at all, so I'm very tempted to suggest ripping it out completely, but lets ignore that for now). Then we have debugger_trap_*() which claims to be arch-neutral wrappers to a common debugging interface, which is only actually backed by gdbstub in x86. Both of these facilities are to do with debugging Xen itself when Xen crashes. Then there is gdbsx which is totally unrelated to the above, and is a daemon in dom0 to translate the gdb remote protocol into a set of hypercalls to perform on a guest under test. domain_pause_for_debugger() is gdbsx functionality, and nothing to do with Xen crashing. On top of that, debugger_trap_entry() is actually a layering violation merging the two. Therefore, I recommend the following, in this order: 1) Patch emptying debugger_trap_entry() and expanding the contents inline in do_int3/debug(). Both already have an if ( !guest_mode() ) path, so add an else if ( ... ) clause. This supersedes patch 3. (Also, fix the logic to have "const struct vcpu *curr = current" and avoid the opencoded use of current lower down). curr->arch.gdbsx_vcpu_event only being set for TRAP_int3 looks totally bogus (the non-int3 paths cause gdbsx to miss notifications), but is repeated all across Xen. Keep the logic unchanged across the move, and leave fixing gdbsx bugs to some future point. 2) Patch (or patches) renaming arch/x86/debug.c to arch/x86/gdbsx.c, and add a new include/asm-x86/gdbsx.h. domain_pause_for_debugger() wants moving (prototype and definition) which subsumes patch 4, and deletes domain.c's include of debugger.h domctl.s ifdef'd gdbsx_guest_mem_io() wants moving too, as it has one caller, and is the sole caller of dbg_rw_mem(). The two functions likely want merging so we don't just have a wrapper making trivial API change. This will also require some header file renames. With this done, there is now a properly split between the actually-CONFIG_GDBSX stuff and the actually-CONFIG_DEBUG_CRASH stuff. 3) What is currently patch 1 wants to be next, taking with it the header file rename from patch 2. 4) Finally, the remanent of patch 2. The CONFIG_CRASH_DEBUG implementation is now just the gdbstub call in _fatal(), so I don't think a new debugger.c file is necessary. Hopefully this all makes sense. ~Andrew
Currently, any architecture wishing to use common/ is likely to be required to implement the functions found in "asm/debugger.h". Some architectures, however, do not have an actual use for these functions and so are forced to implement stubs. This patch does the following: * Supplies common stubs if !CONFIG_CRASH_DEBUG for any architecture, removing the need for all new architectures to have "asm/debugger.h". * Moves parts of the x86 implementation to "arch/x86/debugger.c". * Removes the ARM calls to its stubs. * Centralizes non-inlined x86 code conditionally compiled by CONFIG_CRASH_DEBUG into arch/x86/debugger.c, which is now conditionally built for CONFIG_CRASH_DEBUG via Kbuild (i.e., obj-$(CONFIG_CRASH_DEBUG)). * Tries to improve the x86 implementation by not inlining large functions (but preserving inlining for those that seemed "small"). Bobby Eshleman (4): arm/traps: remove debugger_trap_fatal() calls build: use common stubs for debugger_trap_* functions if !CONFIG_CRASH_DEBUG x86/debug: move debugger_trap_entry into debugger.c not inlined x86/debug: move domain_pause_for_debugger to debugger.c xen/arch/arm/traps.c | 8 +-- xen/arch/x86/Makefile | 1 + xen/arch/x86/debug.c | 2 +- xen/arch/x86/debugger.c | 53 ++++++++++++++++++++ xen/arch/x86/domain.c | 15 +----- xen/arch/x86/domctl.c | 2 +- xen/arch/x86/gdbstub.c | 4 +- xen/arch/x86/hvm/svm/svm.c | 2 +- xen/arch/x86/hvm/vmx/realmode.c | 2 +- xen/arch/x86/hvm/vmx/vmx.c | 2 +- xen/arch/x86/nmi.c | 2 +- xen/arch/x86/traps.c | 2 +- xen/arch/x86/x86_64/gdbstub.c | 3 +- xen/common/domain.c | 2 +- xen/common/gdbstub.c | 3 +- xen/common/keyhandler.c | 2 +- xen/common/shutdown.c | 2 +- xen/drivers/char/console.c | 2 +- xen/include/asm-arm/debugger.h | 15 ------ xen/include/asm-x86/debugger.h | 86 ++++++--------------------------- xen/include/xen/debugger.h | 69 ++++++++++++++++++++++++++ 21 files changed, 157 insertions(+), 122 deletions(-) create mode 100644 xen/arch/x86/debugger.c delete mode 100644 xen/include/asm-arm/debugger.h create mode 100644 xen/include/xen/debugger.h