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

From: Sourabh Jain <sourabhjain@linux.ibm.com>
To: Baoquan He <bhe@redhat.com>
Cc: linuxppc-dev@ozlabs.org, Akhil Raj <lf32.dev@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	"Aneesh Kumar K . V" <aneesh.kumar@kernel.org>,
	Borislav Petkov <bp@alien8.de>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Christophe Leroy <christophe.leroy@csgroup.eu>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Dave Young <dyoung@redhat.com>,
	David Hildenbrand <david@redhat.com>,
	Eric DeVolder <eric.devolder@oracle.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Hari Bathini <hbathini@linux.ibm.com>,
	Laurent Dufour <laurent.dufour@fr.ibm.com>,
	Mahesh Salgaonkar <mahesh@linux.ibm.com>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Mimi Zohar <zohar@linux.ibm.com>,
	Naveen N Rao <naveen@kernel.org>,
	Oscar Salvador <osalvador@suse.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Valentin Schneider <vschneid@redhat.com>,
	Vivek Goyal <vgoyal@redhat.com>,
	kexec@lists.infradead.org, x86@kernel.org
Subject: Re: [PATCH v15 2/5] crash: add a new kexec flag for hotplug support
Date: Wed, 14 Feb 2024 19:05:55 +0530	[thread overview]
Message-ID: <7c5f8e9a-b6d4-4ebf-9bed-4b9bddb62e25@linux.ibm.com> (raw)
In-Reply-To: <ZcrgHvTbDGbXc894@MiWiFi-R3L-srv>


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

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