Re: [PATCH 1/8] keyhandler: don't pass cpu_user_regs around

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/01/2024 12:11 pm, Jan Beulich wrote:
>>> Have
>>> handle_keypress() make the pointer available via a per-CPU variable,
>>> thus eliminating the need to pass it to all IRQ key handlers, making
>>> sure that a console-invoked key's handling can still nest inside a
>>> sysctl-invoked one's.
>> I know this is the current behaviour, and I'm not suggesting altering it
>> in this patch, but the sysctl was added so you had a way of using debug
>> keys without necessarily having a working serial connection.
>>
>> It was never expected or intended for both mechanisms to work
>> concurrently, and I don't think we need to take any care to make/keep it
>> working.
> Well, all it takes is the saving and restoring of keypress_regs in
> handle_keypress(). You you really think it would be better to risk
> a cash, but not doing that tiny bit of extra work?

I presume you mean crash?

I'm not advocating for leaving something explicitly unsafe, but I'm also
looking to see if we can avoid having keypress_regs to begin with.  i.e.
I think we've already got unnecessary complexity, and it would be good
to reduce it.
>> Trap to debugger is an x86-ism for gdbstub only.  I'm tempted to drop
>> all the gdbstub code.  I've never encountered it working in 13y, and the
>> number of build fixes I've done personally, I don't believe the code can
>> plausibly be in a non-bitrotten state.
>>
>> Nevertheless, an explicit trap-to-debugger which isn't a manually
>> inserted debugger_trap_{immediate,fatal}() is a weird construct in the
>> first place, not least because an attached debugger can do this on its
>> own anyway.
> Not sure here, and I'd hope purging of that (if so wanted) can be dealt
> with separately.

It can, yes.

It was just if there was an easy way to avoid introducing keypress_regs
then it would have been better to remove this first, than to refactor
and then delete.

> I have a vague recollection of such "on its own" wasn't
> very reliable, when trying to use such elsewhere (in the distant past,
> before I started working on Xen). That's not to say that I have proof
> that our debug-key is any more reliable.

This really comes back to whether gdbstub works or not.

Frankly, if you really do need to debug Xen these days, you'd be better
off running it under Qemu and using the qemu gdbstub.  If nothing else
it's more likely to work, and it does not need an alive-enough Xen to
have a working irq/serial subsystem.

>
>> The more I think about this, the more I think we should just remove 'D',
>> even if we don't go for dropping gdbstub.  It's the only place where
>> gdbstub really escapes out of x86 into common code.  (I see there's a
>> new one in bug.h but that is abstracted with a macro.)
>>
>> Also, `xl debug-keys D` was clearly something that just got swept up
>> with "make all debug keys usable via sysctl", not because it was a
>> plausibly useful construct.
> Did you really mean D (EPT table dump), not %? If the latter, then yes,
> that may better be filtered out of what can be done via sysctl.

I did mean %, yes.

>
>> This just leaves dump regs, which I think can safely use get_irq_regs()
>> || guest_cpu_user_regs().  All it wants is something to dump_execstate()
>> to, which just wants to be the start of the path which led here.
> I don't think so - consider the case of 'd' hitting while handling an
> interrupt (and, say, stuck there in an infinite loop with IRQs enabled).
> We'd then wrongly dump the context of what the earlier IRQ interrupted.

The serial IRQ producing the 'd' keypress will push a irq frame, which
is what will be returned by get_irq_regs().

It does occur to me that we're trying to accommodate for two behaviours
here.

For a real keypress, we want to dump from the the point the interrupt
hit because that's the interesting bit of stack to see.  For a SYSCTL,
there's nothing, and we're using BUGFRAME_run_fn to generate one.

So actually we just simply want "regs = get_irq_regs();" here and retain
prior NULL check, don't we?

~Andrew