Fix arm64 CONFIG_CMDLINE handling and remove CMDLINE_EXTEND

Hi again,

This is version two of the series I previously posted here:

	https://lore.kernel.org/r/[email protected]

The main change since v1 is that, rather than "fix" the FDT code to follow the documented behaviour for CMDLINE_EXTEND, I've opted to remove the thing entirely for arm64 while a less ambiguous and generic replacement is developed, probably based on either [1] or [2].

I've left the first-patch as-is so that it's easier to incorporate whatever ends up replacing CMDLINE_EXTEND in future.

Cheers,

Will

[1] https://patchwork.ozlabs.org/project/linuxppc-dev/cover/[email protected]/
[2] https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/

--->8

Will Deacon (2):
arm64: cpufeatures: Fix handling of CONFIG_CMDLINE for idreg overrides
arm64: Drop support for CMDLINE_EXTEND

 arch/arm64/Kconfig                 |  6 -----
 arch/arm64/kernel/idreg-override.c | 43 ++++++++++++++++--------------
 2 files changed, 23 insertions(+), 26 deletions(-)

2.30.1.766.gb4fecdf3b7-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The built-in kernel commandline (CONFIG_CMDLINE) can be configured in three different ways:

1. CMDLINE_FORCE: Use CONFIG_CMDLINE instead of any bootloader args
2. CMDLINE_EXTEND: Append the bootloader args to CONFIG_CMDLINE
3. CMDLINE_FROM_BOOTLOADER: Only use CONFIG_CMDLINE if there aren't any bootloader args.

The early cmdline parsing to detect idreg overrides gets (2) and (3) slightly wrong: in the case of (2) the bootloader args are parsed first and in the case of (3) the CMDLINE is always parsed.

Fix these issues by moving the bootargs parsing out into a helper function and following the same logic as that used by the EFI stub.

---
 arch/arm64/kernel/idreg-override.c | 44 +++++++++++++++++-------------
 1 file changed, 25 insertions(+), 19 deletions(-)

index dffb16682330..cc071712c6f9 100644
@@ -163,33 +163,39 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases)
 	} while (1);
 }
 
-static __init void parse_cmdline(void)
+static __init const u8 *get_bootargs_cmdline(void)
 {
-	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
-		const u8 *prop;
-		void *fdt;
-		int node;
+	const u8 *prop;
+	void *fdt;
+	int node;
 
-		fdt = get_early_fdt_ptr();
-		if (!fdt)
-			goto out;
+	fdt = get_early_fdt_ptr();
+	if (!fdt)
+		return NULL;
 
-		node = fdt_path_offset(fdt, "/chosen");
-		if (node < 0)
-			goto out;
+	node = fdt_path_offset(fdt, "/chosen");
+	if (node < 0)
+		return NULL;
 
-		prop = fdt_getprop(fdt, node, "bootargs", NULL);
-		if (!prop)
-			goto out;
+	prop = fdt_getprop(fdt, node, "bootargs", NULL);
+	if (!prop)
+		return NULL;
 
-		__parse_cmdline(prop, true);
+	return strlen(prop) ? prop : NULL;
+}
 
-		if (!IS_ENABLED(CONFIG_CMDLINE_EXTEND))
-			return;
+static __init void parse_cmdline(void)
+{
+	const u8 *prop = get_bootargs_cmdline();
+
+	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
+	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
+	    !prop) {
+		__parse_cmdline(CONFIG_CMDLINE, true);
 	}
 
-out:
-	__parse_cmdline(CONFIG_CMDLINE, true);
+	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && prop)
+		__parse_cmdline(prop, true);
 }
 
 /* Keep checkers quiet */

2.30.1.766.gb4fecdf3b7-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

It bothers me a bit having command line handling here. It means how the command line is assembled in 2 places. I guess if we get rid of ambiguous 'extend' then it's better, but perhaps a better implementation would be an api get a specific command line parameter. The main downside would be searching the DT again for each parameter if we can't store any data in between calls, but there's ways around that. PowerPC also needs similar functionality in disabled_on_cmdline().

Anyways, that's all beyond the scope of this.

Rob

It's more than 2 places, it's also done in the EFI stub; see efi_pe_entry() in drivers/firmware/efi/libstub/efi-stub.c. That's why I'm pushing on this, because at the moment the command-line reported by /proc/cmdline doesn't match the command-line that was parsed there because they stitch it together in opposite orders when CMDLINE_EXTEND is used.

Christophe's patches at least aim to put the assembling all in one place, although we'll have to see whether or not it can be used in the EFI stub environment.

Will

The documented behaviour for CMDLINE_EXTEND is that the arguments from the bootloader are appended to the built-in kernel command line. This also matches the option parsing behaviour for the EFI stub and early ID register overrides.

Bizarrely, the fdt behaviour is the other way around: appending the built-in command line to the bootloader arguments, resulting in a command-line that doesn't necessarily line-up with the parsing order and definitely doesn't line-up with the documented behaviour.

As it turns out, there is a proposal [1] to replace CMDLINE_EXTEND with CMDLINE_PREPEND and CMDLINE_APPEND options which should hopefully make the intended behaviour much clearer. While we wait for those to land, drop CMDLINE_EXTEND for now as there appears to be little enthusiasm for changing the current FDT behaviour.

[1] https://lore.kernel.org/lkml/[email protected]/

---
 arch/arm64/Kconfig                 | 6 ------
 arch/arm64/kernel/idreg-override.c | 5 +----
 2 files changed, 1 insertion(+), 10 deletions(-)

index 1f212b47a48a..f15418332d16 100644
@@ -1855,12 +1855,6 @@ config CMDLINE_FROM_BOOTLOADER
 	  the boot loader doesn't provide any, the default kernel command
 	  string provided in CMDLINE will be used.
 
-config CMDLINE_EXTEND
-	bool "Extend bootloader kernel arguments"
-	help
-	  The command-line arguments provided by the boot loader will be
-	  appended to the default kernel command string.
-
 config CMDLINE_FORCE
 	bool "Always use the default kernel command string"
 	help

index cc071712c6f9..83f1c4b92095 100644
@@ -188,11 +188,8 @@ static __init void parse_cmdline(void)
 {
 	const u8 *prop = get_bootargs_cmdline();
 
-	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
-	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
-	    !prop) {
+	if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !prop)
 		__parse_cmdline(CONFIG_CMDLINE, true);
-	}
 
 	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && prop)
 		__parse_cmdline(prop, true);

2.30.1.766.gb4fecdf3b7-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Applied to arm64 (for-next/fixes), thanks!

[1/2] arm64: cpufeatures: Fix handling of CONFIG_CMDLINE for idreg overrides

      https://git.kernel.org/arm64/c/df304c2d0dfd
[2/2] arm64: Drop support for CMDLINE_EXTEND
      https://git.kernel.org/arm64/c/cae118b6acc3

Cheers,

Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev