Re: [PATCH] consolidate do_bug_frame() / bug_fn_t

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.01.2024 02:14, Andrew Cooper wrote:
> 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.

Hmm, I'm not sure I see how reverting that would end up working. However,
aiui you need to revert primarily for the non-const-ness of the pointers
involved in [gs]et_irq_regs(). I wonder whether, if we followed your
underlying thought here, those shouldn't be const-ified then anyway.

> 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'.

If the timer happens to run on an idle vCPU, what "outer regs context"
would we have there?

> 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.

All fine. Still I wonder whether in the meantime this patch isn't an
improvement on its own, and hence whether the const couldn't sensibly
be added subsequently.

Jan