| Message ID | 5258d8a9-a7ef-4342-9b5c-fc0078139bb2@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | limit passing around of cpu_user_regs | expand |
Hi Jan, On 05/02/2024 13:28, Jan Beulich wrote: > In preparation for further removal of regs parameters, drop it here. In > the two places where it's actually needed, retrieve IRQ context if > available, or else guest context. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > As an alternative to the new boolean parameter, I wonder if we couldn't > special-case the idle vCPU case: It's only there where we would not have > proper context retrievable via guest_cpu_user_regs(). I am trying to understand the implication. Looking at the code, it seems in the case where we pass NULL, we would expect to call run_in_exception_handler(). If I am not mistaken, at least for Arm, regs would not be the same as guest_cpu_user_regs(). So I think your current approach is more correct. Did I miss anything? Cheers,
On 08.02.2024 23:09, Julien Grall wrote: > On 05/02/2024 13:28, Jan Beulich wrote: >> In preparation for further removal of regs parameters, drop it here. In >> the two places where it's actually needed, retrieve IRQ context if >> available, or else guest context. >> >> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> As an alternative to the new boolean parameter, I wonder if we couldn't >> special-case the idle vCPU case: It's only there where we would not have >> proper context retrievable via guest_cpu_user_regs(). > > I am trying to understand the implication. Looking at the code, it seems > in the case where we pass NULL, we would expect to call > run_in_exception_handler(). Right, when NULL was passed so far, and when true is passed now, that's to indicate to invoke run_in_exception_handler(). > If I am not mistaken, at least for Arm, regs would not be the same as > guest_cpu_user_regs(). So I think your current approach is more correct. > > Did I miss anything? Whether regs are the same isn't overly relevant here. The thing that's relevant is whether what would be logged actually makes sense. And invoking guest_cpu_user_regs() in idle vCPU context makes no sense. Whereas in other contexts its result is good enough to show the present state of the CPU; there's no real need in such a case to go through run_in_exception_handler(). The present approach therefore isn't necessarily "more correct", but it is closer to prior behavior. The corner case that makes me prefer the presently chosen approach is when a CPU is in the middle of scheduling, especially considering x86's lazy switching (when current != this_cpu(curr_vcpu). The main reason to mention the alternative is because iirc Andrew had suggested to move more in that direction. Jan
Hi Jan, On 12/02/2024 09:13, Jan Beulich wrote: > On 08.02.2024 23:09, Julien Grall wrote: >> On 05/02/2024 13:28, Jan Beulich wrote: >>> In preparation for further removal of regs parameters, drop it here. In >>> the two places where it's actually needed, retrieve IRQ context if >>> available, or else guest context. >>> >>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> As an alternative to the new boolean parameter, I wonder if we couldn't >>> special-case the idle vCPU case: It's only there where we would not have >>> proper context retrievable via guest_cpu_user_regs(). >> >> I am trying to understand the implication. Looking at the code, it seems >> in the case where we pass NULL, we would expect to call >> run_in_exception_handler(). > > Right, when NULL was passed so far, and when true is passed now, that's > to indicate to invoke run_in_exception_handler(). > >> If I am not mistaken, at least for Arm, regs would not be the same as >> guest_cpu_user_regs(). So I think your current approach is more correct. >> >> Did I miss anything? > > Whether regs are the same isn't overly relevant here. The thing that's > relevant is whether what would be logged actually makes sense. And > invoking guest_cpu_user_regs() in idle vCPU context makes no sense. > Whereas in other contexts its result is good enough to show the present > state of the CPU; there's no real need in such a case to go through > run_in_exception_handler(). > > The present approach therefore isn't necessarily "more correct", but it > is closer to prior behavior. > > The corner case that makes me prefer the presently chosen approach is > when a CPU is in the middle of scheduling, especially considering x86's > lazy switching (when current != this_cpu(curr_vcpu). The main reason to > mention the alternative is because iirc Andrew had suggested to move > more in that direction. Thanks for the clarification. I don't have any strong preference either way. So I would be happy to go with your solution. Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
--- a/xen/common/keyhandler.c +++ b/xen/common/keyhandler.c @@ -73,12 +73,12 @@ static struct keyhandler { static void cf_check keypress_action(void *unused) { - handle_keypress(keypress_key, NULL); + handle_keypress(keypress_key, true); } static DECLARE_TASKLET(keypress_tasklet, keypress_action, NULL); -void handle_keypress(unsigned char key, struct cpu_user_regs *regs) +void handle_keypress(unsigned char key, bool need_context) { struct keyhandler *h; @@ -88,7 +88,7 @@ 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); + h->irq_callback ? h->irq_fn(key, need_context) : h->fn(key); console_end_log_everything(); } else @@ -169,7 +169,7 @@ void cf_check dump_execstate(struct cpu_ } static void cf_check dump_registers( - unsigned char key, struct cpu_user_regs *regs) + unsigned char key, bool need_context) { unsigned int cpu; @@ -182,8 +182,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 ( !need_context ) + dump_execstate(get_irq_regs() ?: guest_cpu_user_regs()); else run_in_exception_handler(dump_execstate); @@ -245,8 +245,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, bool unused) { printk("'%c' pressed -> rebooting machine\n", key); machine_restart(0); @@ -474,8 +473,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, bool need_context) { struct keyhandler *h; unsigned int k; @@ -491,7 +489,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, need_context); } watchdog_enable(); @@ -500,8 +498,7 @@ static void cf_check run_all_keyhandlers tasklet_schedule(&run_all_keyhandlers_tasklet); } -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, bool unused) { alt_key_handling = !alt_key_handling; printk("'%c' pressed -> using %s key handling\n", key, @@ -566,7 +563,7 @@ void keyhandler_crash_action(enum crash_ if ( *action == '+' ) mdelay(10); else - handle_keypress(*action, NULL); + handle_keypress(*action, true); action++; } } --- a/xen/common/sysctl.c +++ b/xen/common/sysctl.c @@ -134,7 +134,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe { if ( copy_from_guest_offset(&c, op->u.debug_keys.keys, i, 1) ) goto out; - handle_keypress(c, guest_cpu_user_regs()); + handle_keypress(c, false); } ret = 0; copyback = 0; --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -279,7 +279,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, bool unused) { if ( upper_thresh_adj == &xenlog_upper_thresh ) { @@ -306,13 +306,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, bool unused) { ++*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, bool unused) { if ( *lower_thresh_adj ) --*lower_thresh_adj; @@ -531,7 +531,7 @@ static void __serial_rx(char c, struct c switch ( console_rx ) { case 0: - return handle_keypress(c, regs); + return handle_keypress(c, false); case 1: /* --- a/xen/include/xen/keyhandler.h +++ b/xen/include/xen/keyhandler.h @@ -24,9 +24,8 @@ 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); + bool need_context); /* Initialize keytable with default handlers. */ void initialize_keytable(void); @@ -46,7 +45,7 @@ void register_irq_keyhandler(unsigned ch bool diagnostic); /* Inject a keypress into the key-handling subsystem. */ -extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs); +extern void handle_keypress(unsigned char key, bool need_context); enum crash_reason { CRASHREASON_PANIC,
In preparation for further removal of regs parameters, drop it here. In the two places where it's actually needed, retrieve IRQ context if available, or else guest context. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- As an alternative to the new boolean parameter, I wonder if we couldn't special-case the idle vCPU case: It's only there where we would not have proper context retrievable via guest_cpu_user_regs(). --- v3: Re-base. v2: New.