[PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Andy Shevchenko]

The nmi_shootdown_cpus_on_restart() in some cases may be not used.
This, in particular, prevents kernel builds with clang, `make W=1`
and CONFIG_WERROR=y:

arch/x86/kernel/reboot.c:957:20: error: unused function 'nmi_shootdown_cpus_on_restart' [-Werror,-Wunused-function]
  957 | static inline void nmi_shootdown_cpus_on_restart(void)
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fix this by guarging the definitions with the respective KVM ifdeffery.

See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
inline functions for W=1 build").

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v1: suggested by Dave Hansen
v2: rebased on top of the latest changes in the file
 arch/x86/kernel/reboot.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 615922838c51..67551c474203 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -528,9 +528,9 @@ static inline void kb_wait(void)
 	}
 }
 
+#if IS_ENABLED(CONFIG_KVM_X86)
 static inline void nmi_shootdown_cpus_on_restart(void);
 
-#if IS_ENABLED(CONFIG_KVM_X86)
 /* RCU-protected callback to disable virtualization prior to reboot. */
 static cpu_emergency_virt_cb __rcu *cpu_emergency_virt_callback;
 
@@ -954,12 +954,6 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 	 */
 }
 
-static inline void nmi_shootdown_cpus_on_restart(void)
-{
-	if (!crash_ipi_issued)
-		nmi_shootdown_cpus(NULL);
-}
-
 /*
  * Check if the crash dumping IPI got issued and if so, call its callback
  * directly. This function is used when we have already been in NMI handler.
@@ -987,9 +981,19 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 	/* No other CPUs to shoot down */
 }
 
-static inline void nmi_shootdown_cpus_on_restart(void) { }
-
 void run_crash_ipi_callback(struct pt_regs *regs)
 {
 }
 #endif
+
+#if IS_ENABLED(CONFIG_KVM_X86)
+#if defined(CONFIG_SMP)
+static inline void nmi_shootdown_cpus_on_restart(void)
+{
+	if (!crash_ipi_issued)
+		nmi_shootdown_cpus(NULL);
+}
+#else /* !CONFIG_SMP */
+static inline void nmi_shootdown_cpus_on_restart(void) { }
+#endif /* !CONFIG_SMP */
+#endif
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Sean Christopherson]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

On Tue, Oct 08, 2024, Andy Shevchenko wrote:
> The nmi_shootdown_cpus_on_restart() in some cases may be not used.
> This, in particular, prevents kernel builds with clang, `make W=1`
> and CONFIG_WERROR=y:
> 
> arch/x86/kernel/reboot.c:957:20: error: unused function 'nmi_shootdown_cpus_on_restart' [-Werror,-Wunused-function]
>   957 | static inline void nmi_shootdown_cpus_on_restart(void)
>       |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fix this by guarging the definitions with the respective KVM ifdeffery.
> 
> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> inline functions for W=1 build").
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v1: suggested by Dave Hansen
> v2: rebased on top of the latest changes in the file
>  arch/x86/kernel/reboot.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)


Heh, I tried fixing this too, and have patches to clean things up, but I ended up
not posting them because I decided the W=1 warning was less ugly than the resulting
code in nmi_shootdown_cpus().

If we're willing to take on a bit of weirdness in nmi_shootdown_cpus(), then much
of the #ifdeffery can go away.  Patches attached (lightly tested).

$ git diff --stat next..HEAD
 arch/x86/kernel/reboot.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

[-- Attachment #2: 0001-x86-reboot-Allow-blindly-calling-nmi_shootdown_cpus-.patch --]
[-- Type: text/x-diff, Size: 2084 bytes --]

From f91d4f1cc04d7955587aac3f919fd6696a648f5f Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 3 Sep 2024 10:14:27 -0700
Subject: [PATCH 1/3] x86/reboot: Allow "blindly" calling nmi_shootdown_cpus()
 without a callback

Warn if nmi_shootdown_cpus() is called after the crash IPI has been issued
if and only if the caller wants to run a specific callback, and drop the
check nmi_shootdown_cpus_on_restart() whose sole purpose was to avoid
triggering the warning.  This will allow removing the "on restart" variant.

Note, the only caller of nmi_shootdown_cpus_on_restart() unconditionally
disables IRQs, i.e. doubling down on disabling IRQs when a crash IPI has
already been issued doesn't affect the resulting functionality.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kernel/reboot.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 615922838c51..25f68952af57 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -915,10 +915,14 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 	/*
 	 * Avoid certain doom if a shootdown already occurred; re-registering
 	 * the NMI handler will cause list corruption, modifying the callback
-	 * will do who knows what, etc...
+	 * will do who knows what, etc...  Blindly attempting a shootdown is
+	 * allowed if the caller's goal is purely to ensure a shootdown occurs,
+	 * i.e. if the caller doesn't want to run a specific callback.
 	 */
-	if (WARN_ON_ONCE(crash_ipi_issued))
+	if (crash_ipi_issued) {
+		WARN_ON_ONCE(callback);
 		return;
+	}
 
 	/* Make a note of crashing cpu. Will be used in NMI callback. */
 	crashing_cpu = safe_smp_processor_id();
@@ -956,8 +960,7 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 
 static inline void nmi_shootdown_cpus_on_restart(void)
 {
-	if (!crash_ipi_issued)
-		nmi_shootdown_cpus(NULL);
+	nmi_shootdown_cpus(NULL);
 }
 
 /*

base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
-- 
2.47.0.rc1.288.g06298d1525-goog


[-- Attachment #3: 0002-x86-reboot-Open-code-nmi_shootdown_cpus_on_restart-i.patch --]
[-- Type: text/x-diff, Size: 2117 bytes --]

From 5d417af0c51adc11ed0de6a30aa1ddfb2ccba875 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 5 Sep 2024 11:36:45 -0700
Subject: [PATCH 2/3] x86/reboot: Open code nmi_shootdown_cpus_on_restart()
 into its sole user

To fix a W=1 warning due to nmi_shootdown_cpus_on_restart() being unused
if KVM support is disabled, fold nmi_shootdown_cpus_on_restart() into its
one and only user, emergency_reboot_disable_virtualization().

No functional change intented.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202409010207.jrH6sNV4-lkp@intel.com
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kernel/reboot.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 25f68952af57..8c6f6da6ee8e 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -528,8 +528,6 @@ static inline void kb_wait(void)
 	}
 }
 
-static inline void nmi_shootdown_cpus_on_restart(void);
-
 #if IS_ENABLED(CONFIG_KVM_X86)
 /* RCU-protected callback to disable virtualization prior to reboot. */
 static cpu_emergency_virt_cb __rcu *cpu_emergency_virt_callback;
@@ -595,7 +593,8 @@ static void emergency_reboot_disable_virtualization(void)
 		cpu_emergency_disable_virtualization();
 
 		/* Disable VMX/SVM and halt on other CPUs. */
-		nmi_shootdown_cpus_on_restart();
+		if (IS_ENABLED(CONFIG_SMP))
+			nmi_shootdown_cpus(NULL);
 	}
 }
 #else
@@ -958,11 +957,6 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 	 */
 }
 
-static inline void nmi_shootdown_cpus_on_restart(void)
-{
-	nmi_shootdown_cpus(NULL);
-}
-
 /*
  * Check if the crash dumping IPI got issued and if so, call its callback
  * directly. This function is used when we have already been in NMI handler.
@@ -990,8 +984,6 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 	/* No other CPUs to shoot down */
 }
 
-static inline void nmi_shootdown_cpus_on_restart(void) { }
-
 void run_crash_ipi_callback(struct pt_regs *regs)
 {
 }
-- 
2.47.0.rc1.288.g06298d1525-goog


[-- Attachment #4: 0003-x86-reboot-Delete-CONFIG_SMP-n-stub-for-nmi_shootdow.patch --]
[-- Type: text/x-diff, Size: 1209 bytes --]

From dfbd634a1acafb969794fff9ed027bedcfd187ef Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 3 Sep 2024 10:21:49 -0700
Subject: [PATCH 3/3] x86/reboot: Delete CONFIG_SMP=n stub for
 nmi_shootdown_cpus()

Remove the CONFIG_SMP=n implementation of nmi_shootdown_cpus() as all
callers invoke nmi_shootdown_cpus() if and only if CONFIG_SMP=y.  Keep
the unguarded declaration to play nice with using IS_ENABLED(CONFIG_SMP)
in if-statements (to avoid #ifdefs); thanks to dead code elimination, all
supported compilers will drop the call before linking.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kernel/reboot.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 8c6f6da6ee8e..2c9299394a22 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -979,11 +979,6 @@ void __noreturn nmi_panic_self_stop(struct pt_regs *regs)
 }
 
 #else /* !CONFIG_SMP */
-void nmi_shootdown_cpus(nmi_shootdown_cb callback)
-{
-	/* No other CPUs to shoot down */
-}
-
 void run_crash_ipi_callback(struct pt_regs *regs)
 {
 }
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Andy Shevchenko]

