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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> with a few trivial
tweaks, but see below.

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

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.

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.


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.

With both of those sorted (albeit it with patch 2 needing to move ahead
of 1), I don't think we need to keep keypress_regs.

> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -80,6 +80,7 @@ static void cf_check keypress_action(voi
>  }
>  
>  static DECLARE_TASKLET(keypress_tasklet, keypress_action, NULL);
> +static DEFINE_PER_CPU(struct cpu_user_regs *, keypress_regs);
>  
>  void handle_keypress(unsigned char key, struct cpu_user_regs *regs)
>  {
> @@ -91,7 +92,16 @@ void handle_keypress(unsigned char key,
>      if ( !in_irq() || h->irq_callback )
>      {
>          console_start_log_everything();
> -        h->irq_callback ? h->irq_fn(key, regs) : h->fn(key);

This would read better with a blank line here, and ...

> +        if ( h->irq_callback )
> +        {
> +            struct cpu_user_regs *old = this_cpu(keypress_regs);
> +
> +            this_cpu(keypress_regs) = regs;
> +            h->irq_fn(key);
> +            this_cpu(keypress_regs) = old;
> +        }
> +        else
> +            h->fn(key);

... here.

~Andrew