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

[Sean Christopherson <seanjc@google.com>]

[-- 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