Re: [PATCH v3 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions
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 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions
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 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions
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 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions
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
[PATCH v3 2/6] x86/debugger: separate Xen and guest debugging debugger_trap_* functions
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 .
Signed-off-by: Bobby Eshleman
---
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) && e
