[PATCH v5 0/6] Clean up common/arch split for debugger.h

[PATCH v5 0/6] Clean up common/arch split for debugger.h

[Andrew Cooper]

This work is primarily to prevent new architectures from needing to implement
a stub debugger.h for something that is in practice only implemented on x86,
and probably bitrotten into oblivion.  It also resolves a lot of technical
debt on the x86 side.

Andrew Cooper (3):
  x86/gdbsx: Move domain_pause_for_debugger() into gdbsx
  x86/gdbstub: Clean up includes
  x86/debugger: Misc cleanup prior to splitting

Bobby Eshleman (3):
  x86/debugger: Remove debugger_trap_entry()
  x86/gdbsx: Rename debug.c to gdbsx.c
  xen: Split x86/debugger.h into common and arch specific parts

 xen/arch/arm/include/asm/debugger.h | 15 ------
 xen/arch/x86/Makefile               |  2 +-
 xen/arch/x86/domain.c               | 14 ------
 xen/arch/x86/domctl.c               | 14 +-----
 xen/arch/x86/gdbstub.c              |  5 +-
 xen/arch/x86/{debug.c => gdbsx.c}   | 37 +++++++++------
 xen/arch/x86/hvm/svm/svm.c          |  2 +-
 xen/arch/x86/hvm/vmx/realmode.c     |  3 +-
 xen/arch/x86/hvm/vmx/vmx.c          |  2 +-
 xen/arch/x86/include/asm/debugger.h | 93 +++++--------------------------------
 xen/arch/x86/include/asm/gdbsx.h    | 19 ++++++++
 xen/arch/x86/nmi.c                  |  1 -
 xen/arch/x86/setup.c                |  1 -
 xen/arch/x86/traps.c                | 37 +++++++--------
 xen/arch/x86/x86_64/gdbstub.c       |  2 +-
 xen/common/domain.c                 |  1 -
 xen/common/gdbstub.c                |  3 +-
 xen/common/keyhandler.c             |  2 +-
 xen/common/shutdown.c               |  2 +-
 xen/drivers/char/console.c          |  2 +-
 xen/include/xen/debugger.h          | 44 ++++++++++++++++++
 xen/include/xen/gdbstub.h           |  2 +
 22 files changed, 133 insertions(+), 170 deletions(-)
 delete mode 100644 xen/arch/arm/include/asm/debugger.h
 rename xen/arch/x86/{debug.c => gdbsx.c} (87%)
 create mode 100644 xen/arch/x86/include/asm/gdbsx.h
 create mode 100644 xen/include/xen/debugger.h

-- 
2.11.0

[PATCH v5 1/6] x86/debugger: Remove debugger_trap_entry()

[Andrew Cooper]

From: Bobby Eshleman <bobby.eshleman@gmail.com>

debugger_trap_entry() is unrelated to the other contents of debugger.h.  It is
a no-op for everything other than #DB/#BP, and for those it invokes guest
debugging (CONFIG_GDBSX) not host debugging (CONFIG_CRASH_DEBUG).

Furthermore, the description of how to use debugger_trap_entry() is at best,
stale.  It is not called from all exception paths, and because the developer
is forced to modify Xen to perform debugging, editing debugger_trap_entry() is
not the way one would efficiently go about diagnosing the problem.

Simplify everything by expanding debugger_trap_entry() into its two non-empty
locations, fixing bugs with their positioning (vs early exceptions and curr
not being safe to deference) and for #DB, deferring the pause until the
changes in %dr6 are saved to v->arch.dr6 so the debugger can actually see
which condition triggered.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v5:
 * Remove dead logic.  Move GDBSX changes into a later patch.
 * Rewrite commmit message.
---
 xen/arch/x86/include/asm/debugger.h | 42 ++-----------------------------------
 xen/arch/x86/traps.c                | 34 +++++++++++++-----------------
 2 files changed, 16 insertions(+), 60 deletions(-)

diff --git a/xen/arch/x86/include/asm/debugger.h b/xen/arch/x86/include/asm/debugger.h
index 221bcde13796..e83b346a21d1 100644
--- a/xen/arch/x86/include/asm/debugger.h
+++ b/xen/arch/x86/include/asm/debugger.h
@@ -5,19 +5,12 @@
  * 
  * 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():
