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

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

[Bobby Eshleman]

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

-- 
2.30.0

[PATCH v2 1/4] arm/traps: remove debugger_trap_fatal() calls

[Bobby Eshleman]

ARM doesn't actually use debugger_trap_* anything, and is stubbed out.

Simply remove the calls. This also renders TRAP_invalid_op unused in
any common code, so remove that definition too.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
---
 xen/arch/arm/traps.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 4ccb6e7d18..dd09d2a4a9 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1266,10 +1266,6 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
 
     case BUGFRAME_bug:
         printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
-
-        if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
-            return 0;
-
         show_execution_state(regs);
         panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
 
@@ -1281,8 +1277,6 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
 
         printk("Assertion '%s' failed at %s%s:%d\n",
                predicate, prefix, filename, lineno);
-        if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
-            return 0;
         show_execution_state(regs);
         panic("Assertion '%s' failed at %s%s:%d\n",
               predicate, prefix, filename, lineno);
-- 
2.30.0

[PATCH v2 2/4] build: use common stubs for debugger_trap_* functions if !CONFIG_CRASH_DEBUG

[Bobby Eshleman]

Previously Xen required all architectures implement the debugger_trap_*
functions whether or not it actually needs them.

This commit makes debugger_trap* functions resolve to arch-specific
function definitions if CONFIG_CRASH_DEBUG=y, but resolves to a set of
common no-op stubs if !CONFIG_CRASH_DEBUG, which avoids requiring every
arch to carry its own stubs.  This means asm/debugger.h may also be
dropped for architectures that do not need this functionality.

Inside xen/debugger.h:
    * If !CONFIG_CRASH_DEBUG, use stubs.
    * Otherwise, include arch-specific <asm/debugger.h>
---

Changes in v2:
- #include ordering (xen/.h before asm/.h, alphabetical order)
- Other style (no double blanks in comments).
- Removed dummy TRAP_invalid_op for ARM (not needed because
  the patch to remove the calls is now prior to this patch)
- Removed common-izing of dbg_rw_mem

 xen/arch/arm/traps.c            |  2 +-
 xen/arch/x86/debug.c            |  2 +-
 xen/arch/x86/domain.c           |  5 ++-
 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  | 60 ++++++----------------------
 xen/include/xen/debugger.h      | 69 +++++++++++++++++++++++++++++++++
 19 files changed, 103 insertions(+), 80 deletions(-)
 delete mode 100644 xen/include/asm-arm/debugger.h
 create mode 100644 xen/include/xen/debugger.h

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index dd09d2a4a9..10db98e01a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -16,6 +16,7 @@
  * GNU General Public License for more details.
  */
 
+#include <xen/debugger.h>
 #include <xen/domain_page.h>
 #include <xen/errno.h>
 #include <xen/hypercall.h>
@@ -41,7 +42,6 @@
 #include <asm/acpi.h>
 #include <asm/cpuerrata.h>
 #include <asm/cpufeature.h>
-#include <asm/debugger.h>
 #include <asm/event.h>
 #include <asm/hsr.h>
 #include <asm/mmio.h>
diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index d90dc93056..583a9a042a 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -14,12 +14,12 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/debugger.h>
 #include <xen/sched.h>
 #include <xen/compile.h>
 #include <xen/mm.h>
 #include <xen/domain_page.h>
 #include <xen/guest_access.h>
-#include <asm/debugger.h>
 #include <asm/p2m.h>
 
 typedef unsigned long dbgva_t;
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ef1812dc14..5b948ff270 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -15,6 +15,7 @@
 #include <xen/lib.h>
 #include <xen/errno.h>
 #include <xen/sched.h>
+#include <xen/debugger.h>
 #include <xen/domain.h>
 #include <xen/smp.h>
 #include <xen/delay.h>
@@ -2539,9 +2540,9 @@ static int __init init_vcpu_kick_softirq(void)
 }
 __initcall(init_vcpu_kick_softirq);
 
+#ifdef CONFIG_CRASH_DEBUG
 void domain_pause_for_debugger(void)
 {
-#ifdef CONFIG_CRASH_DEBUG
     struct vcpu *curr = current;
     struct domain *d = curr->domain;
 
@@ -2550,8 +2551,8 @@ void domain_pause_for_debugger(void)
     /* if gdbsx active, we just need to pause the domain */
     if ( curr->arch.gdbsx_vcpu_event == 0 )
         send_global_virq(VIRQ_DEBUGGER);
-#endif
 }
+#endif
 
 /*
  * Local variables:
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 26a76d2be9..24bdb86de7 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -12,6 +12,7 @@
 #include <xen/pci.h>
 #include <public/domctl.h>
 #include <xen/sched.h>
+#include <xen/debugger.h>
 #include <xen/domain.h>
 #include <xen/event.h>
 #include <xen/domain_page.h>
@@ -33,7 +34,6 @@
 #include <public/vm_event.h>
 #include <asm/mem_sharing.h>
 #include <asm/xstate.h>
-#include <asm/debugger.h>
 #include <asm/psr.h>
 #include <asm/cpuid.h>
 
diff --git a/xen/arch/x86/gdbstub.c b/xen/arch/x86/gdbstub.c
index 8f4f49fd3b..5e23d27f62 100644
--- a/xen/arch/x86/gdbstub.c
+++ b/xen/arch/x86/gdbstub.c
@@ -18,7 +18,9 @@
  * You should have received a copy of the GNU General Public License
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
-#include <asm/debugger.h>
+#include <xen/debugger.h>
+#include <xen/gdbstub.h>
+#include <asm/uaccess.h>
 
 u16
 gdb_arch_signal_num(struct cpu_user_regs *regs, unsigned long cookie)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 642a64b747..ce1cbe7825 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -24,6 +24,7 @@
 #include <xen/irq.h>
 #include <xen/softirq.h>
 #include <xen/hypercall.h>
+#include <xen/debugger.h>
 #include <xen/domain_page.h>
 #include <xen/xenoprof.h>
 #include <asm/current.h>
@@ -58,7 +59,6 @@
 #include <asm/hvm/trace.h>
 #include <asm/hap.h>
 #include <asm/apic.h>
-#include <asm/debugger.h>
 #include <asm/hvm/monitor.h>
 #include <asm/monitor.h>
 #include <asm/xstate.h>
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index cc23afa788..57d230c4fe 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -9,12 +9,12 @@
  *    Keir Fraser <keir@xen.org>
  */
 
+#include <xen/debugger.h>
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xen/paging.h>
 #include <xen/softirq.h>
-#include <asm/debugger.h>
 #include <asm/event.h>
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/hvm.h>
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index e09b7e3af9..00107e8a3b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -23,6 +23,7 @@
 #include <xen/sched.h>
 #include <xen/irq.h>
 #include <xen/softirq.h>
+#include <xen/debugger.h>
 #include <xen/domain_page.h>
 #include <xen/hypercall.h>
 #include <xen/perfc.h>
@@ -51,7 +52,6 @@
 #include <asm/hvm/trace.h>
 #include <asm/hvm/monitor.h>
 #include <asm/xenoprof.h>
-#include <asm/debugger.h>
 #include <asm/apic.h>
 #include <asm/hvm/nestedhvm.h>
 #include <asm/altp2m.h>
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index ab94a96c4d..505358b656 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -18,6 +18,7 @@
 #include <xen/mm.h>
 #include <xen/param.h>
 #include <xen/irq.h>
+#include <xen/debugger.h>
 #include <xen/delay.h>
 #include <xen/time.h>
 #include <xen/sched.h>
@@ -30,7 +31,6 @@
 #include <asm/msr.h>
 #include <asm/mpspec.h>
 #include <asm/nmi.h>
-#include <asm/debugger.h>
 #include <asm/div64.h>
 #include <asm/apic.h>
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e60af16ddd..8561ffe3fe 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -27,6 +27,7 @@
 #include <xen/init.h>
 #include <xen/sched.h>
 #include <xen/lib.h>
+#include <xen/debugger.h>
 #include <xen/err.h>
 #include <xen/errno.h>
 #include <xen/mm.h>
@@ -62,7 +63,6 @@
 #include <asm/uaccess.h>
 #include <asm/i387.h>
 #include <asm/xstate.h>
-#include <asm/debugger.h>
 #include <asm/msr.h>
 #include <asm/nmi.h>
 #include <asm/xenoprof.h>
diff --git a/xen/arch/x86/x86_64/gdbstub.c b/xen/arch/x86/x86_64/gdbstub.c
index 2626519c89..126af03f50 100644
--- a/xen/arch/x86/x86_64/gdbstub.c
+++ b/xen/arch/x86/x86_64/gdbstub.c
@@ -17,7 +17,8 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <asm/debugger.h>
+#include <xen/debugger.h>
+#include <xen/gdbstub.h>
 
 #define GDB_REG64(r) gdb_write_to_packet_hex(r, sizeof(u64), ctx)
 #define GDB_REG32(r)  gdb_write_to_packet_hex(r, sizeof(u32), ctx)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6b71c6d6a9..a87d814b38 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -11,6 +11,7 @@
 #include <xen/err.h>
 #include <xen/param.h>
 #include <xen/sched.h>
+#include <xen/debugger.h>
 #include <xen/domain.h>
 #include <xen/mm.h>
 #include <xen/event.h>
@@ -33,7 +34,6 @@
 #include <xen/xenoprof.h>
 #include <xen/irq.h>
 #include <xen/argo.h>
-#include <asm/debugger.h>
 #include <asm/p2m.h>
 #include <asm/processor.h>
 #include <public/sched.h>
diff --git a/xen/common/gdbstub.c b/xen/common/gdbstub.c
index 848c1f4327..c233eb2b49 100644
--- a/xen/common/gdbstub.c
+++ b/xen/common/gdbstub.c
@@ -38,12 +38,13 @@
 #include <xen/serial.h>
 #include <xen/irq.h>
 #include <xen/watchdog.h>
-#include <asm/debugger.h>
 #include <xen/init.h>
 #include <xen/param.h>
 #include <xen/smp.h>
 #include <xen/console.h>
 #include <xen/errno.h>
+#include <xen/gdbstub.h>
+#include <xen/debugger.h>
 #include <xen/delay.h>
 #include <asm/byteorder.h>
 
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 8b9f378371..1eafaef9b2 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -3,6 +3,7 @@
  */
 
 #include <asm/regs.h>
+#include <xen/debugger.h>
 #include <xen/delay.h>
 #include <xen/keyhandler.h>
 #include <xen/param.h>
@@ -20,7 +21,6 @@
 #include <xen/mm.h>
 #include <xen/watchdog.h>
 #include <xen/init.h>
-#include <asm/debugger.h>
 #include <asm/div64.h>
 
 static unsigned char keypress_key;
diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index abde48aa4c..f51d44fdc4 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -1,3 +1,4 @@
+#include <xen/debugger.h>
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/param.h>
@@ -8,7 +9,6 @@
 #include <xen/shutdown.h>
 #include <xen/console.h>
 #include <xen/kexec.h>
-#include <asm/debugger.h>
 #include <public/sched.h>
 
 /* opt_noreboot: If true, machine will need manual reset on error. */
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 7d0a603d03..3d1cdde821 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -15,6 +15,7 @@
 #include <xen/init.h>
 #include <xen/event.h>
 #include <xen/console.h>
+#include <xen/debugger.h>
 #include <xen/param.h>
 #include <xen/serial.h>
 #include <xen/softirq.h>
@@ -26,7 +27,6 @@
 #include <xen/kexec.h>
 #include <xen/ctype.h>
 #include <xen/warning.h>
-#include <asm/debugger.h>
 #include <asm/div64.h>
 #include <xen/hypercall.h> /* for do_console_io */
 #include <xen/early_printk.h>
