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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.01.2024 12:49, Andrew Cooper wrote:
> On 11/01/2024 7:31 am, Jan Beulich wrote:
>> There are exactly two handlers which care about the registers.
> 
> Which two?  dump regs and trap to debugger?
> 
> [Edit, oh yes, this is clear in the patch, but IMO it would be helpful
> to state them here.]

Sure, added mention of 'd' and '%'.

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

>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> with a few trivial
> tweaks, but see below.

Thanks, and yes, I've inserted the two blank lines.

>> ---
>> Subsequently we may want to eliminate the fn/irq_fn union as well,
>> along with dropping the now redundant irq_keyhandler_fn_t.
> 
> Yes - this simplification was one I was hoping to be able to make in due
> course.  I suspect the split is only because some functionality wanted
> regs and others not.

That's my recollection, yes.

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

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

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

Jan