On 2021/12/24 1:26, Borislav Petkov wrote:

On Wed, Dec 22, 2021 at 09:08:05PM +0800, Zhen Lei wrote:

quoted

From: Chen Zhou <redacted>

We will make the functions reserve_crashkernel() as generic, the
xen_pv_domain() check in reserve_crashkernel() is relevant only to
x86,

Why is that so? Is Xen-PV x86-only?

quoted

the same as insert_resource() in reserve_crashkernel[_low]().

Why?

Looking at

  0212f9159694 ("x86: Add Crash kernel low reservation")

it *surprisingly* explains why that resources thing is being added:

    We need to add another range in /proc/iomem like "Crash kernel low",
    so kexec-tools could find that info and append to kdump kernel
    command line.

Then,

  157752d84f5d ("kexec: use Crash kernel for Crash kernel low")

renamed it because, as it states, kexec-tools was taught to handle
multiple resources of the same name.

So why does kexec-tools on arm *not* need those iomem resources? How
does it parse the ranges there? Questions over questions...

https://lkml.org/lkml/2019/4/4/1758

Chen Zhou has explained before, see below. I'll analyze why x86 and arm64 need
to process iomem resources at different times.

 < This very reminds what x86 does. Any chance some of the code can be reused
 < rather than duplicated?
As i said in the comment, i transport reserve_crashkernel_low() from x86_64. There are minor
differences. In arm64, we don't need to do insert_resource(), we do request_resource()
in request_standard_resources() later.

So last time I told you to sit down and take your time with this cleanup.

quoted

From reading this here, it doesn't look like it. Rather, it looks like

hastily done in a hurry and hurrying stuff doesn't help you one bit - it
actually makes it worse.

Your commit messages need to explain *why* a change is being done and
why is that ok. This one doesn't.

OK, I'll do this in follow-up patches.

quoted

@@ -1120,7 +1109,17 @@ void __init setup_arch(char **cmdline_p)
 	 * Reserve memory for crash kernel after SRAT is parsed so that it
 	 * won't consume hotpluggable memory.
 	 */
-	reserve_crashkernel();
+#ifdef CONFIG_KEXEC_CORE
+	if (xen_pv_domain())
+		pr_info("Ignoring crashkernel for a Xen PV domain\n");

This is wrong - the check is currently being done inside
reserve_crashkernel(), *after* it has parsed a crashkernel= cmdline
correctly - and not before.

Your change would print on Xen PV, regardless of whether it has received
crashkernel= on the cmdline or not.

Yes, you're right. There are changes in code logic, but the print doesn't
seem to cause any misunderstanding.

This is exactly why I say that making those functions generic and shared
might not be such a good idea, after all, because then you'd have to
sprinkle around arch-specific stuff.

Yes, I'm thinking about that too. Perhaps they are not suitable for full
code sharing, but it looks like there's some code that can be shared.
For example, the function parse_crashkernel_in_order() that I extracted
based on your suggestion, it could also be parse_crashkernel_high_low().
Or the function reserve_crashkernel_low().

There are two ways to reserve memory above 4G:
1. Use crashkernel=X,high, with or without crashkernel=X,low
2. Use crashkernel=X,[offset], but try low memory first. If failed, then
   try high memory, and retry at least 256M low memory.

I plan to only implement 2 in the next version so that there can be fewer
changes. Then implement 1 after 2 is applied.

One of the ways how to address this particular case here would be:

1. Add a x86-specific wrapper around parse_crashkernel() which does
all the parsing. When that wrapper finishes, you should have parsed
everything that has crashkernel= on the cmdline.

2. At the end of that wrapper, you do arch-specific checks and setup
like the xen_pv_domain() one.

3. Now, you do reserve_crashkernel(), if those checks pass.

The question is, whether the flow on arm64 can do the same. Probably but
it needs careful auditing.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel