| Message ID | 7925a89cf830e0e3705a8700fce09a408fcfc27c.1629315873.git.bobby.eshleman@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove unconditional arch dependency on asm/debugger.h | expand |
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
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
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
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
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
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(-)