diff --git a/xen/include/asm-arm/debugger.h b/xen/include/asm-arm/debugger.h
deleted file mode 100644
index ac776efa78..0000000000
--- a/xen/include/asm-arm/debugger.h
+++ /dev/null
@@ -1,15 +0,0 @@
-#ifndef __ARM_DEBUGGER_H__
-#define __ARM_DEBUGGER_H__
-
-#define debugger_trap_fatal(v, r) (0)
-#define debugger_trap_immediate() ((void) 0)
-
-#endif /* __ARM_DEBUGGER_H__ */
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index 99803bfd0c..c856c1b795 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/debugger.h
@@ -1,44 +1,26 @@
 /******************************************************************************
- * asm/debugger.h
- * 
- * Generic hooks into arch-dependent Xen.
- * 
- * Each debugger should define two functions here:
- * 
- * 1. debugger_trap_entry(): 
- *  Called at start of any synchronous fault or trap, before any other work
- *  is done. The idea is that if your debugger deliberately caused the trap
- *  (e.g. to implement breakpoints or data watchpoints) then you can take
- *  appropriate action and return a non-zero value to cause early exit from
- *  the trap function.
- * 
- * 2. debugger_trap_fatal():
- *  Called when Xen is about to give up and crash. Typically you will use this
- *  hook to drop into a debug session. It can also be used to hook off
- *  deliberately caused traps (which you then handle and return non-zero).
+ * x86 Debugger Hooks
  *
- * 3. debugger_trap_immediate():
- *  Called if we want to drop into a debugger now.  This is essentially the
- *  same as debugger_trap_fatal, except that we use the current register state
- *  rather than the state which was in effect when we took the trap.
- *  For example: if we're dying because of an unhandled exception, we call
- *  debugger_trap_fatal; if we're dying because of a panic() we call
- *  debugger_trap_immediate().
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
-
 #ifndef __X86_DEBUGGER_H__
 #define __X86_DEBUGGER_H__
 
+#include <xen/domain.h>
+#include <xen/event.h>
+#include <xen/gdbstub.h>
 #include <xen/sched.h>
 #include <asm/regs.h>
 #include <asm/processor.h>
 
 void domain_pause_for_debugger(void);
 
-#ifdef CONFIG_CRASH_DEBUG
-
-#include <xen/gdbstub.h>
-
 static inline bool debugger_trap_fatal(
     unsigned int vector, struct cpu_user_regs *regs)
 {
@@ -74,28 +56,10 @@ static inline bool debugger_trap_entry(
     return false;
 }
 
-#else
-
-static inline bool debugger_trap_fatal(
-    unsigned int vector, struct cpu_user_regs *regs)
-{
-    return false;
-}
-
-#define debugger_trap_immediate() ((void)0)
-
-static inline bool debugger_trap_entry(
-    unsigned int vector, struct cpu_user_regs *regs)
-{
-    return false;
-}
-
-#endif
-
 #ifdef CONFIG_GDBSX
 unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf,
                         unsigned int len, domid_t domid, bool toaddr,
                         uint64_t pgd3);
-#endif
+#endif /* CONFIG_GDBSX */
 
 #endif /* __X86_DEBUGGER_H__ */
diff --git a/xen/include/xen/debugger.h b/xen/include/xen/debugger.h
new file mode 100644
index 0000000000..64745fbe8f
--- /dev/null
+++ b/xen/include/xen/debugger.h
@@ -0,0 +1,69 @@
+/******************************************************************************
+ * Generic hooks into arch-dependent Xen.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Each debugger should define three functions here:
+ *
+ * 1. debugger_trap_entry():
+ *  Called at start of any synchronous fault or trap, before any other work
+ *  is done. The idea is that if your debugger deliberately caused the trap
+ *  (e.g. to implement breakpoints or data watchpoints) then you can take
+ *  appropriate action and return a non-zero value to cause early exit from
+ *  the trap function.
+ *
+ * 2. debugger_trap_fatal():
+ *  Called when Xen is about to give up and crash. Typically you will use this
+ *  hook to drop into a debug session. It can also be used to hook off
+ *  deliberately caused traps (which you then handle and return non-zero).
+ *
+ * 3. debugger_trap_immediate():
+ *  Called if we want to drop into a debugger now.  This is essentially the
+ *  same as debugger_trap_fatal, except that we use the current register state
+ *  rather than the state which was in effect when we took the trap.
+ *  For example: if we're dying because of an unhandled exception, we call
+ *  debugger_trap_fatal; if we're dying because of a panic() we call
+ *  debugger_trap_immediate().
+ */
+
+#ifndef __XEN_DEBUGGER_H__
+#define __XEN_DEBUGGER_H__
+
+#ifdef CONFIG_CRASH_DEBUG
+
+#include <asm/debugger.h>
+
+#else
+
+#include <asm/regs.h>
+#include <asm/processor.h>
+
+static inline void domain_pause_for_debugger(void)
+{
+}
+
+static inline bool debugger_trap_fatal(
+    unsigned int vector, const struct cpu_user_regs *regs)
+{
+    return false;
+}
+
+static inline void debugger_trap_immediate(void)
+{
+}
+
+static inline bool debugger_trap_entry(
+    unsigned int vector, const struct cpu_user_regs *regs)
+{
+    return false;
+}
+
+#endif /* CONFIG_CRASH_DEBUG */
+
+#endif /* __XEN_DEBUGGER_H__ */
-- 
2.30.0

[PATCH v2 3/4] x86/debug: move debugger_trap_entry into debugger.c not inlined

[Bobby Eshleman]

The function debugger_trap_entry() is somewhat large for an inlined
function.  This commit moves debugger_trap_entry() into debugger.c and
makes it not inlined.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
---

Changes in v2:
- Move obj-$(CONFIG_CRASH_DEBUG) += debugger.o to be in alphabetical
  order
- Constify cpu_user_regs *regs and struct vcpu *v in
  debugger_trap_entry()

 xen/arch/x86/Makefile          |  1 +
 xen/arch/x86/debugger.c        | 41 ++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/debugger.h | 26 ++-------------------
 3 files changed, 44 insertions(+), 24 deletions(-)
 create mode 100644 xen/arch/x86/debugger.c

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 2ec883456e..c5c9f37570 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_PV) += compat.o
 obj-$(CONFIG_PV32) += x86_64/compat.o
 obj-$(CONFIG_KEXEC) += crash.o
 obj-$(CONFIG_GDBSX) += debug.o
+obj-$(CONFIG_CRASH_DEBUG) += debugger.o
 obj-y += delay.o
 obj-y += desc.o
 obj-bin-y += dmi_scan.init.o
diff --git a/xen/arch/x86/debugger.c b/xen/arch/x86/debugger.c
new file mode 100644
index 0000000000..985c1275b4
--- /dev/null
+++ b/xen/arch/x86/debugger.c
@@ -0,0 +1,41 @@
+/******************************************************************************
+ * x86 crash debug hooks
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <asm/debugger.h>
+#include <xen/domain.h>
+#include <xen/event.h>
+#include <xen/sched.h>
+
+bool debugger_trap_entry(
+    unsigned int vector, const struct cpu_user_regs *regs)
+{
+    /*
+     * This function is called before any checks are made.  Amongst other
+     * things, be aware that during early boot, current is not a safe pointer
+     * to follow.
+     */
+    const struct vcpu *v = current;
+
+    if ( vector != TRAP_int3 && vector != TRAP_debug )
+        return false;
+
+    if ( guest_mode(regs) && guest_kernel_mode(v, regs) &&
+         v->domain->debugger_attached  )
+    {
+        if ( vector != TRAP_debug ) /* domain pause is good enough */
+            current->arch.gdbsx_vcpu_event = vector;
+        domain_pause_for_debugger();
+        return true;
+    }
+
+    return false;
+}
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index c856c1b795..75e35c7902 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/debugger.h
@@ -31,30 +31,8 @@ static inline bool debugger_trap_fatal(
 /* Int3 is a trivial way to gather cpu_user_regs context. */
 #define debugger_trap_immediate() __asm__ __volatile__ ( "int3" );
 
-static inline bool debugger_trap_entry(
-    unsigned int vector, struct cpu_user_regs *regs)
-{
-    /*
-     * This function is called before any checks are made.  Amongst other
-     * things, be aware that during early boot, current is not a safe pointer
-     * to follow.
-     */
-    struct vcpu *v = current;
-
-    if ( vector != TRAP_int3 && vector != TRAP_debug )
-        return false;
-
-    if ( guest_mode(regs) && guest_kernel_mode(v, regs) &&
-         v->domain->debugger_attached  )
-    {
-        if ( vector != TRAP_debug ) /* domain pause is good enough */
-            current->arch.gdbsx_vcpu_event = vector;
-        domain_pause_for_debugger();
-        return true;
-    }
-
-    return false;
-}
+bool debugger_trap_entry(
+    unsigned int vector, const struct cpu_user_regs *regs);
 
 #ifdef CONFIG_GDBSX
 unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf,
-- 
2.30.0

[PATCH v2 4/4] x86/debug: move domain_pause_for_debugger to debugger.c

[Bobby Eshleman]

The function domain_pause_for_debugger() is conditionally compiled if
CONFIG_CRASH_DEBUG=y.  Instead of placing an extra #ifdef inside
domain.c, this commit moves domain_pause_for_debugger() into
x86/debugger.c which is only built by Kbuild given CONFIG_CRASH_DEBUG=y.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
---

Changes in v2:
- constify struct vcpu *curr

 xen/arch/x86/debugger.c | 12 ++++++++++++
 xen/arch/x86/domain.c   | 14 --------------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/debugger.c b/xen/arch/x86/debugger.c
index 985c1275b4..54e81b99f4 100644
--- a/xen/arch/x86/debugger.c
+++ b/xen/arch/x86/debugger.c
@@ -15,6 +15,18 @@
 #include <xen/event.h>
 #include <xen/sched.h>
 
+void domain_pause_for_debugger(void)
+{
+    const struct vcpu *curr = current;
+    struct domain *d = curr->domain;
+
+    domain_pause_by_systemcontroller_nosync(d);
+
+    /* if gdbsx active, we just need to pause the domain */
+    if ( curr->arch.gdbsx_vcpu_event == 0 )
+        send_global_virq(VIRQ_DEBUGGER);
+}
+
 bool debugger_trap_entry(
     unsigned int vector, const struct cpu_user_regs *regs)
 {
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 5b948ff270..450b2ca831 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2540,20 +2540,6 @@ static int __init init_vcpu_kick_softirq(void)
 }
 __initcall(init_vcpu_kick_softirq);
 
-#ifdef CONFIG_CRASH_DEBUG
-void domain_pause_for_debugger(void)
-{
-    struct vcpu *curr = current;
-    struct domain *d = curr->domain;
-
-    domain_pause_by_systemcontroller_nosync(d);
-
-    /* if gdbsx active, we just need to pause the domain */
-    if ( curr->arch.gdbsx_vcpu_event == 0 )
-        send_global_virq(VIRQ_DEBUGGER);
-}
-#endif
-
 /*
  * Local variables:
  * mode: C
-- 
2.30.0

Re: [PATCH v2 1/4] arm/traps: remove debugger_trap_fatal() calls

[Jan Beulich]

On 14.07.2021 22:37, Bobby Eshleman wrote:
> ARM doesn't actually use debugger_trap_* anything, and is stubbed out.
> 
> Simply remove the calls. This also renders TRAP_invalid_op unused in
> any common code, so remove that definition too.

This part of the description is now stale; I guess if no other need
arises to resubmit the committer could take care of removing this.

Jan

Re: [PATCH v2 2/4] build: use common stubs for debugger_trap_* functions if !CONFIG_CRASH_DEBUG

[Jan Beulich]

On 14.07.2021 22:37, Bobby Eshleman wrote:
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -16,6 +16,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <xen/debugger.h>

I don't think this is needed here; instead I think ...

> @@ -41,7 +42,6 @@
>  #include <asm/acpi.h>
>  #include <asm/cpuerrata.h>
>  #include <asm/cpufeature.h>
> -#include <asm/debugger.h>

... this wants to be done in patch 1 already.

> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -3,6 +3,7 @@
>   */
>  
>  #include <asm/regs.h>
> +#include <xen/debugger.h>
>  #include <xen/delay.h>
>  #include <xen/keyhandler.h>
>  #include <xen/param.h>
> @@ -20,7 +21,6 @@
>  #include <xen/mm.h>
>  #include <xen/watchdog.h>
>  #include <xen/init.h>
> -#include <asm/debugger.h>
>  #include <asm/div64.h>

Not sure about this - as indicated maybe the code needing the include
instead wants to move to x86'es new debugger.c.

> --- /dev/null
> +++ b/xen/include/xen/debugger.h
> @@ -0,0 +1,69 @@
> +/******************************************************************************
> + * Generic hooks into arch-dependent Xen.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Each debugger should define three functions here:
> + *
> + * 1. debugger_trap_entry():
> + *  Called at start of any synchronous fault or trap, before any other work
> + *  is done. The idea is that if your debugger deliberately caused the trap
> + *  (e.g. to implement breakpoints or data watchpoints) then you can take
> + *  appropriate action and return a non-zero value to cause early exit from
> + *  the trap function.
> + *
> + * 2. debugger_trap_fatal():
> + *  Called when Xen is about to give up and crash. Typically you will use this
> + *  hook to drop into a debug session. It can also be used to hook off
> + *  deliberately caused traps (which you then handle and return non-zero).
> + *
> + * 3. debugger_trap_immediate():
> + *  Called if we want to drop into a debugger now.  This is essentially the
> + *  same as debugger_trap_fatal, except that we use the current register state
> + *  rather than the state which was in effect when we took the trap.
> + *  For example: if we're dying because of an unhandled exception, we call
> + *  debugger_trap_fatal; if we're dying because of a panic() we call
> + *  debugger_trap_immediate().
> + */
> +
> +#ifndef __XEN_DEBUGGER_H__
> +#define __XEN_DEBUGGER_H__
> +
> +#ifdef CONFIG_CRASH_DEBUG
> +
> +#include <asm/debugger.h>
> +
> +#else
> +
> +#include <asm/regs.h>
> +#include <asm/processor.h>

I don't think you need either of these for the stubs below.

> +static inline void domain_pause_for_debugger(void)
> +{
> +}
> +
> +static inline bool debugger_trap_fatal(
> +    unsigned int vector, const struct cpu_user_regs *regs)
> +{
> +    return false;
> +}
> +
> +static inline void debugger_trap_immediate(void)
> +{
> +}
> +
> +static inline bool debugger_trap_entry(
> +    unsigned int vector, const struct cpu_user_regs *regs)
> +{
> +    return false;
> +}

Of these stubs, after patch 1 only debugger_trap_immediate() is needed
outside of arch/x86/. Perhaps everything else wants to remain in x86'es
per-arch header?

Jan

Re: [PATCH v2 3/4] x86/debug: move debugger_trap_entry into debugger.c not inlined

[Jan Beulich]

On 14.07.2021 22:37, Bobby Eshleman wrote:
> The function debugger_trap_entry() is somewhat large for an inlined
> function.  This commit moves debugger_trap_entry() into debugger.c and
> makes it not inlined.
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 4/4] x86/debug: move domain_pause_for_debugger to debugger.c

