[PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Eric DeVolder]

When the kdump service is loaded, if a CPU or memory is hot
un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
and memory in the system, must also be updated, else the resulting
vmcore is inaccurate (eg. missing either CPU context or memory
regions).

The current solution utilizes udev to initiate an unload-then-reload
of the kdump image (e. kernel, initrd, boot_params, puratory and
elfcorehdr) by the userspace kexec utility. In previous posts I have
outlined the significant performance problems related to offloading
this activity to userspace.

This patchset introduces a generic crash hot un/plug handler that
registers with the CPU and memory notifiers. Upon CPU or memory
changes, this generic handler is invoked and performs important
housekeeping, for example obtaining the appropriate lock, and then
invokes an architecture specific handler to do the appropriate
updates.

In the case of x86_64, the arch specific handler generates a new
elfcorehdr, and overwrites the old one in memory. No involvement
with userspace needed.

To realize the benefits/test this patchset, one must make a couple
of minor changes to userspace:

 - Disable the udev rule for updating kdump on hot un/plug changes.
   Add the following as the first two lines to the udev rule file
   /usr/lib/udev/rules.d/98-kexec.rules:

# For x86_64, the kernel handles updates to crash elfcorehdr
CONST{arch}=="x86-64", GOTO="kdump_reload_end"

   These two lines will cause cpu and memory hot un/plug events
   to be skipped within this rule file, for x86_64.

 - Change to the kexec_file_load for loading the kdump kernel:
   Eg. on RHEL: in /usr/bin/kdumpctl, change to:
    standard_kexec_args="-p -d -s"
   which adds the -s to select kexec_file_load syscall.

This patchset supports kexec_load with a modified kexec userspace
utility, and a working changeset to the kexec userspace utility
is provided here (and to use, the above change to standard_kexec_args
would be, for example, to append --hotplug-size=131072 instead of -s).

 diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
 index 9826f6d..06adb7e 100644
 --- a/kexec/arch/i386/crashdump-x86.c
 +++ b/kexec/arch/i386/crashdump-x86.c
 @@ -48,6 +48,7 @@
  #include <x86/x86-linux.h>
  
  extern struct arch_options_t arch_options;
 +extern unsigned long long hotplug_size;
  
  static int get_kernel_page_offset(struct kexec_info *UNUSED(info),
  				  struct crash_elf_info *elf_info)
 @@ -975,6 +976,13 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
  	} else {
  		memsz = bufsz;
  	}
 +
 +    /* If hotplug support enabled, use that size */
 +    if (hotplug_size) {
 +        memsz = hotplug_size;
 +    }
 +
 +    info->elfcorehdr =
  	elfcorehdr = add_buffer(info, tmp, bufsz, memsz, align, min_base,
  							max_addr, -1);
  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
 diff --git a/kexec/kexec.c b/kexec/kexec.c
 index f63b36b..9569d9a 100644
 --- a/kexec/kexec.c
 +++ b/kexec/kexec.c
 @@ -58,6 +58,7 @@
  
  unsigned long long mem_min = 0;
  unsigned long long mem_max = ULONG_MAX;
 +unsigned long long hotplug_size = 0;
  static unsigned long kexec_flags = 0;
  /* Flags for kexec file (fd) based syscall */
  static unsigned long kexec_file_flags = 0;
 @@ -672,6 +673,12 @@ static void update_purgatory(struct kexec_info *info)
  		if (info->segment[i].mem == (void *)info->rhdr.rel_addr) {
  			continue;
  		}
 +        /* Don't include elfcorehdr in the checksum, if hotplug
 +         * support enabled.
 +         */
 +        if (hotplug_size && (info->segment[i].mem == (void *)info->elfcorehdr)) {
 +			continue;
 +		}
  		sha256_update(&ctx, info->segment[i].buf,
  			      info->segment[i].bufsz);
  		nullsz = info->segment[i].memsz - info->segment[i].bufsz;
 @@ -1504,6 +1511,17 @@ int main(int argc, char *argv[])
  		case OPT_PRINT_CKR_SIZE:
  			print_crashkernel_region_size();
  			return 0;
 +		case OPT_HOTPLUG_SIZE:
 +            /* Reserved the specified size for hotplug growth */
 +			hotplug_size = strtoul(optarg, &endptr, 0);
 +			if (*endptr) {
 +				fprintf(stderr,
 +					"Bad option value in --hotplug-size=%s\n",
 +					optarg);
 +				usage();
 +				return 1;
 +			}
 +			break;
  		default:
  			break;
  		}
 diff --git a/kexec/kexec.h b/kexec/kexec.h
 index 595dd68..b30dda4 100644
 --- a/kexec/kexec.h
 +++ b/kexec/kexec.h
 @@ -169,6 +169,7 @@ struct kexec_info {
  	int command_line_len;
  
  	int skip_checks;
 +    unsigned long elfcorehdr;
   };
  
  struct arch_map_entry {
 @@ -231,7 +232,8 @@ extern int file_types;
  #define OPT_PRINT_CKR_SIZE	262
  #define OPT_LOAD_LIVE_UPDATE	263
  #define OPT_EXEC_LIVE_UPDATE	264
 -#define OPT_MAX			265
 +#define OPT_HOTPLUG_SIZE	265
 +#define OPT_MAX			266
  #define KEXEC_OPTIONS \
  	{ "help",		0, 0, OPT_HELP }, \
  	{ "version",		0, 0, OPT_VERSION }, \
 @@ -258,6 +260,7 @@ extern int file_types;
  	{ "debug",		0, 0, OPT_DEBUG }, \
  	{ "status",		0, 0, OPT_STATUS }, \
  	{ "print-ckr-size",     0, 0, OPT_PRINT_CKR_SIZE }, \
 +	{ "hotplug-size",     2, 0, OPT_HOTPLUG_SIZE }, \
  
  #define KEXEC_OPT_STR "h?vdfixyluet:pscaS"
 

Regards,
eric
---
v8: 5may2022
 - Per Borislav Petkov, eliminated CONFIG_CRASH_HOTPLUG in favor
   of CONFIG_HOTPLUG_CPU || CONFIG_MEMORY_HOTPLUG, ie a new define
   is not needed. Also use of IS_ENABLED() rather than #ifdef's.
   Renamed crash_hotplug_handler() to handle_hotplug_event().
   And other corrections.
 - Per Baoquan, minimized the parameters to the arch_crash_
   handle_hotplug_event() to hp_action and cpu.
 - Introduce KEXEC_CRASH_HP_INVALID_CPU definition, per Baoquan.
 - Per Sourabh Jain, renamed and repurposed CRASH_HOTPLUG_ELFCOREHDR_SZ
   to CONFIG_CRASH_MAX_MEMORY_RANGES, mirroring kexec-tools change
   by David Hildebrand. Folded this patch into the x86
   kexec_file_load support patch.

v7: 13apr2022
 https://lkml.org/lkml/2022/4/13/850
 - Resolved parameter usage to crash_hotplug_handler(), per Baoquan.

v6: 1apr2022
 https://lkml.org/lkml/2022/4/1/1203
 - Reword commit messages and some comment cleanup per Baoquan.
 - Changed elf_index to elfcorehdr_index for clarity.
 - Minor code changes per Baoquan.

v5: 3mar2022
 https://lkml.org/lkml/2022/3/3/674
 - Reworded description of CRASH_HOTPLUG_ELFCOREHDR_SZ, per
   David Hildenbrand.
 - Refactored slightly a few patches per Baoquan recommendation.

v4: 9feb2022
 https://lkml.org/lkml/2022/2/9/1406
 - Refactored patches per Baoquan suggestsions.
 - A few corrections, per Baoquan.

v3: 10jan2022
 https://lkml.org/lkml/2022/1/10/1212
 - Rebasing per Baoquan He request.
 - Changed memory notifier per David Hildenbrand.
 - Providing example kexec userspace change in cover letter.

RFC v2: 7dec2021
 https://lkml.org/lkml/2021/12/7/1088
 - Acting upon Baoquan He suggestion of removing elfcorehdr from
   the purgatory list of segments, removed purgatory code from
   patchset, and it is signficiantly simpler now.

RFC v1: 18nov2021
 https://lkml.org/lkml/2021/11/18/845
 - working patchset demonstrating kernel handling of hotplug
   updates to x86 elfcorehdr for kexec_file_load

RFC: 14dec2020
 https://lkml.org/lkml/2020/12/14/532
 - proposed concept of allowing kernel to handle hotplug update
   of elfcorehdr
---


Eric DeVolder (7):
  x86/crash: fix minor typo/bug in debug message
  crash: prototype change for crash_prepare_elf64_headers
  crash: add generic infrastructure for crash hotplug support
  kexec: exclude elfcorehdr from the segment digest
  kexec: exclude hot remove cpu from elfcorehdr notes
  x86/crash: Add x86 crash hotplug support for kexec_file_load
  x86/crash: Add x86 crash hotplug support for kexec_load

 arch/arm64/kernel/machine_kexec_file.c |   6 +-
 arch/powerpc/kexec/file_load_64.c      |   2 +-
 arch/x86/Kconfig                       |  11 ++
 arch/x86/kernel/crash.c                | 146 ++++++++++++++++++++++++-
 include/linux/crash_core.h             |  10 ++
 include/linux/kexec.h                  |  10 +-
 kernel/crash_core.c                    | 102 +++++++++++++++++
 kernel/kexec_file.c                    |  15 ++-
 8 files changed, 292 insertions(+), 10 deletions(-)

-- 
2.27.0

[PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Eric DeVolder]

When the kdump service is loaded, if a CPU or memory is hot
un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
and memory in the system, must also be updated, else the resulting
vmcore is inaccurate (eg. missing either CPU context or memory
regions).

The current solution utilizes udev to initiate an unload-then-reload
of the kdump image (e. kernel, initrd, boot_params, puratory and
elfcorehdr) by the userspace kexec utility. In previous posts I have
outlined the significant performance problems related to offloading
this activity to userspace.

This patchset introduces a generic crash hot un/plug handler that
registers with the CPU and memory notifiers. Upon CPU or memory
changes, this generic handler is invoked and performs important
housekeeping, for example obtaining the appropriate lock, and then
invokes an architecture specific handler to do the appropriate
updates.

In the case of x86_64, the arch specific handler generates a new
elfcorehdr, and overwrites the old one in memory. No involvement
with userspace needed.

To realize the benefits/test this patchset, one must make a couple
of minor changes to userspace:

 - Disable the udev rule for updating kdump on hot un/plug changes.
   Add the following as the first two lines to the udev rule file
   /usr/lib/udev/rules.d/98-kexec.rules:

# For x86_64, the kernel handles updates to crash elfcorehdr
CONST{arch}=="x86-64", GOTO="kdump_reload_end"

   These two lines will cause cpu and memory hot un/plug events
   to be skipped within this rule file, for x86_64.

 - Change to the kexec_file_load for loading the kdump kernel:
   Eg. on RHEL: in /usr/bin/kdumpctl, change to:
    standard_kexec_args="-p -d -s"
   which adds the -s to select kexec_file_load syscall.

This patchset supports kexec_load with a modified kexec userspace
utility, and a working changeset to the kexec userspace utility
is provided here (and to use, the above change to standard_kexec_args
would be, for example, to append --hotplug-size=131072 instead of -s).

 diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
 index 9826f6d..06adb7e 100644
 --- a/kexec/arch/i386/crashdump-x86.c
 +++ b/kexec/arch/i386/crashdump-x86.c
 @@ -48,6 +48,7 @@
  #include <x86/x86-linux.h>
  
  extern struct arch_options_t arch_options;
 +extern unsigned long long hotplug_size;
  
  static int get_kernel_page_offset(struct kexec_info *UNUSED(info),
  				  struct crash_elf_info *elf_info)
 @@ -975,6 +976,13 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
  	} else {
  		memsz = bufsz;
  	}
 +
 +    /* If hotplug support enabled, use that size */
 +    if (hotplug_size) {
 +        memsz = hotplug_size;
 +    }
 +
 +    info->elfcorehdr =
  	elfcorehdr = add_buffer(info, tmp, bufsz, memsz, align, min_base,
  							max_addr, -1);
  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
 diff --git a/kexec/kexec.c b/kexec/kexec.c
 index f63b36b..9569d9a 100644
 --- a/kexec/kexec.c
 +++ b/kexec/kexec.c
 @@ -58,6 +58,7 @@
  
  unsigned long long mem_min = 0;
  unsigned long long mem_max = ULONG_MAX;
 +unsigned long long hotplug_size = 0;
  static unsigned long kexec_flags = 0;
  /* Flags for kexec file (fd) based syscall */
  static unsigned long kexec_file_flags = 0;
 @@ -672,6 +673,12 @@ static void update_purgatory(struct kexec_info *info)
  		if (info->segment[i].mem == (void *)info->rhdr.rel_addr) {
  			continue;
  		}
 +        /* Don't include elfcorehdr in the checksum, if hotplug
 +         * support enabled.
 +         */
 +        if (hotplug_size && (info->segment[i].mem == (void *)info->elfcorehdr)) {
 +			continue;
 +		}
  		sha256_update(&ctx, info->segment[i].buf,
  			      info->segment[i].bufsz);
  		nullsz = info->segment[i].memsz - info->segment[i].bufsz;
 @@ -1504,6 +1511,17 @@ int main(int argc, char *argv[])
  		case OPT_PRINT_CKR_SIZE:
  			print_crashkernel_region_size();
  			return 0;
 +		case OPT_HOTPLUG_SIZE:
 +            /* Reserved the specified size for hotplug growth */
 +			hotplug_size = strtoul(optarg, &endptr, 0);
 +			if (*endptr) {
 +				fprintf(stderr,
 +					"Bad option value in --hotplug-size=%s\n",
 +					optarg);
 +				usage();
 +				return 1;
 +			}
 +			break;
  		default:
  			break;
  		}
 diff --git a/kexec/kexec.h b/kexec/kexec.h
 index 595dd68..b30dda4 100644
 --- a/kexec/kexec.h
 +++ b/kexec/kexec.h
 @@ -169,6 +169,7 @@ struct kexec_info {
  	int command_line_len;
  
  	int skip_checks;
 +    unsigned long elfcorehdr;
   };
  
  struct arch_map_entry {
 @@ -231,7 +232,8 @@ extern int file_types;
  #define OPT_PRINT_CKR_SIZE	262
  #define OPT_LOAD_LIVE_UPDATE	263
  #define OPT_EXEC_LIVE_UPDATE	264
 -#define OPT_MAX			265
 +#define OPT_HOTPLUG_SIZE	265
 +#define OPT_MAX			266
  #define KEXEC_OPTIONS \
  	{ "help",		0, 0, OPT_HELP }, \
  	{ "version",		0, 0, OPT_VERSION }, \
 @@ -258,6 +260,7 @@ extern int file_types;
  	{ "debug",		0, 0, OPT_DEBUG }, \
  	{ "status",		0, 0, OPT_STATUS }, \
  	{ "print-ckr-size",     0, 0, OPT_PRINT_CKR_SIZE }, \
 +	{ "hotplug-size",     2, 0, OPT_HOTPLUG_SIZE }, \
  
  #define KEXEC_OPT_STR "h?vdfixyluet:pscaS"
 

Regards,
eric
---
v8: 5may2022
 - Per Borislav Petkov, eliminated CONFIG_CRASH_HOTPLUG in favor
   of CONFIG_HOTPLUG_CPU || CONFIG_MEMORY_HOTPLUG, ie a new define
   is not needed. Also use of IS_ENABLED() rather than #ifdef's.
   Renamed crash_hotplug_handler() to handle_hotplug_event().
   And other corrections.
 - Per Baoquan, minimized the parameters to the arch_crash_
   handle_hotplug_event() to hp_action and cpu.
 - Introduce KEXEC_CRASH_HP_INVALID_CPU definition, per Baoquan.
 - Per Sourabh Jain, renamed and repurposed CRASH_HOTPLUG_ELFCOREHDR_SZ
   to CONFIG_CRASH_MAX_MEMORY_RANGES, mirroring kexec-tools change
   by David Hildebrand. Folded this patch into the x86
   kexec_file_load support patch.

v7: 13apr2022
 https://lkml.org/lkml/2022/4/13/850
 - Resolved parameter usage to crash_hotplug_handler(), per Baoquan.

v6: 1apr2022
 https://lkml.org/lkml/2022/4/1/1203
 - Reword commit messages and some comment cleanup per Baoquan.
 - Changed elf_index to elfcorehdr_index for clarity.
 - Minor code changes per Baoquan.

v5: 3mar2022
 https://lkml.org/lkml/2022/3/3/674
 - Reworded description of CRASH_HOTPLUG_ELFCOREHDR_SZ, per
   David Hildenbrand.
 - Refactored slightly a few patches per Baoquan recommendation.

v4: 9feb2022
 https://lkml.org/lkml/2022/2/9/1406
 - Refactored patches per Baoquan suggestsions.
 - A few corrections, per Baoquan.

v3: 10jan2022
 https://lkml.org/lkml/2022/1/10/1212
 - Rebasing per Baoquan He request.
 - Changed memory notifier per David Hildenbrand.
 - Providing example kexec userspace change in cover letter.

RFC v2: 7dec2021
 https://lkml.org/lkml/2021/12/7/1088
 - Acting upon Baoquan He suggestion of removing elfcorehdr from
   the purgatory list of segments, removed purgatory code from
   patchset, and it is signficiantly simpler now.

RFC v1: 18nov2021
 https://lkml.org/lkml/2021/11/18/845
 - working patchset demonstrating kernel handling of hotplug
   updates to x86 elfcorehdr for kexec_file_load

RFC: 14dec2020
 https://lkml.org/lkml/2020/12/14/532
 - proposed concept of allowing kernel to handle hotplug update
   of elfcorehdr
---


Eric DeVolder (7):
  x86/crash: fix minor typo/bug in debug message
  crash: prototype change for crash_prepare_elf64_headers
  crash: add generic infrastructure for crash hotplug support
  kexec: exclude elfcorehdr from the segment digest
  kexec: exclude hot remove cpu from elfcorehdr notes
  x86/crash: Add x86 crash hotplug support for kexec_file_load
  x86/crash: Add x86 crash hotplug support for kexec_load

 arch/arm64/kernel/machine_kexec_file.c |   6 +-
 arch/powerpc/kexec/file_load_64.c      |   2 +-
 arch/x86/Kconfig                       |  11 ++
 arch/x86/kernel/crash.c                | 146 ++++++++++++++++++++++++-
 include/linux/crash_core.h             |  10 ++
 include/linux/kexec.h                  |  10 +-
 kernel/crash_core.c                    | 102 +++++++++++++++++
 kernel/kexec_file.c                    |  15 ++-
 8 files changed, 292 insertions(+), 10 deletions(-)

-- 
2.27.0

[PATCH v8 1/7] x86/crash: fix minor typo/bug in debug message

[Eric DeVolder]

The pr_debug() intends to display the memsz member, but the
parameter is actually the bufsz member (which is already
displayed). Correct this to display memsz value.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Acked-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/kernel/crash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index e8326a8d1c5d..9730c88530fc 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -407,7 +407,7 @@ int crash_load_segments(struct kimage *image)
 	}
 	image->elf_load_addr = kbuf.mem;
 	pr_debug("Loaded ELF headers at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-		 image->elf_load_addr, kbuf.bufsz, kbuf.bufsz);
+		 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
 
 	return ret;
 }
-- 
2.27.0

[PATCH v8 1/7] x86/crash: fix minor typo/bug in debug message

[Eric DeVolder]

The pr_debug() intends to display the memsz member, but the
parameter is actually the bufsz member (which is already
displayed). Correct this to display memsz value.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Acked-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/kernel/crash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index e8326a8d1c5d..9730c88530fc 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -407,7 +407,7 @@ int crash_load_segments(struct kimage *image)
 	}
 	image->elf_load_addr = kbuf.mem;
 	pr_debug("Loaded ELF headers at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-		 image->elf_load_addr, kbuf.bufsz, kbuf.bufsz);
+		 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
 
 	return ret;
 }
-- 
2.27.0

[PATCH v8 2/7] crash: prototype change for crash_prepare_elf64_headers

[Eric DeVolder]

From within crash_prepare_elf64_headers() there is a need to
reference the struct kimage hotplug members. As such, this
change passes the struct kimage as a parameter to the
crash_prepare_elf64_headers().

This is preparation for later patch, no functionality change.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Acked-by: Baoquan He <bhe@redhat.com>
---
 arch/arm64/kernel/machine_kexec_file.c | 6 +++---
 arch/powerpc/kexec/file_load_64.c      | 2 +-
 arch/x86/kernel/crash.c                | 3 ++-
 include/linux/kexec.h                  | 5 +++--
 kernel/kexec_file.c                    | 4 ++--
 5 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 59c648d51848..7dbafb42ecf2 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -39,7 +39,7 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
 	return kexec_image_post_load_cleanup_default(image);
 }
 
-static int prepare_elf_headers(void **addr, unsigned long *sz)
+static int prepare_elf_headers(struct kimage *image, void **addr, unsigned long *sz)
 {
 	struct crash_mem *cmem;
 	unsigned int nr_ranges;
@@ -67,7 +67,7 @@ static int prepare_elf_headers(void **addr, unsigned long *sz)
 	ret = crash_exclude_mem_range(cmem, crashk_res.start, crashk_res.end);
 
 	if (!ret)
-		ret =  crash_prepare_elf64_headers(cmem, true, addr, sz);
+		ret =  crash_prepare_elf64_headers(image, cmem, true, addr, sz);
 
 	kfree(cmem);
 	return ret;
@@ -96,7 +96,7 @@ int load_other_segments(struct kimage *image,
 
 	/* load elf core header */
 	if (image->type == KEXEC_TYPE_CRASH) {
-		ret = prepare_elf_headers(&headers, &headers_sz);
+		ret = prepare_elf_headers(image, &headers, &headers_sz);
 		if (ret) {
 			pr_err("Preparing elf core header failed\n");
 			goto out_err;
diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
index b4981b651d9a..07da6bf1cf24 100644
--- a/arch/powerpc/kexec/file_load_64.c
+++ b/arch/powerpc/kexec/file_load_64.c
@@ -797,7 +797,7 @@ static int load_elfcorehdr_segment(struct kimage *image, struct kexec_buf *kbuf)
 		goto out;
 
 	/* Setup elfcorehdr segment */
-	ret = crash_prepare_elf64_headers(cmem, false, &headers, &headers_sz);
+	ret = crash_prepare_elf64_headers(image, cmem, false, &headers, &headers_sz);
 	if (ret) {
 		pr_err("Failed to prepare elf headers for the core\n");
 		goto out;
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9730c88530fc..9db41cce8d97 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -265,7 +265,8 @@ static int prepare_elf_headers(struct kimage *image, void **addr,
 		goto out;
 
 	/* By default prepare 64bit headers */
-	ret =  crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
+	ret =  crash_prepare_elf64_headers(image, cmem,
+				IS_ENABLED(CONFIG_X86_64), addr, sz);
 
 out:
 	vfree(cmem);
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 58d1b58a971e..f93f2591fc1e 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -227,8 +227,9 @@ struct crash_mem {
 extern int crash_exclude_mem_range(struct crash_mem *mem,
 				   unsigned long long mstart,
 				   unsigned long long mend);
-extern int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
-				       void **addr, unsigned long *sz);
+extern int crash_prepare_elf64_headers(struct kimage *image,
+	struct crash_mem *mem, int kernel_map,
+	void **addr, unsigned long *sz);
 #endif /* CONFIG_KEXEC_FILE */
 
 #ifdef CONFIG_KEXEC_ELF
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 8347fc158d2b..801d0d0a5012 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -1260,8 +1260,8 @@ int crash_exclude_mem_range(struct crash_mem *mem,
 	return 0;
 }
 
-int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
-			  void **addr, unsigned long *sz)
+int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem,
+	int kernel_map, void **addr, unsigned long *sz)
 {
 	Elf64_Ehdr *ehdr;
 	Elf64_Phdr *phdr;
-- 
2.27.0

[PATCH v8 2/7] crash: prototype change for crash_prepare_elf64_headers

[Eric DeVolder]

From within crash_prepare_elf64_headers() there is a need to
reference the struct kimage hotplug members. As such, this
change passes the struct kimage as a parameter to the
crash_prepare_elf64_headers().

This is preparation for later patch, no functionality change.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Acked-by: Baoquan He <bhe@redhat.com>
---
 arch/arm64/kernel/machine_kexec_file.c | 6 +++---
 arch/powerpc/kexec/file_load_64.c      | 2 +-
 arch/x86/kernel/crash.c                | 3 ++-
 include/linux/kexec.h                  | 5 +++--
 kernel/kexec_file.c                    | 4 ++--
 5 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 59c648d51848..7dbafb42ecf2 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -39,7 +39,7 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
 	return kexec_image_post_load_cleanup_default(image);
 }
 
-static int prepare_elf_headers(void **addr, unsigned long *sz)
+static int prepare_elf_headers(struct kimage *image, void **addr, unsigned long *sz)
 {
 	struct crash_mem *cmem;
 	unsigned int nr_ranges;
@@ -67,7 +67,7 @@ static int prepare_elf_headers(void **addr, unsigned long *sz)
 	ret = crash_exclude_mem_range(cmem, crashk_res.start, crashk_res.end);
 
 	if (!ret)
-		ret =  crash_prepare_elf64_headers(cmem, true, addr, sz);
+		ret =  crash_prepare_elf64_headers(image, cmem, true, addr, sz);
 
 	kfree(cmem);
 	return ret;
@@ -96,7 +96,7 @@ int load_other_segments(struct kimage *image,
 
 	/* load elf core header */
 	if (image->type == KEXEC_TYPE_CRASH) {
-		ret = prepare_elf_headers(&headers, &headers_sz);
+		ret = prepare_elf_headers(image, &headers, &headers_sz);
 		if (ret) {
 			pr_err("Preparing elf core header failed\n");
 			goto out_err;
diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
index b4981b651d9a..07da6bf1cf24 100644
--- a/arch/powerpc/kexec/file_load_64.c
+++ b/arch/powerpc/kexec/file_load_64.c
@@ -797,7 +797,7 @@ static int load_elfcorehdr_segment(struct kimage *image, struct kexec_buf *kbuf)
 		goto out;
 
 	/* Setup elfcorehdr segment */
-	ret = crash_prepare_elf64_headers(cmem, false, &headers, &headers_sz);
+	ret = crash_prepare_elf64_headers(image, cmem, false, &headers, &headers_sz);
 	if (ret) {
 		pr_err("Failed to prepare elf headers for the core\n");
 		goto out;
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9730c88530fc..9db41cce8d97 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -265,7 +265,8 @@ static int prepare_elf_headers(struct kimage *image, void **addr,
 		goto out;
 
 	/* By default prepare 64bit headers */
-	ret =  crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
+	ret =  crash_prepare_elf64_headers(image, cmem,
+				IS_ENABLED(CONFIG_X86_64), addr, sz);
 
 out:
 	vfree(cmem);
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 58d1b58a971e..f93f2591fc1e 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -227,8 +227,9 @@ struct crash_mem {
 extern int crash_exclude_mem_range(struct crash_mem *mem,
 				   unsigned long long mstart,
 				   unsigned long long mend);
-extern int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
-				       void **addr, unsigned long *sz);
+extern int crash_prepare_elf64_headers(struct kimage *image,
+	struct crash_mem *mem, int kernel_map,
+	void **addr, unsigned long *sz);
 #endif /* CONFIG_KEXEC_FILE */
 
 #ifdef CONFIG_KEXEC_ELF
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 8347fc158d2b..801d0d0a5012 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -1260,8 +1260,8 @@ int crash_exclude_mem_range(struct crash_mem *mem,
 	return 0;
 }
 
-int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
-			  void **addr, unsigned long *sz)
+int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem,
+	int kernel_map, void **addr, unsigned long *sz)
 {
 	Elf64_Ehdr *ehdr;
 	Elf64_Phdr *phdr;
-- 
2.27.0

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

CPU and memory change notifications are received in order to
regenerate the elfcorehdr.

To support cpu hotplug, a callback is registered to capture the
CPUHP_AP_ONLINE_DYN online and offline events via
cpuhp_setup_state_nocalls().

To support memory hotplug, a notifier is registered to capture the
MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().

The cpu callback and memory notifiers call handle_hotplug_event()
to handle the hot plug/unplug event. Then handle_hotplug_event()
dispatches the event to the architecture specific
arch_crash_handle_hotplug_event(). During the process, the
kexec_mutex is held.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 include/linux/crash_core.h | 10 +++++
 include/linux/kexec.h      |  5 +++
 kernel/crash_core.c        | 92 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index de62a722431e..a240f84348aa 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -84,4 +84,14 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
 int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
 
+#define KEXEC_CRASH_HP_REMOVE_CPU		0
+#define KEXEC_CRASH_HP_ADD_CPU			1
+#define KEXEC_CRASH_HP_REMOVE_MEMORY	2
+#define KEXEC_CRASH_HP_ADD_MEMORY		3
+#define KEXEC_CRASH_HP_INVALID_CPU		-1U
+
+struct kimage;
+void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
+									unsigned int cpu);
+
 #endif /* LINUX_CRASH_CORE_H */
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index f93f2591fc1e..5935bc78ed7f 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -308,6 +308,11 @@ struct kimage {
 	struct purgatory_info purgatory_info;
 #endif
 
+	bool hotplug_event;
+	unsigned int offlinecpu;
+	bool elfcorehdr_index_valid;
+	int elfcorehdr_index;
+
 #ifdef CONFIG_IMA_KEXEC
 	/* Virtual address of IMA measurement buffer for kexec syscall */
 	void *ima_buffer;
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 256cf6db573c..f197af50def6 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -9,12 +9,17 @@
 #include <linux/init.h>
 #include <linux/utsname.h>
 #include <linux/vmalloc.h>
+#include <linux/highmem.h>
+#include <linux/memory.h>
+#include <linux/cpuhotplug.h>
 
 #include <asm/page.h>
 #include <asm/sections.h>
 
 #include <crypto/sha1.h>
 
+#include "kexec_internal.h"
+
 /* vmcoreinfo stuff */
 unsigned char *vmcoreinfo_data;
 size_t vmcoreinfo_size;
@@ -491,3 +496,90 @@ static int __init crash_save_vmcoreinfo_init(void)
 }
 
 subsys_initcall(crash_save_vmcoreinfo_init);
+
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+void __weak arch_crash_handle_hotplug_event(struct kimage *image,
+							unsigned int hp_action, unsigned int cpu)
+{
+	WARN(1, "crash hotplug handler not implemented");
+}
+
+static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
+{
+	/* Obtain lock while changing crash information */
+	if (!mutex_trylock(&kexec_mutex))
+		return;
+
+	/* Check kdump is loaded */
+	if (kexec_crash_image) {
+		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
+
+		/* Needed in order for the segments to be updated */
+		arch_kexec_unprotect_crashkres();
+
+		/* Flag to differentiate between normal load and hotplug */
+		kexec_crash_image->hotplug_event = true;
+
+		/* Now invoke arch-specific update handler */
+		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
+
+		/* No longer handling a hotplug event */
+		kexec_crash_image->hotplug_event = false;
+
+		/* Change back to read-only */
+		arch_kexec_protect_crashkres();
+	}
+
+	/* Release lock now that update complete */
+	mutex_unlock(&kexec_mutex);
+}
+
+static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
+{
+	switch (val) {
+	case MEM_ONLINE:
+		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
+		break;
+
+	case MEM_OFFLINE:
+		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block crash_memhp_nb = {
+	.notifier_call = crash_memhp_notifier,
+	.priority = 0
+};
+
+static int crash_cpuhp_online(unsigned int cpu)
+{
+	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
+	return 0;
+}
+
+static int crash_cpuhp_offline(unsigned int cpu)
+{
+	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
+	return 0;
+}
+
+static int __init crash_hotplug_init(void)
+{
+	int result = 0;
+
+	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
+		register_memory_notifier(&crash_memhp_nb);
+
+	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
+		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+									"crash/cpuhp",
+									crash_cpuhp_online,
+									crash_cpuhp_offline);
+
+	return result;
+}
+
+subsys_initcall(crash_hotplug_init);
+#endif
-- 
2.27.0

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

CPU and memory change notifications are received in order to
regenerate the elfcorehdr.

To support cpu hotplug, a callback is registered to capture the
CPUHP_AP_ONLINE_DYN online and offline events via
cpuhp_setup_state_nocalls().

To support memory hotplug, a notifier is registered to capture the
MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().

The cpu callback and memory notifiers call handle_hotplug_event()
to handle the hot plug/unplug event. Then handle_hotplug_event()
dispatches the event to the architecture specific
arch_crash_handle_hotplug_event(). During the process, the
kexec_mutex is held.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 include/linux/crash_core.h | 10 +++++
 include/linux/kexec.h      |  5 +++
 kernel/crash_core.c        | 92 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index de62a722431e..a240f84348aa 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -84,4 +84,14 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
 int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
 
+#define KEXEC_CRASH_HP_REMOVE_CPU		0
+#define KEXEC_CRASH_HP_ADD_CPU			1
+#define KEXEC_CRASH_HP_REMOVE_MEMORY	2
+#define KEXEC_CRASH_HP_ADD_MEMORY		3
+#define KEXEC_CRASH_HP_INVALID_CPU		-1U
+
+struct kimage;
+void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
+									unsigned int cpu);
+
 #endif /* LINUX_CRASH_CORE_H */
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index f93f2591fc1e..5935bc78ed7f 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -308,6 +308,11 @@ struct kimage {
 	struct purgatory_info purgatory_info;
 #endif
 
+	bool hotplug_event;
+	unsigned int offlinecpu;
+	bool elfcorehdr_index_valid;
+	int elfcorehdr_index;
+
 #ifdef CONFIG_IMA_KEXEC
 	/* Virtual address of IMA measurement buffer for kexec syscall */
 	void *ima_buffer;
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 256cf6db573c..f197af50def6 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -9,12 +9,17 @@
 #include <linux/init.h>
 #include <linux/utsname.h>
 #include <linux/vmalloc.h>
+#include <linux/highmem.h>
+#include <linux/memory.h>
+#include <linux/cpuhotplug.h>
 
 #include <asm/page.h>
 #include <asm/sections.h>
 
 #include <crypto/sha1.h>
 
+#include "kexec_internal.h"
+
 /* vmcoreinfo stuff */
 unsigned char *vmcoreinfo_data;
 size_t vmcoreinfo_size;
@@ -491,3 +496,90 @@ static int __init crash_save_vmcoreinfo_init(void)
 }
 
 subsys_initcall(crash_save_vmcoreinfo_init);
+
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+void __weak arch_crash_handle_hotplug_event(struct kimage *image,
+							unsigned int hp_action, unsigned int cpu)
+{
+	WARN(1, "crash hotplug handler not implemented");
+}
+
+static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
+{
+	/* Obtain lock while changing crash information */
+	if (!mutex_trylock(&kexec_mutex))
+		return;
+
+	/* Check kdump is loaded */
+	if (kexec_crash_image) {
+		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
+
+		/* Needed in order for the segments to be updated */
+		arch_kexec_unprotect_crashkres();
+
+		/* Flag to differentiate between normal load and hotplug */
+		kexec_crash_image->hotplug_event = true;
+
+		/* Now invoke arch-specific update handler */
+		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
+
+		/* No longer handling a hotplug event */
+		kexec_crash_image->hotplug_event = false;
+
+		/* Change back to read-only */
+		arch_kexec_protect_crashkres();
+	}
+
+	/* Release lock now that update complete */
+	mutex_unlock(&kexec_mutex);
+}
+
+static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
+{
+	switch (val) {
+	case MEM_ONLINE:
+		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
+		break;
+
+	case MEM_OFFLINE:
+		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block crash_memhp_nb = {
+	.notifier_call = crash_memhp_notifier,
+	.priority = 0
+};
+
+static int crash_cpuhp_online(unsigned int cpu)
+{
+	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
+	return 0;
+}
+
+static int crash_cpuhp_offline(unsigned int cpu)
+{
+	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
+	return 0;
+}
+
+static int __init crash_hotplug_init(void)
+{
+	int result = 0;
+
+	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
+		register_memory_notifier(&crash_memhp_nb);
+
+	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
+		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+									"crash/cpuhp",
+									crash_cpuhp_online,
+									crash_cpuhp_offline);
+
+	return result;
+}
+
+subsys_initcall(crash_hotplug_init);
+#endif
-- 
2.27.0

[PATCH v8 4/7] kexec: exclude elfcorehdr from the segment digest

[Eric DeVolder]

When a crash kernel is loaded via the kexec_file_load syscall, the
kernel places the various segments (ie crash kernel, crash initrd,
boot_params, elfcorehdr, purgatory, etc) in memory. For those
architectures that utilize purgatory, a hash digest of the segments
is calculated for integrity checking. This digest is embedded into
the purgatory image prior to placing purgatory in memory.

Since hotplug events cause changes to the elfcorehdr, purgatory
integrity checking fails (at crash time, and no kdump created).
As a result, this change explicitly excludes the elfcorehdr segment
from the list of segments used to create the digest. By doing so,
this permits changes to the elfcorehdr in response to hotplug events,
without having to also reload purgatory due to the change to the
digest.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 kernel/kexec_file.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 801d0d0a5012..aacdf93c3507 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -765,6 +765,12 @@ static int kexec_calculate_store_digests(struct kimage *image)
 	for (j = i = 0; i < image->nr_segments; i++) {
 		struct kexec_segment *ksegment;
 
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+		/* This segment excluded to allow future changes via hotplug */
+		if (image->elfcorehdr_index_valid && (j == image->elfcorehdr_index))
+			continue;
+#endif
+
 		ksegment = &image->segment[i];
 		/*
 		 * Skip purgatory as it will be modified once we put digest
-- 
2.27.0

[PATCH v8 4/7] kexec: exclude elfcorehdr from the segment digest

[Eric DeVolder]

When a crash kernel is loaded via the kexec_file_load syscall, the
kernel places the various segments (ie crash kernel, crash initrd,
boot_params, elfcorehdr, purgatory, etc) in memory. For those
architectures that utilize purgatory, a hash digest of the segments
is calculated for integrity checking. This digest is embedded into
the purgatory image prior to placing purgatory in memory.

Since hotplug events cause changes to the elfcorehdr, purgatory
integrity checking fails (at crash time, and no kdump created).
As a result, this change explicitly excludes the elfcorehdr segment
from the list of segments used to create the digest. By doing so,
this permits changes to the elfcorehdr in response to hotplug events,
without having to also reload purgatory due to the change to the
digest.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 kernel/kexec_file.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 801d0d0a5012..aacdf93c3507 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -765,6 +765,12 @@ static int kexec_calculate_store_digests(struct kimage *image)
 	for (j = i = 0; i < image->nr_segments; i++) {
 		struct kexec_segment *ksegment;
 
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+		/* This segment excluded to allow future changes via hotplug */
+		if (image->elfcorehdr_index_valid && (j == image->elfcorehdr_index))
+			continue;
+#endif
+
 		ksegment = &image->segment[i];
 		/*
 		 * Skip purgatory as it will be modified once we put digest
-- 
2.27.0

[PATCH v8 5/7] kexec: exclude hot remove cpu from elfcorehdr notes

[Eric DeVolder]

Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
still in the for_each_present_cpu() list when within the
handle_hotplug_event(). Thus the CPU must be explicitly excluded
when building the new list of CPUs.

This change identifies in handle_hotplug_event() the CPU to be
excluded, and the check for excluding the CPU in
crash_prepare_elf64_headers().

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 kernel/crash_core.c | 10 ++++++++++
 kernel/kexec_file.c |  5 +++++
 2 files changed, 15 insertions(+)

diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index f197af50def6..7ba43f058d82 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -520,6 +520,16 @@ static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
 		/* Flag to differentiate between normal load and hotplug */
 		kexec_crash_image->hotplug_event = true;
 
+		/*
+		 * Due to use of CPUHP_AP_ONLINE_DYN, upon unplug and during
+		 * this callback, the CPU is still in the for_each_present_cpu()
+		 * list. Must explicitly look to exclude this CPU when building
+		 * new list.
+		 */
+		kexec_crash_image->offlinecpu =
+			(hp_action == KEXEC_CRASH_HP_REMOVE_CPU) ?
+				cpu : KEXEC_CRASH_HP_INVALID_CPU;
+
 		/* Now invoke arch-specific update handler */
 		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
 
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index aacdf93c3507..d68e5769b428 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -1314,6 +1314,11 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem,
 
 	/* Prepare one phdr of type PT_NOTE for each present CPU */
 	for_each_present_cpu(cpu) {
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+		/* Skip the soon-to-be offlined cpu */
+		if (image->hotplug_event && (cpu == image->offlinecpu))
+			continue;
+#endif
 		phdr->p_type = PT_NOTE;
 		notes_addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpu));
 		phdr->p_offset = phdr->p_paddr = notes_addr;
-- 
2.27.0

[PATCH v8 5/7] kexec: exclude hot remove cpu from elfcorehdr notes

[Eric DeVolder]

Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
still in the for_each_present_cpu() list when within the
handle_hotplug_event(). Thus the CPU must be explicitly excluded
when building the new list of CPUs.

This change identifies in handle_hotplug_event() the CPU to be
excluded, and the check for excluding the CPU in
crash_prepare_elf64_headers().

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 kernel/crash_core.c | 10 ++++++++++
 kernel/kexec_file.c |  5 +++++
 2 files changed, 15 insertions(+)

diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index f197af50def6..7ba43f058d82 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -520,6 +520,16 @@ static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
 		/* Flag to differentiate between normal load and hotplug */
 		kexec_crash_image->hotplug_event = true;
 
+		/*
+		 * Due to use of CPUHP_AP_ONLINE_DYN, upon unplug and during
+		 * this callback, the CPU is still in the for_each_present_cpu()
+		 * list. Must explicitly look to exclude this CPU when building
+		 * new list.
+		 */
+		kexec_crash_image->offlinecpu =
+			(hp_action == KEXEC_CRASH_HP_REMOVE_CPU) ?
+				cpu : KEXEC_CRASH_HP_INVALID_CPU;
+
 		/* Now invoke arch-specific update handler */
 		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
 
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index aacdf93c3507..d68e5769b428 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -1314,6 +1314,11 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem,
 
 	/* Prepare one phdr of type PT_NOTE for each present CPU */
 	for_each_present_cpu(cpu) {
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+		/* Skip the soon-to-be offlined cpu */
+		if (image->hotplug_event && (cpu == image->offlinecpu))
+			continue;
+#endif
 		phdr->p_type = PT_NOTE;
 		notes_addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpu));
 		phdr->p_offset = phdr->p_paddr = notes_addr;
-- 
2.27.0

[PATCH v8 6/7] x86/crash: Add x86 crash hotplug support for kexec_file_load

[Eric DeVolder]

For x86_64, when CPU or memory is hot un/plugged, the crash
elfcorehdr, which describes the CPUs and memory in the system,
must also be updated.

To update the elfcorehdr for x86_64, a new elfcorehdr must be
generated from the available CPUs and memory. The new elfcorehdr
is prepared into a buffer, and then installed over the top of
the existing elfcorehdr.

In the patch 'kexec: exclude elfcorehdr from the segment digest'
the need to update purgatory due to the change in elfcorehdr was
eliminated.  As a result, no changes to purgatory or boot_params
(as the elfcorehdr= kernel command line parameter pointer
remains unchanged and correct) are needed, just elfcorehdr.

To accommodate a growing number of resources via hotplug, the
elfcorehdr segment must be sufficiently large enough to accommodate
changes, see the CRASH_MAX_MEMORY_RANGES configure item.

With this change, crash hotplug for kexec_file_load syscall
is supported. When loading the crash kernel via kexec_file_load,
the elfcorehdr is identified at load time in crash_load_segments().

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 arch/x86/Kconfig        |  11 ++++
 arch/x86/kernel/crash.c | 117 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 4bed3abf444d..bf1201fe6981 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2072,6 +2072,17 @@ config CRASH_DUMP
 	  (CONFIG_RELOCATABLE=y).
 	  For more details see Documentation/admin-guide/kdump/kdump.rst
 
+config CRASH_MAX_MEMORY_RANGES
+	depends on CRASH_DUMP && KEXEC_FILE && (HOTPLUG_CPU || MEMORY_HOTPLUG)
+	int
+	default 32768
+	help
+	  For the kexec_file_load path, specify the maximum number of
+	  memory regions, eg. as represented by the 'System RAM' entries
+	  in /proc/iomem, that the elfcorehdr buffer/segment can accommodate.
+	  This value is combined with NR_CPUS and multiplied by Elf64_Phdr
+	  size to determine the final buffer size.
+
 config KEXEC_JUMP
 	bool "kexec jump"
 	depends on KEXEC && HIBERNATION
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9db41cce8d97..951ef365f0a7 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -25,6 +25,7 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/memblock.h>
+#include <linux/highmem.h>
 
 #include <asm/processor.h>
 #include <asm/hardirq.h>
@@ -398,7 +399,17 @@ int crash_load_segments(struct kimage *image)
 	image->elf_headers = kbuf.buffer;
 	image->elf_headers_sz = kbuf.bufsz;
 
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+	/* Ensure elfcorehdr segment large enough for hotplug changes */
+	kbuf.memsz = (CONFIG_NR_CPUS_DEFAULT + CONFIG_CRASH_MAX_MEMORY_RANGES) * sizeof(Elf64_Phdr);
+	/* For marking as usable to crash kernel */
+	image->elf_headers_sz = kbuf.memsz;
+	/* Record the index of the elfcorehdr segment */
+	image->elfcorehdr_index = image->nr_segments;
+	image->elfcorehdr_index_valid = true;
+#else
 	kbuf.memsz = kbuf.bufsz;
+#endif
 	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
 	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
 	ret = kexec_add_buffer(&kbuf);
@@ -413,3 +424,109 @@ int crash_load_segments(struct kimage *image)
 	return ret;
 }
 #endif /* CONFIG_KEXEC_FILE */
+
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+static void *map_crash_pages(unsigned long paddr, unsigned long size)
+{
+	/*
+	 * NOTE: The addresses and sizes passed to this routine have
+	 * already been fully aligned on page boundaries. There is no
+	 * need for massaging the address or size.
+	 */
+	void *ptr = NULL;
+
+	/* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
+	if (size > 0) {
+		struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
+
+		ptr = kmap(page);
+	}
+
+	return ptr;
+}
+
+static void unmap_crash_pages(void **ptr)
+{
+	if (ptr) {
+		if (*ptr)
+			kunmap(*ptr);
+		*ptr = NULL;
+	}
+}
+
+/**
+ * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
+ * @image: the active struct kimage
+ * @hp_action: the hot un/plug action being handled
+ * @cpu: when KEXEC_CRASH_HP_ADD/REMOVE_CPU, the cpu affected
+ *
+ * To accurately reflect hot un/plug changes, the elfcorehdr (which
+ * is passed to the crash kernel via the elfcorehdr= parameter)
+ * must be updated with the new list of CPUs and memories. The new
+ * elfcorehdr is prepared in a kernel buffer, and then it is
+ * written on top of the existing/old elfcorehdr.
+ *
+ * For hotplug changes to elfcorehdr to work, two conditions are
+ * needed:
+ * First, the segment containing the elfcorehdr must be large enough
+ * to permit a growing number of resources. See the
+ * CONFIG_CRASH_MAX_MEMORY_RANGES description.
+ * Second, purgatory must explicitly exclude the elfcorehdr from the
+ * list of segments it checks (since the elfcorehdr changes and thus
+ * would require an update to purgatory itself to update the digest).
+ *
+ */
+void arch_crash_handle_hotplug_event(struct kimage *image,
+	unsigned int hp_action, unsigned int cpu)
+{
+	struct kexec_segment *ksegment;
+	unsigned char *ptr = NULL;
+	unsigned long elfsz = 0;
+	void *elfbuf = NULL;
+	unsigned long mem, memsz;
+
+	if (!image->elfcorehdr_index_valid) {
+		pr_err("crash hp: unable to locate elfcorehdr segment");
+		goto out;
+	}
+
+	ksegment = &image->segment[image->elfcorehdr_index];
+	mem = ksegment->mem;
+	memsz = ksegment->memsz;
+
+	/*
+	 * Create the new elfcorehdr reflecting the changes to CPU and/or
+	 * memory resources.
+	 */
+	if (prepare_elf_headers(image, &elfbuf, &elfsz)) {
+		pr_err("crash hp: unable to prepare elfcore headers");
+		goto out;
+	}
+	if (elfsz > memsz) {
+		pr_err("crash hp: update elfcorehdr elfsz %lu > memsz %lu",
+			elfsz, memsz);
+		goto out;
+	}
+
+	/*
+	 * At this point, we are all but assured of success.
+	 * Copy new elfcorehdr into destination.
+	 */
+	ptr = map_crash_pages(mem, memsz);
+	if (ptr) {
+		/*
+		 * Temporarily invalidate the crash image while the
+		 * elfcorehdr is updated.
+		 */
+		xchg(&kexec_crash_image, NULL);
+		memcpy_flushcache((void *)ptr, elfbuf, elfsz);
+		xchg(&kexec_crash_image, image);
+	}
+	unmap_crash_pages((void **)&ptr);
+	pr_debug("crash hp: re-loaded elfcorehdr at 0x%lx\n", mem);
+
+out:
+	if (elfbuf)
+		vfree(elfbuf);
+}
+#endif
-- 
2.27.0

[PATCH v8 6/7] x86/crash: Add x86 crash hotplug support for kexec_file_load

[Eric DeVolder]

For x86_64, when CPU or memory is hot un/plugged, the crash
elfcorehdr, which describes the CPUs and memory in the system,
must also be updated.

To update the elfcorehdr for x86_64, a new elfcorehdr must be
generated from the available CPUs and memory. The new elfcorehdr
is prepared into a buffer, and then installed over the top of
the existing elfcorehdr.

In the patch 'kexec: exclude elfcorehdr from the segment digest'
the need to update purgatory due to the change in elfcorehdr was
eliminated.  As a result, no changes to purgatory or boot_params
(as the elfcorehdr= kernel command line parameter pointer
remains unchanged and correct) are needed, just elfcorehdr.

To accommodate a growing number of resources via hotplug, the
elfcorehdr segment must be sufficiently large enough to accommodate
changes, see the CRASH_MAX_MEMORY_RANGES configure item.

With this change, crash hotplug for kexec_file_load syscall
is supported. When loading the crash kernel via kexec_file_load,
the elfcorehdr is identified at load time in crash_load_segments().

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 arch/x86/Kconfig        |  11 ++++
 arch/x86/kernel/crash.c | 117 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 4bed3abf444d..bf1201fe6981 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2072,6 +2072,17 @@ config CRASH_DUMP
 	  (CONFIG_RELOCATABLE=y).
 	  For more details see Documentation/admin-guide/kdump/kdump.rst
 
+config CRASH_MAX_MEMORY_RANGES
+	depends on CRASH_DUMP && KEXEC_FILE && (HOTPLUG_CPU || MEMORY_HOTPLUG)
+	int
+	default 32768
+	help
+	  For the kexec_file_load path, specify the maximum number of
+	  memory regions, eg. as represented by the 'System RAM' entries
+	  in /proc/iomem, that the elfcorehdr buffer/segment can accommodate.
+	  This value is combined with NR_CPUS and multiplied by Elf64_Phdr
+	  size to determine the final buffer size.
+
 config KEXEC_JUMP
 	bool "kexec jump"
 	depends on KEXEC && HIBERNATION
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9db41cce8d97..951ef365f0a7 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -25,6 +25,7 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/memblock.h>
+#include <linux/highmem.h>
 
 #include <asm/processor.h>
 #include <asm/hardirq.h>
@@ -398,7 +399,17 @@ int crash_load_segments(struct kimage *image)
 	image->elf_headers = kbuf.buffer;
 	image->elf_headers_sz = kbuf.bufsz;
 
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+	/* Ensure elfcorehdr segment large enough for hotplug changes */
+	kbuf.memsz = (CONFIG_NR_CPUS_DEFAULT + CONFIG_CRASH_MAX_MEMORY_RANGES) * sizeof(Elf64_Phdr);
+	/* For marking as usable to crash kernel */
+	image->elf_headers_sz = kbuf.memsz;
+	/* Record the index of the elfcorehdr segment */
+	image->elfcorehdr_index = image->nr_segments;
+	image->elfcorehdr_index_valid = true;
+#else
 	kbuf.memsz = kbuf.bufsz;
+#endif
 	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
 	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
 	ret = kexec_add_buffer(&kbuf);
@@ -413,3 +424,109 @@ int crash_load_segments(struct kimage *image)
 	return ret;
 }
 #endif /* CONFIG_KEXEC_FILE */
+
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+static void *map_crash_pages(unsigned long paddr, unsigned long size)
+{
+	/*
+	 * NOTE: The addresses and sizes passed to this routine have
+	 * already been fully aligned on page boundaries. There is no
+	 * need for massaging the address or size.
+	 */
+	void *ptr = NULL;
+
+	/* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
+	if (size > 0) {
+		struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
+
+		ptr = kmap(page);
+	}
+
+	return ptr;
+}
+
+static void unmap_crash_pages(void **ptr)
+{
+	if (ptr) {
+		if (*ptr)
+			kunmap(*ptr);
+		*ptr = NULL;
+	}
+}
+
+/**
+ * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
+ * @image: the active struct kimage
+ * @hp_action: the hot un/plug action being handled
+ * @cpu: when KEXEC_CRASH_HP_ADD/REMOVE_CPU, the cpu affected
+ *
+ * To accurately reflect hot un/plug changes, the elfcorehdr (which
+ * is passed to the crash kernel via the elfcorehdr= parameter)
+ * must be updated with the new list of CPUs and memories. The new
+ * elfcorehdr is prepared in a kernel buffer, and then it is
+ * written on top of the existing/old elfcorehdr.
+ *
+ * For hotplug changes to elfcorehdr to work, two conditions are
+ * needed:
+ * First, the segment containing the elfcorehdr must be large enough
+ * to permit a growing number of resources. See the
+ * CONFIG_CRASH_MAX_MEMORY_RANGES description.
+ * Second, purgatory must explicitly exclude the elfcorehdr from the
+ * list of segments it checks (since the elfcorehdr changes and thus
+ * would require an update to purgatory itself to update the digest).
+ *
+ */
+void arch_crash_handle_hotplug_event(struct kimage *image,
+	unsigned int hp_action, unsigned int cpu)
+{
+	struct kexec_segment *ksegment;
+	unsigned char *ptr = NULL;
+	unsigned long elfsz = 0;
+	void *elfbuf = NULL;
+	unsigned long mem, memsz;
+
+	if (!image->elfcorehdr_index_valid) {
+		pr_err("crash hp: unable to locate elfcorehdr segment");
+		goto out;
+	}
+
+	ksegment = &image->segment[image->elfcorehdr_index];
+	mem = ksegment->mem;
+	memsz = ksegment->memsz;
+
+	/*
+	 * Create the new elfcorehdr reflecting the changes to CPU and/or
+	 * memory resources.
+	 */
+	if (prepare_elf_headers(image, &elfbuf, &elfsz)) {
+		pr_err("crash hp: unable to prepare elfcore headers");
+		goto out;
+	}
+	if (elfsz > memsz) {
+		pr_err("crash hp: update elfcorehdr elfsz %lu > memsz %lu",
+			elfsz, memsz);
+		goto out;
+	}
+
+	/*
+	 * At this point, we are all but assured of success.
+	 * Copy new elfcorehdr into destination.
+	 */
+	ptr = map_crash_pages(mem, memsz);
+	if (ptr) {
+		/*
+		 * Temporarily invalidate the crash image while the
+		 * elfcorehdr is updated.
+		 */
+		xchg(&kexec_crash_image, NULL);
+		memcpy_flushcache((void *)ptr, elfbuf, elfsz);
+		xchg(&kexec_crash_image, image);
+	}
+	unmap_crash_pages((void **)&ptr);
+	pr_debug("crash hp: re-loaded elfcorehdr at 0x%lx\n", mem);
+
+out:
+	if (elfbuf)
+		vfree(elfbuf);
+}
+#endif
-- 
2.27.0

[PATCH v8 7/7] x86/crash: Add x86 crash hotplug support for kexec_load

[Eric DeVolder]

For kexec_file_load support, the loading of the crash kernel occurs
entirely within the kernel, and as such the elfcorehdr is readily
identified (so that it can be modified upon hotplug events).

This change enables support for kexec_load by identifying the
elfcorehdr segment in the arch_crash_handle_hotplug_event(),
if it has not already been identified.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Acked-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/kernel/crash.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 951ef365f0a7..845d7c77854d 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -485,6 +485,30 @@ void arch_crash_handle_hotplug_event(struct kimage *image,
 	void *elfbuf = NULL;
 	unsigned long mem, memsz;
 
+	/*
+	 * When the struct kimage is alloced, it is wiped to zero, so
+	 * the elfcorehdr_index_valid defaults to false. It is set on the
+	 * kexec_file_load path, or here for kexec_load, if not already
+	 * identified.
+	 */
+	if (!image->elfcorehdr_index_valid) {
+		unsigned int n;
+
+		for (n = 0; n < image->nr_segments; n++) {
+			mem = image->segment[n].mem;
+			memsz = image->segment[n].memsz;
+			ptr = map_crash_pages(mem, memsz);
+			if (ptr) {
+				/* The segment containing elfcorehdr */
+				if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
+					image->elfcorehdr_index = (int)n;
+					image->elfcorehdr_index_valid = true;
+				}
+			}
+			unmap_crash_pages((void **)&ptr);
+		}
+	}
+
 	if (!image->elfcorehdr_index_valid) {
 		pr_err("crash hp: unable to locate elfcorehdr segment");
 		goto out;
-- 
2.27.0

[PATCH v8 7/7] x86/crash: Add x86 crash hotplug support for kexec_load

[Eric DeVolder]

For kexec_file_load support, the loading of the crash kernel occurs
entirely within the kernel, and as such the elfcorehdr is readily
identified (so that it can be modified upon hotplug events).

This change enables support for kexec_load by identifying the
elfcorehdr segment in the arch_crash_handle_hotplug_event(),
if it has not already been identified.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Acked-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/kernel/crash.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 951ef365f0a7..845d7c77854d 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -485,6 +485,30 @@ void arch_crash_handle_hotplug_event(struct kimage *image,
 	void *elfbuf = NULL;
 	unsigned long mem, memsz;
 
+	/*
+	 * When the struct kimage is alloced, it is wiped to zero, so
+	 * the elfcorehdr_index_valid defaults to false. It is set on the
+	 * kexec_file_load path, or here for kexec_load, if not already
+	 * identified.
+	 */
+	if (!image->elfcorehdr_index_valid) {
+		unsigned int n;
+
+		for (n = 0; n < image->nr_segments; n++) {
+			mem = image->segment[n].mem;
+			memsz = image->segment[n].memsz;
+			ptr = map_crash_pages(mem, memsz);
+			if (ptr) {
+				/* The segment containing elfcorehdr */
+				if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
+					image->elfcorehdr_index = (int)n;
+					image->elfcorehdr_index_valid = true;
+				}
+			}
+			unmap_crash_pages((void **)&ptr);
+		}
+	}
+
 	if (!image->elfcorehdr_index_valid) {
 		pr_err("crash hp: unable to locate elfcorehdr segment");
 		goto out;
-- 
2.27.0

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Baoquan He]

On 05/05/22 at 02:45pm, Eric DeVolder wrote:
......
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 256cf6db573c..f197af50def6 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -9,12 +9,17 @@
>  #include <linux/init.h>
>  #include <linux/utsname.h>
>  #include <linux/vmalloc.h>
> +#include <linux/highmem.h>

Wondering where highmem.h is needed. Just curious.

> +#include <linux/memory.h>
> +#include <linux/cpuhotplug.h>
>  
>  #include <asm/page.h>
>  #include <asm/sections.h>
>  
>  #include <crypto/sha1.h>
>  
> +#include "kexec_internal.h"
> +
>  /* vmcoreinfo stuff */
>  unsigned char *vmcoreinfo_data;
>  size_t vmcoreinfo_size;
> @@ -491,3 +496,90 @@ static int __init crash_save_vmcoreinfo_init(void)
>  }
>  
>  subsys_initcall(crash_save_vmcoreinfo_init);
> +
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
> +							unsigned int hp_action, unsigned int cpu)
> +{
> +	WARN(1, "crash hotplug handler not implemented");
> +}
> +
> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> +{
> +	/* Obtain lock while changing crash information */
> +	if (!mutex_trylock(&kexec_mutex))
> +		return;
> +
> +	/* Check kdump is loaded */
> +	if (kexec_crash_image) {
> +		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
> +
> +		/* Needed in order for the segments to be updated */
> +		arch_kexec_unprotect_crashkres();
> +
> +		/* Flag to differentiate between normal load and hotplug */
> +		kexec_crash_image->hotplug_event = true;
> +
> +		/* Now invoke arch-specific update handler */
> +		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
> +
> +		/* No longer handling a hotplug event */
> +		kexec_crash_image->hotplug_event = false;
> +
> +		/* Change back to read-only */
> +		arch_kexec_protect_crashkres();
> +	}
> +
> +	/* Release lock now that update complete */
> +	mutex_unlock(&kexec_mutex);
> +}
> +
> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> +{
> +	switch (val) {
> +	case MEM_ONLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
> +		break;
> +
> +	case MEM_OFFLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
> +		break;
> +	}
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block crash_memhp_nb = {
> +	.notifier_call = crash_memhp_notifier,
> +	.priority = 0
> +};
> +
> +static int crash_cpuhp_online(unsigned int cpu)
> +{
> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> +	return 0;
> +}
> +
> +static int crash_cpuhp_offline(unsigned int cpu)
> +{
> +	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> +	return 0;
> +}
> +
> +static int __init crash_hotplug_init(void)
> +{
> +	int result = 0;
> +
> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> +		register_memory_notifier(&crash_memhp_nb);
> +
> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> +		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> +									"crash/cpuhp",
> +									crash_cpuhp_online,
> +									crash_cpuhp_offline);
> +
> +	return result;
> +}
> +
> +subsys_initcall(crash_hotplug_init);
> +#endif
> -- 
> 2.27.0
> 

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Baoquan He]

On 05/05/22 at 02:45pm, Eric DeVolder wrote:
......
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 256cf6db573c..f197af50def6 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -9,12 +9,17 @@
>  #include <linux/init.h>
>  #include <linux/utsname.h>
>  #include <linux/vmalloc.h>
> +#include <linux/highmem.h>

Wondering where highmem.h is needed. Just curious.

> +#include <linux/memory.h>
> +#include <linux/cpuhotplug.h>
>  
>  #include <asm/page.h>
>  #include <asm/sections.h>
>  
>  #include <crypto/sha1.h>
>  
> +#include "kexec_internal.h"
> +
>  /* vmcoreinfo stuff */
>  unsigned char *vmcoreinfo_data;
>  size_t vmcoreinfo_size;
> @@ -491,3 +496,90 @@ static int __init crash_save_vmcoreinfo_init(void)
>  }
>  
>  subsys_initcall(crash_save_vmcoreinfo_init);
> +
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
> +							unsigned int hp_action, unsigned int cpu)
> +{
> +	WARN(1, "crash hotplug handler not implemented");
> +}
> +
> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> +{
> +	/* Obtain lock while changing crash information */
> +	if (!mutex_trylock(&kexec_mutex))
> +		return;
> +
> +	/* Check kdump is loaded */
> +	if (kexec_crash_image) {
> +		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
> +
> +		/* Needed in order for the segments to be updated */
> +		arch_kexec_unprotect_crashkres();
> +
> +		/* Flag to differentiate between normal load and hotplug */
> +		kexec_crash_image->hotplug_event = true;
> +
> +		/* Now invoke arch-specific update handler */
> +		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
> +
> +		/* No longer handling a hotplug event */
> +		kexec_crash_image->hotplug_event = false;
> +
> +		/* Change back to read-only */
> +		arch_kexec_protect_crashkres();
> +	}
> +
> +	/* Release lock now that update complete */
> +	mutex_unlock(&kexec_mutex);
> +}
> +
> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> +{
> +	switch (val) {
> +	case MEM_ONLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
> +		break;
> +
> +	case MEM_OFFLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
> +		break;
> +	}
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block crash_memhp_nb = {
> +	.notifier_call = crash_memhp_notifier,
> +	.priority = 0
> +};
> +
> +static int crash_cpuhp_online(unsigned int cpu)
> +{
> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> +	return 0;
> +}
> +
> +static int crash_cpuhp_offline(unsigned int cpu)
> +{
> +	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> +	return 0;
> +}
> +
> +static int __init crash_hotplug_init(void)
> +{
> +	int result = 0;
> +
> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> +		register_memory_notifier(&crash_memhp_nb);
> +
> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> +		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> +									"crash/cpuhp",
> +									crash_cpuhp_online,
> +									crash_cpuhp_offline);
> +
> +	return result;
> +}
> +
> +subsys_initcall(crash_hotplug_init);
> +#endif
> -- 
> 2.27.0
> 

Re: [PATCH v8 1/7] x86/crash: fix minor typo/bug in debug message

[Baoquan He]

On 05/09/22 at 10:46am, Sourabh Jain wrote:
> 
> On 06/05/22 00:15, Eric DeVolder wrote:
> > The pr_debug() intends to display the memsz member, but the
> > parameter is actually the bufsz member (which is already
> > displayed). Correct this to display memsz value.
> > 
> > Signed-off-by: Eric DeVolder<eric.devolder@oracle.com>
> > Reviewed-by: David Hildenbrand<david@redhat.com>
> > Acked-by: Baoquan He<bhe@redhat.com>
> > ---
> >   arch/x86/kernel/crash.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index e8326a8d1c5d..9730c88530fc 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -407,7 +407,7 @@ int crash_load_segments(struct kimage *image)
> >   	}
> >   	image->elf_load_addr = kbuf.mem;
> >   	pr_debug("Loaded ELF headers at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > -		 image->elf_load_addr, kbuf.bufsz, kbuf.bufsz);
> > +		 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > 
> I think we can push this patch separately.

Boris has taken this into tip/x86/kdump.

[PATCH v8 1/7] x86/crash: fix minor typo/bug in debug message

[Baoquan He]

On 05/09/22 at 10:46am, Sourabh Jain wrote:
> 
> On 06/05/22 00:15, Eric DeVolder wrote:
> > The pr_debug() intends to display the memsz member, but the
> > parameter is actually the bufsz member (which is already
> > displayed). Correct this to display memsz value.
> > 
> > Signed-off-by: Eric DeVolder<eric.devolder@oracle.com>
> > Reviewed-by: David Hildenbrand<david@redhat.com>
> > Acked-by: Baoquan He<bhe@redhat.com>
> > ---
> >   arch/x86/kernel/crash.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index e8326a8d1c5d..9730c88530fc 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -407,7 +407,7 @@ int crash_load_segments(struct kimage *image)
> >   	}
> >   	image->elf_load_addr = kbuf.mem;
> >   	pr_debug("Loaded ELF headers at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > -		 image->elf_load_addr, kbuf.bufsz, kbuf.bufsz);
> > +		 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > 
> I think we can push this patch separately.

Boris has taken this into tip/x86/kdump.

Re: [PATCH v8 1/7] x86/crash: fix minor typo/bug in debug message

[Eric DeVolder]

On 5/9/22 00:26, Baoquan He wrote:
> On 05/09/22 at 10:46am, Sourabh Jain wrote:
>>
>> On 06/05/22 00:15, Eric DeVolder wrote:
>>> The pr_debug() intends to display the memsz member, but the
>>> parameter is actually the bufsz member (which is already
>>> displayed). Correct this to display memsz value.
>>>
>>> Signed-off-by: Eric DeVolder<eric.devolder@oracle.com>
>>> Reviewed-by: David Hildenbrand<david@redhat.com>
>>> Acked-by: Baoquan He<bhe@redhat.com>
>>> ---
>>>    arch/x86/kernel/crash.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>>> index e8326a8d1c5d..9730c88530fc 100644
>>> --- a/arch/x86/kernel/crash.c
>>> +++ b/arch/x86/kernel/crash.c
>>> @@ -407,7 +407,7 @@ int crash_load_segments(struct kimage *image)
>>>    	}
>>>    	image->elf_load_addr = kbuf.mem;
>>>    	pr_debug("Loaded ELF headers at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
>>> -		 image->elf_load_addr, kbuf.bufsz, kbuf.bufsz);
>>> +		 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
>>>
>> I think we can push this patch separately.
> 
> Boris has taken this into tip/x86/kdump.
> 
Excellent, I'll remove this patch going forward.
Eric

[PATCH v8 1/7] x86/crash: fix minor typo/bug in debug message

[Eric DeVolder]

On 5/9/22 00:26, Baoquan He wrote:
> On 05/09/22 at 10:46am, Sourabh Jain wrote:
>>
>> On 06/05/22 00:15, Eric DeVolder wrote:
>>> The pr_debug() intends to display the memsz member, but the
>>> parameter is actually the bufsz member (which is already
>>> displayed). Correct this to display memsz value.
>>>
>>> Signed-off-by: Eric DeVolder<eric.devolder@oracle.com>
>>> Reviewed-by: David Hildenbrand<david@redhat.com>
>>> Acked-by: Baoquan He<bhe@redhat.com>
>>> ---
>>>    arch/x86/kernel/crash.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>>> index e8326a8d1c5d..9730c88530fc 100644
>>> --- a/arch/x86/kernel/crash.c
>>> +++ b/arch/x86/kernel/crash.c
>>> @@ -407,7 +407,7 @@ int crash_load_segments(struct kimage *image)
>>>    	}
>>>    	image->elf_load_addr = kbuf.mem;
>>>    	pr_debug("Loaded ELF headers at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
>>> -		 image->elf_load_addr, kbuf.bufsz, kbuf.bufsz);
>>> +		 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
>>>
>> I think we can push this patch separately.
> 
> Boris has taken this into tip/x86/kdump.
> 
Excellent, I'll remove this patch going forward.
Eric

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

On 5/6/22 02:12, Baoquan He wrote:
> On 05/05/22 at 02:45pm, Eric DeVolder wrote:
> ......
>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>> index 256cf6db573c..f197af50def6 100644
>> --- a/kernel/crash_core.c
>> +++ b/kernel/crash_core.c
>> @@ -9,12 +9,17 @@
>>   #include <linux/init.h>
>>   #include <linux/utsname.h>
>>   #include <linux/vmalloc.h>
>> +#include <linux/highmem.h>
> 
> Wondering where highmem.h is needed. Just curious.

Ahh, I missed that. At one point in time we moved map_crash_pages() into this file, which brought 
highmem.h along with it. But we have since moved map_crash_pages() into x86/crash.c. And I missed 
eliminating highmem.h at that time.

I have removed this for v9.

eric


> 
>> +#include <linux/memory.h>
>> +#include <linux/cpuhotplug.h>
>>   
>>   #include <asm/page.h>
>>   #include <asm/sections.h>
>>   
>>   #include <crypto/sha1.h>
>>   
>> +#include "kexec_internal.h"
>> +
>>   /* vmcoreinfo stuff */
>>   unsigned char *vmcoreinfo_data;
>>   size_t vmcoreinfo_size;
>> @@ -491,3 +496,90 @@ static int __init crash_save_vmcoreinfo_init(void)
>>   }
>>   
>>   subsys_initcall(crash_save_vmcoreinfo_init);
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>> +							unsigned int hp_action, unsigned int cpu)
>> +{
>> +	WARN(1, "crash hotplug handler not implemented");
>> +}
>> +
>> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> +	/* Obtain lock while changing crash information */
>> +	if (!mutex_trylock(&kexec_mutex))
>> +		return;
>> +
>> +	/* Check kdump is loaded */
>> +	if (kexec_crash_image) {
>> +		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
>> +
>> +		/* Needed in order for the segments to be updated */
>> +		arch_kexec_unprotect_crashkres();
>> +
>> +		/* Flag to differentiate between normal load and hotplug */
>> +		kexec_crash_image->hotplug_event = true;
>> +
>> +		/* Now invoke arch-specific update handler */
>> +		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>> +
>> +		/* No longer handling a hotplug event */
>> +		kexec_crash_image->hotplug_event = false;
>> +
>> +		/* Change back to read-only */
>> +		arch_kexec_protect_crashkres();
>> +	}
>> +
>> +	/* Release lock now that update complete */
>> +	mutex_unlock(&kexec_mutex);
>> +}
>> +
>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>> +{
>> +	switch (val) {
>> +	case MEM_ONLINE:
>> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
>> +		break;
>> +
>> +	case MEM_OFFLINE:
>> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
>> +		break;
>> +	}
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> +	.notifier_call = crash_memhp_notifier,
>> +	.priority = 0
>> +};
>> +
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> +	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> +	int result = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>> +		register_memory_notifier(&crash_memhp_nb);
>> +
>> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
>> +		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> +									"crash/cpuhp",
>> +									crash_cpuhp_online,
>> +									crash_cpuhp_offline);
>> +
>> +	return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif
>> -- 
>> 2.27.0
>>
> 

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

On 5/6/22 02:12, Baoquan He wrote:
> On 05/05/22 at 02:45pm, Eric DeVolder wrote:
> ......
>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>> index 256cf6db573c..f197af50def6 100644
>> --- a/kernel/crash_core.c
>> +++ b/kernel/crash_core.c
>> @@ -9,12 +9,17 @@
>>   #include <linux/init.h>
>>   #include <linux/utsname.h>
>>   #include <linux/vmalloc.h>
>> +#include <linux/highmem.h>
> 
> Wondering where highmem.h is needed. Just curious.

Ahh, I missed that. At one point in time we moved map_crash_pages() into this file, which brought 
highmem.h along with it. But we have since moved map_crash_pages() into x86/crash.c. And I missed 
eliminating highmem.h at that time.

I have removed this for v9.

eric


> 
>> +#include <linux/memory.h>
>> +#include <linux/cpuhotplug.h>
>>   
>>   #include <asm/page.h>
>>   #include <asm/sections.h>
>>   
>>   #include <crypto/sha1.h>
>>   
>> +#include "kexec_internal.h"
>> +
>>   /* vmcoreinfo stuff */
>>   unsigned char *vmcoreinfo_data;
>>   size_t vmcoreinfo_size;
>> @@ -491,3 +496,90 @@ static int __init crash_save_vmcoreinfo_init(void)
>>   }
>>   
>>   subsys_initcall(crash_save_vmcoreinfo_init);
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>> +							unsigned int hp_action, unsigned int cpu)
>> +{
>> +	WARN(1, "crash hotplug handler not implemented");
>> +}
>> +
>> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> +	/* Obtain lock while changing crash information */
>> +	if (!mutex_trylock(&kexec_mutex))
>> +		return;
>> +
>> +	/* Check kdump is loaded */
>> +	if (kexec_crash_image) {
>> +		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
>> +
>> +		/* Needed in order for the segments to be updated */
>> +		arch_kexec_unprotect_crashkres();
>> +
>> +		/* Flag to differentiate between normal load and hotplug */
>> +		kexec_crash_image->hotplug_event = true;
>> +
>> +		/* Now invoke arch-specific update handler */
>> +		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>> +
>> +		/* No longer handling a hotplug event */
>> +		kexec_crash_image->hotplug_event = false;
>> +
>> +		/* Change back to read-only */
>> +		arch_kexec_protect_crashkres();
>> +	}
>> +
>> +	/* Release lock now that update complete */
>> +	mutex_unlock(&kexec_mutex);
>> +}
>> +
>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>> +{
>> +	switch (val) {
>> +	case MEM_ONLINE:
>> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
>> +		break;
>> +
>> +	case MEM_OFFLINE:
>> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
>> +		break;
>> +	}
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> +	.notifier_call = crash_memhp_notifier,
>> +	.priority = 0
>> +};
>> +
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> +	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> +	int result = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>> +		register_memory_notifier(&crash_memhp_nb);
>> +
>> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
>> +		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> +									"crash/cpuhp",
>> +									crash_cpuhp_online,
>> +									crash_cpuhp_offline);
>> +
>> +	return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif
>> -- 
>> 2.27.0
>>
> 

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Baoquan He]

On 05/09/22 at 10:43am, Eric DeVolder wrote:
> 
> 
> On 5/6/22 02:12, Baoquan He wrote:
> > On 05/05/22 at 02:45pm, Eric DeVolder wrote:
> > ......
> > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > > index 256cf6db573c..f197af50def6 100644
> > > --- a/kernel/crash_core.c
> > > +++ b/kernel/crash_core.c
> > > @@ -9,12 +9,17 @@
> > >   #include <linux/init.h>
> > >   #include <linux/utsname.h>
> > >   #include <linux/vmalloc.h>
> > > +#include <linux/highmem.h>
> > 
> > Wondering where highmem.h is needed. Just curious.
> 
> Ahh, I missed that. At one point in time we moved map_crash_pages() into
> this file, which brought highmem.h along with it. But we have since moved
> map_crash_pages() into x86/crash.c. And I missed eliminating highmem.h at
> that time.
> 
> I have removed this for v9.

That's nice, and you can add my ack when repost.

Acked-by: Baoquan He <bhe@redhat.com>

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Baoquan He]

On 05/09/22 at 10:43am, Eric DeVolder wrote:
> 
> 
> On 5/6/22 02:12, Baoquan He wrote:
> > On 05/05/22 at 02:45pm, Eric DeVolder wrote:
> > ......
> > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > > index 256cf6db573c..f197af50def6 100644
> > > --- a/kernel/crash_core.c
> > > +++ b/kernel/crash_core.c
> > > @@ -9,12 +9,17 @@
> > >   #include <linux/init.h>
> > >   #include <linux/utsname.h>
> > >   #include <linux/vmalloc.h>
> > > +#include <linux/highmem.h>
> > 
> > Wondering where highmem.h is needed. Just curious.
> 
> Ahh, I missed that. At one point in time we moved map_crash_pages() into
> this file, which brought highmem.h along with it. But we have since moved
> map_crash_pages() into x86/crash.c. And I missed eliminating highmem.h at
> that time.
> 
> I have removed this for v9.

That's nice, and you can add my ack when repost.

Acked-by: Baoquan He <bhe@redhat.com>

Re: [PATCH v8 4/7] kexec: exclude elfcorehdr from the segment digest

[Baoquan He]

On 05/05/22 at 02:46pm, Eric DeVolder wrote:
> When a crash kernel is loaded via the kexec_file_load syscall, the
> kernel places the various segments (ie crash kernel, crash initrd,
> boot_params, elfcorehdr, purgatory, etc) in memory. For those
> architectures that utilize purgatory, a hash digest of the segments
> is calculated for integrity checking. This digest is embedded into
> the purgatory image prior to placing purgatory in memory.
> 
> Since hotplug events cause changes to the elfcorehdr, purgatory
> integrity checking fails (at crash time, and no kdump created).
> As a result, this change explicitly excludes the elfcorehdr segment
> from the list of segments used to create the digest. By doing so,
> this permits changes to the elfcorehdr in response to hotplug events,
> without having to also reload purgatory due to the change to the
> digest.

Remember I acked this one. Seems that is dropped, assuming no change is
made since v7. Anyway,

Acked-by: Baoquan He <bhe@redhat.com>

> 
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
>  kernel/kexec_file.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 801d0d0a5012..aacdf93c3507 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -765,6 +765,12 @@ static int kexec_calculate_store_digests(struct kimage *image)
>  	for (j = i = 0; i < image->nr_segments; i++) {
>  		struct kexec_segment *ksegment;
>  
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +		/* This segment excluded to allow future changes via hotplug */
> +		if (image->elfcorehdr_index_valid && (j == image->elfcorehdr_index))
> +			continue;
> +#endif
> +
>  		ksegment = &image->segment[i];
>  		/*
>  		 * Skip purgatory as it will be modified once we put digest
> -- 
> 2.27.0
> 

[PATCH v8 4/7] kexec: exclude elfcorehdr from the segment digest

[Baoquan He]

On 05/05/22 at 02:46pm, Eric DeVolder wrote:
> When a crash kernel is loaded via the kexec_file_load syscall, the
> kernel places the various segments (ie crash kernel, crash initrd,
> boot_params, elfcorehdr, purgatory, etc) in memory. For those
> architectures that utilize purgatory, a hash digest of the segments
> is calculated for integrity checking. This digest is embedded into
> the purgatory image prior to placing purgatory in memory.
> 
> Since hotplug events cause changes to the elfcorehdr, purgatory
> integrity checking fails (at crash time, and no kdump created).
> As a result, this change explicitly excludes the elfcorehdr segment
> from the list of segments used to create the digest. By doing so,
> this permits changes to the elfcorehdr in response to hotplug events,
> without having to also reload purgatory due to the change to the
> digest.

Remember I acked this one. Seems that is dropped, assuming no change is
made since v7. Anyway,

Acked-by: Baoquan He <bhe@redhat.com>

> 
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
>  kernel/kexec_file.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 801d0d0a5012..aacdf93c3507 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -765,6 +765,12 @@ static int kexec_calculate_store_digests(struct kimage *image)
>  	for (j = i = 0; i < image->nr_segments; i++) {
>  		struct kexec_segment *ksegment;
>  
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +		/* This segment excluded to allow future changes via hotplug */
> +		if (image->elfcorehdr_index_valid && (j == image->elfcorehdr_index))
> +			continue;
> +#endif
> +
>  		ksegment = &image->segment[i];
>  		/*
>  		 * Skip purgatory as it will be modified once we put digest
> -- 
> 2.27.0
> 

Re: [PATCH v8 5/7] kexec: exclude hot remove cpu from elfcorehdr notes

[Baoquan He]

On 05/05/22 at 02:46pm, Eric DeVolder wrote:
> Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
> still in the for_each_present_cpu() list when within the
> handle_hotplug_event(). Thus the CPU must be explicitly excluded
> when building the new list of CPUs.
> 
> This change identifies in handle_hotplug_event() the CPU to be
> excluded, and the check for excluding the CPU in
> crash_prepare_elf64_headers().
> 
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>

LGTM,

Acked-by: Baoquan He <bhe@redhat.com>

> ---
>  kernel/crash_core.c | 10 ++++++++++
>  kernel/kexec_file.c |  5 +++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index f197af50def6..7ba43f058d82 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -520,6 +520,16 @@ static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>  		/* Flag to differentiate between normal load and hotplug */
>  		kexec_crash_image->hotplug_event = true;
>  
> +		/*
> +		 * Due to use of CPUHP_AP_ONLINE_DYN, upon unplug and during
> +		 * this callback, the CPU is still in the for_each_present_cpu()
> +		 * list. Must explicitly look to exclude this CPU when building
> +		 * new list.
> +		 */
> +		kexec_crash_image->offlinecpu =
> +			(hp_action == KEXEC_CRASH_HP_REMOVE_CPU) ?
> +				cpu : KEXEC_CRASH_HP_INVALID_CPU;
> +
>  		/* Now invoke arch-specific update handler */
>  		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>  
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index aacdf93c3507..d68e5769b428 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -1314,6 +1314,11 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem,
>  
>  	/* Prepare one phdr of type PT_NOTE for each present CPU */
>  	for_each_present_cpu(cpu) {
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +		/* Skip the soon-to-be offlined cpu */
> +		if (image->hotplug_event && (cpu == image->offlinecpu))
> +			continue;
> +#endif
>  		phdr->p_type = PT_NOTE;
>  		notes_addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpu));
>  		phdr->p_offset = phdr->p_paddr = notes_addr;
> -- 
> 2.27.0
> 

[PATCH v8 5/7] kexec: exclude hot remove cpu from elfcorehdr notes

[Baoquan He]

On 05/05/22 at 02:46pm, Eric DeVolder wrote:
> Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
> still in the for_each_present_cpu() list when within the
> handle_hotplug_event(). Thus the CPU must be explicitly excluded
> when building the new list of CPUs.
> 
> This change identifies in handle_hotplug_event() the CPU to be
> excluded, and the check for excluding the CPU in
> crash_prepare_elf64_headers().
> 
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>

LGTM,

Acked-by: Baoquan He <bhe@redhat.com>

> ---
>  kernel/crash_core.c | 10 ++++++++++
>  kernel/kexec_file.c |  5 +++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index f197af50def6..7ba43f058d82 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -520,6 +520,16 @@ static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>  		/* Flag to differentiate between normal load and hotplug */
>  		kexec_crash_image->hotplug_event = true;
>  
> +		/*
> +		 * Due to use of CPUHP_AP_ONLINE_DYN, upon unplug and during
> +		 * this callback, the CPU is still in the for_each_present_cpu()
> +		 * list. Must explicitly look to exclude this CPU when building
> +		 * new list.
> +		 */
> +		kexec_crash_image->offlinecpu =
> +			(hp_action == KEXEC_CRASH_HP_REMOVE_CPU) ?
> +				cpu : KEXEC_CRASH_HP_INVALID_CPU;
> +
>  		/* Now invoke arch-specific update handler */
>  		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>  
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index aacdf93c3507..d68e5769b428 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -1314,6 +1314,11 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem,
>  
>  	/* Prepare one phdr of type PT_NOTE for each present CPU */
>  	for_each_present_cpu(cpu) {
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +		/* Skip the soon-to-be offlined cpu */
> +		if (image->hotplug_event && (cpu == image->offlinecpu))
> +			continue;
> +#endif
>  		phdr->p_type = PT_NOTE;
>  		notes_addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpu));
>  		phdr->p_offset = phdr->p_paddr = notes_addr;
> -- 
> 2.27.0
> 

Re: [PATCH v8 2/7] crash: prototype change for crash_prepare_elf64_headers

[David Hildenbrand]

On 05.05.22 20:45, Eric DeVolder wrote:
> From within crash_prepare_elf64_headers() there is a need to
> reference the struct kimage hotplug members. As such, this
> change passes the struct kimage as a parameter to the
> crash_prepare_elf64_headers().

You should make it clearer that the hotplug members will be added by
successive patches.

> 
> This is preparation for later patch, no functionality change.
> 
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> Acked-by: Baoquan He <bhe@redhat.com>

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v8 2/7] crash: prototype change for crash_prepare_elf64_headers

[David Hildenbrand]

On 05.05.22 20:45, Eric DeVolder wrote:
> From within crash_prepare_elf64_headers() there is a need to
> reference the struct kimage hotplug members. As such, this
> change passes the struct kimage as a parameter to the
> crash_prepare_elf64_headers().

You should make it clearer that the hotplug members will be added by
successive patches.

> 
> This is preparation for later patch, no functionality change.
> 
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> Acked-by: Baoquan He <bhe@redhat.com>

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[David Hildenbrand]

On 05.05.22 20:45, Eric DeVolder wrote:
> CPU and memory change notifications are received in order to
> regenerate the elfcorehdr.
> 
> To support cpu hotplug, a callback is registered to capture the
> CPUHP_AP_ONLINE_DYN online and offline events via
> cpuhp_setup_state_nocalls().
> 
> To support memory hotplug, a notifier is registered to capture the
> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
> 
> The cpu callback and memory notifiers call handle_hotplug_event()
> to handle the hot plug/unplug event. Then handle_hotplug_event()
> dispatches the event to the architecture specific
> arch_crash_handle_hotplug_event(). During the process, the
> kexec_mutex is held.
> 
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>

[...]

> +
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
> +							unsigned int hp_action, unsigned int cpu)
> +{
> +	WARN(1, "crash hotplug handler not implemented");


Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
triggering?

> +}
> +
> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> +{
> +	/* Obtain lock while changing crash information */
> +	if (!mutex_trylock(&kexec_mutex))
> +		return;

This looks wrong. What if you offline memory but for some reason the mutex
is currently locked? You'd miss updating the vmcore, which would be broken.

Why is this trylock in place? Some workaround for potential locking issues,
or what is the rationale?

> +
> +	/* Check kdump is loaded */
> +	if (kexec_crash_image) {
> +		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
> +
> +		/* Needed in order for the segments to be updated */
> +		arch_kexec_unprotect_crashkres();
> +
> +		/* Flag to differentiate between normal load and hotplug */
> +		kexec_crash_image->hotplug_event = true;

1. Why is that required? Why can't arch_crash_handle_hotplug_event() forward that
information? I mean, *hotplug* in the anme implies that the function should be
aware what's happening.

2. Why can't the unprotect+reprotect not be done inside
arch_crash_handle_hotplug_event() ? It's all arch specific either way.

IMHO, this code here should be as simple as

if (kexec_crash_image)
	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);

3. Why do we have to forward the CPU for CPU onlining/offlining but not the
memory block id (or similar) when onlining/offlining a memory block?

> +
> +		/* Now invoke arch-specific update handler */
> +		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
> +
> +		/* No longer handling a hotplug event */
> +		kexec_crash_image->hotplug_event = false;
> +
> +		/* Change back to read-only */
> +		arch_kexec_protect_crashkres();
> +	}
> +
> +	/* Release lock now that update complete */
> +	mutex_unlock(&kexec_mutex);
> +}
> +
> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> +{
> +	switch (val) {
> +	case MEM_ONLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
> +		break;
> +
> +	case MEM_OFFLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
> +		break;
> +	}
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block crash_memhp_nb = {
> +	.notifier_call = crash_memhp_notifier,
> +	.priority = 0
> +};
> +
> +static int crash_cpuhp_online(unsigned int cpu)
> +{
> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> +	return 0;
> +}
> +
> +static int crash_cpuhp_offline(unsigned int cpu)
> +{
> +	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> +	return 0;
> +}
> +
> +static int __init crash_hotplug_init(void)
> +{
> +	int result = 0;
> +
> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> +		register_memory_notifier(&crash_memhp_nb);
> +
> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> +		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> +									"crash/cpuhp",
> +									crash_cpuhp_online,
> +									crash_cpuhp_offline);

Ehm, this indentation looks very weird.

> +
> +	return result;
> +}
> +
> +subsys_initcall(crash_hotplug_init);
> +#endif


-- 
Thanks,

David / dhildenb

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[David Hildenbrand]

On 05.05.22 20:45, Eric DeVolder wrote:
> CPU and memory change notifications are received in order to
> regenerate the elfcorehdr.
> 
> To support cpu hotplug, a callback is registered to capture the
> CPUHP_AP_ONLINE_DYN online and offline events via
> cpuhp_setup_state_nocalls().
> 
> To support memory hotplug, a notifier is registered to capture the
> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
> 
> The cpu callback and memory notifiers call handle_hotplug_event()
> to handle the hot plug/unplug event. Then handle_hotplug_event()
> dispatches the event to the architecture specific
> arch_crash_handle_hotplug_event(). During the process, the
> kexec_mutex is held.
> 
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>

[...]

> +
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
> +							unsigned int hp_action, unsigned int cpu)
> +{
> +	WARN(1, "crash hotplug handler not implemented");


Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
triggering?

> +}
> +
> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> +{
> +	/* Obtain lock while changing crash information */
> +	if (!mutex_trylock(&kexec_mutex))
> +		return;

This looks wrong. What if you offline memory but for some reason the mutex
is currently locked? You'd miss updating the vmcore, which would be broken.

Why is this trylock in place? Some workaround for potential locking issues,
or what is the rationale?

> +
> +	/* Check kdump is loaded */
> +	if (kexec_crash_image) {
> +		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
> +
> +		/* Needed in order for the segments to be updated */
> +		arch_kexec_unprotect_crashkres();
> +
> +		/* Flag to differentiate between normal load and hotplug */
> +		kexec_crash_image->hotplug_event = true;

1. Why is that required? Why can't arch_crash_handle_hotplug_event() forward that
information? I mean, *hotplug* in the anme implies that the function should be
aware what's happening.

2. Why can't the unprotect+reprotect not be done inside
arch_crash_handle_hotplug_event() ? It's all arch specific either way.

IMHO, this code here should be as simple as

if (kexec_crash_image)
	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);

3. Why do we have to forward the CPU for CPU onlining/offlining but not the
memory block id (or similar) when onlining/offlining a memory block?

> +
> +		/* Now invoke arch-specific update handler */
> +		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
> +
> +		/* No longer handling a hotplug event */
> +		kexec_crash_image->hotplug_event = false;
> +
> +		/* Change back to read-only */
> +		arch_kexec_protect_crashkres();
> +	}
> +
> +	/* Release lock now that update complete */
> +	mutex_unlock(&kexec_mutex);
> +}
> +
> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> +{
> +	switch (val) {
> +	case MEM_ONLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
> +		break;
> +
> +	case MEM_OFFLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
> +		break;
> +	}
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block crash_memhp_nb = {
> +	.notifier_call = crash_memhp_notifier,
> +	.priority = 0
> +};
> +
> +static int crash_cpuhp_online(unsigned int cpu)
> +{
> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> +	return 0;
> +}
> +
> +static int crash_cpuhp_offline(unsigned int cpu)
> +{
> +	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> +	return 0;
> +}
> +
> +static int __init crash_hotplug_init(void)
> +{
> +	int result = 0;
> +
> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> +		register_memory_notifier(&crash_memhp_nb);
> +
> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> +		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> +									"crash/cpuhp",
> +									crash_cpuhp_online,
> +									crash_cpuhp_offline);

Ehm, this indentation looks very weird.

> +
> +	return result;
> +}
> +
> +subsys_initcall(crash_hotplug_init);
> +#endif


-- 
Thanks,

David / dhildenb

Re: [PATCH v8 2/7] crash: prototype change for crash_prepare_elf64_headers

[Eric DeVolder]

On 5/12/22 03:42, David Hildenbrand wrote:
> On 05.05.22 20:45, Eric DeVolder wrote:
>>  From within crash_prepare_elf64_headers() there is a need to
>> reference the struct kimage hotplug members. As such, this
>> change passes the struct kimage as a parameter to the
>> crash_prepare_elf64_headers().
> 
> You should make it clearer that the hotplug members will be added by
> successive patches.
I've tweaked the commit message to indicate as such.

> 
>>
>> This is preparation for later patch, no functionality change.
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> Acked-by: Baoquan He <bhe@redhat.com>
> 
> Acked-by: David Hildenbrand <david@redhat.com>
Thank you!
eric

> 

[PATCH v8 2/7] crash: prototype change for crash_prepare_elf64_headers

[Eric DeVolder]

On 5/12/22 03:42, David Hildenbrand wrote:
> On 05.05.22 20:45, Eric DeVolder wrote:
>>  From within crash_prepare_elf64_headers() there is a need to
>> reference the struct kimage hotplug members. As such, this
>> change passes the struct kimage as a parameter to the
>> crash_prepare_elf64_headers().
> 
> You should make it clearer that the hotplug members will be added by
> successive patches.
I've tweaked the commit message to indicate as such.

> 
>>
>> This is preparation for later patch, no functionality change.
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> Acked-by: Baoquan He <bhe@redhat.com>
> 
> Acked-by: David Hildenbrand <david@redhat.com>
Thank you!
eric

> 

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

David,
Great questions! See inline responses below.
eric

On 5/12/22 03:52, David Hildenbrand wrote:
> On 05.05.22 20:45, Eric DeVolder wrote:
>> CPU and memory change notifications are received in order to
>> regenerate the elfcorehdr.
>>
>> To support cpu hotplug, a callback is registered to capture the
>> CPUHP_AP_ONLINE_DYN online and offline events via
>> cpuhp_setup_state_nocalls().
>>
>> To support memory hotplug, a notifier is registered to capture the
>> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>>
>> The cpu callback and memory notifiers call handle_hotplug_event()
>> to handle the hot plug/unplug event. Then handle_hotplug_event()
>> dispatches the event to the architecture specific
>> arch_crash_handle_hotplug_event(). During the process, the
>> kexec_mutex is held.
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> 
> [...]
> 
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>> +							unsigned int hp_action, unsigned int cpu)
>> +{
>> +	WARN(1, "crash hotplug handler not implemented");
> 
> 
> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
> triggering?
> 
You're correct. What about: printk_once(KERN_DEBUG "...") ?

>> +}
>> +
>> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> +	/* Obtain lock while changing crash information */
>> +	if (!mutex_trylock(&kexec_mutex))
>> +		return;
> 
> This looks wrong. What if you offline memory but for some reason the mutex
> is currently locked? You'd miss updating the vmcore, which would be broken.
> 
> Why is this trylock in place? Some workaround for potential locking issues,
> or what is the rationale?

I took this from kernel/kexec.c:do_my_load(), but you are right, this is not
right.

      *
      * KISS: always take the mutex.
      */
     if (!mutex_trylock(&kexec_mutex))
         return -EBUSY;

This should simply be mutex_lock(&kexec_mutex).

> 
>> +
>> +	/* Check kdump is loaded */
>> +	if (kexec_crash_image) {
>> +		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
>> +
>> +		/* Needed in order for the segments to be updated */
>> +		arch_kexec_unprotect_crashkres();
>> +
>> +		/* Flag to differentiate between normal load and hotplug */
>> +		kexec_crash_image->hotplug_event = true;
> 
> 1. Why is that required? Why can't arch_crash_handle_hotplug_event() forward that
> information? I mean, *hotplug* in the anme implies that the function should be
> aware what's happening.
Member .hotplug_event is needed in crash_prepare_elf64_headers(). To date, it has made
sense (to me) to pass this parameter to crash_prepare_elf64_headers() via the
struct kimage, rather than directly. The patch "crash: prototype change for 
crash_prepare_elf64_headers" changes crash_prepare_elf64_headers() to accept the
struct kimage. If it is so desired, it could just as well be changed to pass just
this one parameter; though struct kimage seems a more useful way to go.

> 
> 2. Why can't the unprotect+reprotect not be done inside
> arch_crash_handle_hotplug_event() ? It's all arch specific either way.
> 
> IMHO, this code here should be as simple as
> 
> if (kexec_crash_image)
> 	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
> 

The intent of this code was to be generic infrastructure. Just invoking the
arch_crash_handle_hotplug_event() would certainly be as generic as it gets.
But there were a series of steps that seemed to be common, so those I hoisted
into this bit of code.

> 3. Why do we have to forward the CPU for CPU onlining/offlining but not the
> memory block id (or similar) when onlining/offlining a memory block?
 From patch "kexec: exclude hot remove cpu from elfcorehdr notes" commit message:

Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
still in the for_each_present_cpu() list when within the
handle_hotplug_event(). Thus the CPU must be explicitly excluded
when building the new list of CPUs.

This change identifies in handle_hotplug_event() the CPU to be
excluded, and the check for excluding the CPU in
crash_prepare_elf64_headers().

If there is a better CPUHP_ to use than _DYN, I'd be all for that!

> 
>> +
>> +		/* Now invoke arch-specific update handler */
>> +		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>> +
>> +		/* No longer handling a hotplug event */
>> +		kexec_crash_image->hotplug_event = false;
>> +
>> +		/* Change back to read-only */
>> +		arch_kexec_protect_crashkres();
>> +	}
>> +
>> +	/* Release lock now that update complete */
>> +	mutex_unlock(&kexec_mutex);
>> +}
>> +
>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>> +{
>> +	switch (val) {
>> +	case MEM_ONLINE:
>> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
>> +		break;
>> +
>> +	case MEM_OFFLINE:
>> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
>> +		break;
>> +	}
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> +	.notifier_call = crash_memhp_notifier,
>> +	.priority = 0
>> +};
>> +
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> +	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> +	int result = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>> +		register_memory_notifier(&crash_memhp_nb);
>> +
>> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
>> +		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> +									"crash/cpuhp",
>> +									crash_cpuhp_online,
>> +									crash_cpuhp_offline);
> 
> Ehm, this indentation looks very weird.
> 
>> +
>> +	return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif
> 
> 

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

David,
Great questions! See inline responses below.
eric

On 5/12/22 03:52, David Hildenbrand wrote:
> On 05.05.22 20:45, Eric DeVolder wrote:
>> CPU and memory change notifications are received in order to
>> regenerate the elfcorehdr.
>>
>> To support cpu hotplug, a callback is registered to capture the
>> CPUHP_AP_ONLINE_DYN online and offline events via
>> cpuhp_setup_state_nocalls().
>>
>> To support memory hotplug, a notifier is registered to capture the
>> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>>
>> The cpu callback and memory notifiers call handle_hotplug_event()
>> to handle the hot plug/unplug event. Then handle_hotplug_event()
>> dispatches the event to the architecture specific
>> arch_crash_handle_hotplug_event(). During the process, the
>> kexec_mutex is held.
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> 
> [...]
> 
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>> +							unsigned int hp_action, unsigned int cpu)
>> +{
>> +	WARN(1, "crash hotplug handler not implemented");
> 
> 
> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
> triggering?
> 
You're correct. What about: printk_once(KERN_DEBUG "...") ?

>> +}
>> +
>> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> +	/* Obtain lock while changing crash information */
>> +	if (!mutex_trylock(&kexec_mutex))
>> +		return;
> 
> This looks wrong. What if you offline memory but for some reason the mutex
> is currently locked? You'd miss updating the vmcore, which would be broken.
> 
> Why is this trylock in place? Some workaround for potential locking issues,
> or what is the rationale?

I took this from kernel/kexec.c:do_my_load(), but you are right, this is not
right.

      *
      * KISS: always take the mutex.
      */
     if (!mutex_trylock(&kexec_mutex))
         return -EBUSY;

This should simply be mutex_lock(&kexec_mutex).

> 
>> +
>> +	/* Check kdump is loaded */
>> +	if (kexec_crash_image) {
>> +		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
>> +
>> +		/* Needed in order for the segments to be updated */
>> +		arch_kexec_unprotect_crashkres();
>> +
>> +		/* Flag to differentiate between normal load and hotplug */
>> +		kexec_crash_image->hotplug_event = true;
> 
> 1. Why is that required? Why can't arch_crash_handle_hotplug_event() forward that
> information? I mean, *hotplug* in the anme implies that the function should be
> aware what's happening.
Member .hotplug_event is needed in crash_prepare_elf64_headers(). To date, it has made
sense (to me) to pass this parameter to crash_prepare_elf64_headers() via the
struct kimage, rather than directly. The patch "crash: prototype change for 
crash_prepare_elf64_headers" changes crash_prepare_elf64_headers() to accept the
struct kimage. If it is so desired, it could just as well be changed to pass just
this one parameter; though struct kimage seems a more useful way to go.

> 
> 2. Why can't the unprotect+reprotect not be done inside
> arch_crash_handle_hotplug_event() ? It's all arch specific either way.
> 
> IMHO, this code here should be as simple as
> 
> if (kexec_crash_image)
> 	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
> 

The intent of this code was to be generic infrastructure. Just invoking the
arch_crash_handle_hotplug_event() would certainly be as generic as it gets.
But there were a series of steps that seemed to be common, so those I hoisted
into this bit of code.

> 3. Why do we have to forward the CPU for CPU onlining/offlining but not the
> memory block id (or similar) when onlining/offlining a memory block?
 From patch "kexec: exclude hot remove cpu from elfcorehdr notes" commit message:

Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
still in the for_each_present_cpu() list when within the
handle_hotplug_event(). Thus the CPU must be explicitly excluded
when building the new list of CPUs.

This change identifies in handle_hotplug_event() the CPU to be
excluded, and the check for excluding the CPU in
crash_prepare_elf64_headers().

If there is a better CPUHP_ to use than _DYN, I'd be all for that!

> 
>> +
>> +		/* Now invoke arch-specific update handler */
>> +		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>> +
>> +		/* No longer handling a hotplug event */
>> +		kexec_crash_image->hotplug_event = false;
>> +
>> +		/* Change back to read-only */
>> +		arch_kexec_protect_crashkres();
>> +	}
>> +
>> +	/* Release lock now that update complete */
>> +	mutex_unlock(&kexec_mutex);
>> +}
>> +
>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>> +{
>> +	switch (val) {
>> +	case MEM_ONLINE:
>> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
>> +		break;
>> +
>> +	case MEM_OFFLINE:
>> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
>> +		break;
>> +	}
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> +	.notifier_call = crash_memhp_notifier,
>> +	.priority = 0
>> +};
>> +
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> +	handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> +	int result = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>> +		register_memory_notifier(&crash_memhp_nb);
>> +
>> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
>> +		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> +									"crash/cpuhp",
>> +									crash_cpuhp_online,
>> +									crash_cpuhp_offline);
> 
> Ehm, this indentation looks very weird.
> 
>> +
>> +	return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif
> 
> 

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Sourabh Jain]

Hello Eric,

On 06/05/22 00:15, Eric DeVolder wrote:
> CPU and memory change notifications are received in order to
> regenerate the elfcorehdr.
>
> To support cpu hotplug, a callback is registered to capture the
> CPUHP_AP_ONLINE_DYN online and offline events via
> cpuhp_setup_state_nocalls().
>
> To support memory hotplug, a notifier is registered to capture the
> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>
> The cpu callback and memory notifiers call handle_hotplug_event()
> to handle the hot plug/unplug event. Then handle_hotplug_event()
> dispatches the event to the architecture specific
> arch_crash_handle_hotplug_event(). During the process, the
> kexec_mutex is held.
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
>   include/linux/crash_core.h | 10 +++++
>   include/linux/kexec.h      |  5 +++
>   kernel/crash_core.c        | 92 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 107 insertions(+)
>
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index de62a722431e..a240f84348aa 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -84,4 +84,14 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>   int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>   		unsigned long long *crash_size, unsigned long long *crash_base);
>   
> +#define KEXEC_CRASH_HP_REMOVE_CPU		0
> +#define KEXEC_CRASH_HP_ADD_CPU			1
> +#define KEXEC_CRASH_HP_REMOVE_MEMORY	2
> +#define KEXEC_CRASH_HP_ADD_MEMORY		3
> +#define KEXEC_CRASH_HP_INVALID_CPU		-1U
> +
> +struct kimage;
> +void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
> +									unsigned int cpu);
> +
>   #endif /* LINUX_CRASH_CORE_H */
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index f93f2591fc1e..5935bc78ed7f 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -308,6 +308,11 @@ struct kimage {
>   	struct purgatory_info purgatory_info;
>   #endif
>   
> +	bool hotplug_event;
> +	unsigned int offlinecpu;
> +	bool elfcorehdr_index_valid;
> +	int elfcorehdr_index;
How about keeping above kimage elements under below config?
#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)

Thanks,
Sourabh Jain

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Sourabh Jain]

Hello Eric,

On 06/05/22 00:15, Eric DeVolder wrote:
> CPU and memory change notifications are received in order to
> regenerate the elfcorehdr.
>
> To support cpu hotplug, a callback is registered to capture the
> CPUHP_AP_ONLINE_DYN online and offline events via
> cpuhp_setup_state_nocalls().
>
> To support memory hotplug, a notifier is registered to capture the
> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>
> The cpu callback and memory notifiers call handle_hotplug_event()
> to handle the hot plug/unplug event. Then handle_hotplug_event()
> dispatches the event to the architecture specific
> arch_crash_handle_hotplug_event(). During the process, the
> kexec_mutex is held.
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
>   include/linux/crash_core.h | 10 +++++
>   include/linux/kexec.h      |  5 +++
>   kernel/crash_core.c        | 92 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 107 insertions(+)
>
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index de62a722431e..a240f84348aa 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -84,4 +84,14 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>   int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>   		unsigned long long *crash_size, unsigned long long *crash_base);
>   
> +#define KEXEC_CRASH_HP_REMOVE_CPU		0
> +#define KEXEC_CRASH_HP_ADD_CPU			1
> +#define KEXEC_CRASH_HP_REMOVE_MEMORY	2
> +#define KEXEC_CRASH_HP_ADD_MEMORY		3
> +#define KEXEC_CRASH_HP_INVALID_CPU		-1U
> +
> +struct kimage;
> +void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
> +									unsigned int cpu);
> +
>   #endif /* LINUX_CRASH_CORE_H */
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index f93f2591fc1e..5935bc78ed7f 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -308,6 +308,11 @@ struct kimage {
>   	struct purgatory_info purgatory_info;
>   #endif
>   
> +	bool hotplug_event;
> +	unsigned int offlinecpu;
> +	bool elfcorehdr_index_valid;
> +	int elfcorehdr_index;
How about keeping above kimage elements under below config?
#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)

Thanks,
Sourabh Jain

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

On 5/23/22 03:36, Sourabh Jain wrote:
> Hello Eric,
> 
> On 06/05/22 00:15, Eric DeVolder wrote:
>> CPU and memory change notifications are received in order to
>> regenerate the elfcorehdr.
>>
>> To support cpu hotplug, a callback is registered to capture the
>> CPUHP_AP_ONLINE_DYN online and offline events via
>> cpuhp_setup_state_nocalls().
>>
>> To support memory hotplug, a notifier is registered to capture the
>> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>>
>> The cpu callback and memory notifiers call handle_hotplug_event()
>> to handle the hot plug/unplug event. Then handle_hotplug_event()
>> dispatches the event to the architecture specific
>> arch_crash_handle_hotplug_event(). During the process, the
>> kexec_mutex is held.
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> ---
>>   include/linux/crash_core.h | 10 +++++
>>   include/linux/kexec.h      |  5 +++
>>   kernel/crash_core.c        | 92 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 107 insertions(+)
>>
>> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
>> index de62a722431e..a240f84348aa 100644
>> --- a/include/linux/crash_core.h
>> +++ b/include/linux/crash_core.h
>> @@ -84,4 +84,14 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>>   int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>>           unsigned long long *crash_size, unsigned long long *crash_base);
>> +#define KEXEC_CRASH_HP_REMOVE_CPU        0
>> +#define KEXEC_CRASH_HP_ADD_CPU            1
>> +#define KEXEC_CRASH_HP_REMOVE_MEMORY    2
>> +#define KEXEC_CRASH_HP_ADD_MEMORY        3
>> +#define KEXEC_CRASH_HP_INVALID_CPU        -1U
>> +
>> +struct kimage;
>> +void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
>> +                                    unsigned int cpu);
>> +
>>   #endif /* LINUX_CRASH_CORE_H */
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index f93f2591fc1e..5935bc78ed7f 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -308,6 +308,11 @@ struct kimage {
>>       struct purgatory_info purgatory_info;
>>   #endif
>> +    bool hotplug_event;
>> +    unsigned int offlinecpu;
>> +    bool elfcorehdr_index_valid;
>> +    int elfcorehdr_index;
> How about keeping above kimage elements under below config?
> #if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> 
> Thanks,
> Sourabh Jain

Yes, of course.
Eric

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

On 5/23/22 03:36, Sourabh Jain wrote:
> Hello Eric,
> 
> On 06/05/22 00:15, Eric DeVolder wrote:
>> CPU and memory change notifications are received in order to
>> regenerate the elfcorehdr.
>>
>> To support cpu hotplug, a callback is registered to capture the
>> CPUHP_AP_ONLINE_DYN online and offline events via
>> cpuhp_setup_state_nocalls().
>>
>> To support memory hotplug, a notifier is registered to capture the
>> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>>
>> The cpu callback and memory notifiers call handle_hotplug_event()
>> to handle the hot plug/unplug event. Then handle_hotplug_event()
>> dispatches the event to the architecture specific
>> arch_crash_handle_hotplug_event(). During the process, the
>> kexec_mutex is held.
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> ---
>> ? include/linux/crash_core.h | 10 +++++
>> ? include/linux/kexec.h????? |? 5 +++
>> ? kernel/crash_core.c??????? | 92 ++++++++++++++++++++++++++++++++++++++
>> ? 3 files changed, 107 insertions(+)
>>
>> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
>> index de62a722431e..a240f84348aa 100644
>> --- a/include/linux/crash_core.h
>> +++ b/include/linux/crash_core.h
>> @@ -84,4 +84,14 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>> ? int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>> ????????? unsigned long long *crash_size, unsigned long long *crash_base);
>> +#define KEXEC_CRASH_HP_REMOVE_CPU??????? 0
>> +#define KEXEC_CRASH_HP_ADD_CPU??????????? 1
>> +#define KEXEC_CRASH_HP_REMOVE_MEMORY??? 2
>> +#define KEXEC_CRASH_HP_ADD_MEMORY??????? 3
>> +#define KEXEC_CRASH_HP_INVALID_CPU??????? -1U
>> +
>> +struct kimage;
>> +void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
>> +??????????????????????????????????? unsigned int cpu);
>> +
>> ? #endif /* LINUX_CRASH_CORE_H */
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index f93f2591fc1e..5935bc78ed7f 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -308,6 +308,11 @@ struct kimage {
>> ????? struct purgatory_info purgatory_info;
>> ? #endif
>> +??? bool hotplug_event;
>> +??? unsigned int offlinecpu;
>> +??? bool elfcorehdr_index_valid;
>> +??? int elfcorehdr_index;
> How about keeping above kimage elements under below config?
> #if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> 
> Thanks,
> Sourabh Jain

Yes, of course.
Eric

Re: [PATCH v8 6/7] x86/crash: Add x86 crash hotplug support for kexec_file_load

[Sourabh Jain]

Hello Eric,

On 06/05/22 00:16, Eric DeVolder wrote:
> For x86_64, when CPU or memory is hot un/plugged, the crash
> elfcorehdr, which describes the CPUs and memory in the system,
> must also be updated.
>
> To update the elfcorehdr for x86_64, a new elfcorehdr must be
> generated from the available CPUs and memory. The new elfcorehdr
> is prepared into a buffer, and then installed over the top of
> the existing elfcorehdr.
>
> In the patch 'kexec: exclude elfcorehdr from the segment digest'
> the need to update purgatory due to the change in elfcorehdr was
> eliminated.  As a result, no changes to purgatory or boot_params
> (as the elfcorehdr= kernel command line parameter pointer
> remains unchanged and correct) are needed, just elfcorehdr.
>
> To accommodate a growing number of resources via hotplug, the
> elfcorehdr segment must be sufficiently large enough to accommodate
> changes, see the CRASH_MAX_MEMORY_RANGES configure item.
>
> With this change, crash hotplug for kexec_file_load syscall
> is supported. When loading the crash kernel via kexec_file_load,
> the elfcorehdr is identified at load time in crash_load_segments().
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
>   arch/x86/Kconfig        |  11 ++++
>   arch/x86/kernel/crash.c | 117 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 128 insertions(+)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 4bed3abf444d..bf1201fe6981 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2072,6 +2072,17 @@ config CRASH_DUMP
>   	  (CONFIG_RELOCATABLE=y).
>   	  For more details see Documentation/admin-guide/kdump/kdump.rst
>   
> +config CRASH_MAX_MEMORY_RANGES
> +	depends on CRASH_DUMP && KEXEC_FILE && (HOTPLUG_CPU || MEMORY_HOTPLUG)
> +	int
> +	default 32768
> +	help
> +	  For the kexec_file_load path, specify the maximum number of
> +	  memory regions, eg. as represented by the 'System RAM' entries
> +	  in /proc/iomem, that the elfcorehdr buffer/segment can accommodate.
> +	  This value is combined with NR_CPUS and multiplied by Elf64_Phdr
> +	  size to determine the final buffer size.
> +
>   config KEXEC_JUMP
>   	bool "kexec jump"
>   	depends on KEXEC && HIBERNATION
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 9db41cce8d97..951ef365f0a7 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -25,6 +25,7 @@
>   #include <linux/slab.h>
>   #include <linux/vmalloc.h>
>   #include <linux/memblock.h>
> +#include <linux/highmem.h>
>   
>   #include <asm/processor.h>
>   #include <asm/hardirq.h>
> @@ -398,7 +399,17 @@ int crash_load_segments(struct kimage *image)
>   	image->elf_headers = kbuf.buffer;
>   	image->elf_headers_sz = kbuf.bufsz;
>   
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +	/* Ensure elfcorehdr segment large enough for hotplug changes */
> +	kbuf.memsz = (CONFIG_NR_CPUS_DEFAULT + CONFIG_CRASH_MAX_MEMORY_RANGES) * sizeof(Elf64_Phdr);
> +	/* For marking as usable to crash kernel */
> +	image->elf_headers_sz = kbuf.memsz;
> +	/* Record the index of the elfcorehdr segment */
> +	image->elfcorehdr_index = image->nr_segments;
> +	image->elfcorehdr_index_valid = true;
> +#else
>   	kbuf.memsz = kbuf.bufsz;
> +#endif
>   	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
>   	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>   	ret = kexec_add_buffer(&kbuf);
> @@ -413,3 +424,109 @@ int crash_load_segments(struct kimage *image)
>   	return ret;
>   }
>   #endif /* CONFIG_KEXEC_FILE */
> +
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +static void *map_crash_pages(unsigned long paddr, unsigned long size)
> +{
> +	/*
> +	 * NOTE: The addresses and sizes passed to this routine have
> +	 * already been fully aligned on page boundaries. There is no
> +	 * need for massaging the address or size.
> +	 */
> +	void *ptr = NULL;
> +
> +	/* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
> +	if (size > 0) {
> +		struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
> +
> +		ptr = kmap(page);
> +	}
> +
> +	return ptr;
> +}
> +
> +static void unmap_crash_pages(void **ptr)
> +{
> +	if (ptr) {
> +		if (*ptr)
> +			kunmap(*ptr);
> +		*ptr = NULL;
> +	}
> +}
> +
> +/**
> + * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
> + * @image: the active struct kimage
> + * @hp_action: the hot un/plug action being handled
> + * @cpu: when KEXEC_CRASH_HP_ADD/REMOVE_CPU, the cpu affected
> + *
> + * To accurately reflect hot un/plug changes, the elfcorehdr (which
> + * is passed to the crash kernel via the elfcorehdr= parameter)
> + * must be updated with the new list of CPUs and memories. The new
> + * elfcorehdr is prepared in a kernel buffer, and then it is
> + * written on top of the existing/old elfcorehdr.
> + *
> + * For hotplug changes to elfcorehdr to work, two conditions are
> + * needed:
> + * First, the segment containing the elfcorehdr must be large enough
> + * to permit a growing number of resources. See the
> + * CONFIG_CRASH_MAX_MEMORY_RANGES description.
> + * Second, purgatory must explicitly exclude the elfcorehdr from the
> + * list of segments it checks (since the elfcorehdr changes and thus
> + * would require an update to purgatory itself to update the digest).
> + *
> + */
> +void arch_crash_handle_hotplug_event(struct kimage *image,
> +	unsigned int hp_action, unsigned int cpu)
> +{
> +	struct kexec_segment *ksegment;
> +	unsigned char *ptr = NULL;
> +	unsigned long elfsz = 0;
> +	void *elfbuf = NULL;
> +	unsigned long mem, memsz;
> +
> +	if (!image->elfcorehdr_index_valid) {
> +		pr_err("crash hp: unable to locate elfcorehdr segment");
> +		goto out;
> +	}
> +
> +	ksegment = &image->segment[image->elfcorehdr_index];
> +	mem = ksegment->mem;
> +	memsz = ksegment->memsz;
> +
> +	/*
> +	 * Create the new elfcorehdr reflecting the changes to CPU and/or
> +	 * memory resources.
> +	 */
> +	if (prepare_elf_headers(image, &elfbuf, &elfsz)) {
> +		pr_err("crash hp: unable to prepare elfcore headers");
> +		goto out;

Prepare_elf_header uses crash_prepare_elf64_headers function to generate elfcorehdr.
Since crash_prepare_elf64_headers is defined under CONFIG_KEXEC_FILE option we
will have build issues if CONFIG_KEXEC is enabled and CONFIG_KEXEC_FILE is disabled.

How about moving crash_prepare_elf64_headers function to kernel/kexec_core.c?
  
Thanks,
Sourabh Jain

[PATCH v8 6/7] x86/crash: Add x86 crash hotplug support for kexec_file_load

[Sourabh Jain]

Hello Eric,

On 06/05/22 00:16, Eric DeVolder wrote:
> For x86_64, when CPU or memory is hot un/plugged, the crash
> elfcorehdr, which describes the CPUs and memory in the system,
> must also be updated.
>
> To update the elfcorehdr for x86_64, a new elfcorehdr must be
> generated from the available CPUs and memory. The new elfcorehdr
> is prepared into a buffer, and then installed over the top of
> the existing elfcorehdr.
>
> In the patch 'kexec: exclude elfcorehdr from the segment digest'
> the need to update purgatory due to the change in elfcorehdr was
> eliminated.  As a result, no changes to purgatory or boot_params
> (as the elfcorehdr= kernel command line parameter pointer
> remains unchanged and correct) are needed, just elfcorehdr.
>
> To accommodate a growing number of resources via hotplug, the
> elfcorehdr segment must be sufficiently large enough to accommodate
> changes, see the CRASH_MAX_MEMORY_RANGES configure item.
>
> With this change, crash hotplug for kexec_file_load syscall
> is supported. When loading the crash kernel via kexec_file_load,
> the elfcorehdr is identified at load time in crash_load_segments().
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
>   arch/x86/Kconfig        |  11 ++++
>   arch/x86/kernel/crash.c | 117 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 128 insertions(+)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 4bed3abf444d..bf1201fe6981 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2072,6 +2072,17 @@ config CRASH_DUMP
>   	  (CONFIG_RELOCATABLE=y).
>   	  For more details see Documentation/admin-guide/kdump/kdump.rst
>   
> +config CRASH_MAX_MEMORY_RANGES
> +	depends on CRASH_DUMP && KEXEC_FILE && (HOTPLUG_CPU || MEMORY_HOTPLUG)
> +	int
> +	default 32768
> +	help
> +	  For the kexec_file_load path, specify the maximum number of
> +	  memory regions, eg. as represented by the 'System RAM' entries
> +	  in /proc/iomem, that the elfcorehdr buffer/segment can accommodate.
> +	  This value is combined with NR_CPUS and multiplied by Elf64_Phdr
> +	  size to determine the final buffer size.
> +
>   config KEXEC_JUMP
>   	bool "kexec jump"
>   	depends on KEXEC && HIBERNATION
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 9db41cce8d97..951ef365f0a7 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -25,6 +25,7 @@
>   #include <linux/slab.h>
>   #include <linux/vmalloc.h>
>   #include <linux/memblock.h>
> +#include <linux/highmem.h>
>   
>   #include <asm/processor.h>
>   #include <asm/hardirq.h>
> @@ -398,7 +399,17 @@ int crash_load_segments(struct kimage *image)
>   	image->elf_headers = kbuf.buffer;
>   	image->elf_headers_sz = kbuf.bufsz;
>   
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +	/* Ensure elfcorehdr segment large enough for hotplug changes */
> +	kbuf.memsz = (CONFIG_NR_CPUS_DEFAULT + CONFIG_CRASH_MAX_MEMORY_RANGES) * sizeof(Elf64_Phdr);
> +	/* For marking as usable to crash kernel */
> +	image->elf_headers_sz = kbuf.memsz;
> +	/* Record the index of the elfcorehdr segment */
> +	image->elfcorehdr_index = image->nr_segments;
> +	image->elfcorehdr_index_valid = true;
> +#else
>   	kbuf.memsz = kbuf.bufsz;
> +#endif
>   	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
>   	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>   	ret = kexec_add_buffer(&kbuf);
> @@ -413,3 +424,109 @@ int crash_load_segments(struct kimage *image)
>   	return ret;
>   }
>   #endif /* CONFIG_KEXEC_FILE */
> +
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +static void *map_crash_pages(unsigned long paddr, unsigned long size)
> +{
> +	/*
> +	 * NOTE: The addresses and sizes passed to this routine have
> +	 * already been fully aligned on page boundaries. There is no
> +	 * need for massaging the address or size.
> +	 */
> +	void *ptr = NULL;
> +
> +	/* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
> +	if (size > 0) {
> +		struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
> +
> +		ptr = kmap(page);
> +	}
> +
> +	return ptr;
> +}
> +
> +static void unmap_crash_pages(void **ptr)
> +{
> +	if (ptr) {
> +		if (*ptr)
> +			kunmap(*ptr);
> +		*ptr = NULL;
> +	}
> +}
> +
> +/**
> + * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
> + * @image: the active struct kimage
> + * @hp_action: the hot un/plug action being handled
> + * @cpu: when KEXEC_CRASH_HP_ADD/REMOVE_CPU, the cpu affected
> + *
> + * To accurately reflect hot un/plug changes, the elfcorehdr (which
> + * is passed to the crash kernel via the elfcorehdr= parameter)
> + * must be updated with the new list of CPUs and memories. The new
> + * elfcorehdr is prepared in a kernel buffer, and then it is
> + * written on top of the existing/old elfcorehdr.
> + *
> + * For hotplug changes to elfcorehdr to work, two conditions are
> + * needed:
> + * First, the segment containing the elfcorehdr must be large enough
> + * to permit a growing number of resources. See the
> + * CONFIG_CRASH_MAX_MEMORY_RANGES description.
> + * Second, purgatory must explicitly exclude the elfcorehdr from the
> + * list of segments it checks (since the elfcorehdr changes and thus
> + * would require an update to purgatory itself to update the digest).
> + *
> + */
> +void arch_crash_handle_hotplug_event(struct kimage *image,
> +	unsigned int hp_action, unsigned int cpu)
> +{
> +	struct kexec_segment *ksegment;
> +	unsigned char *ptr = NULL;
> +	unsigned long elfsz = 0;
> +	void *elfbuf = NULL;
> +	unsigned long mem, memsz;
> +
> +	if (!image->elfcorehdr_index_valid) {
> +		pr_err("crash hp: unable to locate elfcorehdr segment");
> +		goto out;
> +	}
> +
> +	ksegment = &image->segment[image->elfcorehdr_index];
> +	mem = ksegment->mem;
> +	memsz = ksegment->memsz;
> +
> +	/*
> +	 * Create the new elfcorehdr reflecting the changes to CPU and/or
> +	 * memory resources.
> +	 */
> +	if (prepare_elf_headers(image, &elfbuf, &elfsz)) {
> +		pr_err("crash hp: unable to prepare elfcore headers");
> +		goto out;

Prepare_elf_header uses crash_prepare_elf64_headers function to generate elfcorehdr.
Since crash_prepare_elf64_headers is defined under CONFIG_KEXEC_FILE option we
will have build issues if CONFIG_KEXEC is enabled and CONFIG_KEXEC_FILE is disabled.

How about moving crash_prepare_elf64_headers function to kernel/kexec_core.c?
  
Thanks,
Sourabh Jain

Re: [PATCH v8 6/7] x86/crash: Add x86 crash hotplug support for kexec_file_load

[Eric DeVolder]

On 5/25/22 00:25, Sourabh Jain wrote:
> Hello Eric,
> 
> On 06/05/22 00:16, Eric DeVolder wrote:
>> For x86_64, when CPU or memory is hot un/plugged, the crash
>> elfcorehdr, which describes the CPUs and memory in the system,
>> must also be updated.
>>
>> To update the elfcorehdr for x86_64, a new elfcorehdr must be
>> generated from the available CPUs and memory. The new elfcorehdr
>> is prepared into a buffer, and then installed over the top of
>> the existing elfcorehdr.
>>
>> In the patch 'kexec: exclude elfcorehdr from the segment digest'
>> the need to update purgatory due to the change in elfcorehdr was
>> eliminated.  As a result, no changes to purgatory or boot_params
>> (as the elfcorehdr= kernel command line parameter pointer
>> remains unchanged and correct) are needed, just elfcorehdr.
>>
>> To accommodate a growing number of resources via hotplug, the
>> elfcorehdr segment must be sufficiently large enough to accommodate
>> changes, see the CRASH_MAX_MEMORY_RANGES configure item.
>>
>> With this change, crash hotplug for kexec_file_load syscall
>> is supported. When loading the crash kernel via kexec_file_load,
>> the elfcorehdr is identified at load time in crash_load_segments().
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> ---
>>   arch/x86/Kconfig        |  11 ++++
>>   arch/x86/kernel/crash.c | 117 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 128 insertions(+)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 4bed3abf444d..bf1201fe6981 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2072,6 +2072,17 @@ config CRASH_DUMP
>>         (CONFIG_RELOCATABLE=y).
>>         For more details see Documentation/admin-guide/kdump/kdump.rst
>> +config CRASH_MAX_MEMORY_RANGES
>> +    depends on CRASH_DUMP && KEXEC_FILE && (HOTPLUG_CPU || MEMORY_HOTPLUG)
>> +    int
>> +    default 32768
>> +    help
>> +      For the kexec_file_load path, specify the maximum number of
>> +      memory regions, eg. as represented by the 'System RAM' entries
>> +      in /proc/iomem, that the elfcorehdr buffer/segment can accommodate.
>> +      This value is combined with NR_CPUS and multiplied by Elf64_Phdr
>> +      size to determine the final buffer size.
>> +
>>   config KEXEC_JUMP
>>       bool "kexec jump"
>>       depends on KEXEC && HIBERNATION
>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>> index 9db41cce8d97..951ef365f0a7 100644
>> --- a/arch/x86/kernel/crash.c
>> +++ b/arch/x86/kernel/crash.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/vmalloc.h>
>>   #include <linux/memblock.h>
>> +#include <linux/highmem.h>
>>   #include <asm/processor.h>
>>   #include <asm/hardirq.h>
>> @@ -398,7 +399,17 @@ int crash_load_segments(struct kimage *image)
>>       image->elf_headers = kbuf.buffer;
>>       image->elf_headers_sz = kbuf.bufsz;
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +    /* Ensure elfcorehdr segment large enough for hotplug changes */
>> +    kbuf.memsz = (CONFIG_NR_CPUS_DEFAULT + CONFIG_CRASH_MAX_MEMORY_RANGES) * sizeof(Elf64_Phdr);
>> +    /* For marking as usable to crash kernel */
>> +    image->elf_headers_sz = kbuf.memsz;
>> +    /* Record the index of the elfcorehdr segment */
>> +    image->elfcorehdr_index = image->nr_segments;
>> +    image->elfcorehdr_index_valid = true;
>> +#else
>>       kbuf.memsz = kbuf.bufsz;
>> +#endif
>>       kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
>>       kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>>       ret = kexec_add_buffer(&kbuf);
>> @@ -413,3 +424,109 @@ int crash_load_segments(struct kimage *image)
>>       return ret;
>>   }
>>   #endif /* CONFIG_KEXEC_FILE */
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +static void *map_crash_pages(unsigned long paddr, unsigned long size)
>> +{
>> +    /*
>> +     * NOTE: The addresses and sizes passed to this routine have
>> +     * already been fully aligned on page boundaries. There is no
>> +     * need for massaging the address or size.
>> +     */
>> +    void *ptr = NULL;
>> +
>> +    /* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
>> +    if (size > 0) {
>> +        struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
>> +
>> +        ptr = kmap(page);
>> +    }
>> +
>> +    return ptr;
>> +}
>> +
>> +static void unmap_crash_pages(void **ptr)
>> +{
>> +    if (ptr) {
>> +        if (*ptr)
>> +            kunmap(*ptr);
>> +        *ptr = NULL;
>> +    }
>> +}
>> +
>> +/**
>> + * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
>> + * @image: the active struct kimage
>> + * @hp_action: the hot un/plug action being handled
>> + * @cpu: when KEXEC_CRASH_HP_ADD/REMOVE_CPU, the cpu affected
>> + *
>> + * To accurately reflect hot un/plug changes, the elfcorehdr (which
>> + * is passed to the crash kernel via the elfcorehdr= parameter)
>> + * must be updated with the new list of CPUs and memories. The new
>> + * elfcorehdr is prepared in a kernel buffer, and then it is
>> + * written on top of the existing/old elfcorehdr.
>> + *
>> + * For hotplug changes to elfcorehdr to work, two conditions are
>> + * needed:
>> + * First, the segment containing the elfcorehdr must be large enough
>> + * to permit a growing number of resources. See the
>> + * CONFIG_CRASH_MAX_MEMORY_RANGES description.
>> + * Second, purgatory must explicitly exclude the elfcorehdr from the
>> + * list of segments it checks (since the elfcorehdr changes and thus
>> + * would require an update to purgatory itself to update the digest).
>> + *
>> + */
>> +void arch_crash_handle_hotplug_event(struct kimage *image,
>> +    unsigned int hp_action, unsigned int cpu)
>> +{
>> +    struct kexec_segment *ksegment;
>> +    unsigned char *ptr = NULL;
>> +    unsigned long elfsz = 0;
>> +    void *elfbuf = NULL;
>> +    unsigned long mem, memsz;
>> +
>> +    if (!image->elfcorehdr_index_valid) {
>> +        pr_err("crash hp: unable to locate elfcorehdr segment");
>> +        goto out;
>> +    }
>> +
>> +    ksegment = &image->segment[image->elfcorehdr_index];
>> +    mem = ksegment->mem;
>> +    memsz = ksegment->memsz;
>> +
>> +    /*
>> +     * Create the new elfcorehdr reflecting the changes to CPU and/or
>> +     * memory resources.
>> +     */
>> +    if (prepare_elf_headers(image, &elfbuf, &elfsz)) {
>> +        pr_err("crash hp: unable to prepare elfcore headers");
>> +        goto out;
> 
> Prepare_elf_header uses crash_prepare_elf64_headers function to generate elfcorehdr.
> Since crash_prepare_elf64_headers is defined under CONFIG_KEXEC_FILE option we
> will have build issues if CONFIG_KEXEC is enabled and CONFIG_KEXEC_FILE is disabled.
> 
> How about moving crash_prepare_elf64_headers function to kernel/kexec_core.c?

Yes, of course. I originally protected against this scenario with having the
CRASH_HOTPLUG dependent on KEXEC_FILE, but as we eliminated CRASH_HOTPLUG, your
suggestion above is on point.

Thanks!
eric

> 
> Thanks,
> Sourabh Jain
> 

[PATCH v8 6/7] x86/crash: Add x86 crash hotplug support for kexec_file_load

[Eric DeVolder]

On 5/25/22 00:25, Sourabh Jain wrote:
> Hello Eric,
> 
> On 06/05/22 00:16, Eric DeVolder wrote:
>> For x86_64, when CPU or memory is hot un/plugged, the crash
>> elfcorehdr, which describes the CPUs and memory in the system,
>> must also be updated.
>>
>> To update the elfcorehdr for x86_64, a new elfcorehdr must be
>> generated from the available CPUs and memory. The new elfcorehdr
>> is prepared into a buffer, and then installed over the top of
>> the existing elfcorehdr.
>>
>> In the patch 'kexec: exclude elfcorehdr from the segment digest'
>> the need to update purgatory due to the change in elfcorehdr was
>> eliminated.? As a result, no changes to purgatory or boot_params
>> (as the elfcorehdr= kernel command line parameter pointer
>> remains unchanged and correct) are needed, just elfcorehdr.
>>
>> To accommodate a growing number of resources via hotplug, the
>> elfcorehdr segment must be sufficiently large enough to accommodate
>> changes, see the CRASH_MAX_MEMORY_RANGES configure item.
>>
>> With this change, crash hotplug for kexec_file_load syscall
>> is supported. When loading the crash kernel via kexec_file_load,
>> the elfcorehdr is identified at load time in crash_load_segments().
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> ---
>> ? arch/x86/Kconfig??????? |? 11 ++++
>> ? arch/x86/kernel/crash.c | 117 ++++++++++++++++++++++++++++++++++++++++
>> ? 2 files changed, 128 insertions(+)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 4bed3abf444d..bf1201fe6981 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2072,6 +2072,17 @@ config CRASH_DUMP
>> ??????? (CONFIG_RELOCATABLE=y).
>> ??????? For more details see Documentation/admin-guide/kdump/kdump.rst
>> +config CRASH_MAX_MEMORY_RANGES
>> +??? depends on CRASH_DUMP && KEXEC_FILE && (HOTPLUG_CPU || MEMORY_HOTPLUG)
>> +??? int
>> +??? default 32768
>> +??? help
>> +????? For the kexec_file_load path, specify the maximum number of
>> +????? memory regions, eg. as represented by the 'System RAM' entries
>> +????? in /proc/iomem, that the elfcorehdr buffer/segment can accommodate.
>> +????? This value is combined with NR_CPUS and multiplied by Elf64_Phdr
>> +????? size to determine the final buffer size.
>> +
>> ? config KEXEC_JUMP
>> ????? bool "kexec jump"
>> ????? depends on KEXEC && HIBERNATION
>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>> index 9db41cce8d97..951ef365f0a7 100644
>> --- a/arch/x86/kernel/crash.c
>> +++ b/arch/x86/kernel/crash.c
>> @@ -25,6 +25,7 @@
>> ? #include <linux/slab.h>
>> ? #include <linux/vmalloc.h>
>> ? #include <linux/memblock.h>
>> +#include <linux/highmem.h>
>> ? #include <asm/processor.h>
>> ? #include <asm/hardirq.h>
>> @@ -398,7 +399,17 @@ int crash_load_segments(struct kimage *image)
>> ????? image->elf_headers = kbuf.buffer;
>> ????? image->elf_headers_sz = kbuf.bufsz;
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +??? /* Ensure elfcorehdr segment large enough for hotplug changes */
>> +??? kbuf.memsz = (CONFIG_NR_CPUS_DEFAULT + CONFIG_CRASH_MAX_MEMORY_RANGES) * sizeof(Elf64_Phdr);
>> +??? /* For marking as usable to crash kernel */
>> +??? image->elf_headers_sz = kbuf.memsz;
>> +??? /* Record the index of the elfcorehdr segment */
>> +??? image->elfcorehdr_index = image->nr_segments;
>> +??? image->elfcorehdr_index_valid = true;
>> +#else
>> ????? kbuf.memsz = kbuf.bufsz;
>> +#endif
>> ????? kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
>> ????? kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> ????? ret = kexec_add_buffer(&kbuf);
>> @@ -413,3 +424,109 @@ int crash_load_segments(struct kimage *image)
>> ????? return ret;
>> ? }
>> ? #endif /* CONFIG_KEXEC_FILE */
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +static void *map_crash_pages(unsigned long paddr, unsigned long size)
>> +{
>> +??? /*
>> +???? * NOTE: The addresses and sizes passed to this routine have
>> +???? * already been fully aligned on page boundaries. There is no
>> +???? * need for massaging the address or size.
>> +???? */
>> +??? void *ptr = NULL;
>> +
>> +??? /* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
>> +??? if (size > 0) {
>> +??????? struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
>> +
>> +??????? ptr = kmap(page);
>> +??? }
>> +
>> +??? return ptr;
>> +}
>> +
>> +static void unmap_crash_pages(void **ptr)
>> +{
>> +??? if (ptr) {
>> +??????? if (*ptr)
>> +??????????? kunmap(*ptr);
>> +??????? *ptr = NULL;
>> +??? }
>> +}
>> +
>> +/**
>> + * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
>> + * @image: the active struct kimage
>> + * @hp_action: the hot un/plug action being handled
>> + * @cpu: when KEXEC_CRASH_HP_ADD/REMOVE_CPU, the cpu affected
>> + *
>> + * To accurately reflect hot un/plug changes, the elfcorehdr (which
>> + * is passed to the crash kernel via the elfcorehdr= parameter)
>> + * must be updated with the new list of CPUs and memories. The new
>> + * elfcorehdr is prepared in a kernel buffer, and then it is
>> + * written on top of the existing/old elfcorehdr.
>> + *
>> + * For hotplug changes to elfcorehdr to work, two conditions are
>> + * needed:
>> + * First, the segment containing the elfcorehdr must be large enough
>> + * to permit a growing number of resources. See the
>> + * CONFIG_CRASH_MAX_MEMORY_RANGES description.
>> + * Second, purgatory must explicitly exclude the elfcorehdr from the
>> + * list of segments it checks (since the elfcorehdr changes and thus
>> + * would require an update to purgatory itself to update the digest).
>> + *
>> + */
>> +void arch_crash_handle_hotplug_event(struct kimage *image,
>> +??? unsigned int hp_action, unsigned int cpu)
>> +{
>> +??? struct kexec_segment *ksegment;
>> +??? unsigned char *ptr = NULL;
>> +??? unsigned long elfsz = 0;
>> +??? void *elfbuf = NULL;
>> +??? unsigned long mem, memsz;
>> +
>> +??? if (!image->elfcorehdr_index_valid) {
>> +??????? pr_err("crash hp: unable to locate elfcorehdr segment");
>> +??????? goto out;
>> +??? }
>> +
>> +??? ksegment = &image->segment[image->elfcorehdr_index];
>> +??? mem = ksegment->mem;
>> +??? memsz = ksegment->memsz;
>> +
>> +??? /*
>> +???? * Create the new elfcorehdr reflecting the changes to CPU and/or
>> +???? * memory resources.
>> +???? */
>> +??? if (prepare_elf_headers(image, &elfbuf, &elfsz)) {
>> +??????? pr_err("crash hp: unable to prepare elfcore headers");
>> +??????? goto out;
> 
> Prepare_elf_header uses crash_prepare_elf64_headers function to generate elfcorehdr.
> Since crash_prepare_elf64_headers is defined under CONFIG_KEXEC_FILE option we
> will have build issues if CONFIG_KEXEC is enabled and CONFIG_KEXEC_FILE is disabled.
> 
> How about moving crash_prepare_elf64_headers function to kernel/kexec_core.c?

Yes, of course. I originally protected against this scenario with having the
CRASH_HOTPLUG dependent on KEXEC_FILE, but as we eliminated CRASH_HOTPLUG, your
suggestion above is on point.

Thanks!
eric

> 
> Thanks,
> Sourabh Jain
> 

Re: [PATCH v8 7/7] x86/crash: Add x86 crash hotplug support for kexec_load

[Sourabh Jain]

Hello Eric,

On 06/05/22 00:16, Eric DeVolder wrote:
> For kexec_file_load support, the loading of the crash kernel occurs
> entirely within the kernel, and as such the elfcorehdr is readily
> identified (so that it can be modified upon hotplug events).
>
> This change enables support for kexec_load by identifying the
> elfcorehdr segment in the arch_crash_handle_hotplug_event(),
> if it has not already been identified.
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> Acked-by: Baoquan He <bhe@redhat.com>
> ---
>   arch/x86/kernel/crash.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 951ef365f0a7..845d7c77854d 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -485,6 +485,30 @@ void arch_crash_handle_hotplug_event(struct kimage *image,
>   	void *elfbuf = NULL;
>   	unsigned long mem, memsz;
>   
> +	/*
> +	 * When the struct kimage is alloced, it is wiped to zero, so
> +	 * the elfcorehdr_index_valid defaults to false. It is set on the
> +	 * kexec_file_load path, or here for kexec_load, if not already
> +	 * identified.
> +	 */
> +	if (!image->elfcorehdr_index_valid) {
> +		unsigned int n;
> +
> +		for (n = 0; n < image->nr_segments; n++) {
> +			mem = image->segment[n].mem;
> +			memsz = image->segment[n].memsz;
> +			ptr = map_crash_pages(mem, memsz);
> +			if (ptr) {
> +				/* The segment containing elfcorehdr */
> +				if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
> +					image->elfcorehdr_index = (int)n;
> +					image->elfcorehdr_index_valid = true;

How about finding elfcorehdr index on kexec_load path post 
kimage_load_segment call in
do_kexec_load (kernel/kexec.c) or other suitable place? This way we can 
avoid checking for
elfcorehdr index for every hotplug. Also we might not need 
image->elfcorehdr_index_valid.

Thanks,
Sourabh Jain

[PATCH v8 7/7] x86/crash: Add x86 crash hotplug support for kexec_load

[Sourabh Jain]

Hello Eric,

On 06/05/22 00:16, Eric DeVolder wrote:
> For kexec_file_load support, the loading of the crash kernel occurs
> entirely within the kernel, and as such the elfcorehdr is readily
> identified (so that it can be modified upon hotplug events).
>
> This change enables support for kexec_load by identifying the
> elfcorehdr segment in the arch_crash_handle_hotplug_event(),
> if it has not already been identified.
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> Acked-by: Baoquan He <bhe@redhat.com>
> ---
>   arch/x86/kernel/crash.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 951ef365f0a7..845d7c77854d 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -485,6 +485,30 @@ void arch_crash_handle_hotplug_event(struct kimage *image,
>   	void *elfbuf = NULL;
>   	unsigned long mem, memsz;
>   
> +	/*
> +	 * When the struct kimage is alloced, it is wiped to zero, so
> +	 * the elfcorehdr_index_valid defaults to false. It is set on the
> +	 * kexec_file_load path, or here for kexec_load, if not already
> +	 * identified.
> +	 */
> +	if (!image->elfcorehdr_index_valid) {
> +		unsigned int n;
> +
> +		for (n = 0; n < image->nr_segments; n++) {
> +			mem = image->segment[n].mem;
> +			memsz = image->segment[n].memsz;
> +			ptr = map_crash_pages(mem, memsz);
> +			if (ptr) {
> +				/* The segment containing elfcorehdr */
> +				if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
> +					image->elfcorehdr_index = (int)n;
> +					image->elfcorehdr_index_valid = true;

How about finding elfcorehdr index on kexec_load path post 
kimage_load_segment call in
do_kexec_load (kernel/kexec.c) or other suitable place? This way we can 
avoid checking for
elfcorehdr index for every hotplug. Also we might not need 
image->elfcorehdr_index_valid.

Thanks,
Sourabh Jain

Re: [PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Sourabh Jain]

Hello Eric,

On 06/05/22 00:15, Eric DeVolder wrote:
> When the kdump service is loaded, if a CPU or memory is hot
> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
> and memory in the system, must also be updated, else the resulting
> vmcore is inaccurate (eg. missing either CPU context or memory
> regions).
>
> The current solution utilizes udev to initiate an unload-then-reload
> of the kdump image (e. kernel, initrd, boot_params, puratory and
> elfcorehdr) by the userspace kexec utility. In previous posts I have
> outlined the significant performance problems related to offloading
> this activity to userspace.
>
> This patchset introduces a generic crash hot un/plug handler that
> registers with the CPU and memory notifiers. Upon CPU or memory
> changes, this generic handler is invoked and performs important
> housekeeping, for example obtaining the appropriate lock, and then
> invokes an architecture specific handler to do the appropriate
> updates.
>
> In the case of x86_64, the arch specific handler generates a new
> elfcorehdr, and overwrites the old one in memory. No involvement
> with userspace needed.
>
> To realize the benefits/test this patchset, one must make a couple
> of minor changes to userspace:
>
>   - Disable the udev rule for updating kdump on hot un/plug changes.
>     Add the following as the first two lines to the udev rule file
>     /usr/lib/udev/rules.d/98-kexec.rules:

If we can have a sysfs attribute to advertise this feature then userspace
utilities (kexec tool/udev rules) can take action accordingly. In short, 
it will
help us maintain backward compatibility.

kexec tool can use the new sysfs attribute and allocate additional 
buffer space
for elfcorehdr accordingly. Similarly, the checksum-related changes can come
under this check.

Udev rule can use this sysfs file to decide kdump service reload is 
required or not.

Thanks,
Sourabh Jain

[PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Sourabh Jain]

Hello Eric,

On 06/05/22 00:15, Eric DeVolder wrote:
> When the kdump service is loaded, if a CPU or memory is hot
> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
> and memory in the system, must also be updated, else the resulting
> vmcore is inaccurate (eg. missing either CPU context or memory
> regions).
>
> The current solution utilizes udev to initiate an unload-then-reload
> of the kdump image (e. kernel, initrd, boot_params, puratory and
> elfcorehdr) by the userspace kexec utility. In previous posts I have
> outlined the significant performance problems related to offloading
> this activity to userspace.
>
> This patchset introduces a generic crash hot un/plug handler that
> registers with the CPU and memory notifiers. Upon CPU or memory
> changes, this generic handler is invoked and performs important
> housekeeping, for example obtaining the appropriate lock, and then
> invokes an architecture specific handler to do the appropriate
> updates.
>
> In the case of x86_64, the arch specific handler generates a new
> elfcorehdr, and overwrites the old one in memory. No involvement
> with userspace needed.
>
> To realize the benefits/test this patchset, one must make a couple
> of minor changes to userspace:
>
>   - Disable the udev rule for updating kdump on hot un/plug changes.
>     Add the following as the first two lines to the udev rule file
>     /usr/lib/udev/rules.d/98-kexec.rules:

If we can have a sysfs attribute to advertise this feature then userspace
utilities (kexec tool/udev rules) can take action accordingly. In short, 
it will
help us maintain backward compatibility.

kexec tool can use the new sysfs attribute and allocate additional 
buffer space
for elfcorehdr accordingly. Similarly, the checksum-related changes can come
under this check.

Udev rule can use this sysfs file to decide kdump service reload is 
required or not.

Thanks,
Sourabh Jain

Re: [PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Eric DeVolder]

On 5/25/22 10:13, Sourabh Jain wrote:
> Hello Eric,
> 
> On 06/05/22 00:15, Eric DeVolder wrote:
>> When the kdump service is loaded, if a CPU or memory is hot
>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>> and memory in the system, must also be updated, else the resulting
>> vmcore is inaccurate (eg. missing either CPU context or memory
>> regions).
>>
>> The current solution utilizes udev to initiate an unload-then-reload
>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>> outlined the significant performance problems related to offloading
>> this activity to userspace.
>>
>> This patchset introduces a generic crash hot un/plug handler that
>> registers with the CPU and memory notifiers. Upon CPU or memory
>> changes, this generic handler is invoked and performs important
>> housekeeping, for example obtaining the appropriate lock, and then
>> invokes an architecture specific handler to do the appropriate
>> updates.
>>
>> In the case of x86_64, the arch specific handler generates a new
>> elfcorehdr, and overwrites the old one in memory. No involvement
>> with userspace needed.
>>
>> To realize the benefits/test this patchset, one must make a couple
>> of minor changes to userspace:
>>
>>   - Disable the udev rule for updating kdump on hot un/plug changes.
>>     Add the following as the first two lines to the udev rule file
>>     /usr/lib/udev/rules.d/98-kexec.rules:
> 
> If we can have a sysfs attribute to advertise this feature then userspace
> utilities (kexec tool/udev rules) can take action accordingly. In short, it will
> help us maintain backward compatibility.
> 
> kexec tool can use the new sysfs attribute and allocate additional buffer space
> for elfcorehdr accordingly. Similarly, the checksum-related changes can come
> under this check.
> 
> Udev rule can use this sysfs file to decide kdump service reload is required or not.

Great idea. I've been working on the corresponding udev and kexec-tools changes and your input/idea 
here is quite timely.

I have boolean "crash_hotplug" as a core_param(), so it will show up as:

# cat /sys/module/kernel/parameters/crash_hotplug
N

This will provide userspace the indication it needs.

> 
> Thanks,
> Sourabh Jain
> 

[PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Eric DeVolder]

On 5/25/22 10:13, Sourabh Jain wrote:
> Hello Eric,
> 
> On 06/05/22 00:15, Eric DeVolder wrote:
>> When the kdump service is loaded, if a CPU or memory is hot
>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>> and memory in the system, must also be updated, else the resulting
>> vmcore is inaccurate (eg. missing either CPU context or memory
>> regions).
>>
>> The current solution utilizes udev to initiate an unload-then-reload
>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>> outlined the significant performance problems related to offloading
>> this activity to userspace.
>>
>> This patchset introduces a generic crash hot un/plug handler that
>> registers with the CPU and memory notifiers. Upon CPU or memory
>> changes, this generic handler is invoked and performs important
>> housekeeping, for example obtaining the appropriate lock, and then
>> invokes an architecture specific handler to do the appropriate
>> updates.
>>
>> In the case of x86_64, the arch specific handler generates a new
>> elfcorehdr, and overwrites the old one in memory. No involvement
>> with userspace needed.
>>
>> To realize the benefits/test this patchset, one must make a couple
>> of minor changes to userspace:
>>
>> ? - Disable the udev rule for updating kdump on hot un/plug changes.
>> ??? Add the following as the first two lines to the udev rule file
>> ??? /usr/lib/udev/rules.d/98-kexec.rules:
> 
> If we can have a sysfs attribute to advertise this feature then userspace
> utilities (kexec tool/udev rules) can take action accordingly. In short, it will
> help us maintain backward compatibility.
> 
> kexec tool can use the new sysfs attribute and allocate additional buffer space
> for elfcorehdr accordingly. Similarly, the checksum-related changes can come
> under this check.
> 
> Udev rule can use this sysfs file to decide kdump service reload is required or not.

Great idea. I've been working on the corresponding udev and kexec-tools changes and your input/idea 
here is quite timely.

I have boolean "crash_hotplug" as a core_param(), so it will show up as:

# cat /sys/module/kernel/parameters/crash_hotplug
N

This will provide userspace the indication it needs.

> 
> Thanks,
> Sourabh Jain
> 

Re: [PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Sourabh Jain]

Hello Eric,

On 26/05/22 18:46, Eric DeVolder wrote:
>
>
> On 5/25/22 10:13, Sourabh Jain wrote:
>> Hello Eric,
>>
>> On 06/05/22 00:15, Eric DeVolder wrote:
>>> When the kdump service is loaded, if a CPU or memory is hot
>>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>>> and memory in the system, must also be updated, else the resulting
>>> vmcore is inaccurate (eg. missing either CPU context or memory
>>> regions).
>>>
>>> The current solution utilizes udev to initiate an unload-then-reload
>>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>>> outlined the significant performance problems related to offloading
>>> this activity to userspace.
>>>
>>> This patchset introduces a generic crash hot un/plug handler that
>>> registers with the CPU and memory notifiers. Upon CPU or memory
>>> changes, this generic handler is invoked and performs important
>>> housekeeping, for example obtaining the appropriate lock, and then
>>> invokes an architecture specific handler to do the appropriate
>>> updates.
>>>
>>> In the case of x86_64, the arch specific handler generates a new
>>> elfcorehdr, and overwrites the old one in memory. No involvement
>>> with userspace needed.
>>>
>>> To realize the benefits/test this patchset, one must make a couple
>>> of minor changes to userspace:
>>>
>>>   - Disable the udev rule for updating kdump on hot un/plug changes.
>>>     Add the following as the first two lines to the udev rule file
>>>     /usr/lib/udev/rules.d/98-kexec.rules:
>>
>> If we can have a sysfs attribute to advertise this feature then 
>> userspace
>> utilities (kexec tool/udev rules) can take action accordingly. In 
>> short, it will
>> help us maintain backward compatibility.
>>
>> kexec tool can use the new sysfs attribute and allocate additional 
>> buffer space
>> for elfcorehdr accordingly. Similarly, the checksum-related changes 
>> can come
>> under this check.
>>
>> Udev rule can use this sysfs file to decide kdump service reload is 
>> required or not.
>
> Great idea. I've been working on the corresponding udev and 
> kexec-tools changes and your input/idea here is quite timely.
>
> I have boolean "crash_hotplug" as a core_param(), so it will show up as:
>
> # cat /sys/module/kernel/parameters/crash_hotplug
> N

How about using 0-1 instead Y/N?
0 = crash hotplug not supported
1 = crash hotplug supported

Also how about keeping sysfs here instead?
/sys/kernel/kexec_crash_hotplug

Thanks,
Souabh Jain

[PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Sourabh Jain]

Hello Eric,

On 26/05/22 18:46, Eric DeVolder wrote:
>
>
> On 5/25/22 10:13, Sourabh Jain wrote:
>> Hello Eric,
>>
>> On 06/05/22 00:15, Eric DeVolder wrote:
>>> When the kdump service is loaded, if a CPU or memory is hot
>>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>>> and memory in the system, must also be updated, else the resulting
>>> vmcore is inaccurate (eg. missing either CPU context or memory
>>> regions).
>>>
>>> The current solution utilizes udev to initiate an unload-then-reload
>>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>>> outlined the significant performance problems related to offloading
>>> this activity to userspace.
>>>
>>> This patchset introduces a generic crash hot un/plug handler that
>>> registers with the CPU and memory notifiers. Upon CPU or memory
>>> changes, this generic handler is invoked and performs important
>>> housekeeping, for example obtaining the appropriate lock, and then
>>> invokes an architecture specific handler to do the appropriate
>>> updates.
>>>
>>> In the case of x86_64, the arch specific handler generates a new
>>> elfcorehdr, and overwrites the old one in memory. No involvement
>>> with userspace needed.
>>>
>>> To realize the benefits/test this patchset, one must make a couple
>>> of minor changes to userspace:
>>>
>>> ? - Disable the udev rule for updating kdump on hot un/plug changes.
>>> ??? Add the following as the first two lines to the udev rule file
>>> ??? /usr/lib/udev/rules.d/98-kexec.rules:
>>
>> If we can have a sysfs attribute to advertise this feature then 
>> userspace
>> utilities (kexec tool/udev rules) can take action accordingly. In 
>> short, it will
>> help us maintain backward compatibility.
>>
>> kexec tool can use the new sysfs attribute and allocate additional 
>> buffer space
>> for elfcorehdr accordingly. Similarly, the checksum-related changes 
>> can come
>> under this check.
>>
>> Udev rule can use this sysfs file to decide kdump service reload is 
>> required or not.
>
> Great idea. I've been working on the corresponding udev and 
> kexec-tools changes and your input/idea here is quite timely.
>
> I have boolean "crash_hotplug" as a core_param(), so it will show up as:
>
> # cat /sys/module/kernel/parameters/crash_hotplug
> N

How about using 0-1 instead Y/N?
0 = crash hotplug not supported
1 = crash hotplug supported

Also how about keeping sysfs here instead?
/sys/kernel/kexec_crash_hotplug

Thanks,
Souabh Jain

Re: [PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Eric DeVolder]

On 5/26/22 08:39, Sourabh Jain wrote:
> Hello Eric,
> 
> On 26/05/22 18:46, Eric DeVolder wrote:
>>
>>
>> On 5/25/22 10:13, Sourabh Jain wrote:
>>> Hello Eric,
>>>
>>> On 06/05/22 00:15, Eric DeVolder wrote:
>>>> When the kdump service is loaded, if a CPU or memory is hot
>>>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>>>> and memory in the system, must also be updated, else the resulting
>>>> vmcore is inaccurate (eg. missing either CPU context or memory
>>>> regions).
>>>>
>>>> The current solution utilizes udev to initiate an unload-then-reload
>>>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>>>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>>>> outlined the significant performance problems related to offloading
>>>> this activity to userspace.
>>>>
>>>> This patchset introduces a generic crash hot un/plug handler that
>>>> registers with the CPU and memory notifiers. Upon CPU or memory
>>>> changes, this generic handler is invoked and performs important
>>>> housekeeping, for example obtaining the appropriate lock, and then
>>>> invokes an architecture specific handler to do the appropriate
>>>> updates.
>>>>
>>>> In the case of x86_64, the arch specific handler generates a new
>>>> elfcorehdr, and overwrites the old one in memory. No involvement
>>>> with userspace needed.
>>>>
>>>> To realize the benefits/test this patchset, one must make a couple
>>>> of minor changes to userspace:
>>>>
>>>>   - Disable the udev rule for updating kdump on hot un/plug changes.
>>>>     Add the following as the first two lines to the udev rule file
>>>>     /usr/lib/udev/rules.d/98-kexec.rules:
>>>
>>> If we can have a sysfs attribute to advertise this feature then userspace
>>> utilities (kexec tool/udev rules) can take action accordingly. In short, it will
>>> help us maintain backward compatibility.
>>>
>>> kexec tool can use the new sysfs attribute and allocate additional buffer space
>>> for elfcorehdr accordingly. Similarly, the checksum-related changes can come
>>> under this check.
>>>
>>> Udev rule can use this sysfs file to decide kdump service reload is required or not.
>>
>> Great idea. I've been working on the corresponding udev and kexec-tools changes and your 
>> input/idea here is quite timely.
>>
>> I have boolean "crash_hotplug" as a core_param(), so it will show up as:
>>
>> # cat /sys/module/kernel/parameters/crash_hotplug
>> N
> 
> How about using 0-1 instead Y/N?
> 0 = crash hotplug not supported
> 1 = crash hotplug supported
> 
> Also how about keeping sysfs here instead?
> /sys/kernel/kexec_crash_hotplug

Yes, that makes more sense.
Thanks!
eric

> 
> Thanks,
> Souabh Jain
> 

[PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Eric DeVolder]

On 5/26/22 08:39, Sourabh Jain wrote:
> Hello Eric,
> 
> On 26/05/22 18:46, Eric DeVolder wrote:
>>
>>
>> On 5/25/22 10:13, Sourabh Jain wrote:
>>> Hello Eric,
>>>
>>> On 06/05/22 00:15, Eric DeVolder wrote:
>>>> When the kdump service is loaded, if a CPU or memory is hot
>>>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>>>> and memory in the system, must also be updated, else the resulting
>>>> vmcore is inaccurate (eg. missing either CPU context or memory
>>>> regions).
>>>>
>>>> The current solution utilizes udev to initiate an unload-then-reload
>>>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>>>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>>>> outlined the significant performance problems related to offloading
>>>> this activity to userspace.
>>>>
>>>> This patchset introduces a generic crash hot un/plug handler that
>>>> registers with the CPU and memory notifiers. Upon CPU or memory
>>>> changes, this generic handler is invoked and performs important
>>>> housekeeping, for example obtaining the appropriate lock, and then
>>>> invokes an architecture specific handler to do the appropriate
>>>> updates.
>>>>
>>>> In the case of x86_64, the arch specific handler generates a new
>>>> elfcorehdr, and overwrites the old one in memory. No involvement
>>>> with userspace needed.
>>>>
>>>> To realize the benefits/test this patchset, one must make a couple
>>>> of minor changes to userspace:
>>>>
>>>> ? - Disable the udev rule for updating kdump on hot un/plug changes.
>>>> ??? Add the following as the first two lines to the udev rule file
>>>> ??? /usr/lib/udev/rules.d/98-kexec.rules:
>>>
>>> If we can have a sysfs attribute to advertise this feature then userspace
>>> utilities (kexec tool/udev rules) can take action accordingly. In short, it will
>>> help us maintain backward compatibility.
>>>
>>> kexec tool can use the new sysfs attribute and allocate additional buffer space
>>> for elfcorehdr accordingly. Similarly, the checksum-related changes can come
>>> under this check.
>>>
>>> Udev rule can use this sysfs file to decide kdump service reload is required or not.
>>
>> Great idea. I've been working on the corresponding udev and kexec-tools changes and your 
>> input/idea here is quite timely.
>>
>> I have boolean "crash_hotplug" as a core_param(), so it will show up as:
>>
>> # cat /sys/module/kernel/parameters/crash_hotplug
>> N
> 
> How about using 0-1 instead Y/N?
> 0 = crash hotplug not supported
> 1 = crash hotplug supported
> 
> Also how about keeping sysfs here instead?
> /sys/kernel/kexec_crash_hotplug

Yes, that makes more sense.
Thanks!
eric

> 
> Thanks,
> Souabh Jain
> 

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[David Hildenbrand]

On 12.05.22 18:10, Eric DeVolder wrote:
> David,
> Great questions! See inline responses below.
> eric

Sorry for the late reply, travel and vacation ...

>>
>>> +
>>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>>> +							unsigned int hp_action, unsigned int cpu)
>>> +{
>>> +	WARN(1, "crash hotplug handler not implemented");
>>
>>
>> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
>> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
>> triggering?
>>
> You're correct. What about: printk_once(KERN_DEBUG "...") ?

Why even bother about printing anything? If the feature is not
supported, there should be some way for user space to figure out that it
sill has to reload on hot(un)plug manually, no?


[...]

> 
>>
>> 2. Why can't the unprotect+reprotect not be done inside
>> arch_crash_handle_hotplug_event() ? It's all arch specific either way.
>>
>> IMHO, this code here should be as simple as
>>
>> if (kexec_crash_image)
>> 	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>>
> 
> The intent of this code was to be generic infrastructure. Just invoking the
> arch_crash_handle_hotplug_event() would certainly be as generic as it gets.
> But there were a series of steps that seemed to be common, so those I hoisted
> into this bit of code.

But most common parts are actually arch_* calls already? :)

Anyhow, no strong opinion.

> 
>> 3. Why do we have to forward the CPU for CPU onlining/offlining but not the
>> memory block id (or similar) when onlining/offlining a memory block?
>  From patch "kexec: exclude hot remove cpu from elfcorehdr notes" commit message:
> 
> Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
> still in the for_each_present_cpu() list when within the
> handle_hotplug_event(). Thus the CPU must be explicitly excluded
> when building the new list of CPUs.
> 
> This change identifies in handle_hotplug_event() the CPU to be
> excluded, and the check for excluding the CPU in
> crash_prepare_elf64_headers().
> 
> If there is a better CPUHP_ to use than _DYN, I'd be all for that!

Ah okay, thanks.

-- 
Thanks,

David / dhildenb

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[David Hildenbrand]

On 12.05.22 18:10, Eric DeVolder wrote:
> David,
> Great questions! See inline responses below.
> eric

Sorry for the late reply, travel and vacation ...

>>
>>> +
>>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>>> +							unsigned int hp_action, unsigned int cpu)
>>> +{
>>> +	WARN(1, "crash hotplug handler not implemented");
>>
>>
>> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
>> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
>> triggering?
>>
> You're correct. What about: printk_once(KERN_DEBUG "...") ?

Why even bother about printing anything? If the feature is not
supported, there should be some way for user space to figure out that it
sill has to reload on hot(un)plug manually, no?


[...]

> 
>>
>> 2. Why can't the unprotect+reprotect not be done inside
>> arch_crash_handle_hotplug_event() ? It's all arch specific either way.
>>
>> IMHO, this code here should be as simple as
>>
>> if (kexec_crash_image)
>> 	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>>
> 
> The intent of this code was to be generic infrastructure. Just invoking the
> arch_crash_handle_hotplug_event() would certainly be as generic as it gets.
> But there were a series of steps that seemed to be common, so those I hoisted
> into this bit of code.

But most common parts are actually arch_* calls already? :)

Anyhow, no strong opinion.

> 
>> 3. Why do we have to forward the CPU for CPU onlining/offlining but not the
>> memory block id (or similar) when onlining/offlining a memory block?
>  From patch "kexec: exclude hot remove cpu from elfcorehdr notes" commit message:
> 
> Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
> still in the for_each_present_cpu() list when within the
> handle_hotplug_event(). Thus the CPU must be explicitly excluded
> when building the new list of CPUs.
> 
> This change identifies in handle_hotplug_event() the CPU to be
> excluded, and the check for excluding the CPU in
> crash_prepare_elf64_headers().
> 
> If there is a better CPUHP_ to use than _DYN, I'd be all for that!

Ah okay, thanks.

-- 
Thanks,

David / dhildenb

Re: [PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[David Hildenbrand]

On 26.05.22 15:39, Sourabh Jain wrote:
> Hello Eric,
> 
> On 26/05/22 18:46, Eric DeVolder wrote:
>>
>>
>> On 5/25/22 10:13, Sourabh Jain wrote:
>>> Hello Eric,
>>>
>>> On 06/05/22 00:15, Eric DeVolder wrote:
>>>> When the kdump service is loaded, if a CPU or memory is hot
>>>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>>>> and memory in the system, must also be updated, else the resulting
>>>> vmcore is inaccurate (eg. missing either CPU context or memory
>>>> regions).
>>>>
>>>> The current solution utilizes udev to initiate an unload-then-reload
>>>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>>>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>>>> outlined the significant performance problems related to offloading
>>>> this activity to userspace.
>>>>
>>>> This patchset introduces a generic crash hot un/plug handler that
>>>> registers with the CPU and memory notifiers. Upon CPU or memory
>>>> changes, this generic handler is invoked and performs important
>>>> housekeeping, for example obtaining the appropriate lock, and then
>>>> invokes an architecture specific handler to do the appropriate
>>>> updates.
>>>>
>>>> In the case of x86_64, the arch specific handler generates a new
>>>> elfcorehdr, and overwrites the old one in memory. No involvement
>>>> with userspace needed.
>>>>
>>>> To realize the benefits/test this patchset, one must make a couple
>>>> of minor changes to userspace:
>>>>
>>>>   - Disable the udev rule for updating kdump on hot un/plug changes.
>>>>     Add the following as the first two lines to the udev rule file
>>>>     /usr/lib/udev/rules.d/98-kexec.rules:
>>>
>>> If we can have a sysfs attribute to advertise this feature then 
>>> userspace
>>> utilities (kexec tool/udev rules) can take action accordingly. In 
>>> short, it will
>>> help us maintain backward compatibility.
>>>
>>> kexec tool can use the new sysfs attribute and allocate additional 
>>> buffer space
>>> for elfcorehdr accordingly. Similarly, the checksum-related changes 
>>> can come
>>> under this check.
>>>
>>> Udev rule can use this sysfs file to decide kdump service reload is 
>>> required or not.
>>
>> Great idea. I've been working on the corresponding udev and 
>> kexec-tools changes and your input/idea here is quite timely.
>>
>> I have boolean "crash_hotplug" as a core_param(), so it will show up as:
>>
>> # cat /sys/module/kernel/parameters/crash_hotplug
>> N
> 
> How about using 0-1 instead Y/N?
> 0 = crash hotplug not supported
> 1 = crash hotplug supported
> 
> Also how about keeping sysfs here instead?
> /sys/kernel/kexec_crash_hotplug

It's not only about hotplug, though. And actually we care about
onlining/offlining. Hmm, I wonder if there is a better name for this
automatic handling of cpu and memory devices.

-- 
Thanks,

David / dhildenb

[PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[David Hildenbrand]

On 26.05.22 15:39, Sourabh Jain wrote:
> Hello Eric,
> 
> On 26/05/22 18:46, Eric DeVolder wrote:
>>
>>
>> On 5/25/22 10:13, Sourabh Jain wrote:
>>> Hello Eric,
>>>
>>> On 06/05/22 00:15, Eric DeVolder wrote:
>>>> When the kdump service is loaded, if a CPU or memory is hot
>>>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>>>> and memory in the system, must also be updated, else the resulting
>>>> vmcore is inaccurate (eg. missing either CPU context or memory
>>>> regions).
>>>>
>>>> The current solution utilizes udev to initiate an unload-then-reload
>>>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>>>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>>>> outlined the significant performance problems related to offloading
>>>> this activity to userspace.
>>>>
>>>> This patchset introduces a generic crash hot un/plug handler that
>>>> registers with the CPU and memory notifiers. Upon CPU or memory
>>>> changes, this generic handler is invoked and performs important
>>>> housekeeping, for example obtaining the appropriate lock, and then
>>>> invokes an architecture specific handler to do the appropriate
>>>> updates.
>>>>
>>>> In the case of x86_64, the arch specific handler generates a new
>>>> elfcorehdr, and overwrites the old one in memory. No involvement
>>>> with userspace needed.
>>>>
>>>> To realize the benefits/test this patchset, one must make a couple
>>>> of minor changes to userspace:
>>>>
>>>> ? - Disable the udev rule for updating kdump on hot un/plug changes.
>>>> ??? Add the following as the first two lines to the udev rule file
>>>> ??? /usr/lib/udev/rules.d/98-kexec.rules:
>>>
>>> If we can have a sysfs attribute to advertise this feature then 
>>> userspace
>>> utilities (kexec tool/udev rules) can take action accordingly. In 
>>> short, it will
>>> help us maintain backward compatibility.
>>>
>>> kexec tool can use the new sysfs attribute and allocate additional 
>>> buffer space
>>> for elfcorehdr accordingly. Similarly, the checksum-related changes 
>>> can come
>>> under this check.
>>>
>>> Udev rule can use this sysfs file to decide kdump service reload is 
>>> required or not.
>>
>> Great idea. I've been working on the corresponding udev and 
>> kexec-tools changes and your input/idea here is quite timely.
>>
>> I have boolean "crash_hotplug" as a core_param(), so it will show up as:
>>
>> # cat /sys/module/kernel/parameters/crash_hotplug
>> N
> 
> How about using 0-1 instead Y/N?
> 0 = crash hotplug not supported
> 1 = crash hotplug supported
> 
> Also how about keeping sysfs here instead?
> /sys/kernel/kexec_crash_hotplug

It's not only about hotplug, though. And actually we care about
onlining/offlining. Hmm, I wonder if there is a better name for this
automatic handling of cpu and memory devices.

-- 
Thanks,

David / dhildenb

Re: [PATCH v8 7/7] x86/crash: Add x86 crash hotplug support for kexec_load

[Eric DeVolder]

On 5/25/22 09:26, Sourabh Jain wrote:
> Hello Eric,
> 
> On 06/05/22 00:16, Eric DeVolder wrote:
>> For kexec_file_load support, the loading of the crash kernel occurs
>> entirely within the kernel, and as such the elfcorehdr is readily
>> identified (so that it can be modified upon hotplug events).
>>
>> This change enables support for kexec_load by identifying the
>> elfcorehdr segment in the arch_crash_handle_hotplug_event(),
>> if it has not already been identified.
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> Acked-by: Baoquan He <bhe@redhat.com>
>> ---
>>   arch/x86/kernel/crash.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>> index 951ef365f0a7..845d7c77854d 100644
>> --- a/arch/x86/kernel/crash.c
>> +++ b/arch/x86/kernel/crash.c
>> @@ -485,6 +485,30 @@ void arch_crash_handle_hotplug_event(struct kimage *image,
>>       void *elfbuf = NULL;
>>       unsigned long mem, memsz;
>> +    /*
>> +     * When the struct kimage is alloced, it is wiped to zero, so
>> +     * the elfcorehdr_index_valid defaults to false. It is set on the
>> +     * kexec_file_load path, or here for kexec_load, if not already
>> +     * identified.
>> +     */
>> +    if (!image->elfcorehdr_index_valid) {
>> +        unsigned int n;
>> +
>> +        for (n = 0; n < image->nr_segments; n++) {
>> +            mem = image->segment[n].mem;
>> +            memsz = image->segment[n].memsz;
>> +            ptr = map_crash_pages(mem, memsz);
>> +            if (ptr) {
>> +                /* The segment containing elfcorehdr */
>> +                if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
>> +                    image->elfcorehdr_index = (int)n;
>> +                    image->elfcorehdr_index_valid = true;
> 
> How about finding elfcorehdr index on kexec_load path post kimage_load_segment call in
> do_kexec_load (kernel/kexec.c) or other suitable place? This way we can avoid checking for
> elfcorehdr index for every hotplug. Also we might not need image->elfcorehdr_index_valid.

That would be a viable place to put it. However, a couple of notes.

This code actually works for both kexec_load and kexec_file_load paths (how I originally used this 
code); so if we were to move this to a more common location, I'd be in favor of a location that 
serves both paths (ie handle_hotplug_event() makes sense to me). That would eliminate the need for 
setting the index in the kexec_file_load paths.

Also, I don't see a scenario where elfcorehdr_index_valid can be eliminated; the index isn't always 
valid. What are you thinking?

And the check is rather low cost being the check of a boolean.

eric
> 
> Thanks,
> Sourabh Jain
> 
> 

[PATCH v8 7/7] x86/crash: Add x86 crash hotplug support for kexec_load

[Eric DeVolder]

On 5/25/22 09:26, Sourabh Jain wrote:
> Hello Eric,
> 
> On 06/05/22 00:16, Eric DeVolder wrote:
>> For kexec_file_load support, the loading of the crash kernel occurs
>> entirely within the kernel, and as such the elfcorehdr is readily
>> identified (so that it can be modified upon hotplug events).
>>
>> This change enables support for kexec_load by identifying the
>> elfcorehdr segment in the arch_crash_handle_hotplug_event(),
>> if it has not already been identified.
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> Acked-by: Baoquan He <bhe@redhat.com>
>> ---
>> ? arch/x86/kernel/crash.c | 24 ++++++++++++++++++++++++
>> ? 1 file changed, 24 insertions(+)
>>
>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>> index 951ef365f0a7..845d7c77854d 100644
>> --- a/arch/x86/kernel/crash.c
>> +++ b/arch/x86/kernel/crash.c
>> @@ -485,6 +485,30 @@ void arch_crash_handle_hotplug_event(struct kimage *image,
>> ????? void *elfbuf = NULL;
>> ????? unsigned long mem, memsz;
>> +??? /*
>> +???? * When the struct kimage is alloced, it is wiped to zero, so
>> +???? * the elfcorehdr_index_valid defaults to false. It is set on the
>> +???? * kexec_file_load path, or here for kexec_load, if not already
>> +???? * identified.
>> +???? */
>> +??? if (!image->elfcorehdr_index_valid) {
>> +??????? unsigned int n;
>> +
>> +??????? for (n = 0; n < image->nr_segments; n++) {
>> +??????????? mem = image->segment[n].mem;
>> +??????????? memsz = image->segment[n].memsz;
>> +??????????? ptr = map_crash_pages(mem, memsz);
>> +??????????? if (ptr) {
>> +??????????????? /* The segment containing elfcorehdr */
>> +??????????????? if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
>> +??????????????????? image->elfcorehdr_index = (int)n;
>> +??????????????????? image->elfcorehdr_index_valid = true;
> 
> How about finding elfcorehdr index on kexec_load path post kimage_load_segment call in
> do_kexec_load (kernel/kexec.c) or other suitable place? This way we can avoid checking for
> elfcorehdr index for every hotplug. Also we might not need image->elfcorehdr_index_valid.

That would be a viable place to put it. However, a couple of notes.

This code actually works for both kexec_load and kexec_file_load paths (how I originally used this 
code); so if we were to move this to a more common location, I'd be in favor of a location that 
serves both paths (ie handle_hotplug_event() makes sense to me). That would eliminate the need for 
setting the index in the kexec_file_load paths.

Also, I don't see a scenario where elfcorehdr_index_valid can be eliminated; the index isn't always 
valid. What are you thinking?

And the check is rather low cost being the check of a boolean.

eric
> 
> Thanks,
> Sourabh Jain
> 
> 

Re: [PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Eric DeVolder]

On 5/31/22 08:18, David Hildenbrand wrote:
> On 26.05.22 15:39, Sourabh Jain wrote:
>> Hello Eric,
>>
>> On 26/05/22 18:46, Eric DeVolder wrote:
>>>
>>>
>>> On 5/25/22 10:13, Sourabh Jain wrote:
>>>> Hello Eric,
>>>>
>>>> On 06/05/22 00:15, Eric DeVolder wrote:
>>>>> When the kdump service is loaded, if a CPU or memory is hot
>>>>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>>>>> and memory in the system, must also be updated, else the resulting
>>>>> vmcore is inaccurate (eg. missing either CPU context or memory
>>>>> regions).
>>>>>
>>>>> The current solution utilizes udev to initiate an unload-then-reload
>>>>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>>>>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>>>>> outlined the significant performance problems related to offloading
>>>>> this activity to userspace.
>>>>>
>>>>> This patchset introduces a generic crash hot un/plug handler that
>>>>> registers with the CPU and memory notifiers. Upon CPU or memory
>>>>> changes, this generic handler is invoked and performs important
>>>>> housekeeping, for example obtaining the appropriate lock, and then
>>>>> invokes an architecture specific handler to do the appropriate
>>>>> updates.
>>>>>
>>>>> In the case of x86_64, the arch specific handler generates a new
>>>>> elfcorehdr, and overwrites the old one in memory. No involvement
>>>>> with userspace needed.
>>>>>
>>>>> To realize the benefits/test this patchset, one must make a couple
>>>>> of minor changes to userspace:
>>>>>
>>>>>    - Disable the udev rule for updating kdump on hot un/plug changes.
>>>>>      Add the following as the first two lines to the udev rule file
>>>>>      /usr/lib/udev/rules.d/98-kexec.rules:
>>>>
>>>> If we can have a sysfs attribute to advertise this feature then
>>>> userspace
>>>> utilities (kexec tool/udev rules) can take action accordingly. In
>>>> short, it will
>>>> help us maintain backward compatibility.
>>>>
>>>> kexec tool can use the new sysfs attribute and allocate additional
>>>> buffer space
>>>> for elfcorehdr accordingly. Similarly, the checksum-related changes
>>>> can come
>>>> under this check.
>>>>
>>>> Udev rule can use this sysfs file to decide kdump service reload is
>>>> required or not.
>>>
>>> Great idea. I've been working on the corresponding udev and
>>> kexec-tools changes and your input/idea here is quite timely.
>>>
>>> I have boolean "crash_hotplug" as a core_param(), so it will show up as:
>>>
>>> # cat /sys/module/kernel/parameters/crash_hotplug
>>> N
>>
>> How about using 0-1 instead Y/N?
>> 0 = crash hotplug not supported
>> 1 = crash hotplug supported
>>
>> Also how about keeping sysfs here instead?
>> /sys/kernel/kexec_crash_hotplug
> 
> It's not only about hotplug, though. And actually we care about
> onlining/offlining. Hmm, I wonder if there is a better name for this
> automatic handling of cpu and memory devices.
> 
In the upcoming v9, there is no /sys/kernel/crash/kexec_crash_hotplug; I have sysfs attributes for 
memory blocks and CPUs named 'crash_hotplug' that can be utilized directly in udev rule as 
ATTR{crash_hotplug} to determine if the kernel is handling this for crash kernel update purposes.

Here's the current commit message for that change:

====
crash: memory and CPU hotplug sysfs attributes

This introduces the crash_hotplug attribute for memory and CPUs
for use by userspace.  This change directly facilitates the udev
rule for managing userspace re-loading of the crash kernel.

For memory, this changeset introduces the crash_hotplug attribute
to the /sys/devices/system/memory directory. For example:

  # udevadm info --attribute-walk /sys/devices/system/memory/memory81
   looking at device '/devices/system/memory/memory81':
     KERNEL=="memory81"
     SUBSYSTEM=="memory"
     DRIVER==""
     ATTR{online}=="1"
     ATTR{phys_device}=="0"
     ATTR{phys_index}=="00000051"
     ATTR{removable}=="1"
     ATTR{state}=="online"
     ATTR{valid_zones}=="Movable"

   looking at parent device '/devices/system/memory':
     KERNELS=="memory"
     SUBSYSTEMS==""
     DRIVERS==""
     ATTRS{auto_online_blocks}=="offline"
     ATTRS{block_size_bytes}=="8000000"
     ATTRS{crash_hotplug}=="1"

For CPUs, this changeset introduces the crash_hotplug attribute
to the /sys/devices/system/cpu directory. For example:

  # udevadm info --attribute-walk /sys/devices/system/cpu/cpu0
   looking at device '/devices/system/cpu/cpu0':
     KERNEL=="cpu0"
     SUBSYSTEM=="cpu"
     DRIVER=="processor"
     ATTR{crash_notes}=="277c38600"
     ATTR{crash_notes_size}=="368"
     ATTR{online}=="1"

   looking at parent device '/devices/system/cpu':
     KERNELS=="cpu"
     SUBSYSTEMS==""
     DRIVERS==""
     ATTRS{crash_hotplug}=="1"
     ATTRS{isolated}==""
     ATTRS{kernel_max}=="8191"
     ATTRS{nohz_full}=="  (null)"
     ATTRS{offline}=="4-7"
     ATTRS{online}=="0-3"
     ATTRS{possible}=="0-7"
     ATTRS{present}=="0-3"

With these changes in place, and by using the same attribute
crash_hotplug name, it is possible to efficiently instruct the
udev rule to skip crash kernel reloading.

For example, the following is the proposed udev rule change for RHEL
system 98-kexec.rules (as the first two lines of the rule file):

  # The kernel handles updates to crash elfcorehdr
  ATTRS{crash_hotplug}=="1", GOTO="kdump_reload_end"

When examined in the context of 98-kexec.rules, the above change
tests if crash_hotplug is set, and if so, it skips the userspace
initiated unload-then-reload of the crash kernel.
=====

Does that work for you?
Eric

[PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug

[Eric DeVolder]

On 5/31/22 08:18, David Hildenbrand wrote:
> On 26.05.22 15:39, Sourabh Jain wrote:
>> Hello Eric,
>>
>> On 26/05/22 18:46, Eric DeVolder wrote:
>>>
>>>
>>> On 5/25/22 10:13, Sourabh Jain wrote:
>>>> Hello Eric,
>>>>
>>>> On 06/05/22 00:15, Eric DeVolder wrote:
>>>>> When the kdump service is loaded, if a CPU or memory is hot
>>>>> un/plugged, the crash elfcorehdr (for x86), which describes the CPUs
>>>>> and memory in the system, must also be updated, else the resulting
>>>>> vmcore is inaccurate (eg. missing either CPU context or memory
>>>>> regions).
>>>>>
>>>>> The current solution utilizes udev to initiate an unload-then-reload
>>>>> of the kdump image (e. kernel, initrd, boot_params, puratory and
>>>>> elfcorehdr) by the userspace kexec utility. In previous posts I have
>>>>> outlined the significant performance problems related to offloading
>>>>> this activity to userspace.
>>>>>
>>>>> This patchset introduces a generic crash hot un/plug handler that
>>>>> registers with the CPU and memory notifiers. Upon CPU or memory
>>>>> changes, this generic handler is invoked and performs important
>>>>> housekeeping, for example obtaining the appropriate lock, and then
>>>>> invokes an architecture specific handler to do the appropriate
>>>>> updates.
>>>>>
>>>>> In the case of x86_64, the arch specific handler generates a new
>>>>> elfcorehdr, and overwrites the old one in memory. No involvement
>>>>> with userspace needed.
>>>>>
>>>>> To realize the benefits/test this patchset, one must make a couple
>>>>> of minor changes to userspace:
>>>>>
>>>>>  ? - Disable the udev rule for updating kdump on hot un/plug changes.
>>>>>  ??? Add the following as the first two lines to the udev rule file
>>>>>  ??? /usr/lib/udev/rules.d/98-kexec.rules:
>>>>
>>>> If we can have a sysfs attribute to advertise this feature then
>>>> userspace
>>>> utilities (kexec tool/udev rules) can take action accordingly. In
>>>> short, it will
>>>> help us maintain backward compatibility.
>>>>
>>>> kexec tool can use the new sysfs attribute and allocate additional
>>>> buffer space
>>>> for elfcorehdr accordingly. Similarly, the checksum-related changes
>>>> can come
>>>> under this check.
>>>>
>>>> Udev rule can use this sysfs file to decide kdump service reload is
>>>> required or not.
>>>
>>> Great idea. I've been working on the corresponding udev and
>>> kexec-tools changes and your input/idea here is quite timely.
>>>
>>> I have boolean "crash_hotplug" as a core_param(), so it will show up as:
>>>
>>> # cat /sys/module/kernel/parameters/crash_hotplug
>>> N
>>
>> How about using 0-1 instead Y/N?
>> 0 = crash hotplug not supported
>> 1 = crash hotplug supported
>>
>> Also how about keeping sysfs here instead?
>> /sys/kernel/kexec_crash_hotplug
> 
> It's not only about hotplug, though. And actually we care about
> onlining/offlining. Hmm, I wonder if there is a better name for this
> automatic handling of cpu and memory devices.
> 
In the upcoming v9, there is no /sys/kernel/crash/kexec_crash_hotplug; I have sysfs attributes for 
memory blocks and CPUs named 'crash_hotplug' that can be utilized directly in udev rule as 
ATTR{crash_hotplug} to determine if the kernel is handling this for crash kernel update purposes.

Here's the current commit message for that change:

====
crash: memory and CPU hotplug sysfs attributes

This introduces the crash_hotplug attribute for memory and CPUs
for use by userspace.  This change directly facilitates the udev
rule for managing userspace re-loading of the crash kernel.

For memory, this changeset introduces the crash_hotplug attribute
to the /sys/devices/system/memory directory. For example:

  # udevadm info --attribute-walk /sys/devices/system/memory/memory81
   looking at device '/devices/system/memory/memory81':
     KERNEL=="memory81"
     SUBSYSTEM=="memory"
     DRIVER==""
     ATTR{online}=="1"
     ATTR{phys_device}=="0"
     ATTR{phys_index}=="00000051"
     ATTR{removable}=="1"
     ATTR{state}=="online"
     ATTR{valid_zones}=="Movable"

   looking at parent device '/devices/system/memory':
     KERNELS=="memory"
     SUBSYSTEMS==""
     DRIVERS==""
     ATTRS{auto_online_blocks}=="offline"
     ATTRS{block_size_bytes}=="8000000"
     ATTRS{crash_hotplug}=="1"

For CPUs, this changeset introduces the crash_hotplug attribute
to the /sys/devices/system/cpu directory. For example:

  # udevadm info --attribute-walk /sys/devices/system/cpu/cpu0
   looking at device '/devices/system/cpu/cpu0':
     KERNEL=="cpu0"
     SUBSYSTEM=="cpu"
     DRIVER=="processor"
     ATTR{crash_notes}=="277c38600"
     ATTR{crash_notes_size}=="368"
     ATTR{online}=="1"

   looking at parent device '/devices/system/cpu':
     KERNELS=="cpu"
     SUBSYSTEMS==""
     DRIVERS==""
     ATTRS{crash_hotplug}=="1"
     ATTRS{isolated}==""
     ATTRS{kernel_max}=="8191"
     ATTRS{nohz_full}=="  (null)"
     ATTRS{offline}=="4-7"
     ATTRS{online}=="0-3"
     ATTRS{possible}=="0-7"
     ATTRS{present}=="0-3"

With these changes in place, and by using the same attribute
crash_hotplug name, it is possible to efficiently instruct the
udev rule to skip crash kernel reloading.

For example, the following is the proposed udev rule change for RHEL
system 98-kexec.rules (as the first two lines of the rule file):

  # The kernel handles updates to crash elfcorehdr
  ATTRS{crash_hotplug}=="1", GOTO="kdump_reload_end"

When examined in the context of 98-kexec.rules, the above change
tests if crash_hotplug is set, and if so, it skips the userspace
initiated unload-then-reload of the crash kernel.
=====

Does that work for you?
Eric

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

On 5/31/22 08:15, David Hildenbrand wrote:
> On 12.05.22 18:10, Eric DeVolder wrote:
>> David,
>> Great questions! See inline responses below.
>> eric
> 
> Sorry for the late reply, travel and vacation ...
No problem, greatly appreciate the feedback!
eric

> 
>>>
>>>> +
>>>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>>>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>>>> +							unsigned int hp_action, unsigned int cpu)
>>>> +{
>>>> +	WARN(1, "crash hotplug handler not implemented");
>>>
>>>
>>> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
>>> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
>>> triggering?
>>>
>> You're correct. What about: printk_once(KERN_DEBUG "...") ?
> 
> Why even bother about printing anything? If the feature is not
> supported, there should be some way for user space to figure out that it
> sill has to reload on hot(un)plug manually, no?

I've changed this to WARN_ONCE(). If that isn't agreeable, I'll remove it.

> 
> 
> [...]
> 
>>
>>>
>>> 2. Why can't the unprotect+reprotect not be done inside
>>> arch_crash_handle_hotplug_event() ? It's all arch specific either way.
>>>
>>> IMHO, this code here should be as simple as
>>>
>>> if (kexec_crash_image)
>>> 	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>>>
>>
>> The intent of this code was to be generic infrastructure. Just invoking the
>> arch_crash_handle_hotplug_event() would certainly be as generic as it gets.
>> But there were a series of steps that seemed to be common, so those I hoisted
>> into this bit of code.
> 
> But most common parts are actually arch_* calls already? :)
> 
> Anyhow, no strong opinion.
For the time being, I'd like to leave as is. Let's see if the detection of the elfcorehdr
segment gets moved to this code too... (discussion thread on "x86/crash: Add x86 crash hotplug 
support for kexec_load".

> 
>>
>>> 3. Why do we have to forward the CPU for CPU onlining/offlining but not the
>>> memory block id (or similar) when onlining/offlining a memory block?
>>   From patch "kexec: exclude hot remove cpu from elfcorehdr notes" commit message:
>>
>> Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
>> still in the for_each_present_cpu() list when within the
>> handle_hotplug_event(). Thus the CPU must be explicitly excluded
>> when building the new list of CPUs.
>>
>> This change identifies in handle_hotplug_event() the CPU to be
>> excluded, and the check for excluding the CPU in
>> crash_prepare_elf64_headers().
>>
>> If there is a better CPUHP_ to use than _DYN, I'd be all for that!
> 
> Ah okay, thanks.
> 

[PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

On 5/31/22 08:15, David Hildenbrand wrote:
> On 12.05.22 18:10, Eric DeVolder wrote:
>> David,
>> Great questions! See inline responses below.
>> eric
> 
> Sorry for the late reply, travel and vacation ...
No problem, greatly appreciate the feedback!
eric

> 
>>>
>>>> +
>>>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>>>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>>>> +							unsigned int hp_action, unsigned int cpu)
>>>> +{
>>>> +	WARN(1, "crash hotplug handler not implemented");
>>>
>>>
>>> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
>>> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
>>> triggering?
>>>
>> You're correct. What about: printk_once(KERN_DEBUG "...") ?
> 
> Why even bother about printing anything? If the feature is not
> supported, there should be some way for user space to figure out that it
> sill has to reload on hot(un)plug manually, no?

I've changed this to WARN_ONCE(). If that isn't agreeable, I'll remove it.

> 
> 
> [...]
> 
>>
>>>
>>> 2. Why can't the unprotect+reprotect not be done inside
>>> arch_crash_handle_hotplug_event() ? It's all arch specific either way.
>>>
>>> IMHO, this code here should be as simple as
>>>
>>> if (kexec_crash_image)
>>> 	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>>>
>>
>> The intent of this code was to be generic infrastructure. Just invoking the
>> arch_crash_handle_hotplug_event() would certainly be as generic as it gets.
>> But there were a series of steps that seemed to be common, so those I hoisted
>> into this bit of code.
> 
> But most common parts are actually arch_* calls already? :)
> 
> Anyhow, no strong opinion.
For the time being, I'd like to leave as is. Let's see if the detection of the elfcorehdr
segment gets moved to this code too... (discussion thread on "x86/crash: Add x86 crash hotplug 
support for kexec_load".

> 
>>
>>> 3. Why do we have to forward the CPU for CPU onlining/offlining but not the
>>> memory block id (or similar) when onlining/offlining a memory block?
>>   From patch "kexec: exclude hot remove cpu from elfcorehdr notes" commit message:
>>
>> Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
>> still in the for_each_present_cpu() list when within the
>> handle_hotplug_event(). Thus the CPU must be explicitly excluded
>> when building the new list of CPUs.
>>
>> This change identifies in handle_hotplug_event() the CPU to be
>> excluded, and the check for excluding the CPU in
>> crash_prepare_elf64_headers().
>>
>> If there is a better CPUHP_ to use than _DYN, I'd be all for that!
> 
> Ah okay, thanks.
> 

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[David Hildenbrand]

On 01.06.22 00:25, Eric DeVolder wrote:
> 
> 
> On 5/31/22 08:15, David Hildenbrand wrote:
>> On 12.05.22 18:10, Eric DeVolder wrote:
>>> David,
>>> Great questions! See inline responses below.
>>> eric
>>
>> Sorry for the late reply, travel and vacation ...
> No problem, greatly appreciate the feedback!
> eric
> 
>>
>>>>
>>>>> +
>>>>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>>>>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>>>>> +							unsigned int hp_action, unsigned int cpu)
>>>>> +{
>>>>> +	WARN(1, "crash hotplug handler not implemented");
>>>>
>>>>
>>>> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
>>>> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
>>>> triggering?
>>>>
>>> You're correct. What about: printk_once(KERN_DEBUG "...") ?
>>
>> Why even bother about printing anything? If the feature is not
>> supported, there should be some way for user space to figure out that it
>> sill has to reload on hot(un)plug manually, no?
> 
> I've changed this to WARN_ONCE(). If that isn't agreeable, I'll remove it.

Please don't use WARN* on expected error paths.

-- 
Thanks,

David / dhildenb

Re: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support

[David Hildenbrand]

On 01.06.22 00:25, Eric DeVolder wrote:
> 
> 
> On 5/31/22 08:15, David Hildenbrand wrote:
>> On 12.05.22 18:10, Eric DeVolder wrote:
>>> David,
>>> Great questions! See inline responses below.
>>> eric
>>
>> Sorry for the late reply, travel and vacation ...
> No problem, greatly appreciate the feedback!
> eric
> 
>>
>>>>
>>>>> +
>>>>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>>>>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>>>>> +							unsigned int hp_action, unsigned int cpu)
>>>>> +{
>>>>> +	WARN(1, "crash hotplug handler not implemented");
>>>>
>>>>
>>>> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
>>>> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
>>>> triggering?
>>>>
>>> You're correct. What about: printk_once(KERN_DEBUG "...") ?
>>
>> Why even bother about printing anything? If the feature is not
>> supported, there should be some way for user space to figure out that it
>> sill has to reload on hot(un)plug manually, no?
> 
> I've changed this to WARN_ONCE(). If that isn't agreeable, I'll remove it.

Please don't use WARN* on expected error paths.

-- 
Thanks,

David / dhildenb


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec