| Message ID | 20240802092510.3915986-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ARM: Support allocating crashkernel above 4G for LPAE | expand |
On Fri, Aug 02, 2024 at 05:25:10PM +0800, Jinjie Ruan wrote: > As ARM LPAE feature support accessing memory beyond the 4G limit, define > HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to support reserving crash > memory above 4G for ARM32 LPAE. > > No test because there is no LPAE ARM32 hardware. Why are you submitting patches for features you can't test? I'm not going to apply this without it being properly tested, because I don't believe that this will work in the generic case. If the crash kernel is located in memory outside of the lower 4GiB of address space, and there is no alias within physical address space for that memory, then there is *no* *way* for such a kernel to boot. So, right now I believe this patch to be *fundamentally* wrong.
On 2024/8/2 19:01, Russell King (Oracle) wrote: > On Fri, Aug 02, 2024 at 05:25:10PM +0800, Jinjie Ruan wrote: >> As ARM LPAE feature support accessing memory beyond the 4G limit, define >> HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to support reserving crash >> memory above 4G for ARM32 LPAE. >> >> No test because there is no LPAE ARM32 hardware. > > Why are you submitting patches for features you can't test? > > I'm not going to apply this without it being properly tested, because I > don't believe that this will work in the generic case. > > If the crash kernel is located in memory outside of the lower 4GiB of > address space, and there is no alias within physical address space > for that memory, then there is *no* *way* for such a kernel to boot. I'm sorry that I released this patch without testing it. I actually intended to bring up this issue for discussion. If anyone has the environment to test it, that would be great. In the meantime, we could have a discussion on the significance and relevance of this approach. > > So, right now I believe this patch to be *fundamentally* wrong. >
On 08/05/24 at 09:23am, Jinjie Ruan wrote: > > > On 2024/8/2 19:01, Russell King (Oracle) wrote: > > On Fri, Aug 02, 2024 at 05:25:10PM +0800, Jinjie Ruan wrote: > >> As ARM LPAE feature support accessing memory beyond the 4G limit, define > >> HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to support reserving crash > >> memory above 4G for ARM32 LPAE. > >> > >> No test because there is no LPAE ARM32 hardware. > > > > Why are you submitting patches for features you can't test? > > > > I'm not going to apply this without it being properly tested, because I > > don't believe that this will work in the generic case. > > > > If the crash kernel is located in memory outside of the lower 4GiB of > > address space, and there is no alias within physical address space > > for that memory, then there is *no* *way* for such a kernel to boot. > > I'm sorry that I released this patch without testing it. I actually > intended to bring up this issue for discussion. If anyone has the > environment to test it, that would be great. In the meantime, we could > have a discussion on the significance and relevance of this approach. I don't know arm32 and its LPAE. I know a little about x86_32 where crashkernel can only be reserved below 896M because of the virtual memory layout, and all memory above that is high memory which can't be used as kernel memory directly. So from this patch, arm32 is different than x86_32. > > > > > So, right now I believe this patch to be *fundamentally* wrong. > > >
On Fri, Aug 02, 2024 at 12:01:43PM +0100, Russell King wrote: > On Fri, Aug 02, 2024 at 05:25:10PM +0800, Jinjie Ruan wrote: > > As ARM LPAE feature support accessing memory beyond the 4G limit, define > > HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to support reserving crash At least in 6.11-rc1, there's no trace of such macro anywhere. So not sure this patch has any effect (I haven't checked linux-next though). > > memory above 4G for ARM32 LPAE. > > > > No test because there is no LPAE ARM32 hardware. > > Why are you submitting patches for features you can't test? > > I'm not going to apply this without it being properly tested, because I > don't believe that this will work in the generic case. > > If the crash kernel is located in memory outside of the lower 4GiB of > address space, and there is no alias within physical address space > for that memory, then there is *no* *way* for such a kernel to boot. > > So, right now I believe this patch to be *fundamentally* wrong. Indeed. Even on arm64, we keep some crashkernel reservations in the lower parts of the memory for ZONE_DMA allocations. On arch/arm with LPAE, we could do something similar like forcing some lowmem reservation and allowing explicit allocation in the higher ranges with crashkernel=<size>,high. We should, of course, force the kdump image placement in the lower memory. The user kexec tools must be taught to interpret this information and provide a DT accordingly to the crash kernel.
On 2024/8/5 21:18, Catalin Marinas wrote: > On Fri, Aug 02, 2024 at 12:01:43PM +0100, Russell King wrote: >> On Fri, Aug 02, 2024 at 05:25:10PM +0800, Jinjie Ruan wrote: >>> As ARM LPAE feature support accessing memory beyond the 4G limit, define >>> HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to support reserving crash > > At least in 6.11-rc1, there's no trace of such macro anywhere. So not > sure this patch has any effect (I haven't checked linux-next though). Sorry, this macro is introduced in linux-next, the -next subject has been missed. > >>> memory above 4G for ARM32 LPAE. >>> >>> No test because there is no LPAE ARM32 hardware. >> >> Why are you submitting patches for features you can't test? >> >> I'm not going to apply this without it being properly tested, because I >> don't believe that this will work in the generic case. >> >> If the crash kernel is located in memory outside of the lower 4GiB of >> address space, and there is no alias within physical address space >> for that memory, then there is *no* *way* for such a kernel to boot. >> >> So, right now I believe this patch to be *fundamentally* wrong. > > Indeed. Even on arm64, we keep some crashkernel reservations in the > lower parts of the memory for ZONE_DMA allocations. Indeed, it is. > > On arch/arm with LPAE, we could do something similar like forcing some > lowmem reservation and allowing explicit allocation in the higher ranges > with crashkernel=<size>,high. We should, of course, force the kdump In linux-next, with the HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro defined, it is ok. > image placement in the lower memory. The user kexec tools must be taught > to interpret this information and provide a DT accordingly to the crash > kernel. >
On 2024/8/5 10:56, Baoquan He wrote: > On 08/05/24 at 09:23am, Jinjie Ruan wrote: >> >> >> On 2024/8/2 19:01, Russell King (Oracle) wrote: >>> On Fri, Aug 02, 2024 at 05:25:10PM +0800, Jinjie Ruan wrote: >>>> As ARM LPAE feature support accessing memory beyond the 4G limit, define >>>> HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to support reserving crash >>>> memory above 4G for ARM32 LPAE. >>>> >>>> No test because there is no LPAE ARM32 hardware. >>> >>> Why are you submitting patches for features you can't test? >>> >>> I'm not going to apply this without it being properly tested, because I >>> don't believe that this will work in the generic case. >>> >>> If the crash kernel is located in memory outside of the lower 4GiB of >>> address space, and there is no alias within physical address space >>> for that memory, then there is *no* *way* for such a kernel to boot. >> >> I'm sorry that I released this patch without testing it. I actually >> intended to bring up this issue for discussion. If anyone has the >> environment to test it, that would be great. In the meantime, we could >> have a discussion on the significance and relevance of this approach. > > I don't know arm32 and its LPAE. I know a little about x86_32 where > crashkernel can only be reserved below 896M because of the virtual > memory layout, and all memory above that is high memory which can't be > used as kernel memory directly. So from this patch, arm32 is different > than x86_32. Hi，Baoquan Does the following code make sense? Now parse_crashkernel() use HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to parse "high", but use CONFIG_64BIT when reserving "low" memory in reserve_crashkernel_low(). And when LPAE is enabled in ARM32, and "high" is reserved, reserve_crashkernel_low() need also function ok. --- a/kernel/crash_reserve.c +++ b/kernel/crash_reserve.c @@ -354,7 +354,7 @@ early_param("crashkernel", parse_crashkernel_dummy); #ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION static int __init reserve_crashkernel_low(unsigned long long low_size) { -#ifdef CONFIG_64BIT +#ifdef HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH unsigned long long low_base; > >> >>> >>> So, right now I believe this patch to be *fundamentally* wrong. >>> >> > >
On 08/06/24 at 10:46am, Jinjie Ruan wrote: > > > On 2024/8/5 10:56, Baoquan He wrote: > > On 08/05/24 at 09:23am, Jinjie Ruan wrote: > >> > >> > >> On 2024/8/2 19:01, Russell King (Oracle) wrote: > >>> On Fri, Aug 02, 2024 at 05:25:10PM +0800, Jinjie Ruan wrote: > >>>> As ARM LPAE feature support accessing memory beyond the 4G limit, define > >>>> HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to support reserving crash > >>>> memory above 4G for ARM32 LPAE. > >>>> > >>>> No test because there is no LPAE ARM32 hardware. > >>> > >>> Why are you submitting patches for features you can't test? > >>> > >>> I'm not going to apply this without it being properly tested, because I > >>> don't believe that this will work in the generic case. > >>> > >>> If the crash kernel is located in memory outside of the lower 4GiB of > >>> address space, and there is no alias within physical address space > >>> for that memory, then there is *no* *way* for such a kernel to boot. > >> > >> I'm sorry that I released this patch without testing it. I actually > >> intended to bring up this issue for discussion. If anyone has the > >> environment to test it, that would be great. In the meantime, we could > >> have a discussion on the significance and relevance of this approach. > > > > I don't know arm32 and its LPAE. I know a little about x86_32 where > > crashkernel can only be reserved below 896M because of the virtual > > memory layout, and all memory above that is high memory which can't be > > used as kernel memory directly. So from this patch, arm32 is different > > than x86_32. > > Hi，Baoquan > > Does the following code make sense? Now parse_crashkernel() use > HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to parse "high", but use > CONFIG_64BIT when reserving "low" memory in reserve_crashkernel_low(). I am fine with it. BUT have you addressed Russell's concern, e.g how to test it actually? > > And when LPAE is enabled in ARM32, and "high" is reserved, > reserve_crashkernel_low() need also function ok. > > --- a/kernel/crash_reserve.c > +++ b/kernel/crash_reserve.c > @@ -354,7 +354,7 @@ early_param("crashkernel", parse_crashkernel_dummy); > #ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION > static int __init reserve_crashkernel_low(unsigned long long low_size) > { > -#ifdef CONFIG_64BIT > +#ifdef HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH > unsigned long long low_base; > > > > > >> > >>> > >>> So, right now I believe this patch to be *fundamentally* wrong. > >>> > >> > > > > >
On Tue, Aug 06, 2024 at 04:11:30PM +0800, Baoquan He wrote: > I am fine with it. BUT have you addressed Russell's concern, e.g how to > test it actually? Thanks for bringing that up. Let me reinforce my position on this. I will _not_ be accepting a patch that allows the crash kernel to be placed into high memory on 32-bit ARM unless it has been thoroughly tested to prove that it can actually work. Right now, I don't believe it can work as placing the kernel in highmem likely means it will be located *outside* of the lower 4GiB of physical memory which is all that will be accessible when the MMU is turned off. This is a pre-condition to boot a kernel - the kernel image _must_ be located within a region of memory which is exposed to the CPU when the MMU is turned off. Unless it can be proven that placing the kernel in highmem means that the kernel will be located in the lower 4GiB of physical memory space with the MMU off, then further work on this patch is a waste of time.
On 08/06/24 at 09:34am, Russell King (Oracle) wrote: > On Tue, Aug 06, 2024 at 04:11:30PM +0800, Baoquan He wrote: > > I am fine with it. BUT have you addressed Russell's concern, e.g how to > > test it actually? > > Thanks for bringing that up. > > Let me reinforce my position on this. I will _not_ be accepting a patch > that allows the crash kernel to be placed into high memory on 32-bit > ARM unless it has been thoroughly tested to prove that it can actually > work. > > Right now, I don't believe it can work as placing the kernel in highmem > likely means it will be located *outside* of the lower 4GiB of physical > memory which is all that will be accessible when the MMU is turned off. > This is a pre-condition to boot a kernel - the kernel image _must_ be > located within a region of memory which is exposed to the CPU when the > MMU is turned off. > > Unless it can be proven that placing the kernel in highmem means that > the kernel will be located in the lower 4GiB of physical memory space > with the MMU off, then further work on this patch is a waste of time. Yeah, totally agree.
On 2024/8/6 16:11, Baoquan He wrote: > On 08/06/24 at 10:46am, Jinjie Ruan wrote: >> >> >> On 2024/8/5 10:56, Baoquan He wrote: >>> On 08/05/24 at 09:23am, Jinjie Ruan wrote: >>>> >>>> >>>> On 2024/8/2 19:01, Russell King (Oracle) wrote: >>>>> On Fri, Aug 02, 2024 at 05:25:10PM +0800, Jinjie Ruan wrote: >>>>>> As ARM LPAE feature support accessing memory beyond the 4G limit, define >>>>>> HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to support reserving crash >>>>>> memory above 4G for ARM32 LPAE. >>>>>> >>>>>> No test because there is no LPAE ARM32 hardware. >>>>> >>>>> Why are you submitting patches for features you can't test? >>>>> >>>>> I'm not going to apply this without it being properly tested, because I >>>>> don't believe that this will work in the generic case. >>>>> >>>>> If the crash kernel is located in memory outside of the lower 4GiB of >>>>> address space, and there is no alias within physical address space >>>>> for that memory, then there is *no* *way* for such a kernel to boot. >>>> >>>> I'm sorry that I released this patch without testing it. I actually >>>> intended to bring up this issue for discussion. If anyone has the >>>> environment to test it, that would be great. In the meantime, we could >>>> have a discussion on the significance and relevance of this approach. >>> >>> I don't know arm32 and its LPAE. I know a little about x86_32 where >>> crashkernel can only be reserved below 896M because of the virtual >>> memory layout, and all memory above that is high memory which can't be >>> used as kernel memory directly. So from this patch, arm32 is different >>> than x86_32. >> >> Hi，Baoquan >> >> Does the following code make sense? Now parse_crashkernel() use >> HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to parse "high", but use >> CONFIG_64BIT when reserving "low" memory in reserve_crashkernel_low(). > > I am fine with it. BUT have you addressed Russell's concern, e.g how to > test it actually? Thank you! Let me find the test environment. > >> >> And when LPAE is enabled in ARM32, and "high" is reserved, >> reserve_crashkernel_low() need also function ok. >> >> --- a/kernel/crash_reserve.c >> +++ b/kernel/crash_reserve.c >> @@ -354,7 +354,7 @@ early_param("crashkernel", parse_crashkernel_dummy); >> #ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION >> static int __init reserve_crashkernel_low(unsigned long long low_size) >> { >> -#ifdef CONFIG_64BIT >> +#ifdef HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH >> unsigned long long low_base; >> >> >>> >>>> >>>>> >>>>> So, right now I believe this patch to be *fundamentally* wrong. >>>>> >>>> >>> >>> >> > >
On 2024/8/6 16:34, Russell King (Oracle) wrote: > On Tue, Aug 06, 2024 at 04:11:30PM +0800, Baoquan He wrote: >> I am fine with it. BUT have you addressed Russell's concern, e.g how to >> test it actually? > > Thanks for bringing that up. > > Let me reinforce my position on this. I will _not_ be accepting a patch > that allows the crash kernel to be placed into high memory on 32-bit > ARM unless it has been thoroughly tested to prove that it can actually > work. Thank you! I'm looking for an available environment to test, otherwise, the patch is invalid. > > Right now, I don't believe it can work as placing the kernel in highmem > likely means it will be located *outside* of the lower 4GiB of physical > memory which is all that will be accessible when the MMU is turned off. > This is a pre-condition to boot a kernel - the kernel image _must_ be > located within a region of memory which is exposed to the CPU when the > MMU is turned off. > > Unless it can be proven that placing the kernel in highmem means that > the kernel will be located in the lower 4GiB of physical memory space > with the MMU off, then further work on this patch is a waste of time. >
diff --git a/arch/arm/include/asm/crash_reserve.h b/arch/arm/include/asm/crash_reserve.h index 85c9298bd3b7..33a2f18b0ec1 100644 --- a/arch/arm/include/asm/crash_reserve.h +++ b/arch/arm/include/asm/crash_reserve.h @@ -19,6 +19,10 @@ static inline unsigned long crash_addr_low_max(void) return (crash_max > lowmem_max) ? lowmem_max : crash_max; } - #define HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY + +#if defined(CONFIG_ARM_LPAE) && defined(CONFIG_HIGHMEM) +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH +#endif + #endif
As ARM LPAE feature support accessing memory beyond the 4G limit, define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH macro to support reserving crash memory above 4G for ARM32 LPAE. No test because there is no LPAE ARM32 hardware. Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- arch/arm/include/asm/crash_reserve.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)