On 02/26/21 at 09:38am, Eric W. Biederman wrote:
chenzhou [off-list ref] writes:
quoted
On 2021/2/25 15:25, Baoquan He wrote:
quoted
On 02/24/21 at 02:19pm, Catalin Marinas wrote:
quoted
On Sat, Jan 30, 2021 at 03:10:15PM +0800, Chen Zhou wrote:
quoted
Move CRASH_ALIGN to header asm/kexec.h for later use. Besides, the
alignment of crash kernel regions in x86 is 16M(CRASH_ALIGN), but
function reserve_crashkernel() also used 1M alignment. So just
replace hard-coded alignment 1M with macro CRASH_ALIGN.
[...]
quoted
@@ -510,7 +507,7 @@ static void __init reserve_crashkernel(void)
} else {
unsigned long long start;
- start = memblock_phys_alloc_range(crash_size, SZ_1M, crash_base,
+ start = memblock_phys_alloc_range(crash_size, CRASH_ALIGN, crash_base,
crash_base + crash_size);
if (start != crash_base) {
pr_info("crashkernel reservation failed - memory is in use.\n");
There is a small functional change here for x86. Prior to this patch,
crash_base passed by the user on the command line is allowed to be 1MB
aligned. With this patch, such reservation will fail.
Is the current behaviour a bug in the current x86 code or it does allow
1MB-aligned reservations?
Hmm, you are right. Here we should keep 1MB alignment as is because
users specify the address and size, their intention should be respected.
The 1MB alignment for fixed memory region reservation was introduced in
below commit, but it doesn't tell what is Eric's request at that time, I
guess it meant respecting users' specifying.
quoted
I think we could make the alignment unified. Why is the alignment system reserved and
user specified different? Besides, there is no document about the 1MB alignment.
How about adding the alignment size(16MB) in doc if user specified
start address as arm64 does.
Looking at what the code is doing. Attempting to reserve a crash region
at the location the user specified. Adding unnecessary alignment
constraints is totally broken.
I am not even certain enforcing a 1MB alignment makes sense. I suspect
it was added so that we don't accidentally reserve low memory on x86.
Frankly I am not even certain that makes sense.
Now in practice there might be an argument for 2MB alignment that goes
with huge page sizes on x86. But until someone finds that there are
actual problems with 1MB alignment I would not touch it.
The proper response to something that isn't documented and confusing is
not to arbitrarily change it and risk breaking users. Especially in
this case where it is clear that adding additional alignment is total
nonsense. The proper response to something that isn't clear and
documented is to dig in and document it, or to leave it alone and let it
Sounds reasonable. Then adding document or code comment around looks
like a good way to go further so that people can easily get why its
alignment is different than other reservation.
be the next persons problem.
In this case there is no reason for changing this bit of code.
All CRASH_ALIGN is about is a default alignment when none is specified.
It is not a functional requirement but just something so that things
come out nicely.
Eric