| Message ID | 20210130071025.65258-2-chenzhou10@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support reserving crashkernel above 4G on arm64 kdump | expand |
On 01/30/21 at 03:10pm, Chen Zhou wrote: > 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. > > Suggested-by: Dave Young <dyoung@redhat.com> > Suggested-by: Baoquan He <bhe@redhat.com> > Signed-off-by: Chen Zhou <chenzhou10@huawei.com> > Tested-by: John Donnelly <John.p.donnelly@oracle.com> > --- > arch/x86/include/asm/kexec.h | 3 +++ > arch/x86/kernel/setup.c | 5 +---- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h > index 6802c59e8252..be18dc7ae51f 100644 > --- a/arch/x86/include/asm/kexec.h > +++ b/arch/x86/include/asm/kexec.h > @@ -18,6 +18,9 @@ > > # define KEXEC_CONTROL_CODE_MAX_SIZE 2048 > > +/* 16M alignment for crash kernel regions */ > +#define CRASH_ALIGN SZ_16M > + > #ifndef __ASSEMBLY__ > > #include <linux/string.h> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 3412c4595efd..da769845597d 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -390,9 +390,6 @@ static void __init memblock_x86_reserve_range_setup_data(void) > > #ifdef CONFIG_KEXEC_CORE > > -/* 16M alignment for crash kernel regions */ > -#define CRASH_ALIGN SZ_16M > - > /* > * Keep the crash kernel below this limit. > * > @@ -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); Looks good to me, thx. Acked-by: Baoquan He <bhe@redhat.com> > if (start != crash_base) { > pr_info("crashkernel reservation failed - memory is in use.\n"); > -- > 2.20.1 >
On Sat, Jan 30, 2021 at 03:10:15PM +0800, Chen Zhou wrote: > 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. [...] > @@ -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?
On 02/24/21 at 02:19pm, Catalin Marinas wrote: > On Sat, Jan 30, 2021 at 03:10:15PM +0800, Chen Zhou wrote: > > 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. > [...] > > @@ -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. commit 44280733e71ad15377735b42d8538c109c94d7e3 Author: Yinghai Lu <yinghai@kernel.org> Date: Sun Nov 22 17:18:49 2009 -0800 x86: Change crash kernel to reserve via reserve_early() use find_e820_area()/reserve_early() instead. -v2: address Eric's request, to restore original semantics. will fail, if the provided address can not be used.
On 2021/2/25 15:25, Baoquan He wrote: > On 02/24/21 at 02:19pm, Catalin Marinas wrote: >> On Sat, Jan 30, 2021 at 03:10:15PM +0800, Chen Zhou wrote: >>> 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. >> [...] >>> @@ -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. 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. Thanks, Chen Zhou > > commit 44280733e71ad15377735b42d8538c109c94d7e3 > Author: Yinghai Lu <yinghai@kernel.org> > Date: Sun Nov 22 17:18:49 2009 -0800 > > x86: Change crash kernel to reserve via reserve_early() > > use find_e820_area()/reserve_early() instead. > > -v2: address Eric's request, to restore original semantics. > will fail, if the provided address can not be used. > > . >
chenzhou <chenzhou10@huawei.com> writes: > On 2021/2/25 15:25, Baoquan He wrote: >> On 02/24/21 at 02:19pm, Catalin Marinas wrote: >>> On Sat, Jan 30, 2021 at 03:10:15PM +0800, Chen Zhou wrote: >>>> 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. >>> [...] >>>> @@ -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. > 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 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
On 02/26/21 at 09:38am, Eric W. Biederman wrote: > chenzhou <chenzhou10@huawei.com> writes: > > > On 2021/2/25 15:25, Baoquan He wrote: > >> On 02/24/21 at 02:19pm, Catalin Marinas wrote: > >>> On Sat, Jan 30, 2021 at 03:10:15PM +0800, Chen Zhou wrote: > >>>> 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. > >>> [...] > >>>> @@ -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. > > > > 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 >
On 2021/3/2 15:43, Baoquan He wrote: > On 02/26/21 at 09:38am, Eric W. Biederman wrote: >> chenzhou <chenzhou10@huawei.com> writes: >> >>> On 2021/2/25 15:25, Baoquan He wrote: >>>> On 02/24/21 at 02:19pm, Catalin Marinas wrote: >>>>> On Sat, Jan 30, 2021 at 03:10:15PM +0800, Chen Zhou wrote: >>>>>> 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. >>>>> [...] >>>>>> @@ -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. >> >>> 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. Hi Baoquan & Eric, Sorry for late reply, i missed it earlier. Thanks for your explanation, i will just leave the 1MB alignment here as is. I will introduce CRASH_ALIGN_SPECIFIED to help make function reserve_crashkernel generic. CRASH_ALIGN_SPECIFIED is used for user specified start address which is distinct from default CRASH_ALIGN. Thanks, Chen Zhou > >> 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 >> > . >
diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h index 6802c59e8252..be18dc7ae51f 100644 --- a/arch/x86/include/asm/kexec.h +++ b/arch/x86/include/asm/kexec.h @@ -18,6 +18,9 @@ # define KEXEC_CONTROL_CODE_MAX_SIZE 2048 +/* 16M alignment for crash kernel regions */ +#define CRASH_ALIGN SZ_16M + #ifndef __ASSEMBLY__ #include <linux/string.h> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 3412c4595efd..da769845597d 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -390,9 +390,6 @@ static void __init memblock_x86_reserve_range_setup_data(void) #ifdef CONFIG_KEXEC_CORE -/* 16M alignment for crash kernel regions */ -#define CRASH_ALIGN SZ_16M - /* * Keep the crash kernel below this limit. * @@ -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");