[v15,2/5] crash: add a new kexec flag for hotplug support

Commit a72bbec70da2 ("crash: hotplug support for kexec_load()")
introduced a new kexec flag, `KEXEC_UPDATE_ELFCOREHDR`. Kexec tool uses
this flag to indicate to the kernel that it is safe to modify the
elfcorehdr of the kdump image loaded using the kexec_load system call.

However, it is possible that architectures may need to update kexec
segments other then elfcorehdr. For example, FDT (Flatten Device Tree)
on PowerPC. Introducing a new kexec flag for every new kexec segment
may not be a good solution. Hence, a generic kexec flag bit,
`KEXEC_CRASH_HOTPLUG_SUPPORT`, is introduced to share the CPU/Memory
hotplug support intent between the kexec tool and the kernel for the
kexec_load system call.

Now, if the kexec tool sends KEXEC_CRASH_HOTPLUG_SUPPORT kexec flag to
the kernel, it indicates to the kernel that all the required kexec
segment is skipped from SHA calculation and it is safe to update kdump
image loaded using the kexec_load syscall.

While loading the kdump image using the kexec_load syscall, the
@update_elfcorehdr member of struct kimage is set if the kexec tool
sends the KEXEC_UPDATE_ELFCOREHDR kexec flag. This member is later used
to determine whether it is safe to update elfcorehdr on hotplug events.
However, with the introduction of the KEXEC_CRASH_HOTPLUG_SUPPORT kexec
flag, the kexec tool could mark all the required kexec segments on an
architecture as safe to update. So rename the @update_elfcorehdr to
@hotplug_support. If @hotplug_support is set, the kernel can safely
update all the required kexec segments of the kdump image during
CPU/Memory hotplug events.

Introduce an architecture-specific function to process kexec flags for
determining hotplug support. Set the @hotplug_support member of struct
kimage for both kexec_load and kexec_file_load system calls. This
simplifies kernel checks to identify hotplug support for the currently
loaded kdump image by just examining the value of @hotplug_support.

Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
Cc: Akhil Raj <lf32.dev@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Aneesh Kumar K.V <aneesh.kumar@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Borislav Petkov (AMD) <bp@alien8.de>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Eric DeVolder <eric.devolder@oracle.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Laurent Dufour <laurent.dufour@fr.ibm.com>
Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Naveen N Rao <naveen@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Valentin Schneider <vschneid@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: kexec@lists.infradead.org
Cc: x86@kernel.org
---
 arch/x86/include/asm/kexec.h |  3 +++
 arch/x86/kernel/crash.c      | 18 +++++++-----------
 drivers/base/cpu.c           |  2 +-
 drivers/base/memory.c        |  2 +-
 include/linux/kexec.h        | 25 +++++++++++++++----------
 include/uapi/linux/kexec.h   |  1 +
 kernel/crash_core.c          | 11 ++++-------
 kernel/kexec.c               |  4 ++--
 kernel/kexec_file.c          |  5 +++++
 9 files changed, 39 insertions(+), 32 deletions(-)

Hi Sourabh,

Thanks for the great work. There are some concerns, please see inline
comments.

On 01/11/24 at 04:21pm, Sourabh Jain wrote:
......
> Now, if the kexec tool sends KEXEC_CRASH_HOTPLUG_SUPPORT kexec flag to
> the kernel, it indicates to the kernel that all the required kexec
> segment is skipped from SHA calculation and it is safe to update kdump
> image loaded using the kexec_load syscall.

So finally you add a new KEXEC_CRASH_HOTPLUG_SUPPORT flag, that's fine.

> 
...... 
> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> index 9bb6607e864e..e791129fdf6c 100644
> --- a/arch/x86/include/asm/kexec.h
> +++ b/arch/x86/include/asm/kexec.h
> @@ -211,6 +211,9 @@ extern void kdump_nmi_shootdown_cpus(void);
>  void arch_crash_handle_hotplug_event(struct kimage *image, void *arg);
>  #define arch_crash_handle_hotplug_event arch_crash_handle_hotplug_event
>  
> +int arch_crash_hotplug_support(struct kimage *image, unsigned long kexec_flags);
> +#define arch_crash_hotplug_support arch_crash_hotplug_support
> +
>  #ifdef CONFIG_HOTPLUG_CPU
>  int arch_crash_hotplug_cpu_support(void);
>  #define crash_hotplug_cpu_support arch_crash_hotplug_cpu_support

Then crash_hotplug_cpu_support is not needed any more on x86_64, and
crash_hotplug_memory_support(), if you remove their implementation in
arch/x86/kernel/crash.c, won't it cause building warning or error on x86?

> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 44744e9c68ec..293b54bff706 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -398,20 +398,16 @@ int crash_load_segments(struct kimage *image)
>  #undef pr_fmt
>  #define pr_fmt(fmt) "crash hp: " fmt
>  
> -/* These functions provide the value for the sysfs crash_hotplug nodes */
> -#ifdef CONFIG_HOTPLUG_CPU
> -int arch_crash_hotplug_cpu_support(void)
> +int arch_crash_hotplug_support(struct kimage *image, unsigned long kexec_flags)
>  {
> -	return crash_check_update_elfcorehdr();
> -}
> -#endif
>  
> -#ifdef CONFIG_MEMORY_HOTPLUG
> -int arch_crash_hotplug_memory_support(void)
> -{
> -	return crash_check_update_elfcorehdr();
> -}
> +#ifdef CONFIG_KEXEC_FILE
> +	if (image->file_mode)
> +		return 1;
>  #endif
> +	return (kexec_flags & KEXEC_UPDATE_ELFCOREHDR ||
> +		kexec_flags & KEXEC_CRASH_HOTPLUG_SUPPORT);

Do we need add some document to tell why there are two kexec flags on
x86_64, except of checking this patch log?

> +}
>  
>  unsigned int arch_crash_get_elfcorehdr_size(void)
>  {
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 548491de818e..2f411ddfbd8b 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -306,7 +306,7 @@ static ssize_t crash_hotplug_show(struct device *dev,
>  				     struct device_attribute *attr,
>  				     char *buf)
>  {
> -	return sysfs_emit(buf, "%d\n", crash_hotplug_cpu_support());
> +	return sysfs_emit(buf, "%d\n", crash_check_hotplug_support());
>  }
>  static DEVICE_ATTR_ADMIN_RO(crash_hotplug);
>  #endif
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 8a13babd826c..e70ab1d3428e 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -514,7 +514,7 @@ static DEVICE_ATTR_RW(auto_online_blocks);
>  static ssize_t crash_hotplug_show(struct device *dev,
>  				       struct device_attribute *attr, char *buf)
>  {
> -	return sysfs_emit(buf, "%d\n", crash_hotplug_memory_support());
> +	return sysfs_emit(buf, "%d\n", crash_check_hotplug_support());
>  }
>  static DEVICE_ATTR_RO(crash_hotplug);
>  #endif
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 802052d9c64b..7880d74dc5c4 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -317,8 +317,8 @@ struct kimage {
>  	/* If set, we are using file mode kexec syscall */
>  	unsigned int file_mode:1;
>  #ifdef CONFIG_CRASH_HOTPLUG
> -	/* If set, allow changes to elfcorehdr of kexec_load'd image */
> -	unsigned int update_elfcorehdr:1;
> +	/* If set, allow changes to kexec segments of kexec_load'd image */

The code comment doesn't reflect the usage of the flag. You set it too
when it's kexec_file_load. Speaking of this, I do wonder why you need
set it too for kexec_file_load, and why we have
arch_crash_hotplug_support(), then crash_check_hotplug_support() both of
which have the same effect.

> +	unsigned int hotplug_support:1;
>  #endif
>  
>  #ifdef ARCH_HAS_KIMAGE_ARCH
> @@ -396,9 +396,10 @@ bool kexec_load_permitted(int kexec_image_type);
>  
>  /* List of defined/legal kexec flags */
>  #ifndef CONFIG_KEXEC_JUMP
> -#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_UPDATE_ELFCOREHDR)
> +#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_UPDATE_ELFCOREHDR | KEXEC_CRASH_HOTPLUG_SUPPORT)
>  #else
> -#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_PRESERVE_CONTEXT | KEXEC_UPDATE_ELFCOREHDR)
> +#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_PRESERVE_CONTEXT | KEXEC_UPDATE_ELFCOREHDR | \
> +			KEXEC_CRASH_HOTPLUG_SUPPORT)
>  #endif
>  
>  /* List of defined/legal kexec file flags */
> @@ -486,14 +487,18 @@ static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) {
>  static inline void arch_crash_handle_hotplug_event(struct kimage *image, void *arg) { }
>  #endif
>  
> -int crash_check_update_elfcorehdr(void);
> +int crash_check_hotplug_support(void);
>  
> -#ifndef crash_hotplug_cpu_support
> -static inline int crash_hotplug_cpu_support(void) { return 0; }
> -#endif
> +#ifndef arch_crash_hotplug_support
> +static inline int arch_crash_hotplug_support(struct kimage *image, unsigned long kexec_flags)
> +{
>  
> -#ifndef crash_hotplug_memory_support
> -static inline int crash_hotplug_memory_support(void) { return 0; }
> +#ifdef CONFIG_KEXEC_FILE
> +	if (image->file_mode)
> +		return 1;
> +#endif
> +	return kexec_flags & KEXEC_CRASH_HOTPLUG_SUPPORT;
> +}
>  #endif
>  
>  #ifndef crash_get_elfcorehdr_size
> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> index c17bb096ea68..5ae1741ea8ea 100644
> --- a/include/uapi/linux/kexec.h
> +++ b/include/uapi/linux/kexec.h
> @@ -13,6 +13,7 @@
>  #define KEXEC_ON_CRASH		0x00000001
>  #define KEXEC_PRESERVE_CONTEXT	0x00000002
>  #define KEXEC_UPDATE_ELFCOREHDR	0x00000004
> +#define KEXEC_CRASH_HOTPLUG_SUPPORT 0x00000008
>  #define KEXEC_ARCH_MASK		0xffff0000
>  
>  /*
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index ab1c8e79759d..111548ad03e9 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -876,7 +876,7 @@ DEFINE_MUTEX(__crash_hotplug_lock);
>   * It reflects the kernel's ability/permission to update the crash
>   * elfcorehdr directly.
>   */
> -int crash_check_update_elfcorehdr(void)
> +int crash_check_hotplug_support(void)
>  {
>  	int rc = 0;
>  
> @@ -888,10 +888,7 @@ int crash_check_update_elfcorehdr(void)
>  		return 0;
>  	}
>  	if (kexec_crash_image) {
> -		if (kexec_crash_image->file_mode)
> -			rc = 1;
> -		else
> -			rc = kexec_crash_image->update_elfcorehdr;
> +		rc = kexec_crash_image->hotplug_support;
>  	}
>  	/* Release lock now that update complete */
>  	kexec_unlock();
> @@ -932,8 +929,8 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu,
>  
>  	image = kexec_crash_image;
>  
> -	/* Check that updating elfcorehdr is permitted */
> -	if (!(image->file_mode || image->update_elfcorehdr))
> +	/* Check that kexec segments update is permitted */
> +	if (!image->hotplug_support)
>  		goto out;
>  
>  	if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 8f35a5a42af8..9dc5b7ed5b73 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -130,8 +130,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
>  		image->preserve_context = 1;
>  
>  #ifdef CONFIG_CRASH_HOTPLUG
> -	if (flags & KEXEC_UPDATE_ELFCOREHDR)
> -		image->update_elfcorehdr = 1;
> +	if ((flags & KEXEC_ON_CRASH) && arch_crash_hotplug_support(image, flags))
> +		image->hotplug_support = 1;
>  #endif
>  
>  	ret = machine_kexec_prepare(image);
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index bef2f6f2571b..4dddf9264a69 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -373,6 +373,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  	if (ret)
>  		goto out;
>  
> +#ifdef CONFIG_CRASH_HOTPLUG
> +	if ((flags & KEXEC_FILE_ON_CRASH) && arch_crash_hotplug_support(image, flags))
> +		image->hotplug_support = 1;
> +#endif
> +
>  	ret = machine_kexec_prepare(image);
>  	if (ret)
>  		goto out;
> -- 
> 2.41.0
>

Hello Baoquan,

On 05/02/24 08:40, Baoquan He wrote:
> Hi Sourabh,
>
> Thanks for the great work. There are some concerns, please see inline
> comments.

Thank you :)

>
> On 01/11/24 at 04:21pm, Sourabh Jain wrote:
> ......
>> Now, if the kexec tool sends KEXEC_CRASH_HOTPLUG_SUPPORT kexec flag to
>> the kernel, it indicates to the kernel that all the required kexec
>> segment is skipped from SHA calculation and it is safe to update kdump
>> image loaded using the kexec_load syscall.
> So finally you add a new KEXEC_CRASH_HOTPLUG_SUPPORT flag, that's fine.
>
> ......
>> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
>> index 9bb6607e864e..e791129fdf6c 100644
>> --- a/arch/x86/include/asm/kexec.h
>> +++ b/arch/x86/include/asm/kexec.h
>> @@ -211,6 +211,9 @@ extern void kdump_nmi_shootdown_cpus(void);
>>   void arch_crash_handle_hotplug_event(struct kimage *image, void *arg);
>>   #define arch_crash_handle_hotplug_event arch_crash_handle_hotplug_event
>>   
>> +int arch_crash_hotplug_support(struct kimage *image, unsigned long kexec_flags);
>> +#define arch_crash_hotplug_support arch_crash_hotplug_support
>> +
>>   #ifdef CONFIG_HOTPLUG_CPU
>>   int arch_crash_hotplug_cpu_support(void);
>>   #define crash_hotplug_cpu_support arch_crash_hotplug_cpu_support
> Then crash_hotplug_cpu_support is not needed any more on x86_64, and
> crash_hotplug_memory_support(), if you remove their implementation in
> arch/x86/kernel/crash.c, won't it cause building warning or error on x86?

Yeah, crash_hotplug_cpu_support and crash_hotplug_memory_support are
no longer required. My bad, I forgot to remove them.

>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>> index 44744e9c68ec..293b54bff706 100644
>> --- a/arch/x86/kernel/crash.c
>> +++ b/arch/x86/kernel/crash.c
>> @@ -398,20 +398,16 @@ int crash_load_segments(struct kimage *image)
>>   #undef pr_fmt
>>   #define pr_fmt(fmt) "crash hp: " fmt
>>   
>> -/* These functions provide the value for the sysfs crash_hotplug nodes */
>> -#ifdef CONFIG_HOTPLUG_CPU
>> -int arch_crash_hotplug_cpu_support(void)
>> +int arch_crash_hotplug_support(struct kimage *image, unsigned long kexec_flags)
>>   {
>> -	return crash_check_update_elfcorehdr();
>> -}
>> -#endif
>>   
>> -#ifdef CONFIG_MEMORY_HOTPLUG
>> -int arch_crash_hotplug_memory_support(void)
>> -{
>> -	return crash_check_update_elfcorehdr();
>> -}
>> +#ifdef CONFIG_KEXEC_FILE
>> +	if (image->file_mode)
>> +		return 1;
>>   #endif
>> +	return (kexec_flags & KEXEC_UPDATE_ELFCOREHDR ||
>> +		kexec_flags & KEXEC_CRASH_HOTPLUG_SUPPORT);
> Do we need add some document to tell why there are two kexec flags on
> x86_64, except of checking this patch log?

Sure I will add a comment about it.

>
>> +}
>>   
>>   unsigned int arch_crash_get_elfcorehdr_size(void)
>>   {
>> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
>> index 548491de818e..2f411ddfbd8b 100644
>> --- a/drivers/base/cpu.c
>> +++ b/drivers/base/cpu.c
>> @@ -306,7 +306,7 @@ static ssize_t crash_hotplug_show(struct device *dev,
>>   				     struct device_attribute *attr,
>>   				     char *buf)
>>   {
>> -	return sysfs_emit(buf, "%d\n", crash_hotplug_cpu_support());
>> +	return sysfs_emit(buf, "%d\n", crash_check_hotplug_support());
>>   }
>>   static DEVICE_ATTR_ADMIN_RO(crash_hotplug);
>>   #endif
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 8a13babd826c..e70ab1d3428e 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -514,7 +514,7 @@ static DEVICE_ATTR_RW(auto_online_blocks);
>>   static ssize_t crash_hotplug_show(struct device *dev,
>>   				       struct device_attribute *attr, char *buf)
>>   {
>> -	return sysfs_emit(buf, "%d\n", crash_hotplug_memory_support());
>> +	return sysfs_emit(buf, "%d\n", crash_check_hotplug_support());
>>   }
>>   static DEVICE_ATTR_RO(crash_hotplug);
>>   #endif
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 802052d9c64b..7880d74dc5c4 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -317,8 +317,8 @@ struct kimage {
>>   	/* If set, we are using file mode kexec syscall */
>>   	unsigned int file_mode:1;
>>   #ifdef CONFIG_CRASH_HOTPLUG
>> -	/* If set, allow changes to elfcorehdr of kexec_load'd image */
>> -	unsigned int update_elfcorehdr:1;
>> +	/* If set, allow changes to kexec segments of kexec_load'd image */
> The code comment doesn't reflect the usage of the flag.
I should have updated the comment to indicate that this flag is for both 
system calls.
More comments below.

> You set it too
> when it's kexec_file_load. Speaking of this, I do wonder why you need
> set it too for kexec_file_load,
If we do this one can just access image->hotplug_support to find hotplug
support for currently loaded kdump image without bothering about which
system call was used to load the kdump image.

> and why we have
> arch_crash_hotplug_support(), then crash_check_hotplug_support() both of
> which have the same effect.

arch_crash_hotplug_support(): This function processes the kexec flags 
and finds the
hotplug support for the kdump image. Based on the return value of this 
function,
the image->hotplug_support attribute is set.

Now, once the kdump image is loaded, we no longer have access to the 
kexec flags.
Therefore, crash_check_hotplug_support simply returns the value of 
image->hotplug_support
when user space accesses the following sysfs files: 
/sys/devices/system/[cpu|memory]/crash_hotplug.

To keep things simple, I have introduced two functions: One function 
processes the kexec flags
and determines the hotplug support for the image being loaded. And other 
function simply
accesses image->hotplug_support and advertises CPU/Memory hotplug 
support to userspace.

Let me know you opinion.

Thanks for reviewing the patch.

- Sourabh Jain
>
>> +	unsigned int hotplug_support:1;
>>   #endif
>>   
>>   #ifdef ARCH_HAS_KIMAGE_ARCH
>> @@ -396,9 +396,10 @@ bool kexec_load_permitted(int kexec_image_type);
>>   
>>   /* List of defined/legal kexec flags */
>>   #ifndef CONFIG_KEXEC_JUMP
>> -#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_UPDATE_ELFCOREHDR)
>> +#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_UPDATE_ELFCOREHDR | KEXEC_CRASH_HOTPLUG_SUPPORT)
>>   #else
>> -#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_PRESERVE_CONTEXT | KEXEC_UPDATE_ELFCOREHDR)
>> +#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_PRESERVE_CONTEXT | KEXEC_UPDATE_ELFCOREHDR | \
>> +			KEXEC_CRASH_HOTPLUG_SUPPORT)
>>   #endif
>>   
>>   /* List of defined/legal kexec file flags */
>> @@ -486,14 +487,18 @@ static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) {
>>   static inline void arch_crash_handle_hotplug_event(struct kimage *image, void *arg) { }
>>   #endif
>>   
>> -int crash_check_update_elfcorehdr(void);
>> +int crash_check_hotplug_support(void);
>>   
>> -#ifndef crash_hotplug_cpu_support
>> -static inline int crash_hotplug_cpu_support(void) { return 0; }
>> -#endif
>> +#ifndef arch_crash_hotplug_support
>> +static inline int arch_crash_hotplug_support(struct kimage *image, unsigned long kexec_flags)
>> +{
>>   
>> -#ifndef crash_hotplug_memory_support
>> -static inline int crash_hotplug_memory_support(void) { return 0; }
>> +#ifdef CONFIG_KEXEC_FILE
>> +	if (image->file_mode)
>> +		return 1;
>> +#endif
>> +	return kexec_flags & KEXEC_CRASH_HOTPLUG_SUPPORT;
>> +}
>>   #endif
>>   
>>   #ifndef crash_get_elfcorehdr_size
>> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
>> index c17bb096ea68..5ae1741ea8ea 100644
>> --- a/include/uapi/linux/kexec.h
>> +++ b/include/uapi/linux/kexec.h
>> @@ -13,6 +13,7 @@
>>   #define KEXEC_ON_CRASH		0x00000001
>>   #define KEXEC_PRESERVE_CONTEXT	0x00000002
>>   #define KEXEC_UPDATE_ELFCOREHDR	0x00000004
>> +#define KEXEC_CRASH_HOTPLUG_SUPPORT 0x00000008
>>   #define KEXEC_ARCH_MASK		0xffff0000
>>   
>>   /*
>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>> index ab1c8e79759d..111548ad03e9 100644
>> --- a/kernel/crash_core.c
>> +++ b/kernel/crash_core.c
>> @@ -876,7 +876,7 @@ DEFINE_MUTEX(__crash_hotplug_lock);
>>    * It reflects the kernel's ability/permission to update the crash
>>    * elfcorehdr directly.
>>    */
>> -int crash_check_update_elfcorehdr(void)
>> +int crash_check_hotplug_support(void)
>>   {
>>   	int rc = 0;
>>   
>> @@ -888,10 +888,7 @@ int crash_check_update_elfcorehdr(void)
>>   		return 0;
>>   	}
>>   	if (kexec_crash_image) {
>> -		if (kexec_crash_image->file_mode)
>> -			rc = 1;
>> -		else
>> -			rc = kexec_crash_image->update_elfcorehdr;
>> +		rc = kexec_crash_image->hotplug_support;
>>   	}
>>   	/* Release lock now that update complete */
>>   	kexec_unlock();
>> @@ -932,8 +929,8 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu,
>>   
>>   	image = kexec_crash_image;
>>   
>> -	/* Check that updating elfcorehdr is permitted */
>> -	if (!(image->file_mode || image->update_elfcorehdr))
>> +	/* Check that kexec segments update is permitted */
>> +	if (!image->hotplug_support)
>>   		goto out;
>>   
>>   	if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
>> diff --git a/kernel/kexec.c b/kernel/kexec.c
>> index 8f35a5a42af8..9dc5b7ed5b73 100644
>> --- a/kernel/kexec.c
>> +++ b/kernel/kexec.c
>> @@ -130,8 +130,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
>>   		image->preserve_context = 1;
>>   
>>   #ifdef CONFIG_CRASH_HOTPLUG
>> -	if (flags & KEXEC_UPDATE_ELFCOREHDR)
>> -		image->update_elfcorehdr = 1;
>> +	if ((flags & KEXEC_ON_CRASH) && arch_crash_hotplug_support(image, flags))
>> +		image->hotplug_support = 1;
>>   #endif
>>   
>>   	ret = machine_kexec_prepare(image);
>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
>> index bef2f6f2571b..4dddf9264a69 100644
>> --- a/kernel/kexec_file.c
>> +++ b/kernel/kexec_file.c
>> @@ -373,6 +373,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>>   	if (ret)
>>   		goto out;
>>   
>> +#ifdef CONFIG_CRASH_HOTPLUG
>> +	if ((flags & KEXEC_FILE_ON_CRASH) && arch_crash_hotplug_support(image, flags))
>> +		image->hotplug_support = 1;
>> +#endif
>> +
>>   	ret = machine_kexec_prepare(image);
>>   	if (ret)
>>   		goto out;
>> -- 
>> 2.41.0
>>

On 02/12/24 at 07:27pm, Sourabh Jain wrote:
> Hello Baoquan,
> 
> On 05/02/24 08:40, Baoquan He wrote:
> > Hi Sourabh,
> > 
......
> > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > index 802052d9c64b..7880d74dc5c4 100644
> > > --- a/include/linux/kexec.h
> > > +++ b/include/linux/kexec.h
> > > @@ -317,8 +317,8 @@ struct kimage {
> > >   	/* If set, we are using file mode kexec syscall */
> > >   	unsigned int file_mode:1;
> > >   #ifdef CONFIG_CRASH_HOTPLUG
> > > -	/* If set, allow changes to elfcorehdr of kexec_load'd image */
> > > -	unsigned int update_elfcorehdr:1;
> > > +	/* If set, allow changes to kexec segments of kexec_load'd image */
> > The code comment doesn't reflect the usage of the flag.
> I should have updated the comment to indicate that this flag is for both
> system calls.
> More comments below.
> 
> > You set it too
> > when it's kexec_file_load. Speaking of this, I do wonder why you need
> > set it too for kexec_file_load,
> If we do this one can just access image->hotplug_support to find hotplug
> support for currently loaded kdump image without bothering about which
> system call was used to load the kdump image.
> 
> > and why we have
> > arch_crash_hotplug_support(), then crash_check_hotplug_support() both of
> > which have the same effect.
> 
> arch_crash_hotplug_support(): This function processes the kexec flags and
> finds the
> hotplug support for the kdump image. Based on the return value of this
> function,
> the image->hotplug_support attribute is set.
> 
> Now, once the kdump image is loaded, we no longer have access to the kexec
> flags.
> Therefore, crash_check_hotplug_support simply returns the value of
> image->hotplug_support
> when user space accesses the following sysfs files:
> /sys/devices/system/[cpu|memory]/crash_hotplug.
> 
> To keep things simple, I have introduced two functions: One function
> processes the kexec flags
> and determines the hotplug support for the image being loaded. And other
> function simply
> accesses image->hotplug_support and advertises CPU/Memory hotplug support to
> userspace.

From the function name and their functionality, they seems to be
duplicated, even though it's different from the internal detail. This
could bring a little confusion to code understanding. It's fine, we can
refactor them if needed in the future. So let's keep it as the patch is.
Thanks.

> 
> > 
> > > +	unsigned int hotplug_support:1;
> > >   #endif
> > >   #ifdef ARCH_HAS_KIMAGE_ARCH
> > > @@ -396,9 +396,10 @@ bool kexec_load_permitted(int kexec_image_type);
> > >   /* List of defined/legal kexec flags */
> > >   #ifndef CONFIG_KEXEC_JUMP
> > > -#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_UPDATE_ELFCOREHDR)
> > > +#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_UPDATE_ELFCOREHDR | KEXEC_CRASH_HOTPLUG_SUPPORT)
> > >   #else
> > > -#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_PRESERVE_CONTEXT | KEXEC_UPDATE_ELFCOREHDR)
> > > +#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_PRESERVE_CONTEXT | KEXEC_UPDATE_ELFCOREHDR | \
> > > +			KEXEC_CRASH_HOTPLUG_SUPPORT)
> > >   #endif
> > >   /* List of defined/legal kexec file flags */
> > > @@ -486,14 +487,18 @@ static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) {
> > >   static inline void arch_crash_handle_hotplug_event(struct kimage *image, void *arg) { }
> > >   #endif
> > > -int crash_check_update_elfcorehdr(void);
> > > +int crash_check_hotplug_support(void);
> > > -#ifndef crash_hotplug_cpu_support
> > > -static inline int crash_hotplug_cpu_support(void) { return 0; }
> > > -#endif
> > > +#ifndef arch_crash_hotplug_support
> > > +static inline int arch_crash_hotplug_support(struct kimage *image, unsigned long kexec_flags)
> > > +{
> > > -#ifndef crash_hotplug_memory_support
> > > -static inline int crash_hotplug_memory_support(void) { return 0; }
> > > +#ifdef CONFIG_KEXEC_FILE
> > > +	if (image->file_mode)
> > > +		return 1;
> > > +#endif
> > > +	return kexec_flags & KEXEC_CRASH_HOTPLUG_SUPPORT;
> > > +}
> > >   #endif
> > >   #ifndef crash_get_elfcorehdr_size
......

On 13/02/24 08:51, Baoquan He wrote:
> On 02/12/24 at 07:27pm, Sourabh Jain wrote:
>> Hello Baoquan,
>>
>> On 05/02/24 08:40, Baoquan He wrote:
>>> Hi Sourabh,
>>>
> ......
>>>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>>>> index 802052d9c64b..7880d74dc5c4 100644
>>>> --- a/include/linux/kexec.h
>>>> +++ b/include/linux/kexec.h
>>>> @@ -317,8 +317,8 @@ struct kimage {
>>>>    	/* If set, we are using file mode kexec syscall */
>>>>    	unsigned int file_mode:1;
>>>>    #ifdef CONFIG_CRASH_HOTPLUG
>>>> -	/* If set, allow changes to elfcorehdr of kexec_load'd image */
>>>> -	unsigned int update_elfcorehdr:1;
>>>> +	/* If set, allow changes to kexec segments of kexec_load'd image */
>>> The code comment doesn't reflect the usage of the flag.
>> I should have updated the comment to indicate that this flag is for both
>> system calls.
>> More comments below.
>>
>>> You set it too
>>> when it's kexec_file_load. Speaking of this, I do wonder why you need
>>> set it too for kexec_file_load,
>> If we do this one can just access image->hotplug_support to find hotplug
>> support for currently loaded kdump image without bothering about which
>> system call was used to load the kdump image.
>>
>>> and why we have
>>> arch_crash_hotplug_support(), then crash_check_hotplug_support() both of
>>> which have the same effect.
>> arch_crash_hotplug_support(): This function processes the kexec flags and
>> finds the
>> hotplug support for the kdump image. Based on the return value of this
>> function,
>> the image->hotplug_support attribute is set.
>>
>> Now, once the kdump image is loaded, we no longer have access to the kexec
>> flags.
>> Therefore, crash_check_hotplug_support simply returns the value of
>> image->hotplug_support
>> when user space accesses the following sysfs files:
>> /sys/devices/system/[cpu|memory]/crash_hotplug.
>>
>> To keep things simple, I have introduced two functions: One function
>> processes the kexec flags
>> and determines the hotplug support for the image being loaded. And other
>> function simply
>> accesses image->hotplug_support and advertises CPU/Memory hotplug support to
>> userspace.
>  From the function name and their functionality, they seems to be
> duplicated, even though it's different from the internal detail. This
> could bring a little confusion to code understanding. It's fine, we can
> refactor them if needed in the future. So let's keep it as the patch is.
> Thanks.
Ok sure.

- Sourabh