From: Baoquan He <bhe@redhat.com> Sent: Wednesday, January 24, 2024 8:10 PM

On 01/24/24 at 11:02pm, Michael Kelley wrote:

quoted

quoted

diff --git a/arch/x86/kernel/cpu/mshyperv.c

b/arch/x86/kernel/cpu/mshyperv.c
index 01fa06dd06b6..f8163a59026b 100644

--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c

@@ -210,6 +210,7 @@ static void hv_machine_shutdown(void)
 		hyperv_cleanup();
 }

+#ifdef CONFIG_CRASH_DUMP
 static void hv_machine_crash_shutdown(struct pt_regs *regs)
 {
 	if (hv_crash_handler)

@@ -221,6 +222,7 @@ static void hv_machine_crash_shutdown(struct pt_regs *regs)
 	/* Disable the hypercall page when there is only 1 active CPU. */
 	hyperv_cleanup();
 }
+#endif
 #endif /* CONFIG_KEXEC_CORE */

Note that the #ifdef CONFIG_CRASH_DUMP is nested inside
#ifdef CONFIG_KEXEC_CODE here, and in the other Hyper-V code
just below.   It's also nested in xen_hvm_guest_init() at the bottom
of this patch.  But the KVM case of setting crash_shutdown is
not nested -- you changed #ifdef CONFIG_KEXEC_CORE to #ifdef
CONFIG_CRASH_DUMP.

I think both approaches work because CONFIG_CRASH_DUMP implies
CONFIG_KEXEC_CORE, but I wonder if it would be better to *not* nest
in all cases.  I'd like to see the cases be consistent so in the future
someone doesn't wonder why there's a difference (unless there's
a reason for the difference that I missed).

I agree with you, it's a great suggestion. Thanks.

Do you think below draft patch includes all changes you are concerned
about?

Yes, these changes look good as a delta to your original patch.

But also look at xen_hvm_guest_init().  It looks like your original patch
does nesting there as well, and it could probably be "un-nested".

Michael