On 01/24/24 at 11:02pm, Michael Kelley wrote: > > 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? diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c index f8163a59026b..2e8cd5a4ae85 100644 --- a/arch/x86/kernel/cpu/mshyperv.c +++ b/arch/x86/kernel/cpu/mshyperv.c @@ -209,6 +209,7 @@ static void hv_machine_shutdown(void) if (kexec_in_progress) hyperv_cleanup(); } +#endif /* CONFIG_KEXEC_CORE */ #ifdef CONFIG_CRASH_DUMP static void hv_machine_crash_shutdown(struct pt_regs *regs) @@ -222,8 +223,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 */ +#endif /* CONFIG_CRASH_DUMP */ #endif /* CONFIG_HYPERV */ static uint32_t __init ms_hyperv_platform(void) @@ -497,9 +497,11 @@ static void __init ms_hyperv_init_platform(void) no_timer_check = 1; #endif -#if IS_ENABLED(CONFIG_HYPERV) && defined(CONFIG_KEXEC_CORE) +#if IS_ENABLED(CONFIG_HYPERV) +#if defined(CONFIG_KEXEC_CORE) machine_ops.shutdown = hv_machine_shutdown; -#ifdef CONFIG_CRASH_DUMP +#endif +#if defined(CONFIG_CRASH_DUMP) machine_ops.crash_shutdown = hv_machine_crash_shutdown; #endif #endif diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c index 1287b0d5962f..f3130f762784 100644 --- a/arch/x86/kernel/reboot.c +++ b/arch/x86/kernel/reboot.c @@ -826,7 +826,7 @@ void machine_halt(void) machine_ops.halt(); } -#ifdef CONFIG_KEXEC_CORE +#ifdef CONFIG_CRASH_DUMP void machine_crash_shutdown(struct pt_regs *regs) { machine_ops.crash_shutdown(regs);