[PATCH v3 0/6] Remove unconditional arch dependency on asm/debugger.h

[PATCH v3 0/6] Remove unconditional arch dependency on asm/debugger.h

[Bobby Eshleman]

This series removes the unconditional requirement that all architectures
implement asm/debugger.h. It additionally removes arm's debugger.h and
disentangles some of the x86 gdbsx/gdbstub/generic debugger code.

Additionally, this series does the following:
- Provides generic stubs when !CONFIG_CRASH_DEBUG
- Adds stronger separation between gdbstub, gdbsx, and generic debugger
  code.

The patches in this v3 are quite a bit different than in v2, so
per-patch changes are omitted. This difference in the patchset version
is largely due to the need to decouple the debugger_trap_* functions.

Changes from v2:
- The first patch drops ARM's calls to the debugger stubs, removing the
  need to add fake values.
- No debugger.c is added, as it was unnecessary when code was moved into
  existing and appropriate files.
- debugger_trap_entry() expands inline into its few call sites
- debug.c becomes gdbsx.c
- All gdbsx functions move into gdbsx.c and become dependent on
  CONFIG_GDBSX (instead of CONFIG_CRASH_DEBUG as was the case for
  domain_pause_for_debugger(), for example)

It's worth noting that debugger.h is still not truly generic as
debugger_trap_fatal() for x86 necessarily calls into the gdbstub… but
further generalization is unnecessary while it is still only
backed by the gdbstub.

As I'm not *exactly* an expert on this code, so feel free to inform me of my
confusion where you see it.

Bobby Eshleman (6):
  arm/traps: remove debugger_trap_fatal() calls
  x86/debugger: separate Xen and guest debugging debugger_trap_*
    functions
  arch/x86: rename debug.c to gdbsx.c
  x86/gdbsx: expand dbg_rw_mem() inline
  arch/x86: move domain_pause_for_debugger() to domain.h
  x86: change asm/debugger.h to xen/debugger.h

 xen/arch/arm/traps.c              |  7 ----
 xen/arch/x86/Makefile             |  2 +-
 xen/arch/x86/domain.c             |  2 +-
 xen/arch/x86/domctl.c             | 12 +-----
 xen/arch/x86/{debug.c => gdbsx.c} | 30 +++++++-------
 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                |  1 -
 xen/arch/x86/traps.c              | 51 ++++++++++++++----------
 xen/common/domain.c               |  2 +-
 xen/common/gdbstub.c              |  2 +-
 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    | 65 +------------------------------
 xen/include/asm-x86/domain.h      |  2 +
 xen/include/asm-x86/gdbsx.h       | 17 ++++++++
 xen/include/xen/debugger.h        | 51 ++++++++++++++++++++++++
 20 files changed, 127 insertions(+), 144 deletions(-)
 rename xen/arch/x86/{debug.c => gdbsx.c} (89%)
 delete mode 100644 xen/include/asm-arm/debugger.h
 create mode 100644 xen/include/asm-x86/gdbsx.h
 create mode 100644 xen/include/xen/debugger.h

-- 
2.32.0

[PATCH v3 1/6] arm/traps: remove debugger_trap_fatal() calls

[Bobby Eshleman]

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

This commit simply removes the unneeded calls.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 4ccb6e7d18..889650ba63 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -41,7 +41,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>
@@ -1266,10 +1265,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 +1276,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.32.0