+ * 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():
+ * 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.
@@ -49,31 +42,6 @@ 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;
-}
-
 #else
 
 static inline bool debugger_trap_fatal(
@@ -84,12 +52,6 @@ static inline bool debugger_trap_fatal(
 
 #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
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 4c38f6c01539..84cd038dc38b 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -946,9 +946,6 @@ void do_trap(struct cpu_user_regs *regs)
     if ( regs->error_code & X86_XEC_EXT )
         goto hardware_trap;
 
-    if ( debugger_trap_entry(trapnr, regs) )
-        return;
-
     ASSERT(trapnr < 32);
 
     if ( guest_mode(regs) )
@@ -1177,9 +1174,6 @@ void do_invalid_op(struct cpu_user_regs *regs)
     int id = -1, lineno;
     const struct virtual_region *region;
 
-    if ( debugger_trap_entry(TRAP_invalid_op, regs) )
-        return;
-
     if ( likely(guest_mode(regs)) )
     {
         if ( pv_emulate_invalid_op(regs) )
@@ -1284,8 +1278,7 @@ void do_invalid_op(struct cpu_user_regs *regs)
 
 void do_int3(struct cpu_user_regs *regs)
 {
-    if ( debugger_trap_entry(TRAP_int3, regs) )
-        return;
+    struct vcpu *curr = current;
 
     if ( !guest_mode(regs) )
     {
@@ -1299,6 +1292,13 @@ void do_int3(struct cpu_user_regs *regs)
         return;
     }
 
+    if ( guest_kernel_mode(curr, regs) && curr->domain->debugger_attached )
+    {
+        curr->arch.gdbsx_vcpu_event = TRAP_int3;
+        domain_pause_for_debugger();
+        return;
+    }
+
     pv_inject_hw_exception(TRAP_int3, X86_EVENT_NO_EC);
 }
 
@@ -1575,9 +1575,6 @@ void do_page_fault(struct cpu_user_regs *regs)
     /* fixup_page_fault() might change regs->error_code, so cache it here. */
     error_code = regs->error_code;
 
-    if ( debugger_trap_entry(TRAP_page_fault, regs) )
-        return;
-
     perfc_incr(page_faults);
 
     /* Any shadow stack access fault is a bug in Xen. */
@@ -1676,9 +1673,6 @@ void do_general_protection(struct cpu_user_regs *regs)
     struct vcpu *v = current;
 #endif
 
-    if ( debugger_trap_entry(TRAP_gp_fault, regs) )
-        return;
-
     if ( regs->error_code & X86_XEC_EXT )
         goto hardware_gp;
 
@@ -1971,9 +1965,6 @@ void do_debug(struct cpu_user_regs *regs)
     /* Stash dr6 as early as possible. */
     dr6 = read_debugreg(6);
 
-    if ( debugger_trap_entry(TRAP_debug, regs) )
-        return;
-
     /*
      * At the time of writing (March 2018), on the subject of %dr6:
      *
@@ -2082,6 +2073,12 @@ void do_debug(struct cpu_user_regs *regs)
     v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
     v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
 
+    if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
+    {
+        domain_pause_for_debugger();
+        return;
+    }
+
     pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
 }
 
@@ -2097,9 +2094,6 @@ void do_entry_CP(struct cpu_user_regs *regs)
     const char *err = "??";
     unsigned int ec = regs->error_code;
 
-    if ( debugger_trap_entry(X86_EXC_CP, regs) )
-        return;
-
     /* Decode ec if possible */
     if ( ec < ARRAY_SIZE(errors) && errors[ec][0] )
         err = errors[ec];
-- 
2.11.0

[PATCH v5 2/6] x86/gdbsx: Rename debug.c to gdbsx.c

[Andrew Cooper]

From: Bobby Eshleman <bobby.eshleman@gmail.com>

debug.c contains only dbg_rw_mem().  Rename it to gdbsx.c.

Move gdbsx_guest_mem_io(), and the prior setup of iop->remain, from domctl.c
to gdbsx.c, merging it with dbg_rw_mem().

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v5:
 * Consolidate hunks from multiple v4 patches
 * Rewrite commit message

The semantics are rather broken.  XEN_DOMCTL_gdbsx_guestmemio only sets
copyback when there's nothing to copy back, and skips copying back in the
-EFAULT case when the iop->remain field is relevant.  Furthermore, it can be
asked to move up to 4GB in one go, with no continuability whatsoever.
---
 xen/arch/x86/Makefile               |  2 +-
 xen/arch/x86/domctl.c               | 14 ++------------
 xen/arch/x86/{debug.c => gdbsx.c}   | 23 ++++++++++-------------
 xen/arch/x86/include/asm/debugger.h |  6 ------
 xen/arch/x86/include/asm/gdbsx.h    | 13 +++++++++++++
 5 files changed, 26 insertions(+), 32 deletions(-)
 rename xen/arch/x86/{debug.c => gdbsx.c} (89%)
 create mode 100644 xen/arch/x86/include/asm/gdbsx.h

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 717bcbcac7a0..177a2ff74272 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -22,7 +22,6 @@ obj-y += cpuid.o
 obj-$(CONFIG_PV) += compat.o
 obj-$(CONFIG_PV32) += x86_64/compat.o
 obj-$(CONFIG_KEXEC) += crash.o
-obj-$(CONFIG_GDBSX) += debug.o
 obj-y += delay.o
 obj-y += desc.o
 obj-bin-y += dmi_scan.init.o
@@ -34,6 +33,7 @@ obj-y += emul-i8254.o
 obj-y += extable.o
 obj-y += flushtlb.o
 obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
+obj-$(CONFIG_GDBSX) += gdbsx.o
 obj-y += hypercall.o
 obj-y += i387.o
 obj-y += i8259.o
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a6aae500a30b..c20ab4352715 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -20,6 +20,8 @@
 #include <xen/console.h>
 #include <xen/iocap.h>
 #include <xen/paging.h>
+
+#include <asm/gdbsx.h>
 #include <asm/irq.h>
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/hvm.h>
@@ -33,20 +35,9 @@
 #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>
 
-#ifdef CONFIG_GDBSX
-static int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop)
-{
-    iop->remain = dbg_rw_mem(iop->gva, guest_handle_from_ptr(iop->uva, void),
-                             iop->len, d, iop->gwr, iop->pgd3val);
-
-    return iop->remain ? -EFAULT : 0;
-}
-#endif
-
 static int update_domain_cpu_policy(struct domain *d,
                                     xen_domctl_cpu_policy_t *xdpc)
 {
@@ -827,7 +818,6 @@ long arch_do_domctl(
 
 #ifdef CONFIG_GDBSX
     case XEN_DOMCTL_gdbsx_guestmemio:
-        domctl->u.gdbsx_guest_memio.remain = domctl->u.gdbsx_guest_memio.len;
         ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio);
         if ( !ret )
            copyback = true;
diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/gdbsx.c
similarity index 89%
rename from xen/arch/x86/debug.c
rename to xen/arch/x86/gdbsx.c
index 91034a852e5f..59eb31fc9a6a 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/gdbsx.c
@@ -18,7 +18,7 @@
 #include <xen/mm.h>
 #include <xen/domain_page.h>
 #include <xen/guest_access.h>
-#include <asm/debugger.h>
+#include <asm/gdbsx.h>
 #include <asm/p2m.h>
 
 typedef unsigned long dbgva_t;
@@ -150,21 +150,18 @@ static unsigned int dbg_rw_guest_mem(struct domain *dp, unsigned long addr,
     return len;
 }
 
-/*
- * addr is guest addr
- * buf is debugger buffer.
- * if toaddr, then addr = buf (write to addr), else buf = addr (rd from guest)
- * pgd3: value of init_mm.pgd[3] in guest. see above.
- * Returns: number of bytes remaining to be copied.
- */
-unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf,
-                        unsigned int len, struct domain *d, bool toaddr,
-                        uint64_t pgd3)
+int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop)
 {
     if ( d && !d->is_dying )
-        len = dbg_rw_guest_mem(d, gva, buf, len, toaddr, pgd3);
+    {
+        iop->remain = dbg_rw_guest_mem(
+            d, iop->gva, guest_handle_from_ptr(iop->uva, void),
+            iop->len, iop->gwr, iop->pgd3val);
+    }
+    else
+        iop->remain = iop->len;
 
-    return len;
+    return iop->remain ? -EFAULT : 0;
 }
 
 /*
diff --git a/xen/arch/x86/include/asm/debugger.h b/xen/arch/x86/include/asm/debugger.h
index e83b346a21d1..c5585752cae7 100644
--- a/xen/arch/x86/include/asm/debugger.h
+++ b/xen/arch/x86/include/asm/debugger.h
@@ -54,10 +54,4 @@ static inline bool debugger_trap_fatal(
 
 #endif
 
-#ifdef CONFIG_GDBSX
-unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf,
-                        unsigned int len, struct domain *d, bool toaddr,
-                        uint64_t pgd3);
-#endif
-
 #endif /* __X86_DEBUGGER_H__ */
diff --git a/xen/arch/x86/include/asm/gdbsx.h b/xen/arch/x86/include/asm/gdbsx.h
new file mode 100644
index 000000000000..eee746fc01d0
--- /dev/null
+++ b/xen/arch/x86/include/asm/gdbsx.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __X86_GDBX_H__
+#define __X86_GDBX_H__
+
+#ifdef CONFIG_GDBSX
+
+struct domain;
+struct xen_domctl_gdbsx_memio;
+
+int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop);
+
+#endif /* CONFIG_GDBSX */
+#endif /* __X86_GDBX_H__ */
-- 
2.11.0

