On 2022/5/7 17:35, Leizhen (ThunderTown) wrote:
>
>
> On 2022/5/7 11:37, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2022/5/7 10:07, Baoquan He wrote:
>>> On 05/07/22 at 09:34am, Leizhen (ThunderTown) wrote:
>>>>
>>>>
>>>> On 2022/5/7 7:10, Baoquan He wrote:
>>>>> On 05/06/22 at 07:43pm, Zhen Lei wrote:
>>>>> ......
>>>>>> @@ -118,8 +162,7 @@ static void __init reserve_crashkernel(void)
>>>>>> if (crash_base)
>>>>>> crash_max = crash_base + crash_size;
>>>>>>
>>>>>> - /* Current arm64 boot protocol requires 2MB alignment */
>>>>>> - crash_base = memblock_phys_alloc_range(crash_size, SZ_2M,
>>>>>> + crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
>>>>>> crash_base, crash_max);
>>>>>> if (!crash_base) {
>>>>>> pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
>>>>>> @@ -127,6 +170,11 @@ static void __init reserve_crashkernel(void)
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>
>>>>> There's corner case missed, e.g
>>>>> 1) ,high and ,low are specified, CONFIG_ZONE_DMA|DMA32 is not enabled;
>>>>> 2) ,high and ,low are specified, the whole system memory is under 4G.
>>>>>
>>>>> Below judgement can filter them away:
>>>>>
>>>>> if (crash_base > arm64_dma_phys_limit && crash_low_size &&
>>>>> reserve_crashkernel_low(crash_low_size)) {
>>>>>
>>>>> What's your opinion? Leave it and add document to notice user, or fix it
>>>>> with code change?
>
> I decided to modify the code and document. But the code changes aren't what
> you suggested. For the following reasons:
> 1. The memory allocated for 'high' may be partially under 4G. So the low
> memory may not be enough. Of course, it's rare.
> 2. The second kernel can work properly only when the high and low memory
> are successfully applied for. For example, high=128M, low=128M, but the
> second kernel need 256M.
>
> So for the cases you listed:
> 1) ,high and ,low are specified, CONFIG_ZONE_DMA|DMA32 is not enabled;
> --> Follow you suggestion, ignore crashkernel=Y,low, don't allocate low memory.
>
> @@ -100,6 +100,14 @@ static int __init reserve_crashkernel_low(unsigned long long low_size)
> {
> unsigned long long low_base;
>
> + /*
> + * The kernel does not have any DMA zone, so the range of each DMA
> + * zone is unknown. Please make sure both CONFIG_ZONE_DMA and
> + * CONFIG_ZONE_DMA32 are also not set in the second kernel.
> + */
> + if (!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32))
> + return 0;
> +
>
> 2) ,high and ,low are specified, the whole system memory is under 4G.
> --> two memory ranges will be allocated, the size is what 'high' and 'low' specified.
> --> Yes, the memory of 'low' may be above 'high', but the 'high' just hint allocation
> --> from top, try high memory first. Of course, this may cause kexec to fail to load.
> --> Because the memory of 'low' with small size will be used to store Image, etc..
> --> But the memory of 'low' above 'high' is almost impossible, we use memblock API to
> --> allocate memory from top to bottem, 'low' above 'high' need a sizeable memory block
> --> (128M, 256M?) to be freed at init phase.
> --> Maybe I should add: crash_max = min(crash_base, CRASH_ADDR_LOW_MAX);
> --> to make sure the memory of 'low' is always under 'high'
I have added the min() above.
Test result:
1) ,high and ,low are specified, CONFIG_ZONE_DMA|DMA32 is not enabled;
root@localhost:~# dmesg | grep crash
[ 0.000000] crashkernel reserved: 0x0000000420000000 - 0x0000000440000000 (512 MB)
[ 0.000000] Kernel command line: console=ttyAMA0 root=/dev/vda rw panic_on_oops=1 oops=panic crashkernel=512M,high crashkernel=128M,low
2) ,high and ,low are specified, the whole system memory is under 4G.
root@localhost:~# dmesg | grep crash
[ 0.000000] crashkernel tmp reserved: 0x00000000f2800000 - 0x00000000fa800000 (128 MB)
[ 0.000000] crashkernel low memory reserved: 0xca800000 - 0xd2800000 (128 MB)
[ 0.000000] crashkernel reserved: 0x00000000d2800000 - 0x00000000f2800000 (512 MB)
[ 0.000000] Kernel command line: console=ttyAMA0 root=/dev/vda rw panic_on_oops=1 oops=panic crashkernel=512M,high crashkernel=128M,low
test stub for 2):
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 5cb73bbd286b100..abbde2158a0976a 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -147,6 +147,7 @@ static void __init reserve_crashkernel(void)
unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
char *cmdline = boot_command_line;
int ret;
+ unsigned long long tmp_base;
if (!IS_ENABLED(CONFIG_KEXEC_CORE))
return;
@@ -179,6 +180,11 @@ static void __init reserve_crashkernel(void)
if (crash_base)
crash_max = crash_base + crash_size;
+ tmp_base = memblock_phys_alloc_range(crash_low_size, CRASH_ALIGN, crash_base, crash_max);
+ BUG_ON(!tmp_base);
+ pr_info("crashkernel tmp reserved: 0x%016llx - 0x%016llx (%lld MB)\n",
+ tmp_base, tmp_base + crash_low_size, crash_low_size >> 20);
+
crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
crash_base, crash_max);
if (!crash_base) {
@@ -186,6 +192,7 @@ static void __init reserve_crashkernel(void)
crash_size);
return;
}
+ memblock_phys_free(tmp_base, crash_low_size);
if (crash_low_size && reserve_crashkernel_low(crash_low_size, crash_base)) {
memblock_phys_free(crash_base, crash_size);
>
>>>>
>>>> I think maybe we can leave it unchanged. If the user configures two memory ranges,
>>>> we'd better apply for two. Otherwise, he'll be confused when he inquires. Currently,
>>>> crash_low_size is non-zero only when 'crashkernel=Y,low' is explicitly configured.
>>>
>>> Then user need know the system information, e.g how much is the high
>>> memory, low memory, if CONFIG_ZONE_DMA|DMA32 is enabled. And we need
>>> describe these cases in document. Any corner case or exception need
>>> be noted if we don't handle it in code.
>>>
>>> Caring about this very much because we have CI with existed test cases
>>> to run on the system, and QA will check these manually too. Support
>>> engineer need detailed document if anything special but happened.
>>> Anything unclear or uncovered will be reported as bug to our kernel dev.
>>> Guess your company do the similar thing like this.
>>>
>>> This crashkerne,high and crashkernel,low reservation is special if we
>>> allow ,high, ,low existing in the same zone. Imagine on system with
>>> CONFIG_ZONE_DMA|DMA32 disabled, people copy the crashkernel=512M,high
>>> and crashkernel=128M,low from other system, and he could get
>>> crash_res at [5G, 5G+512M], while crash_low_res at [6G, 6G+128M]. Guess
>>> how they will judge us.
>>
>> OK, I got it.
>>
>>>
>>>>
>>>>>
>>>>> I would suggest merging this series, Lei can add this corner case
>>>>> handling on top. Since this is a newly added support, we don't have
>>>>> to make it one step. Doing step by step can make reviewing easier.
>>>>>
>>>>>> + if (crash_low_size && reserve_crashkernel_low(crash_low_size)) {
>>>>>> + memblock_phys_free(crash_base, crash_size);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> pr_info("crashkernel reserved: 0x%016llx - 0x%016llx (%lld MB)\n",
>>>>>> crash_base, crash_base + crash_size, crash_size >> 20);
>>>>>>
>>>>>> @@ -135,6 +183,9 @@ static void __init reserve_crashkernel(void)
>>>>>> * map. Inform kmemleak so that it won't try to access it.
>>>>>> */
>>>>>> kmemleak_ignore_phys(crash_base);
>>>>>> + if (crashk_low_res.end)
>>>>>> + kmemleak_ignore_phys(crashk_low_res.start);
>>>>>> +
>>>>>> crashk_res.start = crash_base;
>>>>>> crashk_res.end = crash_base + crash_size - 1;
>>>>>> insert_resource(&iomem_resource, &crashk_res);
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>> --
>>>> Regards,
>>>> Zhen Lei
>>>>
>>>
>>> .
>>>
>>
>
--
Regards,
Zhen Lei
