Messages in this thread | | | From | Doug Anderson <> | Date | Fri, 24 Apr 2020 13:46:59 -0700 | Subject | Re: [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
| |