Re: [PATCH linux-next v3 00/14] Split crash out from kexec and clean up related config items
On 01/25/24 at 09:55pm, Nathan Chancellor wrote: .. > I am seeing a few build failures in my test matrix on next-20240125 that > appear to be caused by this series although I have not bisected. Some > reproduction steps: Thanks for trying this, I have reproduced the linking failure on arm, will work out a way to fix it. It's weird, I remember I have built these and passed. > > $ curl -LSso .config > https://git.alpinelinux.org/aports/plain/community/linux-edge/config-edge.armv7 > $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- olddefconfig > all > ... > arm-linux-gnueabi-ld: arch/arm/kernel/machine_kexec.o: in function > `arch_crash_save_vmcoreinfo': > machine_kexec.c:(.text+0x488): undefined reference to `vmcoreinfo_append_str' > ... > > $ curl -LSso .config > https://github.com/archlinuxarm/PKGBUILDs/raw/master/core/linux-aarch64/config > $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux- olddefconfig all > ... > aarch64-linux-ld: kernel/kexec_file.o: in function > `kexec_walk_memblock.constprop.0': > kexec_file.c:(.text+0x314): undefined reference to `crashk_res' > aarch64-linux-ld: kernel/kexec_file.o: relocation R_AARCH64_ADR_PREL_PG_HI21 > against symbol `crashk_res' which may bind externally can not be used when > making a shared object; recompile with -fPIC > kexec_file.c:(.text+0x314): dangerous relocation: unsupported relocation > aarch64-linux-ld: kexec_file.c:(.text+0x318): undefined reference to > `crashk_res' > aarch64-linux-ld: drivers/of/kexec.o: in function > `of_kexec_alloc_and_setup_fdt': > kexec.c:(.text+0x580): undefined reference to `crashk_res' > aarch64-linux-ld: drivers/of/kexec.o: relocation R_AARCH64_ADR_PREL_PG_HI21 > against symbol `crashk_res' which may bind externally can not be used when > making a shared object; recompile with -fPIC > kexec.c:(.text+0x580): dangerous relocation: unsupported relocation > aarch64-linux-ld: kexec.c:(.text+0x584): undefined reference to `crashk_res' > aarch64-linux-ld: kexec.c:(.text+0x590): undefined reference to `crashk_res' > aarch64-linux-ld: kexec.c:(.text+0x5b0): undefined reference to > `crashk_low_res' > aarch64-linux-ld: drivers/of/kexec.o: relocation R_AARCH64_ADR_PREL_PG_HI21 > against symbol `crashk_low_res' which may bind externally can not be used > when making a shared object; recompile with -fPIC > kexec.c:(.text+0x5b0): dangerous relocation: unsupported relocation > aarch64-linux-ld: kexec.c:(.text+0x5b4): undefined reference to > `crashk_low_res' > aarch64-linux-ld: kexec.c:(.text+0x5c0): undefined reference to > `crashk_low_res' > ... > > $ curl -LSso .config > https://git.alpinelinux.org/aports/plain/community/linux-edge/config-edge.x86_64 > $ make -skj"$(nproc)" ARCH=x86_64 CROSS_COMPILE=x86_64-linux- olddefconfig all > ... > x86_64-linux-ld: arch/x86/xen/mmu_pv.o: in function `paddr_vmcoreinfo_note': > mmu_pv.c:(.text+0x3af3): undefined reference to `vmcoreinfo_note' > ... > > Cheers, > Nathan > ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH linux-next v3 00/14] Split crash out from kexec and clean up related config items
> # Kexec and crash features > CONFIG_KEXEC_CORE=y > CONFIG_KEXEC_FILE=y > # end of Kexec and crash features > > > Note: > For ppc, it needs investigation to make clear how to split out crash > code in arch folder. Hope Hari and Pingfan can help have a look, see if > it's doable. Now, I make it either have both kexec and crash enabled, or > disable both of them altogether. I am seeing a few build failures in my test matrix on next-20240125 that appear to be caused by this series although I have not bisected. Some reproduction steps: $ curl -LSso .config https://git.alpinelinux.org/aports/plain/community/linux-edge/config-edge.armv7 $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- olddefconfig all ... arm-linux-gnueabi-ld: arch/arm/kernel/machine_kexec.o: in function `arch_crash_save_vmcoreinfo': machine_kexec.c:(.text+0x488): undefined reference to `vmcoreinfo_append_str' ... $ curl -LSso .config https://github.com/archlinuxarm/PKGBUILDs/raw/master/core/linux-aarch64/config $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux- olddefconfig all ... aarch64-linux-ld: kernel/kexec_file.o: in function `kexec_walk_memblock.constprop.0': kexec_file.c:(.text+0x314): undefined reference to `crashk_res' aarch64-linux-ld: kernel/kexec_file.o: relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol `crashk_res' which may bind externally can not be used when making a shared object; recompile with -fPIC kexec_file.c:(.text+0x314): dangerous relocation: unsupported relocation aarch64-linux-ld: kexec_file.c:(.text+0x318): undefined reference to `crashk_res' aarch64-linux-ld: drivers/of/kexec.o: in function `of_kexec_alloc_and_setup_fdt': kexec.c:(.text+0x580): undefined reference to `crashk_res' aarch64-linux-ld: drivers/of/kexec.o: relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol `crashk_res' which may bind externally can not be used when making a shared object; recompile with -fPIC kexec.c:(.text+0x580): dangerous relocation: unsupported relocation aarch64-linux-ld: kexec.c:(.text+0x584): undefined reference to `crashk_res' aarch64-linux-ld: kexec.c:(.text+0x590): undefined reference to `crashk_res' aarch64-linux-ld: kexec.c:(.text+0x5b0): undefined reference to `crashk_low_res' aarch64-linux-ld: drivers/of/kexec.o: relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol `crashk_low_res' which may bind externally can not be used when making a shared object; recompile with -fPIC kexec.c:(.text+0x5b0): dangerous relocation: unsupported relocation aarch64-linux-ld: kexec.c:(.text+0x5b4): undefined reference to `crashk_low_res' aarch64-linux-ld: kexec.c:(.text+0x5c0): undefined reference to `crashk_low_res' ... $ curl -LSso .config https://git.alpinelinux.org/aports/plain/community/linux-edge/config-edge.x86_64 $ make -skj"$(nproc)" ARCH=x86_64 CROSS_COMPILE=x86_64-linux- olddefconfig all ... x86_64-linux-ld: arch/x86/xen/mmu_pv.o: in function `paddr_vmcoreinfo_note': mmu_pv.c:(.text+0x3af3): undefined reference to `vmcoreinfo_note' ... Cheers, Nathan ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH -next] kexec: Remove duplicated include in crash_reserve.c
The header files kexec.h is included twice in crash_reserve.c, so one inclusion can be removed. Signed-off-by: Yang Li --- kernel/crash_reserve.c | 1 - 1 file changed, 1 deletion(-) diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c index bbb6c3cb00e4..db8bdf47a2c0 100644 --- a/kernel/crash_reserve.c +++ b/kernel/crash_reserve.c @@ -13,7 +13,6 @@ #include #include #include -#include #include #include -- 2.20.1.7.g153144c ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 (proposal)] powerpc/cpu: enable nr_cpus for crash kernel
Hi,
Le 22/05/2018 à 10:23, Pingfan Liu a écrit :
> For kexec -p, the boot cpu can be not the cpu0, this causes the problem
> to alloc paca[]. In theory, there is no requirement to assign cpu's logical
> id as its present seq by device tree. But we have something like
> cpu_first_thread_sibling(), which makes assumption on the mapping inside
> a core. Hence partially changing the mapping, i.e. unbind the mapping of
> core while keep the mapping inside a core. After this patch, the core with
> boot-cpu will always be mapped into core 0.
>
> And at present, the code to discovery cpu spreads over two functions:
> early_init_dt_scan_cpus() and smp_setup_cpu_maps().
> This patch tries to fold smp_setup_cpu_maps() into the "previous" one
This patch is pretty old and doesn't apply anymore. If still relevant
can you please rebase and resubmit.
Thanks
Christophe
>
> Signed-off-by: Pingfan Liu
> ---
> v5 -> v6:
>simplify the loop logic (Hope it can answer Benjamin's concern)
>concentrate the cpu recovery code to early stage (Hope it can answer
> Michael's concern)
> Todo: (if this method is accepted)
>fold the whole smp_setup_cpu_maps()
>
> arch/powerpc/include/asm/smp.h | 1 +
> arch/powerpc/kernel/prom.c | 123
> -
> arch/powerpc/kernel/setup-common.c | 58 ++---
> drivers/of/fdt.c | 2 +-
> include/linux/of_fdt.h | 2 +
> 5 files changed, 103 insertions(+), 83 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
> index fac963e..80c7693 100644
> --- a/arch/powerpc/include/asm/smp.h
> +++ b/arch/powerpc/include/asm/smp.h
> @@ -30,6 +30,7 @@
> #include
>
> extern int boot_cpuid;
> +extern int threads_in_core;
> extern int spinning_secondaries;
>
> extern void cpu_die(void);
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 4922162..2ae0b4a 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -77,7 +77,6 @@ unsigned long tce_alloc_start, tce_alloc_end;
> u64 ppc64_rma_size;
> #endif
> static phys_addr_t first_memblock_size;
> -static int __initdata boot_cpu_count;
>
> static int __init early_parse_mem(char *p)
> {
> @@ -305,6 +304,14 @@ static void __init check_cpu_feature_properties(unsigned
> long node)
> }
> }
>
> +struct bootinfo {
> + int boot_thread_id;
> + unsigned int cpu_cnt;
> + int cpu_hwids[NR_CPUS];
> + bool avail[NR_CPUS];
> +};
> +static struct bootinfo *bt_info;
> +
> static int __init early_init_dt_scan_cpus(unsigned long node,
> const char *uname, int depth,
> void *data)
> @@ -312,10 +319,12 @@ static int __init early_init_dt_scan_cpus(unsigned long
> node,
> const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> const __be32 *prop;
> const __be32 *intserv;
> - int i, nthreads;
> + int i, nthreads, maxidx;
> int len;
> - int found = -1;
> - int found_thread = 0;
> + int found_thread = -1;
> + struct bootinfo *info = data;
> + bool avail;
> + int rotate_cnt, id;
>
> /* We are scanning "cpu" nodes only */
> if (type == NULL || strcmp(type, "cpu") != 0)
> @@ -325,8 +334,15 @@ static int __init early_init_dt_scan_cpus(unsigned long
> node,
> intserv = of_get_flat_dt_prop(node, "ibm,ppc-interrupt-server#s", );
> if (!intserv)
> intserv = of_get_flat_dt_prop(node, "reg", );
> + avail = of_fdt_device_is_available(initial_boot_params, node);
> +#if 0
> + //todo
> + if (!avail)
> + avail = !of_fdt_property_match_string(node,
> + "enable-method", "spin-table");
> +#endif
>
> - nthreads = len / sizeof(int);
> + threads_in_core = nthreads = len / sizeof(int);
>
> /*
>* Now see if any of these threads match our boot cpu.
> @@ -338,9 +354,10 @@ static int __init early_init_dt_scan_cpus(unsigned long
> node,
>* booted proc.
>*/
> if (fdt_version(initial_boot_params) >= 2) {
> + info->cpu_hwids[info->cpu_cnt] =
> + be32_to_cpu(intserv[i]);
> if (be32_to_cpu(intserv[i]) ==
> fdt_boot_cpuid_phys(initial_boot_params)) {
> - found = boot_cpu_count;
> found_thread = i;
> }
> } else {
> @@ -351,22 +368,37 @@ static int __init early_init_dt_scan_cpus(unsigned long
> node,
>*/
> if (of_get_flat_dt_prop(node,
> "linux,boot-cpu", NULL) != NULL)
> - found = boot_cpu_count;
> + found_thread
Re: [PATCH v4 7/7] ima: measure kexec load and exec events as critical data
On 1/24/24 06:35, Mimi Zohar wrote: On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote: The problem statement could be written as: The amount of memory allocated at kexec load, even with the extra memory allocated, might not be large enough for the entire measurement list. The indeterminate interval between kexec 'load' and 'execute' could exacerbate this problem. Mimi Thank you again for taking efforts to write one more example for me. Appreciate it. Will do. ~Tushar There could be a potential mismatch between IMA measurements and TPM PCR quotes caused by the indeterminate interval between kexec 'load' and 'execute'. The extra memory allocated at kexec 'load' for IMA log buffer may run out. It can lead to missing events in the IMA log after a soft reboot to the new Kernel, resulting in TPM PCR quotes mismatch and remote attestation failures. Define two new IMA events, 'kexec_load' and 'kexec_execute', to be measured as critical data at kexec 'load' and 'execute' respectively. Report the allocated kexec segment size, IMA binary log size and the runtime measurements count as part of those events. These events, and the values reported through them, serve as markers in the IMA log to verify the IMA events are captured during kexec soft reboot. The presence of a 'kexec_load' event in between the last two 'boot_aggregate' events in the IMA log implies this is a kexec soft reboot, and not a cold-boot. And the absence of 'kexec_execute' event after kexec soft reboot implies missing events in that window which results in inconsistency with TPM PCR quotes, necessitating a cold boot for a successful remote attestation. Signed-off-by: Tushar Sugandhi
Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
On 1/24/24 06:07, Mimi Zohar wrote:
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -121,6 +121,7 @@ void ima_add_kexec_buffer(struct kimage *image)
.buf_min = 0, .buf_max = ULONG_MAX,
.top_down = true };
unsigned long binary_runtime_size;
+ unsigned long extra_size;
/* use more understandable variable names than defined in kbuf */
void *kexec_buffer = NULL;
@@ -128,15 +129,19 @@ void ima_add_kexec_buffer(struct kimage *image)
int ret;
/*
-* Reserve an extra half page of memory for additional measurements
-* added during the kexec load.
+* Reserve extra memory for measurements added during kexec.
*/
The memory is still being allocated at kexec "load", so the extra memory is for
additional measurement records "since" kexec load.
Mimi
This wording was an attempt to address the comment in v3[1].
So I tried to make the comment generic. But maybe I made it too generic.
I will update.
[1] Re: [PATCH v3 6/7] ima: configure memory to log events between kexec
load and execute
https://lore.kernel.org/all/fbe6aa7577875b23a9913a39f858f06f1d2aa903.ca...@linux.ibm.com/
"Additional records could be added as a result of the kexec
load itself.
...
Please remove any references to measurements between kexec load and
execute."
~Tushar
- binary_runtime_size = ima_get_binary_runtime_size();
+ if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0)
+ extra_size = PAGE_SIZE / 2;
+ else
+ extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
+ binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
+
if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE)
kexec_segment_size = ULONG_MAX;
else
- kexec_segment_size = ALIGN(ima_get_binary_runtime_size() +
- PAGE_SIZE / 2, PAGE_SIZE);
+ kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE);
+
if ((kexec_segment_size == ULONG_MAX) ||
((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
pr_err("Binary measurement list too large.\n");
Re: [PATCH v4 4/7] ima: kexec: move ima log copy from kexec load to execute
On 1/24/24 08:11, Mimi Zohar wrote: On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote: ima_dump_measurement_list() is called during kexec 'load', which may result in loss of IMA measurements during kexec soft reboot. It needs to be called during kexec 'execute'. The below changes need to be part of the same patch to ensure this patch series remains bisect-safe by ensuring the IMA log gets copied over during kexec soft reboot both before and after this patch. Implement ima_update_kexec_buffer() to be called during kexec 'execute'. Move ima_dump_measurement_list() from ima_add_kexec_buffer() to ima_update_kexec_buffer(). Make the necessary variables local static to the file, so that they are accessible during both kexec 'load' - where the memory is allocated and mapped to a segment in the new Kernel, and during 'execute' - where the IMA log gets copied over. Implement kimage_file_post_load() and ima_kexec_post_load() to be invoked after the new Kernel image has been loaded for kexec. ima_kexec_post_load() will map the IMA buffer to a segment in the newly loaded Kernel. It will also register the reboot notifier_block to trigger ima_update_kexec_buffer() at exec 'execute'. This defines two new IMA hooks - ima_kexec_post_load() and ima_update_kexec_buffer(). They shouldn't be hidden here in the move of copying the measurement list from kexec load to execute. If "ima_update_kexec_buffer()" was initially defined as a stub function, the infrastructure could be set up ahead of time. This patch could then be limited to just moving the copy from kexec "load" to "execute", by replacing the stub function with the real function. Agreed. Making ima_kexec_post_load() and ima_update_kexec_buffer() as stubs/hooks did cross my mind. Thanks for confirming that. I will split this patch (4/7) into two. First will define the stubs, setup the infrastructure. And second will move the copy from 'load' to 'execute'. ~Tushar Modify kexec_file_load() syscall to call kimage_file_post_load() after the image has been loaded and prepared for kexec. Call it only on kexec soft reboot and not for KEXEC_FILE_ON_CRASH.
Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
Thanks Mimi. On 1/24/24 05:33, Mimi Zohar wrote: Hi Tushar, On Mon, 2024-01-22 at 10:37 -0800, Tushar Sugandhi wrote: Missing from this and the other patch descriptions is the problem description. Please refer to the section titled "Describe your changes" in https://docs.kernel.org/process/submitting-patches.html. "Describe your problem. Whether your patch is a one-line bug fix or 5000 lines of a new feature, there must be an underlying problem that motivated you to do this work. Convince the reviewer that there is a problem worth fixing and that it makes sense for them to read past the first paragraph." In this case, "why" you need to refactor ima_dump_measurement_list() is the problem. Thanks. I will revisit all the patch descriptions in this series to take into account the 'why' specific to that particular patch. For example: Carrying the IMA measurement list across kexec requires allocating a buffer and copying the measurement records. Separate allocating the buffer and copying the measurement records into separate functions in order to allocate the buffer at kexec "load" and copy the measurements at kexec "execute". Appreciate you giving an example in this case. I will try to follow it in other patches too. "Once the problem is established, describe what you are actually doing about it in technical detail. It's important to describe the change in plain English for the reviewer to verify that the code is behaving as you intend it to." Refactor ima_dump_measurement_list() to move the memory allocation part to a separate function ima_alloc_kexec_file_buf() which allocates buffer of size 'kexec_segment_size' at kexec 'load'. Make the local variable ima_kexec_file in function ima_dump_measurement_list() as local static to the file, so that it can be accessed from ima_alloc_kexec_file_buf(). Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions. Please make this into an unordered list. Will do. Thanks again. ~Tushar
Re: [PATCH] kexec: don't use kexec_file_load on XEN
On Tue, Jan 16, 2024 at 06:14:31PM +0100, Jiri Bohac wrote:
> Since commit 29fe5067ed07 ("kexec: make -a the default")
> kexec tries the kexec_file_load syscall first and only falls back to
> kexec_load on
> selected error codes.
>
> This effectively breaks kexec on XEN, unless -c is pecified to force the
> kexec_load
> syscall.
>
> The XEN-specific functions (xen_kexec_load / xen_kexec_unload) are only called
> from my_load / k_unload, i.e. the kexec_load code path.
>
> With -p (panic kernel) kexec_file_load on XEN fails with -EADDRNOTAVAIL (crash
> kernel reservation is ignored by the kernel on XEN), which is not in the list
> of return codes that cause the fallback to kexec_file.
>
> Without -p kexec_file_load actualy leads to a kernel oops on v6.4.0
> (needs to be dubugged separately).
>
> Signed-off-by: Jiri Bohac
> Fixes: 29fe5067ed07 ("kexec: make -a the default")
Thanks Jiri,
and sorry for the delay.
Applied to main.
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
RE: [PATCH linux-next v3 06/14] x86, crash: wrap crash dumping code into crash related ifdefs
From: Baoquan He Sent: Thursday, January 25, 2024 1:17 AM
>
> On 01/25/24 at 05:12am, Michael Kelley wrote:
> > From: Baoquan He 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
>
Re: [PATCH linux-next v3 06/14] x86, crash: wrap crash dumping code into crash related ifdefs
On 01/25/24 at 05:12am, Michael Kelley wrote:
> From: Baoquan He 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)
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