[PATCH v3 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions

[Bobby Eshleman]

Unlike debugger_trap_fatal() and debugger_trap_immediate(),
debugger_trap_entry() is specific to guest debugging and *NOT* the
debugging of Xen itself. That is, it is part of gdbsx functionality and
not the Xen gdstub. This is evidenced by debugger_trap_fatal()'s usage
of domain_pause_for_debugger(). Because of this, debugger_trap_entry()
does not belong alongside the generic Xen debugger functionality.

This commit fixes this by expanding inline debugger_trap_entry() into
its usage in x86/traps.c and stubbing out domain_pause_for_debugger()
when !CONFIG_GDBSX. Properly placing what was debugger_trap_entry()
under the scope of gdbsx instead of gdbstub.

The function calls that caused an effective no-op and early exit out of
debugger_trap_entry() are removed completely (when the trapnr is not
int3/debug).

This commit is one of a series geared towards removing the unnecessary
requirement that all architectures to implement <asm/debugger.h>.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
---
 xen/arch/x86/domain.c          |  2 +-
 xen/arch/x86/traps.c           | 48 ++++++++++++++++++++--------------
 xen/include/asm-x86/debugger.h | 42 ++---------------------------
 3 files changed, 31 insertions(+), 61 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ef1812dc14..70894ff826 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2541,7 +2541,7 @@ __initcall(init_vcpu_kick_softirq);
 
 void domain_pause_for_debugger(void)
 {
-#ifdef CONFIG_CRASH_DEBUG
+#ifdef CONFIG_GDBSX
     struct vcpu *curr = current;
     struct domain *d = curr->domain;
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e60af16ddd..d0a4c0ea74 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -858,13 +858,20 @@ static 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) )
     {
+        struct vcpu *curr = current;
+        if ( (trapnr == TRAP_debug || trapnr == TRAP_int3) &&
+              guest_kernel_mode(curr, regs) &&
+              curr->domain->debugger_attached )
+        {
+            if ( trapnr != TRAP_debug )
+                curr->arch.gdbsx_vcpu_event = trapnr;
+            domain_pause_for_debugger();
+            return;
+        }
         pv_inject_hw_exception(trapnr,
                                (TRAP_HAVE_EC & (1u << trapnr))
                                ? regs->error_code : X86_EVENT_NO_EC);
@@ -1094,9 +1101,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) )
@@ -1201,8 +1205,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) )
     {
@@ -1215,6 +1218,12 @@ void do_int3(struct cpu_user_regs *regs)
 
         return;
     }
+    else 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);
 }
@@ -1492,9 +1501,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. */
@@ -1593,9 +1599,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;
 
@@ -1888,9 +1891,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:
      *
@@ -1994,6 +1994,11 @@ void do_debug(struct cpu_user_regs *regs)
 
         return;
     }
+    else if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
+    {
+        domain_pause_for_debugger();
+        return;
+    }
 
     /* Save debug status register where guest OS can peek at it */
     v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
@@ -2014,9 +2019,6 @@ void do_entry_CP(struct cpu_user_regs *regs)
     const char *err = "??";
     unsigned int ec = regs->error_code;
 
-    if ( debugger_trap_entry(TRAP_debug, regs) )
-        return;
-
     /* Decode ec if possible */
     if ( ec < ARRAY_SIZE(errors) && errors[ec][0] )
         err = errors[ec];
@@ -2028,6 +2030,12 @@ void do_entry_CP(struct cpu_user_regs *regs)
      */
     if ( guest_mode(regs) )
     {
+        struct vcpu *curr = current;
+        if ( guest_kernel_mode(curr, regs) && curr->domain->debugger_attached )
+        {
+            domain_pause_for_debugger();
+            return;
+        }
         gprintk(XENLOG_ERR, "Hit #CP[%04x] in guest context %04x:%p\n",
                 ec, regs->cs, _p(regs->rip));
         ASSERT_UNREACHABLE();
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index 99803bfd0c..cd6b9477f7 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/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():
+ * 1. 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():
+ * 2. 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
-- 
2.32.0

[PATCH v3 3/6] arch/x86: rename debug.c to gdbsx.c

[Bobby Eshleman]

This commit renames debug.c to gdbsx.c to clarify its purpose.

The function gdbsx_guest_mem_io() is moved from domctl.c to gdbsx.c.

Although gdbsx_guest_mem_io() is conditionally removed from its single
call site in domctl.c upon !CONFIG_GDBSX and so no stub is technically
necessary, this commit adds a stub that would preserve the functioning
of that call site if the #ifdef CONFIG_GDBSX were to ever be removed or
the function were to ever be called outside of such an ifdef block.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
---
 xen/arch/x86/Makefile             |  2 +-
 xen/arch/x86/domctl.c             | 12 +-----------
 xen/arch/x86/{debug.c => gdbsx.c} | 12 ++++++++++--
 xen/include/asm-x86/debugger.h    |  6 ------
 xen/include/asm-x86/gdbsx.h       | 17 +++++++++++++++++
 5 files changed, 29 insertions(+), 20 deletions(-)
 rename xen/arch/x86/{debug.c => gdbsx.c} (93%)
 create mode 100644 xen/include/asm-x86/gdbsx.h

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index fe38cfd544..ef8c2c4770 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -20,7 +20,7 @@ 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-$(CONFIG_GDBSX) += gdbsx.o
 obj-y += delay.o
 obj-y += desc.o
 obj-bin-y += dmi_scan.init.o
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 26a76d2be9..a492fe140e 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -20,6 +20,7 @@
 #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 +34,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(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
-{
-    iop->remain = dbg_rw_mem(iop->gva, guest_handle_from_ptr(iop->uva, void),
-                             iop->len, domid, 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)
 {
diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/gdbsx.c
similarity index 93%
rename from xen/arch/x86/debug.c
rename to xen/arch/x86/gdbsx.c
index d90dc93056..adea0f017b 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/gdbsx.c
@@ -19,7 +19,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;
@@ -158,7 +158,7 @@ static unsigned int dbg_rw_guest_mem(struct domain *dp, unsigned long addr,
  * 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,
+static 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)
 {
@@ -174,6 +174,14 @@ unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf,
     return len;
 }
 
+int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
+{
+    iop->remain = dbg_rw_mem(iop->gva, guest_handle_from_ptr(iop->uva, void),
+                             iop->len, domid, iop->gwr, iop->pgd3val);
+
+    return iop->remain ? -EFAULT : 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index cd6b9477f7..ed4d5c829b 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/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, domid_t domid, bool toaddr,
-                        uint64_t pgd3);
-#endif
-
 #endif /* __X86_DEBUGGER_H__ */
diff --git a/xen/include/asm-x86/gdbsx.h b/xen/include/asm-x86/gdbsx.h
new file mode 100644
index 0000000000..2b8812881d
--- /dev/null
+++ b/xen/include/asm-x86/gdbsx.h
@@ -0,0 +1,17 @@
+#ifndef __X86_GDBX_H
+#define __X86_GDBX_H__
+
+#ifdef CONFIG_GDBSX
+
+int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop);
+
+#else
+
+static inline int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
+{
+    return -EOPNOTSUPP;
+}
+
+#endif
+
+#endif
-- 
2.32.0

[PATCH v3 4/6] x86/gdbsx: expand dbg_rw_mem() inline

[Bobby Eshleman]

Because dbg_rw_mem() has only a single call site, this commit
expands it inline.
---
 xen/arch/x86/gdbsx.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/gdbsx.c b/xen/arch/x86/gdbsx.c
index adea0f017b..4cb8e042f9 100644
--- a/xen/arch/x86/gdbsx.c
+++ b/xen/arch/x86/gdbsx.c
@@ -151,33 +151,23 @@ 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.
- */
-static 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)
+int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
 {
     struct domain *d = rcu_lock_domain_by_id(domid);
 
-    if ( d )
+    if ( d && !d->is_dying )
     {
-        if ( !d->is_dying )
-            len = dbg_rw_guest_mem(d, gva, buf, len, toaddr, pgd3);
-        rcu_unlock_domain(d);
+        iop->remain = dbg_rw_guest_mem(
+                d, iop->gva, guest_handle_from_ptr(iop->uva, void),
+                iop->len, domid, iop->pgd3val);
+    }
+    else
+    {
+        iop->remain = iop->len;
     }
 
-    return len;
-}
-
-int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
-{
-    iop->remain = dbg_rw_mem(iop->gva, guest_handle_from_ptr(iop->uva, void),
-                             iop->len, domid, iop->gwr, iop->pgd3val);
+    if ( d )
+        rcu_unlock_domain(d);
 
     return iop->remain ? -EFAULT : 0;
 }
-- 
2.32.0

[PATCH v3 5/6] arch/x86: move domain_pause_for_debugger() to domain.h

[Bobby Eshleman]

domain_pause_for_debugger() was previously in debugger.h.  This commit
moves it to domain.h because its implementation is in domain.c.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
---
Changes in v3:
- domain_pause_for_debugger() is now moved into debugger.h, not a new
  file debugger.c

 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              | 1 -
 xen/arch/x86/traps.c            | 1 +
 xen/include/asm-x86/debugger.h  | 2 --
 xen/include/asm-x86/domain.h    | 2 ++
 7 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 642a64b747..84448e496f 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -36,6 +36,7 @@
 #include <asm/processor.h>
 #include <asm/amd.h>
 #include <asm/debugreg.h>
+#include <asm/domain.h>
 #include <asm/msr.h>
 #include <asm/i387.h>
 #include <asm/iocap.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..5c4b1910a9 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 <asm/domain.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..6fd59865c7 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -27,6 +27,7 @@
 #include <xen/hypercall.h>
 #include <xen/perfc.h>
 #include <asm/current.h>
+#include <asm/domain.h>
 #include <asm/io.h>
 #include <asm/iocap.h>
 #include <asm/regs.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..11d5f5a917 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 d0a4c0ea74..5947ed25d6 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -63,6 +63,7 @@
 #include <asm/i387.h>
 #include <asm/xstate.h>
 #include <asm/debugger.h>
+#include <asm/domain.h>
 #include <asm/msr.h>
 #include <asm/nmi.h>
 #include <asm/xenoprof.h>
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index ed4d5c829b..8f6222956e 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/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/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 92d54de0b9..de854b5bfa 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -672,6 +672,8 @@ void update_guest_memory_policy(struct vcpu *v,
 
 void domain_cpu_policy_changed(struct domain *d);
 
+void domain_pause_for_debugger(void);
+
 bool update_runstate_area(struct vcpu *);
 bool update_secondary_system_time(struct vcpu *,
                                   struct vcpu_time_info *);
-- 
2.32.0

[PATCH v3 6/6] x86: change asm/debugger.h to xen/debugger.h

[Bobby Eshleman]

This commit allows non-x86 architecture to omit the file asm/debugger.h
if they do not require it.  It changes debugger.h to be a general
xen/debugger.h which, if CONFIG_CRASH_DEBUG, resolves to include
asm/debugger.h.

It also changes all asm/debugger.h includes to xen/debugger.h.

Because it is no longer required, arm/debugger.h is removed.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
---
Changes in v3:
- No longer introduces a fake TRAP_invalid_op for arm to consume, it
  is not necessary given the removal of the arm calls now precedes
  this patch.
- Instead of defining prototypes that arch/ is expected to implement,
  this version simply #includes <arch/debugger.h> when CONFIG_CRASH_DEBUG.

 xen/arch/x86/traps.c           |  2 +-
 xen/common/domain.c            |  2 +-
 xen/common/gdbstub.c           |  2 +-
 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 | 15 ----------
 xen/include/xen/debugger.h     | 51 ++++++++++++++++++++++++++++++++++
 9 files changed, 57 insertions(+), 36 deletions(-)
 delete mode 100644 xen/include/asm-arm/debugger.h
 create mode 100644 xen/include/xen/debugger.h

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 5947ed25d6..a540f0832e 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -35,6 +35,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>
@@ -62,7 +63,6 @@
 #include <asm/uaccess.h>
 #include <asm/i387.h>
 #include <asm/xstate.h>
-#include <asm/debugger.h>
 #include <asm/domain.h>
 #include <asm/msr.h>
 #include <asm/nmi.h>
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..1d7b98cdac 100644
--- a/xen/common/gdbstub.c
+++ b/xen/common/gdbstub.c
@@ -38,12 +38,12 @@
 #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/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..a933ee001e 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 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 8f6222956e..b9eeed395c 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/debugger.h
@@ -25,9 +25,6 @@
 #include <xen/sched.h>
 #include <asm/regs.h>
 #include <asm/processor.h>
-
-#ifdef CONFIG_CRASH_DEBUG
-
 #include <xen/gdbstub.h>
 
 static inline bool debugger_trap_fatal(
@@ -40,16 +37,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/include/xen/debugger.h b/xen/include/xen/debugger.h
new file mode 100644
index 0000000000..166fad9d2e
--- /dev/null
+++ b/xen/include/xen/debugger.h
@@ -0,0 +1,51 @@
+/******************************************************************************
+ * 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 two functions here:
+ *
+ * 1. 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).
+ *
+ * 2. 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>
+
+static inline bool debugger_trap_fatal(
+    unsigned int vector, const struct cpu_user_regs *regs)
+{
+    return false;
+}
+
+static inline void debugger_trap_immediate(void)
+{
+}
+
+#endif /* CONFIG_CRASH_DEBUG */
+
+#endif /* __XEN_DEBUGGER_H__ */
-- 
2.32.0

Re: [PATCH v3 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions

[Andrew Cooper]

On 18/08/2021 21:29, Bobby Eshleman wrote:
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index e60af16ddd..d0a4c0ea74 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -858,13 +858,20 @@ static 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) )
>      {
> +        struct vcpu *curr = current;
> +        if ( (trapnr == TRAP_debug || trapnr == TRAP_int3) &&
> +              guest_kernel_mode(curr, regs) &&
> +              curr->domain->debugger_attached )
> +        {
> +            if ( trapnr != TRAP_debug )
> +                curr->arch.gdbsx_vcpu_event = trapnr;
> +            domain_pause_for_debugger();
> +            return;
> +        }

There's no need for this.  Both TRAP_debug and TRAP_int3 have their own
custom handers, and don't use do_trap().

> @@ -1215,6 +1218,12 @@ void do_int3(struct cpu_user_regs *regs)
>  
>          return;
>      }
> +    else if ( guest_kernel_mode(curr, regs) && curr->domain->debugger_attached )

if ( foo ) { ...; return; } else if ( bar )  is a dangerous anti-pattern.

This should just be a plain if().

> @@ -1994,6 +1994,11 @@ void do_debug(struct cpu_user_regs *regs)
>  
>          return;
>      }
> +    else if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )

Same here.

> @@ -2028,6 +2030,12 @@ void do_entry_CP(struct cpu_user_regs *regs)
>       */
>      if ( guest_mode(regs) )
>      {
> +        struct vcpu *curr = current;
> +        if ( guest_kernel_mode(curr, regs) && curr->domain->debugger_attached )
> +        {
> +            domain_pause_for_debugger();
> +            return;
> +        }
>          gprintk(XENLOG_ERR, "Hit #CP[%04x] in guest context %04x:%p\n",
>                  ec, regs->cs, _p(regs->rip));
>          ASSERT_UNREACHABLE();

This path is fatal to Xen, because it should be impossible.  If we ever
get around to supporting CET for PV guests, #CP still isn't #DB/#BP so
shouldn't pause for the debugger.

I can fix all of these on commit.

~Andrew

Re: [PATCH v3 3/6] arch/x86: rename debug.c to gdbsx.c

[Andrew Cooper]

On 18/08/2021 21:29, Bobby Eshleman wrote:
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index fe38cfd544..ef8c2c4770 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -20,7 +20,7 @@ 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-$(CONFIG_GDBSX) += gdbsx.o

This wants moving now to retain alphabetical order.

> diff --git a/xen/include/asm-x86/gdbsx.h b/xen/include/asm-x86/gdbsx.h
> new file mode 100644
> index 0000000000..2b8812881d
> --- /dev/null
> +++ b/xen/include/asm-x86/gdbsx.h
> @@ -0,0 +1,17 @@
> +#ifndef __X86_GDBX_H
> +#define __X86_GDBX_H__

Broken header guard.

> +
> +#ifdef CONFIG_GDBSX
> +
> +int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop);
> +
> +#else
> +

