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.
--- 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,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, -- Julien Grall
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, -- Julien Grall
© 2016 - 2024 Red Hat, Inc.