[PATCH v3 0/3] crash: Fix x86_32 memory reserve dead loop bug

[PATCH v3 0/3] crash: Fix x86_32 memory reserve dead loop bug

[Jinjie Ruan]

Fix two bugs for x86_32 crash memory reserve, and prepare to apply generic
crashkernel reservation to 32bit system. Then use generic interface to
simplify crashkernel reservation for ARM32.

changes in v3:
- Fix it as Baoquan suggested.
- Update the commit message.

changes in v2:
- Peel off the other two patches.
- Update the commit message and fix tag.

Jinjie Ruan (3):
  crash: Fix x86_32 crash memory reserve dead loop bug
  crash: Fix x86_32 crash memory reserve dead loop bug at high
  ARM: Use generic interface to simplify crashkernel reservation

 arch/arm/Kconfig                       |  3 ++
 arch/arm/include/asm/crash_reserve.h   | 24 ++++++++++
 arch/arm/kernel/setup.c                | 63 ++++----------------------
 arch/arm64/include/asm/crash_reserve.h |  2 +
 arch/riscv/include/asm/crash_reserve.h |  2 +
 arch/x86/include/asm/crash_reserve.h   |  1 +
 kernel/crash_reserve.c                 |  5 +-
 7 files changed, 44 insertions(+), 56 deletions(-)
 create mode 100644 arch/arm/include/asm/crash_reserve.h

-- 
2.34.1

[PATCH v3 1/3] crash: Fix x86_32 crash memory reserve dead loop bug

[Jinjie Ruan]

On x86_32 Qemu machine with 1GB memory, the cmdline "crashkernel=1G,high"
will cause system stall as below:

	ACPI: Reserving FACP table memory at [mem 0x3ffe18b8-0x3ffe192b]
	ACPI: Reserving DSDT table memory at [mem 0x3ffe0040-0x3ffe18b7]
	ACPI: Reserving FACS table memory at [mem 0x3ffe0000-0x3ffe003f]
	ACPI: Reserving APIC table memory at [mem 0x3ffe192c-0x3ffe19bb]
	ACPI: Reserving HPET table memory at [mem 0x3ffe19bc-0x3ffe19f3]
	ACPI: Reserving WAET table memory at [mem 0x3ffe19f4-0x3ffe1a1b]
	143MB HIGHMEM available.
	879MB LOWMEM available.
	  mapped low ram: 0 - 36ffe000
	  low ram: 0 - 36ffe000
	 (stall here)

The reason is that the CRASH_ADDR_LOW_MAX is equal to CRASH_ADDR_HIGH_MAX
on x86_32, the first high crash kernel memory reservation will fail, then
go into the "retry" loop and never came out as below.

-> reserve_crashkernel_generic() and high is true
 -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail
    -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly
       (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX).

Fix it by prevent crashkernel=,high from being parsed successfully on 32bit
system with a architecture-defined macro.

After this patch, the 'crashkernel=,high' for 32bit system can't succeed,
and it has no chance to call reserve_crashkernel_generic(), therefore this
issue on x86_32 is solved.

Fixes: 9c08a2a139fe ("x86: kdump: use generic interface to simplify crashkernel reservation code")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Signed-off-by: Baoquan He <bhe@redhat.com>
Tested-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v3:
- Fix it as Baoquan suggested.
- Update the commit message.
v2:
- Peel off the other two patches.
- Update the commit message and fix tag.
---
 arch/arm64/include/asm/crash_reserve.h | 2 ++
 arch/riscv/include/asm/crash_reserve.h | 2 ++
 arch/x86/include/asm/crash_reserve.h   | 1 +
 kernel/crash_reserve.c                 | 2 +-
 4 files changed, 6 insertions(+), 1 deletion(-)

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.
-- 
2.34.1

Re: [PATCH v3 1/3] crash: Fix x86_32 crash memory reserve dead loop bug

[Baoquan He]

