[Xen-devel] [PATCH v3 0/2] xen: make more debugger support code conditional

[Xen-devel] [PATCH v3 0/2] xen: make more debugger support code conditional

[Juergen Gross]

Support for debugging the hypervisor of guests via gdb/gdbsx should be
configurable.

Changes in V3:
- remove possibility to access hypervisor memory via gdbsx domctl
- default gdbsx support to on
- some code moving

Changes in V2:
- split support for gdbstub and gdbsx (Andrew Cooper)

Juergen Gross (2):
  xen: put more code under CONFIG_CRASH_DEBUG
  xen: make gdbsx support configurable

 xen/Kconfig.debug               |  8 +++++
 xen/arch/x86/Kconfig            |  1 -
 xen/arch/x86/Makefile           |  2 +-
 xen/arch/x86/debug.c            | 78 +++++------------------------------------
 xen/arch/x86/domain.c           | 13 +++++++
 xen/arch/x86/domctl.c           |  4 +++
 xen/arch/x86/hvm/vmx/realmode.c |  1 +
 xen/common/Kconfig              |  3 --
 xen/common/domain.c             | 14 --------
 xen/include/asm-x86/debugger.h  | 34 +++++++++++-------
 xen/include/xen/sched.h         |  1 -
 11 files changed, 58 insertions(+), 101 deletions(-)

-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 1/2] xen: put more code under CONFIG_CRASH_DEBUG

[Juergen Gross]

Some code is not needed with CONFIG_CRASH_DEBUG, so only include it if
CONFIG_CRASH_DEBUG is defined.

While at it remove CONFIG_HAS_GDBSX as it can easily be replaced by
CONFIG_CRASH_DEBUG.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- move domain_pause_for_debugger() into arch/x86/domain.c (Andrew Cooper)
---
 xen/arch/x86/Kconfig            |  1 -
 xen/arch/x86/domain.c           | 13 +++++++++++++
 xen/arch/x86/hvm/vmx/realmode.c |  1 +
 xen/common/Kconfig              |  3 ---
 xen/common/domain.c             | 14 --------------
 xen/include/asm-x86/debugger.h  | 32 ++++++++++++++++++++------------
 xen/include/xen/sched.h         |  1 -
 7 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 02bb05f42e..f853c04564 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -13,7 +13,6 @@ config X86
 	select HAS_EHCI
 	select HAS_EX_TABLE
 	select HAS_FAST_MULTIPLY
-	select HAS_GDBSX
 	select HAS_IOPORTS
 	select HAS_KEXEC
 	select MEM_ACCESS_ALWAYS_ON
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7cb7fd31dd..3a3fbde642 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2318,6 +2318,19 @@ static int __init 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:
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index bb0b4439df..bdbd9cb921 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -14,6 +14,7 @@
 #include <xen/sched.h>
 #include <xen/paging.h>
 #include <xen/softirq.h>
+#include <asm/debugger.h>
 #include <asm/event.h>
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/hvm.h>
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 2f516da101..b3d161d057 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -57,9 +57,6 @@ config HAS_UBSAN
 config HAS_KEXEC
 	bool
 
-config HAS_GDBSX
-	bool
-
 config HAS_IOPORTS
 	bool
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 611116c7fc..3a77d717db 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -915,20 +915,6 @@ void vcpu_end_shutdown_deferral(struct vcpu *v)
         vcpu_check_shutdown(v);
 }
 
-#ifdef CONFIG_HAS_GDBSX
-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
-
 /* Complete domain destroy after RCU readers are not holding old references. */
 static void complete_domain_destroy(struct rcu_head *head)
 {
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index b1b627f1fa..f58726daec 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/debugger.h
@@ -33,6 +33,8 @@
 #include <asm/regs.h>
 #include <asm/processor.h>
 
+void domain_pause_for_debugger(void);
+
 #ifdef CONFIG_CRASH_DEBUG
 
 #include <xen/gdbstub.h>
@@ -47,18 +49,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" );
 
-#else
-
-static inline bool debugger_trap_fatal(
-    unsigned int vector, struct cpu_user_regs *regs)
-{
-    return false;
-}
-
-#define debugger_trap_immediate() ((void)0)
-
-#endif
-
 static inline bool debugger_trap_entry(
     unsigned int vector, struct cpu_user_regs *regs)
 {
@@ -84,6 +74,24 @@ 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
+
 unsigned int dbg_rw_mem(void * __user addr, void * __user buf,
                         unsigned int len, domid_t domid, bool toaddr,
                         uint64_t pgd3);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 9f7bc69293..0b41e936d5 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -652,7 +652,6 @@ void domain_destroy(struct domain *d);
 int domain_kill(struct domain *d);
 int domain_shutdown(struct domain *d, u8 reason);
 void domain_resume(struct domain *d);
-void domain_pause_for_debugger(void);
 
 int domain_soft_reset(struct domain *d);
 
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 2/2] xen: make gdbsx support configurable

[Juergen Gross]

Gdbsx support in the hypervisor is rarely used and it is opening a
way for dom0 to modify the running hypervisor by very easy means.

Remove the possibility to read/write hypervisor memory, it was never
used by gdbsx.

Add a Kconfig option to control support of gdbsx. Default is on.

While at it correct a wrong comment in related code and remove dead
code.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- default Kconfig option to Y (Andrew Cooper)
- remove hypervisor memory access (Andrew Cooper)
---
 xen/Kconfig.debug              |  8 +++++
 xen/arch/x86/Makefile          |  2 +-
 xen/arch/x86/debug.c           | 78 +++++-------------------------------------
 xen/arch/x86/domctl.c          |  4 +++
 xen/include/asm-x86/debugger.h |  2 ++
 5 files changed, 24 insertions(+), 70 deletions(-)

diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index cf42e5e7a0..b3511e81a2 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -20,6 +20,14 @@ config CRASH_DEBUG
 	  If you want to attach gdb to Xen to debug Xen if it crashes
 	  then say Y.
 
+config GDBSX
+	bool "Guest debugging with gdbsx"
+	depends on X86
+	default y
+	---help---
+	  If you want to enable support for debugging guests from dom0 via
+	  gdbsx then say Y.
+
 config DEBUG_INFO
 	bool "Compile Xen with debug info"
 	default y
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 7da5a2631e..6783688b00 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -19,7 +19,7 @@ obj-bin-y += copy_page.o
 obj-y += cpuid.o
 obj-$(CONFIG_PV) += compat.o x86_64/compat.o
 obj-$(CONFIG_KEXEC) += crash.o
-obj-y += debug.o
+obj-$(CONFIG_GDBSX) += debug.o
 obj-y += delay.o
 obj-y += desc.o
 obj-bin-y += dmi_scan.init.o
diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index a500df01ac..5d8acdad71 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -22,22 +22,6 @@
 #include <asm/debugger.h>
 #include <asm/p2m.h>
 
-/* 
- * This file for general routines common to more than one debugger, like kdb,
- * gdbsx, etc..
- */
-
-#ifdef XEN_KDB_CONFIG
-#include "../kdb/include/kdbdefs.h"
-#include "../kdb/include/kdbproto.h"
-#define DBGP(...) {(kdbdbg) ? kdbp(__VA_ARGS__):0;}
-#define DBGP1(...) {(kdbdbg>1) ? kdbp(__VA_ARGS__):0;}
-#define DBGP2(...) {(kdbdbg>2) ? kdbp(__VA_ARGS__):0;}
-#else
-#define DBGP1(...) ((void)0)
-#define DBGP2(...) ((void)0)
-#endif
-
 typedef unsigned long dbgva_t;
 typedef unsigned char dbgbyte_t;
 
@@ -49,24 +33,13 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, gfn_t *gfn)
     uint32_t pfec = PFEC_page_present;
     p2m_type_t gfntype;
 
-    DBGP2("vaddr:%lx domid:%d\n", vaddr, dp->domain_id);
-
     *gfn = _gfn(paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec));
     if ( gfn_eq(*gfn, INVALID_GFN) )
-    {
-        DBGP2("kdb:bad gfn from gva_to_gfn\n");
         return INVALID_MFN;
-    }
 
     mfn = get_gfn(dp, gfn_x(*gfn), &gfntype);
     if ( p2m_is_readonly(gfntype) && toaddr )
-    {
-        DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
         mfn = INVALID_MFN;
-    }
-    else
-        DBGP2("X: vaddr:%lx domid:%d mfn:%#"PRI_mfn"\n",
-              vaddr, dp->domain_id, mfn_x(mfn));
 
     if ( mfn_eq(mfn, INVALID_MFN) )
     {
@@ -100,55 +73,36 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val)
     unsigned long cr3 = (pgd3val ? pgd3val : dp->vcpu[0]->arch.cr3);
     mfn_t mfn = maddr_to_mfn(cr3_pa(cr3));
 
-    DBGP2("vaddr:%lx domid:%d cr3:%lx pgd3:%lx\n", vaddr, dp->domain_id, 
-          cr3, pgd3val);
-
     if ( pgd3val == 0 )
     {
         l4t = map_domain_page(mfn);
         l4e = l4t[l4_table_offset(vaddr)];
         unmap_domain_page(l4t);
         mfn = l4e_get_mfn(l4e);
-        DBGP2("l4t:%p l4to:%lx l4e:%lx mfn:%#"PRI_mfn"\n", l4t,
-              l4_table_offset(vaddr), l4e, mfn_x(mfn));
         if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
-        {
-            DBGP1("l4 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
             return INVALID_MFN;
-        }
 
         l3t = map_domain_page(mfn);
         l3e = l3t[l3_table_offset(vaddr)];
         unmap_domain_page(l3t);
         mfn = l3e_get_mfn(l3e);
-        DBGP2("l3t:%p l3to:%lx l3e:%lx mfn:%#"PRI_mfn"\n", l3t,
-              l3_table_offset(vaddr), l3e, mfn_x(mfn));
         if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) ||
              (l3e_get_flags(l3e) & _PAGE_PSE) )
-        {
-            DBGP1("l3 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
             return INVALID_MFN;
-        }
     }
 
     l2t = map_domain_page(mfn);
     l2e = l2t[l2_table_offset(vaddr)];
     unmap_domain_page(l2t);
     mfn = l2e_get_mfn(l2e);
-    DBGP2("l2t:%p l2to:%lx l2e:%lx mfn:%#"PRI_mfn"\n",
-          l2t, l2_table_offset(vaddr), l2e, mfn_x(mfn));
     if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) ||
          (l2e_get_flags(l2e) & _PAGE_PSE) )
-    {
-        DBGP1("l2 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
         return INVALID_MFN;
-    }
+
     l1t = map_domain_page(mfn);
     l1e = l1t[l1_table_offset(vaddr)];
     unmap_domain_page(l1t);
     mfn = l1e_get_mfn(l1e);
-    DBGP2("l1t:%p l1to:%lx l1e:%lx mfn:%#"PRI_mfn"\n", l1t, l1_table_offset(vaddr),
-          l1e, mfn_x(mfn));
 
     return mfn_valid(mfn) ? mfn : INVALID_MFN;
 }
@@ -199,40 +153,26 @@ static unsigned int dbg_rw_guest_mem(struct domain *dp, void * __user gaddr,
     return len;
 }
 
-/* 
- * addr is hypervisor addr if domid == DOMID_IDLE, else it's guest addr
+/*
+ * 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. 
+ * Returns: number of bytes remaining to be copied.
  */
 unsigned int dbg_rw_mem(void * __user addr, void * __user buf,
                         unsigned int len, domid_t domid, bool toaddr,
                         uint64_t pgd3)
 {
-    DBGP2("gmem:addr:%lx buf:%p len:$%u domid:%d toaddr:%x\n",
-          addr, buf, len, domid, toaddr);
-
-    if ( domid == DOMID_IDLE )
-    {
-        if ( toaddr )
-            len = __copy_to_user(addr, buf, len);
-        else
-            len = __copy_from_user(buf, addr, len);
-    }
-    else
-    {
         struct domain *d = get_domain_by_id(domid);
 
-        if ( d )
-        {
-            if ( !d->is_dying )
-                len = dbg_rw_guest_mem(d, addr, buf, len, toaddr, pgd3);
-            put_domain(d);
-        }
+    if ( d )
+    {
+        if ( !d->is_dying )
+            len = dbg_rw_guest_mem(d, addr, buf, len, toaddr, pgd3);
+        put_domain(d);
     }
 
-    DBGP2("gmem:exit:len:$%d\n", len);
     return len;
 }
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b461aadbd6..43cd38b43f 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -36,6 +36,7 @@
 #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)
 {
     void * __user gva = (void *)iop->gva, * __user uva = (void *)iop->uva;
@@ -45,6 +46,7 @@ static int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
 
     return iop->remain ? -EFAULT : 0;
 }
+#endif
 
 static void domain_cpu_policy_changed(struct domain *d)
 {
@@ -932,6 +934,7 @@ long arch_do_domctl(
     }
 #endif
 
+#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(domctl->domain, &domctl->u.gdbsx_guest_memio);
@@ -996,6 +999,7 @@ long arch_do_domctl(
         copyback = true;
         break;
     }
+#endif
 
     case XEN_DOMCTL_setvcpuextstate:
     case XEN_DOMCTL_getvcpuextstate:
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index f58726daec..a9ddb01433 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/debugger.h
@@ -92,8 +92,10 @@ static inline bool debugger_trap_entry(
 
 #endif
 
+#ifdef CONFIG_GDBSX
 unsigned int dbg_rw_mem(void * __user addr, void * __user buf,
                         unsigned int len, domid_t domid, bool toaddr,
                         uint64_t pgd3);
+#endif
 
 #endif /* __X86_DEBUGGER_H__ */
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 0/2] xen: make more debugger support code conditional

[Andrew Cooper]

On 19/12/2019 07:42, Juergen Gross wrote:
> Support for debugging the hypervisor of guests via gdb/gdbsx should be
> configurable.
>
> Changes in V3:
> - remove possibility to access hypervisor memory via gdbsx domctl
> - default gdbsx support to on
> - some code moving
>
> Changes in V2:
> - split support for gdbstub and gdbsx (Andrew Cooper)
>
> Juergen Gross (2):
>   xen: put more code under CONFIG_CRASH_DEBUG
>   xen: make gdbsx support configurable

LGTM.  Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] xen: put more code under CONFIG_CRASH_DEBUG

[Andrew Cooper]

On 19/12/2019 07:42, Juergen Gross wrote:
> Some code is not needed with CONFIG_CRASH_DEBUG, so only include it if
> CONFIG_CRASH_DEBUG is defined.

s/with/without/ ? and I presume you are referring to debugger_trap_entry() ?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] xen: put more code under CONFIG_CRASH_DEBUG

[Jürgen Groß]

On 20.12.19 13:52, Andrew Cooper wrote:
> On 19/12/2019 07:42, Juergen Gross wrote:
>> Some code is not needed with CONFIG_CRASH_DEBUG, so only include it if
>> CONFIG_CRASH_DEBUG is defined.
> 
> s/with/without/ ? and I presume you are referring to debugger_trap_entry() ?

Yes and yes.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] xen: put more code under CONFIG_CRASH_DEBUG

[George Dunlap]

On 12/19/19 7:42 AM, Juergen Gross wrote:
> Some code is not needed with CONFIG_CRASH_DEBUG, so only include it if
> CONFIG_CRASH_DEBUG is defined.
> 
> While at it remove CONFIG_HAS_GDBSX as it can easily be replaced by
> CONFIG_CRASH_DEBUG.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

In case you need it for sched.h:

Acked-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel