Re: [PATCH v2 0/4] Remove unconditional arch dependency on asm/debugger.h

[Andrew Cooper <andrew.cooper3@citrix.com>]

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