Wed, Oct 09, 2024 at 12:34:37PM -0700, Sean Christopherson kirjoitti:
> On Tue, Oct 08, 2024, Andy Shevchenko wrote:
> > The nmi_shootdown_cpus_on_restart() in some cases may be not used.
> > This, in particular, prevents kernel builds with clang, `make W=1`
> > and CONFIG_WERROR=y:
> > 
> > arch/x86/kernel/reboot.c:957:20: error: unused function 'nmi_shootdown_cpus_on_restart' [-Werror,-Wunused-function]
> >   957 | static inline void nmi_shootdown_cpus_on_restart(void)
> >       |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Fix this by guarging the definitions with the respective KVM ifdeffery.
> > 
> > See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> > inline functions for W=1 build").

> Heh, I tried fixing this too, and have patches to clean things up, but I ended up
> not posting them because I decided the W=1 warning was less ugly than the resulting
> code in nmi_shootdown_cpus().

CONFIG_WERROR=y is the default (at least in the current defconfigs for x86).
My goal is to match what ARM builds survive (to some extend?), i.e. W=1, so
may one apply either version I provided or yours?

> If we're willing to take on a bit of weirdness in nmi_shootdown_cpus(), then much
> of the #ifdeffery can go away.  Patches attached (lightly tested).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 02:37:59AM +0300, Andy Shevchenko wrote:
> Wed, Oct 09, 2024 at 12:34:37PM -0700, Sean Christopherson kirjoitti:
> > On Tue, Oct 08, 2024, Andy Shevchenko wrote:
> > > The nmi_shootdown_cpus_on_restart() in some cases may be not used.
> > > This, in particular, prevents kernel builds with clang, `make W=1`
> > > and CONFIG_WERROR=y:
> > > 
> > > arch/x86/kernel/reboot.c:957:20: error: unused function 'nmi_shootdown_cpus_on_restart' [-Werror,-Wunused-function]
> > >   957 | static inline void nmi_shootdown_cpus_on_restart(void)
> > >       |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > Fix this by guarging the definitions with the respective KVM ifdeffery.
> > > 
> > > See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> > > inline functions for W=1 build").
> 
> > Heh, I tried fixing this too, and have patches to clean things up, but I ended up
> > not posting them because I decided the W=1 warning was less ugly than the resulting
> > code in nmi_shootdown_cpus().
> 
> CONFIG_WERROR=y is the default (at least in the current defconfigs for x86).
> My goal is to match what ARM builds survive (to some extend?), i.e. W=1, so
> may one apply either version I provided or yours?
> 
> > If we're willing to take on a bit of weirdness in nmi_shootdown_cpus(), then much
> > of the #ifdeffery can go away.  Patches attached (lightly tested).

So, what's the status either with the proposed change or with what Sean shared?
Can we somehow move forward to fix the issue?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Andy Shevchenko]

On Mon, Nov 04, 2024 at 09:08:45PM +0200, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 02:37:59AM +0300, Andy Shevchenko wrote:
> > Wed, Oct 09, 2024 at 12:34:37PM -0700, Sean Christopherson kirjoitti:
> > > On Tue, Oct 08, 2024, Andy Shevchenko wrote:
> > > > The nmi_shootdown_cpus_on_restart() in some cases may be not used.
> > > > This, in particular, prevents kernel builds with clang, `make W=1`
> > > > and CONFIG_WERROR=y:
> > > > 
> > > > arch/x86/kernel/reboot.c:957:20: error: unused function 'nmi_shootdown_cpus_on_restart' [-Werror,-Wunused-function]
> > > >   957 | static inline void nmi_shootdown_cpus_on_restart(void)
> > > >       |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > 
> > > > Fix this by guarging the definitions with the respective KVM ifdeffery.
> > > > 
> > > > See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> > > > inline functions for W=1 build").
> > 
> > > Heh, I tried fixing this too, and have patches to clean things up, but I ended up
> > > not posting them because I decided the W=1 warning was less ugly than the resulting
> > > code in nmi_shootdown_cpus().
> > 
> > CONFIG_WERROR=y is the default (at least in the current defconfigs for x86).
> > My goal is to match what ARM builds survive (to some extend?), i.e. W=1, so
> > may one apply either version I provided or yours?
> > 
> > > If we're willing to take on a bit of weirdness in nmi_shootdown_cpus(), then much
> > > of the #ifdeffery can go away.  Patches attached (lightly tested).
> 
> So, what's the status either with the proposed change or with what Sean shared?
> Can we somehow move forward to fix the issue?

Any news here? Build error is still reproducible.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Dave Hansen]

On 4/8/25 05:19, Andy Shevchenko wrote:
> Any news here? Build error is still reproducible.

In the end, adding the #ifdefs leads to worse code, less maintainable
code. I'll take the occasional actual unused static inline in a .c file
over a mess of #ifdefs to make the compiler happy.

I really think that warning needs to go away in some way, shape or form.
Either get rid of it entirely, or kick it out of -Wall somehow. It's a
super pedantic warning that leads to worse code most of the time.

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Andy Shevchenko]

On Tue, Apr 08, 2025 at 07:17:51AM -0700, Dave Hansen wrote:
> On 4/8/25 05:19, Andy Shevchenko wrote:
> > Any news here? Build error is still reproducible.
> 
> In the end, adding the #ifdefs leads to worse code, less maintainable
> code. I'll take the occasional actual unused static inline in a .c file
> over a mess of #ifdefs to make the compiler happy.
> 
> I really think that warning needs to go away in some way, shape or form.
> Either get rid of it entirely, or kick it out of -Wall somehow. It's a
> super pedantic warning that leads to worse code most of the time.

Does it mean you can take Sean's approach?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Dave Hansen]

On 4/8/25 07:36, Andy Shevchenko wrote:
> On Tue, Apr 08, 2025 at 07:17:51AM -0700, Dave Hansen wrote:
>> On 4/8/25 05:19, Andy Shevchenko wrote:
>>> Any news here? Build error is still reproducible.
>> In the end, adding the #ifdefs leads to worse code, less maintainable
>> code. I'll take the occasional actual unused static inline in a .c file
>> over a mess of #ifdefs to make the compiler happy.
>>
>> I really think that warning needs to go away in some way, shape or form.
>> Either get rid of it entirely, or kick it out of -Wall somehow. It's a
>> super pedantic warning that leads to worse code most of the time.
> Does it mean you can take Sean's approach?

It looks fine at first glance.

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Andy Shevchenko]

On Tue, Apr 08, 2025 at 07:56:58AM -0700, Dave Hansen wrote:
> On 4/8/25 07:36, Andy Shevchenko wrote:
> > On Tue, Apr 08, 2025 at 07:17:51AM -0700, Dave Hansen wrote:
> >> On 4/8/25 05:19, Andy Shevchenko wrote:
> >>> Any news here? Build error is still reproducible.
> >> In the end, adding the #ifdefs leads to worse code, less maintainable
> >> code. I'll take the occasional actual unused static inline in a .c file
> >> over a mess of #ifdefs to make the compiler happy.
> >>
> >> I really think that warning needs to go away in some way, shape or form.
> >> Either get rid of it entirely, or kick it out of -Wall somehow. It's a
> >> super pedantic warning that leads to worse code most of the time.
> > Does it mean you can take Sean's approach?
> 
> It looks fine at first glance.

Can it be applied, please? The problem still persists as of today (v6.15-rc4).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Dave Hansen]

On 5/2/25 07:04, Andy Shevchenko wrote:
> Can it be applied, please? The problem still persists as of today (v6.15-rc4).

I fundamentally disagree with the idea that the kernel programmer should
be doing the work of telling the compiler *exactly* when a static inline
function is unused. Compilers are good at that, humans are not.

The "fixes" for these issues generally make the code worse, not better.

I'd frankly rather have a kernel with some unused 'static inline'
functions in .c files than one filled with #ifdefs to shut up the compiler.

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[Peter Zijlstra]

On Fri, May 02, 2025 at 07:24:37AM -0700, Dave Hansen wrote:
> On 5/2/25 07:04, Andy Shevchenko wrote:
> > Can it be applied, please? The problem still persists as of today (v6.15-rc4).
> 
> I fundamentally disagree with the idea that the kernel programmer should
> be doing the work of telling the compiler *exactly* when a static inline
> function is unused. Compilers are good at that, humans are not.
> 
> The "fixes" for these issues generally make the code worse, not better.
> 
> I'd frankly rather have a kernel with some unused 'static inline'
> functions in .c files than one filled with #ifdefs to shut up the compiler.

Yeah, this unused function warning nonsense is terribly annoying :/

Re: [PATCH v2 1/1] x86/reboot: KVM: Guard nmi_shootdown_cpus_on_restart() with ifdeffery

[H. Peter Anvin]

On May 2, 2025 7:24:37 AM PDT, Dave Hansen <dave.hansen@intel.com> wrote:
>On 5/2/25 07:04, Andy Shevchenko wrote:
>> Can it be applied, please? The problem still persists as of today (v6.15-rc4).
>
>I fundamentally disagree with the idea that the kernel programmer should
>be doing the work of telling the compiler *exactly* when a static inline
>function is unused. Compilers are good at that, humans are not.
>
>The "fixes" for these issues generally make the code worse, not better.
>
>I'd frankly rather have a kernel with some unused 'static inline'
>functions in .c files than one filled with #ifdefs to shut up the compiler.
>

__attribute__((unused)) if they don't need to be inlined?