Re: [PATCH] consolidate do_bug_frame() / bug_fn_t

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/01/2024 7:28 am, Jan Beulich wrote:
> On 24.01.2024 02:34, Stefano Stabellini wrote:
>> I managed to get back to read the mailing list and noticed this patch.
>>
>> Is it still relevant and needs to be reviewed?
>>
>> Are there any outstanding disagreements between maintainers on the
>> approach to take here?  Or should I just go ahead and review it?
> It is still relevant from my pov, and everything that may be controversial
> is said ...

BUGFRAME_* cannot legitimately modify the interrupted context.  Two are
fatal paths, and other two are side-effect-less as far as C can tell.

So the infrastructure ought to take a const pointer.

The reason why this pointer is non-const is to do with the interaction
of the serial and keyhandler infrastructures.  Because we're adjusting
that for other reasons, I was hoping it would subsequently be easy to
switch Xen to being properly const in this regard.

Turns out it is:

 
https://gitlab.com/xen-project/people/andyhhp/xen/-/commit/4f857075005da1d28632e4f9198c2e7d0f404b9a

with a couple of caveats.  (Only the buster-gcc-ibt run failed, so I've
got some cf_check-ing to adjust, but all the other builds worked fine).


To make the serial code compile, I ended up having to revert patch 2 of
the regs series, which I believe is safe to do following patch 3-5 which
un-plumb the regs pointer deeper in the call chain.  If this is turns
out to be true, then the patch ought to be added and reverted in the
same series so it isn't left hanging about after the fact.

The _$X_poll() functions are used in timer context, which means there's
an outer regs context already latched, and that's arguably a better
context to use anyway for 'd'.

This in turn allows us to remove a #UD from a fast(ish) path, and remove
some per-cpu or static variables which are just used for non-standard
parameter passing because run_in_exception_handler() doesn't let you
pass any.


This leaves the '%' debugger infrastructure.  Being a debugger, it's
making arbitrary changes anyway and I'd much rather cast away constness
for a debugger, than to keep everything else mutable when it oughtn't to be.

If absolutely nothing else, registration and handling '%' ought to be
from x86 code rather than common code, which would remove the
do_debugger_trap_fatal() layering violation.

But, the more I look into the gdbstub the more I'm convinced that it
doesn't work.  For example, this gem:

/* Resuming after we've stopped used to work, but more through luck than
any actual intention.  It doesn't at the moment. */

>From c/s b69f92f3012 in July 2004, and more specifically the commit
which added the gdbstub functionality to begin with.  I.e. it doesn't
appear to have ever supported more than "poke around in the crashed
state".  In the 2 decades that noone has fixed this, we've gained far
better technologies for doing this, such as running it in a VM.

I am going to submit some patches deleting gdbstub.  It clearly had not
much value to begin with, and is not definitely not worth the problems
it is creating in adjacent code these days.

~Andrew