Need to include <xen/errno.h> to avoid breaking the !GDBSX build.

Can fix all on commit.

~Andrew

Re: [PATCH v3 4/6] x86/gdbsx: expand dbg_rw_mem() inline

[Andrew Cooper]

On 18/08/2021 21:29, Bobby Eshleman wrote:
> Because dbg_rw_mem() has only a single call site, this commit
> expands it inline.

Missing a SoB.

Otherwise, looks ok.

Re: [PATCH v3 6/6] x86: change asm/debugger.h to xen/debugger.h

[Andrew Cooper]

On 18/08/2021 21:29, Bobby Eshleman wrote:
> diff --git a/xen/include/xen/debugger.h b/xen/include/xen/debugger.h
> new file mode 100644
> index 0000000000..166fad9d2e
> --- /dev/null
> +++ b/xen/include/xen/debugger.h
> @@ -0,0 +1,51 @@
> +/******************************************************************************
> + * 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 two functions here:
> + *
> + * 1. 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).
> + *
> + * 2. 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>

This include should be deleted, and replaced with simply

struct cpu_user_regs;

~Andrew

> +
> +static inline bool debugger_trap_fatal(
> +    unsigned int vector, const struct cpu_user_regs *regs)
> +{
> +    return false;
> +}
> +
> +static inline void debugger_trap_immediate(void)
> +{
> +}
> +
> +#endif /* CONFIG_CRASH_DEBUG */
> +
> +#endif /* __XEN_DEBUGGER_H__ */

Re: [PATCH v3 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions

[Jan Beulich]

On 18.08.2021 22:29, Bobby Eshleman wrote:
> Unlike debugger_trap_fatal() and debugger_trap_immediate(),
> debugger_trap_entry() is specific to guest debugging and *NOT* the
> debugging of Xen itself. That is, it is part of gdbsx functionality and
> not the Xen gdstub. This is evidenced by debugger_trap_fatal()'s usage
> of domain_pause_for_debugger(). Because of this, debugger_trap_entry()
> does not belong alongside the generic Xen debugger functionality.

I'm not convinced this is what the original intentions were. Instead I
think this was meant to be a generic hook function which initially
only cared to deal with the gdbsx needs.

Jan

Re: [PATCH v3 3/6] arch/x86: rename debug.c to gdbsx.c

[Jan Beulich]

On 18.08.2021 22:29, Bobby Eshleman wrote:
> --- /dev/null
> +++ b/xen/include/asm-x86/gdbsx.h
> @@ -0,0 +1,17 @@
> +#ifndef __X86_GDBX_H
> +#define __X86_GDBX_H__
> +
> +#ifdef CONFIG_GDBSX
> +
> +int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop);
> +
> +#else
> +
> +static inline int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
> +{
> +    return -EOPNOTSUPP;
> +}
> +
> +#endif

