On 2024/7/16 14:22, Baoquan He wrote: > On 07/16/24 at 11:44am, Jinjie Ruan wrote: >> >> >> On 2024/7/15 22:48, Baoquan He wrote: >>> On 07/13/24 at 09:48am, Jinjie Ruan wrote: >>>> On x86_32 Qemu machine with 1GB memory, the cmdline "crashkernel=4G" is ok >>>> as below: >>>> crashkernel reserved: 0x0000000020000000 - 0x0000000120000000 (4096 MB) >>>> >>>> And on Qemu vexpress-a9 with 1GB memory, the crash kernel "crashkernel=4G" >>>> is also ok as below: >>>> Reserving 4096MB of memory at 2432MB for crashkernel (System RAM: 1024MB) >>>> >>>> The cause is that the crash_size is parsed and printed with "unsigned long >>>> long" data type which is 8 bytes but allocated used with "phys_addr_t" >>>> which is 4 bytes in memblock_phys_alloc_range(). >>>> >>>> Fix it by limiting the "crash_size" to phys_addr_t and bypass the invalid >>>> input size. >>> >>> I am not sure if this is a good idea. Shouldn't we handle this in >>> arch_reserve_crashkernel() to check the system RAM size? >>> >>> With this patch, if you specify crashkernel=4352M (namely 4G+256M) in >>> kernel cmdline, then you will reserve 256M crashkernel in system, don't >>> you think that is confusing? >> >> You are right! >> >> In the case you mentioned, it can still allocate 256M successfully, but >> the log shows 4352M successfully allocated, which is not taken into >> account by this patch. >> >> And handle this in arch_reserve_crashkernel() is a good idea, which will >> bypass all these corner case, I'll do it next version. >> >>> >>> By the way, I am considering changing code to apply generic crashkernel >>> reservation to 32bit system. Maybe below draft code can prevent >>> crashkernel=,high from being parsed successfully on 32bit system. >>> >>> What do you think? >> >> I agree with you, I've thought about passing in a parameter in the >> generic interface whether high is supported or not to implement it, >> which is so incompatible. An architecture-defined macro to filter out >> parsing of "high" fundamentally avoid using the generic interface to >> allocate memory in "high" for the architecture that does not support >> "high". The below code do prevent "crashkernel=,high" from being parsed >> successfully on 32bit system. >> >> But if it is to support 32 bit system to use generic crash memory >> reserve interface, reserve_crashkernel_generic() needs more modification >> , as it may try to allocate memory at high. > > You are right. Below change may be able to fix that. > > And I have been thinking if one case need be taken off in which the > first attempt was for high memory, then fall back to low memory. Surely, > this is not related to the 32bit crashkernel reservation. It seems that ARM64 has the possibility before the refactoring. However, x86 supports only the "low" -> "high" retry but not the "high" -> "low" retry before the refactoring. In my opinion, "low" -> "high" retry is more usefull, but I'm not sure if we should get rid of the other way. > > By the way, do you figure out these issues from code reading and qemu > testing, or there's need for deploying kdump on 32bit system, e.g i386 > or arm32? Just curious. I found these problems during testing on QEMU when trying to support this generic crash memory retention on ARM32, and I further found that x86_32 also has the same problem by code reading and uncommon configuration test. > > diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c > index 5b2722a93a48..ac087ba442cd 100644 > --- a/kernel/crash_reserve.c > +++ b/kernel/crash_reserve.c > @@ -414,7 +414,8 @@ void __init reserve_crashkernel_generic(char *cmdline, > search_end = CRASH_ADDR_HIGH_MAX; > search_base = CRASH_ADDR_LOW_MAX; > crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE; > - goto retry; > + if (search_base != search_end) > + goto retry; > } > > /* > > _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec
