[PATCH] x86: Allow built-in command line to work in early kernel init

[PATCH] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

The kernel supports having a command line built into it. Unfortunately this
doesn't work in all cases - the built-in command line is only appended
after we've jumped to the kernel proper, but various parts of the early
boot process also pay attention to the command line.

This patch moves the command line override code from the kernel itself to
the early init code. Unfortunately the kernel can be executed by jumping
to the 16-bit entry point, the UEFI entry point or directly to the 32-bit
entry point, and there is no guarantee that any of these will have access
to data held in the others. As a result, three copies of the command line
will be embedded in the kernel.

This patch also adds a new flag to the xloadflags field of the setup header
in order to allow the 16-bit and UEFI entry points to inform the generic
setup code that the command line has already been appended and so shouldn't
be added once more.

Signed-off-by: Matthew Garrett <mjg59@coreos.com>
---
 Documentation/x86/boot.txt                     |  4 +++
 arch/x86/boot/boot.h                           | 10 ++++++
 arch/x86/boot/cmdline.c                        | 37 +++++++++++++++++++
 arch/x86/boot/compressed/cmdline.c             | 18 +++++++++-
 arch/x86/boot/compressed/eboot.c               |  4 ++-
 arch/x86/boot/compressed/misc.c                |  2 ++
 arch/x86/boot/compressed/misc.h                |  3 +-
 arch/x86/boot/main.c                           |  3 ++
 arch/x86/include/uapi/asm/bootparam.h          |  1 +
 arch/x86/kernel/asm-offsets.c                  |  1 +
 arch/x86/kernel/setup.c                        | 16 ---------
 drivers/firmware/efi/libstub/efi-stub-helper.c | 49 ++++++++++++++++++++++++--
 12 files changed, 127 insertions(+), 21 deletions(-)

diff --git a/Documentation/x86/boot.txt b/Documentation/x86/boot.txt
index ed98366..44fedd3 100644
--- a/Documentation/x86/boot.txt
+++ b/Documentation/x86/boot.txt
@@ -611,6 +611,10 @@ Protocol:       2.12+
   Bit 4 (read): XLF_EFI_KEXEC
 	- If 1, the kernel supports kexec EFI boot with EFI runtime support.
 
+  Bit 5 (reserved): XLF_CMDLINE_APPENDED
+	- If 1, CONFIG_CMDLINE has already been appended to the cmdline.
+	  For internal use only, ignored by bootloaders.
+
 Field name:	cmdline_size
 Type:		read
 Offset/size:	0x238/4
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index bd49ec6..45fc708 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -273,6 +273,7 @@ void intcall(u8 int_no, const struct biosregs *ireg, struct biosregs *oreg);
 /* cmdline.c */
 int __cmdline_find_option(unsigned long cmdline_ptr, const char *option, char *buffer, int bufsize);
 int __cmdline_find_option_bool(unsigned long cmdline_ptr, const char *option);
+int __cmdline_init(unsigned long cmdline_ptr, struct setup_header *hdr);
 static inline int cmdline_find_option(const char *option, char *buffer, int bufsize)
 {
 	unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
@@ -293,6 +294,15 @@ static inline int cmdline_find_option_bool(const char *option)
 	return __cmdline_find_option_bool(cmd_line_ptr, option);
 }
 
+static inline int cmdline_init(void)
+{
+	unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
+
+	if (cmd_line_ptr >= 0x100000)
+		return -1;      /* inaccessible */
+
+	return __cmdline_init(cmd_line_ptr, &boot_params.hdr);
+}
 /* cpu.c, cpucheck.c */
 int check_cpu(int *cpu_level_ptr, int *req_level_ptr, u32 **err_flags_ptr);
 int validate_cpu(void);
diff --git a/arch/x86/boot/cmdline.c b/arch/x86/boot/cmdline.c
index 625d21b..aadc192 100644
--- a/arch/x86/boot/cmdline.c
+++ b/arch/x86/boot/cmdline.c
@@ -14,6 +14,10 @@
 
 #include "boot.h"
 
+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[] = CONFIG_CMDLINE;
+#endif
+
 static inline int myisspace(u8 c)
 {
 	return c <= ' ';	/* Close enough approximation */
@@ -156,3 +160,36 @@ int __cmdline_find_option_bool(unsigned long cmdline_ptr, const char *option)
 
 	return 0;	/* Buffer overrun */
 }
+
+int __cmdline_init(unsigned long cmdline_ptr, struct setup_header *hdr)
+{
+#ifdef CONFIG_CMDLINE_BOOL
+	addr_t cptr;
+	int i = 0;
+
+	if (!cmdline_ptr)
+		return -1;      /* No command line */
+
+	set_fs(cmdline_ptr >> 4);
+	cptr = cmdline_ptr & 0xf;
+
+#ifndef CONFIG_CMDLINE_OVERRIDE
+	while (cptr < 0x10000) {
+		char c = rdfs8(cptr);
+		if (!c) {
+			wrfs8(' ', cptr++);
+			break;
+		}
+		cptr++;
+	}
+#endif /* !CONFIG_CMDLINE_OVERRIDE */
+	while (builtin_cmdline[i] && cptr < 0xffff)
+		wrfs8(builtin_cmdline[i++], cptr++);
+
+	wrfs8('\0', cptr);
+
+	hdr->xloadflags |= XLF_CMDLINE_APPENDED;
+#endif /* CONFIG_CMDLINE_BOOL */
+
+	return 0;
+}
diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index b68e303..e1b4b60 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -1,6 +1,6 @@
 #include "misc.h"
 
-#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
+#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL
 
 static unsigned long fs;
 static inline void set_fs(unsigned long seg)
@@ -12,6 +12,10 @@ static inline char rdfs8(addr_t addr)
 {
 	return *((char *)(fs + addr));
 }
+static inline void wrfs8(u8 v, addr_t addr)
+{
+	*((char *)(fs + addr)) = v;
+}
 #include "../cmdline.c"
 static unsigned long get_cmd_line_ptr(void)
 {
@@ -30,4 +34,16 @@ int cmdline_find_option_bool(const char *option)
 	return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
 }
 
+int cmdline_init(void)
+{
+	if (!(real_mode->hdr.xloadflags & XLF_CMDLINE_APPENDED))
+		return __cmdline_init(get_cmd_line_ptr(), &real_mode->hdr);
+	return 0;
+}
+#else
+int cmdline_init(void)
+{
+#error "BAD"
+	return 0;
+}
 #endif
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index ef17683..cf373c2 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1109,7 +1109,9 @@ struct boot_params *make_boot_params(struct efi_config *c)
 	if (!cmdline_ptr)
 		goto fail;
 	hdr->cmd_line_ptr = (unsigned long)cmdline_ptr;
-
+#ifdef CONFIG_CMDLINE_BOOL
+	hdr->xloadflags |= XLF_CMDLINE_APPENDED;
+#endif
 	hdr->ramdisk_image = 0;
 	hdr->ramdisk_size = 0;
 
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index a950864..dc8a287 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -390,6 +390,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 	lines = real_mode->screen_info.orig_video_lines;
 	cols = real_mode->screen_info.orig_video_cols;
 
+	cmdline_init();
+
 	console_init();
 	debug_putstr("early console in decompress_kernel\n");
 
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 04477d6..9cbec86 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -48,10 +48,11 @@ static inline void debug_putstr(const char *s)
 
 #endif
 
-#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
+#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL
 /* cmdline.c */
 int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
+int cmdline_init(void);
 #endif
 
 
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index fd6c9f2..7c24862 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -137,6 +137,9 @@ void main(void)
 	/* First, copy the boot header into the "zeropage" */
 	copy_boot_params();
 
+	/* Handle built-in command line */
+	cmdline_init();
+
 	/* Initialize the early-boot console */
 	console_init();
 	if (cmdline_find_option_bool("debug"))
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index 89a033a..524498a 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -26,6 +26,7 @@
 #define XLF_EFI_HANDOVER_32		(1<<2)
 #define XLF_EFI_HANDOVER_64		(1<<3)
 #define XLF_EFI_KEXEC			(1<<4)
+#define XLF_CMDLINE_APPENDED		(1<<5)
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 9f6b934..c10ab92 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -68,6 +68,7 @@ void common(void) {
 	OFFSET(BP_kernel_alignment, boot_params, hdr.kernel_alignment);
 	OFFSET(BP_pref_address, boot_params, hdr.pref_address);
 	OFFSET(BP_code32_start, boot_params, hdr.code32_start);
+	OFFSET(BP_xloadflags, boot_params, hdr.xloadflags);
 
 	BLANK();
 	DEFINE(PTREGS_SIZE, sizeof(struct pt_regs));
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 0a2421c..1f68d32 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -232,9 +232,6 @@ unsigned long saved_video_mode;
 #define RAMDISK_LOAD_FLAG		0x4000
 
 static char __initdata command_line[COMMAND_LINE_SIZE];
-#ifdef CONFIG_CMDLINE_BOOL
-static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
-#endif
 
 #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
 struct edd edd;
@@ -968,19 +965,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;
 
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index f07d4a6..54afe1f 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -12,9 +12,14 @@
 
 #include <linux/efi.h>
 #include <asm/efi.h>
+#include <asm/setup.h>
 
 #include "efistub.h"
 
+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[] = CONFIG_CMDLINE;
+#endif
+
 /*
  * Some firmware implementations have problems reading files in one go.
  * A read chunk size of 1MB seems to work for most platforms.
@@ -649,6 +654,21 @@ static u8 *efi_utf16_to_utf8(u8 *dst, const u16 *src, int n)
 	return dst;
 }
 
+static size_t efi_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = strlen(dest);
+	size_t len = strlen(src);
+	size_t res = dsize + len;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count-1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}
+
 /*
  * Convert the unicode UEFI command line to ASCII to pass to kernel.
  * Size of memory allocated return in *cmd_line_len.
@@ -658,14 +678,16 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 			  efi_loaded_image_t *image,
 			  int *cmd_line_len)
 {
+	unsigned long cmdline_addr = 0;
+	int i;
+	efi_status_t status;
+#ifndef CONFIG_CMDLINE_OVERRIDE
 	const u16 *s2;
 	u8 *s1 = NULL;
-	unsigned long cmdline_addr = 0;
 	int load_options_chars = image->load_options_size / 2; /* UTF-16 */
 	const u16 *options = image->load_options;
 	int options_bytes = 0;  /* UTF-8 bytes */
 	int options_chars = 0;  /* UTF-16 chars */
-	efi_status_t status;
 	u16 zero = 0;
 
 	if (options) {
@@ -684,6 +706,12 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 
 	options_bytes++;	/* NUL termination */
 
+#ifdef CONFIG_CMDLINE_BOOL
+	/* Add length of the built-in command line, plus a space */
+	options_bytes += strlen(builtin_cmdline);
+	options_bytes++;
+#endif
+
 	status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr);
 	if (status != EFI_SUCCESS)
 		return NULL;
@@ -694,6 +722,23 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 	s1 = efi_utf16_to_utf8(s1, s2, options_chars);
 	*s1 = '\0';
 
+#ifdef CONFIG_CMDLINE_BOOL
+	efi_strlcat((char *)cmdline_addr, " ", COMMAND_LINE_SIZE);
+	efi_strlcat((char *)cmdline_addr, builtin_cmdline, COMMAND_LINE_SIZE);
+#endif
 	*cmd_line_len = options_bytes;
+#else /* CONFIG_CMDLINE_OVERRIDE */
+	status = efi_low_alloc(sys_table_arg, strlen(builtin_cmdline), 0,
+			       &cmdline_addr);
+	if (status != EFI_SUCCESS)
+		return NULL;
+	while (builtin_cmdline[i] && i < COMMAND_LINE_SIZE) {
+		((char *)cmdline_addr)[i] = builtin_cmdline[i];
+		i++;
+	}
+	((char *)cmdline_addr)[i] = '\0';
+	*cmd_line_len = strlen(builtin_cmdline);
+#endif /* CONFIG_CMDLINE_OVERRIDE */
+
 	return (char *)cmdline_addr;
 }
-- 
2.3.4

[PATCH V2] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

From: Matthew Garrett <mjg59@coreos.com>

