There are following issues in arm64 kdump:
1. We use crashkernel=X to reserve crashkernel below 4G, which
will fail when there is no enough low memory.
2. If reserving crashkernel above 4G, in this case, crash dump
kernel will boot failure because there is no low memory available
for allocation.
To solve these issues, change the behavior of crashkernel=X.
crashkernel=X tries low allocation in DMA zone and fall back to high
allocation if it fails.
We can also use "crashkernel=X,high" to select a high region above
DMA zone, which also tries to allocate at least 256M low memory in
DMA zone automatically and "crashkernel=Y,low" can be used to allocate
specified size low memory.
When reserving crashkernel in high memory, some low memory is reserved
for crash dump kernel devices. So there may be two regions reserved for
crash dump kernel.
In order to distinct from the high region and make no effect to the use
of existing kexec-tools, rename the low region as "Crash kernel (low)",
and pass the low region by reusing DT property
"linux,usable-memory-range". We made the low memory region as the last
range of "linux,usable-memory-range" to keep compatibility with existing
user-space and older kdump kernels.
Besides, we need to modify kexec-tools:
arm64: support more than one crash kernel regions(see [1])
Another update is document about DT property 'linux,usable-memory-range':
schemas: update 'linux,usable-memory-range' node schema(see [2])
Changes since [v17]: v17 --> v19
1. Patch 0001-0004
Introduce generic parse_crashkernel_high_low() to bring the parsing of
"crashkernel=X,high" and the parsing of "crashkernel=X,low" together,
then use it instead of the call to parse_crashkernel_{high|low}(). Two
confusing parameters of parse_crashkernel_{high|low}() are deleted.
I previously sent these four patches separately:
[1] https://lkml.org/lkml/2021/12/25/40
2. Patch 0005-0009
Introduce generic reserve_crashkernel_mem[_low](), the implementation of
these two functions is based on function reserve_crashkernel[_low]() in
arch/x86/kernel/setup.c. There is no functional change for x86.
1) The check position of xen_pv_domain() does not change.
2) Still 1M alignment for crash kernel fixed region, when 'base' is specified.
To avoid compilation problems on other architectures: patch 0004 moves
the definition of global variable crashk[_low]_res from kexec_core.c to
crash_core.c, and provide default definitions for all macros involved, a
particular platform can redefine these macros to override the default
values.
3. 0010, only one line of comment was changed.
4. 0011
1) crashk_low_res may also a valid reserved memory, should be checked
in crash_is_nosave(), see arch/arm64/kernel/machine_kexec.
2) Drop memblock_mark_nomap() for crashk_low_res, because of:
2687275a5843 arm64: Force NO_BLOCK_MAPPINGS if crashkernel reservation is required
3) Also call kmemleak_ignore_phys() for crashk_low_res, because of:
85f58eb18898 arm64: kdump: Skip kmemleak scan reserved memory for kdump
5. 0012, slightly rebased, because the following patch is applied in advance.
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/linus&id=8347b41748c3019157312fbe7f8a6792ae396eb7
6. 0013, no change.
Others:
1. Discard add ARCH_WANT_RESERVE_CRASH_KERNEL
2. When allocating crash low memory, the start address still starts from 0.
low_base = memblock_phys_alloc_range(low_size, CRASH_ALIGN, 0, CRASH_ADDR_LOW_MAX);
3. Discard change (1ULL << 32) to CRASH_ADDR_LOW_MAX.
4. Ensure the check position of xen_pv_domain() have no change.
5. Except patch 0010 and 0012, all "Tested-by", "Reviewed-by", "Acked-by" are removed.
6. Update description.
Changes since [v16]
- Because no functional changes in this version, so add
"Tested-by: Dave Kleikamp [off-list ref]" for patch 1-9
- Add "Reviewed-by: Rob Herring [off-list ref]" for patch 8
- Update patch 9 based on the review comments of Rob Herring
- As Catalin Marinas's suggestion, merge the implementation of
ARCH_WANT_RESERVE_CRASH_KERNEL into patch 5. Ensure that the
contents of X86 and ARM64 do not overlap, and reduce unnecessary
temporary differences.
Changes since [v15]
- Aggregate the processing of "linux,usable-memory-range" into one function.
Only patch 9-10 have been updated.
Changes since [v14]
- Recovering the requirement that the CrashKernel memory regions on X86
only requires 1 MiB alignment.
- Combine patches 5 and 6 in v14 into one. The compilation warning fixed
by patch 6 was introduced by patch 5 in v14.
- As with crashk_res, crashk_low_res is also processed by
crash_exclude_mem_range() in patch 7.
- Due to commit b261dba2fdb2 ("arm64: kdump: Remove custom linux,usable-memory-range handling")
has removed the architecture-specific code, extend the property "linux,usable-memory-range"
in the platform-agnostic FDT core code. See patch 9.
- Discard the x86 description update in the document, because the description
has been updated by commit b1f4c363666c ("Documentation: kdump: update kdump guide").
- Change "arm64" to "ARM64" in Doc.
Changes since [v13]
- Rebased on top of 5.11-rc5.
- Introduce config CONFIG_ARCH_WANT_RESERVE_CRASH_KERNEL.
Since reserve_crashkernel[_low]() implementations are quite similar on
other architectures, so have CONFIG_ARCH_WANT_RESERVE_CRASH_KERNEL in
arch/Kconfig and select this by X86 and ARM64.
- Some minor cleanup.
Changes since [v12]
- Rebased on top of 5.10-rc1.
- Keep CRASH_ALIGN as 16M suggested by Dave.
- Drop patch "kdump: add threshold for the required memory".
- Add Tested-by from John.
Changes since [v11]
- Rebased on top of 5.9-rc4.
- Make the function reserve_crashkernel() of x86 generic.
Suggested by Catalin, make the function reserve_crashkernel() of x86 generic
and arm64 use the generic version to reimplement crashkernel=X.
Changes since [v10]
- Reimplement crashkernel=X suggested by Catalin, Many thanks to Catalin.
Changes since [v9]
- Patch 1 add Acked-by from Dave.
- Update patch 5 according to Dave's comments.
- Update chosen schema.
Changes since [v8]
- Reuse DT property "linux,usable-memory-range".
Suggested by Rob, reuse DT property "linux,usable-memory-range" to pass the low
memory region.
- Fix kdump broken with ZONE_DMA reintroduced.
- Update chosen schema.
Changes since [v7]
- Move x86 CRASH_ALIGN to 2M
Suggested by Dave and do some test, move x86 CRASH_ALIGN to 2M.
- Update Documentation/devicetree/bindings/chosen.txt.
Add corresponding documentation to Documentation/devicetree/bindings/chosen.txt
suggested by Arnd.
- Add Tested-by from Jhon and pk.
Changes since [v6]
- Fix build errors reported by kbuild test robot.
Changes since [v5]
- Move reserve_crashkernel_low() into kernel/crash_core.c.
- Delete crashkernel=X,high.
- Modify crashkernel=X,low.
If crashkernel=X,low is specified simultaneously, reserve spcified size low
memory for crash kdump kernel devices firstly and then reserve memory above 4G.
In addition, rename crashk_low_res as "Crash kernel (low)" for arm64, and then
pass to crash dump kernel by DT property "linux,low-memory-range".
- Update Documentation/admin-guide/kdump/kdump.rst.
Changes since [v4]
- Reimplement memblock_cap_memory_ranges for multiple ranges by Mike.
Changes since [v3]
- Add memblock_cap_memory_ranges back for multiple ranges.
- Fix some compiling warnings.
Changes since [v2]
- Split patch "arm64: kdump: support reserving crashkernel above 4G" as
two. Put "move reserve_crashkernel_low() into kexec_core.c" in a separate
patch.
Changes since [v1]:
- Move common reserve_crashkernel_low() code into kernel/kexec_core.c.
- Remove memblock_cap_memory_ranges() i added in v1 and implement that
in fdt_enforce_memory_region().
There are at most two crash kernel regions, for two crash kernel regions
case, we cap the memory range [min(regs[*].start), max(regs[*].end)]
and then remove the memory range in the middle.
[1]: http://lists.infradead.org/pipermail/kexec/2020-June/020737.html
[2]: https://github.com/robherring/dt-schema/pull/19
[v1]: https://lkml.org/lkml/2019/4/2/1174
[v2]: https://lkml.org/lkml/2019/4/9/86
[v3]: https://lkml.org/lkml/2019/4/9/306
[v4]: https://lkml.org/lkml/2019/4/15/273
[v5]: https://lkml.org/lkml/2019/5/6/1360
[v6]: https://lkml.org/lkml/2019/8/30/142
[v7]: https://lkml.org/lkml/2019/12/23/411
[v8]: https://lkml.org/lkml/2020/5/21/213
[v9]: https://lkml.org/lkml/2020/6/28/73
[v10]: https://lkml.org/lkml/2020/7/2/1443
[v11]: https://lkml.org/lkml/2020/8/1/150
[v12]: https://lkml.org/lkml/2020/9/7/1037
[v13]: https://lkml.org/lkml/2020/10/31/34
[v14]: https://lkml.org/lkml/2021/1/30/53
[v15]: https://lkml.org/lkml/2021/10/19/1405
[v16]: https://lkml.org/lkml/2021/11/23/435
[v17]: https://lkml.org/lkml/2021/12/10/38
[v18]: https://lkml.org/lkml/2021/12/22/424
Chen Zhou (6):
kexec: move crashk[_low]_res to crash_core module
x86/setup: Move CRASH[_BASE]_ALIGN and CRASH_ADDR_{LOW|HIGH}_MAX to
asm/kexec.h
arm64: kdump: introduce some macros for crash kernel reservation
arm64: kdump: reimplement crashkernel=X
of: fdt: Add memory for devices by DT property
"linux,usable-memory-range"
kdump: update Documentation about crashkernel
Zhen Lei (7):
kdump: add helper parse_crashkernel_high_low()
x86/setup: Use parse_crashkernel_high_low() to simplify code
kdump: make parse_crashkernel_{high|low}() static
kdump: reduce unnecessary parameters of parse_crashkernel_{high|low}()
x86/setup: Add and use CRASH_BASE_ALIGN
kdump: Add helper reserve_crashkernel_mem[_low]()
x86/setup: Use generic reserve_crashkernel_mem[_low]()
Documentation/admin-guide/kdump/kdump.rst | 11 +-
.../admin-guide/kernel-parameters.txt | 11 +-
arch/arm64/include/asm/kexec.h | 6 +
arch/arm64/kernel/machine_kexec.c | 5 +-
arch/arm64/kernel/machine_kexec_file.c | 12 +-
arch/arm64/kernel/setup.c | 13 +-
arch/arm64/mm/init.c | 41 ++--
arch/x86/include/asm/kexec.h | 27 +++
arch/x86/kernel/setup.c | 128 +---------
drivers/of/fdt.c | 33 ++-
include/linux/crash_core.h | 17 +-
include/linux/kexec.h | 4 -
kernel/crash_core.c | 224 +++++++++++++++++-
kernel/kexec_core.c | 17 --
14 files changed, 350 insertions(+), 199 deletions(-)
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Use parse_crashkernel_high_low() to bring the parsing of
"crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
are strongly dependent, make code logic clear and more readable.
Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Zhen Lei <redacted>
---
arch/x86/kernel/setup.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
Use parse_crashkernel_high_low() to bring the parsing of
"crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
are strongly dependent, make code logic clear and more readable.
Suggested-by: Borislav Petkov <bp@alien8.de>
Yeah, doesn't look like something I suggested...
quoted hunk
@@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) /* crashkernel=XM */ ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); if (ret != 0 || crash_size <= 0) {- /* crashkernel=X,high */- ret = parse_crashkernel_high(boot_command_line, total_mem,- &crash_size, &crash_base);- if (ret != 0 || crash_size <= 0)+ /* crashkernel=X,high and possible crashkernel=Y,low */+ ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
So this calls parse_crashkernel() and when that one fails, it calls this
new weird parse high/low helper you added.
But then all three end up in the same __parse_crashkernel() worker
function which seems to do the actual parsing.
What I suggested and what would be real clean is if the arches would
simply call a *single*
parse_crashkernel()
function and when that one returns, *all* crashkernel= options would
have been parsed properly, low, high, middle crashkernel, whatever...
and the caller would know what crash kernel needs to be allocated.
Then each arch can do its memory allocations and checks based on that
parsed data and decide to allocate or bail.
So it is getting there but it needs more surgery...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
quoted
Use parse_crashkernel_high_low() to bring the parsing of
"crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
are strongly dependent, make code logic clear and more readable.
Suggested-by: Borislav Petkov <bp@alien8.de>
Yeah, doesn't look like something I suggested...
quoted
@@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) /* crashkernel=XM */ ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); if (ret != 0 || crash_size <= 0) {- /* crashkernel=X,high */- ret = parse_crashkernel_high(boot_command_line, total_mem,- &crash_size, &crash_base);- if (ret != 0 || crash_size <= 0)+ /* crashkernel=X,high and possible crashkernel=Y,low */+ ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
So this calls parse_crashkernel() and when that one fails, it calls this
new weird parse high/low helper you added.
But then all three end up in the same __parse_crashkernel() worker
function which seems to do the actual parsing.
What I suggested and what would be real clean is if the arches would
simply call a *single*
parse_crashkernel()
function and when that one returns, *all* crashkernel= options would
have been parsed properly, low, high, middle crashkernel, whatever...
and the caller would know what crash kernel needs to be allocated.
Then each arch can do its memory allocations and checks based on that
parsed data and decide to allocate or bail.
However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
will also support it. It is not supported on other architectures. So changing parse_crashkernel()
is not appropriate unless a new function is introduced. But naming this new function isn't easy,
and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
Of course, we can also consider changing parse_crashkernel() to another name, then use
parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
other architectures to change as well.
So it is getting there but it needs more surgery...
Thx.
From: Dave Young <hidden> Date: 2021-12-29 07:28:23
On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote:
On 2021/12/29 0:13, Borislav Petkov wrote:
quoted
On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
quoted
Use parse_crashkernel_high_low() to bring the parsing of
"crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
are strongly dependent, make code logic clear and more readable.
Suggested-by: Borislav Petkov <bp@alien8.de>
Yeah, doesn't look like something I suggested...
quoted
@@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) /* crashkernel=XM */ ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); if (ret != 0 || crash_size <= 0) {- /* crashkernel=X,high */- ret = parse_crashkernel_high(boot_command_line, total_mem,- &crash_size, &crash_base);- if (ret != 0 || crash_size <= 0)+ /* crashkernel=X,high and possible crashkernel=Y,low */+ ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
So this calls parse_crashkernel() and when that one fails, it calls this
new weird parse high/low helper you added.
But then all three end up in the same __parse_crashkernel() worker
function which seems to do the actual parsing.
What I suggested and what would be real clean is if the arches would
simply call a *single*
parse_crashkernel()
function and when that one returns, *all* crashkernel= options would
have been parsed properly, low, high, middle crashkernel, whatever...
and the caller would know what crash kernel needs to be allocated.
Then each arch can do its memory allocations and checks based on that
parsed data and decide to allocate or bail.
However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
will also support it. It is not supported on other architectures. So changing parse_crashkernel()
is not appropriate unless a new function is introduced. But naming this new function isn't easy,
and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
Of course, we can also consider changing parse_crashkernel() to another name, then use
parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
other architectures to change as well.
Hi, I did not follow up all discussions, but if the only difference is
about the low -> high fallback, I think you can add another argument in
parse_crashkernel(..., *fallback_high), and doing some changes in
__parse_crashkernel() before it returns. But since there are two
many arguments, you could need a wrapper struct for crashkernel_param if
needed.
If you do not want to touch other arches, another function maybe
something like parse_crashkernel_fallback() for x86 and arm64 to use.
But I may not get all the context, please ignore if this is not the
case. I agree that calling parse_crash_kernel* in the
reserve_crashkernel funtions looks not good though.
OTOH there are bunch of other logics in param parsing code,
eg. determin the final size and offset etc. To split the logic out more
things need to be done, eg. firstly parsing function just get all the
inputs from cmdline string eg. an array of struct crashkernel_param with
mem_range, expected size, expected offset, high, or low, and do not do
any other things. Then pass these parsed inputs to another function to
determine the final crash_size and crash_base, that part can be arch
specific somehow.
So I think you can unify the parse_crashkernel* in x86 first with just
one function. And leave the further improvements to later work. But
let's see how Boris think about this.
quoted
So it is getting there but it needs more surgery...
Thx.
From: Dave Young <hidden> Date: 2021-12-29 07:45:40
On 12/29/21 at 03:27pm, Dave Young wrote:
On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote:
quoted
On 2021/12/29 0:13, Borislav Petkov wrote:
quoted
On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
quoted
Use parse_crashkernel_high_low() to bring the parsing of
"crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
are strongly dependent, make code logic clear and more readable.
Suggested-by: Borislav Petkov <bp@alien8.de>
Yeah, doesn't look like something I suggested...
quoted
@@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) /* crashkernel=XM */ ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); if (ret != 0 || crash_size <= 0) {- /* crashkernel=X,high */- ret = parse_crashkernel_high(boot_command_line, total_mem,- &crash_size, &crash_base);- if (ret != 0 || crash_size <= 0)+ /* crashkernel=X,high and possible crashkernel=Y,low */+ ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
So this calls parse_crashkernel() and when that one fails, it calls this
new weird parse high/low helper you added.
But then all three end up in the same __parse_crashkernel() worker
function which seems to do the actual parsing.
What I suggested and what would be real clean is if the arches would
simply call a *single*
parse_crashkernel()
function and when that one returns, *all* crashkernel= options would
have been parsed properly, low, high, middle crashkernel, whatever...
and the caller would know what crash kernel needs to be allocated.
Then each arch can do its memory allocations and checks based on that
parsed data and decide to allocate or bail.
However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
will also support it. It is not supported on other architectures. So changing parse_crashkernel()
is not appropriate unless a new function is introduced. But naming this new function isn't easy,
and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
Of course, we can also consider changing parse_crashkernel() to another name, then use
parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
other architectures to change as well.
Hi, I did not follow up all discussions, but if the only difference is
about the low -> high fallback, I think you can add another argument in
parse_crashkernel(..., *fallback_high), and doing some changes in
__parse_crashkernel() before it returns. But since there are two
many arguments, you could need a wrapper struct for crashkernel_param if
needed.
If you do not want to touch other arches, another function maybe
something like parse_crashkernel_fallback() for x86 and arm64 to use.
But I may not get all the context, please ignore if this is not the
case. I agree that calling parse_crash_kernel* in the
reserve_crashkernel funtions looks not good though.
OTOH there are bunch of other logics in param parsing code,
eg. determin the final size and offset etc. To split the logic out more
things need to be done, eg. firstly parsing function just get all the
inputs from cmdline string eg. an array of struct crashkernel_param with
mem_range, expected size, expected offset, high, or low, and do not do
any other things. Then pass these parsed inputs to another function to
determine the final crash_size and crash_base, that part can be arch
specific somehow.
So I think you can unify the parse_crashkernel* in x86 first with just
one function. And leave the further improvements to later work. But
let's see how Boris think about this.
BTW, I would suggest to wait for reviewers to response (eg. one week at
least, or more due to the holidays) before updating another version
Do not worry to miss the 5.17. I would say take it easy if it will
miss then let's just leave with it and continue to work on the future
improvements. I think one reason this issue takes too long time is that it was
discussed some time but no followup and later people need to warm up
again. Just keep it warm and continue to engage in the improvements, do
not hurry for the specific mainline release.
Thanks
Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Dec 29, 2021 at 03:45:12PM +0800, Dave Young wrote:
BTW, I would suggest to wait for reviewers to response (eg. one week at
least, or more due to the holidays) before updating another version
Do not worry to miss the 5.17. I would say take it easy if it will
miss then let's just leave with it and continue to work on the future
improvements. I think one reason this issue takes too long time is that it was
discussed some time but no followup and later people need to warm up
again. Just keep it warm and continue to engage in the improvements, do
not hurry for the specific mainline release.
Can you tell this to *all* patch submitters please?
I can't count the times where people simply hurry to send the new
revision just to get it in the next kernel, and make silly mistakes
while doing so. Or not think things straight and misdesign it all.
And what this causes is the opposite of what they wanna achieve - pissed
maintainers and ignored threads.
And they all *know* that the next kernel is around the corner. So why
the hell does it even matter when?
What most submitters fail to realize is, the moment your code hits
upstream, it becomes the maintainers' problem and submitters can relax.
But maintainers get to deal with this code forever. So after a while
maintainers learn that they either accept ready code and it all just
works or they make the mistake to take half-baked crap in and then they
themselves get to clean it up and fix it.
So maintainers learn quickly to push back.
But it is annoying and it would help immensely if submitters would
consider this and stop hurrying the code in but try to do a *good* job
first, design-wise and code-wise by thinking hard about what they're
trying to do.
Yeah, things could be a lot simpler and easier - it only takes a little
bit of effort...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Dave Young <hidden> Date: 2021-12-29 10:39:10
On 12/29/21 at 11:11am, Borislav Petkov wrote:
On Wed, Dec 29, 2021 at 03:45:12PM +0800, Dave Young wrote:
quoted
BTW, I would suggest to wait for reviewers to response (eg. one week at
least, or more due to the holidays) before updating another version
Do not worry to miss the 5.17. I would say take it easy if it will
miss then let's just leave with it and continue to work on the future
improvements. I think one reason this issue takes too long time is that it was
discussed some time but no followup and later people need to warm up
again. Just keep it warm and continue to engage in the improvements, do
not hurry for the specific mainline release.
Can you tell this to *all* patch submitters please?
I appreciate you further explanation below to describe the situation. I do not
see how can I tell this to *all* submitters, but I am and I will try to do this
as far as I can. Maintainers and patch submitters, it would help for both
parties show sympathy with each other, some soft reminders will help
people to understand each other, especially for new comers.
I can't count the times where people simply hurry to send the new
revision just to get it in the next kernel, and make silly mistakes
while doing so. Or not think things straight and misdesign it all.
And what this causes is the opposite of what they wanna achieve - pissed
maintainers and ignored threads.
And they all *know* that the next kernel is around the corner. So why
the hell does it even matter when?
What most submitters fail to realize is, the moment your code hits
upstream, it becomes the maintainers' problem and submitters can relax.
But maintainers get to deal with this code forever. So after a while
maintainers learn that they either accept ready code and it all just
works or they make the mistake to take half-baked crap in and then they
themselves get to clean it up and fix it.
So maintainers learn quickly to push back.
But it is annoying and it would help immensely if submitters would
consider this and stop hurrying the code in but try to do a *good* job
first, design-wise and code-wise by thinking hard about what they're
trying to do.
Yeah, things could be a lot simpler and easier - it only takes a little
bit of effort...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Dec 29, 2021 at 06:38:43PM +0800, Dave Young wrote:
I appreciate you further explanation below to describe the situation.
I do not see how can I tell this to *all* submitters,
You don't have to - that was hypothetical. :-)
I'm typing this on a public mailing list with the hope that people will
see it.
but I am and I will try to do this as far as I can.
Much appreciated.
Maintainers and patch submitters, it would help for both
parties show sympathy with each other, some soft reminders will help
people to understand each other, especially for new comers.
On Wed, Dec 29, 2021 at 03:45:12PM +0800, Dave Young wrote:
quoted
BTW, I would suggest to wait for reviewers to response (eg. one week at
least, or more due to the holidays) before updating another version
Do not worry to miss the 5.17. I would say take it easy if it will
miss then let's just leave with it and continue to work on the future
improvements. I think one reason this issue takes too long time is that it was
discussed some time but no followup and later people need to warm up
again. Just keep it warm and continue to engage in the improvements, do
not hurry for the specific mainline release.
Can you tell this to *all* patch submitters please?
I appreciate you further explanation below to describe the situation. I do not
see how can I tell this to *all* submitters, but I am and I will try to do this
as far as I can. Maintainers and patch submitters, it would help for both
parties show sympathy with each other, some soft reminders will help
people to understand each other, especially for new comers.
quoted
I can't count the times where people simply hurry to send the new
revision just to get it in the next kernel, and make silly mistakes
while doing so. Or not think things straight and misdesign it all.
And what this causes is the opposite of what they wanna achieve - pissed
maintainers and ignored threads.
I just hope the first 4 patches can be merged into v5.17. It seems to me
that it is quite clear. Although the goal of the final stage is to modify
function parse_crashkernel() according to the current opinion. But that's not a
lightweight change after all. The final parse_crashkernel() change may take
one version or two. In this process, it maybe OK to do a part of cleanup first.
It's like someone who wants to buy a luxury car to commute to work six months
later. He buys a cheap used car and sells it six months later. It sounds right
to me, don't you think?
quoted
And they all *know* that the next kernel is around the corner. So why
the hell does it even matter when?
Because all programmers should have confidence in the code they write. I have
a new idea, and I'm free these days, so I updated v19. I can't rely on people
telling me to take a step forward, then take a step forward. Otherwise, stand
still.
quoted
What most submitters fail to realize is, the moment your code hits
upstream, it becomes the maintainers' problem and submitters can relax.
Sorry, I'll make sure all the comments are collected and then send the next
edition.
quoted
But maintainers get to deal with this code forever. So after a while
maintainers learn that they either accept ready code and it all just
works or they make the mistake to take half-baked crap in and then they
themselves get to clean it up and fix it.
So maintainers learn quickly to push back.
But it is annoying and it would help immensely if submitters would
consider this and stop hurrying the code in but try to do a *good* job
first, design-wise and code-wise by thinking hard about what they're
trying to do.
Yeah, things could be a lot simpler and easier - it only takes a little
bit of effort...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Dec 29, 2021 at 03:27:48PM +0800, Dave Young wrote:
So I think you can unify the parse_crashkernel* in x86 first with just
one function. And leave the further improvements to later work. But
let's see how Boris think about this.
Well, I think this all unnecessary work. Why?
If the goal is to support crashkernel...high,low on arm64, then you
should simply *copy* the functionality on arm64 and be done with it.
Unification is done by looking at code which is duplicated across
architectures and which has been untouched for a while now, i.e., no
new or arch-specific changes are going to it so a unification can be
as simple as trivially switching the architectures to call a generic
function.
What this does is carve out the "generic" parts and then try not to
break existing usage.
Which is a total waste of energy and resources. And it is casting that
functionality in stone so that when x86 wants to change something there,
it should do it in a way not to break arm64. And I fail to see the
advantage of all that. Code sharing ain't it.
So what it should do is simply copy the necessary code to arm64.
Unifications can always be done later, when the dust settles.
IMNSVHO.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Dave Young <hidden> Date: 2021-12-29 10:48:17
On 12/29/21 at 11:03am, Borislav Petkov wrote:
On Wed, Dec 29, 2021 at 03:27:48PM +0800, Dave Young wrote:
quoted
So I think you can unify the parse_crashkernel* in x86 first with just
one function. And leave the further improvements to later work. But
let's see how Boris think about this.
Well, I think this all unnecessary work. Why?
If the goal is to support crashkernel...high,low on arm64, then you
should simply *copy* the functionality on arm64 and be done with it.
Unification is done by looking at code which is duplicated across
architectures and which has been untouched for a while now, i.e., no
new or arch-specific changes are going to it so a unification can be
as simple as trivially switching the architectures to call a generic
function.
What this does is carve out the "generic" parts and then try not to
break existing usage.
Which is a total waste of energy and resources. And it is casting that
functionality in stone so that when x86 wants to change something there,
it should do it in a way not to break arm64. And I fail to see the
advantage of all that. Code sharing ain't it.
So what it should do is simply copy the necessary code to arm64.
Unifications can always be done later, when the dust settles.
I think I agree with you about the better way is to doing some
improvements so that arches can logically doing things better. I can
leave with the way I suggested although it is not the best. But I think
Leizhen needs a clear direction about how to do it. It is very clear
now. See how he will handle this.
On Wed, Dec 29, 2021 at 03:27:48PM +0800, Dave Young wrote:
quoted
So I think you can unify the parse_crashkernel* in x86 first with just
one function. And leave the further improvements to later work. But
let's see how Boris think about this.
Well, I think this all unnecessary work. Why?
If the goal is to support crashkernel...high,low on arm64, then you
should simply *copy* the functionality on arm64 and be done with it.
Unification is done by looking at code which is duplicated across
architectures and which has been untouched for a while now, i.e., no
new or arch-specific changes are going to it so a unification can be
as simple as trivially switching the architectures to call a generic
function.
What this does is carve out the "generic" parts and then try not to
break existing usage.
Which is a total waste of energy and resources. And it is casting that
functionality in stone so that when x86 wants to change something there,
it should do it in a way not to break arm64. And I fail to see the
advantage of all that. Code sharing ain't it.
It's just a worry, there's uncertainty about whether it's going to be. I think
the only thing that might change is the default value of "low_size". Of course,
the alignment size and start address may also change, but most of them can be
controlled by macros.
Chen Zhou and I tried to share the code because of a suggestion. After so many
attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured
out a good solution yet.
quoted
So what it should do is simply copy the necessary code to arm64.
Unifications can always be done later, when the dust settles.
I think I agree with you about the better way is to doing some
improvements so that arches can logically doing things better. I can
leave with the way I suggested although it is not the best. But I think
Leizhen needs a clear direction about how to do it. It is very clear
now. See how he will handle this.
Surviving, then pursuing ideals.
I will put the patches that make arm64 support crashkernel...high,low to
the front, then the parse_crashkernel() unification patches. Even if the
second half of the patches is not ready for v5.18, the first half of the
patches is ready.
On Wed, Dec 29, 2021 at 11:04:21PM +0800, Leizhen (ThunderTown) wrote:
Chen Zhou and I tried to share the code because of a suggestion. After so many
attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured
out a good solution yet.
Well, you learned a very important lesson and the many attempts are not
in vain: code sharing does not make sense in every case.
I will put the patches that make arm64 support crashkernel...high,low to
the front, then the parse_crashkernel() unification patches. Even if the
second half of the patches is not ready for v5.18, the first half of the
patches is ready.
I think you should concentrate on the arm64 side which is, AFAICT, what
you're trying to achieve.
The "parse_crashkernel() unification" needs more thought because, as I
said already, that doesn't make a whole lot of sense to me.
If you want to enforce the fact that "low" makes sense only when "high"
is supplied, parse_crashkernel_high_low() is not the right thing to do.
You need to have a *single* function which does all the parsing where
you can decide what to do: "if high, parse low", "if no high supplied,
ignore low" and so on.
And if those are supported on certain architectures only, you can do
ifdeffery...
But I think I already stated that I don't like such unifications which
introduce unnecessary dependencies between architectures. Therefore, I
won't accept them into x86 unless there's a strong compelling reason.
Which I don't see ATM.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Dec 29, 2021 at 11:04:21PM +0800, Leizhen (ThunderTown) wrote:
quoted
Chen Zhou and I tried to share the code because of a suggestion. After so many
attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured
out a good solution yet.
Well, you learned a very important lesson and the many attempts are not
in vain: code sharing does not make sense in every case.
quoted
I will put the patches that make arm64 support crashkernel...high,low to
the front, then the parse_crashkernel() unification patches. Even if the
second half of the patches is not ready for v5.18, the first half of the
patches is ready.
I think you should concentrate on the arm64 side which is, AFAICT, what
you're trying to achieve.
Right, a patchset should focus on just one thing.
The "parse_crashkernel() unification" needs more thought because, as I
said already, that doesn't make a whole lot of sense to me.
Yes, because it's not a functional improvement, it's not a performance optimization,
it's also not a fix for a known bug, it's just a programmer's artistic pursuit.
If you want to enforce the fact that "low" makes sense only when "high"
is supplied, parse_crashkernel_high_low() is not the right thing to do.
You need to have a *single* function which does all the parsing where
you can decide what to do: "if high, parse low", "if no high supplied,
ignore low" and so on.
I understand your proposal, but parse_crashkernel_high_low() is a cost-effective
and profitable change, it makes the current code a little clearer, and avoid passing
unnecessary parameters "system_ram" and "crash_base" when other architectures use
parse_crashkernel_{high|low}().
I actually followed your advice in the beginning to do "parse_crashkernel() and
parse_crashkernel_{high|low}() unification". But I found it's difficult and the
end result may not be as good as expected. So I introduced parse_crashkernel_high_low().
The parameter "system_ram" and "crash_base" of parse_crashkernel() is not need by
"crashkernel=X,[high,low]". And parameter "low_size" of parse_crashkernel_high_low()
is not need by "crashkernel=X[@offset]". The "parse_crashkernel() unification"
complicates things. For example, the parameter "crash_size" means "low or high" memory
size for "crashkernel=X[@offset]", but only means "high" memory size for "crashkernel=X,high".
So we'd better give it two names with union.
And if those are supported on certain architectures only, you can do
ifdeffery...
I don't think so. These __init functions are small and architecture-independent, and do not
affect compilation of other architectures. There may be other architectures that use
it in the future, such as the current arm64.
But I think I already stated that I don't like such unifications which
introduce unnecessary dependencies between architectures. Therefore, I
won't accept them into x86 unless there's a strong compelling reason.
Which I don't see ATM.
On Wed, Dec 29, 2021 at 11:04:21PM +0800, Leizhen (ThunderTown) wrote:
quoted
Chen Zhou and I tried to share the code because of a suggestion. After so many
attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured
out a good solution yet.
Well, you learned a very important lesson and the many attempts are not
in vain: code sharing does not make sense in every case.
quoted
I will put the patches that make arm64 support crashkernel...high,low to
the front, then the parse_crashkernel() unification patches. Even if the
second half of the patches is not ready for v5.18, the first half of the
patches is ready.
I think you should concentrate on the arm64 side which is, AFAICT, what
you're trying to achieve.
Right, a patchset should focus on just one thing.
quoted
The "parse_crashkernel() unification" needs more thought because, as I
said already, that doesn't make a whole lot of sense to me.
Yes, because it's not a functional improvement, it's not a performance optimization,
it's also not a fix for a known bug, it's just a programmer's artistic pursuit.
quoted
If you want to enforce the fact that "low" makes sense only when "high"
is supplied, parse_crashkernel_high_low() is not the right thing to do.
You need to have a *single* function which does all the parsing where
you can decide what to do: "if high, parse low", "if no high supplied,
ignore low" and so on.
In fact, this is how my current function parse_crashkernel_high_low() is
implemented.
+ /* crashkernel=X,high */
+ ret = parse_crashkernel_high(cmdline, 0, high_size, &base);
+ if (ret) //crashkernel=X,high is not specified
+ return ret;
+
+ if (*high_size <= 0) //crashkernel=X,high is specified but the value is invalid
+ return -EINVAL; //Sorry, the type of high_size is "unsigned long long *", so less than zero is impossible
+
+ /* crashkernel=Y,low */
+ ret = parse_crashkernel_low(cmdline, 0, low_size, &base); //If crashkernel=Y,low is specified, the parsed value is stored in *low_size
+ if (ret)
+ *low_size = -1; //crashkernel=Y,low is not specified
I understand your proposal, but parse_crashkernel_high_low() is a cost-effective
and profitable change, it makes the current code a little clearer, and avoid passing
unnecessary parameters "system_ram" and "crash_base" when other architectures use
parse_crashkernel_{high|low}().
I actually followed your advice in the beginning to do "parse_crashkernel() and
parse_crashkernel_{high|low}() unification". But I found it's difficult and the
end result may not be as good as expected. So I introduced parse_crashkernel_high_low().
The parameter "system_ram" and "crash_base" of parse_crashkernel() is not need by
"crashkernel=X,[high,low]". And parameter "low_size" of parse_crashkernel_high_low()
is not need by "crashkernel=X[@offset]". The "parse_crashkernel() unification"
complicates things. For example, the parameter "crash_size" means "low or high" memory
size for "crashkernel=X[@offset]", but only means "high" memory size for "crashkernel=X,high".
So we'd better give it two names with union.
quoted
And if those are supported on certain architectures only, you can do
ifdeffery...
I don't think so. These __init functions are small and architecture-independent, and do not
affect compilation of other architectures. There may be other architectures that use
it in the future, such as the current arm64.
quoted
But I think I already stated that I don't like such unifications which
introduce unnecessary dependencies between architectures. Therefore, I
won't accept them into x86 unless there's a strong compelling reason.
Which I don't see ATM.
On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote:
quoted
On 2021/12/29 0:13, Borislav Petkov wrote:
quoted
On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
quoted
Use parse_crashkernel_high_low() to bring the parsing of
"crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
are strongly dependent, make code logic clear and more readable.
Suggested-by: Borislav Petkov <bp@alien8.de>
Yeah, doesn't look like something I suggested...
quoted
@@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) /* crashkernel=XM */ ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); if (ret != 0 || crash_size <= 0) {- /* crashkernel=X,high */- ret = parse_crashkernel_high(boot_command_line, total_mem,- &crash_size, &crash_base);- if (ret != 0 || crash_size <= 0)+ /* crashkernel=X,high and possible crashkernel=Y,low */+ ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
So this calls parse_crashkernel() and when that one fails, it calls this
new weird parse high/low helper you added.
But then all three end up in the same __parse_crashkernel() worker
function which seems to do the actual parsing.
What I suggested and what would be real clean is if the arches would
simply call a *single*
parse_crashkernel()
function and when that one returns, *all* crashkernel= options would
have been parsed properly, low, high, middle crashkernel, whatever...
and the caller would know what crash kernel needs to be allocated.
Then each arch can do its memory allocations and checks based on that
parsed data and decide to allocate or bail.
However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
will also support it. It is not supported on other architectures. So changing parse_crashkernel()
is not appropriate unless a new function is introduced. But naming this new function isn't easy,
and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
Of course, we can also consider changing parse_crashkernel() to another name, then use
parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
other architectures to change as well.
Hi, I did not follow up all discussions, but if the only difference is
about the low -> high fallback, I think you can add another argument in
parse_crashkernel(..., *fallback_high), and doing some changes in
__parse_crashkernel() before it returns. But since there are two
many arguments, you could need a wrapper struct for crashkernel_param if
needed.
A wrapper struct is needed. Because at least two new output parameters need to be added:
1. high, to indicate whether "crashkernel=X,high" is specified
2. low_size, to save the memory size specified by "crashkernel=Y,low"
If you do not want to touch other arches, another function maybe
something like parse_crashkernel_fallback() for x86 and arm64 to use.
But I may not get all the context, please ignore if this is not the
case. I agree that calling parse_crash_kernel* in the
reserve_crashkernel funtions looks not good though.
OTOH there are bunch of other logics in param parsing code,
eg. determin the final size and offset etc. To split the logic out more
things need to be done, eg. firstly parsing function just get all the
inputs from cmdline string eg. an array of struct crashkernel_param with
mem_range, expected size, expected offset, high, or low, and do not do
any other things. Then pass these parsed inputs to another function to
determine the final crash_size and crash_base, that part can be arch
specific somehow.
Yes, this way makes the code logic clear. But there's a bit of trouble with
"crashkernel=range1:size1[,range2:size2,...][@offset]", the array size is dynamic.
So I think you can unify the parse_crashkernel* in x86 first with just
one function. And leave the further improvements to later work. But
let's see how Boris think about this.
quoted
quoted
So it is getting there but it needs more surgery...
Thx.
Use parse_crashkernel_high_low() to bring the parsing of
"crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
are strongly dependent, make code logic clear and more readable.
Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Zhen Lei <redacted>
The bootup command line option crashkernel=Y,low is valid only when
crashkernel=X,high is specified. Putting their parsing into a separate
function makes the code logic clearer and easier to understand the strong
dependencies between them.
Signed-off-by: Zhen Lei <redacted>
---
include/linux/crash_core.h | 3 +++
kernel/crash_core.c | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
@@ -83,5 +83,8 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,unsignedlonglong*crash_size,unsignedlonglong*crash_base);intparse_crashkernel_low(char*cmdline,unsignedlonglongsystem_ram,unsignedlonglong*crash_size,unsignedlonglong*crash_base);+int__initparse_crashkernel_high_low(char*cmdline,+unsignedlonglong*high_size,+unsignedlonglong*low_size);#endif /* LINUX_CRASH_CORE_H */
Hi, Dave, Baoquan, Borislav:
What do you think about the introduction of parse_crashkernel_high_low()? If everyone
doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
patches, see below. If there's any objection, I still strongly recommend removing the
parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
How about splitting __parse_crashkernel() into two parts? One for parsing
"crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
suffixes in the future. So the parameter requirements are clear at the lowest level.
On 2021/12/28 21:26, Zhen Lei wrote:
quoted hunk
The bootup command line option crashkernel=Y,low is valid only when
crashkernel=X,high is specified. Putting their parsing into a separate
function makes the code logic clearer and easier to understand the strong
dependencies between them.
Signed-off-by: Zhen Lei <redacted>
---
include/linux/crash_core.h | 3 +++
kernel/crash_core.c | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
@@ -83,5 +83,8 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,unsignedlonglong*crash_size,unsignedlonglong*crash_base);intparse_crashkernel_low(char*cmdline,unsignedlonglongsystem_ram,unsignedlonglong*crash_size,unsignedlonglong*crash_base);+int__initparse_crashkernel_high_low(char*cmdline,+unsignedlonglong*high_size,+unsignedlonglong*low_size);#endif /* LINUX_CRASH_CORE_H */
On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote:
Hi, Dave, Baoquan, Borislav:
What do you think about the introduction of parse_crashkernel_high_low()? If everyone
doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
patches, see below. If there's any objection, I still strongly recommend removing the
parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
How about splitting __parse_crashkernel() into two parts? One for parsing
"crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
suffixes in the future. So the parameter requirements are clear at the lowest level.
On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote:
quoted
Hi, Dave, Baoquan, Borislav:
What do you think about the introduction of parse_crashkernel_high_low()? If everyone
doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
patches, see below. If there's any objection, I still strongly recommend removing the
parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
How about splitting __parse_crashkernel() into two parts? One for parsing
"crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
suffixes in the future. So the parameter requirements are clear at the lowest level.
First of all, please do not top post!
Now, I already explained to you what I'd like to see:
https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic
yet you still don't get it.
So let me make myself clear: in its current form, this is not really an
improvement so for all x86 changes:
NAKed-by: Borislav Petkov [off-list ref]
OK, thanks for your immediate reply, so I can take less detours.
--
Regards,
Zhen Lei
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote:
quoted
Hi, Dave, Baoquan, Borislav:
What do you think about the introduction of parse_crashkernel_high_low()? If everyone
doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
patches, see below. If there's any objection, I still strongly recommend removing the
parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
How about splitting __parse_crashkernel() into two parts? One for parsing
"crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
suffixes in the future. So the parameter requirements are clear at the lowest level.
First of all, please do not top post!
Now, I already explained to you what I'd like to see:
https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic
yet you still don't get it.
So let me make myself clear: in its current form, this is not really an
improvement so for all x86 changes:
NAKed-by: Borislav Petkov [off-list ref]
Hi Borislav:
I'm sorry to bother you again. Do you mind if I make the following changes?
I can't stand so many comments appearing twice. Even if the size needs to be
changed in the future, mode "low_size = CRASH_LOW_SIZE_MIN + <increment>" can
be used for adaptation without affecting other architectures.
On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote:
quoted
Hi, Dave, Baoquan, Borislav:
What do you think about the introduction of parse_crashkernel_high_low()? If everyone
doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
patches, see below. If there's any objection, I still strongly recommend removing the
parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
How about splitting __parse_crashkernel() into two parts? One for parsing
"crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
suffixes in the future. So the parameter requirements are clear at the lowest level.
First of all, please do not top post!
Now, I already explained to you what I'd like to see:
https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic
yet you still don't get it.
So let me make myself clear: in its current form, this is not really an
improvement so for all x86 changes:
NAKed-by: Borislav Petkov [off-list ref]
Hi Borislav:
I'm sorry to bother you again. Do you mind if I make the following changes?
I can't stand so many comments appearing twice. Even if the size needs to be
changed in the future, mode "low_size = CRASH_LOW_SIZE_MIN + <increment>" can
be used for adaptation without affecting other architectures.
I rethink it, the default value of default_nslabs is IO_TLB_DEFAULT_SIZE=64M.
The value of default_nslabs can only be changed by swiotlb_adjust_size() and
bootup command line option "swiotlb=". Currently, swiotlb_adjust_size() is
invoked only on x86, so I can just ignore it on arm64. Then, 64M is much
smaller than 256M, the first kernel works fine with the default 64M on arm64,
and I don't think the second kernel needs to grow to 256M. Therefore, I think
swiotlb_adjust_size() is probably a pseudo requirement for arm64.
So I will directly use 256M on arm64. If anyone gets into trouble, he/she can
add it back. Besides, there is also "crashkernel=Y,low" can be used.
The bootup command line option crashkernel=Y,low is valid only when
crashkernel=X,high is specified. Putting their parsing into a separate
function makes the code logic clearer and easier to understand the strong
dependencies between them.
Signed-off-by: Zhen Lei <redacted>
@@ -83,5 +83,8 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,unsignedlonglong*crash_size,unsignedlonglong*crash_base);intparse_crashkernel_low(char*cmdline,unsignedlonglongsystem_ram,unsignedlonglong*crash_size,unsignedlonglong*crash_base);+int__initparse_crashkernel_high_low(char*cmdline,+unsignedlonglong*high_size,+unsignedlonglong*low_size);#endif /* LINUX_CRASH_CORE_H */
Make parse_crashkernel_{high|low}() static, they are only referenced by
parse_crashkernel_high_low() in the same file. The latter is recommended.
Signed-off-by: Zhen Lei <redacted>
---
include/linux/crash_core.h | 4 ----
kernel/crash_core.c | 4 ++--
2 files changed, 2 insertions(+), 6 deletions(-)
Make parse_crashkernel_{high|low}() static, they are only referenced by
parse_crashkernel_high_low() in the same file. The latter is recommended.
Signed-off-by: Zhen Lei <redacted>
Delete confusing parameters 'system_ram' and 'crash_base' of
parse_crashkernel_{high|low}(), they are only needed by the case of
"crashkernel=X@[offset]".
Signed-off-by: Zhen Lei <redacted>
---
kernel/crash_core.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
Delete confusing parameters 'system_ram' and 'crash_base' of
parse_crashkernel_{high|low}(), they are only needed by the case of
"crashkernel=X@[offset]".
Signed-off-by: Zhen Lei <redacted>
Add macro CRASH_BASE_ALIGN to indicate the alignment for crash kernel
fixed region, in preparation for making partial implementation of
reserve_crashkernel[_low]() generic.
Signed-off-by: Zhen Lei <redacted>
---
arch/x86/kernel/setup.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
Add macro CRASH_BASE_ALIGN to indicate the alignment for crash kernel
fixed region, in preparation for making partial implementation of
reserve_crashkernel[_low]() generic.
Signed-off-by: Zhen Lei <redacted>
From: Chen Zhou <redacted>
Move the definition and declaration of global variable crashk[_low]_res
from kexec module to crash_core module, in preparation of adding generic
reserve_crashkernel_mem[_low]() to crash_core.c, the latter refers to
variable crashk[_low]_res. Due to the config KEXEC automatically selects
CRASH_CORE, and the header crash_core.h is included by kexec.h, so there
is no functional change.
Signed-off-by: Chen Zhou <redacted>
Signed-off-by: Zhen Lei <redacted>
---
include/linux/crash_core.h | 4 ++++
include/linux/kexec.h | 4 ----
kernel/crash_core.c | 16 ++++++++++++++++
kernel/kexec_core.c | 17 -----------------
4 files changed, 20 insertions(+), 21 deletions(-)
@@ -73,6 +73,10 @@ extern unsigned char *vmcoreinfo_data;externsize_tvmcoreinfo_size;externu32*vmcoreinfo_note;+/* Location of a reserved region to hold the crash kernel. */+externstructresourcecrashk_res;+externstructresourcecrashk_low_res;+Elf_Word*append_elf_note(Elf_Word*buf,char*name,unsignedinttype,void*data,size_tdata_len);voidfinal_note(Elf_Word*buf);
@@ -350,10 +350,6 @@ extern int kexec_load_disabled;#define KEXEC_FILE_FLAGS (KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \KEXEC_FILE_NO_INITRAMFS)-/* Location of a reserved region to hold the crash kernel.-*/-externstructresourcecrashk_res;-externstructresourcecrashk_low_res;externnote_buf_t__percpu*crash_notes;/* flag to track if kexec reboot is in progress */
@@ -22,6 +22,22 @@ u32 *vmcoreinfo_note;/* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */staticunsignedchar*vmcoreinfo_data_safecopy;+/* Location of the reserved area for the crash kernel */+structresourcecrashk_res={+.name="Crash kernel",+.start=0,+.end=0,+.flags=IORESOURCE_BUSY|IORESOURCE_SYSTEM_RAM,+.desc=IORES_DESC_CRASH_KERNEL+};+structresourcecrashk_low_res={+.name="Crash kernel",+.start=0,+.end=0,+.flags=IORESOURCE_BUSY|IORESOURCE_SYSTEM_RAM,+.desc=IORES_DESC_CRASH_KERNEL+};+/**parsingthe"crashkernel"commandline*
@@ -54,23 +54,6 @@ note_buf_t __percpu *crash_notes;/* Flag to indicate we are going to kexec a new kernel */boolkexec_in_progress=false;--/* Location of the reserved area for the crash kernel */-structresourcecrashk_res={-.name="Crash kernel",-.start=0,-.end=0,-.flags=IORESOURCE_BUSY|IORESOURCE_SYSTEM_RAM,-.desc=IORES_DESC_CRASH_KERNEL-};-structresourcecrashk_low_res={-.name="Crash kernel",-.start=0,-.end=0,-.flags=IORESOURCE_BUSY|IORESOURCE_SYSTEM_RAM,-.desc=IORES_DESC_CRASH_KERNEL-};-intkexec_should_crash(structtask_struct*p){/*
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Chen Zhou <redacted>
Move the definition and declaration of global variable crashk[_low]_res
from kexec module to crash_core module, in preparation of adding generic
reserve_crashkernel_mem[_low]() to crash_core.c, the latter refers to
variable crashk[_low]_res. Due to the config KEXEC automatically selects
CRASH_CORE, and the header crash_core.h is included by kexec.h, so there
is no functional change.
Signed-off-by: Chen Zhou <redacted>
Signed-off-by: Zhen Lei <redacted>
@@ -73,6 +73,10 @@ extern unsigned char *vmcoreinfo_data;externsize_tvmcoreinfo_size;externu32*vmcoreinfo_note;+/* Location of a reserved region to hold the crash kernel. */+externstructresourcecrashk_res;+externstructresourcecrashk_low_res;+Elf_Word*append_elf_note(Elf_Word*buf,char*name,unsignedinttype,void*data,size_tdata_len);voidfinal_note(Elf_Word*buf);
@@ -350,10 +350,6 @@ extern int kexec_load_disabled;#define KEXEC_FILE_FLAGS (KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \KEXEC_FILE_NO_INITRAMFS)-/* Location of a reserved region to hold the crash kernel.-*/-externstructresourcecrashk_res;-externstructresourcecrashk_low_res;externnote_buf_t__percpu*crash_notes;/* flag to track if kexec reboot is in progress */
@@ -22,6 +22,22 @@ u32 *vmcoreinfo_note;/* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */staticunsignedchar*vmcoreinfo_data_safecopy;+/* Location of the reserved area for the crash kernel */+structresourcecrashk_res={+.name="Crash kernel",+.start=0,+.end=0,+.flags=IORESOURCE_BUSY|IORESOURCE_SYSTEM_RAM,+.desc=IORES_DESC_CRASH_KERNEL+};+structresourcecrashk_low_res={+.name="Crash kernel",+.start=0,+.end=0,+.flags=IORESOURCE_BUSY|IORESOURCE_SYSTEM_RAM,+.desc=IORES_DESC_CRASH_KERNEL+};+/**parsingthe"crashkernel"commandline*
@@ -54,23 +54,6 @@ note_buf_t __percpu *crash_notes;/* Flag to indicate we are going to kexec a new kernel */boolkexec_in_progress=false;--/* Location of the reserved area for the crash kernel */-structresourcecrashk_res={-.name="Crash kernel",-.start=0,-.end=0,-.flags=IORESOURCE_BUSY|IORESOURCE_SYSTEM_RAM,-.desc=IORES_DESC_CRASH_KERNEL-};-structresourcecrashk_low_res={-.name="Crash kernel",-.start=0,-.end=0,-.flags=IORESOURCE_BUSY|IORESOURCE_SYSTEM_RAM,-.desc=IORES_DESC_CRASH_KERNEL-};-intkexec_should_crash(structtask_struct*p){/*
Use generic reserve_crashkernel_mem[_low]() to replace arch-specific
reserve_crashkernel_low() and a partial implementation of
reserve_crashkernel(). The only difference is that
"insert_resource(&iomem_resource, &crashk_low_res);" is moved into
reserve_crashkernel(), no functional change.
Signed-off-by: Zhen Lei <redacted>
---
arch/x86/kernel/setup.c | 93 ++---------------------------------------
1 file changed, 4 insertions(+), 89 deletions(-)
@@ -391,52 +391,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)*/#ifdef CONFIG_KEXEC_CORE--staticint__initreserve_crashkernel_low(unsignedlonglonglow_size)-{-#ifdef CONFIG_X86_64-unsignedlonglonglow_base=0;-unsignedlonglow_mem_limit;--low_mem_limit=min(memblock_phys_mem_size(),CRASH_ADDR_LOW_MAX);--/* crashkernel=Y,low is not specified */-if((long)low_size<0){-/*-*twopartsfromkernel/dma/swiotlb.c:-*-swiotlbsize:user-specifiedwithswiotlb=ordefault.-*-*-swiotlboverflowbuffer:nowhardcodedto32k.Weroundit-*to8Mforotherbuffersthatmayneedtostaylowtoo.Also-*makesureweallocateenoughextralowmemorysothatwe-*don'trunoutofDMAbuffersfor32-bitdevices.-*/-low_size=max(swiotlb_size_or_default()+(8UL<<20),256UL<<20);-}else{-/* passed with crashkernel=0,low ? */-if(!low_size)-return0;-}--low_base=memblock_phys_alloc_range(low_size,CRASH_ALIGN,0,CRASH_ADDR_LOW_MAX);-if(!low_base){-pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",-(unsignedlong)(low_size>>20));-return-ENOMEM;-}--pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (low RAM limit: %ldMB)\n",-(unsignedlong)(low_size>>20),-(unsignedlong)(low_base>>20),-(unsignedlong)(low_mem_limit>>20));--crashk_low_res.start=low_base;-crashk_low_res.end=low_base+low_size-1;-insert_resource(&iomem_resource,&crashk_low_res);-#endif-return0;-}-staticvoid__initreserve_crashkernel(void){unsignedlonglongcrash_size,crash_base,total_mem,low_size;
@@ -460,51 +414,12 @@ static void __init reserve_crashkernel(void)return;}-/* 0 means: find the address automatically */-if(!crash_base){-/*-*SetCRASH_ADDR_LOW_MAXupperboundforcrashmemory,-*crashkernel=x,highreservesmemoryover4G,alsoallocates-*256MextralowmemoryforDMAbuffersandswiotlb.-*Buttheextramemoryisnotrequiredforallmachines.-*Sotrylowmemoryfirstandfallbacktohighmemory-*unless"crashkernel=size[KMG],high"isspecified.-*/-if(!high)-crash_base=memblock_phys_alloc_range(crash_size,-CRASH_ALIGN,CRASH_ALIGN,-CRASH_ADDR_LOW_MAX);-if(!crash_base)-crash_base=memblock_phys_alloc_range(crash_size,-CRASH_ALIGN,CRASH_ALIGN,-CRASH_ADDR_HIGH_MAX);-if(!crash_base){-pr_info("crashkernel reservation failed - No suitable area found.\n");-return;-}-}else{-unsignedlonglongstart;--start=memblock_phys_alloc_range(crash_size,CRASH_BASE_ALIGN,crash_base,-crash_base+crash_size);-if(start!=crash_base){-pr_info("crashkernel reservation failed - memory is in use.\n");-return;-}-}--if(crash_base>=(1ULL<<32)&&reserve_crashkernel_low(low_size)){-memblock_phys_free(crash_base,crash_size);+ret=reserve_crashkernel_mem(total_mem,crash_size,crash_base,low_size,high);+if(ret)return;-}--pr_info("Reserving %ldMB of memory at %ldMB for crashkernel (System RAM: %ldMB)\n",-(unsignedlong)(crash_size>>20),-(unsignedlong)(crash_base>>20),-(unsignedlong)(total_mem>>20));-crashk_res.start=crash_base;-crashk_res.end=crash_base+crash_size-1;+if(crashk_low_res.end>crashk_low_res.start)+insert_resource(&iomem_resource,&crashk_low_res);insert_resource(&iomem_resource,&crashk_res);}#else
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Chen Zhou <redacted>
For arm64, the behavior of crashkernel=X has been changed, which
tries low allocation in DMA zone and fall back to high allocation
if it fails.
We can also use "crashkernel=X,high" to select a high region above
DMA zone, which also tries to allocate at least 256M low memory in
DMA zone automatically and "crashkernel=Y,low" can be used to allocate
specified size low memory.
So update the Documentation.
Signed-off-by: Chen Zhou <redacted>
Signed-off-by: Zhen Lei <redacted>
---
Documentation/admin-guide/kdump/kdump.rst | 11 +++++++++--
Documentation/admin-guide/kernel-parameters.txt | 11 +++++++++--
2 files changed, 18 insertions(+), 4 deletions(-)
@@ -361,8 +361,15 @@ Boot into System Kernel kernel will automatically locate the crash kernel image within the first 512MB of RAM if X is not given.- On arm64, use "crashkernel=Y[@X]". Note that the start address of- the kernel, X if explicitly specified, must be aligned to 2MiB (0x200000).+ On arm64, use "crashkernel=X" to try low allocation in DMA zone and+ fall back to high allocation if it fails.+ We can also use "crashkernel=X,high" to select a high region above+ DMA zone, which also tries to allocate at least 256M low memory in+ DMA zone automatically.+ "crashkernel=Y,low" can be used to allocate specified size low memory.+ Use "crashkernel=Y@X" if you really have to reserve memory from+ specified start address X. Note that the start address of the kernel,+ X if explicitly specified, must be aligned to 2MiB (0x200000). Load the Dump-capture Kernel ============================
@@ -783,6 +783,9 @@ [KNL, X86-64] Select a region under 4G first, and fall back to reserve region above 4G when '@offset' hasn't been specified.+ [KNL, ARM64] Try low allocation in DMA zone and fall back+ to high allocation if it fails when '@offset' hasn't been+ specified. See Documentation/admin-guide/kdump/kdump.rst for further details. crashkernel=range1:size1[,range2:size2,...][@offset]
@@ -799,6 +802,8 @@ Otherwise memory region will be allocated below 4G, if available. It will be ignored if crashkernel=X is specified.+ [KNL, ARM64] range in high memory.+ Allow kernel to allocate physical memory region from top. crashkernel=size[KMG],low [KNL, X86-64] range under 4G. When crashkernel=X,high is passed, kernel could allocate physical memory region
@@ -807,13 +812,15 @@ requires at least 64M+32K low memory, also enough extra low memory is needed to make sure DMA buffers for 32-bit devices won't run out. Kernel would try to allocate at- at least 256M below 4G automatically.+ least 256M below 4G automatically. This one let user to specify own low range under 4G for second kernel instead. 0: to disable low allocation. It will be ignored when crashkernel=X,high is not used or memory reserved is below 4G.-+ [KNL, ARM64] range in low memory.+ This one let user to specify a low range in DMA zone for+ crash dump kernel. cryptomgr.notests [KNL] Disable crypto self-tests
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Chen Zhou <redacted>
When reserving crashkernel in high memory, some low memory is reserved
for crash dump kernel devices and never mapped by the first kernel.
This memory range is advertised to crash dump kernel via DT property
under /chosen,
linux,usable-memory-range = <BASE1 SIZE1 [BASE2 SIZE2]>
We reused the DT property linux,usable-memory-range and made the low
memory region as the second range "BASE2 SIZE2", which keeps compatibility
with existing user-space and older kdump kernels.
Crash dump kernel reads this property at boot time and call memblock_add()
to add the low memory region after memblock_cap_memory_range() has been
called.
Signed-off-by: Chen Zhou <redacted>
Co-developed-by: Zhen Lei <redacted>
Signed-off-by: Zhen Lei <redacted>
Reviewed-by: Rob Herring <robh@kernel.org>
Tested-by: Dave Kleikamp <redacted>
---
drivers/of/fdt.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
From: Chen Zhou <redacted>
There are following issues in arm64 kdump:
1. We use crashkernel=X to reserve crashkernel below 4G, which
will fail when there is no enough low memory.
2. If reserving crashkernel above 4G, in this case, crash dump
kernel will boot failure because there is no low memory available
for allocation.
To solve these issues, change the behavior of crashkernel=X and
introduce crashkernel=X,[high,low]. crashkernel=X tries low allocation
in DMA zone, and fall back to high allocation if it fails.
We can also use "crashkernel=X,high" to select a region above DMA zone,
which also tries to allocate at least 256M in DMA zone automatically.
"crashkernel=Y,low" can be used to allocate specified size low memory.
Another minor change, there may be two regions reserved for crash
dump kernel, in order to distinct from the high region and make no
effect to the use of existing kexec-tools, rename the low region as
"Crash kernel (low)".
Signed-off-by: Chen Zhou <redacted>
Co-developed-by: Zhen Lei <redacted>
Signed-off-by: Zhen Lei <redacted>
---
arch/arm64/kernel/machine_kexec.c | 5 +++-
arch/arm64/kernel/machine_kexec_file.c | 12 ++++++--
arch/arm64/kernel/setup.c | 13 +++++++-
arch/arm64/mm/init.c | 41 ++++++++++----------------
4 files changed, 42 insertions(+), 29 deletions(-)
@@ -65,10 +65,18 @@ static int prepare_elf_headers(void **addr, unsigned long *sz)/* Exclude crashkernel region */ret=crash_exclude_mem_range(cmem,crashk_res.start,crashk_res.end);+if(ret)+gotoout;++if(crashk_low_res.end){+ret=crash_exclude_mem_range(cmem,crashk_low_res.start,crashk_low_res.end);+if(ret)+gotoout;+}-if(!ret)-ret=crash_prepare_elf64_headers(cmem,true,addr,sz);+ret=crash_prepare_elf64_headers(cmem,true,addr,sz);+out:kfree(cmem);returnret;}
From: Dave Kleikamp <hidden> Date: 2022-01-12 14:46:05
On 12/28/21 7:26AM, Zhen Lei wrote:
quoted hunk
From: Chen Zhou <redacted>
There are following issues in arm64 kdump:
1. We use crashkernel=X to reserve crashkernel below 4G, which
will fail when there is no enough low memory.
2. If reserving crashkernel above 4G, in this case, crash dump
kernel will boot failure because there is no low memory available
for allocation.
To solve these issues, change the behavior of crashkernel=X and
introduce crashkernel=X,[high,low]. crashkernel=X tries low allocation
in DMA zone, and fall back to high allocation if it fails.
We can also use "crashkernel=X,high" to select a region above DMA zone,
which also tries to allocate at least 256M in DMA zone automatically.
"crashkernel=Y,low" can be used to allocate specified size low memory.
Another minor change, there may be two regions reserved for crash
dump kernel, in order to distinct from the high region and make no
effect to the use of existing kexec-tools, rename the low region as
"Crash kernel (low)".
Signed-off-by: Chen Zhou <redacted>
Co-developed-by: Zhen Lei <redacted>
Signed-off-by: Zhen Lei <redacted>
---
arch/arm64/kernel/machine_kexec.c | 5 +++-
arch/arm64/kernel/machine_kexec_file.c | 12 ++++++--
arch/arm64/kernel/setup.c | 13 +++++++-
arch/arm64/mm/init.c | 41 ++++++++++----------------
4 files changed, 42 insertions(+), 29 deletions(-)
@@ -65,10 +65,18 @@ static int prepare_elf_headers(void **addr, unsigned long *sz)/* Exclude crashkernel region */ret=crash_exclude_mem_range(cmem,crashk_res.start,crashk_res.end);+if(ret)+gotoout;++if(crashk_low_res.end){+ret=crash_exclude_mem_range(cmem,crashk_low_res.start,crashk_low_res.end);+if(ret)+gotoout;+}-if(!ret)-ret=crash_prepare_elf64_headers(cmem,true,addr,sz);+ret=crash_prepare_elf64_headers(cmem,true,addr,sz);+out:kfree(cmem);returnret;}
From: Chen Zhou <redacted>
There are following issues in arm64 kdump:
1. We use crashkernel=X to reserve crashkernel below 4G, which
will fail when there is no enough low memory.
2. If reserving crashkernel above 4G, in this case, crash dump
kernel will boot failure because there is no low memory available
for allocation.
To solve these issues, change the behavior of crashkernel=X and
introduce crashkernel=X,[high,low]. crashkernel=X tries low allocation
in DMA zone, and fall back to high allocation if it fails.
We can also use "crashkernel=X,high" to select a region above DMA zone,
which also tries to allocate at least 256M in DMA zone automatically.
"crashkernel=Y,low" can be used to allocate specified size low memory.
Another minor change, there may be two regions reserved for crash
dump kernel, in order to distinct from the high region and make no
effect to the use of existing kexec-tools, rename the low region as
"Crash kernel (low)".
Signed-off-by: Chen Zhou <redacted>
Co-developed-by: Zhen Lei <redacted>
Signed-off-by: Zhen Lei <redacted>
---
arch/arm64/kernel/machine_kexec.c | 5 +++-
arch/arm64/kernel/machine_kexec_file.c | 12 ++++++--
arch/arm64/kernel/setup.c | 13 +++++++-
arch/arm64/mm/init.c | 41 ++++++++++----------------
4 files changed, 42 insertions(+), 29 deletions(-)
kernel_data.end <= res->end)
request_resource(res, &kernel_data);
#ifdef CONFIG_KEXEC_CORE
- /* Userspace will find "Crash kernel" region in /proc/iomem. */
+ /*
+ * Userspace will find "Crash kernel" or "Crash kernel (low)"
+ * region in /proc/iomem.
+ * In order to distinct from the high region and make no effect
+ * to the use of existing kexec-tools, rename the low region as
+ * "Crash kernel (low)".
+ */
+ if (crashk_low_res.end && crashk_low_res.start >= res->start &&
+ crashk_low_res.end <= res->end) {
+ crashk_low_res.name = "Crash kernel (low)";
+ request_resource(res, &crashk_low_res);
+ }
if (crashk_res.end && crashk_res.start >= res->start &&
crashk_res.end <= res->end)
request_resource(res, &crashk_res);
*/
static void __init reserve_crashkernel(void)
{
- unsigned long long crash_base, crash_size;
- unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
+ unsigned long long crash_size, crash_base, total_mem, low_size;
low_size needs to be initialized to -1.
If parse_crashkernel() succeeds, then an uninitialized low_size will be passed to reserve_crashkernel_mem().
Right, thanks, I noticed that too. I'm waiting for v5.17-rc1 to release v20.
In addition, I found that the current implementation on x86 was problematic in case
"crashkernel=4G crashkernel=512M,low". According to the document, "crashkernel=512M,low"
should not take effect at this case. But reserve_crashkernel_low() didn't do that well.
quoted
+ bool high = false;
int ret;
- ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
- &crash_size, &crash_base);
- /* no crashkernel= or invalid value specified */
- if (ret || !crash_size)
- return;
-
- crash_size = PAGE_ALIGN(crash_size);
-
- /* User specifies base address explicitly. */
- if (crash_base)
- crash_max = crash_base + crash_size;
+ total_mem = memblock_phys_mem_size();
- /* Current arm64 boot protocol requires 2MB alignment */
- crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
- crash_base, crash_max);
- if (!crash_base) {
- pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
- crash_size);
- return;
+ ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
+ if (ret != 0 || crash_size <= 0) {
+ /* crashkernel=X,high and possible crashkernel=Y,low */
+ ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
+ if (ret)
+ return;
+ high = true;
}
- pr_info("crashkernel reserved: 0x%016llx - 0x%016llx (%lld MB)\n",
- crash_base, crash_base + crash_size, crash_size >> 20);
+ ret = reserve_crashkernel_mem(total_mem, crash_size, crash_base, low_size, high);
+ if (ret)
+ return;
/*
* The crashkernel memory will be removed from the kernel linear
* map. Inform kmemleak so that it won't try to access it.
*/
- kmemleak_ignore_phys(crash_base);
- crashk_res.start = crash_base;
- crashk_res.end = crash_base + crash_size - 1;
+ kmemleak_ignore_phys(crashk_res.start);
+ if (crashk_low_res.end)
+ kmemleak_ignore_phys(crashk_low_res.start);
}
#else
static void __init reserve_crashkernel(void)
Add helper reserve_crashkernel_mem[_low]() to reserve high and low memory
for crash kernel. The implementation of these two functions is based on
function reserve_crashkernel[_low]() in arch/x86/kernel/setup.c. But the
following adaptations are made:
1. To avoid compilation problems on other architectures, provide default
values for macro CRASH[_BASE]_ALIGN, CRASH_ADDR_{LOW|HIGH}_MAX, and add
new macro CRASH_LOW_SIZE_MIN.
2. Only functions that reserve crash memory are extracted from
reserve_crashkernel(), excluding functions such as parse_crashkernel()
and insert_resource().
3. Change "return;" in reserve_crashkernel() to "return -ENOMEM;".
4. Change call reserve_crashkernel_low() to call
reserve_crashkernel_mem_low().
5. Change CONFIG_X86_64 to CONFIG_64BIT.
Signed-off-by: Zhen Lei <redacted>
---
include/linux/crash_core.h | 6 ++
kernel/crash_core.c | 154 ++++++++++++++++++++++++++++++++++++-
2 files changed, 159 insertions(+), 1 deletion(-)
@@ -86,5 +86,11 @@ int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,int__initparse_crashkernel_high_low(char*cmdline,unsignedlonglong*high_size,unsignedlonglong*low_size);+int__initreserve_crashkernel_mem_low(unsignedlonglonglow_size);+int__initreserve_crashkernel_mem(unsignedlonglongsystem_ram,+unsignedlonglongcrash_size,+unsignedlonglongcrash_base,+unsignedlonglonglow_size,+boolhigh);#endif /* LINUX_CRASH_CORE_H */