RE: [PATCH linux-next v3 06/14] x86, crash: wrap crash dumping code into crash related ifdefs
From: Michael Kelley
Date: Wed Jan 24 2024 - 18:03:08 EST
From: Baoquan He <bhe@xxxxxxxxxx> Sent: Tuesday, January 23, 2024 9:13 PM
>
> Now crash codes under kernel/ folder has been split out from kexec
> code, crash dumping can be separated from kexec reboot in config
> items on x86 with some adjustments.
>
> Here, also change some ifdefs or IS_ENABLED() check to more appropriate
> ones, e,g
> - #ifdef CONFIG_KEXEC_CORE -> #ifdef CONFIG_CRASH_DUMP
> - (!IS_ENABLED(CONFIG_KEXEC_CORE)) - >
> (!IS_ENABLED(CONFIG_CRASH_RESERVE))
>
> Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
> ---
> arch/x86/kernel/Makefile | 4 ++--
> arch/x86/kernel/cpu/mshyperv.c | 4 ++++
> arch/x86/kernel/kexec-bzimage64.c | 4 ++++
> arch/x86/kernel/kvm.c | 4 ++--
> arch/x86/kernel/machine_kexec_64.c | 3 +++
> arch/x86/kernel/reboot.c | 2 +-
> arch/x86/kernel/setup.c | 2 +-
> arch/x86/kernel/smp.c | 2 +-
> arch/x86/xen/enlighten_hvm.c | 4 ++++
> 9 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 913d4022131e..3668b1edef2d 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -100,9 +100,9 @@ obj-$(CONFIG_TRACING) += trace.o
> obj-$(CONFIG_RETHOOK) += rethook.o
> obj-$(CONFIG_VMCORE_INFO) += vmcore_info_$(BITS).o
> obj-$(CONFIG_KEXEC_CORE) += machine_kexec_$(BITS).o
> -obj-$(CONFIG_KEXEC_CORE) += relocate_kernel_$(BITS).o crash.o
> +obj-$(CONFIG_KEXEC_CORE) += relocate_kernel_$(BITS).o
> obj-$(CONFIG_KEXEC_FILE) += kexec-bzimage64.o
> -obj-$(CONFIG_CRASH_DUMP) += crash_dump_$(BITS).o
> +obj-$(CONFIG_CRASH_DUMP) += crash_dump_$(BITS).o crash.o
> obj-y += kprobes/
> obj-$(CONFIG_MODULES) += module.o
> obj-$(CONFIG_X86_32) += doublefault_32.o
> 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).
> #endif /* CONFIG_HYPERV */
>
> @@ -497,7 +499,9 @@ static void __init ms_hyperv_init_platform(void)
>
> #if IS_ENABLED(CONFIG_HYPERV) && defined(CONFIG_KEXEC_CORE)
> machine_ops.shutdown = hv_machine_shutdown;
> +#ifdef CONFIG_CRASH_DUMP
> machine_ops.crash_shutdown = hv_machine_crash_shutdown;
> +#endif
> #endif
> if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT) {
> /*
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-
> bzimage64.c
> index 2a422e00ed4b..b55737b83a84 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -263,11 +263,13 @@ setup_boot_parameters(struct kimage *image,
> struct boot_params *params,
> memset(¶ms->hd0_info, 0, sizeof(params->hd0_info));
> memset(¶ms->hd1_info, 0, sizeof(params->hd1_info));
>
> +#ifdef CONFIG_CRASH_DUMP
> if (image->type == KEXEC_TYPE_CRASH) {
> ret = crash_setup_memmap_entries(image, params);
> if (ret)
> return ret;
> } else
> +#endif
> setup_e820_entries(params);
>
> nr_e820_entries = params->e820_entries;
> @@ -433,12 +435,14 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
> return ERR_PTR(-EINVAL);
> }
>
> +#ifdef CONFIG_CRASH_DUMP
> /* Allocate and load backup region */
> if (image->type == KEXEC_TYPE_CRASH) {
> ret = crash_load_segments(image);
> if (ret)
> return ERR_PTR(ret);
> }
> +#endif
>
> /*
> * Load purgatory. For 64bit entry point, purgatory code can be
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index dfe9945b9bec..acfc2d3183bc 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -769,7 +769,7 @@ static struct notifier_block kvm_pv_reboot_nb = {
> * won't be valid. In cases like kexec, in which you install a new kernel, this
> * means a random memory location will be kept being written.
> */
> -#ifdef CONFIG_KEXEC_CORE
> +#ifdef CONFIG_CRASH_DUMP
> static void kvm_crash_shutdown(struct pt_regs *regs)
> {
> kvm_guest_cpu_offline(true);
> @@ -852,7 +852,7 @@ static void __init kvm_guest_init(void)
> kvm_guest_cpu_init();
> #endif
>
> -#ifdef CONFIG_KEXEC_CORE
> +#ifdef CONFIG_CRASH_DUMP
> machine_ops.crash_shutdown = kvm_crash_shutdown;
> #endif
>
> diff --git a/arch/x86/kernel/machine_kexec_64.c
> b/arch/x86/kernel/machine_kexec_64.c
> index bc0a5348b4a6..b180d8e497c3 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -508,6 +508,8 @@ int arch_kimage_file_post_load_cleanup(struct
> kimage *image)
> }
> #endif /* CONFIG_KEXEC_FILE */
>
> +#ifdef CONFIG_CRASH_DUMP
> +
> static int
> kexec_mark_range(unsigned long start, unsigned long end, bool protect)
> {
> @@ -552,6 +554,7 @@ void arch_kexec_unprotect_crashkres(void)
> {
> kexec_mark_crashkres(false);
> }
> +#endif
>
> /*
> * During a traditional boot under SME, SME will encrypt the kernel,
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 830425e6d38e..1287b0d5962f 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -796,7 +796,7 @@ struct machine_ops machine_ops __ro_after_init = {
> .emergency_restart = native_machine_emergency_restart,
> .restart = native_machine_restart,
> .halt = native_machine_halt,
> -#ifdef CONFIG_KEXEC_CORE
> +#ifdef CONFIG_CRASH_DUMP
> .crash_shutdown = native_machine_crash_shutdown,
> #endif
> };
Also in arch/x86/kernel/reboot.c, should the function
machine_crash_shutdown() be updated with
#ifdef CONFIG_CRASH_SHUTDOWN instead of #ifdef
CONFIG_KEXEC_CORE?
Michael
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 84201071dfac..899d839a2954 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -471,7 +471,7 @@ static void __init arch_reserve_crashkernel(void)
> bool high = false;
> int ret;
>
> - if (!IS_ENABLED(CONFIG_KEXEC_CORE))
> + if (!IS_ENABLED(CONFIG_CRASH_RESERVE))
> return;
>
> ret = parse_crashkernel(cmdline, memblock_phys_mem_size(),
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index 2908e063d7d8..18266cc3d98c 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -286,7 +286,7 @@ struct smp_ops smp_ops = {
> .smp_cpus_done = native_smp_cpus_done,
>
> .stop_other_cpus = native_stop_other_cpus,
> -#if defined(CONFIG_KEXEC_CORE)
> +#if defined(CONFIG_CRASH_DUMP)
> .crash_stop_other_cpus = kdump_nmi_shootdown_cpus,
> #endif
> .smp_send_reschedule = native_smp_send_reschedule,
> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
> index 3f8c34707c50..09e3db7ff990 100644
> --- a/arch/x86/xen/enlighten_hvm.c
> +++ b/arch/x86/xen/enlighten_hvm.c
> @@ -149,12 +149,14 @@ static void xen_hvm_shutdown(void)
> xen_reboot(SHUTDOWN_soft_reset);
> }
>
> +#ifdef CONFIG_CRASH_DUMP
> static void xen_hvm_crash_shutdown(struct pt_regs *regs)
> {
> native_machine_crash_shutdown(regs);
> xen_reboot(SHUTDOWN_soft_reset);
> }
> #endif
> +#endif
>
> static int xen_cpu_up_prepare_hvm(unsigned int cpu)
> {
> @@ -236,8 +238,10 @@ static void __init xen_hvm_guest_init(void)
>
> #ifdef CONFIG_KEXEC_CORE
> machine_ops.shutdown = xen_hvm_shutdown;
> +#ifdef CONFIG_CRASH_DUMP
> machine_ops.crash_shutdown = xen_hvm_crash_shutdown;
> #endif
> +#endif
> }
>
> static __init int xen_parse_nopv(char *arg)
> --
> 2.41.0
>