The kernel supports having a command line built into it. Unfortunately this
doesn't work in all cases - the built-in command line is only appended
after we've jumped to the kernel proper, but various parts of the early
boot process also pay attention to the command line.

This patch moves the command line override code from the kernel itself to
the early init code. Unfortunately the kernel can be executed by jumping
to the 16-bit entry point, the UEFI entry point, directly to the 32-bit
entry point or even to the entry point of the uncompressed image, and
there is no guarantee that any of these will have access to data held in
the others. As a result, four copies of the command line will be embedded
in the kernel.

This patch also defines a new field in boot_params in order to allow the
earlier entry points to inform the generic setup code that the command line
has already been appended and so shouldn't be added once more.

Signed-off-by: Matthew Garrett <mjg59@coreos.com>
---

V2: Use some empty data in bootparams rather than in the setup header since
there's no need for the bootloader to touch this. Add back the code to
kernel/setup.c because some scenarios may involve skipping the setup code
entirely.

 Documentation/x86/zero-page.txt                |  1 +
 arch/x86/boot/boot.h                           | 10 ++++++
 arch/x86/boot/cmdline.c                        | 37 +++++++++++++++++++
 arch/x86/boot/compressed/cmdline.c             | 18 +++++++++-
 arch/x86/boot/compressed/eboot.c               |  4 ++-
 arch/x86/boot/compressed/misc.c                |  2 ++
 arch/x86/boot/compressed/misc.h                |  3 +-
 arch/x86/boot/main.c                           |  3 ++
 arch/x86/include/uapi/asm/bootparam.h          |  5 ++-
 arch/x86/kernel/setup.c                        | 16 +++++----
 drivers/firmware/efi/libstub/efi-stub-helper.c | 49 ++++++++++++++++++++++++--
 11 files changed, 135 insertions(+), 13 deletions(-)

diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
index 82fbdbc..22eaecf 100644
--- a/Documentation/x86/zero-page.txt
+++ b/Documentation/x86/zero-page.txt
@@ -12,6 +12,7 @@ Offset	Proto	Name		Meaning
 000/040	ALL	screen_info	Text mode or frame buffer information
 				(struct screen_info)
 040/014	ALL	apm_bios_info	APM BIOS information (struct apm_bios_info)
+054/004	ALL	setup_flags	Flags passed from early kernel setup
 058/008	ALL	tboot_addr      Physical address of tboot shared page
 060/010	ALL	ist_info	Intel SpeedStep (IST) BIOS support information
 				(struct ist_info)
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index bd49ec6..3718244 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -273,6 +273,7 @@ void intcall(u8 int_no, const struct biosregs *ireg, struct biosregs *oreg);
 /* cmdline.c */
 int __cmdline_find_option(unsigned long cmdline_ptr, const char *option, char *buffer, int bufsize);
 int __cmdline_find_option_bool(unsigned long cmdline_ptr, const char *option);
+int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params);
 static inline int cmdline_find_option(const char *option, char *buffer, int bufsize)
 {
 	unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
@@ -293,6 +294,15 @@ static inline int cmdline_find_option_bool(const char *option)
 	return __cmdline_find_option_bool(cmd_line_ptr, option);
 }
 
+static inline int cmdline_init(void)
+{
+	unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
+
+	if (cmd_line_ptr >= 0x100000)
+		return -1;      /* inaccessible */
+
+	return __cmdline_init(cmd_line_ptr, &boot_params);
+}
 /* cpu.c, cpucheck.c */
 int check_cpu(int *cpu_level_ptr, int *req_level_ptr, u32 **err_flags_ptr);
 int validate_cpu(void);
diff --git a/arch/x86/boot/cmdline.c b/arch/x86/boot/cmdline.c
index 625d21b..2273d27 100644
--- a/arch/x86/boot/cmdline.c
+++ b/arch/x86/boot/cmdline.c
@@ -14,6 +14,10 @@
 
 #include "boot.h"
 
+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[] = CONFIG_CMDLINE;
+#endif
+
 static inline int myisspace(u8 c)
 {
 	return c <= ' ';	/* Close enough approximation */
@@ -156,3 +160,36 @@ int __cmdline_find_option_bool(unsigned long cmdline_ptr, const char *option)
 
 	return 0;	/* Buffer overrun */
 }
+
+int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params)
+{
+#ifdef CONFIG_CMDLINE_BOOL
+	addr_t cptr;
+	int i = 0;
+
+	if (!cmdline_ptr)
+		return -1;      /* No command line */
+
+	set_fs(cmdline_ptr >> 4);
+	cptr = cmdline_ptr & 0xf;
+
+#ifndef CONFIG_CMDLINE_OVERRIDE
+	while (cptr < 0x10000) {
+		char c = rdfs8(cptr);
+		if (!c) {
+			wrfs8(' ', cptr++);
+			break;
+		}
+		cptr++;
+	}
+#endif /* !CONFIG_CMDLINE_OVERRIDE */
+	while (builtin_cmdline[i] && cptr < 0xffff)
+		wrfs8(builtin_cmdline[i++], cptr++);
+
+	wrfs8('\0', cptr);
+
+	params->setup_flags |= SETUP_CMDLINE_APPENDED;
+#endif /* CONFIG_CMDLINE_BOOL */
+
+	return 0;
+}
diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index b68e303..bf8ea07 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -1,6 +1,6 @@
 #include "misc.h"
 
-#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
+#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL
 
 static unsigned long fs;
 static inline void set_fs(unsigned long seg)
@@ -12,6 +12,10 @@ static inline char rdfs8(addr_t addr)
 {
 	return *((char *)(fs + addr));
 }
+static inline void wrfs8(u8 v, addr_t addr)
+{
+	*((char *)(fs + addr)) = v;
+}
 #include "../cmdline.c"
 static unsigned long get_cmd_line_ptr(void)
 {
@@ -30,4 +34,16 @@ int cmdline_find_option_bool(const char *option)
 	return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
 }
 
+int cmdline_init(void)
+{
+	if (!(real_mode->setup_flags & SETUP_CMDLINE_APPENDED))
+		return __cmdline_init(get_cmd_line_ptr(), real_mode);
+	return 0;
+}
+#else
+int cmdline_init(void)
+{
+#error "BAD"
+	return 0;
+}
 #endif
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index ef17683..15a932c 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1109,7 +1109,9 @@ struct boot_params *make_boot_params(struct efi_config *c)
 	if (!cmdline_ptr)
 		goto fail;
 	hdr->cmd_line_ptr = (unsigned long)cmdline_ptr;
-
+#ifdef CONFIG_CMDLINE_BOOL
+	boot_params->setup_flags |= SETUP_CMDLINE_APPENDED;
+#endif
 	hdr->ramdisk_image = 0;
 	hdr->ramdisk_size = 0;
 
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index a950864..dc8a287 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -390,6 +390,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 	lines = real_mode->screen_info.orig_video_lines;
 	cols = real_mode->screen_info.orig_video_cols;
 
+	cmdline_init();
+
 	console_init();
 	debug_putstr("early console in decompress_kernel\n");
 
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 04477d6..9cbec86 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -48,10 +48,11 @@ static inline void debug_putstr(const char *s)
 
 #endif
 
-#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
+#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL
 /* cmdline.c */
 int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
+int cmdline_init(void);
 #endif
 
 
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index fd6c9f2..7c24862 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -137,6 +137,9 @@ void main(void)
 	/* First, copy the boot header into the "zeropage" */
 	copy_boot_params();
 
+	/* Handle built-in command line */
+	cmdline_init();
+
 	/* Initialize the early-boot console */
 	console_init();
 	if (cmdline_find_option_bool("debug"))
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index 89a033a..2dfb945 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -27,6 +27,9 @@
 #define XLF_EFI_HANDOVER_64		(1<<3)
 #define XLF_EFI_KEXEC			(1<<4)
 
+/* setup_flags */
+#define SETUP_CMDLINE_APPENDED		(1<<0)
+
 #ifndef __ASSEMBLY__
 
 #include <linux/types.h>
@@ -114,7 +117,7 @@ struct efi_info {
 struct boot_params {
 	struct screen_info screen_info;			/* 0x000 */
 	struct apm_bios_info apm_bios_info;		/* 0x040 */
-	__u8  _pad2[4];					/* 0x054 */
+	__u32 setup_flags;				/* 0x054 */
 	__u64  tboot_addr;				/* 0x058 */
 	struct ist_info ist_info;			/* 0x060 */
 	__u8  _pad3[16];				/* 0x070 */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 0a2421c..b0d50b1 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -969,16 +969,18 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.end = __pa_symbol(__bss_stop)-1;
 
 #ifdef CONFIG_CMDLINE_BOOL
+	if (!(boot_params.setup_flags & SETUP_CMDLINE_APPENDED)) {
 #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);
-	}
+#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);
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index f07d4a6..54afe1f 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -12,9 +12,14 @@
 
 #include <linux/efi.h>
 #include <asm/efi.h>
+#include <asm/setup.h>
 
 #include "efistub.h"
 
+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[] = CONFIG_CMDLINE;
+#endif
+
 /*
  * Some firmware implementations have problems reading files in one go.
  * A read chunk size of 1MB seems to work for most platforms.
@@ -649,6 +654,21 @@ static u8 *efi_utf16_to_utf8(u8 *dst, const u16 *src, int n)
 	return dst;
 }
 
+static size_t efi_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = strlen(dest);
+	size_t len = strlen(src);
+	size_t res = dsize + len;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count-1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}
+
 /*
  * Convert the unicode UEFI command line to ASCII to pass to kernel.
  * Size of memory allocated return in *cmd_line_len.
@@ -658,14 +678,16 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 			  efi_loaded_image_t *image,
 			  int *cmd_line_len)
 {
+	unsigned long cmdline_addr = 0;
+	int i;
+	efi_status_t status;
+#ifndef CONFIG_CMDLINE_OVERRIDE
 	const u16 *s2;
 	u8 *s1 = NULL;
-	unsigned long cmdline_addr = 0;
 	int load_options_chars = image->load_options_size / 2; /* UTF-16 */
 	const u16 *options = image->load_options;
 	int options_bytes = 0;  /* UTF-8 bytes */
 	int options_chars = 0;  /* UTF-16 chars */
-	efi_status_t status;
 	u16 zero = 0;
 
 	if (options) {
@@ -684,6 +706,12 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 
 	options_bytes++;	/* NUL termination */
 
+#ifdef CONFIG_CMDLINE_BOOL
+	/* Add length of the built-in command line, plus a space */
+	options_bytes += strlen(builtin_cmdline);
+	options_bytes++;
+#endif
+
 	status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr);
 	if (status != EFI_SUCCESS)
 		return NULL;
@@ -694,6 +722,23 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 	s1 = efi_utf16_to_utf8(s1, s2, options_chars);
 	*s1 = '\0';
 
+#ifdef CONFIG_CMDLINE_BOOL
+	efi_strlcat((char *)cmdline_addr, " ", COMMAND_LINE_SIZE);
+	efi_strlcat((char *)cmdline_addr, builtin_cmdline, COMMAND_LINE_SIZE);
+#endif
 	*cmd_line_len = options_bytes;
+#else /* CONFIG_CMDLINE_OVERRIDE */
+	status = efi_low_alloc(sys_table_arg, strlen(builtin_cmdline), 0,
+			       &cmdline_addr);
+	if (status != EFI_SUCCESS)
+		return NULL;
+	while (builtin_cmdline[i] && i < COMMAND_LINE_SIZE) {
+		((char *)cmdline_addr)[i] = builtin_cmdline[i];
+		i++;
+	}
+	((char *)cmdline_addr)[i] = '\0';
+	*cmd_line_len = strlen(builtin_cmdline);
+#endif /* CONFIG_CMDLINE_OVERRIDE */
+
 	return (char *)cmdline_addr;
 }
-- 
2.3.4

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

Any feedback on this?

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Ingo Molnar]

* Matthew Garrett <matthew.garrett@coreos.com> wrote:

> Any feedback on this?

So I worry about:

 - the fragmented nature of it: lots of non-obvious pieces of code 
   will now be scattered all around arch/x86/.

 - the crazy #ifdefs scattered around

 - the various pieces of data scattered around

... so while I don't mind the feature in principle, but it should be 
centralized much better IMHO, made Kconfig invariant at least in its 
fragmented callbacks, with only the very strictly boot method specific 
details spelled out explicitly.

I have not thought much about how to achieve that :-/

Thanks,

	Ingo

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

On Tue, May 12, 2015 at 2:28 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Matthew Garrett <matthew.garrett@coreos.com> wrote:
>
>> Any feedback on this?
>
> So I worry about:
>
>  - the fragmented nature of it: lots of non-obvious pieces of code
>    will now be scattered all around arch/x86/.

We've got four different entry points and they all work under very
different constraints - I can't see any real way to share this code.
We could move all the command line handling code into a single file
and #include it with different ifdefs, but I'm not sure that that's
less confusing?

>  - the crazy #ifdefs scattered around

The #ifdefs could be abstracted behind a couple of simple helper functions?

>  - the various pieces of data scattered around

We're trying to use the same data in what are pretty much four
logically distinct codebases. I'm not sure that there's any way around
that other than linker magic, and that seems like it'd make things
even more non-obvious.

> ... so while I don't mind the feature in principle, but it should be
> centralized much better IMHO, made Kconfig invariant at least in its
> fragmented callbacks, with only the very strictly boot method specific
> details spelled out explicitly.
>
> I have not thought much about how to achieve that :-/

I'm happy to reimplement this, but outside losing the #ifdefs I'm
struggling to see any real way of improving it.

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

Ok, maybe something more like this? It even gets rid of some of the
#ifdefs in setup.c.

diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
index 82fbdbc..22eaecf 100644
--- a/Documentation/x86/zero-page.txt
+++ b/Documentation/x86/zero-page.txt
@@ -12,6 +12,7 @@ Offset    Proto    Name        Meaning
 000/040    ALL    screen_info    Text mode or frame buffer information
                 (struct screen_info)
 040/014    ALL    apm_bios_info    APM BIOS information (struct apm_bios_info)
+054/004    ALL    setup_flags    Flags passed from early kernel setup
 058/008    ALL    tboot_addr      Physical address of tboot shared page
 060/010    ALL    ist_info    Intel SpeedStep (IST) BIOS support information
                 (struct ist_info)
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index bd49ec6..3718244 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -273,6 +273,7 @@ void intcall(u8 int_no, const struct biosregs
*ireg, struct biosregs *oreg);
 /* cmdline.c */
 int __cmdline_find_option(unsigned long cmdline_ptr, const char
*option, char *buffer, int bufsize);
 int __cmdline_find_option_bool(unsigned long cmdline_ptr, const char *option);
+int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params);
 static inline int cmdline_find_option(const char *option, char
*buffer, int bufsize)
 {
     unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
@@ -293,6 +294,15 @@ static inline int cmdline_find_option_bool(const
char *option)
     return __cmdline_find_option_bool(cmd_line_ptr, option);
 }

+static inline int cmdline_init(void)
+{
+    unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
+
+    if (cmd_line_ptr >= 0x100000)
+        return -1;      /* inaccessible */
+
+    return __cmdline_init(cmd_line_ptr, &boot_params);
+}
 /* cpu.c, cpucheck.c */
 int check_cpu(int *cpu_level_ptr, int *req_level_ptr, u32 **err_flags_ptr);
 int validate_cpu(void);
diff --git a/arch/x86/boot/cmdline.c b/arch/x86/boot/cmdline.c
index 625d21b..9ddffd0 100644
--- a/arch/x86/boot/cmdline.c
+++ b/arch/x86/boot/cmdline.c
@@ -12,8 +12,15 @@
  * Simple command-line parser for early boot.
  */

+#include <asm/cmdline_append.h>
 #include "boot.h"

+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[] = CONFIG_CMDLINE;
+#else
+static char *builtin_cmdline;
+#endif
+
 static inline int myisspace(u8 c)
 {
     return c <= ' ';    /* Close enough approximation */
@@ -156,3 +163,38 @@ int __cmdline_find_option_bool(unsigned long
cmdline_ptr, const char *option)

     return 0;    /* Buffer overrun */
 }
+
+int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params)
+{
+    addr_t cptr;
+    int i = 0;
+
+    if (!builtin_cmdline)
+        return 0;
+
+    if (!cmdline_ptr)
+        return -1;      /* No command line */
+
+    set_fs(cmdline_ptr >> 4);
+    cptr = cmdline_ptr & 0xf;
+
+    if (append_cmdline) {
+        while (cptr < 0x10000) {
+            char c = rdfs8(cptr);
+            if (!c) {
+                wrfs8(' ', cptr++);
+                break;
+            }
+            cptr++;
+        }
+    }
+
+    while (builtin_cmdline[i] && cptr < 0xffff)
+        wrfs8(builtin_cmdline[i++], cptr++);
+
+    wrfs8('\0', cptr);
+
+    params->setup_flags |= SETUP_CMDLINE_APPENDED;
+
+    return 0;
+}
diff --git a/arch/x86/boot/compressed/cmdline.c
b/arch/x86/boot/compressed/cmdline.c
index b68e303..4348828 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -1,6 +1,6 @@
 #include "misc.h"

-#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
+#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL

 static unsigned long fs;
 static inline void set_fs(unsigned long seg)
@@ -12,6 +12,10 @@ static inline char rdfs8(addr_t addr)
 {
     return *((char *)(fs + addr));
 }
+static inline void wrfs8(u8 v, addr_t addr)
+{
+    *((char *)(fs + addr)) = v;
+}
 #include "../cmdline.c"
 static unsigned long get_cmd_line_ptr(void)
 {
@@ -30,4 +34,15 @@ int cmdline_find_option_bool(const char *option)
     return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
 }

+int cmdline_init(void)
+{
+    if (!(real_mode->setup_flags & SETUP_CMDLINE_APPENDED))
+        return __cmdline_init(get_cmd_line_ptr(), real_mode);
+    return 0;
+}
+#else
+int cmdline_init(void)
+{
+    return 0;
+}
 #endif
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 48304b8..e44146d 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -9,6 +9,7 @@

 #include <linux/efi.h>
 #include <linux/pci.h>
+#include <asm/cmdline_append.h>
 #include <asm/efi.h>
 #include <asm/setup.h>
 #include <asm/desc.h>
@@ -1112,6 +1113,9 @@ struct boot_params *make_boot_params(struct efi_config *c)
     /* Fill in upper bits of command line address, NOP on 32 bit  */
     boot_params->ext_cmd_line_ptr = (u64)(unsigned long)cmdline_ptr >> 32;

+    if (append_cmdline)
+        boot_params->setup_flags |= SETUP_CMDLINE_APPENDED;
+
     hdr->ramdisk_image = 0;
     hdr->ramdisk_size = 0;

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index a107b93..832c9a8 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -393,6 +393,8 @@ asmlinkage __visible void *decompress_kernel(void
*rmode, memptr heap,
     lines = real_mode->screen_info.orig_video_lines;
     cols = real_mode->screen_info.orig_video_cols;

+    cmdline_init();
+
     console_init();
     debug_putstr("early console in decompress_kernel\n");

diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 89dd0d7..1584397 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -48,10 +48,11 @@ static inline void debug_putstr(const char *s)

 #endif

-#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
+#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL
 /* cmdline.c */
 int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
+int cmdline_init(void);
 #endif


diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index fd6c9f2..7c24862 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -137,6 +137,9 @@ void main(void)
     /* First, copy the boot header into the "zeropage" */
     copy_boot_params();

+    /* Handle built-in command line */
+    cmdline_init();
+
     /* Initialize the early-boot console */
     console_init();
     if (cmdline_find_option_bool("debug"))
diff --git a/arch/x86/include/asm/cmdline_append.h
b/arch/x86/include/asm/cmdline_append.h
new file mode 100644
index 0000000..a57bf7e
--- /dev/null
+++ b/arch/x86/include/asm/cmdline_append.h
@@ -0,0 +1,10 @@
+#ifndef _ASM_X86_CMDLINE_APPEND_H
+#define _ASM_X86_CMDLINE_APPEND_H
+
+#ifdef CONFIG_CMDLINE_OVERRIDE
+static char append_cmdline;
+#else
+static char append_cmdline = 1;
+#endif
+
+#endif /* _ASM_X86_CMDLINE_APPEND_H */
diff --git a/arch/x86/include/uapi/asm/bootparam.h
b/arch/x86/include/uapi/asm/bootparam.h
index ab456dc..3fda079 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -27,6 +27,9 @@
 #define XLF_EFI_HANDOVER_64        (1<<3)
 #define XLF_EFI_KEXEC            (1<<4)

+/* setup_flags */
+#define SETUP_CMDLINE_APPENDED        (1<<0)
+
 #ifndef __ASSEMBLY__

 #include <linux/types.h>
@@ -114,7 +117,7 @@ struct efi_info {
 struct boot_params {
     struct screen_info screen_info;            /* 0x000 */
     struct apm_bios_info apm_bios_info;        /* 0x040 */
-    __u8  _pad2[4];                    /* 0x054 */
+    __u32 setup_flags;                /* 0x054 */
     __u64  tboot_addr;                /* 0x058 */
     struct ist_info ist_info;            /* 0x060 */
     __u8  _pad3[16];                /* 0x070 */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index d74ac33..4c6456d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -112,6 +112,7 @@
 #include <asm/alternative.h>
 #include <asm/prom.h>

+#include <asm/cmdline_append.h>
 /*
  * max_low_pfn_mapped: highest direct mapped pfn under 4GB
  * max_pfn_mapped:     highest direct mapped pfn over 4GB
@@ -234,6 +235,8 @@ unsigned long saved_video_mode;
 static char __initdata command_line[COMMAND_LINE_SIZE];
 #ifdef CONFIG_CMDLINE_BOOL
 static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
+#else
+static char *builtin_cmdline;
 #endif

 #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
@@ -973,18 +976,21 @@ 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);
+    if (builtin_cmdline) {
+        /* Fix up the command line if an earlier stage hasn't */
+        if (!(boot_params.setup_flags & SETUP_CMDLINE_APPENDED)) {
+            if (append_cmdline) {
+                /* 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;
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c
b/drivers/firmware/efi/libstub/efi-stub-helper.c
index f07d4a6..743e4a5 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -12,9 +12,22 @@

 #include <linux/efi.h>
 #include <asm/efi.h>
+#include <asm/setup.h>

 #include "efistub.h"

+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[] = CONFIG_CMDLINE;
+#else
+static char* builtin_cmdline;
+#endif
+
+#ifdef CONFIG_CMDLINE_OVERRIDE
+static bool append_cmdline;
+#else
+static bool append_cmdline = true;
+#endif
+
 /*
  * Some firmware implementations have problems reading files in one go.
  * A read chunk size of 1MB seems to work for most platforms.
@@ -649,6 +662,21 @@ static u8 *efi_utf16_to_utf8(u8 *dst, const u16
*src, int n)
     return dst;
 }

+static size_t efi_strlcat(char *dest, const char *src, size_t count)
+{
+    size_t dsize = strlen(dest);
+    size_t len = strlen(src);
+    size_t res = dsize + len;
+
+    dest += dsize;
+    count -= dsize;
+    if (len >= count)
+        len = count-1;
+    memcpy(dest, src, len);
+    dest[len] = 0;
+    return res;
+}
+
 /*
  * Convert the unicode UEFI command line to ASCII to pass to kernel.
  * Size of memory allocated return in *cmd_line_len.
@@ -658,42 +686,72 @@ char *efi_convert_cmdline(efi_system_table_t
*sys_table_arg,
               efi_loaded_image_t *image,
               int *cmd_line_len)
 {
+    unsigned long cmdline_addr = 0;
+    int i;
+    efi_status_t status;
     const u16 *s2;
     u8 *s1 = NULL;
-    unsigned long cmdline_addr = 0;
     int load_options_chars = image->load_options_size / 2; /* UTF-16 */
     const u16 *options = image->load_options;
     int options_bytes = 0;  /* UTF-8 bytes */
     int options_chars = 0;  /* UTF-16 chars */
-    efi_status_t status;
     u16 zero = 0;

-    if (options) {
-        s2 = options;
-        while (*s2 && *s2 != '\n'
-               && options_chars < load_options_chars) {
-            options_bytes += efi_utf8_bytes(*s2++);
-            options_chars++;
+    if (!builtin_cmdline || append_cmdline) {
+        if (options) {
+            s2 = options;
+            while (*s2 && *s2 != '\n'
+                   && options_chars < load_options_chars) {
+                options_bytes += efi_utf8_bytes(*s2++);
+                options_chars++;
+            }
+        }
+
+        if (!options_chars) {
+            /* No command line options, so return empty string*/
+            options = &zero;
         }
-    }

-    if (!options_chars) {
-        /* No command line options, so return empty string*/
-        options = &zero;
-    }
+        options_bytes++;    /* NUL termination */

-    options_bytes++;    /* NUL termination */
+        if (builtin_cmdline) {
+            /* Add length of the built-in command line + space */
+            options_bytes += strlen(builtin_cmdline);
+            options_bytes++;
+        }

-    status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr);
-    if (status != EFI_SUCCESS)
-        return NULL;
+        status = efi_low_alloc(sys_table_arg, options_bytes, 0,
+                       &cmdline_addr);

-    s1 = (u8 *)cmdline_addr;
-    s2 = (const u16 *)options;
+        if (status != EFI_SUCCESS)
+            return NULL;

-    s1 = efi_utf16_to_utf8(s1, s2, options_chars);
-    *s1 = '\0';
+        s1 = (u8 *)cmdline_addr;
+        s2 = (const u16 *)options;
+
+        s1 = efi_utf16_to_utf8(s1, s2, options_chars);
+        *s1 = '\0';
+
+        if (builtin_cmdline) {
+            efi_strlcat((char *)cmdline_addr, " ",
+                    COMMAND_LINE_SIZE);
+            efi_strlcat((char *)cmdline_addr, builtin_cmdline,
+                    COMMAND_LINE_SIZE);
+        }
+        *cmd_line_len = options_bytes;
+    } else {
+        /* Ignore the provided command line, use the built-in one */
+        status = efi_low_alloc(sys_table_arg, strlen(builtin_cmdline),
+                       0, &cmdline_addr);
+        if (status != EFI_SUCCESS)
+            return NULL;
+        while (builtin_cmdline[i] && i < COMMAND_LINE_SIZE) {
+            ((char *)cmdline_addr)[i] = builtin_cmdline[i];
+            i++;
+        }
+        ((char *)cmdline_addr)[i] = '\0';
+        *cmd_line_len = strlen(builtin_cmdline);
+    }

-    *cmd_line_len = options_bytes;
     return (char *)cmdline_addr;
 }

-- 
Matthew Garrett | matthew.garrett@coreos.com

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Yinghai Lu]

On Tue, May 12, 2015 at 11:42 AM, Matthew Garrett
<matthew.garrett@coreos.com> wrote:
> Ok, maybe something more like this? It even gets rid of some of the
> #ifdefs in setup.c.
>
> diff --git a/arch/x86/boot/cmdline.c b/arch/x86/boot/cmdline.c
> index 625d21b..9ddffd0 100644
> --- a/arch/x86/boot/cmdline.c
> +++ b/arch/x86/boot/cmdline.c
> @@ -12,8 +12,15 @@
>   * Simple command-line parser for early boot.
>   */
>
> +#include <asm/cmdline_append.h>
>  #include "boot.h"
>
> +#ifdef CONFIG_CMDLINE_BOOL
> +static char builtin_cmdline[] = CONFIG_CMDLINE;
> +#else
> +static char *builtin_cmdline;
> +#endif
> +
>  static inline int myisspace(u8 c)
>  {
>      return c <= ' ';    /* Close enough approximation */
> @@ -156,3 +163,38 @@ int __cmdline_find_option_bool(unsigned long
> cmdline_ptr, const char *option)
>
>      return 0;    /* Buffer overrun */
>  }
> +
> +int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params)
> +{
> +    addr_t cptr;
> +    int i = 0;
> +
> +    if (!builtin_cmdline)
> +        return 0;
> +
> +    if (!cmdline_ptr)
> +        return -1;      /* No command line */
> +
> +    set_fs(cmdline_ptr >> 4);
> +    cptr = cmdline_ptr & 0xf;
> +
> +    if (append_cmdline) {
> +        while (cptr < 0x10000) {
> +            char c = rdfs8(cptr);
> +            if (!c) {
> +                wrfs8(' ', cptr++);
> +                break;
> +            }
> +            cptr++;
> +        }
> +    }
> +
> +    while (builtin_cmdline[i] && cptr < 0xffff)
> +        wrfs8(builtin_cmdline[i++], cptr++);
> +
> +    wrfs8('\0', cptr);
> +
> +    params->setup_flags |= SETUP_CMDLINE_APPENDED;
> +
> +    return 0;
> +}

You can not assume that you can use buffer after cmd_line from bootloader
blindly.

Thanks

Yinghai

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

On Tue, May 12, 2015 at 04:41:22PM -0700, Yinghai Lu wrote:

> You can not assume that you can use buffer after cmd_line from bootloader
> blindly.

Are there any bootloaders that don't allocate a buffer of the maximum 
command line size?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[H. Peter Anvin]

On 05/12/2015 04:53 PM, Matthew Garrett wrote:
> On Tue, May 12, 2015 at 04:41:22PM -0700, Yinghai Lu wrote:
> 
>> You can not assume that you can use buffer after cmd_line from bootloader
>> blindly.
> 
> Are there any bootloaders that don't allocate a buffer of the maximum 
> command line size?
> 

YES!  In fact, it is quite common for the command line to be dynamically
allocated.

	-hpa

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

On Tue, May 12, 2015 at 07:46:11PM -0700, H. Peter Anvin wrote:
> On 05/12/2015 04:53 PM, Matthew Garrett wrote:
> > On Tue, May 12, 2015 at 04:41:22PM -0700, Yinghai Lu wrote:
> > 
> >> You can not assume that you can use buffer after cmd_line from bootloader
> >> blindly.
> > 
> > Are there any bootloaders that don't allocate a buffer of the maximum 
> > command line size?
> > 
> 
> YES!  In fact, it is quite common for the command line to be dynamically
> allocated.

Hmm. Well, this is a little awkward. Let me think about it.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH V2] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

The kernel supports having a command line built into it. Unfortunately
this doesn't work in all cases - the built-in command line is only
appended after we've jumped to the kernel proper, but various parts of
the early boot process also pay attention to the command line.

This patch moves the command line override code from the kernel itself to
the early init code. Unfortunately the kernel can be executed by jumping
to the 16-bit entry point, the UEFI entry point or directly to the 32-bit
entry point, and there is no guarantee that any of these will have access
to data held in the others. As a result, three copies of the command line
will be embedded in the kernel.

This patch also adds a new flag to the xloadflags field of the setup header
in order to allow the entry points to inform the generic setup code that the
command line has already been appended and so shouldn't be added once more.

Signed-off-by: Matthew Garrett <mjg59@coreos.com>
---

Updated to allocate a buffer for the new command line - I'm not intimitely
familiar with the 16-bit setup code, so I could do with some feedback on
whether the heap it allocates is guaranteed to remain untouched until the
actual kernel setup code has run. I've also followed Ingo's suggestion of
getting rid of most of the #ifdefs, but that results in a build warning
due to an if() test that will always be true if CONFIG_CMDLINE_BOOL is set.
Any good way around that?

 Documentation/x86/zero-page.txt                |   1 +
 arch/x86/boot/boot.h                           |  19 ++++-
 arch/x86/boot/cmdline.c                        |  64 ++++++++++++++++
 arch/x86/boot/compressed/aslr.c                |   2 +-
 arch/x86/boot/compressed/cmdline.c             |  22 +++++-
 arch/x86/boot/compressed/eboot.c               |   4 +
 arch/x86/boot/compressed/misc.c                |   8 +-
 arch/x86/boot/compressed/misc.h                |   4 +-
 arch/x86/boot/main.c                           |  10 ++-
 arch/x86/include/asm/cmdline_append.h          |  10 +++
 arch/x86/include/uapi/asm/bootparam.h          |   5 +-
 arch/x86/kernel/setup.c                        |  28 ++++---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 102 +++++++++++++++++++------
 13 files changed, 236 insertions(+), 43 deletions(-)
 create mode 100644 arch/x86/include/asm/cmdline_append.h

diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
index 82fbdbc..22eaecf 100644
--- a/Documentation/x86/zero-page.txt
+++ b/Documentation/x86/zero-page.txt
@@ -12,6 +12,7 @@ Offset	Proto	Name		Meaning
 000/040	ALL	screen_info	Text mode or frame buffer information
 				(struct screen_info)
 040/014	ALL	apm_bios_info	APM BIOS information (struct apm_bios_info)
+054/004	ALL	setup_flags	Flags passed from early kernel setup
 058/008	ALL	tboot_addr      Physical address of tboot shared page
 060/010	ALL	ist_info	Intel SpeedStep (IST) BIOS support information
 				(struct ist_info)
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index bd49ec6..3b71fde 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -196,7 +196,9 @@ static inline int memcmp_gs(const void *s1, addr_t s2, size_t len)
 extern char _end[];
 extern char *HEAP;
 extern char *heap_end;
-#define RESET_HEAP() ((void *)( HEAP = _end ))
+extern size_t cmdline_len;
+
+#define RESET_HEAP() ((void *)(HEAP = _end + cmdline_len))
 static inline char *__get_heap(size_t s, size_t a, size_t n)
 {
 	char *tmp;
@@ -214,6 +216,11 @@ static inline bool heap_free(size_t n)
 	return (int)(heap_end-HEAP) >= (int)n;
 }
 
+static inline char *alloc_cmdline(size_t s)
+{
+	cmdline_len = s;
+	return _end;
+}
 /* copy.S */
 
 void copy_to_fs(addr_t dst, void *src, size_t len);
@@ -273,6 +280,7 @@ void intcall(u8 int_no, const struct biosregs *ireg, struct biosregs *oreg);
 /* cmdline.c */
 int __cmdline_find_option(unsigned long cmdline_ptr, const char *option, char *buffer, int bufsize);
 int __cmdline_find_option_bool(unsigned long cmdline_ptr, const char *option);
+int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params);
 static inline int cmdline_find_option(const char *option, char *buffer, int bufsize)
 {
 	unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
@@ -293,6 +301,15 @@ static inline int cmdline_find_option_bool(const char *option)
 	return __cmdline_find_option_bool(cmd_line_ptr, option);
 }
 
+static inline int cmdline_init(void)
+{
+	unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
+
+	if (cmd_line_ptr >= 0x100000)
+		return -1;      /* inaccessible */
+
+	return __cmdline_init(cmd_line_ptr, &boot_params);
+}
 /* cpu.c, cpucheck.c */
 int check_cpu(int *cpu_level_ptr, int *req_level_ptr, u32 **err_flags_ptr);
 int validate_cpu(void);
diff --git a/arch/x86/boot/cmdline.c b/arch/x86/boot/cmdline.c
index 625d21b..5f83332 100644
--- a/arch/x86/boot/cmdline.c
+++ b/arch/x86/boot/cmdline.c
@@ -12,8 +12,15 @@
  * Simple command-line parser for early boot.
  */
 
+#include <asm/cmdline_append.h>
 #include "boot.h"
 
+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[] = CONFIG_CMDLINE;
+#else
+static char *builtin_cmdline;
+#endif
+
 static inline int myisspace(u8 c)
 {
 	return c <= ' ';	/* Close enough approximation */
@@ -156,3 +163,60 @@ int __cmdline_find_option_bool(unsigned long cmdline_ptr, const char *option)
 
 	return 0;	/* Buffer overrun */
 }
+
+int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params)
+{
+	addr_t cptr, nptr;
+	int i = 0;
+	int len = 0;
+	unsigned long new_cmdline_ptr;
+
+	if (!builtin_cmdline)
+		return 0;
+
+	set_fs(cmdline_ptr >> 4);
+	cptr = cmdline_ptr & 0xf;
+
+	if (cmdline_ptr && append_cmdline) {
+		char c = rdfs8(cptr);
+
+		while (c)
+			c = rdfs8(cptr+len++);
+
+		len++; /* Trailing space */
+	}
+
+	while (builtin_cmdline[i]) {
+		len++;
+		i++;
+	}
+
+	/* Allocate a new command line */
+	new_cmdline_ptr = (long)alloc_cmdline(len);
+	nptr = new_cmdline_ptr & 0xf;
+
+	if (cmdline_ptr && append_cmdline) {
+		char c = rdfs8(cptr++);
+
+		while (c) {
+			set_fs(new_cmdline_ptr >> 4);
+			wrfs8(c, nptr++);
+			set_fs(cmdline_ptr >> 4);
+			c = rdfs8(cptr++);
+		}
+		set_fs(new_cmdline_ptr >> 4);
+		wrfs8(' ', nptr++);
+	}
+
+	set_fs(new_cmdline_ptr >> 4);
+
+	for (i = 0; builtin_cmdline[i]; i++)
+		wrfs8(builtin_cmdline[i], nptr++);
+
+	wrfs8('\0', nptr);
+
+	params->hdr.cmd_line_ptr = new_cmdline_ptr;
+	params->setup_flags |= SETUP_CMDLINE_APPENDED;
+
+	return 0;
+}
diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
index d7b1f65..9765a4a 100644
--- a/arch/x86/boot/compressed/aslr.c
+++ b/arch/x86/boot/compressed/aslr.c
@@ -170,7 +170,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	mem_avoid[2].size = cmd_line_size;
 
 	/* Avoid heap memory. */
-	mem_avoid[3].start = (unsigned long)free_mem_ptr;
+	mem_avoid[3].start = (unsigned long)orig_free_mem_ptr;
 	mem_avoid[3].size = BOOT_HEAP_SIZE;
 
 	/* Avoid stack memory. */
diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index b68e303..e7a4612 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -1,6 +1,6 @@
 #include "misc.h"
 
-#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
+#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL
 
 static unsigned long fs;
 static inline void set_fs(unsigned long seg)
@@ -12,6 +12,15 @@ static inline char rdfs8(addr_t addr)
 {
 	return *((char *)(fs + addr));
 }
+static inline void wrfs8(u8 v, addr_t addr)
+{
+	*((char *)(fs + addr)) = v;
+}
+static inline int alloc_cmdline(size_t s)
+{
+	free_mem_ptr += s;
+	return orig_free_mem_ptr;
+}
 #include "../cmdline.c"
 static unsigned long get_cmd_line_ptr(void)
 {
@@ -30,4 +39,15 @@ int cmdline_find_option_bool(const char *option)
 	return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
 }
 
+int cmdline_init(void)
+{
+	if (!(real_mode->setup_flags & SETUP_CMDLINE_APPENDED))
+		return __cmdline_init(get_cmd_line_ptr(), real_mode);
+	return 0;
+}
+#else
+int cmdline_init(void)
+{
+	return 0;
+}
 #endif
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 48304b8..e44146d 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -9,6 +9,7 @@
 
 #include <linux/efi.h>
 #include <linux/pci.h>
+#include <asm/cmdline_append.h>
 #include <asm/efi.h>
 #include <asm/setup.h>
 #include <asm/desc.h>
@@ -1112,6 +1113,9 @@ struct boot_params *make_boot_params(struct efi_config *c)
 	/* Fill in upper bits of command line address, NOP on 32 bit  */
 	boot_params->ext_cmd_line_ptr = (u64)(unsigned long)cmdline_ptr >> 32;
 
+	if (append_cmdline)
+		boot_params->setup_flags |= SETUP_CMDLINE_APPENDED;
+
 	hdr->ramdisk_image = 0;
 	hdr->ramdisk_size = 0;
 
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index a107b93..1878bfc 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -116,6 +116,7 @@ static void error(char *m);
  */
 struct boot_params *real_mode;		/* Pointer to real-mode data */
 
+memptr orig_free_mem_ptr;
 memptr free_mem_ptr;
 memptr free_mem_end_ptr;
 
@@ -393,11 +394,14 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 	lines = real_mode->screen_info.orig_video_lines;
 	cols = real_mode->screen_info.orig_video_cols;
 
+	orig_free_mem_ptr = free_mem_ptr = heap;	/* Heap */
+	free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
+
+	cmdline_init();
+
 	console_init();
 	debug_putstr("early console in decompress_kernel\n");
 
-	free_mem_ptr     = heap;	/* Heap */
-	free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
 
 	/*
 	 * The memory hole needed for the kernel is the larger of either
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 89dd0d7..7e50fdf 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -31,6 +31,7 @@
 #endif
 
 /* misc.c */
+extern memptr orig_free_mem_ptr;
 extern memptr free_mem_ptr;
 extern memptr free_mem_end_ptr;
 extern struct boot_params *real_mode;		/* Pointer to real-mode data */
@@ -48,10 +49,11 @@ static inline void debug_putstr(const char *s)
 
 #endif
 
-#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
+#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL
 /* cmdline.c */
 int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
+int cmdline_init(void);
 #endif
 
 
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index fd6c9f2..0ddf387 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -20,6 +20,7 @@ struct boot_params boot_params __attribute__((aligned(16)));
 
 char *HEAP = _end;
 char *heap_end = _end;		/* Default end of heap = no heap */
+size_t cmdline_len;
 
 /*
  * Copy the header into the boot parameter block.  Since this
@@ -137,14 +138,17 @@ void main(void)
 	/* First, copy the boot header into the "zeropage" */
 	copy_boot_params();
 
+	/* End of heap check */
+	init_heap();
+
+	/* Handle built-in command line */
+	cmdline_init();
+
 	/* Initialize the early-boot console */
 	console_init();
 	if (cmdline_find_option_bool("debug"))
 		puts("early console in setup code\n");
 
