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

[Sourabh Jain]

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