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

[Baoquan He]

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
......