[PATCH v5 3/6] x86/gdbsx: Move domain_pause_for_debugger() into gdbsx

[Andrew Cooper]

domain_pause_for_debugger() is guest debugging (CONFIG_GDBSX) not host
debugging (CONFIG_CRASH_DEBUG).

Move it into the new gdbsx.c to drop the (incorrect) ifdefary, and provide a
static inline in the !CONFIG_GDBSX case so callers can optimise away
everything rather than having to emit a call to an empty function.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v5:
 * Basically new.  Reworked entirely after re-considering other cleanup.
---
 xen/arch/x86/domain.c               | 14 --------------
 xen/arch/x86/gdbsx.c                | 14 ++++++++++++++
 xen/arch/x86/hvm/svm/svm.c          |  2 +-
 xen/arch/x86/hvm/vmx/realmode.c     |  3 ++-
 xen/arch/x86/hvm/vmx/vmx.c          |  2 +-
 xen/arch/x86/include/asm/debugger.h |  2 --
 xen/arch/x86/include/asm/gdbsx.h    |  6 ++++++
 xen/arch/x86/nmi.c                  |  1 -
 xen/arch/x86/traps.c                |  1 +
 9 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a5048ed6546a..a72cc9552ad6 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2540,20 +2540,6 @@ static int __init cf_check init_vcpu_kick_softirq(void)
 }
 __initcall(init_vcpu_kick_softirq);
 