In addition to what Andrew has said, you also want to make sure
- domid_t is actually declared (need to include public/xen.h, I think),
- struct xen_domctl_gdbsx_memio has a forward declaration (ahead of the
  #ifdef).
This is so the header can actually be #include-d without needing to
worry about prereq #include-s.

Jan

Re: [PATCH v3 4/6] x86/gdbsx: expand dbg_rw_mem() inline

[Jan Beulich]

On 18.08.2021 22:29, Bobby Eshleman wrote:
> --- a/xen/arch/x86/gdbsx.c
> +++ b/xen/arch/x86/gdbsx.c
> @@ -151,33 +151,23 @@ 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.
> - */
> -static 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)
> +int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
>  {
>      struct domain *d = rcu_lock_domain_by_id(domid);
>  
> -    if ( d )
> +    if ( d && !d->is_dying )
>      {
> -        if ( !d->is_dying )
> -            len = dbg_rw_guest_mem(d, gva, buf, len, toaddr, pgd3);
> -        rcu_unlock_domain(d);
> +        iop->remain = dbg_rw_guest_mem(
> +                d, iop->gva, guest_handle_from_ptr(iop->uva, void),
> +                iop->len, domid, iop->pgd3val);
> +    }
> +    else
> +    {
> +        iop->remain = iop->len;
>      }

Nit: Generally we omit the braces in cases like this one.

Jan

Re: [PATCH v3 5/6] arch/x86: move domain_pause_for_debugger() to domain.h

[Jan Beulich]

On 18.08.2021 22:29, Bobby Eshleman wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -36,6 +36,7 @@
>  #include <asm/processor.h>
>  #include <asm/amd.h>
>  #include <asm/debugreg.h>
> +#include <asm/domain.h>
>  #include <asm/msr.h>
>  #include <asm/i387.h>
>  #include <asm/iocap.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>

While it's generally a good idea to explicitly #include headers that a
source file depends upon, I'm not convinced in this case: sched.h
includes xen/domain.h, which in turn includes asm/domain.h. And this
dependency chain is very unlikely to go away, as sched.h needs to see
full struct domain and struct vcpu, all of which come from */domain.h.

Jan

Re: [PATCH v3 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions

[Andrew Cooper]

On 24/08/2021 13:16, Jan Beulich wrote:
> On 18.08.2021 22:29, Bobby Eshleman wrote:
>> Unlike debugger_trap_fatal() and debugger_trap_immediate(),
>> debugger_trap_entry() is specific to guest debugging and *NOT* the
>> debugging of Xen itself. That is, it is part of gdbsx functionality and
>> not the Xen gdstub. This is evidenced by debugger_trap_fatal()'s usage
>> of domain_pause_for_debugger(). Because of this, debugger_trap_entry()
>> does not belong alongside the generic Xen debugger functionality.
> I'm not convinced this is what the original intentions were. Instead I
> think this was meant to be a generic hook function which initially
> only cared to deal with the gdbsx needs.

It doesn't exactly matter what the original intentions where - what we
currently have is unused and and a clear source of confusion between two
unrelated subsystems.

It is unclear that the gdbstub is even usable, given at least a decade
of bitrot.

Keeping an empty static inline in the enabled case is nonsense, because
at the point you need to edit Xen to insert some real debugging, there
are better ways to do it in something which isn't even a catch-all
despite appearing to be one.

~Andrew

Re: [PATCH v3 1/6] arm/traps: remove debugger_trap_fatal() calls

[Julien Grall]

Hi Bobby,

On 18/08/2021 21:29, Bobby Eshleman wrote:
> ARM doesn't actually use debugger_trap_* anything, and is stubbed out.
I am guessing the plan was to add support for debugging support on Arm. 
But it never made it and it should be easy to re-introduce. So...

> This commit simply removes the unneeded calls.
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 6/6] x86: change asm/debugger.h to xen/debugger.h

[Julien Grall]

Hi,

On 18/08/2021 21:29, Bobby Eshleman wrote:
> This commit allows non-x86 architecture to omit the file asm/debugger.h
> if they do not require it.  It changes debugger.h to be a general
> xen/debugger.h which, if CONFIG_CRASH_DEBUG, resolves to include
> asm/debugger.h.
> 
> It also changes all asm/debugger.h includes to xen/debugger.h.
> 
> Because it is no longer required, arm/debugger.h is removed.
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
> ---
> Changes in v3:
> - No longer introduces a fake TRAP_invalid_op for arm to consume, it
>    is not necessary given the removal of the arm calls now precedes
>    this patch.
> - Instead of defining prototypes that arch/ is expected to implement,
>    this version simply #includes <arch/debugger.h> when CONFIG_CRASH_DEBUG.
> 
>   xen/arch/x86/traps.c           |  2 +-
>   xen/common/domain.c            |  2 +-
>   xen/common/gdbstub.c           |  2 +-
>   xen/common/keyhandler.c        |  2 +-
>   xen/common/shutdown.c          |  2 +-
>   xen/drivers/char/console.c     |  2 +-
>   xen/include/asm-arm/debugger.h | 15 ----------

For the Arm bits:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions

[Jan Beulich]

On 24.08.2021 14:41, Andrew Cooper wrote:
> On 24/08/2021 13:16, Jan Beulich wrote:
>> On 18.08.2021 22:29, Bobby Eshleman wrote:
>>> Unlike debugger_trap_fatal() and debugger_trap_immediate(),
>>> debugger_trap_entry() is specific to guest debugging and *NOT* the
>>> debugging of Xen itself. That is, it is part of gdbsx functionality and
>>> not the Xen gdstub. This is evidenced by debugger_trap_fatal()'s usage
>>> of domain_pause_for_debugger(). Because of this, debugger_trap_entry()
>>> does not belong alongside the generic Xen debugger functionality.
>> I'm not convinced this is what the original intentions were. Instead I
>> think this was meant to be a generic hook function which initially
>> only cared to deal with the gdbsx needs.
> 
> It doesn't exactly matter what the original intentions where - what we
> currently have is unused and and a clear source of confusion between two
> unrelated subsystems.
> 
> It is unclear that the gdbstub is even usable, given at least a decade
> of bitrot.
> 
> Keeping an empty static inline in the enabled case is nonsense, because
> at the point you need to edit Xen to insert some real debugging, there
> are better ways to do it in something which isn't even a catch-all
> despite appearing to be one.

Perhaps I should have said explicitly that my remark isn't an objection
to the code change, but a suggestion to weaken the claims made in the
description.

Jan

Re: [PATCH v3 3/6] arch/x86: rename debug.c to gdbsx.c

[Bobby Eshleman]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Tue, Aug 24, 2021, 2:20 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 18.08.2021 22:29, Bobby Eshleman wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-x86/gdbsx.h
> > @@ -0,0 +1,17 @@
> > +#ifndef __X86_GDBX_H
> > +#define __X86_GDBX_H__
> > +
> > +#ifdef CONFIG_GDBSX
> > +
> > +int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio
> *iop);
> > +
> > +#else
> > +
> > +static inline int gdbsx_guest_mem_io(domid_t domid, struct
> xen_domctl_gdbsx_memio *iop)
> > +{
> > +    return -EOPNOTSUPP;
> > +}
> > +
> > +#endif
>
> In addition to what Andrew has said, you also want to make sure
> - domid_t is actually declared (need to include public/xen.h, I think),
> - struct xen_domctl_gdbsx_memio has a forward declaration (ahead of the
>   #ifdef).
> This is so the header can actually be #include-d without needing to
> worry about prereq #include-s.
>
> Jan
>

Roger that.

I'll be away from work on vacation until mid September but I'll get to
these changes and the others when I get back, if there are some pending /
still uncommitted.

Writing this from my phone, so sorry for gmail's email strangeness.

>

[-- Attachment #2: Type: text/html, Size: 2011 bytes --]