| Message ID | 1476259105-16448-2-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 12.10.16 at 09:58, <tianyu.lan@intel.com> wrote: > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -347,7 +347,7 @@ static void switch_serial_input(void) > static void __serial_rx(char c, struct cpu_user_regs *regs) > { > if ( xen_rx ) > - return handle_keypress(c, regs); > + return handle_keypress(c, regs, true); I think it would be nice to pass true here only when in polling mode, unless you know or can deduce that the a similar problem also exists in IRQ mode. Perhaps you could simply move the !in_irq() here? (Of course the new function parameter would then want to be renamed.) Jan
On 10/12/2016 9:19 PM, Jan Beulich wrote: >>>> On 12.10.16 at 09:58, <tianyu.lan@intel.com> wrote: >> --- a/xen/drivers/char/console.c >> +++ b/xen/drivers/char/console.c >> @@ -347,7 +347,7 @@ static void switch_serial_input(void) >> static void __serial_rx(char c, struct cpu_user_regs *regs) >> { >> if ( xen_rx ) >> - return handle_keypress(c, regs); >> + return handle_keypress(c, regs, true); > > I think it would be nice to pass true here only when in polling mode, > unless you know or can deduce that the a similar problem also exists > in IRQ mode. Perhaps you could simply move the !in_irq() here? That's a good idea. Thanks. >(Of course the new function parameter would then want to be renamed.) Since the issue happens when handle_keypress() runs in a timer handler, how about to name new parameter "intimer"? __serial_rx() is called in a timer handler or interrupt handler. Or do you have other suggestion? > > Jan >
>>> On 12.10.16 at 16:30, <tianyu.lan@intel.com> wrote: > > On 10/12/2016 9:19 PM, Jan Beulich wrote: >>>>> On 12.10.16 at 09:58, <tianyu.lan@intel.com> wrote: >>> --- a/xen/drivers/char/console.c >>> +++ b/xen/drivers/char/console.c >>> @@ -347,7 +347,7 @@ static void switch_serial_input(void) >>> static void __serial_rx(char c, struct cpu_user_regs *regs) >>> { >>> if ( xen_rx ) >>> - return handle_keypress(c, regs); >>> + return handle_keypress(c, regs, true); >> >> I think it would be nice to pass true here only when in polling mode, >> unless you know or can deduce that the a similar problem also exists >> in IRQ mode. Perhaps you could simply move the !in_irq() here? > > That's a good idea. Thanks. > >>(Of course the new function parameter would then want to be renamed.) > > Since the issue happens when handle_keypress() runs in a timer handler, > how about to name new parameter "intimer"? __serial_rx() is called in a > timer handler or interrupt handler. Or do you have other suggestion? I think "intimer" can be confusing (to be mixed up with timer interrupt). How about "force_tasklet"? Jan
On 2016年10月13日 00:03, Jan Beulich wrote: >>>> On 12.10.16 at 16:30, <tianyu.lan@intel.com> wrote: >> >> Since the issue happens when handle_keypress() runs in a timer handler, >> how about to name new parameter "intimer"? __serial_rx() is called in a >> timer handler or interrupt handler. Or do you have other suggestion? > > I think "intimer" can be confusing (to be mixed up with timer interrupt). > How about "force_tasklet"? OK. I will update.
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c index 16de6e8..3d50041 100644 --- a/xen/common/keyhandler.c +++ b/xen/common/keyhandler.c @@ -75,19 +75,21 @@ static struct keyhandler { static void keypress_action(unsigned long unused) { - handle_keypress(keypress_key, NULL); + console_start_log_everything(); + key_table[keypress_key].fn(keypress_key); + console_end_log_everything(); } static DECLARE_TASKLET(keypress_tasklet, keypress_action, 0); -void handle_keypress(unsigned char key, struct cpu_user_regs *regs) +void handle_keypress(unsigned char key, struct cpu_user_regs *regs, bool async) { struct keyhandler *h; if ( key >= ARRAY_SIZE(key_table) || !(h = &key_table[key])->fn ) return; - if ( !in_irq() || h->irq_callback ) + if ( h->irq_callback || !async ) { console_start_log_everything(); h->irq_callback ? h->irq_fn(key, regs) : h->fn(key); diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c index 8aea6ef..1eb7bad 100644 --- a/xen/common/sysctl.c +++ b/xen/common/sysctl.c @@ -136,7 +136,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) { 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, guest_cpu_user_regs(), false); } ret = 0; copyback = 0; diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 55ae31a..184b523 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -347,7 +347,7 @@ static void switch_serial_input(void) static void __serial_rx(char c, struct cpu_user_regs *regs) { if ( xen_rx ) - return handle_keypress(c, regs); + return handle_keypress(c, regs, true); /* Deliver input to guest buffer, unless it is already full. */ if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE ) diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h index 06c05c8..e9595bd 100644 --- a/xen/include/xen/keyhandler.h +++ b/xen/include/xen/keyhandler.h @@ -46,7 +46,9 @@ void register_irq_keyhandler(unsigned char key, bool_t 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, + struct cpu_user_regs *regs, + bool async); /* Scratch space is available for use of any keyhandler. */ extern char keyhandler_scratch[1024];
Keyhandler may run for a long time in serial port driver's timer handler on the large machine with a lot of physical cpus(e,g dump_timerq()) when serial port driver works in the poll mode(via the exception mechanism). If a timer handler runs a long time, it will block nmi_timer_fn() to feed NMI watchdog and cause Xen hypervisor panic. Inserting process_pending_softirqs() in timer handler will not help. when timer interrupt arrives, timer subsystem calls all expired timer handlers before programming next timer interrupt. There is no timer interrupt arriving to trigger timer softirq during run a timer handler. This patch is to fix the issue to make nonirq keyhandler run in tasklet when receive debug key from serial port. Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- xen/common/keyhandler.c | 8 +++++--- xen/common/sysctl.c | 2 +- xen/drivers/char/console.c | 2 +- xen/include/xen/keyhandler.h | 4 +++- 4 files changed, 10 insertions(+), 6 deletions(-)