Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 0/3] arm64: kdump: Restore the write protection for the
 crashkernel memory region
To:     Baoquan He <bhe@redhat.com>
Cc:     Dave Young <dyoung@redhat.com>, Vivek Goyal <vgoyal@redhat.com>,
        "Eric W . Biederman" <ebiederm@xmission.com>,
        kexec@lists.infradead.org, linux-kernel@vger.kernel.org,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        linux-arm-kernel@lists.infradead.org,
        Zhen Lei <thunder.leizhen@huawei.com>
References: <20230721081726.882-1-thunder.leizhen@huaweicloud.com>
 <ZL595WtNlLnvtnJl@MiWiFi-R3L-srv>
From:   "Leizhen (ThunderTown)" <thunder.leizhen@huaweicloud.com>
Message-ID: <f067c293-fce4-6514-fa8b-947cae781c27@huaweicloud.com>
Date:   Tue, 25 Jul 2023 15:14:26 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <ZL595WtNlLnvtnJl@MiWiFi-R3L-srv>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2023/7/24 21:34, Baoquan He wrote:
> Hi,
> 
> On 07/21/23 at 04:17pm, thunder.leizhen@huaweicloud.com wrote:
>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> Unlike in the past, the low memory allocation direction of the crashkernel is
>> changed from top-down to bottom-up. As long as the DMA zone has sufficient
>> continuous free memory, the allocated crashkernel low memory must meet the
>> requirements. The allocation direction of crashkernel high memory remains
>> unchanged, that is, top-down. As long as the high memory(above DMA zone) has
>> sufficient continuous free memory, the allocated crashkernel high memory must
>> meet the requirements. In this way, with the restoration of the original
>> page-level mapping and the implementation of the arch_kexec_protect_crashkres()
>> function, write protection for the crashkernel memory region can be supported.
>>
>> Of course, if the high memory or low memory cannot meet the initial requirements,
>> that is, fall back is required. In this case, write protection is not supported
>> because the newly allocated memory is not page-level mapped.
>>
>> Because the original retry process is eliminated, the new process looks clearer
>> and is a simple sequential flow.
> 
> Thanks, but no.
> 
> The pure semantics and the corresponding implementation have been 
> complicated, it's not worth adding so much more complication to it
> just because of one inessential feature.

It's just that the code looks like if..else branches are a little more, but the
idea is not complicated.
1. Reserve low memory bottom-up(start from 0), reserve high memory top-down(start from CRASH_ADDR_HIGH_MAX)
2. zone_sizes_init() update arm64_dma_phys_limit, now CRASH_ADDR_LOW_MAX is known.
3. Use CRASH_ADDR_LOW_MAX to verify the preserved low memory and high memory, OK or fall back.

To be honest, the code can be simplified a lot if we don't support the following:
-----
If reservation from the high memory failed, the kernel falls back to
searching the low memory with the specified size in crashkernel=,high.
If it succeeds, no further reservation for low memory is needed.

> 
> If stomp really happened and destroy the loaded kdump kernel, the write
> protection truly can save kdump to make vmcore dumping succeed. While
> without write protection, we at least know that stomp happened by the
> later checksum verifycation. That's an advantage over write protection
> which silently ignores the stomp, right?

Theoretically, write protection can catch exceptions in a timely manner
and ensure that kdump is successful. If the problem is easy to reproduce,
it doesn't matter if it fails once. However, for versions that have been
delivered for commercial use, the customer may not give the second chance.

> 
> So, due to the low cost performance, from people maintaining and
> understanding the code point of view, I would like to NACK this series.
> BUT since all these code changes are added into arm64 arch, I won't
> object if arm64 maintainers wants to pikc them up.

This new idea is not bad. After all, before commercial use, "fall back"
can be avoided by adjusting crashkernel size in command line. So the
problem is pretty much solved.

> 
> By the way, as we have talked before, arm64 lacks the loaded kernel
> checksum storing and verifying, would you like to add that?

OKay.

> 
> Thanks
> Baoquan
> 
> .
> 

-- 
Regards,
  Zhen Lei