Message-ID: <20150520074231.GA25959@gmail.com>
Date:	Wed, 20 May 2015 09:42:31 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Matthew Garrett <mjg59@...eos.com>
Cc:	hpa@...or.com, yinghai@...nel.org, x86@...nel.org,
	mingo@...hat.com, tglx@...utronix.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] x86: Allow built-in command line to work in early
 kernel init


* Matthew Garrett <mjg59@...eos.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@...eos.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists