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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>
>>>
>>> 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@xxxxxxxx>

Jan