For x86_64, when CPU or memory is hot un/plugged, the crash
elfcorehdr, which describes the CPUs and memory in the system,
must also be updated.
To update the elfcorehdr for x86_64, a new elfcorehdr must be
generated from the available CPUs and memory. The new elfcorehdr
is prepared into a buffer, and if no errors occur, it is
installed over the top of the existing elfcorehdr.
In the patch 'crash hp: kexec_file changes for crash hotplug support'
the need to update purgatory due to the change in elfcorehdr was
eliminated. As a result, no changes to purgatory or boot_params
(as the elfcorehdr= kernel command line parameter pointer
remains unchanged and correct) are needed, just elfcorehdr.
To accommodate a growing number of resources via hotplug, the
elfcorehdr segment must be sufficiently large enough to accommodate
changes, see the CRASH_HOTPLUG_ELFCOREHDR_SZ configure item.
NOTE that this supports both kexec_load and kexec_file_load. Support
for kexec_load is made possible by identifying the elfcorehdr segment
at load time and updating it as previously described. However, it is
the responsibility of the userspace kexec utility to ensure that:
- the elfcorehdr segment is sufficiently large enough to accommodate
hotplug changes, ala CRASH_HOTPLUG_ELFCOREHDR_SZ.
- provides a purgatory that excludes the elfcorehdr from its list of
run-time segments to check.
These changes to the userspace kexec utility are not yet available.
Signed-off-by: Eric DeVolder <[email protected]>
---
arch/x86/kernel/crash.c | 138 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 137 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9730c88530fc..d185137b33d4 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -25,6 +25,7 @@
#include <linux/slab.h>
#include <linux/vmalloc.h>
#include <linux/memblock.h>
+#include <linux/highmem.h>
#include <asm/processor.h>
#include <asm/hardirq.h>
@@ -265,7 +266,8 @@ static int prepare_elf_headers(struct kimage *image, void **addr,
goto out;
/* By default prepare 64bit headers */
- ret = crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
+ ret = crash_prepare_elf64_headers(image, cmem,
+ IS_ENABLED(CONFIG_X86_64), addr, sz);
out:
vfree(cmem);
@@ -397,7 +399,17 @@ int crash_load_segments(struct kimage *image)
image->elf_headers = kbuf.buffer;
image->elf_headers_sz = kbuf.bufsz;
+#ifdef CONFIG_CRASH_HOTPLUG
+ /* Ensure elfcorehdr segment large enough for hotplug changes */
+ kbuf.memsz = CONFIG_CRASH_HOTPLUG_ELFCOREHDR_SZ;
+ /* For marking as usable to crash kernel */
+ image->elf_headers_sz = kbuf.memsz;
+ /* Record the index of the elfcorehdr segment */
+ image->elf_index = image->nr_segments;
+ image->elf_index_valid = true;
+#else
kbuf.memsz = kbuf.bufsz;
+#endif
kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer(&kbuf);
@@ -412,3 +424,127 @@ int crash_load_segments(struct kimage *image)
return ret;
}
#endif /* CONFIG_KEXEC_FILE */
+
+#ifdef CONFIG_CRASH_HOTPLUG
+void *map_crash_pages(unsigned long paddr, unsigned long size)
+{
+ /*
+ * NOTE: The addresses and sizes passed to this routine have
+ * already been fully aligned on page boundaries. There is no
+ * need for massaging the address or size.
+ */
+ void *ptr = NULL;
+
+ /* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
+ if (size > 0) {
+ struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
+
+ ptr = kmap(page);
+ }
+
+ return ptr;
+}
+
+void unmap_crash_pages(void **ptr)
+{
+ if (ptr) {
+ if (*ptr)
+ kunmap(*ptr);
+ *ptr = NULL;
+ }
+}
+
+void arch_crash_hotplug_handler(struct kimage *image,
+ unsigned int hp_action, unsigned long a, unsigned long b)
+{
+ /*
+ * To accurately reflect hot un/plug changes, the elfcorehdr (which
+ * is passed to the crash kernel via the elfcorehdr= parameter)
+ * must be updated with the new list of CPUs and memories. The new
+ * elfcorehdr is prepared in a kernel buffer, and if no errors,
+ * then it is written on top of the existing/old elfcorehdr.
+ *
+ * Due to the change to the elfcorehdr, purgatory must explicitly
+ * exclude the elfcorehdr from the list of segments it checks.
+ */
+ struct kexec_segment *ksegment;
+ unsigned char *ptr = NULL;
+ unsigned long elfsz = 0;
+ void *elfbuf = NULL;
+ unsigned long mem, memsz;
+ unsigned int n;
+
+ /*
+ * When the struct kimage is alloced, it is wiped to zero, so
+ * the elf_index_valid defaults to false. It is set on the
+ * kexec_file_load path, or here for kexec_load.
+ */
+ if (!image->elf_index_valid) {
+ for (n = 0; n < image->nr_segments; n++) {
+ mem = image->segment[n].mem;
+ memsz = image->segment[n].memsz;
+ ptr = map_crash_pages(mem, memsz);
+ if (ptr) {
+ /* The segment containing elfcorehdr */
+ if ((ptr[0] == 0x7F) &&
+ (ptr[1] == 'E') &&
+ (ptr[2] == 'L') &&
+ (ptr[3] == 'F')) {
+ image->elf_index = (int)n;
+ image->elf_index_valid = true;
+ }
+ }
+ unmap_crash_pages((void **)&ptr);
+ }
+ }
+
+ /* Must have valid elfcorehdr index */
+ if (!image->elf_index_valid) {
+ pr_err("crash hp: unable to locate elfcorehdr segment");
+ goto out;
+ }
+
+ /*
+ * Create the new elfcorehdr reflecting the changes to CPU and/or
+ * memory resources. The elfcorehdr segment memsz must be
+ * sufficiently large to accommodate increases due to hotplug
+ * activity. See CRASH_HOTPLUG_ELFCOREHDR_SZ.
+ */
+ if (prepare_elf_headers(image, &elfbuf, &elfsz)) {
+ pr_err("crash hp: unable to prepare elfcore headers");
+ goto out;
+ }
+ ksegment = &image->segment[image->elf_index];
+ memsz = ksegment->memsz;
+ if (elfsz > memsz) {
+ pr_err("crash hp: update elfcorehdr elfsz %lu > memsz %lu",
+ elfsz, memsz);
+ goto out;
+ }
+
+ /*
+ * At this point, we are all but assured of success.
+ * Copy new elfcorehdr into destination.
+ */
+ ksegment = &image->segment[image->elf_index];
+ mem = ksegment->mem;
+ memsz = ksegment->memsz;
+ ptr = map_crash_pages(mem, memsz);
+ if (ptr) {
+ /* Temporarily invalidate the crash image while it is replaced */
+ xchg(&kexec_crash_image, NULL);
+ /* Write the new elfcorehdr into memory */
+ memcpy((void *)ptr, elfbuf, elfsz);
+ /* The crash image is now valid once again */
+ xchg(&kexec_crash_image, image);
+ }
+ unmap_crash_pages((void **)&ptr);
+ pr_debug("crash hp: re-loaded elfcorehdr at 0x%lx\n", mem);
+
+//FIX??? somekind of cache flush perhaps?
+
+out:
+ if (elfbuf)
+ vfree(elfbuf);
+}
+#endif /* CONFIG_CRASH_HOTPLUG */
--
2.27.0
On 01/10/22 at 02:57pm, Eric DeVolder wrote:
> For x86_64, when CPU or memory is hot un/plugged, the crash
> elfcorehdr, which describes the CPUs and memory in the system,
> must also be updated.
>
> To update the elfcorehdr for x86_64, a new elfcorehdr must be
> generated from the available CPUs and memory. The new elfcorehdr
> is prepared into a buffer, and if no errors occur, it is
> installed over the top of the existing elfcorehdr.
>
> In the patch 'crash hp: kexec_file changes for crash hotplug support'
> the need to update purgatory due to the change in elfcorehdr was
> eliminated. As a result, no changes to purgatory or boot_params
> (as the elfcorehdr= kernel command line parameter pointer
> remains unchanged and correct) are needed, just elfcorehdr.
>
> To accommodate a growing number of resources via hotplug, the
> elfcorehdr segment must be sufficiently large enough to accommodate
> changes, see the CRASH_HOTPLUG_ELFCOREHDR_SZ configure item.
>
> NOTE that this supports both kexec_load and kexec_file_load. Support
> for kexec_load is made possible by identifying the elfcorehdr segment
> at load time and updating it as previously described. However, it is
> the responsibility of the userspace kexec utility to ensure that:
> - the elfcorehdr segment is sufficiently large enough to accommodate
> hotplug changes, ala CRASH_HOTPLUG_ELFCOREHDR_SZ.
> - provides a purgatory that excludes the elfcorehdr from its list of
> run-time segments to check.
> These changes to the userspace kexec utility are not yet available.
>
> Signed-off-by: Eric DeVolder <[email protected]>
> ---
> arch/x86/kernel/crash.c | 138 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 137 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 9730c88530fc..d185137b33d4 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -25,6 +25,7 @@
> #include <linux/slab.h>
> #include <linux/vmalloc.h>
> #include <linux/memblock.h>
> +#include <linux/highmem.h>
>
> #include <asm/processor.h>
> #include <asm/hardirq.h>
> @@ -265,7 +266,8 @@ static int prepare_elf_headers(struct kimage *image, void **addr,
> goto out;
>
> /* By default prepare 64bit headers */
> - ret = crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
> + ret = crash_prepare_elf64_headers(image, cmem,
> + IS_ENABLED(CONFIG_X86_64), addr, sz);
>
> out:
> vfree(cmem);
> @@ -397,7 +399,17 @@ int crash_load_segments(struct kimage *image)
> image->elf_headers = kbuf.buffer;
> image->elf_headers_sz = kbuf.bufsz;
>
> +#ifdef CONFIG_CRASH_HOTPLUG
> + /* Ensure elfcorehdr segment large enough for hotplug changes */
> + kbuf.memsz = CONFIG_CRASH_HOTPLUG_ELFCOREHDR_SZ;
I would define a default value for the size, meantime provide a Kconfig
option to allow user to customize.
> + /* For marking as usable to crash kernel */
> + image->elf_headers_sz = kbuf.memsz;
> + /* Record the index of the elfcorehdr segment */
> + image->elf_index = image->nr_segments;
> + image->elf_index_valid = true;
> +#else
> kbuf.memsz = kbuf.bufsz;
> +#endif
> kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> ret = kexec_add_buffer(&kbuf);
> @@ -412,3 +424,127 @@ int crash_load_segments(struct kimage *image)
> return ret;
> }
> #endif /* CONFIG_KEXEC_FILE */
> +
> +#ifdef CONFIG_CRASH_HOTPLUG
These two helper function should be carved out into a separate patch as
a preparatory one. I am considering how to rearrange and split the
patches, will reply to cover letter.
> +void *map_crash_pages(unsigned long paddr, unsigned long size)
> +{
> + /*
> + * NOTE: The addresses and sizes passed to this routine have
> + * already been fully aligned on page boundaries. There is no
> + * need for massaging the address or size.
> + */
> + void *ptr = NULL;
> +
> + /* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
> + if (size > 0) {
> + struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
> +
> + ptr = kmap(page);
> + }
> +
> + return ptr;
> +}
> +
> +void unmap_crash_pages(void **ptr)
> +{
> + if (ptr) {
> + if (*ptr)
> + kunmap(*ptr);
> + *ptr = NULL;
> + }
> +}
> +
> +void arch_crash_hotplug_handler(struct kimage *image,
> + unsigned int hp_action, unsigned long a, unsigned long b)
> +{
> + /*
> + * To accurately reflect hot un/plug changes, the elfcorehdr (which
> + * is passed to the crash kernel via the elfcorehdr= parameter)
> + * must be updated with the new list of CPUs and memories. The new
> + * elfcorehdr is prepared in a kernel buffer, and if no errors,
> + * then it is written on top of the existing/old elfcorehdr.
> + *
> + * Due to the change to the elfcorehdr, purgatory must explicitly
> + * exclude the elfcorehdr from the list of segments it checks.
> + */
Please move this code comment to above function as kernel-doc if you
this it benefits the entire function. Otherwise should move them above
the code block they are explaining. For this place, I think moving them
to above arch_crash_hotplug_handler() is better.
> + struct kexec_segment *ksegment;
> + unsigned char *ptr = NULL;
> + unsigned long elfsz = 0;
> + void *elfbuf = NULL;
> + unsigned long mem, memsz;
> + unsigned int n;
> +
> + /*
> + * When the struct kimage is alloced, it is wiped to zero, so
> + * the elf_index_valid defaults to false. It is set on the
> + * kexec_file_load path, or here for kexec_load.
> + */
I think this kexec loading part should be taken out and post after this
whole patchset being accepted. At least, it's worth to put them in a
separate patch.
> + if (!image->elf_index_valid) {
> + for (n = 0; n < image->nr_segments; n++) {
> + mem = image->segment[n].mem;
> + memsz = image->segment[n].memsz;
> + ptr = map_crash_pages(mem, memsz);
> + if (ptr) {
> + /* The segment containing elfcorehdr */
> + if ((ptr[0] == 0x7F) &&
> + (ptr[1] == 'E') &&
> + (ptr[2] == 'L') &&
> + (ptr[3] == 'F')) {
Is this for safety checking later?
> + image->elf_index = (int)n;
> + image->elf_index_valid = true;
> + }
> + }
> + unmap_crash_pages((void **)&ptr);
> + }
> + }
> +
> + /* Must have valid elfcorehdr index */
> + if (!image->elf_index_valid) {
> + pr_err("crash hp: unable to locate elfcorehdr segment");
> + goto out;
> + }
> +
> + /*
> + * Create the new elfcorehdr reflecting the changes to CPU and/or
> + * memory resources. The elfcorehdr segment memsz must be
> + * sufficiently large to accommodate increases due to hotplug
> + * activity. See CRASH_HOTPLUG_ELFCOREHDR_SZ.
> + */
> + if (prepare_elf_headers(image, &elfbuf, &elfsz)) {
> + pr_err("crash hp: unable to prepare elfcore headers");
> + goto out;
> + }
> + ksegment = &image->segment[image->elf_index];
> + memsz = ksegment->memsz;
> + if (elfsz > memsz) {
> + pr_err("crash hp: update elfcorehdr elfsz %lu > memsz %lu",
> + elfsz, memsz);
> + goto out;
> + }
> +
> + /*
> + * At this point, we are all but assured of success.
> + * Copy new elfcorehdr into destination.
> + */
> + ksegment = &image->segment[image->elf_index];
> + mem = ksegment->mem;
> + memsz = ksegment->memsz;
The ksegment and memsz have repeated assignment.
> + ptr = map_crash_pages(mem, memsz);
> + if (ptr) {
> + /* Temporarily invalidate the crash image while it is replaced */
> + xchg(&kexec_crash_image, NULL);
> + /* Write the new elfcorehdr into memory */
> + memcpy((void *)ptr, elfbuf, elfsz);
> + /* The crash image is now valid once again */
> + xchg(&kexec_crash_image, image);
> + }
> + unmap_crash_pages((void **)&ptr);
> + pr_debug("crash hp: re-loaded elfcorehdr at 0x%lx\n", mem);
> +
> +//FIX??? somekind of cache flush perhaps?
You might mean memcpy_flushcache() on x86.
> +
> +out:
> + if (elfbuf)
> + vfree(elfbuf);
> +}
> +#endif /* CONFIG_CRASH_HOTPLUG */
> --
> 2.27.0
>
Baoquan,
Thanks for the feedback, inline responses below!
eric
On 1/19/22 04:23, Baoquan He wrote:
> On 01/10/22 at 02:57pm, Eric DeVolder wrote:
>> For x86_64, when CPU or memory is hot un/plugged, the crash
>> elfcorehdr, which describes the CPUs and memory in the system,
>> must also be updated.
>>
>> To update the elfcorehdr for x86_64, a new elfcorehdr must be
>> generated from the available CPUs and memory. The new elfcorehdr
>> is prepared into a buffer, and if no errors occur, it is
>> installed over the top of the existing elfcorehdr.
>>
>> In the patch 'crash hp: kexec_file changes for crash hotplug support'
>> the need to update purgatory due to the change in elfcorehdr was
>> eliminated. As a result, no changes to purgatory or boot_params
>> (as the elfcorehdr= kernel command line parameter pointer
>> remains unchanged and correct) are needed, just elfcorehdr.
>>
>> To accommodate a growing number of resources via hotplug, the
>> elfcorehdr segment must be sufficiently large enough to accommodate
>> changes, see the CRASH_HOTPLUG_ELFCOREHDR_SZ configure item.
>>
>> NOTE that this supports both kexec_load and kexec_file_load. Support
>> for kexec_load is made possible by identifying the elfcorehdr segment
>> at load time and updating it as previously described. However, it is
>> the responsibility of the userspace kexec utility to ensure that:
>> - the elfcorehdr segment is sufficiently large enough to accommodate
>> hotplug changes, ala CRASH_HOTPLUG_ELFCOREHDR_SZ.
>> - provides a purgatory that excludes the elfcorehdr from its list of
>> run-time segments to check.
>> These changes to the userspace kexec utility are not yet available.
>>
>> Signed-off-by: Eric DeVolder <[email protected]>
>> ---
>> arch/x86/kernel/crash.c | 138 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 137 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>> index 9730c88530fc..d185137b33d4 100644
>> --- a/arch/x86/kernel/crash.c
>> +++ b/arch/x86/kernel/crash.c
>> @@ -25,6 +25,7 @@
>> #include <linux/slab.h>
>> #include <linux/vmalloc.h>
>> #include <linux/memblock.h>
>> +#include <linux/highmem.h>
>>
>> #include <asm/processor.h>
>> #include <asm/hardirq.h>
>> @@ -265,7 +266,8 @@ static int prepare_elf_headers(struct kimage *image, void **addr,
>> goto out;
>>
>> /* By default prepare 64bit headers */
>> - ret = crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
>> + ret = crash_prepare_elf64_headers(image, cmem,
>> + IS_ENABLED(CONFIG_X86_64), addr, sz);
>>
>> out:
>> vfree(cmem);
>> @@ -397,7 +399,17 @@ int crash_load_segments(struct kimage *image)
>> image->elf_headers = kbuf.buffer;
>> image->elf_headers_sz = kbuf.bufsz;
>>
>> +#ifdef CONFIG_CRASH_HOTPLUG
>> + /* Ensure elfcorehdr segment large enough for hotplug changes */
>> + kbuf.memsz = CONFIG_CRASH_HOTPLUG_ELFCOREHDR_SZ;
>
> I would define a default value for the size, meantime provide a Kconfig
> option to allow user to customize.
In patch 2/6 of this series, "crash hp: Introduce CRASH_HOTPLUG configuration options", I provide
the following:
+config CRASH_HOTPLUG_ELFCOREHDR_SZ
+ depends on CRASH_HOTPLUG
+ int
+ default 131072
+ help
+ Specify the maximum size of the elfcorehdr buffer/segment.
which defines a default value of 128KiB, and can be overriden at configure time.
Are you asking for a different technique?
>
>> + /* For marking as usable to crash kernel */
>> + image->elf_headers_sz = kbuf.memsz;
>> + /* Record the index of the elfcorehdr segment */
>> + image->elf_index = image->nr_segments;
>> + image->elf_index_valid = true;
>> +#else
>> kbuf.memsz = kbuf.bufsz;
>> +#endif
>> kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> ret = kexec_add_buffer(&kbuf);
>> @@ -412,3 +424,127 @@ int crash_load_segments(struct kimage *image)
>> return ret;
>> }
>> #endif /* CONFIG_KEXEC_FILE */
>> +
>> +#ifdef CONFIG_CRASH_HOTPLUG
>
> These two helper function should be carved out into a separate patch as
> a preparatory one. I am considering how to rearrange and split the
> patches, will reply to cover letter.
OK, I look forward to that insight!
>
>> +void *map_crash_pages(unsigned long paddr, unsigned long size)
>> +{
>> + /*
>> + * NOTE: The addresses and sizes passed to this routine have
>> + * already been fully aligned on page boundaries. There is no
>> + * need for massaging the address or size.
>> + */
>> + void *ptr = NULL;
>> +
>> + /* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
>> + if (size > 0) {
>> + struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
>> +
>> + ptr = kmap(page);
>> + }
>> +
>> + return ptr;
>> +}
>> +
>> +void unmap_crash_pages(void **ptr)
>> +{
>> + if (ptr) {
>> + if (*ptr)
>> + kunmap(*ptr);
>> + *ptr = NULL;
>> + }
>> +}
>> +
>> +void arch_crash_hotplug_handler(struct kimage *image,
>> + unsigned int hp_action, unsigned long a, unsigned long b)
>> +{
>> + /*
>> + * To accurately reflect hot un/plug changes, the elfcorehdr (which
>> + * is passed to the crash kernel via the elfcorehdr= parameter)
>> + * must be updated with the new list of CPUs and memories. The new
>> + * elfcorehdr is prepared in a kernel buffer, and if no errors,
>> + * then it is written on top of the existing/old elfcorehdr.
>> + *
>> + * Due to the change to the elfcorehdr, purgatory must explicitly
>> + * exclude the elfcorehdr from the list of segments it checks.
>> + */
>
> Please move this code comment to above function as kernel-doc if you
> this it benefits the entire function. Otherwise should move them above
> the code block they are explaining. For this place, I think moving them
> to above arch_crash_hotplug_handler() is better.
ok, I will do that!
>
>> + struct kexec_segment *ksegment;
>> + unsigned char *ptr = NULL;
>> + unsigned long elfsz = 0;
>> + void *elfbuf = NULL;
>> + unsigned long mem, memsz;
>> + unsigned int n;
>> +
>> + /*
>> + * When the struct kimage is alloced, it is wiped to zero, so
>> + * the elf_index_valid defaults to false. It is set on the
>> + * kexec_file_load path, or here for kexec_load.
>> + */
>
> I think this kexec loading part should be taken out and post after this
> whole patchset being accepted. At least, it's worth to put them in a
> separate patch.
This little bit of code that identifies the incoming elfcorehdr is all that is needed to support
kexec_load (and the userspace changes of course). I'm happy to split as a separate patch, but I
would think that be maintaining it with this series, then when it is accepted, both the kexec_load
and kexec_file_load paths would be supported? Your call.
>
>> + if (!image->elf_index_valid) {
>> + for (n = 0; n < image->nr_segments; n++) {
>> + mem = image->segment[n].mem;
>> + memsz = image->segment[n].memsz;
>> + ptr = map_crash_pages(mem, memsz);
>> + if (ptr) {
>> + /* The segment containing elfcorehdr */
>> + if ((ptr[0] == 0x7F) &&
>> + (ptr[1] == 'E') &&
>> + (ptr[2] == 'L') &&
>> + (ptr[3] == 'F')) {
>
> Is this for safety checking later?
No, this code is here to support the kexec_load path; the incoming elfcorehdr has to be identified
in order to make changes to it later.
>
>> + image->elf_index = (int)n;
>> + image->elf_index_valid = true;
>> + }
>> + }
>> + unmap_crash_pages((void **)&ptr);
>> + }
>> + }
>> +
>> + /* Must have valid elfcorehdr index */
>> + if (!image->elf_index_valid) {
>> + pr_err("crash hp: unable to locate elfcorehdr segment");
>> + goto out;
>> + }
>> +
>> + /*
>> + * Create the new elfcorehdr reflecting the changes to CPU and/or
>> + * memory resources. The elfcorehdr segment memsz must be
>> + * sufficiently large to accommodate increases due to hotplug
>> + * activity. See CRASH_HOTPLUG_ELFCOREHDR_SZ.
>> + */
>> + if (prepare_elf_headers(image, &elfbuf, &elfsz)) {
>> + pr_err("crash hp: unable to prepare elfcore headers");
>> + goto out;
>> + }
>> + ksegment = &image->segment[image->elf_index];
>> + memsz = ksegment->memsz;
>> + if (elfsz > memsz) {
>> + pr_err("crash hp: update elfcorehdr elfsz %lu > memsz %lu",
>> + elfsz, memsz);
>> + goto out;
>> + }
>> +
>> + /*
>> + * At this point, we are all but assured of success.
>> + * Copy new elfcorehdr into destination.
>> + */
>> + ksegment = &image->segment[image->elf_index];
>> + mem = ksegment->mem;
>> + memsz = ksegment->memsz;
>
> The ksegment and memsz have repeated assignment.
Ah, good catch! I will correct.
>
>> + ptr = map_crash_pages(mem, memsz);
>> + if (ptr) {
>> + /* Temporarily invalidate the crash image while it is replaced */
>> + xchg(&kexec_crash_image, NULL);
>> + /* Write the new elfcorehdr into memory */
>> + memcpy((void *)ptr, elfbuf, elfsz);
>> + /* The crash image is now valid once again */
>> + xchg(&kexec_crash_image, image);
>> + }
>> + unmap_crash_pages((void **)&ptr);
>> + pr_debug("crash hp: re-loaded elfcorehdr at 0x%lx\n", mem);
>> +
>> +//FIX??? somekind of cache flush perhaps?
>
> You might mean memcpy_flushcache() on x86.
Thanks, I'll look into this and hopefully use it above.
>
>
>> +
>> +out:
>> + if (elfbuf)
>> + vfree(elfbuf);
>> +}
>> +#endif /* CONFIG_CRASH_HOTPLUG */
>> --
>> 2.27.0
>>
>
On 01/21/22 at 08:06am, Eric DeVolder wrote:
......
> > > arch/x86/kernel/crash.c | 138 +++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 137 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > > index 9730c88530fc..d185137b33d4 100644
> > > --- a/arch/x86/kernel/crash.c
> > > +++ b/arch/x86/kernel/crash.c
> > > @@ -25,6 +25,7 @@
> > > #include <linux/slab.h>
> > > #include <linux/vmalloc.h>
> > > #include <linux/memblock.h>
> > > +#include <linux/highmem.h>
> > > #include <asm/processor.h>
> > > #include <asm/hardirq.h>
> > > @@ -265,7 +266,8 @@ static int prepare_elf_headers(struct kimage *image, void **addr,
> > > goto out;
> > > /* By default prepare 64bit headers */
> > > - ret = crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
> > > + ret = crash_prepare_elf64_headers(image, cmem,
> > > + IS_ENABLED(CONFIG_X86_64), addr, sz);
> > > out:
> > > vfree(cmem);
> > > @@ -397,7 +399,17 @@ int crash_load_segments(struct kimage *image)
> > > image->elf_headers = kbuf.buffer;
> > > image->elf_headers_sz = kbuf.bufsz;
> > > +#ifdef CONFIG_CRASH_HOTPLUG
> > > + /* Ensure elfcorehdr segment large enough for hotplug changes */
> > > + kbuf.memsz = CONFIG_CRASH_HOTPLUG_ELFCOREHDR_SZ;
> >
> > I would define a default value for the size, meantime provide a Kconfig
> > option to allow user to customize.
>
> In patch 2/6 of this series, "crash hp: Introduce CRASH_HOTPLUG
> configuration options", I provide the following:
>
> +config CRASH_HOTPLUG_ELFCOREHDR_SZ
> + depends on CRASH_HOTPLUG
> + int
> + default 131072
> + help
> + Specify the maximum size of the elfcorehdr buffer/segment.
>
> which defines a default value of 128KiB, and can be overriden at configure time.
>
> Are you asking for a different technique?
I thought to define a global variable, like
/* Defaults to ahve 128K elfcorehdr buffer which contains 2048 entries.*/
unsigned long crash_hotplug_elfcorehdr_size = 0x20000;
Then initialize it in crash_hotplug_init() if CONFIG_CRASH_HOTPLUG_ELFCOREHDR_SZ
is enabled.
Seems using the config directly is also OK. Let's keep it and see if
other people have comment.
>
> >
> > > + /* For marking as usable to crash kernel */
> > > + image->elf_headers_sz = kbuf.memsz;
> > > + /* Record the index of the elfcorehdr segment */
> > > + image->elf_index = image->nr_segments;
> > > + image->elf_index_valid = true;
> > > +#else
> > > kbuf.memsz = kbuf.bufsz;
> > > +#endif
> > > kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> > > kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> > > ret = kexec_add_buffer(&kbuf);
> > > @@ -412,3 +424,127 @@ int crash_load_segments(struct kimage *image)
> > > return ret;
> > > }
> > > #endif /* CONFIG_KEXEC_FILE */
> > > +
> > > +#ifdef CONFIG_CRASH_HOTPLUG
> >
> > These two helper function should be carved out into a separate patch as
> > a preparatory one. I am considering how to rearrange and split the
> > patches, will reply to cover letter.
>
> OK, I look forward to that insight!
>
> >
> > > +void *map_crash_pages(unsigned long paddr, unsigned long size)
> > > +{
> > > + /*
> > > + * NOTE: The addresses and sizes passed to this routine have
> > > + * already been fully aligned on page boundaries. There is no
> > > + * need for massaging the address or size.
> > > + */
> > > + void *ptr = NULL;
> > > +
> > > + /* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
> > > + if (size > 0) {
> > > + struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
> > > +
> > > + ptr = kmap(page);
> > > + }
> > > +
> > > + return ptr;
> > > +}
> > > +
> > > +void unmap_crash_pages(void **ptr)
> > > +{
> > > + if (ptr) {
> > > + if (*ptr)
> > > + kunmap(*ptr);
> > > + *ptr = NULL;
> > > + }
> > > +}
> > > +
> > > +void arch_crash_hotplug_handler(struct kimage *image,
> > > + unsigned int hp_action, unsigned long a, unsigned long b)
> > > +{
> > > + /*
> > > + * To accurately reflect hot un/plug changes, the elfcorehdr (which
> > > + * is passed to the crash kernel via the elfcorehdr= parameter)
> > > + * must be updated with the new list of CPUs and memories. The new
> > > + * elfcorehdr is prepared in a kernel buffer, and if no errors,
> > > + * then it is written on top of the existing/old elfcorehdr.
> > > + *
> > > + * Due to the change to the elfcorehdr, purgatory must explicitly
> > > + * exclude the elfcorehdr from the list of segments it checks.
> > > + */
> >
> > Please move this code comment to above function as kernel-doc if you
> > this it benefits the entire function. Otherwise should move them above
> > the code block they are explaining. For this place, I think moving them
> > to above arch_crash_hotplug_handler() is better.
>
> ok, I will do that!
>
> >
> > > + struct kexec_segment *ksegment;
> > > + unsigned char *ptr = NULL;
> > > + unsigned long elfsz = 0;
> > > + void *elfbuf = NULL;
> > > + unsigned long mem, memsz;
> > > + unsigned int n;
> > > +
> > > + /*
> > > + * When the struct kimage is alloced, it is wiped to zero, so
> > > + * the elf_index_valid defaults to false. It is set on the
> > > + * kexec_file_load path, or here for kexec_load.
> > > + */
> >
> > I think this kexec loading part should be taken out and post after this
> > whole patchset being accepted. At least, it's worth to put them in a
> > separate patch.
>
> This little bit of code that identifies the incoming elfcorehdr is all that
> is needed to support kexec_load (and the userspace changes of course). I'm
> happy to split as a separate patch, but I would think that be maintaining it
> with this series, then when it is accepted, both the kexec_load and
> kexec_file_load paths would be supported? Your call.
Hmm, at first, let's split it out from this patch since it's an
independent action to kdump. I would suggest we don't carry it in this
series. After this series is done, you can post another patchset
including this part as kernel patch, and also the code change in
kexec_tools as user space patch.
......
Baoquan,
Thanks for looking at this! See inline response below.
eric
On 1/26/22 03:12, Baoquan He wrote:
> On 01/21/22 at 08:06am, Eric DeVolder wrote:
> ......
>>>> arch/x86/kernel/crash.c | 138 +++++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 137 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>>>> index 9730c88530fc..d185137b33d4 100644
>>>> --- a/arch/x86/kernel/crash.c
>>>> +++ b/arch/x86/kernel/crash.c
>>>> @@ -25,6 +25,7 @@
>>>> #include <linux/slab.h>
>>>> #include <linux/vmalloc.h>
>>>> #include <linux/memblock.h>
>>>> +#include <linux/highmem.h>
>>>> #include <asm/processor.h>
>>>> #include <asm/hardirq.h>
>>>> @@ -265,7 +266,8 @@ static int prepare_elf_headers(struct kimage *image, void **addr,
>>>> goto out;
>>>> /* By default prepare 64bit headers */
>>>> - ret = crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
>>>> + ret = crash_prepare_elf64_headers(image, cmem,
>>>> + IS_ENABLED(CONFIG_X86_64), addr, sz);
>>>> out:
>>>> vfree(cmem);
>>>> @@ -397,7 +399,17 @@ int crash_load_segments(struct kimage *image)
>>>> image->elf_headers = kbuf.buffer;
>>>> image->elf_headers_sz = kbuf.bufsz;
>>>> +#ifdef CONFIG_CRASH_HOTPLUG
>>>> + /* Ensure elfcorehdr segment large enough for hotplug changes */
>>>> + kbuf.memsz = CONFIG_CRASH_HOTPLUG_ELFCOREHDR_SZ;
>>>
>>> I would define a default value for the size, meantime provide a Kconfig
>>> option to allow user to customize.
>>
>> In patch 2/6 of this series, "crash hp: Introduce CRASH_HOTPLUG
>> configuration options", I provide the following:
>>
>> +config CRASH_HOTPLUG_ELFCOREHDR_SZ
>> + depends on CRASH_HOTPLUG
>> + int
>> + default 131072
>> + help
>> + Specify the maximum size of the elfcorehdr buffer/segment.
>>
>> which defines a default value of 128KiB, and can be overriden at configure time.
>>
>> Are you asking for a different technique?
>
> I thought to define a global variable, like
>
> /* Defaults to ahve 128K elfcorehdr buffer which contains 2048 entries.*/
> unsigned long crash_hotplug_elfcorehdr_size = 0x20000;
>
> Then initialize it in crash_hotplug_init() if CONFIG_CRASH_HOTPLUG_ELFCOREHDR_SZ
> is enabled.
>
> Seems using the config directly is also OK. Let's keep it and see if
> other people have comment.
OK, I will leave alone for the time being.
>
>>
>>>
>>>> + /* For marking as usable to crash kernel */
>>>> + image->elf_headers_sz = kbuf.memsz;
>>>> + /* Record the index of the elfcorehdr segment */
>>>> + image->elf_index = image->nr_segments;
>>>> + image->elf_index_valid = true;
>>>> +#else
>>>> kbuf.memsz = kbuf.bufsz;
>>>> +#endif
>>>> kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
>>>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>>>> ret = kexec_add_buffer(&kbuf);
>>>> @@ -412,3 +424,127 @@ int crash_load_segments(struct kimage *image)
>>>> return ret;
>>>> }
>>>> #endif /* CONFIG_KEXEC_FILE */
>>>> +
>>>> +#ifdef CONFIG_CRASH_HOTPLUG
>>>
>>> These two helper function should be carved out into a separate patch as
>>> a preparatory one. I am considering how to rearrange and split the
>>> patches, will reply to cover letter.
>>
>> OK, I look forward to that insight!
>>
>>>
>>>> +void *map_crash_pages(unsigned long paddr, unsigned long size)
>>>> +{
>>>> + /*
>>>> + * NOTE: The addresses and sizes passed to this routine have
>>>> + * already been fully aligned on page boundaries. There is no
>>>> + * need for massaging the address or size.
>>>> + */
>>>> + void *ptr = NULL;
>>>> +
>>>> + /* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
>>>> + if (size > 0) {
>>>> + struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
>>>> +
>>>> + ptr = kmap(page);
>>>> + }
>>>> +
>>>> + return ptr;
>>>> +}
>>>> +
>>>> +void unmap_crash_pages(void **ptr)
>>>> +{
>>>> + if (ptr) {
>>>> + if (*ptr)
>>>> + kunmap(*ptr);
>>>> + *ptr = NULL;
>>>> + }
>>>> +}
>>>> +
>>>> +void arch_crash_hotplug_handler(struct kimage *image,
>>>> + unsigned int hp_action, unsigned long a, unsigned long b)
>>>> +{
>>>> + /*
>>>> + * To accurately reflect hot un/plug changes, the elfcorehdr (which
>>>> + * is passed to the crash kernel via the elfcorehdr= parameter)
>>>> + * must be updated with the new list of CPUs and memories. The new
>>>> + * elfcorehdr is prepared in a kernel buffer, and if no errors,
>>>> + * then it is written on top of the existing/old elfcorehdr.
>>>> + *
>>>> + * Due to the change to the elfcorehdr, purgatory must explicitly
>>>> + * exclude the elfcorehdr from the list of segments it checks.
>>>> + */
>>>
>>> Please move this code comment to above function as kernel-doc if you
>>> this it benefits the entire function. Otherwise should move them above
>>> the code block they are explaining. For this place, I think moving them
>>> to above arch_crash_hotplug_handler() is better.
>>
>> ok, I will do that!
>>
>>>
>>>> + struct kexec_segment *ksegment;
>>>> + unsigned char *ptr = NULL;
>>>> + unsigned long elfsz = 0;
>>>> + void *elfbuf = NULL;
>>>> + unsigned long mem, memsz;
>>>> + unsigned int n;
>>>> +
>>>> + /*
>>>> + * When the struct kimage is alloced, it is wiped to zero, so
>>>> + * the elf_index_valid defaults to false. It is set on the
>>>> + * kexec_file_load path, or here for kexec_load.
>>>> + */
>>>
>>> I think this kexec loading part should be taken out and post after this
>>> whole patchset being accepted. At least, it's worth to put them in a
>>> separate patch.
>>
>> This little bit of code that identifies the incoming elfcorehdr is all that
>> is needed to support kexec_load (and the userspace changes of course). I'm
>> happy to split as a separate patch, but I would think that be maintaining it
>> with this series, then when it is accepted, both the kexec_load and
>> kexec_file_load paths would be supported? Your call.
>
> Hmm, at first, let's split it out from this patch since it's an
> independent action to kdump. I would suggest we don't carry it in this
> series. After this series is done, you can post another patchset
> including this part as kernel patch, and also the code change in
> kexec_tools as user space patch.
>
> ......
>
OK, I'll remove the bit of code that supports kexec_load, so it can be introduced
later coincident with the changes to kexec-tools.
In a previous message you mentioned making changes to the order of the patches,
was this it, or is there more to come?
Thanks!
eric
On 01/26/22 at 11:32am, Eric DeVolder wrote:
..snip....
> > > > > +void arch_crash_hotplug_handler(struct kimage *image,
> > > > > + unsigned int hp_action, unsigned long a, unsigned long b)
> > > > > +{
> > > > > + /*
> > > > > + * To accurately reflect hot un/plug changes, the elfcorehdr (which
> > > > > + * is passed to the crash kernel via the elfcorehdr= parameter)
> > > > > + * must be updated with the new list of CPUs and memories. The new
> > > > > + * elfcorehdr is prepared in a kernel buffer, and if no errors,
> > > > > + * then it is written on top of the existing/old elfcorehdr.
> > > > > + *
> > > > > + * Due to the change to the elfcorehdr, purgatory must explicitly
> > > > > + * exclude the elfcorehdr from the list of segments it checks.
> > > > > + */
> > > >
> > > > Please move this code comment to above function as kernel-doc if you
> > > > this it benefits the entire function. Otherwise should move them above
> > > > the code block they are explaining. For this place, I think moving them
> > > > to above arch_crash_hotplug_handler() is better.
> > >
> > > ok, I will do that!
> > >
> > > >
> > > > > + struct kexec_segment *ksegment;
> > > > > + unsigned char *ptr = NULL;
> > > > > + unsigned long elfsz = 0;
> > > > > + void *elfbuf = NULL;
> > > > > + unsigned long mem, memsz;
> > > > > + unsigned int n;
> > > > > +
> > > > > + /*
> > > > > + * When the struct kimage is alloced, it is wiped to zero, so
> > > > > + * the elf_index_valid defaults to false. It is set on the
> > > > > + * kexec_file_load path, or here for kexec_load.
> > > > > + */
> > > >
> > > > I think this kexec loading part should be taken out and post after this
> > > > whole patchset being accepted. At least, it's worth to put them in a
> > > > separate patch.
> > >
> > > This little bit of code that identifies the incoming elfcorehdr is all that
> > > is needed to support kexec_load (and the userspace changes of course). I'm
> > > happy to split as a separate patch, but I would think that be maintaining it
> > > with this series, then when it is accepted, both the kexec_load and
> > > kexec_file_load paths would be supported? Your call.
> >
> > Hmm, at first, let's split it out from this patch since it's an
> > independent action to kdump. I would suggest we don't carry it in this
> > series. After this series is done, you can post another patchset
> > including this part as kernel patch, and also the code change in
> > kexec_tools as user space patch.
> >
> > ......
> >
>
> OK, I'll remove the bit of code that supports kexec_load, so it can be introduced
> later coincident with the changes to kexec-tools.
>
> In a previous message you mentioned making changes to the order of the patches,
> was this it, or is there more to come?
Yeah, replied to cover letter, please check it there.