-void domain_pause_for_debugger(void)
-{
-#ifdef CONFIG_CRASH_DEBUG
-    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
diff --git a/xen/arch/x86/gdbsx.c b/xen/arch/x86/gdbsx.c
index 59eb31fc9a6a..6ef46e8ea77d 100644
--- a/xen/arch/x86/gdbsx.c
+++ b/xen/arch/x86/gdbsx.c
@@ -18,6 +18,8 @@
 #include <xen/mm.h>
 #include <xen/domain_page.h>
 #include <xen/guest_access.h>
+#include <xen/event.h>
+
 #include <asm/gdbsx.h>
 #include <asm/p2m.h>
 
@@ -164,6 +166,18 @@ int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop)
     return iop->remain ? -EFAULT : 0;
 }
 
+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);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 2455835eda62..0849a9dc5f41 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 <asm/gdbsx.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 cc23afa788c2..4ac93e081015 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -14,7 +14,8 @@
 #include <xen/sched.h>
 #include <xen/paging.h>
 #include <xen/softirq.h>
-#include <asm/debugger.h>
+
+#include <asm/gdbsx.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 af9ee7cebbe0..cc8c4e9f044a 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 <asm/gdbsx.h>
 #include <asm/apic.h>
 #include <asm/hvm/nestedhvm.h>
 #include <asm/altp2m.h>
diff --git a/xen/arch/x86/include/asm/debugger.h b/xen/arch/x86/include/asm/debugger.h
index c5585752cae7..9a3132356fd6 100644
--- a/xen/arch/x86/include/asm/debugger.h
+++ b/xen/arch/x86/include/asm/debugger.h
@@ -26,8 +26,6 @@
 #include <asm/regs.h>
 #include <asm/processor.h>
 
-void domain_pause_for_debugger(void);
-
 #ifdef CONFIG_CRASH_DEBUG
 
 #include <xen/gdbstub.h>
diff --git a/xen/arch/x86/include/asm/gdbsx.h b/xen/arch/x86/include/asm/gdbsx.h
index eee746fc01d0..938eb74e2e25 100644
--- a/xen/arch/x86/include/asm/gdbsx.h
+++ b/xen/arch/x86/include/asm/gdbsx.h
@@ -9,5 +9,11 @@ struct xen_domctl_gdbsx_memio;
 
 int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop);
 
+void domain_pause_for_debugger(void);
+
+#else
+
+static inline void domain_pause_for_debugger(void) {}
+
 #endif /* CONFIG_GDBSX */
 #endif /* __X86_GDBX_H__ */
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 302eaf2ff39a..765602374802 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -30,7 +30,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 84cd038dc38b..d91532461189 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -58,6 +58,7 @@
 #include <xen/bitops.h>
 #include <asm/desc.h>
 #include <asm/debugreg.h>
+#include <asm/gdbsx.h>
 #include <asm/smp.h>
 #include <asm/flushtlb.h>
 #include <asm/uaccess.h>
-- 
2.11.0

[PATCH v5 4/6] x86/gdbstub: Clean up includes

[Andrew Cooper]

common/gdbstub.c wants struct gdb_context but only gets it transitively
through asm/debugger.h.  None of */gdbstub.c should include asm/debugger.h so
include xen/gdbstub.h instead.

Forward declare struct cpu_user_regs in xen/gdbstub.h so it doesn't depend on
the include order to compile.

x86/setup.c doesn't need xen/gdbstub.h at all, so drop it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v5:
 * New
---
 xen/arch/x86/gdbstub.c        | 5 ++++-
 xen/arch/x86/setup.c          | 1 -
 xen/arch/x86/x86_64/gdbstub.c | 2 +-
 xen/common/gdbstub.c          | 3 ++-
 xen/include/xen/gdbstub.h     | 2 ++
 5 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/gdbstub.c b/xen/arch/x86/gdbstub.c
index 8f4f49fd3b54..961cae0be74f 100644
--- a/xen/arch/x86/gdbstub.c
+++ b/xen/arch/x86/gdbstub.c
@@ -18,7 +18,10 @@
  * 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 <asm/x86-defns.h>
+
+#include <xen/gdbstub.h>
 
 u16
 gdb_arch_signal_num(struct cpu_user_regs *regs, unsigned long cookie)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 6f20e178929f..53a73010e029 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -15,7 +15,6 @@
 #include <xen/multiboot.h>
 #include <xen/domain_page.h>
 #include <xen/version.h>
-#include <xen/gdbstub.h>
 #include <xen/hypercall.h>
 #include <xen/keyhandler.h>
 #include <xen/numa.h>
diff --git a/xen/arch/x86/x86_64/gdbstub.c b/xen/arch/x86/x86_64/gdbstub.c
index 2626519c89c7..8287124dfb1d 100644
--- a/xen/arch/x86/x86_64/gdbstub.c
+++ b/xen/arch/x86/x86_64/gdbstub.c
@@ -17,7 +17,7 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <asm/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/gdbstub.c b/xen/common/gdbstub.c
index d6872721dc0d..df8d122bce8d 100644
--- a/xen/common/gdbstub.c
+++ b/xen/common/gdbstub.c
@@ -38,13 +38,14 @@
 #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/delay.h>
+#include <xen/gdbstub.h>
+
 #include <asm/byteorder.h>
 
 /* Printk isn't particularly safe just after we've trapped to the
diff --git a/xen/include/xen/gdbstub.h b/xen/include/xen/gdbstub.h
index 0b2041095d88..18c960969b76 100644
--- a/xen/include/xen/gdbstub.h
+++ b/xen/include/xen/gdbstub.h
@@ -25,6 +25,8 @@
 
 #ifdef CONFIG_CRASH_DEBUG
 
+struct cpu_user_regs;
+
 struct gdb_context {
     int                 serhnd;           /* handle on our serial line */
     int                 console_steal_id; /* handle on stolen console */
-- 
2.11.0

[PATCH v5 5/6] x86/debugger: Misc cleanup prior to splitting

[Andrew Cooper]

 * Remove inappropriate semicolon from debugger_trap_immediate()
 * Try to explain what debugger_trap_fatal() is doing, and write it in a more
   legible way.
 * Drop unecessary includes.  This includes common/domain.c which doesn't use
   any debugger functionality, even prior to this cleaup.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v5:
 * New
---
 xen/arch/x86/include/asm/debugger.h | 17 +++++++++++------
 xen/common/domain.c                 |  1 -
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/include/asm/debugger.h b/xen/arch/x86/include/asm/debugger.h
index 9a3132356fd6..5bac2ee4c2a4 100644
--- a/xen/arch/x86/include/asm/debugger.h
+++ b/xen/arch/x86/include/asm/debugger.h
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0 */
 /******************************************************************************
  * asm/debugger.h
  * 
@@ -22,23 +23,27 @@
 #ifndef __X86_DEBUGGER_H__
 #define __X86_DEBUGGER_H__
 
-#include <xen/sched.h>
-#include <asm/regs.h>
-#include <asm/processor.h>
-
 #ifdef CONFIG_CRASH_DEBUG
 
 #include <xen/gdbstub.h>
+#include <xen/stdbool.h>
+
+#include <asm/x86-defns.h>
 
+/* Returns true if GDB handled the trap, or it is surviveable. */
 static inline bool debugger_trap_fatal(
     unsigned int vector, struct cpu_user_regs *regs)
 {
     int rc = __trap_to_gdb(regs, vector);
-    return ((rc == 0) || (vector == TRAP_int3));
+
+    if ( rc == 0 )
+        return true;
+
+    return vector == X86_EXC_BP;
 }
 
 /* Int3 is a trivial way to gather cpu_user_regs context. */
-#define debugger_trap_immediate() __asm__ __volatile__ ( "int3" );
+#define debugger_trap_immediate() __asm__ __volatile__ ( "int3" )
 
 #else
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 351029f8b239..8d2c2a989708 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -33,7 +33,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>
-- 
2.11.0

[PATCH v5 6/6] xen: Split x86/debugger.h into common and arch specific parts

[Andrew Cooper]

From: Bobby Eshleman <bobby.eshleman@gmail.com>

With all the non-CONFIG_CRASH_DEBUG functionality moved elsewhere, split
x86/debugger.h in two, with the stubs and explanation moved to xen/debugger.h.

In particular, this means that arches only need to provide an $arch/debugger.h
if they implement CONFIG_CRASH_DEBUG, and ARM's stub can be deleted.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>

v5:
 * Clean up includes
 * Drop rendundant comments
---
 xen/arch/arm/include/asm/debugger.h                | 15 --------
 xen/arch/x86/include/asm/debugger.h                | 34 +----------------
 xen/arch/x86/traps.c                               |  2 +-
 xen/common/keyhandler.c                            |  2 +-
 xen/common/shutdown.c                              |  2 +-
 xen/drivers/char/console.c                         |  2 +-
 .../x86/include/asm => include/xen}/debugger.h     | 44 +++++++---------------
 7 files changed, 20 insertions(+), 81 deletions(-)
 delete mode 100644 xen/arch/arm/include/asm/debugger.h
 copy xen/{arch/x86/include/asm => include/xen}/debugger.h (54%)

diff --git a/xen/arch/arm/include/asm/debugger.h b/xen/arch/arm/include/asm/debugger.h
deleted file mode 100644
index ac776efa7899..000000000000
--- a/xen/arch/arm/include/asm/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/arch/x86/include/asm/debugger.h b/xen/arch/x86/include/asm/debugger.h
index 5bac2ee4c2a4..a5c299c6c34d 100644
--- a/xen/arch/x86/include/asm/debugger.h
+++ b/xen/arch/x86/include/asm/debugger.h
@@ -1,30 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /******************************************************************************
- * asm/debugger.h
- * 
- * Generic hooks into arch-dependent Xen.
- * 
- * Each debugger should define two functions here:
- * 
- * 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).
+ * xen/arch/x86/include/asm/debugger.h
  *
- * 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().
+ * x86-specific debugger hooks.
  */
-
 #ifndef __X86_DEBUGGER_H__
 #define __X86_DEBUGGER_H__
 
-#ifdef CONFIG_CRASH_DEBUG
-
 #include <xen/gdbstub.h>
 #include <xen/stdbool.h>
 
@@ -45,16 +27,4 @@ static inline bool debugger_trap_fatal(
 /* Int3 is a trivial way to gather cpu_user_regs context. */
 #define debugger_trap_immediate() __asm__ __volatile__ ( "int3" )
 
-#else
-
-static inline bool debugger_trap_fatal(
-    unsigned int vector, struct cpu_user_regs *regs)
-{
-    return false;
-}
-
-#define debugger_trap_immediate() ((void)0)
-
-#endif
-
 #endif /* __X86_DEBUGGER_H__ */
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index d91532461189..25bffe47d7ae 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -36,6 +36,7 @@
 #include <xen/shutdown.h>
 #include <xen/guest_access.h>
 #include <asm/regs.h>
+#include <xen/debugger.h>
 #include <xen/delay.h>
 #include <xen/event.h>
 #include <xen/spinlock.h>
@@ -64,7 +65,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/common/keyhandler.c b/xen/common/keyhandler.c
index ca9ee0790149..0a551033c443 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 abde48aa4ca0..a933ee001ea4 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -2,13 +2,13 @@
 #include <xen/lib.h>
 #include <xen/param.h>
 #include <xen/sched.h>
+#include <xen/debugger.h>
 #include <xen/domain.h>
 #include <xen/delay.h>
 #include <xen/watchdog.h>
 #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 d9d6556c2293..f9937c5134c0 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/arch/x86/include/asm/debugger.h b/xen/include/xen/debugger.h
similarity index 54%
copy from xen/arch/x86/include/asm/debugger.h
copy to xen/include/xen/debugger.h
index 5bac2ee4c2a4..72684268aff7 100644
--- a/xen/arch/x86/include/asm/debugger.h
+++ b/xen/include/xen/debugger.h
@@ -1,11 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /******************************************************************************
- * asm/debugger.h
- * 
- * Generic hooks into arch-dependent Xen.
- * 
- * Each debugger should define two functions here:
- * 
+ * Arch specific debuggers should implement:
+ *
  * 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
@@ -20,41 +16,29 @@
  *  debugger_trap_immediate().
  */
 
-#ifndef __X86_DEBUGGER_H__
-#define __X86_DEBUGGER_H__
+#ifndef __XEN_DEBUGGER_H__
+#define __XEN_DEBUGGER_H__
 
 #ifdef CONFIG_CRASH_DEBUG
 
-#include <xen/gdbstub.h>
-#include <xen/stdbool.h>
-
-#include <asm/x86-defns.h>
-
-/* Returns true if GDB handled the trap, or it is surviveable. */
-static inline bool debugger_trap_fatal(
-    unsigned int vector, struct cpu_user_regs *regs)
-{
-    int rc = __trap_to_gdb(regs, vector);
-
-    if ( rc == 0 )
-        return true;
+#include <asm/debugger.h>
 
-    return vector == X86_EXC_BP;
-}
+#else
 
-/* Int3 is a trivial way to gather cpu_user_regs context. */
-#define debugger_trap_immediate() __asm__ __volatile__ ( "int3" )
+#include <xen/stdbool.h>
 
-#else
+struct cpu_user_regs;
 
 static inline bool debugger_trap_fatal(
-    unsigned int vector, struct cpu_user_regs *regs)
+    unsigned int vector, const struct cpu_user_regs *regs)
 {
     return false;
 }
 
-#define debugger_trap_immediate() ((void)0)
+static inline void debugger_trap_immediate(void)
+{
+}
 
-#endif
+#endif /* CONFIG_CRASH_DEBUG */
 
-#endif /* __X86_DEBUGGER_H__ */
+#endif /* __XEN_DEBUGGER_H__ */
-- 
2.11.0

Re: [PATCH v5 1/6] x86/debugger: Remove debugger_trap_entry()

[Jan Beulich]

On 20.04.2022 16:13, Andrew Cooper wrote:
> From: Bobby Eshleman <bobby.eshleman@gmail.com>
> 
> debugger_trap_entry() is unrelated to the other contents of debugger.h.  It is
> a no-op for everything other than #DB/#BP, and for those it invokes guest
> debugging (CONFIG_GDBSX) not host debugging (CONFIG_CRASH_DEBUG).
> 
> Furthermore, the description of how to use debugger_trap_entry() is at best,
> stale.  It is not called from all exception paths,

But on almost all (before this change) - the exception looks to be
#NM.

> and because the developer
> is forced to modify Xen to perform debugging, editing debugger_trap_entry() is
> not the way one would efficiently go about diagnosing the problem.

Shouldn't it be the remote end to request which exceptions it wants
to be notified of? If so, removing the hook invocation isn't very
helpful.

Jan

Re: [PATCH v5 2/6] x86/gdbsx: Rename debug.c to gdbsx.c

[Jan Beulich]

On 20.04.2022 16:13, Andrew Cooper wrote:
> From: Bobby Eshleman <bobby.eshleman@gmail.com>
> 
> debug.c contains only dbg_rw_mem().  Rename it to gdbsx.c.
> 
> Move gdbsx_guest_mem_io(), and the prior setup of iop->remain, from domctl.c
> to gdbsx.c, merging it with dbg_rw_mem().
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> The semantics are rather broken.  XEN_DOMCTL_gdbsx_guestmemio only sets
> copyback when there's nothing to copy back, and skips copying back in the
> -EFAULT case when the iop->remain field is relevant.  Furthermore, it can be
> asked to move up to 4GB in one go, with no continuability whatsoever.

The last point perhaps isn't overly much of a problem for this specific
operation.

Jan

Re: [PATCH v5 3/6] x86/gdbsx: Move domain_pause_for_debugger() into gdbsx

[Jan Beulich]

On 20.04.2022 16:13, Andrew Cooper wrote:
> domain_pause_for_debugger() is guest debugging (CONFIG_GDBSX) not host
> debugging (CONFIG_CRASH_DEBUG).
> 
> Move it into the new gdbsx.c to drop the (incorrect) ifdefary, and provide a
> static inline in the !CONFIG_GDBSX case so callers can optimise away
> everything rather than having to emit a call to an empty function.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v5 4/6] x86/gdbstub: Clean up includes

[Jan Beulich]

On 20.04.2022 16:13, Andrew Cooper wrote:
> common/gdbstub.c wants struct gdb_context but only gets it transitively
> through asm/debugger.h.  None of */gdbstub.c should include asm/debugger.h so
> include xen/gdbstub.h instead.
> 
> Forward declare struct cpu_user_regs in xen/gdbstub.h so it doesn't depend on
> the include order to compile.
> 
> x86/setup.c doesn't need xen/gdbstub.h at all, so drop it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v5 5/6] x86/debugger: Misc cleanup prior to splitting

[Jan Beulich]

On 20.04.2022 16:13, Andrew Cooper wrote:
>  * Remove inappropriate semicolon from debugger_trap_immediate()
>  * Try to explain what debugger_trap_fatal() is doing, and write it in a more
>    legible way.
>  * Drop unecessary includes.  This includes common/domain.c which doesn't use
>    any debugger functionality, even prior to this cleaup.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v5 6/6] xen: Split x86/debugger.h into common and arch specific parts

[Jan Beulich]

On 20.04.2022 16:13, Andrew Cooper wrote:
> From: Bobby Eshleman <bobby.eshleman@gmail.com>
> 
> With all the non-CONFIG_CRASH_DEBUG functionality moved elsewhere, split
> x86/debugger.h in two, with the stubs and explanation moved to xen/debugger.h.
> 
> In particular, this means that arches only need to provide an $arch/debugger.h
> if they implement CONFIG_CRASH_DEBUG, and ARM's stub can be deleted.
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Julien Grall <jgrall@amazon.com>

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

Re: [PATCH v5 1/6] x86/debugger: Remove debugger_trap_entry()

[Andrew Cooper]

On 21/04/2022 14:02, Jan Beulich wrote:
> On 20.04.2022 16:13, Andrew Cooper wrote:
>> From: Bobby Eshleman <bobby.eshleman@gmail.com>
>>
>> debugger_trap_entry() is unrelated to the other contents of debugger.h.  It is
>> a no-op for everything other than #DB/#BP, and for those it invokes guest
>> debugging (CONFIG_GDBSX) not host debugging (CONFIG_CRASH_DEBUG).
>>
>> Furthermore, the description of how to use debugger_trap_entry() is at best,
>> stale.  It is not called from all exception paths,
> But on almost all (before this change) - the exception looks to be
> #NM.
>
>> and because the developer
>> is forced to modify Xen to perform debugging, editing debugger_trap_entry() is
>> not the way one would efficiently go about diagnosing the problem.
> Shouldn't it be the remote end to request which exceptions it wants
> to be notified of? If so, removing the hook invocation isn't very
> helpful.

That's not part of the gdb remote protocol.

In normal conditions, gdb gets to see see anything which manifests as a
signal.  It does not get to see anything which is resolved by the kernel
behind the scenes.  #NM you've already identified, and most #PF's would
count too.  Back in the 32bit days, Xen-induced #GP/#SS's for non-4G
segments would count too.

But in addition to filtering Xen's idea of "fixing up behind the
scenes", you also need to Xen to understand when to skip notifications
based on what a PV guest kernel can fix up, and this is getting even
further out of gdb's comfort zone.

debugger_trap_entry() is empty (WRT gdbstub) specifically because it's
description is nonsense in any practical debugging scenario.  And lets
not start on the fact that the lack of ability to invoke
pv_event_inject() means that any fault from userspace will livelock
under debugging.

Deleting it is absolutely the right way forward, because a theoretical
future with someone wiring this up would have to start again from scratch.

Not that qualifies as a good reason in isolation, do_trap() contains
unreachable logic because the compiler can't figure out that #DB/#BP are
handled via alternative paths, and the gdbsx logic is dead.

~Andrew

Re: [PATCH v5 2/6] x86/gdbsx: Rename debug.c to gdbsx.c

[Andrew Cooper]

