| Message ID | 20240124051254.67105-7-bhe@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Split crash out from kexec and clean up related config items | expand |
From: Baoquan He <bhe@redhat.com> 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@redhat.com> > --- > 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 >
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);
From: Baoquan He <bhe@redhat.com> Sent: Wednesday, January 24, 2024 8:10 PM > > 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? 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 > > 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);
On 01/25/24 at 05:12am, Michael Kelley wrote: > From: Baoquan He <bhe@redhat.com> Sent: Wednesday, January 24, 2024 8:10 PM > > > > 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? > > 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". Right. I checked them all in arch/x86 this time, hope nothing is missed again. I can post a v4 to update this x86 patch later if no other concern. Thanks. 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); diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c index 09e3db7ff990..0b367c1e086d 100644 --- a/arch/x86/xen/enlighten_hvm.c +++ b/arch/x86/xen/enlighten_hvm.c @@ -148,6 +148,7 @@ static void xen_hvm_shutdown(void) if (kexec_in_progress) xen_reboot(SHUTDOWN_soft_reset); } +#endif #ifdef CONFIG_CRASH_DUMP static void xen_hvm_crash_shutdown(struct pt_regs *regs) @@ -156,7 +157,6 @@ static void xen_hvm_crash_shutdown(struct pt_regs *regs) xen_reboot(SHUTDOWN_soft_reset); } #endif -#endif static int xen_cpu_up_prepare_hvm(unsigned int cpu) { @@ -238,10 +238,10 @@ static void __init xen_hvm_guest_init(void) #ifdef CONFIG_KEXEC_CORE machine_ops.shutdown = xen_hvm_shutdown; +#endif #ifdef CONFIG_CRASH_DUMP machine_ops.crash_shutdown = xen_hvm_crash_shutdown; #endif -#endif } static __init int xen_parse_nopv(char *arg)
From: Baoquan He <bhe@redhat.com> Sent: Thursday, January 25, 2024 1:17 AM > > On 01/25/24 at 05:12am, Michael Kelley wrote: > > From: Baoquan He <bhe@redhat.com> Sent: Wednesday, January 24, 2024 > 8:10 PM > > > > > > 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? > > > > 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". > > Right. I checked them all in arch/x86 this time, hope nothing is missed > again. I can post a v4 to update this x86 patch later if no other > concern. Thanks. Yes -- everything looks good to me now. Michael > > 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); > diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c > index 09e3db7ff990..0b367c1e086d 100644 > --- a/arch/x86/xen/enlighten_hvm.c > +++ b/arch/x86/xen/enlighten_hvm.c > @@ -148,6 +148,7 @@ static void xen_hvm_shutdown(void) > if (kexec_in_progress) > xen_reboot(SHUTDOWN_soft_reset); > } > +#endif > > #ifdef CONFIG_CRASH_DUMP > static void xen_hvm_crash_shutdown(struct pt_regs *regs) > @@ -156,7 +157,6 @@ static void xen_hvm_crash_shutdown(struct pt_regs > *regs) > xen_reboot(SHUTDOWN_soft_reset); > } > #endif > -#endif > > static int xen_cpu_up_prepare_hvm(unsigned int cpu) > { > @@ -238,10 +238,10 @@ static void __init xen_hvm_guest_init(void) > > #ifdef CONFIG_KEXEC_CORE > machine_ops.shutdown = xen_hvm_shutdown; > +#endif > #ifdef CONFIG_CRASH_DUMP > machine_ops.crash_shutdown = xen_hvm_crash_shutdown; > #endif > -#endif > } > > static __init int xen_parse_nopv(char *arg)
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 */ #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 }; 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)
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@redhat.com> --- 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(-)