[PATCH 5/8] drivers: firmware: efi: libstub: enable generic commandline

[PATCH 5/8] drivers: firmware: efi: libstub: enable generic commandline

[Daniel Walker]

This adds code to handle the generic command line changes.
The efi code appears that it doesn't benefit as much from this design
as it could.

For example, if you had a prepend command line with "nokaslr" then
you might be helpful to re-enable it in the boot loader or dts,
but there appears to be no way to re-enable kaslr or some of the
other options.

The efi command line handling is incorrect. x86 and arm have an append
system however the efi code prepends the command line.

For example, you could have a non-upgradable bios which sends

efi=disable_early_pci_dma

This hypothetically could have been set because early pci dma caused
issues on early versions of the product.

Then later the early pci dma was made to work and the company desired
to start using it. To override the bios you could set the CONFIG_CMDLINE
to,

efi=no_disable_early_pci_dma

then parsing would normally start with the bios command line, then move
to the CONFIG_CMDLINE and you would end up with early pci dma turned on.

however, current efi code keeps early pci dma off because the bios
arguments always override the built in.

Per my reading this is different from the main body of x86, arm, and
arm64.

The generic command line provides both append and prepend, so it
alleviates this issue if it's used. However not all architectures use
it.

It would be desirable to allow the efi stub to have it's builtin command
line to be modified after compile, but I don't see a feasible way to do
that currently.

Cc: xe-linux-external@cisco.com
Signed-off-by: Daniel Walker <danielwa@cisco.com>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 29 +++++++++++++++++++
 drivers/firmware/efi/libstub/efi-stub.c       |  9 ++++++
 drivers/firmware/efi/libstub/efistub.h        |  1 +
 drivers/firmware/efi/libstub/x86-stub.c       | 13 +++++++--
 4 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index aa8da0a49829..16318f55f187 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -13,6 +13,7 @@
 #include <linux/efi.h>
 #include <linux/kernel.h>
 #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
+#include <linux/cmdline.h>
 #include <asm/efi.h>
 #include <asm/setup.h>
 
@@ -172,6 +173,34 @@ int efi_printk(const char *fmt, ...)
 	return printed;
 }
 
+/**
+ * efi_handle_cmdline() - handle adding in building parts of the command line
+ * @cmdline:	kernel command line
+ *
+ * Add in the generic parts of the commandline and start the parsing of the
+ * command line.
+ *
+ * Return:	status code
+ */
+efi_status_t efi_handle_cmdline(char const *cmdline)
+{
+	efi_status_t status = EFI_SUCCESS;
+
+	if (sizeof(CMDLINE_STATIC_PREPEND) > 1)
+		status |= efi_parse_options(CMDLINE_STATIC_PREPEND);
+
+	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
+		status |= efi_parse_options(cmdline);
+
+	if (sizeof(CMDLINE_STATIC_APPEND) > 1)
+		status |= efi_parse_options(CMDLINE_STATIC_APPEND);
+
+	if (status != EFI_SUCCESS)
+		efi_err("Failed to parse options\n");
+
+	return status;
+}
+
 /**
  * efi_parse_options() - Parse EFI command line options
  * @cmdline:	kernel command line
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index 26e69788f27a..baa69b24cfdd 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -172,6 +172,14 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 		goto fail;
 	}
 
+#ifdef CONFIG_GENERIC_CMDLINE
+	status = efi_handle_cmdline(cmdline_ptr);
+	if (status != EFI_SUCCESS) {
+		goto fail_free_cmdline;
+	}
+#endif
+
+#ifdef CONFIG_CMDLINE
 	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
 	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
 	    cmdline_size == 0) {
@@ -189,6 +197,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 			goto fail_free_cmdline;
 		}
 	}
+#endif
 
 	efi_info("Booting Linux Kernel...\n");
 
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index cde0a2ef507d..07c7f9fdfffc 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -800,6 +800,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
 				 unsigned long alignment,
 				 unsigned long min_addr);
 
+efi_status_t efi_handle_cmdline(char const *cmdline);
 efi_status_t efi_parse_options(char const *cmdline);
 
 void efi_parse_option_graphics(char *option);
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index f14c4ff5839f..30ad8fb7122d 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -673,6 +673,8 @@ unsigned long efi_main(efi_handle_t handle,
 	unsigned long bzimage_addr = (unsigned long)startup_32;
 	unsigned long buffer_start, buffer_end;
 	struct setup_header *hdr = &boot_params->hdr;
+	unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
+				       ((u64)boot_params->ext_cmd_line_ptr << 32));
 	efi_status_t status;
 
 	efi_system_table = sys_table_arg;
@@ -735,6 +737,14 @@ unsigned long efi_main(efi_handle_t handle,
 		image_offset = 0;
 	}
 
+#ifdef CONFIG_GENERIC_CMDLINE
+	status = efi_handle_cmdline((char *)cmdline_paddr);
+	if (status != EFI_SUCCESS) {
+		efi_err("Failed to parse options\n");
+		goto fail;
+	}
+#else /* CONFIG_GENERIC_CMDLINE */
+
 #ifdef CONFIG_CMDLINE_BOOL
 	status = efi_parse_options(CONFIG_CMDLINE);
 	if (status != EFI_SUCCESS) {
@@ -743,8 +753,6 @@ unsigned long efi_main(efi_handle_t handle,
 	}
 #endif
 	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
-		unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
-					       ((u64)boot_params->ext_cmd_line_ptr << 32));
 		status = efi_parse_options((char *)cmdline_paddr);
 		if (status != EFI_SUCCESS) {
 			efi_err("Failed to parse options\n");
@@ -752,6 +760,7 @@ unsigned long efi_main(efi_handle_t handle,
 		}
 	}
 
+#endif
 	/*
 	 * At this point, an initrd may already have been loaded by the
 	 * bootloader and passed via bootparams. We permit an initrd loaded
-- 
2.25.1

[PATCH 5/8] drivers: firmware: efi: libstub: enable generic commandline

[Daniel Walker]

This adds code to handle the generic command line changes.
The efi code appears that it doesn't benefit as much from this design
as it could.

For example, if you had a prepend command line with "nokaslr" then
you might be helpful to re-enable it in the boot loader or dts,
but there appears to be no way to re-enable kaslr or some of the
other options.

The efi command line handling is incorrect. x86 and arm have an append
system however the efi code prepends the command line.

For example, you could have a non-upgradable bios which sends

efi=disable_early_pci_dma

This hypothetically could have been set because early pci dma caused
issues on early versions of the product.

Then later the early pci dma was made to work and the company desired
to start using it. To override the bios you could set the CONFIG_CMDLINE
to,

efi=no_disable_early_pci_dma

then parsing would normally start with the bios command line, then move
to the CONFIG_CMDLINE and you would end up with early pci dma turned on.

however, current efi code keeps early pci dma off because the bios
arguments always override the built in.

Per my reading this is different from the main body of x86, arm, and
arm64.

The generic command line provides both append and prepend, so it
alleviates this issue if it's used. However not all architectures use
it.

It would be desirable to allow the efi stub to have it's builtin command
line to be modified after compile, but I don't see a feasible way to do
that currently.

Cc: xe-linux-external@cisco.com
Signed-off-by: Daniel Walker <danielwa@cisco.com>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 29 +++++++++++++++++++
 drivers/firmware/efi/libstub/efi-stub.c       |  9 ++++++
 drivers/firmware/efi/libstub/efistub.h        |  1 +
 drivers/firmware/efi/libstub/x86-stub.c       | 13 +++++++--
 4 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 3d972061c1b0..372f06ec8c49 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -13,6 +13,7 @@
 #include <linux/efi.h>
 #include <linux/kernel.h>
 #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
+#include <linux/cmdline.h>
 #include <asm/efi.h>
 #include <asm/setup.h>
 
@@ -172,6 +173,34 @@ int efi_printk(const char *fmt, ...)
 	return printed;
 }
 
+/**
+ * efi_handle_cmdline() - handle adding in building parts of the command line
+ * @cmdline:	kernel command line
+ *
+ * Add in the generic parts of the commandline and start the parsing of the
+ * command line.
+ *
+ * Return:	status code
+ */
+efi_status_t efi_handle_cmdline(char const *cmdline)
+{
+	efi_status_t status = EFI_SUCCESS;
+
+	if (sizeof(CMDLINE_STATIC_PREPEND) > 1)
+		status |= efi_parse_options(CMDLINE_STATIC_PREPEND);
+
+	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
+		status |= efi_parse_options(cmdline);
+
+	if (sizeof(CMDLINE_STATIC_APPEND) > 1)
+		status |= efi_parse_options(CMDLINE_STATIC_APPEND);
+
+	if (status != EFI_SUCCESS)
+		efi_err("Failed to parse options\n");
+
+	return status;
+}
+
 /**
  * efi_parse_options() - Parse EFI command line options
  * @cmdline:	kernel command line
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index f515394cce6e..ea52ebd99ea6 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -173,6 +173,14 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 		goto fail;
 	}
 
+#ifdef CONFIG_GENERIC_CMDLINE
+	status = efi_handle_cmdline(cmdline_ptr);
+	if (status != EFI_SUCCESS) {
+		goto fail_free_cmdline;
+	}
+#endif
+
+#ifdef CONFIG_CMDLINE
 	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
 	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
 	    cmdline_size == 0) {
@@ -190,6 +198,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 			goto fail_free_cmdline;
 		}
 	}
+#endif
 
 	efi_info("Booting Linux Kernel...\n");
 
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index b0ae0a454404..44c2e1194378 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -907,6 +907,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
 				 unsigned long alignment,
 				 unsigned long min_addr);
 
+efi_status_t efi_handle_cmdline(char const *cmdline);
 efi_status_t efi_parse_options(char const *cmdline);
 
 void efi_parse_option_graphics(char *option);
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 7a7abc8959d2..87196bb9c3bb 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -783,6 +783,8 @@ unsigned long efi_main(efi_handle_t handle,
 	unsigned long buffer_start, buffer_end;
 	struct setup_header *hdr = &boot_params->hdr;
 	unsigned long addr, size;
+	unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
+				       ((u64)boot_params->ext_cmd_line_ptr << 32));
 	efi_status_t status;
 
 	efi_system_table = sys_table_arg;
@@ -851,6 +853,14 @@ unsigned long efi_main(efi_handle_t handle,
 		image_offset = 0;
 	}
 
+#ifdef CONFIG_GENERIC_CMDLINE
+	status = efi_handle_cmdline((char *)cmdline_paddr);
+	if (status != EFI_SUCCESS) {
+		efi_err("Failed to parse options\n");
+		goto fail;
+	}
+#else /* CONFIG_GENERIC_CMDLINE */
+
 #ifdef CONFIG_CMDLINE_BOOL
 	status = efi_parse_options(CONFIG_CMDLINE);
 	if (status != EFI_SUCCESS) {
@@ -859,8 +869,6 @@ unsigned long efi_main(efi_handle_t handle,
 	}
 #endif
 	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
-		unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
-					       ((u64)boot_params->ext_cmd_line_ptr << 32));
 		status = efi_parse_options((char *)cmdline_paddr);
 		if (status != EFI_SUCCESS) {
 			efi_err("Failed to parse options\n");
@@ -868,6 +876,7 @@ unsigned long efi_main(efi_handle_t handle,
 		}
 	}
 