[Jan Beulich]

On 14.07.2021 22:37, Bobby Eshleman wrote:
> The function domain_pause_for_debugger() is conditionally compiled if
> CONFIG_CRASH_DEBUG=y.  Instead of placing an extra #ifdef inside
> domain.c, this commit moves domain_pause_for_debugger() into
> x86/debugger.c which is only built by Kbuild given CONFIG_CRASH_DEBUG=y.
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>

I already gave

Acked-by: Jan Beulich <jbeulich@suse.com>

on this patch. Please accumulate tags as you send new versions, unless
changes you make require you to drop them (and require them to be re-
offered after re-review).

Jan

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

[Andrew Cooper]

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

Re: [PATCH v2 2/4] build: use common stubs for debugger_trap_* functions if !CONFIG_CRASH_DEBUG

[Julien Grall]

Hi,

On 15/07/2021 16:18, Jan Beulich wrote:
> On 14.07.2021 22:37, Bobby Eshleman wrote:
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -16,6 +16,7 @@
>>    * GNU General Public License for more details.
>>    */
>>   
>> +#include <xen/debugger.h>
> 
> I don't think this is needed here; instead I think ...
> 
>> @@ -41,7 +42,6 @@
>>   #include <asm/acpi.h>
>>   #include <asm/cpuerrata.h>
>>   #include <asm/cpufeature.h>
>> -#include <asm/debugger.h>
> 
> ... this wants to be done in patch 1 already.

+1. I was actually going to ask in patch 1 to drop <asm/debugger.h> as 
there should be no more callers of the debugger helpers.

Cheers,

-- 
Julien Grall