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 <[email protected]>
---
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
* Matthew Garrett <[email protected]> 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 <[email protected]>
> ---
>
> 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
* Matthew Garrett <[email protected]> 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
On Wed, Jun 3, 2015 at 2:24 AM, Ingo Molnar <[email protected]> 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!