-	/* End of heap check */
-	init_heap();
-
 	/* Make sure we have all the proper CPU support */
 	if (validate_cpu()) {
 		puts("Unable to boot - please use a kernel appropriate "
diff --git a/arch/x86/include/asm/cmdline_append.h b/arch/x86/include/asm/cmdline_append.h
new file mode 100644
index 0000000..a57bf7e
--- /dev/null
+++ b/arch/x86/include/asm/cmdline_append.h
@@ -0,0 +1,10 @@
+#ifndef _ASM_X86_CMDLINE_APPEND_H
+#define _ASM_X86_CMDLINE_APPEND_H
+
+#ifdef CONFIG_CMDLINE_OVERRIDE
+static char append_cmdline;
+#else
+static char append_cmdline = 1;
+#endif
+
+#endif /* _ASM_X86_CMDLINE_APPEND_H */
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index ab456dc..3fda079 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -27,6 +27,9 @@
 #define XLF_EFI_HANDOVER_64		(1<<3)
 #define XLF_EFI_KEXEC			(1<<4)
 
+/* setup_flags */
+#define SETUP_CMDLINE_APPENDED		(1<<0)
+
 #ifndef __ASSEMBLY__
 
 #include <linux/types.h>
@@ -114,7 +117,7 @@ struct efi_info {
 struct boot_params {
 	struct screen_info screen_info;			/* 0x000 */
 	struct apm_bios_info apm_bios_info;		/* 0x040 */
-	__u8  _pad2[4];					/* 0x054 */
+	__u32 setup_flags;				/* 0x054 */
 	__u64  tboot_addr;				/* 0x058 */
 	struct ist_info ist_info;			/* 0x060 */
 	__u8  _pad3[16];				/* 0x070 */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index d74ac33..4c6456d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -112,6 +112,7 @@
 #include <asm/alternative.h>
 #include <asm/prom.h>
 
+#include <asm/cmdline_append.h>
 /*
  * max_low_pfn_mapped: highest direct mapped pfn under 4GB
  * max_pfn_mapped:     highest direct mapped pfn over 4GB
@@ -234,6 +235,8 @@ unsigned long saved_video_mode;
 static char __initdata command_line[COMMAND_LINE_SIZE];
 #ifdef CONFIG_CMDLINE_BOOL
 static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
+#else
+static char *builtin_cmdline;
 #endif
 
 #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
@@ -973,18 +976,21 @@ 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);
+	if (builtin_cmdline) {
+		/* Fix up the command line if an earlier stage hasn't */
+		if (!(boot_params.setup_flags & SETUP_CMDLINE_APPENDED)) {
+			if (append_cmdline) {
+				/* 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;
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index f07d4a6..50bd9c2 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -12,9 +12,22 @@
 
 #include <linux/efi.h>
 #include <asm/efi.h>
+#include <asm/setup.h>
 
 #include "efistub.h"
 
+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[] = CONFIG_CMDLINE;
+#else
+static char *builtin_cmdline;
+#endif
+
+#ifdef CONFIG_CMDLINE_OVERRIDE
+static bool append_cmdline;
+#else
+static bool append_cmdline = true;
+#endif
+
 /*
  * Some firmware implementations have problems reading files in one go.
  * A read chunk size of 1MB seems to work for most platforms.
@@ -649,6 +662,21 @@ static u8 *efi_utf16_to_utf8(u8 *dst, const u16 *src, int n)
 	return dst;
 }
 
+static size_t efi_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = strlen(dest);
+	size_t len = strlen(src);
+	size_t res = dsize + len;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count-1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}
+
 /*
  * Convert the unicode UEFI command line to ASCII to pass to kernel.
  * Size of memory allocated return in *cmd_line_len.
@@ -658,42 +686,72 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 			  efi_loaded_image_t *image,
 			  int *cmd_line_len)
 {
+	unsigned long cmdline_addr = 0;
+	int i;
+	efi_status_t status;
 	const u16 *s2;
 	u8 *s1 = NULL;
-	unsigned long cmdline_addr = 0;
 	int load_options_chars = image->load_options_size / 2; /* UTF-16 */
 	const u16 *options = image->load_options;
 	int options_bytes = 0;  /* UTF-8 bytes */
 	int options_chars = 0;  /* UTF-16 chars */
-	efi_status_t status;
 	u16 zero = 0;
 
-	if (options) {
-		s2 = options;
-		while (*s2 && *s2 != '\n'
-		       && options_chars < load_options_chars) {
-			options_bytes += efi_utf8_bytes(*s2++);
-			options_chars++;
+	if (!builtin_cmdline || append_cmdline) {
+		if (options) {
+			s2 = options;
+			while (*s2 && *s2 != '\n'
+			       && options_chars < load_options_chars) {
+				options_bytes += efi_utf8_bytes(*s2++);
+				options_chars++;
+			}
+		}
+
+		if (!options_chars) {
+			/* No command line options, so return empty string*/
+			options = &zero;
 		}
-	}
 
-	if (!options_chars) {
-		/* No command line options, so return empty string*/
-		options = &zero;
-	}
+		options_bytes++;	/* NUL termination */
 
-	options_bytes++;	/* NUL termination */
+		if (builtin_cmdline) {
+			/* Add length of the built-in command line + space */
+			options_bytes += strlen(builtin_cmdline);
+			options_bytes++;
+		}
 
-	status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr);
-	if (status != EFI_SUCCESS)
-		return NULL;
+		status = efi_low_alloc(sys_table_arg, options_bytes, 0,
+				       &cmdline_addr);
 
-	s1 = (u8 *)cmdline_addr;
-	s2 = (const u16 *)options;
+		if (status != EFI_SUCCESS)
+			return NULL;
 
-	s1 = efi_utf16_to_utf8(s1, s2, options_chars);
-	*s1 = '\0';
+		s1 = (u8 *)cmdline_addr;
+		s2 = (const u16 *)options;
+
+		s1 = efi_utf16_to_utf8(s1, s2, options_chars);
+		*s1 = '\0';
+
+		if (builtin_cmdline) {
+			efi_strlcat((char *)cmdline_addr, " ",
+				    COMMAND_LINE_SIZE);
+			efi_strlcat((char *)cmdline_addr, builtin_cmdline,
+				    COMMAND_LINE_SIZE);
+		}
+		*cmd_line_len = options_bytes;
+	} else {
+		/* Ignore the provided command line, use the built-in one */
+		status = efi_low_alloc(sys_table_arg, strlen(builtin_cmdline),
+				       0, &cmdline_addr);
+		if (status != EFI_SUCCESS)
+			return NULL;
+		while (builtin_cmdline[i] && i < COMMAND_LINE_SIZE) {
+			((char *)cmdline_addr)[i] = builtin_cmdline[i];
+			i++;
+		}
+		((char *)cmdline_addr)[i] = '\0';
+		*cmd_line_len = strlen(builtin_cmdline);
+	}
 
-	*cmd_line_len = options_bytes;
 	return (char *)cmdline_addr;
 }
-- 
2.4.0

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Ingo Molnar]

* Matthew Garrett <mjg59@coreos.com> wrote:

> The kernel supports having a command line built into it. Unfortunately
> this doesn't work in all cases - the built-in command line is only
> appended after we've jumped to the kernel proper, but various parts of
> the early boot process also pay attention to the command line.
> 
> This patch moves the command line override code from the kernel itself to
> the early init code. Unfortunately the kernel can be executed by jumping
> to the 16-bit entry point, the UEFI entry point or directly to the 32-bit
> entry point, and there is no guarantee that any of these will have access
> to data held in the others. As a result, three copies of the command line
> will be embedded in the kernel.
> 
> This patch also adds a new flag to the xloadflags field of the setup header
> in order to allow the entry points to inform the generic setup code that the
> command line has already been appended and so shouldn't be added once more.
> 
> Signed-off-by: Matthew Garrett <mjg59@coreos.com>
> ---
> 
> Updated to allocate a buffer for the new command line - I'm not 
> intimitely familiar with the 16-bit setup code, so I could do with 
> some feedback on whether the heap it allocates is guaranteed to 
> remain untouched until the actual kernel setup code has run. I've 
> also followed Ingo's suggestion of getting rid of most of the 
> #ifdefs, but that results in a build warning due to an if() test 
> that will always be true if CONFIG_CMDLINE_BOOL is set. Any good way 
> around that?
> 
>  Documentation/x86/zero-page.txt                |   1 +
>  arch/x86/boot/boot.h                           |  19 ++++-
>  arch/x86/boot/cmdline.c                        |  64 ++++++++++++++++
>  arch/x86/boot/compressed/aslr.c                |   2 +-
>  arch/x86/boot/compressed/cmdline.c             |  22 +++++-
>  arch/x86/boot/compressed/eboot.c               |   4 +
>  arch/x86/boot/compressed/misc.c                |   8 +-
>  arch/x86/boot/compressed/misc.h                |   4 +-
>  arch/x86/boot/main.c                           |  10 ++-
>  arch/x86/include/asm/cmdline_append.h          |  10 +++
>  arch/x86/include/uapi/asm/bootparam.h          |   5 +-
>  arch/x86/kernel/setup.c                        |  28 ++++---
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 102 +++++++++++++++++++------
>  13 files changed, 236 insertions(+), 43 deletions(-)
>  create mode 100644 arch/x86/include/asm/cmdline_append.h

Overall structure looks better now.

One problem is that it's such a huge patch: is there a way to split 
this up into smaller, more obvious, easier to bisect to patches?

> diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
> index bd49ec6..3b71fde 100644
> --- a/arch/x86/boot/boot.h
> +++ b/arch/x86/boot/boot.h
> @@ -196,7 +196,9 @@ static inline int memcmp_gs(const void *s1, addr_t s2, size_t len)
>  extern char _end[];
>  extern char *HEAP;
>  extern char *heap_end;
> -#define RESET_HEAP() ((void *)( HEAP = _end ))
> +extern size_t cmdline_len;
> +
> +#define RESET_HEAP() ((void *)(HEAP = _end + cmdline_len))
>  static inline char *__get_heap(size_t s, size_t a, size_t n)
>  {
>  	char *tmp;
> @@ -214,6 +216,11 @@ static inline bool heap_free(size_t n)
>  	return (int)(heap_end-HEAP) >= (int)n;
>  }
>  
> +static inline char *alloc_cmdline(size_t s)
> +{
> +	cmdline_len = s;
> +	return _end;
> +}
>  /* copy.S */
>  
>  void copy_to_fs(addr_t dst, void *src, size_t len);
> @@ -273,6 +280,7 @@ void intcall(u8 int_no, const struct biosregs *ireg, struct biosregs *oreg);
>  /* cmdline.c */
>  int __cmdline_find_option(unsigned long cmdline_ptr, const char *option, char *buffer, int bufsize);
>  int __cmdline_find_option_bool(unsigned long cmdline_ptr, const char *option);
> +int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params);
>  static inline int cmdline_find_option(const char *option, char *buffer, int bufsize)
>  {
>  	unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
> @@ -293,6 +301,15 @@ static inline int cmdline_find_option_bool(const char *option)
>  	return __cmdline_find_option_bool(cmd_line_ptr, option);
>  }
>  
> +static inline int cmdline_init(void)
> +{
> +	unsigned long cmd_line_ptr = boot_params.hdr.cmd_line_ptr;
> +
> +	if (cmd_line_ptr >= 0x100000)
> +		return -1;      /* inaccessible */
> +
> +	return __cmdline_init(cmd_line_ptr, &boot_params);
> +}
>  /* cpu.c, cpucheck.c */
>  int check_cpu(int *cpu_level_ptr, int *req_level_ptr, u32 **err_flags_ptr);
>  int validate_cpu(void);

So could we please move this into a separate arch/x86/boot/cmdline.h 
include file, so that this stuff stays separate from the various other 
helpers we nurse in boot.h?

> diff --git a/arch/x86/boot/cmdline.c b/arch/x86/boot/cmdline.c
> index 625d21b..5f83332 100644
> --- a/arch/x86/boot/cmdline.c
> +++ b/arch/x86/boot/cmdline.c
> @@ -12,8 +12,15 @@
>   * Simple command-line parser for early boot.
>   */
>  
> +#include <asm/cmdline_append.h>
>  #include "boot.h"
>  
> +#ifdef CONFIG_CMDLINE_BOOL
> +static char builtin_cmdline[] = CONFIG_CMDLINE;
> +#else
> +static char *builtin_cmdline;
> +#endif

> +
>  static inline int myisspace(u8 c)
>  {
>  	return c <= ' ';	/* Close enough approximation */
> @@ -156,3 +163,60 @@ int __cmdline_find_option_bool(unsigned long cmdline_ptr, const char *option)
>  
>  	return 0;	/* Buffer overrun */
>  }
> +
> +int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params)

Please add comments describing the function. Why does it have double 
underscores, who is supposed to use it and how, etc. etc?

> +{
> +	addr_t cptr, nptr;
> +	int i = 0;
> +	int len = 0;
> +	unsigned long new_cmdline_ptr;
> +
> +	if (!builtin_cmdline)
> +		return 0;
> +
> +	set_fs(cmdline_ptr >> 4);
> +	cptr = cmdline_ptr & 0xf;
> +
> +	if (cmdline_ptr && append_cmdline) {
> +		char c = rdfs8(cptr);
> +
> +		while (c)
> +			c = rdfs8(cptr+len++);
> +

So this needs a comment - what do we do here? Skip over to the end of 
the 'cptr' string, using 16-bit ops? Would be nice to have it in a 
suitably named helper inline.

> +		len++; /* Trailing space */

that's a trailing zero delimiter byte, not space, right?

> +	}


> +
> +	while (builtin_cmdline[i]) {
> +		len++;
> +		i++;
> +	}

Ditto - is this:

	len += strlen(builtin_cmdline+i)

in disguise?

> +
> +	/* Allocate a new command line */
> +	new_cmdline_ptr = (long)alloc_cmdline(len);
> +	nptr = new_cmdline_ptr & 0xf;
> +
> +	if (cmdline_ptr && append_cmdline) {
> +		char c = rdfs8(cptr++);
> +
> +		while (c) {
> +			set_fs(new_cmdline_ptr >> 4);
> +			wrfs8(c, nptr++);
> +			set_fs(cmdline_ptr >> 4);
> +			c = rdfs8(cptr++);
> +		}
> +		set_fs(new_cmdline_ptr >> 4);
> +		wrfs8(' ', nptr++);
> +	}

So here we copy from 'cptr' to 'nptr'? Not very well named variables, 
at minimum.

> +
> +	set_fs(new_cmdline_ptr >> 4);
> +
> +	for (i = 0; builtin_cmdline[i]; i++)
> +		wrfs8(builtin_cmdline[i], nptr++);
> +
> +	wrfs8('\0', nptr);
> +
> +	params->hdr.cmd_line_ptr = new_cmdline_ptr;
> +	params->setup_flags |= SETUP_CMDLINE_APPENDED;
> +
> +	return 0;

So I'd really suggest splitting off the string manipulation parts into 
suitably named helper functions, use better variable named and comment 
it better. That will go a long way to document this non-obvious piece 
of 16-bit magic.

> +}
> diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
> index d7b1f65..9765a4a 100644
> --- a/arch/x86/boot/compressed/aslr.c
> +++ b/arch/x86/boot/compressed/aslr.c
> @@ -170,7 +170,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>  	mem_avoid[2].size = cmd_line_size;
>  
>  	/* Avoid heap memory. */
> -	mem_avoid[3].start = (unsigned long)free_mem_ptr;
> +	mem_avoid[3].start = (unsigned long)orig_free_mem_ptr;
>  	mem_avoid[3].size = BOOT_HEAP_SIZE;

it's totally non-obvious what's happening here. Why is it named 
'orig'?

>  
>  	/* Avoid stack memory. */
> diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
> index b68e303..e7a4612 100644
> --- a/arch/x86/boot/compressed/cmdline.c
> +++ b/arch/x86/boot/compressed/cmdline.c
> @@ -1,6 +1,6 @@
>  #include "misc.h"
>  
> -#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
> +#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL

So what's common in all these Kconfig options? That they append 
something to the bootloader provided command line? If yes then please 
create an intermediary helper Kconfig switch that the above options 
select, and only use that helper config here.

>  
>  static unsigned long fs;
>  static inline void set_fs(unsigned long seg)
> @@ -12,6 +12,15 @@ static inline char rdfs8(addr_t addr)
>  {
>  	return *((char *)(fs + addr));
>  }
> +static inline void wrfs8(u8 v, addr_t addr)
> +{
> +	*((char *)(fs + addr)) = v;
> +}
> +static inline int alloc_cmdline(size_t s)
> +{
> +	free_mem_ptr += s;
> +	return orig_free_mem_ptr;
> +}
>  #include "../cmdline.c"
>  static unsigned long get_cmd_line_ptr(void)
>  {
> @@ -30,4 +39,15 @@ int cmdline_find_option_bool(const char *option)
>  	return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
>  }
>  
> +int cmdline_init(void)
> +{
> +	if (!(real_mode->setup_flags & SETUP_CMDLINE_APPENDED))
> +		return __cmdline_init(get_cmd_line_ptr(), real_mode);
> +	return 0;
> +}
> +#else
> +int cmdline_init(void)
> +{
> +	return 0;
> +}
>  #endif

So 'cmdline_init()' seems like a poorly chosen interface name. The 
cmdline is already initialized and set up for us by the bootloader. 
What we do here is that in certain configs we append to the cmdline. 
So cmdline_append() might be a better name.

Also, there's very little reason to inline the main API call itself, 
just do the SETUP_CMDLINE_APPENDED from within __cmdline_init and get 
rid of the underscores.


> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 48304b8..e44146d 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/efi.h>
>  #include <linux/pci.h>
> +#include <asm/cmdline_append.h>
>  #include <asm/efi.h>
>  #include <asm/setup.h>
>  #include <asm/desc.h>
> @@ -1112,6 +1113,9 @@ struct boot_params *make_boot_params(struct efi_config *c)
>  	/* Fill in upper bits of command line address, NOP on 32 bit  */
>  	boot_params->ext_cmd_line_ptr = (u64)(unsigned long)cmdline_ptr >> 32;
>  
> +	if (append_cmdline)
> +		boot_params->setup_flags |= SETUP_CMDLINE_APPENDED;

So why not have a boot_params__set_cmdline_appended() helper:

	boot_params__set_cmdline_appended(boot_params);

instead of this weird flaggery in the include file:

+#ifdef CONFIG_CMDLINE_OVERRIDE
+static char append_cmdline;
+#else
+static char append_cmdline = 1;
+#endif

?

> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index a107b93..1878bfc 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -116,6 +116,7 @@ static void error(char *m);
>   */
>  struct boot_params *real_mode;		/* Pointer to real-mode data */
>  
> +memptr orig_free_mem_ptr;
>  memptr free_mem_ptr;
>  memptr free_mem_end_ptr;
>  
> @@ -393,11 +394,14 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
>  	lines = real_mode->screen_info.orig_video_lines;
>  	cols = real_mode->screen_info.orig_video_cols;
>  
> +	orig_free_mem_ptr = free_mem_ptr = heap;	/* Heap */
> +	free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
> +
> +	cmdline_init();
> +
>  	console_init();
>  	debug_putstr("early console in decompress_kernel\n");
>  
> -	free_mem_ptr     = heap;	/* Heap */
> -	free_mem_end_ptr = heap + BOOT_HEAP_SIZE;

So I don't understand the old heap logic in 
arch/x86/boot/compressed/misc.c, let alone the new one.

So low level boot assembly code sets up the boot_heap, and passes it 
to decompress_kernel() as a 'heap' C function argument. Which then 
stores it in free_mem_ptr and free_mem_end_ptr but does not otherwise 
use it other than some sanity checks.

As per the comments in the code decompression is supposed to use this 
heap, but where does it figure out the address from? It's not passed 
to it in any greppable fashion that I can see.

Confusingly enough, there appears to be another, real mode heap 
mechanism in realmode/rm/stack.S and boot/boot.h, around rm_heap at 
al.

Plus there's both boot/boot.h and include/asm/x86/boot.h, which 
confused me some more.

So this code is highly confusing. It first needs to be unconfused.

> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index 89dd0d7..7e50fdf 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -31,6 +31,7 @@
>  #endif
>  
>  /* misc.c */
> +extern memptr orig_free_mem_ptr;
>  extern memptr free_mem_ptr;
>  extern memptr free_mem_end_ptr;
>  extern struct boot_params *real_mode;		/* Pointer to real-mode data */
> @@ -48,10 +49,11 @@ static inline void debug_putstr(const char *s)
>  
>  #endif
>  
> -#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
> +#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL

This too could use the new Kconfig helper flag.

>  /* cmdline.c */
>  int cmdline_find_option(const char *option, char *buffer, int bufsize);
>  int cmdline_find_option_bool(const char *option);
> +int cmdline_init(void);
>  #endif
>  
>  
> diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
> index fd6c9f2..0ddf387 100644
> --- a/arch/x86/boot/main.c
> +++ b/arch/x86/boot/main.c
> @@ -20,6 +20,7 @@ struct boot_params boot_params __attribute__((aligned(16)));
>  
>  char *HEAP = _end;
>  char *heap_end = _end;		/* Default end of heap = no heap */
> +size_t cmdline_len;
>  
>  /*
>   * Copy the header into the boot parameter block.  Since this
> @@ -137,14 +138,17 @@ void main(void)
>  	/* First, copy the boot header into the "zeropage" */
>  	copy_boot_params();
>  
> +	/* End of heap check */
> +	init_heap();

So init_heap() could need some extra comments as well.

Also, it should get a better name, to express its real functionality: 
heap__reset_and_check() or so? That way it could lose the extra 
comment line at the call site.

> +
> +	/* Handle built-in command line */
> +	cmdline_init();
> +
>  	/* Initialize the early-boot console */
>  	console_init();
>  	if (cmdline_find_option_bool("debug"))
>  		puts("early console in setup code\n");
>  
> -	/* End of heap check */
> -	init_heap();
> -
>  	/* Make sure we have all the proper CPU support */
>  	if (validate_cpu()) {
>  		puts("Unable to boot - please use a kernel appropriate "
> diff --git a/arch/x86/include/asm/cmdline_append.h b/arch/x86/include/asm/cmdline_append.h
> new file mode 100644
> index 0000000..a57bf7e
> --- /dev/null
> +++ b/arch/x86/include/asm/cmdline_append.h
> @@ -0,0 +1,10 @@
> +#ifndef _ASM_X86_CMDLINE_APPEND_H
> +#define _ASM_X86_CMDLINE_APPEND_H
> +
> +#ifdef CONFIG_CMDLINE_OVERRIDE
> +static char append_cmdline;
> +#else
> +static char append_cmdline = 1;
> +#endif
> +
> +#endif /* _ASM_X86_CMDLINE_APPEND_H */
> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> index ab456dc..3fda079 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -27,6 +27,9 @@
>  #define XLF_EFI_HANDOVER_64		(1<<3)
>  #define XLF_EFI_KEXEC			(1<<4)
>  
> +/* setup_flags */
> +#define SETUP_CMDLINE_APPENDED		(1<<0)
> +
>  #ifndef __ASSEMBLY__
>  
>  #include <linux/types.h>
> @@ -114,7 +117,7 @@ struct efi_info {
>  struct boot_params {
>  	struct screen_info screen_info;			/* 0x000 */
>  	struct apm_bios_info apm_bios_info;		/* 0x040 */
> -	__u8  _pad2[4];					/* 0x054 */
> +	__u32 setup_flags;				/* 0x054 */
>  	__u64  tboot_addr;				/* 0x058 */
>  	struct ist_info ist_info;			/* 0x060 */
>  	__u8  _pad3[16];				/* 0x070 */

So data structures should be clearly explained. Who sets this flag, 
when, for what purpose, etc.

Using 'setup' here is highly ambiguous: the whole boot process is an 
act of 'setting up stuff'. So it's very non-obvious what this does, at 
first and second glance as well.

> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index d74ac33..4c6456d 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -112,6 +112,7 @@
>  #include <asm/alternative.h>
>  #include <asm/prom.h>
>  
> +#include <asm/cmdline_append.h>
>  /*
>   * max_low_pfn_mapped: highest direct mapped pfn under 4GB
>   * max_pfn_mapped:     highest direct mapped pfn over 4GB
> @@ -234,6 +235,8 @@ unsigned long saved_video_mode;
>  static char __initdata command_line[COMMAND_LINE_SIZE];
>  #ifdef CONFIG_CMDLINE_BOOL
>  static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
> +#else
> +static char *builtin_cmdline;
>  #endif
>  
>  #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
> @@ -973,18 +976,21 @@ 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);
> +	if (builtin_cmdline) {
> +		/* Fix up the command line if an earlier stage hasn't */
> +		if (!(boot_params.setup_flags & SETUP_CMDLINE_APPENDED)) {
> +			if (append_cmdline) {
> +				/* append boot loader cmdline to builtin */

Please harmonize comment capitalization if you touch a function in 
such a major way, they should all start with capital letters.

> +				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

Why is all this in a high level function named setup_arch()? At 
minimum it should be factored out into a suitably named helper 
function.

With that function we can also do an early:

	if (!builtin_cmdline)
		return;

which gives us an extra identation level which will fix all those ugly 
line breaks later on. (and ignore checkpatch if it's slightly above 
col80)

>  	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>  	*cmdline_p = command_line;
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index f07d4a6..50bd9c2 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -12,9 +12,22 @@
>  
>  #include <linux/efi.h>
>  #include <asm/efi.h>
> +#include <asm/setup.h>
>  
>  #include "efistub.h"
>  
> +#ifdef CONFIG_CMDLINE_BOOL
> +static char builtin_cmdline[] = CONFIG_CMDLINE;
> +#else
> +static char *builtin_cmdline;
> +#endif
> +
> +#ifdef CONFIG_CMDLINE_OVERRIDE
> +static bool append_cmdline;
> +#else
> +static bool append_cmdline = true;
> +#endif

So this is the third time I've seen this repeating pattern in this 
patch, with only slight variations.

Could we create a generic helper module that does most of this, with 
small headers defining the various boot environment specialities, 
linked into the various loader functionalities separately?

That would have various advantages, beyond being easier to read: such 
as adding new functionality to our command line string handling could 
be done in a single place.

So something like:

	arch/x86/boot/cmdline/core.c
	arch/x86/boot/cmdline/16-bit.h
	arch/x86/boot/cmdline/32-bit.h
	arch/x86/boot/cmdline/efi.h

... or so, glued into their respective boot stages.

Unless we can create such a unified, easy to handle structure for 
shared functionality I don't think we want to complicate the boot code 
in 3 separate, slightly (and sometimes not so slightly) different 
copies.

> +
>  /*
>   * Some firmware implementations have problems reading files in one go.
>   * A read chunk size of 1MB seems to work for most platforms.
> @@ -649,6 +662,21 @@ static u8 *efi_utf16_to_utf8(u8 *dst, const u16 *src, int n)
>  	return dst;
>  }
>  
> +static size_t efi_strlcat(char *dest, const char *src, size_t count)
> +{
> +	size_t dsize = strlen(dest);
> +	size_t len = strlen(src);
> +	size_t res = dsize + len;
> +
> +	dest += dsize;
> +	count -= dsize;
> +	if (len >= count)
> +		len = count-1;
> +	memcpy(dest, src, len);
> +	dest[len] = 0;
> +	return res;
> +}

Looks like there's very little 'EFI' about this strlcat 
implementation, right? So why don't we use the real one, is it not 
available yet?

> +
>  /*
>   * Convert the unicode UEFI command line to ASCII to pass to kernel.
>   * Size of memory allocated return in *cmd_line_len.

Couldn't help but notice the various spelling errors in this comment 
block...

> @@ -658,42 +686,72 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
>  			  efi_loaded_image_t *image,
>  			  int *cmd_line_len)
>  {
> +	unsigned long cmdline_addr = 0;
> +	int i;
> +	efi_status_t status;
>  	const u16 *s2;
>  	u8 *s1 = NULL;
> -	unsigned long cmdline_addr = 0;
>  	int load_options_chars = image->load_options_size / 2; /* UTF-16 */
>  	const u16 *options = image->load_options;
>  	int options_bytes = 0;  /* UTF-8 bytes */
>  	int options_chars = 0;  /* UTF-16 chars */
> -	efi_status_t status;
>  	u16 zero = 0;
>  
> -	if (options) {
> -		s2 = options;
> -		while (*s2 && *s2 != '\n'
> -		       && options_chars < load_options_chars) {
> -			options_bytes += efi_utf8_bytes(*s2++);
> -			options_chars++;
> +	if (!builtin_cmdline || append_cmdline) {

So both 'builtin_cmdline' and 'append_cmdline' sound like a flag, but 
only the second one is one - builtin_cmdline is actually a buffer 
address!

So key names are not very well chosen. I'd use something like:

	cmdline_buf_bootloader		/* boot loader provided one */
	cmdline_buf_append		/* the one we want to append */
	cmdline_buf			/* the kernel's final cmdline */

or so - and propagate these names. Using 'builtin' is a misnomer 
really.

We could probably also get rid of 'append_cmdline': whether 
cmdline_buf_append is NULL or not is good enough of a flag?

> +		if (options) {
> +			s2 = options;
> +			while (*s2 && *s2 != '\n'
> +			       && options_chars < load_options_chars) {
> +				options_bytes += efi_utf8_bytes(*s2++);
> +				options_chars++;

This code too is suffering from too many indentations. Having a helper 
function that can do an early return on !cmdline would help with that 
as well.

> +			}
> +		}
> +
> +		if (!options_chars) {
> +			/* No command line options, so return empty string*/

Bad comment style.

> +			options = &zero;
>  		}
> -	}
>  
> -	if (!options_chars) {
> -		/* No command line options, so return empty string*/
> -		options = &zero;
> -	}
> +		options_bytes++;	/* NUL termination */
>  
> -	options_bytes++;	/* NUL termination */
> +		if (builtin_cmdline) {
> +			/* Add length of the built-in command line + space */
> +			options_bytes += strlen(builtin_cmdline);
> +			options_bytes++;
> +		}
>  
> -	status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr);
> -	if (status != EFI_SUCCESS)
> -		return NULL;
> +		status = efi_low_alloc(sys_table_arg, options_bytes, 0,
> +				       &cmdline_addr);
>  
> -	s1 = (u8 *)cmdline_addr;
> -	s2 = (const u16 *)options;
> +		if (status != EFI_SUCCESS)
> +			return NULL;
>  
> -	s1 = efi_utf16_to_utf8(s1, s2, options_chars);
> -	*s1 = '\0';
> +		s1 = (u8 *)cmdline_addr;
> +		s2 = (const u16 *)options;
> +
> +		s1 = efi_utf16_to_utf8(s1, s2, options_chars);
> +		*s1 = '\0';
> +
> +		if (builtin_cmdline) {
> +			efi_strlcat((char *)cmdline_addr, " ",
> +				    COMMAND_LINE_SIZE);
> +			efi_strlcat((char *)cmdline_addr, builtin_cmdline,
> +				    COMMAND_LINE_SIZE);
> +		}
> +		*cmd_line_len = options_bytes;
> +	} else {
> +		/* Ignore the provided command line, use the built-in one */
> +		status = efi_low_alloc(sys_table_arg, strlen(builtin_cmdline),
> +				       0, &cmdline_addr);
> +		if (status != EFI_SUCCESS)
> +			return NULL;
> +		while (builtin_cmdline[i] && i < COMMAND_LINE_SIZE) {
> +			((char *)cmdline_addr)[i] = builtin_cmdline[i];
> +			i++;
> +		}
> +		((char *)cmdline_addr)[i] = '\0';

Is that an strncpy() in disguise?

> +		*cmd_line_len = strlen(builtin_cmdline);

Inconsistent naming: we nave everything 'cmdline' - but the length is 
named cmd_line?

So this needs more work.

Thanks,

	Ingo

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

On Wed, May 20, 2015 at 12:42 AM, Ingo Molnar <mingo@kernel.org> wrote:
> * Matthew Garrett <mjg59@coreos.com> wrote:
> Overall structure looks better now.
>
> One problem is that it's such a huge patch: is there a way to split
> this up into smaller, more obvious, easier to bisect to patches?

Ought to be possible.

> So could we please move this into a separate arch/x86/boot/cmdline.h
> include file, so that this stuff stays separate from the various other
> helpers we nurse in boot.h?

I'll try - there's some awkwardness around how things are #included in
here, and it'll probably have to duplicate a bunch of the boilerplate
from boot.h.

>> +int __cmdline_init(unsigned long cmdline_ptr, struct boot_params *params)
>
> Please add comments describing the function. Why does it have double
> underscores, who is supposed to use it and how, etc. etc?

It seems a little odd to add that when the rest of the file follows
the same style without any documentation.

>> +             while (c)
>> +                     c = rdfs8(cptr+len++);
>> +
>
> So this needs a comment - what do we do here? Skip over to the end of
> the 'cptr' string, using 16-bit ops? Would be nice to have it in a
> suitably named helper inline.

Haha. No, it's far worse than that. I mean, yes, when it's built into
the 16-bit entry point it does that, but when it's built into the
32-bit entry point these are stubbed into nothingness and ugh all of
this is terriblee. But yeah.

>> +             len++; /* Trailing space */
>
> that's a trailing zero delimiter byte, not space, right?

No, a space - we need to separate the built-in fragment from
whatever's supplied on the command line.

>> +     while (builtin_cmdline[i]) {
>> +             len++;
>> +             i++;
>> +     }
>
> Ditto - is this:
>
>         len += strlen(builtin_cmdline+i)
>
> in disguise?

Yes, we don't have a strlen implementation we can use at this point. I
can comment that.

>> +             while (c) {
>> +                     set_fs(new_cmdline_ptr >> 4);
>> +                     wrfs8(c, nptr++);
>> +                     set_fs(cmdline_ptr >> 4);
>> +                     c = rdfs8(cptr++);
>> +             }
>> +             set_fs(new_cmdline_ptr >> 4);
>> +             wrfs8(' ', nptr++);
>> +     }
>
> So here we copy from 'cptr' to 'nptr'? Not very well named variables,
> at minimum.

I have the choice between preserving existing style or nice names. I'm
happy to pick either.

>> +
>> +     set_fs(new_cmdline_ptr >> 4);
>> +
>> +     for (i = 0; builtin_cmdline[i]; i++)
>> +             wrfs8(builtin_cmdline[i], nptr++);
>> +
>> +     wrfs8('\0', nptr);
>> +
>> +     params->hdr.cmd_line_ptr = new_cmdline_ptr;
>> +     params->setup_flags |= SETUP_CMDLINE_APPENDED;
>> +
>> +     return 0;
>
> So I'd really suggest splitting off the string manipulation parts into
> suitably named helper functions, use better variable named and comment
> it better. That will go a long way to document this non-obvious piece
> of 16-bit magic.

Like I said, this also ends up in the 32-bit code through #include
magic, so there are some awkward corner cases. I'll see what I can do.

>> +}
>> diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
>> index d7b1f65..9765a4a 100644
>> --- a/arch/x86/boot/compressed/aslr.c
>> +++ b/arch/x86/boot/compressed/aslr.c
>> @@ -170,7 +170,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>>       mem_avoid[2].size = cmd_line_size;
>>
>>       /* Avoid heap memory. */
>> -     mem_avoid[3].start = (unsigned long)free_mem_ptr;
>> +     mem_avoid[3].start = (unsigned long)orig_free_mem_ptr;
>>       mem_avoid[3].size = BOOT_HEAP_SIZE;
>
> it's totally non-obvious what's happening here. Why is it named
> 'orig'?

It's the original base of the heap, before we moved it up so the new
command line had somewhere to live.

>>
>>       /* Avoid stack memory. */
>> diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
>> index b68e303..e7a4612 100644
>> --- a/arch/x86/boot/compressed/cmdline.c
>> +++ b/arch/x86/boot/compressed/cmdline.c
>> @@ -1,6 +1,6 @@
>>  #include "misc.h"
>>
>> -#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
>> +#if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_CMDLINE_BOOL
>
> So what's common in all these Kconfig options? That they append
> something to the bootloader provided command line? If yes then please
> create an intermediary helper Kconfig switch that the above options
> select, and only use that helper config here.

No, that they interact with the command line in some way. The first
two parse it.

> So 'cmdline_init()' seems like a poorly chosen interface name. The
> cmdline is already initialized and set up for us by the bootloader.
> What we do here is that in certain configs we append to the cmdline.
> So cmdline_append() might be a better name.

Ok.

> Also, there's very little reason to inline the main API call itself,
> just do the SETUP_CMDLINE_APPENDED from within __cmdline_init and get
> rid of the underscores.

I don't think that works for the 16-bit codepath.

>>       /* Fill in upper bits of command line address, NOP on 32 bit  */
>>       boot_params->ext_cmd_line_ptr = (u64)(unsigned long)cmdline_ptr >> 32;
>>
>> +     if (append_cmdline)
>> +             boot_params->setup_flags |= SETUP_CMDLINE_APPENDED;
>
> So why not have a boot_params__set_cmdline_appended() helper:
>
>         boot_params__set_cmdline_appended(boot_params);
>
> instead of this weird flaggery in the include file:
>
> +#ifdef CONFIG_CMDLINE_OVERRIDE
> +static char append_cmdline;
> +#else
> +static char append_cmdline = 1;
> +#endif

Ok, that's cleaner.

>> +     orig_free_mem_ptr = free_mem_ptr = heap;        /* Heap */
>> +     free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
>> +
>> +     cmdline_init();
>> +
>>       console_init();
>>       debug_putstr("early console in decompress_kernel\n");
>>
>> -     free_mem_ptr     = heap;        /* Heap */
>> -     free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
>
> So I don't understand the old heap logic in
> arch/x86/boot/compressed/misc.c, let alone the new one.
>
> So low level boot assembly code sets up the boot_heap, and passes it
> to decompress_kernel() as a 'heap' C function argument. Which then
> stores it in free_mem_ptr and free_mem_end_ptr but does not otherwise
> use it other than some sanity checks.
>
> As per the comments in the code decompression is supposed to use this
> heap, but where does it figure out the address from? It's not passed
> to it in any greppable fashion that I can see.

malloc() in include/linux/decompress/mm.h.

> Confusingly enough, there appears to be another, real mode heap
> mechanism in realmode/rm/stack.S and boot/boot.h, around rm_heap at
> all.

Yup.

> Plus there's both boot/boot.h and include/asm/x86/boot.h, which
> confused me some more.
>
> So this code is highly confusing. It first needs to be unconfused.

I'm not even going there - I don't understand that code well enough to
reimplement it. If that's a dealbreaker then we'll just have to drop
this patch.

>> +     /* End of heap check */
>> +     init_heap();
>
> So init_heap() could need some extra comments as well.

That's existing code that I'm just moving so the heap exists prior to
us handling the command line. Interfering with it would seem to result
in the patch being more confusing?

>> -     __u8  _pad2[4];                                 /* 0x054 */
>> +     __u32 setup_flags;                              /* 0x054 */
>>       __u64  tboot_addr;                              /* 0x058 */
>>       struct ist_info ist_info;                       /* 0x060 */
>>       __u8  _pad3[16];                                /* 0x070 */
>
> So data structures should be clearly explained. Who sets this flag,
> when, for what purpose, etc.

That's documented with the rest of the boot protocol.

> Using 'setup' here is highly ambiguous: the whole boot process is an
> act of 'setting up stuff'. So it's very non-obvious what this does, at
> first and second glance as well.

They're flags used during setup - there's only one at the moment, but
any others might end up scattered anywhere over the x86 init process,
so they really do potentially apply to the entire setup process.

> Why is all this in a high level function named setup_arch()? At
> minimum it should be factored out into a suitably named helper
> function.
>
> With that function we can also do an early:
>
>         if (!builtin_cmdline)
>                 return;
>
> which gives us an extra identation level which will fix all those ugly
> line breaks later on. (and ignore checkpatch if it's slightly above
> col80)

Ok.

> So this is the third time I've seen this repeating pattern in this
> patch, with only slight variations.
>
> Could we create a generic helper module that does most of this, with
> small headers defining the various boot environment specialities,
> linked into the various loader functionalities separately?
>
> That would have various advantages, beyond being easier to read: such
> as adding new functionality to our command line string handling could
> be done in a single place.
>
> So something like:
>
>         arch/x86/boot/cmdline/core.c
>         arch/x86/boot/cmdline/16-bit.h
>         arch/x86/boot/cmdline/32-bit.h
>         arch/x86/boot/cmdline/efi.h
>
> ... or so, glued into their respective boot stages.

Hrm. Possibly? The way the 16-bit and 32-bit code incorporate each
other make this rather more difficult, but there's some hope.

> Looks like there's very little 'EFI' about this strlcat
> implementation, right? So why don't we use the real one, is it not
> available yet?

Right - we're still in the firmware at this point, not the kernel.

>> +
>>  /*
>>   * Convert the unicode UEFI command line to ASCII to pass to kernel.
>>   * Size of memory allocated return in *cmd_line_len.
>
> Couldn't help but notice the various spelling errors in this comment
> block...

Yeah.

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Ingo Molnar]

* Matthew Garrett <matthew.garrett@coreos.com> wrote:

> >> +             while (c) {
> >> +                     set_fs(new_cmdline_ptr >> 4);
> >> +                     wrfs8(c, nptr++);
> >> +                     set_fs(cmdline_ptr >> 4);
> >> +                     c = rdfs8(cptr++);
> >> +             }
> >> +             set_fs(new_cmdline_ptr >> 4);
> >> +             wrfs8(' ', nptr++);
> >> +     }
> >
> > So here we copy from 'cptr' to 'nptr'? Not very well named variables,
> > at minimum.
> 
> I have the choice between preserving existing style or nice names.
> I'm happy to pick either.

So my high level concerns (and conditions) are the following:

We should first do cleanups and reorganization so that the feature addition 
(allowing EFI to parse certain options early on) you are implementing becomes 
obvious and simple.

I.e. we need to first prove it's all maintainable and readable, and then only can 
we add a feature with such a signature:

 13 files changed, 236 insertions(+), 43 deletions(-)

I'd also expect that a good chunk of that linecount increase would disappear after 
a proper set of reorganization, because all that duplication would disappear 
mostly.

It's all pretty non-trivial, so it has to be finegrained, perfectly bisectable, 
etc.

Thanks,

	Ingo

Re: [PATCH V2] x86: Allow built-in command line to work in early kernel init

[Matthew Garrett]

On Wed, Jun 3, 2015 at 2:24 AM, Ingo Molnar <mingo@kernel.org> wrote:
> It's all pretty non-trivial, so it has to be finegrained, perfectly bisectable,
> etc.

Yeah. I'm not really going to be able to justify the time for that -
this is code that dates back a long way, there's undoubtedly
subtleties throughout and the potential for breaking things is pretty
significant (plus I'm certainly not sure that I understand it all - I
grew up on actual CPUs, not these awful segmented things). I'll figure
out something we can use locally and leave this for now. Thanks for
the feedback!