| Message ID | 1594966857-5215-2-git-send-email-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce NMI aware serial drivers | expand |
On Fri, Jul 17, 2020 at 11:50:54AM +0530, Sumit Garg wrote: > With the advent of pseudo NMIs on arm64 it should be possible to request > serial device interrupt as an NMI rather than IRQ. Having NMI driven > serial RX will make it possible to do a magic sysrq in NMI context > rather than in normal IRQ context. So as a preparatory step, make sysrq > handler NMI aware and allow system requests to be marked as NMI safe > which can run directly in NMI context while others being categorized > as not NMI safe will be queued as irq_work for later processing in > normal interrupt context. The opening line seems a little vague and, to be honest whether we use pseudo-NMI, FIQ (on Armv7) or some other architecture trick doesn't really matter at this point. Maybe just: In a future patch we will add support to the serial core to make it possible to trigger a magic sysrq from an NMI context. Prepare for this by marking some sysrq actions as NMI safe. Safe actions will be allowed to run from NMI context whilst that that cannot run from an NMI will be queued as irq_work for later processing. > > A particular sysrq handler is only marked as NMI safe in case the handler > isn't contending for any synchronization primimitives as in NMI context > they are expected to cause deadlocks. > > Note: Here kernel debugger (kgdb) is an exception which is a kind of stop > the world debugger and designed to run in NMI context. A "Note:" is a bit heavy handed here and also somewhat unclear. In particular whether a debugger stops the world doesn't really affect whether or not it was designed to be NMI safe. Maybe merge into the preceding paragraph: Note that the debug sysrq do not contend for any synchronization primitives. It does call kgdb_breakpoint() to provoke a trap but that trap handler should be NMI safe on architectures that implement an NMI. > This feature is especially helpful in case the primary CPU is stuck in > deadlock with interrupts disabled and doesn't honour serial device > interrupt. So having sysrq running in NMI context is helpful to debug > such scenarios. No it doesn't. It simply marks some sysrqs as NMI safe... Daniel. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > drivers/tty/sysrq.c | 33 ++++++++++++++++++++++++++++++++- > include/linux/sysrq.h | 1 + > kernel/debug/debug_core.c | 1 + > 3 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c > index 7c95afa9..8017e33 100644 > --- a/drivers/tty/sysrq.c > +++ b/drivers/tty/sysrq.c > @@ -50,6 +50,8 @@ > #include <linux/syscalls.h> > #include <linux/of.h> > #include <linux/rcupdate.h> > +#include <linux/irq_work.h> > +#include <linux/kfifo.h> > > #include <asm/ptrace.h> > #include <asm/irq_regs.h> > @@ -111,6 +113,7 @@ static const struct sysrq_key_op sysrq_loglevel_op = { > .help_msg = "loglevel(0-9)", > .action_msg = "Changing Loglevel", > .enable_mask = SYSRQ_ENABLE_LOG, > + .nmi_safe = true, > }; > > #ifdef CONFIG_VT > @@ -157,6 +160,7 @@ static const struct sysrq_key_op sysrq_crash_op = { > .help_msg = "crash(c)", > .action_msg = "Trigger a crash", > .enable_mask = SYSRQ_ENABLE_DUMP, > + .nmi_safe = true, > }; > > static void sysrq_handle_reboot(int key) > @@ -170,6 +174,7 @@ static const struct sysrq_key_op sysrq_reboot_op = { > .help_msg = "reboot(b)", > .action_msg = "Resetting", > .enable_mask = SYSRQ_ENABLE_BOOT, > + .nmi_safe = true, > }; > > const struct sysrq_key_op *__sysrq_reboot_op = &sysrq_reboot_op; > @@ -217,6 +222,7 @@ static const struct sysrq_key_op sysrq_showlocks_op = { > .handler = sysrq_handle_showlocks, > .help_msg = "show-all-locks(d)", > .action_msg = "Show Locks Held", > + .nmi_safe = true, > }; > #else > #define sysrq_showlocks_op (*(const struct sysrq_key_op *)NULL) > @@ -289,6 +295,7 @@ static const struct sysrq_key_op sysrq_showregs_op = { > .help_msg = "show-registers(p)", > .action_msg = "Show Regs", > .enable_mask = SYSRQ_ENABLE_DUMP, > + .nmi_safe = true, > }; > > static void sysrq_handle_showstate(int key) > @@ -326,6 +333,7 @@ static const struct sysrq_key_op sysrq_ftrace_dump_op = { > .help_msg = "dump-ftrace-buffer(z)", > .action_msg = "Dump ftrace buffer", > .enable_mask = SYSRQ_ENABLE_DUMP, > + .nmi_safe = true, > }; > #else > #define sysrq_ftrace_dump_op (*(const struct sysrq_key_op *)NULL) > @@ -538,6 +546,23 @@ static void __sysrq_put_key_op(int key, const struct sysrq_key_op *op_p) > sysrq_key_table[i] = op_p; > } > > +#define SYSRQ_NMI_FIFO_SIZE 64 > +static DEFINE_KFIFO(sysrq_nmi_fifo, int, SYSRQ_NMI_FIFO_SIZE); > + > +static void sysrq_do_nmi_work(struct irq_work *work) > +{ > + const struct sysrq_key_op *op_p; > + int key; > + > + while (kfifo_out(&sysrq_nmi_fifo, &key, 1)) { > + op_p = __sysrq_get_key_op(key); > + if (op_p) > + op_p->handler(key); > + } > +} > + > +static DEFINE_IRQ_WORK(sysrq_nmi_work, sysrq_do_nmi_work); > + > void __handle_sysrq(int key, bool check_mask) > { > const struct sysrq_key_op *op_p; > @@ -568,7 +593,13 @@ void __handle_sysrq(int key, bool check_mask) > if (!check_mask || sysrq_on_mask(op_p->enable_mask)) { > pr_info("%s\n", op_p->action_msg); > console_loglevel = orig_log_level; > - op_p->handler(key); > + > + if (in_nmi() && !op_p->nmi_safe) { > + kfifo_in(&sysrq_nmi_fifo, &key, 1); > + irq_work_queue(&sysrq_nmi_work); > + } else { > + op_p->handler(key); > + } > } else { > pr_info("This sysrq operation is disabled.\n"); > console_loglevel = orig_log_level; > diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h > index 3a582ec..630b5b9 100644 > --- a/include/linux/sysrq.h > +++ b/include/linux/sysrq.h > @@ -34,6 +34,7 @@ struct sysrq_key_op { > const char * const help_msg; > const char * const action_msg; > const int enable_mask; > + const bool nmi_safe; > }; > > #ifdef CONFIG_MAGIC_SYSRQ > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c > index 9e59347..2b51173 100644 > --- a/kernel/debug/debug_core.c > +++ b/kernel/debug/debug_core.c > @@ -943,6 +943,7 @@ static const struct sysrq_key_op sysrq_dbg_op = { > .handler = sysrq_handle_dbg, > .help_msg = "debug(g)", > .action_msg = "DEBUG", > + .nmi_safe = true, > }; > #endif > > -- > 2.7.4
On Mon, 20 Jul 2020 at 18:58, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Fri, Jul 17, 2020 at 11:50:54AM +0530, Sumit Garg wrote: > > With the advent of pseudo NMIs on arm64 it should be possible to request > > serial device interrupt as an NMI rather than IRQ. Having NMI driven > > serial RX will make it possible to do a magic sysrq in NMI context > > rather than in normal IRQ context. So as a preparatory step, make sysrq > > handler NMI aware and allow system requests to be marked as NMI safe > > which can run directly in NMI context while others being categorized > > as not NMI safe will be queued as irq_work for later processing in > > normal interrupt context. > > The opening line seems a little vague and, to be honest whether we use > pseudo-NMI, FIQ (on Armv7) or some other architecture trick doesn't > really matter at this point. > > Maybe just: > > In a future patch we will add support to the serial core to make it > possible to trigger a magic sysrq from an NMI context. Prepare for this > by marking some sysrq actions as NMI safe. Safe actions will be allowed > to run from NMI context whilst that that cannot run from an NMI will be > queued as irq_work for later processing. Okay, will use it instead. > > > > > A particular sysrq handler is only marked as NMI safe in case the handler > > isn't contending for any synchronization primimitives as in NMI context > > they are expected to cause deadlocks. > > > > Note: Here kernel debugger (kgdb) is an exception which is a kind of stop > > the world debugger and designed to run in NMI context. > > A "Note:" is a bit heavy handed here and also somewhat unclear. In > particular whether a debugger stops the world doesn't really affect > whether or not it was designed to be NMI safe. > > Maybe merge into the preceding paragraph: > > Note that the debug sysrq do not contend for any synchronization > primitives. It does call kgdb_breakpoint() to provoke a trap but that > trap handler should be NMI safe on architectures that implement an NMI. > Okay. > > > This feature is especially helpful in case the primary CPU is stuck in > > deadlock with interrupts disabled and doesn't honour serial device > > interrupt. So having sysrq running in NMI context is helpful to debug > > such scenarios. > > No it doesn't. It simply marks some sysrqs as NMI safe... > Yeah this patch in itself doesn't provide a complete debugging feature. Will get rid of this text instead. -Sumit > > Daniel. > > > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > drivers/tty/sysrq.c | 33 ++++++++++++++++++++++++++++++++- > > include/linux/sysrq.h | 1 + > > kernel/debug/debug_core.c | 1 + > > 3 files changed, 34 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c > > index 7c95afa9..8017e33 100644 > > --- a/drivers/tty/sysrq.c > > +++ b/drivers/tty/sysrq.c > > @@ -50,6 +50,8 @@ > > #include <linux/syscalls.h> > > #include <linux/of.h> > > #include <linux/rcupdate.h> > > +#include <linux/irq_work.h> > > +#include <linux/kfifo.h> > > > > #include <asm/ptrace.h> > > #include <asm/irq_regs.h> > > @@ -111,6 +113,7 @@ static const struct sysrq_key_op sysrq_loglevel_op = { > > .help_msg = "loglevel(0-9)", > > .action_msg = "Changing Loglevel", > > .enable_mask = SYSRQ_ENABLE_LOG, > > + .nmi_safe = true, > > }; > > > > #ifdef CONFIG_VT > > @@ -157,6 +160,7 @@ static const struct sysrq_key_op sysrq_crash_op = { > > .help_msg = "crash(c)", > > .action_msg = "Trigger a crash", > > .enable_mask = SYSRQ_ENABLE_DUMP, > > + .nmi_safe = true, > > }; > > > > static void sysrq_handle_reboot(int key) > > @@ -170,6 +174,7 @@ static const struct sysrq_key_op sysrq_reboot_op = { > > .help_msg = "reboot(b)", > > .action_msg = "Resetting", > > .enable_mask = SYSRQ_ENABLE_BOOT, > > + .nmi_safe = true, > > }; > > > > const struct sysrq_key_op *__sysrq_reboot_op = &sysrq_reboot_op; > > @@ -217,6 +222,7 @@ static const struct sysrq_key_op sysrq_showlocks_op = { > > .handler = sysrq_handle_showlocks, > > .help_msg = "show-all-locks(d)", > > .action_msg = "Show Locks Held", > > + .nmi_safe = true, > > }; > > #else > > #define sysrq_showlocks_op (*(const struct sysrq_key_op *)NULL) > > @@ -289,6 +295,7 @@ static const struct sysrq_key_op sysrq_showregs_op = { > > .help_msg = "show-registers(p)", > > .action_msg = "Show Regs", > > .enable_mask = SYSRQ_ENABLE_DUMP, > > + .nmi_safe = true, > > }; > > > > static void sysrq_handle_showstate(int key) > > @@ -326,6 +333,7 @@ static const struct sysrq_key_op sysrq_ftrace_dump_op = { > > .help_msg = "dump-ftrace-buffer(z)", > > .action_msg = "Dump ftrace buffer", > > .enable_mask = SYSRQ_ENABLE_DUMP, > > + .nmi_safe = true, > > }; > > #else > > #define sysrq_ftrace_dump_op (*(const struct sysrq_key_op *)NULL) > > @@ -538,6 +546,23 @@ static void __sysrq_put_key_op(int key, const struct sysrq_key_op *op_p) > > sysrq_key_table[i] = op_p; > > } > > > > +#define SYSRQ_NMI_FIFO_SIZE 64 > > +static DEFINE_KFIFO(sysrq_nmi_fifo, int, SYSRQ_NMI_FIFO_SIZE); > > + > > +static void sysrq_do_nmi_work(struct irq_work *work) > > +{ > > + const struct sysrq_key_op *op_p; > > + int key; > > + > > + while (kfifo_out(&sysrq_nmi_fifo, &key, 1)) { > > + op_p = __sysrq_get_key_op(key); > > + if (op_p) > > + op_p->handler(key); > > + } > > +} > > + > > +static DEFINE_IRQ_WORK(sysrq_nmi_work, sysrq_do_nmi_work); > > + > > void __handle_sysrq(int key, bool check_mask) > > { > > const struct sysrq_key_op *op_p; > > @@ -568,7 +593,13 @@ void __handle_sysrq(int key, bool check_mask) > > if (!check_mask || sysrq_on_mask(op_p->enable_mask)) { > > pr_info("%s\n", op_p->action_msg); > > console_loglevel = orig_log_level; > > - op_p->handler(key); > > + > > + if (in_nmi() && !op_p->nmi_safe) { > > + kfifo_in(&sysrq_nmi_fifo, &key, 1); > > + irq_work_queue(&sysrq_nmi_work); > > + } else { > > + op_p->handler(key); > > + } > > } else { > > pr_info("This sysrq operation is disabled.\n"); > > console_loglevel = orig_log_level; > > diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h > > index 3a582ec..630b5b9 100644 > > --- a/include/linux/sysrq.h > > +++ b/include/linux/sysrq.h > > @@ -34,6 +34,7 @@ struct sysrq_key_op { > > const char * const help_msg; > > const char * const action_msg; > > const int enable_mask; > > + const bool nmi_safe; > > }; > > > > #ifdef CONFIG_MAGIC_SYSRQ > > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c > > index 9e59347..2b51173 100644 > > --- a/kernel/debug/debug_core.c > > +++ b/kernel/debug/debug_core.c > > @@ -943,6 +943,7 @@ static const struct sysrq_key_op sysrq_dbg_op = { > > .handler = sysrq_handle_dbg, > > .help_msg = "debug(g)", > > .action_msg = "DEBUG", > > + .nmi_safe = true, > > }; > > #endif > > > > -- > > 2.7.4
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c index 7c95afa9..8017e33 100644 --- a/drivers/tty/sysrq.c +++ b/drivers/tty/sysrq.c @@ -50,6 +50,8 @@ #include <linux/syscalls.h> #include <linux/of.h> #include <linux/rcupdate.h> +#include <linux/irq_work.h> +#include <linux/kfifo.h> #include <asm/ptrace.h> #include <asm/irq_regs.h> @@ -111,6 +113,7 @@ static const struct sysrq_key_op sysrq_loglevel_op = { .help_msg = "loglevel(0-9)", .action_msg = "Changing Loglevel", .enable_mask = SYSRQ_ENABLE_LOG, + .nmi_safe = true, }; #ifdef CONFIG_VT @@ -157,6 +160,7 @@ static const struct sysrq_key_op sysrq_crash_op = { .help_msg = "crash(c)", .action_msg = "Trigger a crash", .enable_mask = SYSRQ_ENABLE_DUMP, + .nmi_safe = true, }; static void sysrq_handle_reboot(int key) @@ -170,6 +174,7 @@ static const struct sysrq_key_op sysrq_reboot_op = { .help_msg = "reboot(b)", .action_msg = "Resetting", .enable_mask = SYSRQ_ENABLE_BOOT, + .nmi_safe = true, }; const struct sysrq_key_op *__sysrq_reboot_op = &sysrq_reboot_op; @@ -217,6 +222,7 @@ static const struct sysrq_key_op sysrq_showlocks_op = { .handler = sysrq_handle_showlocks, .help_msg = "show-all-locks(d)", .action_msg = "Show Locks Held", + .nmi_safe = true, }; #else #define sysrq_showlocks_op (*(const struct sysrq_key_op *)NULL) @@ -289,6 +295,7 @@ static const struct sysrq_key_op sysrq_showregs_op = { .help_msg = "show-registers(p)", .action_msg = "Show Regs", .enable_mask = SYSRQ_ENABLE_DUMP, + .nmi_safe = true, }; static void sysrq_handle_showstate(int key) @@ -326,6 +333,7 @@ static const struct sysrq_key_op sysrq_ftrace_dump_op = { .help_msg = "dump-ftrace-buffer(z)", .action_msg = "Dump ftrace buffer", .enable_mask = SYSRQ_ENABLE_DUMP, + .nmi_safe = true, }; #else #define sysrq_ftrace_dump_op (*(const struct sysrq_key_op *)NULL) @@ -538,6 +546,23 @@ static void __sysrq_put_key_op(int key, const struct sysrq_key_op *op_p) sysrq_key_table[i] = op_p; } +#define SYSRQ_NMI_FIFO_SIZE 64 +static DEFINE_KFIFO(sysrq_nmi_fifo, int, SYSRQ_NMI_FIFO_SIZE); + +static void sysrq_do_nmi_work(struct irq_work *work) +{ + const struct sysrq_key_op *op_p; + int key; + + while (kfifo_out(&sysrq_nmi_fifo, &key, 1)) { + op_p = __sysrq_get_key_op(key); + if (op_p) + op_p->handler(key); + } +} + +static DEFINE_IRQ_WORK(sysrq_nmi_work, sysrq_do_nmi_work); + void __handle_sysrq(int key, bool check_mask) { const struct sysrq_key_op *op_p; @@ -568,7 +593,13 @@ void __handle_sysrq(int key, bool check_mask) if (!check_mask || sysrq_on_mask(op_p->enable_mask)) { pr_info("%s\n", op_p->action_msg); console_loglevel = orig_log_level; - op_p->handler(key); + + if (in_nmi() && !op_p->nmi_safe) { + kfifo_in(&sysrq_nmi_fifo, &key, 1); + irq_work_queue(&sysrq_nmi_work); + } else { + op_p->handler(key); + } } else { pr_info("This sysrq operation is disabled.\n"); console_loglevel = orig_log_level; diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h index 3a582ec..630b5b9 100644 --- a/include/linux/sysrq.h +++ b/include/linux/sysrq.h @@ -34,6 +34,7 @@ struct sysrq_key_op { const char * const help_msg; const char * const action_msg; const int enable_mask; + const bool nmi_safe; }; #ifdef CONFIG_MAGIC_SYSRQ diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c index 9e59347..2b51173 100644 --- a/kernel/debug/debug_core.c +++ b/kernel/debug/debug_core.c @@ -943,6 +943,7 @@ static const struct sysrq_key_op sysrq_dbg_op = { .handler = sysrq_handle_dbg, .help_msg = "debug(g)", .action_msg = "DEBUG", + .nmi_safe = true, }; #endif
With the advent of pseudo NMIs on arm64 it should be possible to request serial device interrupt as an NMI rather than IRQ. Having NMI driven serial RX will make it possible to do a magic sysrq in NMI context rather than in normal IRQ context. So as a preparatory step, make sysrq handler NMI aware and allow system requests to be marked as NMI safe which can run directly in NMI context while others being categorized as not NMI safe will be queued as irq_work for later processing in normal interrupt context. A particular sysrq handler is only marked as NMI safe in case the handler isn't contending for any synchronization primimitives as in NMI context they are expected to cause deadlocks. Note: Here kernel debugger (kgdb) is an exception which is a kind of stop the world debugger and designed to run in NMI context. This feature is especially helpful in case the primary CPU is stuck in deadlock with interrupts disabled and doesn't honour serial device interrupt. So having sysrq running in NMI context is helpful to debug such scenarios. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- drivers/tty/sysrq.c | 33 ++++++++++++++++++++++++++++++++- include/linux/sysrq.h | 1 + kernel/debug/debug_core.c | 1 + 3 files changed, 34 insertions(+), 1 deletion(-) -- 2.7.4