Re: [PATCH v2] crash: Fix x86_32 and arm32 memory reserve bug

[Jinjie Ruan <ruanjinjie@huawei.com>]

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;
>  		}
>  
>  		/*
> 
>