From: Sourabh Jain <sourabhjain@linux.ibm.com>
To: Baoquan He <bhe@redhat.com>
Cc: David Hildenbrand <david@redhat.com>,
Dave Hansen <dave.hansen@linux.intel.com>,
Mimi Zohar <zohar@linux.ibm.com>,
linuxppc-dev@ozlabs.org, Eric DeVolder <eric.devolder@oracle.com>,
Boris Ostrovsky <boris.ostrovsky@oracle.com>,
Valentin Schneider <vschneid@redhat.com>,
x86@kernel.org, "Aneesh Kumar K . V" <aneesh.kumar@kernel.org>,
Laurent Dufour <laurent.dufour@fr.ibm.com>,
Dave Young <dyoung@redhat.com>, Vivek Goyal <vgoyal@redhat.com>,
Naveen N Rao <naveen@kernel.org>, Borislav Petkov <bp@alien8.de>,
Thomas Gleixner <tglx@linutronix.de>,
Hari Bathini <hbathini@linux.ibm.com>,
Oscar Salvador <osalvador@suse.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
kexec@lists.infradead.org,
Mahesh Salgaonkar <mahesh@linux.ibm.com>,
Akhil Raj <lf32.dev@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v15 2/5] crash: add a new kexec flag for hotplug support
Date: Mon, 12 Feb 2024 19:27:02 +0530 [thread overview]
Message-ID: <cc02538f-9460-4cbb-9ae4-194412b85e36@linux.ibm.com> (raw)
In-Reply-To: <ZcBRle7szFrD3cW+@MiWiFi-R3L-srv>
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
>>
next prev parent reply other threads:[~2024-02-12 14:03 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-11 10:51 [PATCH v15 0/5] powerpc/crash: Kernel handling of CPU and memory hotplug Sourabh Jain
2024-01-11 10:51 ` [PATCH v15 1/5] crash: forward memory_notify arg to arch crash hotplug handler Sourabh Jain
2024-02-05 3:11 ` Baoquan He
2024-02-12 11:57 ` Sourabh Jain
2024-01-11 10:51 ` [PATCH v15 2/5] crash: add a new kexec flag for hotplug support Sourabh Jain
2024-02-05 3:10 ` Baoquan He
2024-02-12 13:57 ` Sourabh Jain [this message]
2024-02-13 3:21 ` Baoquan He
2024-02-14 13:35 ` Sourabh Jain
2024-01-11 10:51 ` [PATCH v15 3/5] powerpc/kexec: turn some static helper functions public Sourabh Jain
2024-01-11 10:51 ` [PATCH v15 4/5] powerpc: add crash CPU hotplug support Sourabh Jain
2024-01-23 10:16 ` Hari Bathini
2024-01-11 10:51 ` [PATCH v15 5/5] powerpc: add crash memory " Sourabh Jain
2024-01-23 10:22 ` Hari Bathini
2024-01-29 6:46 ` Sourabh Jain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cc02538f-9460-4cbb-9ae4-194412b85e36@linux.ibm.com \
--to=sourabhjain@linux.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=aneesh.kumar@kernel.org \
--cc=bhe@redhat.com \
--cc=boris.ostrovsky@oracle.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=david@redhat.com \
--cc=dyoung@redhat.com \
--cc=eric.devolder@oracle.com \
--cc=gregkh@linuxfoundation.org \
--cc=hbathini@linux.ibm.com \
--cc=kexec@lists.infradead.org \
--cc=laurent.dufour@fr.ibm.com \
--cc=lf32.dev@gmail.com \
--cc=linuxppc-dev@ozlabs.org \
--cc=mahesh@linux.ibm.com \
--cc=naveen@kernel.org \
--cc=osalvador@suse.de \
--cc=tglx@linutronix.de \
--cc=vgoyal@redhat.com \
--cc=vschneid@redhat.com \
--cc=x86@kernel.org \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).