On 21/04/2022 14:06, Jan Beulich wrote:
> On 20.04.2022 16:13, Andrew Cooper wrote:
>> From: Bobby Eshleman <bobby.eshleman@gmail.com>
>>
>> debug.c contains only dbg_rw_mem().  Rename it to gdbsx.c.
>>
>> Move gdbsx_guest_mem_io(), and the prior setup of iop->remain, from domctl.c
>> to gdbsx.c, merging it with dbg_rw_mem().
>>
>> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> The semantics are rather broken.  XEN_DOMCTL_gdbsx_guestmemio only sets
>> copyback when there's nothing to copy back, and skips copying back in the
>> -EFAULT case when the iop->remain field is relevant.  Furthermore, it can be
>> asked to move up to 4GB in one go, with no continuability whatsoever.
> The last point perhaps isn't overly much of a problem for this specific
> operation.

It's also not terribly hard to fix, but I really don't have time to get
bogged down in "make the gdbsx hypercalls sane". 
XEN_DOMCTL_gdbsx_domstatus is a disaster, and there are far better ways
of doing this.  https://www.youtube.com/watch?v=osZeioYKsxA is one which
was presented at XenSummit in 2019.

~Andrew

Re: [PATCH v5 1/6] x86/debugger: Remove debugger_trap_entry()

[Jan Beulich]

On 21.04.2022 19:23, Andrew Cooper wrote:
> On 21/04/2022 14:02, Jan Beulich wrote:
>> On 20.04.2022 16:13, Andrew Cooper wrote:
>>> From: Bobby Eshleman <bobby.eshleman@gmail.com>
>>>
>>> debugger_trap_entry() is unrelated to the other contents of debugger.h.  It is
>>> a no-op for everything other than #DB/#BP, and for those it invokes guest
>>> debugging (CONFIG_GDBSX) not host debugging (CONFIG_CRASH_DEBUG).
>>>
>>> Furthermore, the description of how to use debugger_trap_entry() is at best,
>>> stale.  It is not called from all exception paths,
>> But on almost all (before this change) - the exception looks to be
>> #NM.
>>
>>> and because the developer
>>> is forced to modify Xen to perform debugging, editing debugger_trap_entry() is
>>> not the way one would efficiently go about diagnosing the problem.
>> Shouldn't it be the remote end to request which exceptions it wants
>> to be notified of? If so, removing the hook invocation isn't very
>> helpful.
> 
> That's not part of the gdb remote protocol.
> 
> In normal conditions, gdb gets to see see anything which manifests as a
> signal.  It does not get to see anything which is resolved by the kernel
> behind the scenes.  #NM you've already identified, and most #PF's would
> count too.  Back in the 32bit days, Xen-induced #GP/#SS's for non-4G
> segments would count too.
> 
> But in addition to filtering Xen's idea of "fixing up behind the
> scenes", you also need to Xen to understand when to skip notifications
> based on what a PV guest kernel can fix up, and this is getting even
> further out of gdb's comfort zone.
> 
> debugger_trap_entry() is empty (WRT gdbstub) specifically because it's
> description is nonsense in any practical debugging scenario.  And lets
> not start on the fact that the lack of ability to invoke
> pv_event_inject() means that any fault from userspace will livelock
> under debugging.
> 
> Deleting it is absolutely the right way forward, because a theoretical
> future with someone wiring this up would have to start again from scratch.
> 
> Not that qualifies as a good reason in isolation, do_trap() contains
> unreachable logic because the compiler can't figure out that #DB/#BP are
> handled via alternative paths, and the gdbsx logic is dead.

The patch description could certainly do with expanding some along these
lines.

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

Jan

Re: [PATCH v5 2/6] x86/gdbsx: Rename debug.c to gdbsx.c

[Jan Beulich]

On 20.04.2022 16:13, Andrew Cooper wrote:
> From: Bobby Eshleman <bobby.eshleman@gmail.com>
> 
> debug.c contains only dbg_rw_mem().  Rename it to gdbsx.c.
> 
> Move gdbsx_guest_mem_io(), and the prior setup of iop->remain, from domctl.c
> to gdbsx.c, merging it with dbg_rw_mem().
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> v5:
>  * Consolidate hunks from multiple v4 patches
>  * Rewrite commit message
> 
> The semantics are rather broken.  XEN_DOMCTL_gdbsx_guestmemio only sets
> copyback when there's nothing to copy back, and skips copying back in the
> -EFAULT case when the iop->remain field is relevant.  Furthermore, it can be
> asked to move up to 4GB in one go, with no continuability whatsoever.
> ---
>  xen/arch/x86/Makefile               |  2 +-
>  xen/arch/x86/domctl.c               | 14 ++------------
>  xen/arch/x86/{debug.c => gdbsx.c}   | 23 ++++++++++-------------
>  xen/arch/x86/include/asm/debugger.h |  6 ------
>  xen/arch/x86/include/asm/gdbsx.h    | 13 +++++++++++++
>  5 files changed, 26 insertions(+), 32 deletions(-)
>  rename xen/arch/x86/{debug.c => gdbsx.c} (89%)
>  create mode 100644 xen/arch/x86/include/asm/gdbsx.h

As I've realized only while reviewing your newer gdbsx patch, this
should have come with an update to ./MAINTAINERS. Quite possibly one
simply deleting the entire entry there.

Jan