+#endif
 	/*
 	 * At this point, an initrd may already have been loaded by the
 	 * bootloader and passed via bootparams. We permit an initrd loaded
-- 
2.25.1

[PATCH 0/8] generic command line v6

[Daniel Walker]

This release is an up-rev of the v5 patches. No additional features have
been added. Some changes were mode to function names and some changes to
Kconfig dependencies. Also updated the config conversion for mips.

There are a number of people who have expressed interest in these
patches either by asking for them to be merge or testing them. If
people are so inclined please continue to request them to be merge
or to ask the status of the next release. It's helpful to motivate me to
release them again and for the maintainers to see the interest
generated.

These patches have been used by Cisco Systems, Inc. on millions of
released products to great effect. Hopefully they can be used by the
entire Linux eco system.

My apologies on the length between releases. I will try to release more
often.


Daniel Walker (8):
  CMDLINE: add generic builtin command line
  scripts: insert-sys-cert: add command line insert capability
  scripts: insert-sys-cert: change name to insert-symbol
  CMDLINE: mips: convert to generic builtin command line
  drivers: firmware: efi: libstub: enable generic commandline
  CMDLINE: x86: convert to generic builtin command line
  of: replace command line handling
  CMDLINE: arm64: convert to generic builtin command line

 arch/arm64/Kconfig                            |  33 +--
 arch/arm64/include/asm/setup.h                |   4 +
 arch/arm64/include/uapi/asm/setup.h           |   2 +
 arch/arm64/kernel/idreg-override.c            |   9 +-
 arch/arm64/kernel/pi/kaslr_early.c            |  14 +-
 arch/mips/Kconfig                             |   4 +-
 arch/mips/Kconfig.debug                       |  44 ----
 arch/mips/configs/ar7_defconfig               |  12 +-
 arch/mips/configs/bcm47xx_defconfig           |  10 +-
 arch/mips/configs/bcm63xx_defconfig           |  21 +-
 arch/mips/configs/bmips_be_defconfig          |  17 +-
 arch/mips/configs/bmips_stb_defconfig         | 139 ++++------
 arch/mips/configs/ci20_defconfig              |   8 +-
 arch/mips/configs/cu1000-neo_defconfig        |  19 +-
 arch/mips/configs/cu1830-neo_defconfig        |  19 +-
 arch/mips/configs/generic_defconfig           |  15 +-
 arch/mips/configs/gpr_defconfig               |  33 +--
 arch/mips/configs/loongson3_defconfig         |  29 +--
 arch/mips/include/asm/setup.h                 |   2 +
 arch/mips/kernel/relocate.c                   |  17 +-
 arch/mips/kernel/setup.c                      |  36 +--
 arch/mips/pic32/pic32mzda/early_console.c     |   2 +-
 arch/mips/pic32/pic32mzda/init.c              |   3 +-
 arch/x86/Kconfig                              |  44 +---
 arch/x86/kernel/setup.c                       |  18 +-
 .../firmware/efi/libstub/efi-stub-helper.c    |  29 +++
 drivers/firmware/efi/libstub/efi-stub.c       |   9 +
 drivers/firmware/efi/libstub/efistub.h        |   1 +
 drivers/firmware/efi/libstub/x86-stub.c       |  14 +-
 drivers/of/fdt.c                              |  22 +-
 include/linux/cmdline.h                       | 137 ++++++++++
 init/Kconfig                                  |  79 ++++++
 lib/Kconfig                                   |   4 +
 lib/Makefile                                  |   3 +
 lib/generic_cmdline.S                         |  53 ++++
 lib/test_cmdline1.c                           | 139 ++++++++++
 scripts/Makefile                              |   2 +-
 .../{insert-sys-cert.c => insert-symbol.c}    | 243 ++++++++++++------
 38 files changed, 807 insertions(+), 482 deletions(-)
 create mode 100644 include/linux/cmdline.h
 create mode 100644 lib/generic_cmdline.S
 create mode 100644 lib/test_cmdline1.c
 rename scripts/{insert-sys-cert.c => insert-symbol.c} (72%)

-- 
2.39.2

[PATCH 5/8] drivers: firmware: efi: libstub: enable generic commandline

[Daniel Walker]

This adds code to handle the generic command line changes.
The efi code appears that it doesn't benefit as much from this design
as it could.

For example, if you had a prepend command line with "nokaslr" then
you might be helpful to re-enable it in the boot loader or dts,
but there appears to be no way to re-enable kaslr or some of the
other options.

The efi command line handling is incorrect. x86 and arm have an append
system however the efi code prepends the command line.

For example, you could have a non-upgradable bios which sends

efi=disable_early_pci_dma

This hypothetically could have been set because early pci dma caused
issues on early versions of the product.

Then later the early pci dma was made to work and the company desired
to start using it. To override the bios you could set the CONFIG_CMDLINE
to,

efi=no_disable_early_pci_dma

then parsing would normally start with the bios command line, then move
to the CONFIG_CMDLINE and you would end up with early pci dma turned on.

however, current efi code keeps early pci dma off because the bios
arguments always override the built in.

Per my reading this is different from the main body of x86, arm, and
arm64.

The generic command line provides both append and prepend, so it
alleviates this issue if it's used. However not all architectures use
it.

It would be desirable to allow the efi stub to have it's builtin command
line to be modified after compile, but I don't see a feasible way to do
that currently.

Cc: xe-linux-external@cisco.com
Signed-off-by: Daniel Walker <danielwa@cisco.com>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 29 +++++++++++++++++++
 drivers/firmware/efi/libstub/efi-stub.c       |  9 ++++++
 drivers/firmware/efi/libstub/efistub.h        |  1 +
 drivers/firmware/efi/libstub/x86-stub.c       | 14 +++++++--
 4 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index bfa30625f5d0..952fa2cdff51 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -11,6 +11,7 @@
 
 #include <linux/efi.h>
 #include <linux/kernel.h>
+#include <linux/cmdline.h>
 #include <asm/efi.h>
 #include <asm/setup.h>
 
@@ -29,6 +30,34 @@ bool __pure __efi_soft_reserve_enabled(void)
 	return !efi_nosoftreserve;
 }
 
+/**
+ * efi_handle_cmdline() - handle adding in built-in parts of the command line
+ * @cmdline:	kernel command line
+ *
+ * Add in the generic parts of the commandline and start the parsing of the
+ * command line.
+ *
+ * Return:	status code
+ */
+efi_status_t efi_handle_builtin_cmdline(char const *cmdline)
+{
+	efi_status_t status = EFI_SUCCESS;
+
+	if (sizeof(CMDLINE_STATIC_PREPEND) > 1)
+		status |= efi_parse_options(CMDLINE_STATIC_PREPEND);
+
+	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
+		status |= efi_parse_options(cmdline);
+
+	if (sizeof(CMDLINE_STATIC_APPEND) > 1)
+		status |= efi_parse_options(CMDLINE_STATIC_APPEND);
+
+	if (status != EFI_SUCCESS)
+		efi_err("Failed to parse options\n");
+
+	return status;
+}
+
 /**
  * efi_parse_options() - Parse EFI command line options
  * @cmdline:	kernel command line
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index f9c1e8a2bd1d..770abe95c0ee 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -127,6 +127,14 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
 		return EFI_OUT_OF_RESOURCES;
 	}
 
+#ifdef CONFIG_GENERIC_CMDLINE
+	status = efi_handle_builtin_cmdline(cmdline);
+	if (status != EFI_SUCCESS) {
+		goto fail_free_cmdline;
+	}
+#endif
+
+#ifdef CONFIG_CMDLINE
 	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
 	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
 	    cmdline_size == 0) {
@@ -144,6 +152,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
 			goto fail_free_cmdline;
 		}
 	}
+#endif
 
 	*cmdline_ptr = cmdline;
 	return EFI_SUCCESS;
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 212687c30d79..1ac6631905c5 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -996,6 +996,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
 				 unsigned long alignment,
 				 unsigned long min_addr);
 
+efi_status_t efi_handle_builtin_cmdline(char const *cmdline);
 efi_status_t efi_parse_options(char const *cmdline);
 
 void efi_parse_option_graphics(char *option);
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 9d5df683f882..273a8a9c8bbb 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -847,6 +847,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 	struct setup_header *hdr = &boot_params->hdr;
 	const struct linux_efi_initrd *initrd = NULL;
 	unsigned long kernel_entry;
+	unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
+				       ((u64)boot_params->ext_cmd_line_ptr << 32));
 	efi_status_t status;
 
 	boot_params_pointer = boot_params;
@@ -877,6 +879,14 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 		goto fail;
 	}
 
+#ifdef CONFIG_GENERIC_CMDLINE
+	status = efi_handle_builtin_cmdline((char *)cmdline_paddr);
+	if (status != EFI_SUCCESS) {
+		efi_err("Failed to parse options\n");
+		goto fail;
+	}
+#else /* CONFIG_GENERIC_CMDLINE */
+
 #ifdef CONFIG_CMDLINE_BOOL
 	status = efi_parse_options(CONFIG_CMDLINE);
 	if (status != EFI_SUCCESS) {
@@ -885,8 +895,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 	}
 #endif
 	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
-		unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
-					       ((u64)boot_params->ext_cmd_line_ptr << 32));
 		status = efi_parse_options((char *)cmdline_paddr);
 		if (status != EFI_SUCCESS) {
 			efi_err("Failed to parse options\n");
@@ -894,6 +902,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 		}
 	}
 
+#endif
+
 	status = efi_decompress_kernel(&kernel_entry);
 	if (status != EFI_SUCCESS) {
 		efi_err("Failed to decompress kernel\n");
-- 
2.39.2

Re: [PATCH 0/8] generic command line v6

[Andrew Morton]

On Thu,  9 Nov 2023 17:38:04 -0800 Daniel Walker <danielwa@cisco.com> wrote:

> This release is an up-rev of the v5 patches. No additional features have
> been added. Some changes were mode to function names and some changes to
> Kconfig dependencies. Also updated the config conversion for mips.
> 
> There are a number of people who have expressed interest in these
> patches either by asking for them to be merge or testing them. If
> people are so inclined please continue to request them to be merge
> or to ask the status of the next release. It's helpful to motivate me to
> release them again and for the maintainers to see the interest
> generated.
> 
> These patches have been used by Cisco Systems, Inc. on millions of
> released products to great effect. Hopefully they can be used by the
> entire Linux eco system.
> 

fyi, none of the above is suitable for a [0/N] changelog - it's all
transitory stuff which tells readers nothing much about what the
patchset does.

And that info is sorely missed.  I can see that it's a code cleanup,
but I'm sure Cisco wouldn't expend resources to maintain such a thing. 
There's something else here.

In [1/8] I see "Even with mips and powerpc enhancement the needs of
Cisco are not met on these platforms" and "This unified implementation
offers the same functionality needed by Cisco on all platform which we
enable it on".

Well OK, what are these needs?   What functionality changes does this
patchset offer which Cisco finds useful?  IOW, what were the
requirements?  What's wrong with the old code and how does this
patchset fix/enhance that?


I see the patchset updates nothing under Documentation/.  Should it do
so?  Could it do so?


I don't know what is the expected merge patch for this work.  I can
grab them if no other maintainer is in the firing line.

Re: [PATCH 0/8] generic command line v6

[Daniel Walker (danielwa)]

On Thu, Nov 09, 2023 at 05:51:42PM -0800, Andrew Morton wrote:
> On Thu,  9 Nov 2023 17:38:04 -0800 Daniel Walker <danielwa@cisco.com> wrote:
> 
> > This release is an up-rev of the v5 patches. No additional features have
> > been added. Some changes were mode to function names and some changes to
> > Kconfig dependencies. Also updated the config conversion for mips.
> > 
> > There are a number of people who have expressed interest in these
> > patches either by asking for them to be merge or testing them. If
> > people are so inclined please continue to request them to be merge
> > or to ask the status of the next release. It's helpful to motivate me to
> > release them again and for the maintainers to see the interest
> > generated.
> > 
> > These patches have been used by Cisco Systems, Inc. on millions of
> > released products to great effect. Hopefully they can be used by the
> > entire Linux eco system.
> > 
> 
> fyi, none of the above is suitable for a [0/N] changelog - it's all
> transitory stuff which tells readers nothing much about what the
> patchset does.
 
I did not think about it this way. It's because I've submitted this so many
times. I guess I assume everyone knows what it is.

> And that info is sorely missed.  I can see that it's a code cleanup,
> but I'm sure Cisco wouldn't expend resources to maintain such a thing. 
> There's something else here.

I think the prior submissions there was no cover letter, maybe I should just
achoo that entirely.

> In [1/8] I see "Even with mips and powerpc enhancement the needs of
> Cisco are not met on these platforms" and "This unified implementation
> offers the same functionality needed by Cisco on all platform which we
> enable it on".
> 
> Well OK, what are these needs?   What functionality changes does this
> patchset offer which Cisco finds useful?  IOW, what were the
> requirements?  What's wrong with the old code and how does this
> patchset fix/enhance that?

The limitation is that you can't append and prepend to the command line at the
same time in any of the architectures. Having access to both allows OEMs to deal
with broken bootloaders which can't easily be upgraded. Others have responded
that they also use these patches for this same reason.

In 2/8 and 3/8 I modify the insert-sys-cert tool to allow modification of the
command line append and prepend after the build. This allow for an SDK
provided with a binary kernel and for the command line append/prepend to still
be modified identically to how that's done with certificates.

Making all this generic means each platform has a unified set of command line
services. Cisco uses x86/arm32/arm64/mips/powerpc , and it's nice to have all
the same features across platforms.

> 
> I see the patchset updates nothing under Documentation/.  Should it do
> so?  Could it do so?

The only documentation is Kconfig descriptions and commit messages. I suppose it
could have something under Documentation/. The only part which could use more
documentation are the changes in 2/8 and 3/8. That feature is maybe confusing
and has limitations which are maybe not clear. Although the same limitation exist for
inserting certificates.

> 
> I don't know what is the expected merge patch for this work.  I can
> grab them if no other maintainer is in the firing line.

merge patch ? Do you mean merge description ? I think your the maintainer in the
firing line for this one.

Daniel

Re: [PATCH 0/8] generic command line v6

[Andrew Morton]

On Fri, 10 Nov 2023 02:22:27 +0000 "Daniel Walker (danielwa)" <danielwa@cisco.com> wrote:

> On Thu, Nov 09, 2023 at 05:51:42PM -0800, Andrew Morton wrote:
> > On Thu,  9 Nov 2023 17:38:04 -0800 Daniel Walker <danielwa@cisco.com> wrote:
> > 
> > > This release is an up-rev of the v5 patches. No additional features have
> > > been added. Some changes were mode to function names and some changes to
> > > Kconfig dependencies. Also updated the config conversion for mips.
> > > 
> > > There are a number of people who have expressed interest in these
> > > patches either by asking for them to be merge or testing them. If
> > > people are so inclined please continue to request them to be merge
> > > or to ask the status of the next release. It's helpful to motivate me to
> > > release them again and for the maintainers to see the interest
> > > generated.
> > > 
> > > These patches have been used by Cisco Systems, Inc. on millions of
> > > released products to great effect. Hopefully they can be used by the
> > > entire Linux eco system.
> > > 
> > 
> > fyi, none of the above is suitable for a [0/N] changelog - it's all
> > transitory stuff which tells readers nothing much about what the
> > patchset does.
>  
> I did not think about it this way. It's because I've submitted this so many
> times. I guess I assume everyone knows what it is.

This is all on the path to the mainline git history.  Think about how
we want it presented to future readers.  10 years from now nobody will
remember the v5 series email spray.

> 
> > In [1/8] I see "Even with mips and powerpc enhancement the needs of
> > Cisco are not met on these platforms" and "This unified implementation
> > offers the same functionality needed by Cisco on all platform which we
> > enable it on".
> > 
> > Well OK, what are these needs?   What functionality changes does this
> > patchset offer which Cisco finds useful?  IOW, what were the
> > requirements?  What's wrong with the old code and how does this
> > patchset fix/enhance that?
> 
> The limitation is that you can't append and prepend to the command line at the
> same time in any of the architectures. Having access to both allows OEMs to deal
> with broken bootloaders which can't easily be upgraded.

I would never ever have guessed that from the emails I received!

> Others have responded
> that they also use these patches for this same reason.

Citing this info in the [0/N] would be useful.

> In 2/8 and 3/8 I modify the insert-sys-cert tool to allow modification of the
> command line append and prepend after the build. This allow for an SDK
> provided with a binary kernel and for the command line append/prepend to still
> be modified identically to how that's done with certificates.

And this.

> Making all this generic means each platform has a unified set of command line
> services. Cisco uses x86/arm32/arm64/mips/powerpc , and it's nice to have all
> the same features across platforms.

Sounds good.

> > 
> > I see the patchset updates nothing under Documentation/.  Should it do
> > so?  Could it do so?
> 
> The only documentation is Kconfig descriptions and commit messages. I suppose it
> could have something under Documentation/. The only part which could use more
> documentation are the changes in 2/8 and 3/8. That feature is maybe confusing
> and has limitations which are maybe not clear. Although the same limitation exist for
> inserting certificates.

Perhaps the new functionality could be described in
Documentation/admin-guide/kernel-parameters.rst

> > 
> > I don't know what is the expected merge patch for this work.  I can
> > grab them if no other maintainer is in the firing line.
> 
> merge patch ?

"path", sorry.

> Do you mean merge description ? I think your the maintainer in the
> firing line for this one.

OK.  

Re: [PATCH 5/8] drivers: firmware: efi: libstub: enable generic commandline

[kernel test robot]

Hi Daniel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.6]
[cannot apply to arm64/for-next/core efi/next tip/x86/core robh/for-next linus/master next-20231110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Walker/CMDLINE-add-generic-builtin-command-line/20231110-094423
base:   v6.6
patch link:    https://lore.kernel.org/r/20231110013817.2378507-6-danielwa%40cisco.com
patch subject: [PATCH 5/8] drivers: firmware: efi: libstub: enable generic commandline
config: loongarch-randconfig-002-20231110 (https://download.01.org/0day-ci/archive/20231110/202311101224.evyh4zgY-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231110/202311101224.evyh4zgY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311101224.evyh4zgY-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/firmware/efi/libstub/efi-stub-helper.c:43: warning: expecting prototype for efi_handle_cmdline(). Prototype was for efi_handle_builtin_cmdline() instead
   drivers/firmware/efi/libstub/efi-stub-helper.c:592: warning: Function parameter or member 'out' not described in 'efi_load_initrd'


vim +43 drivers/firmware/efi/libstub/efi-stub-helper.c

    32	
    33	/**
    34	 * efi_handle_cmdline() - handle adding in built-in parts of the command line
    35	 * @cmdline:	kernel command line
    36	 *
    37	 * Add in the generic parts of the commandline and start the parsing of the
    38	 * command line.
    39	 *
    40	 * Return:	status code
    41	 */
    42	efi_status_t efi_handle_builtin_cmdline(char const *cmdline)
  > 43	{
    44		efi_status_t status = EFI_SUCCESS;
    45	
    46		if (sizeof(CMDLINE_STATIC_PREPEND) > 1)
    47			status |= efi_parse_options(CMDLINE_STATIC_PREPEND);
    48	
    49		if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
    50			status |= efi_parse_options(cmdline);
    51	
    52		if (sizeof(CMDLINE_STATIC_APPEND) > 1)
    53			status |= efi_parse_options(CMDLINE_STATIC_APPEND);
    54	
    55		if (status != EFI_SUCCESS)
    56			efi_err("Failed to parse options\n");
    57	
    58		return status;
    59	}
    60	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/8] generic command line v6

[Christophe Leroy]

Le 10/11/2023 à 02:38, Daniel Walker a écrit :
> This release is an up-rev of the v5 patches. No additional features have
> been added. Some changes were mode to function names and some changes to
> Kconfig dependencies. Also updated the config conversion for mips.
> 
> There are a number of people who have expressed interest in these
> patches either by asking for them to be merge or testing them. If
> people are so inclined please continue to request them to be merge
> or to ask the status of the next release. It's helpful to motivate me to
> release them again and for the maintainers to see the interest
> generated.

Good, then lets work all together and get the best of it.

Your series has good features like the update of the command line addons 
without rebuild, including updating of the signature when applicable. 
However your series still has weaknesses it had in previous versions. I 
was hopping we could work all together to improve it, but you have been 
repeatedly telling that my proposal doesn't fit your needs without 
explaining why. Can we work in a more win-win spirit this time ?

By the way, for the record, here is a link to a series I sent out as an 
exemple of what it could look like. The intention at that time was not 
to have a concurrent series, but just to show what I meant in my 
comments, because exemples are often more explicit than words. That 
series was considered more mature and closer to a mergeable state than 
yours at that time. Link to the said series: 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?state=*&series=237158

Your series only converts a few architectures, and I can't see any 
demonstration that it will be possible to convert other architectures as 
a follow-up without too much pain.

> 
> These patches have been used by Cisco Systems, Inc. on millions of
> released products to great effect. Hopefully they can be used by the
> entire Linux eco system.

This is not an argument. Billions of computers are running Micr*s*ft 
Windows, does it make Windows better ? Do IBM or Google claim about the 
million equipment they are sending out with special Linux features ? 
Showing objectively what's the benefit of your features without 
referring to Cisco would be better.

> 
> My apologies on the length between releases. I will try to release more
> often.

Christophe

> 
> 
> Daniel Walker (8):
>    CMDLINE: add generic builtin command line
>    scripts: insert-sys-cert: add command line insert capability
>    scripts: insert-sys-cert: change name to insert-symbol
>    CMDLINE: mips: convert to generic builtin command line
>    drivers: firmware: efi: libstub: enable generic commandline
>    CMDLINE: x86: convert to generic builtin command line
>    of: replace command line handling
>    CMDLINE: arm64: convert to generic builtin command line
> 
>   arch/arm64/Kconfig                            |  33 +--
>   arch/arm64/include/asm/setup.h                |   4 +
>   arch/arm64/include/uapi/asm/setup.h           |   2 +
>   arch/arm64/kernel/idreg-override.c            |   9 +-
>   arch/arm64/kernel/pi/kaslr_early.c            |  14 +-
>   arch/mips/Kconfig                             |   4 +-
>   arch/mips/Kconfig.debug                       |  44 ----
>   arch/mips/configs/ar7_defconfig               |  12 +-
>   arch/mips/configs/bcm47xx_defconfig           |  10 +-
>   arch/mips/configs/bcm63xx_defconfig           |  21 +-
>   arch/mips/configs/bmips_be_defconfig          |  17 +-
>   arch/mips/configs/bmips_stb_defconfig         | 139 ++++------
>   arch/mips/configs/ci20_defconfig              |   8 +-
>   arch/mips/configs/cu1000-neo_defconfig        |  19 +-
>   arch/mips/configs/cu1830-neo_defconfig        |  19 +-
>   arch/mips/configs/generic_defconfig           |  15 +-
>   arch/mips/configs/gpr_defconfig               |  33 +--
>   arch/mips/configs/loongson3_defconfig         |  29 +--
>   arch/mips/include/asm/setup.h                 |   2 +
>   arch/mips/kernel/relocate.c                   |  17 +-
>   arch/mips/kernel/setup.c                      |  36 +--
>   arch/mips/pic32/pic32mzda/early_console.c     |   2 +-
>   arch/mips/pic32/pic32mzda/init.c              |   3 +-
>   arch/x86/Kconfig                              |  44 +---
>   arch/x86/kernel/setup.c                       |  18 +-
>   .../firmware/efi/libstub/efi-stub-helper.c    |  29 +++
>   drivers/firmware/efi/libstub/efi-stub.c       |   9 +
>   drivers/firmware/efi/libstub/efistub.h        |   1 +
>   drivers/firmware/efi/libstub/x86-stub.c       |  14 +-
>   drivers/of/fdt.c                              |  22 +-
>   include/linux/cmdline.h                       | 137 ++++++++++
>   init/Kconfig                                  |  79 ++++++
>   lib/Kconfig                                   |   4 +
>   lib/Makefile                                  |   3 +
>   lib/generic_cmdline.S                         |  53 ++++
>   lib/test_cmdline1.c                           | 139 ++++++++++
>   scripts/Makefile                              |   2 +-
>   .../{insert-sys-cert.c => insert-symbol.c}    | 243 ++++++++++++------
>   38 files changed, 807 insertions(+), 482 deletions(-)
>   create mode 100644 include/linux/cmdline.h
>   create mode 100644 lib/generic_cmdline.S
>   create mode 100644 lib/test_cmdline1.c
>   rename scripts/{insert-sys-cert.c => insert-symbol.c} (72%)
> 

Re: [PATCH 5/8] drivers: firmware: efi: libstub: enable generic commandline

[Christophe Leroy]

Le 10/11/2023 à 02:38, Daniel Walker a écrit :
> This adds code to handle the generic command line changes.
> The efi code appears that it doesn't benefit as much from this design
> as it could.

So what can we do to improve that ?


> 
> For example, if you had a prepend command line with "nokaslr" then
> you might be helpful to re-enable it in the boot loader or dts,

s/you/it

> but there appears to be no way to re-enable kaslr or some of the
> other options.
> 
> The efi command line handling is incorrect. x86 and arm have an append
> system however the efi code prepends the command line.
> 
> For example, you could have a non-upgradable bios which sends
> 
> efi=disable_early_pci_dma
> 
> This hypothetically could have been set because early pci dma caused
> issues on early versions of the product.
> 
> Then later the early pci dma was made to work and the company desired
> to start using it. To override the bios you could set the CONFIG_CMDLINE
> to,
> 
> efi=no_disable_early_pci_dma
> 
> then parsing would normally start with the bios command line, then move
> to the CONFIG_CMDLINE and you would end up with early pci dma turned on.
> 
> however, current efi code keeps early pci dma off because the bios
> arguments always override the built in.
> 
> Per my reading this is different from the main body of x86, arm, and
> arm64.
> 
> The generic command line provides both append and prepend, so it
> alleviates this issue if it's used. However not all architectures use
> it.
> 
> It would be desirable to allow the efi stub to have it's builtin command
> line to be modified after compile, but I don't see a feasible way to do
> that currently.

Would be desirable or is desirable ? Your explanations are unclear.


> 
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Daniel Walker <danielwa@cisco.com>
> ---
>   .../firmware/efi/libstub/efi-stub-helper.c    | 29 +++++++++++++++++++
>   drivers/firmware/efi/libstub/efi-stub.c       |  9 ++++++
>   drivers/firmware/efi/libstub/efistub.h        |  1 +
>   drivers/firmware/efi/libstub/x86-stub.c       | 14 +++++++--
>   4 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index bfa30625f5d0..952fa2cdff51 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -11,6 +11,7 @@
>   
>   #include <linux/efi.h>
>   #include <linux/kernel.h>
> +#include <linux/cmdline.h>
>   #include <asm/efi.h>
>   #include <asm/setup.h>
>   
> @@ -29,6 +30,34 @@ bool __pure __efi_soft_reserve_enabled(void)
>   	return !efi_nosoftreserve;
>   }
>   
> +/**
> + * efi_handle_cmdline() - handle adding in built-in parts of the command line
> + * @cmdline:	kernel command line
> + *
> + * Add in the generic parts of the commandline and start the parsing of the
> + * command line.
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_handle_builtin_cmdline(char const *cmdline)
> +{
> +	efi_status_t status = EFI_SUCCESS;
> +
> +	if (sizeof(CMDLINE_STATIC_PREPEND) > 1)
> +		status |= efi_parse_options(CMDLINE_STATIC_PREPEND);
> +
> +	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
> +		status |= efi_parse_options(cmdline);
> +
> +	if (sizeof(CMDLINE_STATIC_APPEND) > 1)
> +		status |= efi_parse_options(CMDLINE_STATIC_APPEND);
> +
> +	if (status != EFI_SUCCESS)
> +		efi_err("Failed to parse options\n");
> +
> +	return status;
> +}
> +
>   /**
>    * efi_parse_options() - Parse EFI command line options
>    * @cmdline:	kernel command line
> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
> index f9c1e8a2bd1d..770abe95c0ee 100644
> --- a/drivers/firmware/efi/libstub/efi-stub.c
> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> @@ -127,6 +127,14 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>   		return EFI_OUT_OF_RESOURCES;
>   	}
>   
> +#ifdef CONFIG_GENERIC_CMDLINE
> +	status = efi_handle_builtin_cmdline(cmdline);
> +	if (status != EFI_SUCCESS) {
> +		goto fail_free_cmdline;
> +	}
> +#endif
> +
> +#ifdef CONFIG_CMDLINE
>   	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
>   	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
>   	    cmdline_size == 0) {
> @@ -144,6 +152,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>   			goto fail_free_cmdline;
>   		}
>   	}
> +#endif
>   
>   	*cmdline_ptr = cmdline;
>   	return EFI_SUCCESS;
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 212687c30d79..1ac6631905c5 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -996,6 +996,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
>   				 unsigned long alignment,
>   				 unsigned long min_addr);
>   
> +efi_status_t efi_handle_builtin_cmdline(char const *cmdline);
>   efi_status_t efi_parse_options(char const *cmdline);
>   
>   void efi_parse_option_graphics(char *option);
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 9d5df683f882..273a8a9c8bbb 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -847,6 +847,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>   	struct setup_header *hdr = &boot_params->hdr;
>   	const struct linux_efi_initrd *initrd = NULL;
>   	unsigned long kernel_entry;
> +	unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> +				       ((u64)boot_params->ext_cmd_line_ptr << 32));
>   	efi_status_t status;
>   
>   	boot_params_pointer = boot_params;
> @@ -877,6 +879,14 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>   		goto fail;
>   	}
>   
> +#ifdef CONFIG_GENERIC_CMDLINE
> +	status = efi_handle_builtin_cmdline((char *)cmdline_paddr);
> +	if (status != EFI_SUCCESS) {
> +		efi_err("Failed to parse options\n");
> +		goto fail;
> +	}
> +#else /* CONFIG_GENERIC_CMDLINE */
> +
>   #ifdef CONFIG_CMDLINE_BOOL
>   	status = efi_parse_options(CONFIG_CMDLINE);
>   	if (status != EFI_SUCCESS) {
> @@ -885,8 +895,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>   	}
>   #endif
>   	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
> -		unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> -					       ((u64)boot_params->ext_cmd_line_ptr << 32));
>   		status = efi_parse_options((char *)cmdline_paddr);
>   		if (status != EFI_SUCCESS) {
>   			efi_err("Failed to parse options\n");
> @@ -894,6 +902,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>   		}
>   	}
>   
> +#endif
> +
>   	status = efi_decompress_kernel(&kernel_entry);
>   	if (status != EFI_SUCCESS) {
>   		efi_err("Failed to decompress kernel\n");

Re: [PATCH 5/8] drivers: firmware: efi: libstub: enable generic commandline

[Ard Biesheuvel]

On Fri, 10 Nov 2023 at 02:39, Daniel Walker <danielwa@cisco.com> wrote:
>
> This adds code to handle the generic command line changes.
> The efi code appears that it doesn't benefit as much from this design
> as it could.
>
> For example, if you had a prepend command line with "nokaslr" then
> you might be helpful to re-enable it in the boot loader or dts,
> but there appears to be no way to re-enable kaslr or some of the
> other options.
>
> The efi command line handling is incorrect. x86 and arm have an append
> system however the efi code prepends the command line.
>
> For example, you could have a non-upgradable bios which sends
>
> efi=disable_early_pci_dma
>
> This hypothetically could have been set because early pci dma caused
> issues on early versions of the product.
>
> Then later the early pci dma was made to work and the company desired
> to start using it. To override the bios you could set the CONFIG_CMDLINE
> to,
>
> efi=no_disable_early_pci_dma
>
> then parsing would normally start with the bios command line, then move
> to the CONFIG_CMDLINE and you would end up with early pci dma turned on.
>
> however, current efi code keeps early pci dma off because the bios
> arguments always override the built in.
>
> Per my reading this is different from the main body of x86, arm, and
> arm64.
>
> The generic command line provides both append and prepend, so it
> alleviates this issue if it's used. However not all architectures use
> it.
>
> It would be desirable to allow the efi stub to have it's builtin command
> line to be modified after compile, but I don't see a feasible way to do
> that currently.
>
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Daniel Walker <danielwa@cisco.com>

There are quite some 'might be's and 'hypothetical's in this commit log.

Is there an actual use case that you are addressing here? Without
that, this looks like unnecessary churn to me, tbh.

Note that this code executes in the context of platform firmware,
which may be old and buggy so we should be cautious about making
unnecessary changes here.


> ---
>  .../firmware/efi/libstub/efi-stub-helper.c    | 29 +++++++++++++++++++
>  drivers/firmware/efi/libstub/efi-stub.c       |  9 ++++++
>  drivers/firmware/efi/libstub/efistub.h        |  1 +
>  drivers/firmware/efi/libstub/x86-stub.c       | 14 +++++++--
>  4 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index bfa30625f5d0..952fa2cdff51 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -11,6 +11,7 @@
>
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
> +#include <linux/cmdline.h>
>  #include <asm/efi.h>
>  #include <asm/setup.h>
>
> @@ -29,6 +30,34 @@ bool __pure __efi_soft_reserve_enabled(void)
>         return !efi_nosoftreserve;
>  }
>
> +/**
> + * efi_handle_cmdline() - handle adding in built-in parts of the command line
> + * @cmdline:   kernel command line
> + *
> + * Add in the generic parts of the commandline and start the parsing of the
> + * command line.
> + *
> + * Return:     status code
> + */
> +efi_status_t efi_handle_builtin_cmdline(char const *cmdline)
> +{
> +       efi_status_t status = EFI_SUCCESS;
> +
> +       if (sizeof(CMDLINE_STATIC_PREPEND) > 1)
> +               status |= efi_parse_options(CMDLINE_STATIC_PREPEND);
> +
> +       if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
> +               status |= efi_parse_options(cmdline);
> +
> +       if (sizeof(CMDLINE_STATIC_APPEND) > 1)
> +               status |= efi_parse_options(CMDLINE_STATIC_APPEND);
> +
> +       if (status != EFI_SUCCESS)
> +               efi_err("Failed to parse options\n");
> +
> +       return status;
> +}
> +
>  /**
>   * efi_parse_options() - Parse EFI command line options
>   * @cmdline:   kernel command line
> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
> index f9c1e8a2bd1d..770abe95c0ee 100644
> --- a/drivers/firmware/efi/libstub/efi-stub.c
> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> @@ -127,6 +127,14 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>                 return EFI_OUT_OF_RESOURCES;
>         }
>
> +#ifdef CONFIG_GENERIC_CMDLINE
> +       status = efi_handle_builtin_cmdline(cmdline);
> +       if (status != EFI_SUCCESS) {
> +               goto fail_free_cmdline;
> +       }
> +#endif
> +
> +#ifdef CONFIG_CMDLINE
>         if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
>             IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
>             cmdline_size == 0) {
> @@ -144,6 +152,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>                         goto fail_free_cmdline;
>                 }
>         }
> +#endif
>
>         *cmdline_ptr = cmdline;
>         return EFI_SUCCESS;
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 212687c30d79..1ac6631905c5 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -996,6 +996,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
>                                  unsigned long alignment,
>                                  unsigned long min_addr);
>
> +efi_status_t efi_handle_builtin_cmdline(char const *cmdline);
>  efi_status_t efi_parse_options(char const *cmdline);
>
>  void efi_parse_option_graphics(char *option);
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 9d5df683f882..273a8a9c8bbb 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -847,6 +847,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>         struct setup_header *hdr = &boot_params->hdr;
>         const struct linux_efi_initrd *initrd = NULL;
>         unsigned long kernel_entry;
> +       unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> +                                      ((u64)boot_params->ext_cmd_line_ptr << 32));
>         efi_status_t status;
>
>         boot_params_pointer = boot_params;
> @@ -877,6 +879,14 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>                 goto fail;
>         }
>
> +#ifdef CONFIG_GENERIC_CMDLINE
> +       status = efi_handle_builtin_cmdline((char *)cmdline_paddr);
> +       if (status != EFI_SUCCESS) {
> +               efi_err("Failed to parse options\n");
> +               goto fail;
> +       }
> +#else /* CONFIG_GENERIC_CMDLINE */
> +
>  #ifdef CONFIG_CMDLINE_BOOL
>         status = efi_parse_options(CONFIG_CMDLINE);
>         if (status != EFI_SUCCESS) {
> @@ -885,8 +895,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>         }
>  #endif
>         if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
> -               unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> -                                              ((u64)boot_params->ext_cmd_line_ptr << 32));
>                 status = efi_parse_options((char *)cmdline_paddr);
>                 if (status != EFI_SUCCESS) {
>                         efi_err("Failed to parse options\n");
> @@ -894,6 +902,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>                 }
>         }
>
> +#endif
> +
>         status = efi_decompress_kernel(&kernel_entry);
>         if (status != EFI_SUCCESS) {
>                 efi_err("Failed to decompress kernel\n");
> --
> 2.39.2
>
>

Re: [PATCH 5/8] drivers: firmware: efi: libstub: enable generic commandline

[Daniel Walker (danielwa)]

On Tue, Dec 12, 2023 at 10:55:33AM +0100, Ard Biesheuvel wrote:
> On Fri, 10 Nov 2023 at 02:39, Daniel Walker <danielwa@cisco.com> wrote:
> >
> > This adds code to handle the generic command line changes.
> > The efi code appears that it doesn't benefit as much from this design
> > as it could.
> >
> > For example, if you had a prepend command line with "nokaslr" then
> > you might be helpful to re-enable it in the boot loader or dts,
> > but there appears to be no way to re-enable kaslr or some of the
> > other options.
> >
> > The efi command line handling is incorrect. x86 and arm have an append
> > system however the efi code prepends the command line.
> >
> > For example, you could have a non-upgradable bios which sends
> >
> > efi=disable_early_pci_dma
> >
> > This hypothetically could have been set because early pci dma caused
> > issues on early versions of the product.
> >
> > Then later the early pci dma was made to work and the company desired
> > to start using it. To override the bios you could set the CONFIG_CMDLINE
> > to,
> >
> > efi=no_disable_early_pci_dma
> >
> > then parsing would normally start with the bios command line, then move
> > to the CONFIG_CMDLINE and you would end up with early pci dma turned on.
> >
> > however, current efi code keeps early pci dma off because the bios
> > arguments always override the built in.
> >
> > Per my reading this is different from the main body of x86, arm, and
> > arm64.
> >
> > The generic command line provides both append and prepend, so it
> > alleviates this issue if it's used. However not all architectures use
> > it.
> >
> > It would be desirable to allow the efi stub to have it's builtin command
> > line to be modified after compile, but I don't see a feasible way to do
> > that currently.
> >
> > Cc: xe-linux-external@cisco.com
> > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> 
> There are quite some 'might be's and 'hypothetical's in this commit log.
> 
> Is there an actual use case that you are addressing here? Without
> that, this looks like unnecessary churn to me, tbh.
> 
> Note that this code executes in the context of platform firmware,
> which may be old and buggy so we should be cautious about making
> unnecessary changes here.

It's been a while since I wrote this patch description, but there is an example I
provided in the description. If you intend to enable early pci dma the command
line parsing may still disable it because of how the parsing works inside the
efi stub. There is an implementation difference between the efi stub and the
architectures which results in this problem. This was not an issue I observed in
the wild, it was the results of code review of the efi stub.

Maybe the greater issue is that the efi stub seems to have connected itself to
the kernels command line system. If the kernel changes then EFI stub must also
change.

Daniel