On 07/18/24 at 11:54am, Jinjie Ruan wrote:
> On x86_32 Qemu machine with 1GB memory, the cmdline "crashkernel=1G,high"
> will cause system stall as below:
> 
> 	ACPI: Reserving FACP table memory at [mem 0x3ffe18b8-0x3ffe192b]
> 	ACPI: Reserving DSDT table memory at [mem 0x3ffe0040-0x3ffe18b7]
> 	ACPI: Reserving FACS table memory at [mem 0x3ffe0000-0x3ffe003f]
> 	ACPI: Reserving APIC table memory at [mem 0x3ffe192c-0x3ffe19bb]
> 	ACPI: Reserving HPET table memory at [mem 0x3ffe19bc-0x3ffe19f3]
> 	ACPI: Reserving WAET table memory at [mem 0x3ffe19f4-0x3ffe1a1b]
> 	143MB HIGHMEM available.
> 	879MB LOWMEM available.
> 	  mapped low ram: 0 - 36ffe000
> 	  low ram: 0 - 36ffe000
> 	 (stall here)
> 
> The reason is that the CRASH_ADDR_LOW_MAX is equal to CRASH_ADDR_HIGH_MAX
> on x86_32, the first high crash kernel memory reservation will fail, then
> go into the "retry" loop and never came out as below.
> 
> -> reserve_crashkernel_generic() and high is true
>  -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail
>     -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly
>        (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX).
> 
> Fix it by prevent crashkernel=,high from being parsed successfully on 32bit
> system with a architecture-defined macro.
> 
> After this patch, the 'crashkernel=,high' for 32bit system can't succeed,
> and it has no chance to call reserve_crashkernel_generic(), therefore this
> issue on x86_32 is solved.
> 
> Fixes: 9c08a2a139fe ("x86: kdump: use generic interface to simplify crashkernel reservation code")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Signed-off-by: Baoquan He <bhe@redhat.com>

Just adding my Suggested-by is fine. If multiple people cooperate on one
patch, the Co-developed-by tag is needed. As a maintainer, I prefer to
have the Suggested-by tag in this case.

> Tested-by: Jinjie Ruan <ruanjinjie@huawei.com>

You can't add Tested-by tag for your own patch. When you post patch,
testing it is your obligation.

Other than these tag adding concerns, this patch looks good to me. You
can post v4 to update and add my:

Acked-by: Baoquan He <bhe@redhat.com>

> ---
> v3:
> - Fix it as Baoquan suggested.
> - Update the commit message.
> v2:
> - Peel off the other two patches.
> - Update the commit message and fix tag.
> ---
>  arch/arm64/include/asm/crash_reserve.h | 2 ++
>  arch/riscv/include/asm/crash_reserve.h | 2 ++
>  arch/x86/include/asm/crash_reserve.h   | 1 +
>  kernel/crash_reserve.c                 | 2 +-
>  4 files changed, 6 insertions(+), 1 deletion(-)
> 
> 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.
> -- 
> 2.34.1
> 

Re: [PATCH v3 1/3] crash: Fix x86_32 crash memory reserve dead loop bug

[Jinjie Ruan]

On 2024/7/18 19:10, Baoquan He wrote:
> On 07/18/24 at 11:54am, Jinjie Ruan wrote:
>> On x86_32 Qemu machine with 1GB memory, the cmdline "crashkernel=1G,high"
>> will cause system stall as below:
>>
>> 	ACPI: Reserving FACP table memory at [mem 0x3ffe18b8-0x3ffe192b]
>> 	ACPI: Reserving DSDT table memory at [mem 0x3ffe0040-0x3ffe18b7]
>> 	ACPI: Reserving FACS table memory at [mem 0x3ffe0000-0x3ffe003f]
>> 	ACPI: Reserving APIC table memory at [mem 0x3ffe192c-0x3ffe19bb]
>> 	ACPI: Reserving HPET table memory at [mem 0x3ffe19bc-0x3ffe19f3]
>> 	ACPI: Reserving WAET table memory at [mem 0x3ffe19f4-0x3ffe1a1b]
>> 	143MB HIGHMEM available.
>> 	879MB LOWMEM available.
>> 	  mapped low ram: 0 - 36ffe000
>> 	  low ram: 0 - 36ffe000
>> 	 (stall here)
>>
>> The reason is that the CRASH_ADDR_LOW_MAX is equal to CRASH_ADDR_HIGH_MAX
>> on x86_32, the first high crash kernel memory reservation will fail, then
>> go into the "retry" loop and never came out as below.
>>
>> -> reserve_crashkernel_generic() and high is true
>>  -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail
>>     -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly
>>        (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX).
>>
>> Fix it by prevent crashkernel=,high from being parsed successfully on 32bit
>> system with a architecture-defined macro.
>>
>> After this patch, the 'crashkernel=,high' for 32bit system can't succeed,
>> and it has no chance to call reserve_crashkernel_generic(), therefore this
>> issue on x86_32 is solved.
>>
>> Fixes: 9c08a2a139fe ("x86: kdump: use generic interface to simplify crashkernel reservation code")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> Signed-off-by: Baoquan He <bhe@redhat.com>
> 
> Just adding my Suggested-by is fine. If multiple people cooperate on one
> patch, the Co-developed-by tag is needed. As a maintainer, I prefer to
> have the Suggested-by tag in this case.

OK, thank you very much!

> 
>> Tested-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 
> You can't add Tested-by tag for your own patch. When you post patch,
> testing it is your obligation.
> 
> Other than these tag adding concerns, this patch looks good to me. You
> can post v4 to update and add my:

Thank you, I'll fix it next version.

> 
> Acked-by: Baoquan He <bhe@redhat.com>>
>> ---
>> v3:
>> - Fix it as Baoquan suggested.
>> - Update the commit message.
>> v2:
>> - Peel off the other two patches.
>> - Update the commit message and fix tag.
>> ---
>>  arch/arm64/include/asm/crash_reserve.h | 2 ++
>>  arch/riscv/include/asm/crash_reserve.h | 2 ++
>>  arch/x86/include/asm/crash_reserve.h   | 1 +
>>  kernel/crash_reserve.c                 | 2 +-
>>  4 files changed, 6 insertions(+), 1 deletion(-)
>>
>> 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.
>> -- 
>> 2.34.1
>>
> 
> 

Re: [PATCH v3 1/3] crash: Fix x86_32 crash memory reserve dead loop bug

[Jinjie Ruan]

On 2024/7/18 19:10, Baoquan He wrote:
> On 07/18/24 at 11:54am, Jinjie Ruan wrote:
>> On x86_32 Qemu machine with 1GB memory, the cmdline "crashkernel=1G,high"
>> will cause system stall as below:
>>
>> 	ACPI: Reserving FACP table memory at [mem 0x3ffe18b8-0x3ffe192b]
>> 	ACPI: Reserving DSDT table memory at [mem 0x3ffe0040-0x3ffe18b7]
>> 	ACPI: Reserving FACS table memory at [mem 0x3ffe0000-0x3ffe003f]
>> 	ACPI: Reserving APIC table memory at [mem 0x3ffe192c-0x3ffe19bb]
>> 	ACPI: Reserving HPET table memory at [mem 0x3ffe19bc-0x3ffe19f3]
>> 	ACPI: Reserving WAET table memory at [mem 0x3ffe19f4-0x3ffe1a1b]
>> 	143MB HIGHMEM available.
>> 	879MB LOWMEM available.
>> 	  mapped low ram: 0 - 36ffe000
>> 	  low ram: 0 - 36ffe000
>> 	 (stall here)
>>
>> The reason is that the CRASH_ADDR_LOW_MAX is equal to CRASH_ADDR_HIGH_MAX
>> on x86_32, the first high crash kernel memory reservation will fail, then
>> go into the "retry" loop and never came out as below.
>>
>> -> reserve_crashkernel_generic() and high is true
>>  -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail
>>     -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly
>>        (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX).
>>
>> Fix it by prevent crashkernel=,high from being parsed successfully on 32bit
>> system with a architecture-defined macro.
>>
>> After this patch, the 'crashkernel=,high' for 32bit system can't succeed,
>> and it has no chance to call reserve_crashkernel_generic(), therefore this
>> issue on x86_32 is solved.
>>
>> Fixes: 9c08a2a139fe ("x86: kdump: use generic interface to simplify crashkernel reservation code")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> Signed-off-by: Baoquan He <bhe@redhat.com>
> 
> Just adding my Suggested-by is fine. If multiple people cooperate on one
> patch, the Co-developed-by tag is needed. As a maintainer, I prefer to
> have the Suggested-by tag in this case.

Hi, Baoquan, I wonder if riscv32 have a similar problem, but I'm not sure.

> 
>> Tested-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 
> You can't add Tested-by tag for your own patch. When you post patch,
> testing it is your obligation.
> 
> Other than these tag adding concerns, this patch looks good to me. You
> can post v4 to update and add my:
> 
> Acked-by: Baoquan He <bhe@redhat.com>
> 
>> ---
>> v3:
>> - Fix it as Baoquan suggested.
>> - Update the commit message.
>> v2:
>> - Peel off the other two patches.
>> - Update the commit message and fix tag.
>> ---
>>  arch/arm64/include/asm/crash_reserve.h | 2 ++
>>  arch/riscv/include/asm/crash_reserve.h | 2 ++
>>  arch/x86/include/asm/crash_reserve.h   | 1 +
>>  kernel/crash_reserve.c                 | 2 +-
>>  4 files changed, 6 insertions(+), 1 deletion(-)
>>
>> 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.
>> -- 
>> 2.34.1
>>
> 
> 

[PATCH v3 2/3] crash: Fix x86_32 crash memory reserve dead loop bug at high

[Jinjie Ruan]

On x86_32 Qemu machine with 1GB memory, the cmdline "crashkernel=512M" will
also cause system stall as below:

	ACPI: Reserving FACP table memory at [mem 0x3ffe18b8-0x3ffe192b]
	ACPI: Reserving DSDT table memory at [mem 0x3ffe0040-0x3ffe18b7]
	ACPI: Reserving FACS table memory at [mem 0x3ffe0000-0x3ffe003f]
	ACPI: Reserving APIC table memory at [mem 0x3ffe192c-0x3ffe19bb]
	ACPI: Reserving HPET table memory at [mem 0x3ffe19bc-0x3ffe19f3]
	ACPI: Reserving WAET table memory at [mem 0x3ffe19f4-0x3ffe1a1b]
	143MB HIGHMEM available.
	879MB LOWMEM available.
	  mapped low ram: 0 - 36ffe000
	  low ram: 0 - 36ffe000
	  (stall here)

The reason is that the CRASH_ADDR_LOW_MAX is equal to CRASH_ADDR_HIGH_MAX
on x86_32, the first "low" crash kernel memory reservation for 512M fails,
then it go into the "retry" loop and never came out as below (consider
CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX = 512M):

-> reserve_crashkernel_generic() and high is false
   -> alloc at [0, 0x20000000] fail
      -> alloc at [0x20000000, 0x20000000] fail and repeatedly
      (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX).

Fix it by skipping meaningless calls of memblock_phys_alloc_range() with
`start = end`

After this patch, the retry dead loop is avoided and print below info:
	cannot allocate crashkernel (size:0x20000000)

And apply generic crashkernel reservation to 32bit system will be ready.

Fixes: 9c08a2a139fe ("x86: kdump: use generic interface to simplify crashkernel reservation code")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Signed-off-by: Baoquan He <bhe@redhat.com>
Tested-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v3:
- Fix it as Baoquan suggested.
- Update the commit message.
---
 kernel/crash_reserve.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
index c5213f123e19..dacc268429e2 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;
 		}
 
 		/*
-- 
2.34.1

Re: [PATCH v3 2/3] crash: Fix x86_32 crash memory reserve dead loop bug at high

[Baoquan He]

On 07/18/24 at 11:54am, Jinjie Ruan wrote:

I don't fully catch the subject, what does the 'dead loop bug at high'
mean?

> On x86_32 Qemu machine with 1GB memory, the cmdline "crashkernel=512M" will
> also cause system stall as below:
> 
> 	ACPI: Reserving FACP table memory at [mem 0x3ffe18b8-0x3ffe192b]
> 	ACPI: Reserving DSDT table memory at [mem 0x3ffe0040-0x3ffe18b7]
> 	ACPI: Reserving FACS table memory at [mem 0x3ffe0000-0x3ffe003f]
> 	ACPI: Reserving APIC table memory at [mem 0x3ffe192c-0x3ffe19bb]
> 	ACPI: Reserving HPET table memory at [mem 0x3ffe19bc-0x3ffe19f3]
> 	ACPI: Reserving WAET table memory at [mem 0x3ffe19f4-0x3ffe1a1b]
> 	143MB HIGHMEM available.
> 	879MB LOWMEM available.
> 	  mapped low ram: 0 - 36ffe000
> 	  low ram: 0 - 36ffe000
> 	  (stall here)
> 
> The reason is that the CRASH_ADDR_LOW_MAX is equal to CRASH_ADDR_HIGH_MAX
> on x86_32, the first "low" crash kernel memory reservation for 512M fails,
> then it go into the "retry" loop and never came out as below (consider
> CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX = 512M):
> 
> -> reserve_crashkernel_generic() and high is false
>    -> alloc at [0, 0x20000000] fail
>       -> alloc at [0x20000000, 0x20000000] fail and repeatedly
>       (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX).
> 
> Fix it by skipping meaningless calls of memblock_phys_alloc_range() with
> `start = end`
> 
> After this patch, the retry dead loop is avoided and print below info:
> 	cannot allocate crashkernel (size:0x20000000)
> 
> And apply generic crashkernel reservation to 32bit system will be ready.
> 
> Fixes: 9c08a2a139fe ("x86: kdump: use generic interface to simplify crashkernel reservation code")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Tested-by: Jinjie Ruan <ruanjinjie@huawei.com>

Also the tag issues, please update.

Other than above concerns, the patch looks good to me.

> ---
> v3:
> - Fix it as Baoquan suggested.
> - Update the commit message.
> ---
>  kernel/crash_reserve.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
> index c5213f123e19..dacc268429e2 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;
>  		}
>  
>  		/*
> -- 
> 2.34.1
> 

Re: [PATCH v3 2/3] crash: Fix x86_32 crash memory reserve dead loop bug at high

[Jinjie Ruan]

On 2024/7/18 19:14, Baoquan He wrote:
> On 07/18/24 at 11:54am, Jinjie Ruan wrote:
> 
> I don't fully catch the subject, what does the 'dead loop bug at high'
> mean?

It means alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] repeatedly
which corresponds to the crashkernel parameter of the "high".

> 
>> On x86_32 Qemu machine with 1GB memory, the cmdline "crashkernel=512M" will
>> also cause system stall as below:
>>
>> 	ACPI: Reserving FACP table memory at [mem 0x3ffe18b8-0x3ffe192b]
>> 	ACPI: Reserving DSDT table memory at [mem 0x3ffe0040-0x3ffe18b7]
>> 	ACPI: Reserving FACS table memory at [mem 0x3ffe0000-0x3ffe003f]
>> 	ACPI: Reserving APIC table memory at [mem 0x3ffe192c-0x3ffe19bb]
>> 	ACPI: Reserving HPET table memory at [mem 0x3ffe19bc-0x3ffe19f3]
>> 	ACPI: Reserving WAET table memory at [mem 0x3ffe19f4-0x3ffe1a1b]
>> 	143MB HIGHMEM available.
>> 	879MB LOWMEM available.
>> 	  mapped low ram: 0 - 36ffe000
>> 	  low ram: 0 - 36ffe000
>> 	  (stall here)
>>
>> The reason is that the CRASH_ADDR_LOW_MAX is equal to CRASH_ADDR_HIGH_MAX
>> on x86_32, the first "low" crash kernel memory reservation for 512M fails,
>> then it go into the "retry" loop and never came out as below (consider
>> CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX = 512M):
>>
>> -> reserve_crashkernel_generic() and high is false
>>    -> alloc at [0, 0x20000000] fail
>>       -> alloc at [0x20000000, 0x20000000] fail and repeatedly
>>       (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX).
>>
>> Fix it by skipping meaningless calls of memblock_phys_alloc_range() with
>> `start = end`
>>
>> After this patch, the retry dead loop is avoided and print below info:
>> 	cannot allocate crashkernel (size:0x20000000)
>>
>> And apply generic crashkernel reservation to 32bit system will be ready.
>>
>> Fixes: 9c08a2a139fe ("x86: kdump: use generic interface to simplify crashkernel reservation code")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> Signed-off-by: Baoquan He <bhe@redhat.com>
>> Tested-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 
> Also the tag issues, please update.
> 
> Other than above concerns, the patch looks good to me.

Thank you for your review, I'll fix it.

> 
>> ---
>> v3:
>> - Fix it as Baoquan suggested.
>> - Update the commit message.
>> ---
>>  kernel/crash_reserve.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
>> index c5213f123e19..dacc268429e2 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;
>>  		}
>>  
>>  		/*
>> -- 
>> 2.34.1
>>
> 
> 

Re: [PATCH v3 2/3] crash: Fix x86_32 crash memory reserve dead loop bug at high

[Baoquan He]

On 07/18/24 at 08:10pm, Jinjie Ruan wrote:
> 
> 
> On 2024/7/18 19:14, Baoquan He wrote:
> > On 07/18/24 at 11:54am, Jinjie Ruan wrote:
> > 
> > I don't fully catch the subject, what does the 'dead loop bug at high'
> > mean?
> 
> It means alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] repeatedly
> which corresponds to the crashkernel parameter of the "high".

That may mislead people to think it's a crashkernel=,high setting and
the corresponding issue.

Maybe "crash: Fix x86_32 crashkernel reservation dead loop" is good
enough.

[PATCH v3 3/3] ARM: Use generic interface to simplify crashkernel reservation

[Jinjie Ruan]

Currently, x86, arm64, riscv and loongarch has been switched to generic
crashkernel reservation, which is also ready for 32bit system.
So with the help of function parse_crashkernel() and generic
reserve_crashkernel_generic(), arm32 crashkernel reservation can also
be simplified by steps:

1) Add a new header file <asm/crash_reserve.h>, and define CRASH_ALIGN,
   CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX in it;

2) Add arch_reserve_crashkernel() to call parse_crashkernel() and
   reserve_crashkernel_generic();

3) Add ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION Kconfig in
   arch/arm/Kconfig.

The old reserve_crashkernel() can be removed.

Following test cases have been performed as expected on QEMU vexpress-a9
(1GB system memory):

1) crashkernel=4G,high				// invalid
2) crashkernel=1G,high				// invalid
3) crashkernel=1G,high crashkernel=0M,low	// invalid
4) crashkernel=256M,high			// invalid
5) crashkernel=256M,low				// invalid
6) crashkernel=256M crashkernel=256M,high	// high is ignored, ok
7) crashkernel=256M crashkernel=256M,low	// low is ignored, ok
8) crashkernel=256M,high crashkernel=256M,low	// invalid
9) crashkernel=256M,high crashkernel=4G,low	// invalid
10) crashkernel=256M				// ok
11) crashkernel=512M				// ok
12) crashkernel=256M@0x88000000			// ok
13) crashkernel=256M@0x78000000			// ok
14) crashkernel=512M@0x78000000			// ok

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Tested-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v3:
- Update the commit message.
---
 arch/arm/Kconfig                     |  3 ++
 arch/arm/include/asm/crash_reserve.h | 24 +++++++++++
 arch/arm/kernel/setup.c              | 63 ++++------------------------
 3 files changed, 36 insertions(+), 54 deletions(-)
 create mode 100644 arch/arm/include/asm/crash_reserve.h

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index f434afff4a2c..3f198ae63950 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1597,6 +1597,9 @@ config ATAGS_PROC
 config ARCH_SUPPORTS_CRASH_DUMP
 	def_bool y
 
+config ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
+	def_bool CRASH_RESERVE
+
 config AUTO_ZRELADDR
 	bool "Auto calculation of the decompressed kernel image address" if !ARCH_MULTIPLATFORM
 	default !(ARCH_FOOTBRIDGE || ARCH_RPC || ARCH_SA1100)
diff --git a/arch/arm/include/asm/crash_reserve.h b/arch/arm/include/asm/crash_reserve.h
new file mode 100644
index 000000000000..85c9298bd3b7
--- /dev/null
+++ b/arch/arm/include/asm/crash_reserve.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _ARM_CRASH_RESERVE_H
+#define _ARM_CRASH_RESERVE_H
+
+/*
+ * The crash region must be aligned to 128MB to avoid
+ * zImage relocating below the reserved region.
+ */
+#define CRASH_ALIGN			(128 << 20)
+
+#define CRASH_ADDR_LOW_MAX		crash_addr_low_max()
+#define CRASH_ADDR_HIGH_MAX		memblock_end_of_DRAM()
+
+static inline unsigned long crash_addr_low_max(void)
+{
+	unsigned long long crash_max = idmap_to_phys((u32)~0);
+	unsigned long long lowmem_max = __pa(high_memory - 1) + 1;
+
+	return (crash_max > lowmem_max) ? lowmem_max : crash_max;
+}
+
+
+#define HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
+#endif
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index e6a857bf0ce6..fc0ada003f6d 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -979,13 +979,6 @@ static int __init init_machine_late(void)
 }
 late_initcall(init_machine_late);
 
-#ifdef CONFIG_CRASH_RESERVE
-/*
- * The crash region must be aligned to 128MB to avoid
- * zImage relocating below the reserved region.
- */
-#define CRASH_ALIGN	(128 << 20)
-
 static inline unsigned long long get_total_mem(void)
 {
 	unsigned long total;
@@ -994,60 +987,25 @@ static inline unsigned long long get_total_mem(void)
 	return total << PAGE_SHIFT;
 }
 
-/**
- * reserve_crashkernel() - reserves memory are for crash kernel
- *
- * This function reserves memory area given in "crashkernel=" kernel command
- * line parameter. The memory reserved is used by a dump capture kernel when
- * primary kernel is crashing.
- */
-static void __init reserve_crashkernel(void)
+static void __init arch_reserve_crashkernel(void)
 {
-	unsigned long long crash_size, crash_base;
+	unsigned long long crash_size, crash_base, low_size = 0;
 	unsigned long long total_mem;
+	bool high = false;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_CRASH_RESERVE))
+		return;
+
 	total_mem = get_total_mem();
 	ret = parse_crashkernel(boot_command_line, total_mem,
 				&crash_size, &crash_base,
-				NULL, NULL);
+				&low_size, &high);
 	/* invalid value specified or crashkernel=0 */
 	if (ret || !crash_size)
 		return;
 
