| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAD=FV=VVJsJSc=uQWad4x0EV2-iROFcueW_=4VbM+0N0+aD96g@mail.gmail.com>
Date: Mon, 21 Aug 2023 15:16:56 -0700
From: Doug Anderson <dianders@...omium.org>
To: Mark Rutland <mark.rutland@....com>
Cc: Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Sumit Garg <sumit.garg@...aro.org>,
Daniel Thompson <daniel.thompson@...aro.org>,
Marc Zyngier <maz@...nel.org>,
linux-perf-users@...r.kernel.org, ito-yuichi@...itsu.com,
Chen-Yu Tsai <wens@...e.org>, Ard Biesheuvel <ardb@...nel.org>,
Stephen Boyd <swboyd@...omium.org>,
Peter Zijlstra <peterz@...radead.org>,
Thomas Gleixner <tglx@...utronix.de>,
linux-arm-kernel@...ts.infradead.org,
kgdb-bugreport@...ts.sourceforge.net,
Masayoshi Mizuma <msys.mizuma@...il.com>,
"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
Lecopzer Chen <lecopzer.chen@...iatek.com>,
Andrey Konovalov <andreyknvl@...il.com>,
Masahiro Yamada <masahiroy@...nel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 3/7] arm64: Add framework for a debug IPI
Hi,
On Mon, Aug 7, 2023 at 3:12 AM Mark Rutland <mark.rutland@....com> wrote:
>
> On Thu, Jun 01, 2023 at 02:31:47PM -0700, Douglas Anderson wrote:
> > From: Sumit Garg <sumit.garg@...aro.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@...aro.org>
> > Signed-off-by: Douglas Anderson <dianders@...omium.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...
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;
Thanks!
-Doug
Powered by blists - more mailing lists