Re: [tip: x86/urgent] x86/setup: Call early_reserve_memory() earlier

From: Borislav Petkov <bp@alien8.de>
To: "Williams, Dan J" <dan.j.williams@intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-tip-commits@vger.kernel.org" 
	<linux-tip-commits@vger.kernel.org>,
	"nathan@kernel.org" <nathan@kernel.org>,
	"Gross, Jurgen" <jgross@suse.com>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	"marmarek@invisiblethingslab.com"
	<marmarek@invisiblethingslab.com>,
	"Chagam, Anjaneya" <anjaneya.chagam@intel.com>,
	"bp@suse.de" <bp@suse.de>, "x86@kernel.org" <x86@kernel.org>,
	Mike Rapoport <rppt@kernel.org>
Subject: Re: [tip: x86/urgent] x86/setup: Call early_reserve_memory() earlier
Date: Thu, 4 Nov 2021 12:40:33 +0100	[thread overview]
Message-ID: <YYPGsUNm0UXcYKOA@zn.tnic> (raw)
In-Reply-To: <e8dd8993c38702ee6dd73b3c11f158617e665607.camel@intel.com>

+ Mike.

On Thu, Nov 04, 2021 at 05:38:54AM +0000, Williams, Dan J wrote:
> By inspection, this commit looks like the source of the problem because
> early_reserve_memory() now runs before parse_early_param().

Yah, I see it too:

early_reserve_memory
|-> efi_memblock_x86_reserve_range
    |-> efi_fake_memmap_early

which does

        if (!efi_soft_reserve_enabled())
                return;

and that would have set EFI_MEM_NO_SOFT_RESERVE after having parsed
"nosoftreserve".

And parse_early_param() happens later.

Uuuf, that early memory reservation dance is not going to be over,
ever...

Well, I guess we can do something like this below. The intent being to
carve out the command line preparation into a separate function which
does the early param parsing too. So that it all goes together.

And then call that function before early_reserve_memory() so that the
params have been parsed by then.

Looking at the changed flow, I think we should be ok but I've said that
a bunch of times already regarding this memory reservation early and
something in our house of cards called early boot order always broke.

Damn.

---

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 40ed44ead063..05f69e7d84dc 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -742,6 +742,28 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
 	return 0;
 }
 
+static char *prepare_command_line(void)
+{
+#ifdef CONFIG_CMDLINE_BOOL
+#ifdef CONFIG_CMDLINE_OVERRIDE
+	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+#else
+	if (builtin_cmdline[0]) {
+		/* append boot loader cmdline to builtin */
+		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
+		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
+		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+	}
+#endif
+#endif
+
+	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+
+	parse_early_param();
+
+	return command_line;
+}
+
 /*
  * Determine if we were loaded by an EFI loader.  If so, then we have also been
  * passed the efi memmap, systab, etc., so we should use these data structures
@@ -830,6 +852,23 @@ void __init setup_arch(char **cmdline_p)
 
 	x86_init.oem.arch_setup();
 
+	/*
+	 * x86_configure_nx() is called before parse_early_param() (called by
+	 * prepare_command_line()) to detect whether hardware doesn't support
+	 * NX (so that the early EHCI debug console setup can safely call
+	 * set_fixmap()). It may then be called again from within noexec_setup()
+	 * during parsing early parameters to honor the respective command line
+	 * option.
+	 */
+	x86_configure_nx();
+
+	/*
+	 * This parses early params and it needs to run before
+	 * early_reserve_memory() because latter relies on such settings
+	 * supplies as early params.
+	 */
+	*cmdline_p = prepare_command_line();
+
 	/*
 	 * Do some memory reservations *before* memory is added to memblock, so
 	 * memblock allocations won't overwrite it.
@@ -863,33 +902,6 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.start = __pa_symbol(__bss_start);
 	bss_resource.end = __pa_symbol(__bss_stop)-1;
 
-#ifdef CONFIG_CMDLINE_BOOL
-#ifdef CONFIG_CMDLINE_OVERRIDE
-	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-#else
-	if (builtin_cmdline[0]) {
-		/* append boot loader cmdline to builtin */
-		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
-		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
-		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-	}
-#endif
-#endif
-
-	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
-	*cmdline_p = command_line;
-
-	/*
-	 * x86_configure_nx() is called before parse_early_param() to detect
-	 * whether hardware doesn't support NX (so that the early EHCI debug
-	 * console setup can safely call set_fixmap()). It may then be called
-	 * again from within noexec_setup() during parsing early parameters
-	 * to honor the respective command line option.
-	 */
-	x86_configure_nx();
-
-	parse_early_param();
-
 #ifdef CONFIG_MEMORY_HOTPLUG
 	/*
 	 * Memory used by the kernel cannot be hot-removed because Linux


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
