Messages in this thread | | | Date | Tue, 22 Aug 2023 07:42:47 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH v9 3/7] arm64: Add framework for a debug IPI |
| |
On Mon, Aug 21, 2023 at 03:16:56PM -0700, Doug Anderson wrote: > Hi, > > On Mon, Aug 7, 2023 at 3:12 AM Mark Rutland <mark.rutland@arm.com> wrote: > > > > On Thu, Jun 01, 2023 at 02:31:47PM -0700, Douglas Anderson wrote: > > > From: Sumit Garg <sumit.garg@linaro.org> > > > > > > Introduce a framework for an IPI that will be used for debug > > > purposes. The primary use case of this IPI will be to generate stack > > > crawls on other CPUs, but it will also be used to round up CPUs for > > > kgdb. > > > > > > When possible, we try to allocate this debug IPI as an NMI (or a > > > pseudo NMI). If that fails (due to CONFIG, an incompatible interrupt > > > controller, a quirk, missing the "irqchip.gicv3_pseudo_nmi=1" kernel > > > parameter, etc) we fall back to a normal IPI. > > > > > > NOTE: hooking this up for CPU backtrace / kgdb will happen in a future > > > patch, this just adds the framework. > > > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > I think that we shouldn't add a framework in a separate file for this: > > > > * This is very similar to our existing IPI management in smp.c, so it feels > > like duplication, or at least another thing we'd like to keep in-sync. > > > > * We're going to want an NMI backtrace regardless of KGDB > > > > * We're going to want the IPI_CPU_STOP and IPI_CRASH_CPU_STOP IPIs to be NMIs > > too. > > > > I reckon it'd be better to extend the existing IPI logic in smp.c to allow IPIs > > to be requested as NMIs, e.g. > > > > ---- > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > > index edd63894d61e8..48e6aa62c473e 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> > > @@ -926,6 +927,21 @@ 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) { > > + /* > > + * TODO: select NMI IPIs here > > + */ > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > static void ipi_setup(int cpu) > > { > > int i; > > @@ -933,8 +949,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 > > @@ -945,8 +967,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 > > > > @@ -958,11 +986,19 @@ void __init set_smp_ipi_range(int ipi_base, int n) > > nr_ipi = min(n, NR_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); > > + int err = -EINVAL; > > + > > + 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); > > ---- > > > > ... and then if we need an IPI for KGDB, we can add that to the existing list > > of IPIs, and have it requested/enabled/disabled as usual. > > Sounds good. I'm starting to work on v10 incorporating your feedback. > A few quick questions: > > 1. If I mostly take your patch above verbatim, do you have any > suggested tags for Author/Signed-off-by? I'd tend to set you as the > author but I can't do that because you didn't provide a > Signed-off-by...
Sorry about that. For the above:
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
If squashed into another patch, then feel free to use:
Co-developed-by: Mark Rutland <mark.rutland@arm.com>
> 2. Would you prefer this patch on its own, or would you rather it be > squashed with the first user ("backtrace")? On its own, I think I have > to get rid of the "switch" statement in ipi_should_be_nmi() and just > return false;
I reckon it makes sense to squash it with the first user.
Thanks, Mark.
| |