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

[PATCH 1/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>

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
---
 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  | 66 +++++----------------------
 xen/include/xen/debugger.h      | 81 +++++++++++++++++++++++++++++++++
 19 files changed, 115 insertions(+), 86 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 4ccb6e7d18..5a0a5eff1d 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -41,7 +41,7 @@
 #include <asm/acpi.h>
 #include <asm/cpuerrata.h>
 #include <asm/cpufeature.h>
-#include <asm/debugger.h>
+#include <xen/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..4386e8d1b1 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -19,7 +19,7 @@
 #include <xen/mm.h>
 #include <xen/domain_page.h>
 #include <xen/guest_access.h>
-#include <asm/debugger.h>
+#include <xen/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..47448f2f8c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -16,6 +16,7 @@
 #include <xen/errno.h>
 #include <xen/sched.h>
 #include <xen/domain.h>
+#include <xen/debugger.h>
 #include <xen/smp.h>
 #include <xen/delay.h>
 #include <xen/softirq.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..1bc8ba7251 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -33,7 +33,7 @@
 #include <public/vm_event.h>
 #include <asm/mem_sharing.h>
 #include <asm/xstate.h>
-#include <asm/debugger.h>
+#include <xen/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..f20cbf1f45 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 <asm/uaccess.h>
+#include <xen/debugger.h>
+#include <xen/gdbstub.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..d7ec7c15f9 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -58,7 +58,7 @@
 #include <asm/hvm/trace.h>
 #include <asm/hap.h>
 #include <asm/apic.h>
-#include <asm/debugger.h>
+#include <xen/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..1f8513c132 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -14,7 +14,7 @@
 #include <xen/sched.h>
 #include <xen/paging.h>
 #include <xen/softirq.h>
-#include <asm/debugger.h>
+#include <xen/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..1820e4be0c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -51,7 +51,7 @@
 #include <asm/hvm/trace.h>
 #include <asm/hvm/monitor.h>
 #include <asm/xenoprof.h>
-#include <asm/debugger.h>
+#include <xen/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..eaf404402d 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -30,7 +30,7 @@
 #include <asm/msr.h>
 #include <asm/mpspec.h>
 #include <asm/nmi.h>
-#include <asm/debugger.h>
+#include <xen/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..44811c9599 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -62,7 +62,7 @@
 #include <asm/uaccess.h>
 #include <asm/i387.h>
 #include <asm/xstate.h>
-#include <asm/debugger.h>
+#include <xen/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..88ba680fe7 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -33,7 +33,7 @@
 #include <xen/xenoprof.h>
 #include <xen/irq.h>
 #include <xen/argo.h>
-#include <asm/debugger.h>
+#include <xen/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..6f3d7ca72f 100644
--- a/xen/common/gdbstub.c
+++ b/xen/common/gdbstub.c
@@ -38,7 +38,8 @@
 #include <xen/serial.h>
 #include <xen/irq.h>
 #include <xen/watchdog.h>
-#include <asm/debugger.h>
+#include <xen/debugger.h>
+#include <xen/gdbstub.h>
 #include <xen/init.h>
 #include <xen/param.h>
 #include <xen/smp.h>
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 8b9f378371..5c66ca0056 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -20,7 +20,7 @@
 #include <xen/mm.h>
 #include <xen/watchdog.h>
 #include <xen/init.h>
-#include <asm/debugger.h>
+#include <xen/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..c82a4999d9 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -8,7 +8,7 @@
 #include <xen/shutdown.h>
 #include <xen/console.h>
 #include <xen/kexec.h>
-#include <asm/debugger.h>
+#include <xen/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..060d32757f 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -26,7 +26,7 @@
 #include <xen/kexec.h>
 #include <xen/ctype.h>
 #include <xen/warning.h>
-#include <asm/debugger.h>
+#include <xen/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..38359da0a1 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/sched.h>
 #include <asm/regs.h>
 #include <asm/processor.h>
+#include <xen/gdbstub.h>
+#include <xen/domain.h>
+#include <xen/event.h>
+#include <xen/sched.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,4 @@ 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 /* __X86_DEBUGGER_H__ */
diff --git a/xen/include/xen/debugger.h b/xen/include/xen/debugger.h
new file mode 100644
index 0000000000..d6d820f4e5
--- /dev/null
+++ b/xen/include/xen/debugger.h
@@ -0,0 +1,81 @@
+/******************************************************************************
+ * 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__
+
+/* Dummy value used by ARM stubs. */
+#ifndef TRAP_invalid_op
+# define TRAP_invalid_op 6
+#endif
+
+#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 */
+
+#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 /* CONFIG_GDBSX */
+
+#endif /* __XEN_DEBUGGER_H__ */
-- 
2.30.0

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

[Jan Beulich]

On 13.07.2021 03:59, Bobby Eshleman wrote:
> --- 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 <asm/uaccess.h>
> +#include <xen/debugger.h>
> +#include <xen/gdbstub.h>

Here and in at least one more case below: Our usual pattern is to
have xen/ ones before asm/ ones. And (leaving aside existing
screwed code) ...

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -58,7 +58,7 @@
>  #include <asm/hvm/trace.h>
>  #include <asm/hap.h>
>  #include <asm/apic.h>
> -#include <asm/debugger.h>
> +#include <xen/debugger.h>
>  #include <asm/hvm/monitor.h>
>  #include <asm/monitor.h>
>  #include <asm/xstate.h>

... we also try to avoid introducing any mixture. Plus ...

> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -14,7 +14,7 @@
>  #include <xen/sched.h>
>  #include <xen/paging.h>
>  #include <xen/softirq.h>
> -#include <asm/debugger.h>
> +#include <xen/debugger.h>

... we strive to have new insertions be sorted alphabetically. When
the existing section to insert into isn't suitably sorted yet, what
I normally do is try to find a place where at least the immediately
adjacent neighbors then fit the sorting goal.

Sorry, all just nits, but their scope is about the entire patch.

> --- /dev/null
> +++ b/xen/include/xen/debugger.h
> @@ -0,0 +1,81 @@
> +/******************************************************************************
> + * Generic hooks into arch-dependent Xen.

Now that you move this to be generic, I think it better also would
indeed be. See below.

> + * 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/>.
> + *
> + *

Nit: No double blank (comment) lines please.

> + * 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__
> +
> +/* Dummy value used by ARM stubs. */
> +#ifndef TRAP_invalid_op
> +# define TRAP_invalid_op 6
> +#endif

To avoid the need to introduce this, please flip ordering with the
subsequent patch.

> +#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)

I'm afraid the concept of a vector may not be arch-independent.

> +{
> +    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 */
> +
> +#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 /* CONFIG_GDBSX */

I'm afraid this whole construct isn't arch independent, at least as long
as it has the "pgd3" parameter, documented elsewhere to mean "the value of
init_mm.pgd[3] in a PV guest" (whatever this really is in a 64-bit guest,
or in a non-Linux one).

But I don't see why this needs moving to common code in the first place:
It's x86-specific both on the producer and the consumer sides.

Jan

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

[Bob Eshleman]

On 7/14/21 2:34 AM, Jan Beulich wrote:
> 
> ... we strive to have new insertions be sorted alphabetically. When
> the existing section to insert into isn't suitably sorted yet, what
> I normally do is try to find a place where at least the immediately
> adjacent neighbors then fit the sorting goal.
> 
> Sorry, all just nits, but their scope is about the entire patch.
> 

No worries at all, I welcome the corrections (I need to learn the
proper style somehow).

>> --- /dev/null
>> +++ b/xen/include/xen/debugger.h

...

>> +
>> +static inline bool debugger_trap_fatal(
>> +    unsigned int vector, const struct cpu_user_regs *regs)
> 
> I'm afraid the concept of a vector may not be arch-independent.
>

The only way I can imagine it not being arch-independent
is if it is thought of as a trap number or id, instead of
implying an entry in a vectored trap table.  I don't
really understand this subsystem, so I'm probably missing
context.

Are you suggesting a rename or a different approach entirely?

> 
> Jan
> 

Thanks Jan, everything besides the vector thing are incorporated in
the v2 I've just sent out.

-- 
Bobby Eshleman
SE at Vates SAS

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

[Jan Beulich]

On 14.07.2021 23:03, Bob Eshleman wrote:
> On 7/14/21 2:34 AM, Jan Beulich wrote:
>>> +static inline bool debugger_trap_fatal(
>>> +    unsigned int vector, const struct cpu_user_regs *regs)
>>
>> I'm afraid the concept of a vector may not be arch-independent.
>>
> 
> The only way I can imagine it not being arch-independent
> is if it is thought of as a trap number or id, instead of
> implying an entry in a vectored trap table.  I don't
> really understand this subsystem, so I'm probably missing
> context.
> 
> Are you suggesting a rename or a different approach entirely?

I'm suggesting that we shouldn't be claiming something to be an
abstraction when it isn't really. There's exactly one use of
debugger_trap_fatal() outside x86/ after patch 1 of this series:

static void do_debugger_trap_fatal(struct cpu_user_regs *regs)
{
    (void)debugger_trap_fatal(0xf001, regs);
    ...
}

That's very certainly _not_ arch-independent. Hence I'd rather
see some #ifdef-ary added there and the function remaining
x86-specific for the time being, i.e. until such a time when
someone might come forward with a suitable abstraction. Perhaps
(as an alternative to #ifdef-ary) the '%' debug key should be
x86-specific altogether, and perhaps its setup and handling
could be moved into the new debugger.c?

Jan

[PATCH 2/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 ------
 xen/include/xen/debugger.h | 5 -----
 2 files changed, 11 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 5a0a5eff1d..0310bc91a0 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);
diff --git a/xen/include/xen/debugger.h b/xen/include/xen/debugger.h
index d6d820f4e5..8297e582e4 100644
--- a/xen/include/xen/debugger.h
+++ b/xen/include/xen/debugger.h
@@ -36,11 +36,6 @@
 #ifndef __XEN_DEBUGGER_H__
 #define __XEN_DEBUGGER_H__
 
-/* Dummy value used by ARM stubs. */
-#ifndef TRAP_invalid_op
-# define TRAP_invalid_op 6
-#endif
-
 #ifdef CONFIG_CRASH_DEBUG
 
 #include <asm/debugger.h>
-- 
2.30.0

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

[Bobby Eshleman]

The function debugger_trap_entry() is rather 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>
---
 xen/arch/x86/Makefile          |  1 +
 xen/arch/x86/debugger.c        | 41 ++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/debugger.h | 29 ++----------------------
 3 files changed, 44 insertions(+), 27 deletions(-)
 create mode 100644 xen/arch/x86/debugger.c

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 2ec883456e..ba274fb8e6 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -32,6 +32,7 @@ obj-y += emul-i8254.o
 obj-y += extable.o
 obj-y += flushtlb.o
 obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
+obj-$(CONFIG_CRASH_DEBUG) += debugger.o
 obj-y += hypercall.o
 obj-y += i387.o
 obj-y += i8259.o
diff --git a/xen/arch/x86/debugger.c b/xen/arch/x86/debugger.c
new file mode 100644
index 0000000000..6f33f509ff
--- /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, 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;
+}
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index 38359da0a1..0e30d27a4e 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/debugger.h
@@ -15,9 +15,6 @@
 #include <asm/regs.h>
 #include <asm/processor.h>
 #include <xen/gdbstub.h>
-#include <xen/domain.h>
-#include <xen/event.h>
-#include <xen/sched.h>
 
 void domain_pause_for_debugger(void);
 
@@ -31,29 +28,7 @@ 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, struct cpu_user_regs *regs);
 
 #endif /* __X86_DEBUGGER_H__ */
-- 
2.30.0

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

[Jan Beulich]

On 13.07.2021 03:59, Bobby Eshleman wrote:
> The function debugger_trap_entry() is rather large for an inlined
> function.

Well, yes, perhaps.

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -32,6 +32,7 @@ obj-y += emul-i8254.o
>  obj-y += extable.o
>  obj-y += flushtlb.o
>  obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
> +obj-$(CONFIG_CRASH_DEBUG) += debugger.o

Please insert at the appropriate location, rather than breaking the
(mostly) alphabetical sorting.

> --- /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, struct cpu_user_regs *regs)

As you have to touch this anyway, can you please bring it in line
with the stubs introduced in the first patch, by constifying the
2nd parameter?

> +{
> +    /*
> +     * 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;

This one can (and hence better would) gain a "const" as well.

Jan

[PATCH 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>
---
 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 6f33f509ff..4f7c44600f 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)
+{
+    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, struct cpu_user_regs *regs)
 {
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 47448f2f8c..545da32c3b 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 4/4] x86/debug: move domain_pause_for_debugger to debugger.c

[Jan Beulich]

On 13.07.2021 03:59, 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>

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

> --- 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)
> +{
> +    struct vcpu *curr = current;

... "const" added here while you're moving this anyway.

Jan