Message ID | 20230824083012.v11.4.Ie6c132b96ebbbcddbf6954b9469ed40a6960343c@changeid (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | arm64: Add IPI for backtraces / kgdb; try to use NMI for some IPIs | expand |
Quoting Douglas Anderson (2023-08-24 08:30:30) > diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h > index fac08e18bcd5..50ce8b697ff3 100644 > --- a/arch/arm64/include/asm/irq.h > +++ b/arch/arm64/include/asm/irq.h > @@ -6,6 +6,9 @@ > > #include <asm-generic/irq.h> > > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu); Some nits, but otherwise Reviewed-by: Stephen Boyd <swboyd@chromium.org> > +#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace > + > struct pt_regs; > > int set_handle_irq(void (*handle_irq)(struct pt_regs *)); > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index a5848f1ef817..c8896cbc5327 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -72,12 +73,18 @@ enum ipi_msg_type { > IPI_CPU_CRASH_STOP, > IPI_TIMER, > IPI_IRQ_WORK, > - NR_IPI > + NR_IPI, > + /* > + * Any enum >= NR_IPI and < MAX_IPI is special and not tracable > + * with trace_ipi_* > + */ > + IPI_CPU_BACKTRACE = NR_IPI, > + MAX_IPI > }; > > static int ipi_irq_base __read_mostly; > static int nr_ipi __read_mostly = NR_IPI; > -static struct irq_desc *ipi_desc[NR_IPI] __read_mostly; > +static struct irq_desc *ipi_desc[MAX_IPI] __read_mostly; Side note: it would be nice to mark ipi_desc as __ro_after_init. Same for nr_ipi and ipi_irq_base. > > static void ipi_setup(int cpu); > > @@ -845,6 +852,22 @@ static void __noreturn ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs > #endif > } > > +static void arm64_backtrace_ipi(cpumask_t *mask) > +{ > + __ipi_send_mask(ipi_desc[IPI_CPU_BACKTRACE], mask); > +} > + > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu) Can this be 'bool exclude_self' instead of int? That matches all other implementations from what I can tell. > +{ > + /* > + * NOTE: though nmi_trigger_cpumask_backtrace has "nmi_" in the name, USe nmi_trigger_cpumask_backtrace() to indicate function. > + * nothing about it truly needs to be implemented using an NMI, it's > + * just that it's _allowed_ to work with NMIs. If ipi_should_be_nmi() > + * returned false our backtrace attempt will just use a regular IPI. > + */ > + nmi_trigger_cpumask_backtrace(mask, exclude_cpu, arm64_backtrace_ipi);
Hi, On Fri, Aug 25, 2023 at 3:27 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Douglas Anderson (2023-08-24 08:30:30) > > diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h > > index fac08e18bcd5..50ce8b697ff3 100644 > > --- a/arch/arm64/include/asm/irq.h > > +++ b/arch/arm64/include/asm/irq.h > > @@ -6,6 +6,9 @@ > > > > #include <asm-generic/irq.h> > > > > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu); > > Some nits, but otherwise > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > > +#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace > > + > > struct pt_regs; > > > > int set_handle_irq(void (*handle_irq)(struct pt_regs *)); > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > > index a5848f1ef817..c8896cbc5327 100644 > > --- a/arch/arm64/kernel/smp.c > > +++ b/arch/arm64/kernel/smp.c > > @@ -72,12 +73,18 @@ enum ipi_msg_type { > > IPI_CPU_CRASH_STOP, > > IPI_TIMER, > > IPI_IRQ_WORK, > > - NR_IPI > > + NR_IPI, > > + /* > > + * Any enum >= NR_IPI and < MAX_IPI is special and not tracable > > + * with trace_ipi_* > > + */ > > + IPI_CPU_BACKTRACE = NR_IPI, > > + MAX_IPI > > }; > > > > static int ipi_irq_base __read_mostly; > > static int nr_ipi __read_mostly = NR_IPI; > > -static struct irq_desc *ipi_desc[NR_IPI] __read_mostly; > > +static struct irq_desc *ipi_desc[MAX_IPI] __read_mostly; > > Side note: it would be nice to mark ipi_desc as __ro_after_init. Same > for nr_ipi and ipi_irq_base. I'd rather not change it in this patch since it's a pre-existing and separate issue, but I can add a patch to the end of the series for that if I end up spinning it. Otherwise I can send a follow-up patch for it. > > static void ipi_setup(int cpu); > > > > @@ -845,6 +852,22 @@ static void __noreturn ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs > > #endif > > } > > > > +static void arm64_backtrace_ipi(cpumask_t *mask) > > +{ > > + __ipi_send_mask(ipi_desc[IPI_CPU_BACKTRACE], mask); > > +} > > + > > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu) > > Can this be 'bool exclude_self' instead of int? That matches all other > implementations from what I can tell. Nope. See the part of the commit message that says: This patch depends on commit 36759e343ff9 ("nmi_backtrace: allow excluding an arbitrary CPU") since that commit changed the prototype of arch_trigger_cpumask_backtrace(), which this patch implements. > > +{ > > + /* > > + * NOTE: though nmi_trigger_cpumask_backtrace has "nmi_" in the name, > > USe nmi_trigger_cpumask_backtrace() to indicate function. I won't plan on doing an immediate spin for this and for now I'll wait for additional feedback. If a maintainer is getting ready to land this, I'm happy to post a new version with this fix or also happy if a maintainer wants to add the "()" while applying. -Doug
Quoting Doug Anderson (2023-08-25 16:02:46) > On Fri, Aug 25, 2023 at 3:27 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Douglas Anderson (2023-08-24 08:30:30) > > > diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h > > > > > > static int ipi_irq_base __read_mostly; > > > static int nr_ipi __read_mostly = NR_IPI; > > > -static struct irq_desc *ipi_desc[NR_IPI] __read_mostly; > > > +static struct irq_desc *ipi_desc[MAX_IPI] __read_mostly; > > > > Side note: it would be nice to mark ipi_desc as __ro_after_init. Same > > for nr_ipi and ipi_irq_base. > > I'd rather not change it in this patch since it's a pre-existing and > separate issue, but I can add a patch to the end of the series for > that if I end up spinning it. Otherwise I can send a follow-up patch > for it. Of course. Don't change it in this patch. > > > > > static void ipi_setup(int cpu); > > > > > > @@ -845,6 +852,22 @@ static void __noreturn ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs > > > #endif > > > } > > > > > > +static void arm64_backtrace_ipi(cpumask_t *mask) > > > +{ > > > + __ipi_send_mask(ipi_desc[IPI_CPU_BACKTRACE], mask); > > > +} > > > + > > > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu) > > > > Can this be 'bool exclude_self' instead of int? That matches all other > > implementations from what I can tell. > > Nope. See the part of the commit message that says: > > This patch depends on commit 36759e343ff9 ("nmi_backtrace: allow > excluding an arbitrary CPU") since that commit changed the prototype > of arch_trigger_cpumask_backtrace(), which this patch implements. Ah, ok. Sounds fine then.
> -----Original Message----- > Subject: [PATCH v11 4/6] arm64: smp: Add arch support for backtrace using > pseudo-NMI > > Enable arch_trigger_cpumask_backtrace() support on arm64. This enables > things much like they are enabled on arm32 (including some of the > funky logic around NR_IPI, nr_ipi, and MAX_IPI) but with the > difference that, unlike arm32, we'll try to enable the backtrace to > use pseudo-NMI. > > NOTE: this patch is a squash of the little bit of code adding the > ability to mark an IPI to try to use pseudo-NMI plus the little bit of > code to hook things up for kgdb. This approach was decided upon in the > discussion of v9 [1]. > > This patch depends on commit 36759e343ff9 ("nmi_backtrace: allow Hi, Is this commit ID valid? I believe this commit is not included in the mainline yet. Other than this, Reviewed-by: Misono Tomohiro <misono.tomohiro@fujitsu.com> Regards, Tomohiro > excluding an arbitrary CPU") since that commit changed the prototype > of arch_trigger_cpumask_backtrace(), which this patch implements. > > [1] https://lore.kernel.org/r/ZORY51mF4alI41G1@FVFF77S0Q05N > > Co-developed-by: Sumit Garg <sumit.garg@linaro.org> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > Co-developed-by: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v11: > - Adjust comment about NR_IPI/MAX_IPI. > - Don't use confusing "backed by" idiom in comment. > - Made arm64_backtrace_ipi() static. > > Changes in v10: > - Backtrace now directly supported in smp.c > - Squash backtrace into patch adding support for pseudo-NMI IPIs. > > Changes in v9: > - Added comments that we might not be using NMI always. > - Fold in v8 patch #10 ("Fallback to a regular IPI if NMI isn't enabled") > - Moved header file out of "include" since it didn't need to be there. > - Remove arm64_supports_nmi() > - Renamed "NMI IPI" to "debug IPI" since it might not be backed by NMI. > - arch_trigger_cpumask_backtrace() no longer returns bool > > Changes in v8: > - Removed "#ifdef CONFIG_SMP" since arm64 is always SMP > - debug_ipi_setup() and debug_ipi_teardown() no longer take cpu param > > arch/arm64/include/asm/irq.h | 3 ++ > arch/arm64/kernel/smp.c | 86 > +++++++++++++++++++++++++++++++----- > 2 files changed, 78 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h > index fac08e18bcd5..50ce8b697ff3 100644 > --- a/arch/arm64/include/asm/irq.h > +++ b/arch/arm64/include/asm/irq.h > @@ -6,6 +6,9 @@ > > #include <asm-generic/irq.h> > > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int > exclude_cpu); > +#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace > + > struct pt_regs; > > int set_handle_irq(void (*handle_irq)(struct pt_regs *)); > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index a5848f1ef817..c8896cbc5327 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -33,6 +33,7 @@ > #include <linux/kernel_stat.h> > #include <linux/kexec.h> > #include <linux/kvm_host.h> > +#include <linux/nmi.h> > > #include <asm/alternative.h> > #include <asm/atomic.h> > @@ -72,12 +73,18 @@ enum ipi_msg_type { > IPI_CPU_CRASH_STOP, > IPI_TIMER, > IPI_IRQ_WORK, > - NR_IPI > + NR_IPI, > + /* > + * Any enum >= NR_IPI and < MAX_IPI is special and not tracable > + * with trace_ipi_* > + */ > + IPI_CPU_BACKTRACE = NR_IPI, > + MAX_IPI > }; > > static int ipi_irq_base __read_mostly; > static int nr_ipi __read_mostly = NR_IPI; > -static struct irq_desc *ipi_desc[NR_IPI] __read_mostly; > +static struct irq_desc *ipi_desc[MAX_IPI] __read_mostly; > > static void ipi_setup(int cpu); > > @@ -845,6 +852,22 @@ static void __noreturn ipi_cpu_crash_stop(unsigned int > cpu, struct pt_regs *regs > #endif > } > > +static void arm64_backtrace_ipi(cpumask_t *mask) > +{ > + __ipi_send_mask(ipi_desc[IPI_CPU_BACKTRACE], mask); > +} > + > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int > exclude_cpu) > +{ > + /* > + * NOTE: though nmi_trigger_cpumask_backtrace has "nmi_" in the > name, > + * nothing about it truly needs to be implemented using an NMI, it's > + * just that it's _allowed_ to work with NMIs. If ipi_should_be_nmi() > + * returned false our backtrace attempt will just use a regular IPI. > + */ > + nmi_trigger_cpumask_backtrace(mask, exclude_cpu, > arm64_backtrace_ipi); > +} > + > /* > * Main handler for inter-processor interrupts > */ > @@ -888,6 +911,14 @@ static void do_handle_IPI(int ipinr) > break; > #endif > > + case IPI_CPU_BACKTRACE: > + /* > + * NOTE: in some cases this _won't_ be NMI context. See the > + * comment in arch_trigger_cpumask_backtrace(). > + */ > + nmi_cpu_backtrace(get_irq_regs()); > + break; > + > default: > pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr); > break; > @@ -909,6 +940,19 @@ static void smp_cross_call(const struct cpumask *target, > unsigned int ipinr) > __ipi_send_mask(ipi_desc[ipinr], target); > } > > +static bool ipi_should_be_nmi(enum ipi_msg_type ipi) > +{ > + if (!system_uses_irq_prio_masking()) > + return false; > + > + switch (ipi) { > + case IPI_CPU_BACKTRACE: > + return true; > + default: > + return false; > + } > +} > + > static void ipi_setup(int cpu) > { > int i; > @@ -916,8 +960,14 @@ static void ipi_setup(int cpu) > if (WARN_ON_ONCE(!ipi_irq_base)) > return; > > - for (i = 0; i < nr_ipi; i++) > - enable_percpu_irq(ipi_irq_base + i, 0); > + for (i = 0; i < nr_ipi; i++) { > + if (ipi_should_be_nmi(i)) { > + prepare_percpu_nmi(ipi_irq_base + i); > + enable_percpu_nmi(ipi_irq_base + i, 0); > + } else { > + enable_percpu_irq(ipi_irq_base + i, 0); > + } > + } > } > > #ifdef CONFIG_HOTPLUG_CPU > @@ -928,8 +978,14 @@ static void ipi_teardown(int cpu) > if (WARN_ON_ONCE(!ipi_irq_base)) > return; > > - for (i = 0; i < nr_ipi; i++) > - disable_percpu_irq(ipi_irq_base + i); > + for (i = 0; i < nr_ipi; i++) { > + if (ipi_should_be_nmi(i)) { > + disable_percpu_nmi(ipi_irq_base + i); > + teardown_percpu_nmi(ipi_irq_base + i); > + } else { > + disable_percpu_irq(ipi_irq_base + i); > + } > + } > } > #endif > > @@ -937,15 +993,23 @@ void __init set_smp_ipi_range(int ipi_base, int n) > { > int i; > > - WARN_ON(n < NR_IPI); > - nr_ipi = min(n, NR_IPI); > + WARN_ON(n < MAX_IPI); > + nr_ipi = min(n, MAX_IPI); > > for (i = 0; i < nr_ipi; i++) { > int err; > > - err = request_percpu_irq(ipi_base + i, ipi_handler, > - "IPI", &cpu_number); > - WARN_ON(err); > + if (ipi_should_be_nmi(i)) { > + err = request_percpu_nmi(ipi_base + i, ipi_handler, > + "IPI", &cpu_number); > + WARN(err, "Could not request IPI %d as NMI, > err=%d\n", > + i, err); > + } else { > + err = request_percpu_irq(ipi_base + i, ipi_handler, > + "IPI", &cpu_number); > + WARN(err, "Could not request IPI %d as IRQ, > err=%d\n", > + i, err); > + } > > ipi_desc[i] = irq_to_desc(ipi_base + i); > irq_set_status_flags(ipi_base + i, IRQ_HIDDEN); > -- > 2.42.0.rc1.204.g551eb34607-goog > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi, On Mon, Aug 28, 2023 at 10:23 PM Tomohiro Misono (Fujitsu) <misono.tomohiro@fujitsu.com> wrote: > > > -----Original Message----- > > Subject: [PATCH v11 4/6] arm64: smp: Add arch support for backtrace using > > pseudo-NMI > > > > Enable arch_trigger_cpumask_backtrace() support on arm64. This enables > > things much like they are enabled on arm32 (including some of the > > funky logic around NR_IPI, nr_ipi, and MAX_IPI) but with the > > difference that, unlike arm32, we'll try to enable the backtrace to > > use pseudo-NMI. > > > > NOTE: this patch is a squash of the little bit of code adding the > > ability to mark an IPI to try to use pseudo-NMI plus the little bit of > > code to hook things up for kgdb. This approach was decided upon in the > > discussion of v9 [1]. > > > > This patch depends on commit 36759e343ff9 ("nmi_backtrace: allow > > Hi, > Is this commit ID valid? I believe this commit is not included in the > mainline yet. Other than this, > Reviewed-by: Misono Tomohiro <misono.tomohiro@fujitsu.com> Ah, good call. I must have grabbed that git hash before the commit moved from Andrew Morton's unstable branch to his stable branch. As far as I can tell, it should be commit 8d539b84f1e3 ("nmi_backtrace: allow excluding an arbitrary CPU"). ...at least that's what's in linuxnext today. That also appears to match the commit hash in the pull request that Andrew just sent to Linus [1]. I'll update the git hash and add your tag in v12, which I'm currently aiming to send out tomorrow. [1] https://lore.kernel.org/r/20230828225431.354d3d2d3b80ee5b88e65eb5@linux-foundation.org
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h index fac08e18bcd5..50ce8b697ff3 100644 --- a/arch/arm64/include/asm/irq.h +++ b/arch/arm64/include/asm/irq.h @@ -6,6 +6,9 @@ #include <asm-generic/irq.h> +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu); +#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace + struct pt_regs; int set_handle_irq(void (*handle_irq)(struct pt_regs *)); diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index a5848f1ef817..c8896cbc5327 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -33,6 +33,7 @@ #include <linux/kernel_stat.h> #include <linux/kexec.h> #include <linux/kvm_host.h> +#include <linux/nmi.h> #include <asm/alternative.h> #include <asm/atomic.h> @@ -72,12 +73,18 @@ enum ipi_msg_type { IPI_CPU_CRASH_STOP, IPI_TIMER, IPI_IRQ_WORK, - NR_IPI + NR_IPI, + /* + * Any enum >= NR_IPI and < MAX_IPI is special and not tracable + * with trace_ipi_* + */ + IPI_CPU_BACKTRACE = NR_IPI, + MAX_IPI }; static int ipi_irq_base __read_mostly; static int nr_ipi __read_mostly = NR_IPI; -static struct irq_desc *ipi_desc[NR_IPI] __read_mostly; +static struct irq_desc *ipi_desc[MAX_IPI] __read_mostly; static void ipi_setup(int cpu); @@ -845,6 +852,22 @@ static void __noreturn ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs #endif } +static void arm64_backtrace_ipi(cpumask_t *mask) +{ + __ipi_send_mask(ipi_desc[IPI_CPU_BACKTRACE], mask); +} + +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu) +{ + /* + * NOTE: though nmi_trigger_cpumask_backtrace has "nmi_" in the name, + * nothing about it truly needs to be implemented using an NMI, it's + * just that it's _allowed_ to work with NMIs. If ipi_should_be_nmi() + * returned false our backtrace attempt will just use a regular IPI. + */ + nmi_trigger_cpumask_backtrace(mask, exclude_cpu, arm64_backtrace_ipi); +} + /* * Main handler for inter-processor interrupts */ @@ -888,6 +911,14 @@ static void do_handle_IPI(int ipinr) break; #endif + case IPI_CPU_BACKTRACE: + /* + * NOTE: in some cases this _won't_ be NMI context. See the + * comment in arch_trigger_cpumask_backtrace(). + */ + nmi_cpu_backtrace(get_irq_regs()); + break; + default: pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr); break; @@ -909,6 +940,19 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr) __ipi_send_mask(ipi_desc[ipinr], target); } +static bool ipi_should_be_nmi(enum ipi_msg_type ipi) +{ + if (!system_uses_irq_prio_masking()) + return false; + + switch (ipi) { + case IPI_CPU_BACKTRACE: + return true; + default: + return false; + } +} + static void ipi_setup(int cpu) { int i; @@ -916,8 +960,14 @@ static void ipi_setup(int cpu) if (WARN_ON_ONCE(!ipi_irq_base)) return; - for (i = 0; i < nr_ipi; i++) - enable_percpu_irq(ipi_irq_base + i, 0); + for (i = 0; i < nr_ipi; i++) { + if (ipi_should_be_nmi(i)) { + prepare_percpu_nmi(ipi_irq_base + i); + enable_percpu_nmi(ipi_irq_base + i, 0); + } else { + enable_percpu_irq(ipi_irq_base + i, 0); + } + } } #ifdef CONFIG_HOTPLUG_CPU @@ -928,8 +978,14 @@ static void ipi_teardown(int cpu) if (WARN_ON_ONCE(!ipi_irq_base)) return; - for (i = 0; i < nr_ipi; i++) - disable_percpu_irq(ipi_irq_base + i); + for (i = 0; i < nr_ipi; i++) { + if (ipi_should_be_nmi(i)) { + disable_percpu_nmi(ipi_irq_base + i); + teardown_percpu_nmi(ipi_irq_base + i); + } else { + disable_percpu_irq(ipi_irq_base + i); + } + } } #endif @@ -937,15 +993,23 @@ void __init set_smp_ipi_range(int ipi_base, int n) { int i; - WARN_ON(n < NR_IPI); - nr_ipi = min(n, NR_IPI); + WARN_ON(n < MAX_IPI); + nr_ipi = min(n, MAX_IPI); for (i = 0; i < nr_ipi; i++) { int err; - err = request_percpu_irq(ipi_base + i, ipi_handler, - "IPI", &cpu_number); - WARN_ON(err); + if (ipi_should_be_nmi(i)) { + err = request_percpu_nmi(ipi_base + i, ipi_handler, + "IPI", &cpu_number); + WARN(err, "Could not request IPI %d as NMI, err=%d\n", + i, err); + } else { + err = request_percpu_irq(ipi_base + i, ipi_handler, + "IPI", &cpu_number); + WARN(err, "Could not request IPI %d as IRQ, err=%d\n", + i, err); + } ipi_desc[i] = irq_to_desc(ipi_base + i); irq_set_status_flags(ipi_base + i, IRQ_HIDDEN);