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

[Jinjie Ruan]

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.

> 
> diff --git a/arch/arm64/include/asm/crash_reserve.h b/arch/arm64/include/asm/crash_reserve.h
> index 4afe027a4e7b..bf362c1a612f 100644
> --- a/arch/arm64/include/asm/crash_reserve.h
> +++ b/arch/arm64/include/asm/crash_reserve.h
> @@ -7,4 +7,6 @@
>  
>  #define CRASH_ADDR_LOW_MAX              arm64_dma_phys_limit
>  #define CRASH_ADDR_HIGH_MAX             (PHYS_MASK + 1)
> +
> +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
>  #endif
> diff --git a/arch/riscv/include/asm/crash_reserve.h b/arch/riscv/include/asm/crash_reserve.h
> index 013962e63587..8d7a8fc1d459 100644
> --- a/arch/riscv/include/asm/crash_reserve.h
> +++ b/arch/riscv/include/asm/crash_reserve.h
> @@ -7,5 +7,7 @@
>  #define CRASH_ADDR_LOW_MAX		dma32_phys_limit
>  #define CRASH_ADDR_HIGH_MAX		memblock_end_of_DRAM()
>  
> +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
> +
>  extern phys_addr_t memblock_end_of_DRAM(void);
>  #endif
> diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
> index 7835b2cdff04..24c2327f9a16 100644
> --- a/arch/x86/include/asm/crash_reserve.h
> +++ b/arch/x86/include/asm/crash_reserve.h
> @@ -26,6 +26,7 @@ extern unsigned long swiotlb_size_or_default(void);
>  #else
>  # define CRASH_ADDR_LOW_MAX     SZ_4G
>  # define CRASH_ADDR_HIGH_MAX    SZ_64T
> +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
>  #endif
>  
>  # define DEFAULT_CRASH_KERNEL_LOW_SIZE crash_low_size_default()
> diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
> index 5b2722a93a48..c5213f123e19 100644
> --- a/kernel/crash_reserve.c
> +++ b/kernel/crash_reserve.c
> @@ -306,7 +306,7 @@ int __init parse_crashkernel(char *cmdline,
>  	/* crashkernel=X[@offset] */
>  	ret = __parse_crashkernel(cmdline, system_ram, crash_size,
>  				crash_base, NULL);
> -#ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
> +#ifdef HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
>  	/*
>  	 * If non-NULL 'high' passed in and no normal crashkernel
>  	 * setting detected, try parsing crashkernel=,high|low.
> 
>