| Message ID | dd1c24ec-4054-43e1-b0c9-6c2044b84046@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | limit passing around of cpu_user_regs | expand |
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@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> 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
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@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> 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
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
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
On 11/01/2024 3:49 pm, Jan Beulich wrote: > On 11.01.2024 16:24, Andrew Cooper wrote: >> On 11/01/2024 12:11 pm, Jan Beulich wrote: >> 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. Ok. As you've done this analysis work, can you list these 3 forms? I've clearly missed one in my analysis. > > 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. When complexity is involved, I always prefer to make changes in smaller chunks. ~Andrew
On 11.01.2024 17:08, Andrew Cooper wrote: > On 11/01/2024 3:49 pm, Jan Beulich wrote: >> On 11.01.2024 16:24, Andrew Cooper wrote: >>> On 11/01/2024 12:11 pm, Jan Beulich wrote: >>> 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. > > Ok. As you've done this analysis work, can you list these 3 forms? > > I've clearly missed one in my analysis. The console handler invocation is (now) using get_irq_regs(). The sysctl handling, as said, passes guest_cpu_user_regs(). And then there are two cases where NULL is passed, one from keypress_action() (a tasklet handler) and the other from keyhandler_crash_action(). I'm not convinced the latter is fully correct. The tasklet handler is used only for non-IRQ keys, and hence only in cases where regs wouldn't have been passed to the handler anyway. IOW - leaving the keyhandler_crash_action() uncertainty aside, I think it ought to be possible to do as you did suggest. Jan
Hi Jan, On 11/01/2024 07:31, Jan Beulich wrote: > There are exactly two handlers which care about the registers. 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. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
On 18.01.2024 15:06, Julien Grall wrote: > On 11/01/2024 07:31, Jan Beulich wrote: >> There are exactly two handlers which care about the registers. 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. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Julien Grall <jgrall@amazon.com> Thanks. I had to drop this patch though in v2, doing things differently to address a comment/request from Andrew. Jan
--- 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); + 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); console_end_log_everything(); } else @@ -171,8 +181,7 @@ void cf_check dump_execstate(struct cpu_ watchdog_enable(); } -static void cf_check dump_registers( - unsigned char key, struct cpu_user_regs *regs) +static void cf_check dump_registers(unsigned char key) { unsigned int cpu; @@ -185,8 +194,8 @@ static void cf_check dump_registers( cpumask_copy(&dump_execstate_mask, &cpu_online_map); /* Get local execution state out immediately, in case we get stuck. */ - if ( regs ) - dump_execstate(regs); + if ( this_cpu(keypress_regs) ) + dump_execstate(this_cpu(keypress_regs)); else run_in_exception_handler(dump_execstate); @@ -248,8 +257,7 @@ static void cf_check dump_hwdom_register } } -static void cf_check reboot_machine( - unsigned char key, struct cpu_user_regs *regs) +static void cf_check reboot_machine(unsigned char key) { printk("'%c' pressed -> rebooting machine\n", key); machine_restart(0); @@ -477,8 +485,7 @@ static void cf_check run_all_nonirq_keyh static DECLARE_TASKLET(run_all_keyhandlers_tasklet, run_all_nonirq_keyhandlers, NULL); -static void cf_check run_all_keyhandlers( - unsigned char key, struct cpu_user_regs *regs) +static void cf_check run_all_keyhandlers(unsigned char key) { struct keyhandler *h; unsigned int k; @@ -494,7 +501,7 @@ static void cf_check run_all_keyhandlers if ( !h->irq_fn || !h->diagnostic || !h->irq_callback ) continue; printk("[%c: %s]\n", k, h->desc); - h->irq_fn(k, regs); + h->irq_fn(k); } watchdog_enable(); @@ -511,17 +518,16 @@ static void cf_check do_debugger_trap_fa barrier(); } -static void cf_check do_debug_key(unsigned char key, struct cpu_user_regs *regs) +static void cf_check do_debug_key(unsigned char key) { printk("'%c' pressed -> trapping into debugger\n", key); - if ( regs ) - do_debugger_trap_fatal(regs); + if ( this_cpu(keypress_regs) ) + do_debugger_trap_fatal(this_cpu(keypress_regs)); else run_in_exception_handler(do_debugger_trap_fatal); } -static void cf_check do_toggle_alt_key( - unsigned char key, struct cpu_user_regs *regs) +static void cf_check do_toggle_alt_key(unsigned char key) { alt_key_handling = !alt_key_handling; printk("'%c' pressed -> using %s key handling\n", key, --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -280,7 +280,7 @@ static int *__read_mostly upper_thresh_a static int *__read_mostly lower_thresh_adj = &xenlog_lower_thresh; static const char *__read_mostly thresh_adj = "standard"; -static void cf_check do_toggle_guest(unsigned char key, struct cpu_user_regs *regs) +static void cf_check do_toggle_guest(unsigned char key) { if ( upper_thresh_adj == &xenlog_upper_thresh ) { @@ -307,13 +307,13 @@ static void do_adj_thresh(unsigned char loglvl_str(*upper_thresh_adj)); } -static void cf_check do_inc_thresh(unsigned char key, struct cpu_user_regs *regs) +static void cf_check do_inc_thresh(unsigned char key) { ++*lower_thresh_adj; do_adj_thresh(key); } -static void cf_check do_dec_thresh(unsigned char key, struct cpu_user_regs *regs) +static void cf_check do_dec_thresh(unsigned char key) { if ( *lower_thresh_adj ) --*lower_thresh_adj; --- a/xen/include/xen/keyhandler.h +++ b/xen/include/xen/keyhandler.h @@ -24,9 +24,7 @@ typedef void (keyhandler_fn_t)(unsigned * * Called in hardirq context with interrupts disabled. */ -struct cpu_user_regs; -typedef void (irq_keyhandler_fn_t)(unsigned char key, - struct cpu_user_regs *regs); +typedef void irq_keyhandler_fn_t(unsigned char key); /* Initialize keytable with default handlers. */ void initialize_keytable(void); @@ -46,6 +44,7 @@ void register_irq_keyhandler(unsigned ch bool diagnostic); /* Inject a keypress into the key-handling subsystem. */ +struct cpu_user_regs; extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs); enum crash_reason {
There are exactly two handlers which care about the registers. 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. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Subsequently we may want to eliminate the fn/irq_fn union as well, along with dropping the now redundant irq_keyhandler_fn_t.