Re: [PATCH 0/3] arm64: kdump: Restore the write protection for the crashkernel memory region

[Baoquan He]

Hi,

On 07/21/23 at 04:17pm, thunder.leizhen@xxxxxxxxxxxxxxx wrote:
> From: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
> 
> 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.

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?

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.

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

Thanks
Baoquan