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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.01.2024 16:24, Andrew Cooper wrote:
> 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?

Oops, yes, I do.

> 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.
>[...]
>>> 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().

Hmm, yes. I wonder what I was thinking ...

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

There's three forms of handle_keypress() invocations really, and hence
why (after having dropped the regs parameter already) I re-instated it.

As an aside - no, sysctl handling does not generate an exception frame.
Is uses guest_cpu_user_regs() (and imo validly so).

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

As per above, after I had it that way first, I backed off to accommodate
all present use forms of handle_keypress(). But dealing with that (in
whichever way we may end up deeming workable) can be separate anyway,
afaict.

Jan