lkml.org 
[lkml]   [2020]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC Patch v1 4/4] arm64: kgdb: Round up cpus using IPI_CALL_NMI_FUNC
    Hi,

    On Fri, Apr 24, 2020 at 4:11 AM Sumit Garg <sumit.garg@linaro.org> wrote:
    >
    > arm64 platforms with GICv3 or later supports pseudo NMIs which can be
    > leveraged to round up CPUs which are stuck in hard lockup state with
    > interrupts disabled that wouldn't be possible with a normal IPI.
    >
    > So instead switch to round up CPUs using IPI_CALL_NMI_FUNC. And in
    > case a particular arm64 platform doesn't supports pseudo NMIs,
    > IPI_CALL_NMI_FUNC will act as a normal IPI which maintains existing
    > kgdb functionality.
    >
    > Also, one thing to note here is that with CPUs running in NMI context,
    > kernel has special handling for printk() which involves CPU specific
    > buffers and defering printk() until exit from NMI context. But with kgdb
    > we don't want to defer printk() especially backtrace on corresponding
    > CPUs. So switch to normal printk() context instead prior to entering
    > kgdb context.
    >
    > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
    > ---
    > arch/arm64/kernel/kgdb.c | 15 +++++++++++++++
    > arch/arm64/kernel/smp.c | 17 ++++++++++++++---
    > 2 files changed, 29 insertions(+), 3 deletions(-)
    >
    > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
    > index 4311992..0851ead 100644
    > --- a/arch/arm64/kernel/kgdb.c
    > +++ b/arch/arm64/kernel/kgdb.c
    > @@ -14,6 +14,7 @@
    > #include <linux/kgdb.h>
    > #include <linux/kprobes.h>
    > #include <linux/sched/task_stack.h>
    > +#include <linux/smp.h>
    >
    > #include <asm/debug-monitors.h>
    > #include <asm/insn.h>
    > @@ -353,3 +354,17 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
    > return aarch64_insn_write((void *)bpt->bpt_addr,
    > *(u32 *)bpt->saved_instr);
    > }
    > +
    > +#ifdef CONFIG_SMP
    > +void kgdb_roundup_cpus(void)
    > +{
    > + struct cpumask mask;
    > +
    > + cpumask_copy(&mask, cpu_online_mask);
    > + cpumask_clear_cpu(raw_smp_processor_id(), &mask);
    > + if (cpumask_empty(&mask))
    > + return;
    > +
    > + arch_send_call_nmi_func_ipi_mask(&mask);
    > +}
    > +#endif
    > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
    > index 27c8ee1..c7158f6e8 100644
    > --- a/arch/arm64/kernel/smp.c
    > +++ b/arch/arm64/kernel/smp.c
    > @@ -31,6 +31,7 @@
    > #include <linux/of.h>
    > #include <linux/irq_work.h>
    > #include <linux/kexec.h>
    > +#include <linux/kgdb.h>
    > #include <linux/kvm_host.h>
    >
    > #include <asm/alternative.h>
    > @@ -976,9 +977,19 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
    > /* Handle it as a normal interrupt if not in NMI context */
    > if (!in_nmi())
    > irq_enter();
    > -
    > - /* nop, IPI handlers for special features can be added here. */
    > -
    > +#ifdef CONFIG_KGDB

    My vote would be to keep "ifdef"s out of the middle of functions. Can
    you put your code in "arch/arm64/kernel/kgdb.c" and then have a dummpy
    no-op function if "CONFIG_KGDB" isn't defined?


    > + if (atomic_read(&kgdb_active) != -1) {
    > + /*
    > + * For kgdb to work properly, we need printk to operate
    > + * in normal context.
    > + */
    > + if (in_nmi())
    > + printk_nmi_exit();

    It feels like all the printk management belongs in kgdb_nmicallback().
    ...or is there some reason that this isn't a problem for other
    platforms using NMI? Maybe it's just that nobody has noticed it yet?


    > + kgdb_nmicallback(raw_smp_processor_id(), regs);

    Why do you need to call raw_smp_processor_id()? Are you expecting a
    different value than the local variable "cpu"?


    > + if (in_nmi())
    > + printk_nmi_enter();
    > + }
    > +#endif
    > if (!in_nmi())
    > irq_exit();
    > break;

    Not that I really know what I'm talking about since I really don't
    know arm64 at this level very well, but I'll ask anyway and probably
    look like a fool... I had a note that said:

    * Will Deacon says:
    *
    * the whole roundup code is sketchy and it's the only place in the kernel
    * which tries to perform I-cache maintenance with irqs disabled, leading
    * to this nasty hack in the arch code:
    *
    * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/cacheflush.h#n74

    I presume that, if nothing else, the comment needs to be updated.
    ...but is the situation any better (or worse?) with your new solution?

    -Doug

    \
     
     \ /
      Last update: 2020-04-24 22:49    [W:3.899 / U:0.180 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site