-	if (crash_base <= 0) {
-		unsigned long long crash_max = idmap_to_phys((u32)~0);
-		unsigned long long lowmem_max = __pa(high_memory - 1) + 1;
-		if (crash_max > lowmem_max)
-			crash_max = lowmem_max;
-
-		crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
-						       CRASH_ALIGN, crash_max);
-		if (!crash_base) {
-			pr_err("crashkernel reservation failed - No suitable area found.\n");
-			return;
-		}
-	} else {
-		unsigned long long crash_max = crash_base + crash_size;
-		unsigned long long start;
-
-		start = memblock_phys_alloc_range(crash_size, SECTION_SIZE,
-						  crash_base, crash_max);
-		if (!start) {
-			pr_err("crashkernel reservation failed - memory is in use.\n");
-			return;
-		}
-	}
-
-	pr_info("Reserving %ldMB of memory at %ldMB for crashkernel (System RAM: %ldMB)\n",
-		(unsigned long)(crash_size >> 20),
-		(unsigned long)(crash_base >> 20),
-		(unsigned long)(total_mem >> 20));
-
-	/* The crashk resource must always be located in normal mem */
-	crashk_res.start = crash_base;
-	crashk_res.end = crash_base + crash_size - 1;
-	insert_resource(&iomem_resource, &crashk_res);
+	reserve_crashkernel_generic(boot_command_line, crash_size, crash_base, low_size, high);
 
 	if (arm_has_idmap_alias()) {
 		/*
@@ -1064,9 +1022,6 @@ static void __init reserve_crashkernel(void)
 		insert_resource(&iomem_resource, &crashk_boot_res);
 	}
 }
-#else
-static inline void reserve_crashkernel(void) {}
-#endif /* CONFIG_CRASH_RESERVE*/
 
 void __init hyp_mode_check(void)
 {
@@ -1189,7 +1144,7 @@ void __init setup_arch(char **cmdline_p)
 	if (!is_smp())
 		hyp_mode_check();
 
-	reserve_crashkernel();
+	arch_reserve_crashkernel();
 
 #ifdef CONFIG_VT
 #if defined(CONFIG_VGA_CONSOLE)
-- 
2.34.1

Re: [PATCH v3 3/3] ARM: Use generic interface to simplify crashkernel reservation

[Linus Walleij]

On Thu, Jul 18, 2024 at 5:51 AM Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Currently, x86, arm64, riscv and loongarch has been switched to generic
> crashkernel reservation, which is also ready for 32bit system.
> So with the help of function parse_crashkernel() and generic
> reserve_crashkernel_generic(), arm32 crashkernel reservation can also
> be simplified by steps:
>
> 1) Add a new header file <asm/crash_reserve.h>, and define CRASH_ALIGN,
>    CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX in it;
>
> 2) Add arch_reserve_crashkernel() to call parse_crashkernel() and
>    reserve_crashkernel_generic();
>
> 3) Add ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION Kconfig in
>    arch/arm/Kconfig.
>
> The old reserve_crashkernel() can be removed.
>
> Following test cases have been performed as expected on QEMU vexpress-a9
> (1GB system memory):
>
> 1) crashkernel=4G,high                          // invalid
> 2) crashkernel=1G,high                          // invalid
> 3) crashkernel=1G,high crashkernel=0M,low       // invalid
> 4) crashkernel=256M,high                        // invalid
> 5) crashkernel=256M,low                         // invalid
> 6) crashkernel=256M crashkernel=256M,high       // high is ignored, ok
> 7) crashkernel=256M crashkernel=256M,low        // low is ignored, ok
> 8) crashkernel=256M,high crashkernel=256M,low   // invalid
> 9) crashkernel=256M,high crashkernel=4G,low     // invalid
> 10) crashkernel=256M                            // ok
> 11) crashkernel=512M                            // ok
> 12) crashkernel=256M@0x88000000                 // ok
> 13) crashkernel=256M@0x78000000                 // ok
> 14) crashkernel=512M@0x78000000                 // ok
>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Tested-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> v3:
> - Update the commit message.

I haven't used crash much but it looks right to my untrained eye.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij