[v2,04/14] cmd: add efishell command

Currently, there is no easy way to add or modify UEFI variables.
In particular, bootmgr supports BootOrder/BootXXXX variables, it is
quite hard to define them as u-boot variables because they are represented
in a complicated and encoded format.

The new command, efishell, helps address these issues and give us
more friendly interfaces:
 * efishell boot add: add BootXXXX variable
 * efishell boot rm: remove BootXXXX variable
 * efishell boot dump: display all BootXXXX variables
 * efishell boot order: set/display a boot order (BootOrder)
 * efishell setvar: set an UEFI variable (with limited functionality)
 * efishell dumpvar: display all UEFI variables

As the name suggests, this command basically provides a subset fo UEFI
shell commands with simplified functionality.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/Makefile   |   2 +-
 cmd/efishell.c | 673 +++++++++++++++++++++++++++++++++++++++++++++++++
 cmd/efishell.h |   5 +
 3 files changed, 679 insertions(+), 1 deletion(-)
 create mode 100644 cmd/efishell.c
 create mode 100644 cmd/efishell.h

On 05.11.18 10:06, AKASHI Takahiro wrote:
> Currently, there is no easy way to add or modify UEFI variables.
> In particular, bootmgr supports BootOrder/BootXXXX variables, it is
> quite hard to define them as u-boot variables because they are represented
> in a complicated and encoded format.
> 
> The new command, efishell, helps address these issues and give us
> more friendly interfaces:
>  * efishell boot add: add BootXXXX variable
>  * efishell boot rm: remove BootXXXX variable
>  * efishell boot dump: display all BootXXXX variables
>  * efishell boot order: set/display a boot order (BootOrder)
>  * efishell setvar: set an UEFI variable (with limited functionality)
>  * efishell dumpvar: display all UEFI variables
> 
> As the name suggests, this command basically provides a subset fo UEFI
> shell commands with simplified functionality.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/Makefile   |   2 +-
>  cmd/efishell.c | 673 +++++++++++++++++++++++++++++++++++++++++++++++++
>  cmd/efishell.h |   5 +
>  3 files changed, 679 insertions(+), 1 deletion(-)
>  create mode 100644 cmd/efishell.c
>  create mode 100644 cmd/efishell.h
> 
> diff --git a/cmd/Makefile b/cmd/Makefile
> index d9cdaf6064b8..bd531d505a8e 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -24,7 +24,7 @@ obj-$(CONFIG_CMD_BINOP) += binop.o
>  obj-$(CONFIG_CMD_BLOCK_CACHE) += blkcache.o
>  obj-$(CONFIG_CMD_BMP) += bmp.o
>  obj-$(CONFIG_CMD_BOOTCOUNT) += bootcount.o
> -obj-$(CONFIG_CMD_BOOTEFI) += bootefi.o
> +obj-$(CONFIG_CMD_BOOTEFI) += bootefi.o efishell.o

Please create a separate command line option for efishell and make it
disabled by default.

I think it's a really really useful debugging aid, but in a normal
environment people should only need to run efi binaries (plus bootmgr)
and modify efi variables, no?



>  obj-$(CONFIG_CMD_BOOTMENU) += bootmenu.o
>  obj-$(CONFIG_CMD_BOOTSTAGE) += bootstage.o
>  obj-$(CONFIG_CMD_BOOTZ) += bootz.o
> diff --git a/cmd/efishell.c b/cmd/efishell.c
> new file mode 100644
> index 000000000000..abc8216c7bd6
> --- /dev/null
> +++ b/cmd/efishell.c
> @@ -0,0 +1,673 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  EFI Shell-like command
> + *
> + *  Copyright (c) 2018 AKASHI Takahiro, Linaro Limited
> + */
> +
> +#include <charset.h>
> +#include <common.h>
> +#include <command.h>
> +#include <efi_loader.h>
> +#include <environment.h>
> +#include <errno.h>
> +#include <exports.h>
> +#include <hexdump.h>
> +#include <malloc.h>
> +#include <search.h>
> +#include <linux/ctype.h>
> +#include <asm/global_data.h>
> +#include "efishell.h"
> +
> +DECLARE_GLOBAL_DATA_PTR;

Where in this file do you need gd?

> +
> +static void dump_var_data(char *data, unsigned long len)
> +{
> +	char *start, *end, *p;
> +	unsigned long pos, count;
> +	char hex[3], line[9];
> +	int i;
> +
> +	end = data + len;
> +	for (start = data, pos = 0; start < end; start += count, pos += count) {
> +		count = end - start;
> +		if (count > 16)
> +			count = 16;
> +
> +		/* count should be multiple of two */
> +		printf("%08lx: ", pos);
> +
> +		/* in hex format */
> +		p = start;
> +		for (i = 0; i < count / 2; p += 2, i++)
> +			printf(" %c%c", *p, *(p + 1));
> +		for (; i < 8; i++)
> +			printf("   ");
> +
> +		/* in character format */
> +		p = start;
> +		hex[2] = '\0';
> +		for (i = 0; i < count / 2; i++) {
> +			hex[0] = *p++;
> +			hex[1] = *p++;
> +			line[i] = simple_strtoul(hex, 0, 16);
> +			if (line[i] < 0x20 || line[i] > 0x7f)
> +				line[i] = '.';
> +		}
> +		line[i] = '\0';
> +		printf("  %s\n", line);
> +	}
> +}
> +
> +/*
> + * From efi_variable.c,
> + *
> + * Mapping between EFI variables and u-boot variables:
> + *
> + *   efi_$guid_$varname = {attributes}(type)value
> + */
> +static int do_efi_dump_var(int argc, char * const argv[])
> +{
> +	char regex[256];
> +	char * const regexlist[] = {regex};
> +	char *res = NULL, *start, *end;
> +	int len;
> +
> +	if (argc > 2)
> +		return CMD_RET_USAGE;
> +
> +	if (argc == 2)
> +		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_%s", argv[1]);
> +	else
> +		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");
> +	debug("%s:%d grep uefi var %s\n", __func__, __LINE__, regex);
> +
> +	len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
> +			&res, 0, 1, regexlist);
> +
> +	if (len < 0)
> +		return CMD_RET_FAILURE;
> +
> +	if (len > 0) {
> +		end = res;
> +		while (true) {
> +			/* variable name */
> +			start = strchr(end, '_');
> +			if (!start)
> +				break;
> +			start = strchr(++start, '_');
> +			if (!start)
> +				break;
> +			end = strchr(++start, '=');
> +			if (!end)
> +				break;
> +			printf("%.*s:", (int)(end - start), start);
> +			end++;
> +
> +			/* value */
> +			start = end;
> +			end = strstr(start, "(blob)");
> +			if (!end) {
> +				putc('\n');
> +				break;
> +			}
> +			end += 6;
> +			printf(" %.*s\n", (int)(end - start), start);
> +
> +			start = end;
> +			end = strchr(start, '\n');
> +			if (!end)
> +				break;
> +			dump_var_data(start,  end - start);
> +		}
> +		free(res);
> +
> +		if (len < 2 && argc == 2)
> +			printf("%s: not found\n", argv[1]);
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static int append_value(char **bufp, unsigned long *sizep, char *data)
> +{
> +	char *tmp_buf = NULL, *new_buf = NULL, *value;
> +	unsigned long len = 0;
> +
> +	if (!strncmp(data, "=0x", 2)) { /* hexadecimal number */
> +		union {
> +			u8 u8;
> +			u16 u16;
> +			u32 u32;
> +			u64 u64;
> +		} tmp_data;
> +		unsigned long hex_value;
> +		void *hex_ptr;
> +
> +		data += 3;
> +		len = strlen(data);
> +		if ((len & 0x1)) /* not multiple of two */
> +			return -1;
> +
> +		len /= 2;
> +		if (len > 8)
> +			return -1;
> +		else if (len > 4)
> +			len = 8;
> +		else if (len > 2)
> +			len = 4;
> +
> +		/* convert hex hexadecimal number */
> +		if (strict_strtoul(data, 16, &hex_value) < 0)
> +			return -1;
> +
> +		tmp_buf = malloc(len);
> +		if (!tmp_buf)
> +			return -1;
> +
> +		if (len == 1) {
> +			tmp_data.u8 = hex_value;
> +			hex_ptr = &tmp_data.u8;
> +		} else if (len == 2) {
> +			tmp_data.u16 = hex_value;
> +			hex_ptr = &tmp_data.u16;
> +		} else if (len == 4) {
> +			tmp_data.u32 = hex_value;
> +			hex_ptr = &tmp_data.u32;
> +		} else {
> +			tmp_data.u64 = hex_value;
> +			hex_ptr = &tmp_data.u64;
> +		}
> +		memcpy(tmp_buf, hex_ptr, len);
> +		value = tmp_buf;
> +
> +	} else if (!strncmp(data, "=H", 2)) { /* hexadecimal-byte array */
> +		data += 2;
> +		len = strlen(data);
> +		if (len & 0x1) /* not multiple of two */
> +			return -1;
> +
> +		len /= 2;
> +		tmp_buf = malloc(len);
> +		if (!tmp_buf)
> +			return -1;
> +
> +		if (hex2bin((u8 *)tmp_buf, data, len) < 0)
> +			return -1;
> +
> +		value = tmp_buf;
> +	} else { /* string */
> +		if (!strncmp(data, "=\"", 2) || !strncmp(data, "=S\"", 3)) {
> +			if (data[1] == '"')
> +				data += 2;
> +			else
> +				data += 3;
> +			value = data;
> +			len = strlen(data) - 1;
> +			if (data[len] != '"')
> +				return -1;
> +		} else {
> +			value = data;
> +			len = strlen(data);
> +		}
> +	}
> +
> +	new_buf = realloc(*bufp, *sizep + len);
> +	if (!new_buf)
> +		goto out;
> +
> +	memcpy(new_buf + *sizep, value, len);
> +	*bufp = new_buf;
> +	*sizep += len;
> +
> +out:
> +	free(tmp_buf);
> +
> +	return 0;
> +}
> +
> +static int do_efi_set_var(int argc, char * const argv[])
> +{
> +	char *var_name, *value = NULL;
> +	unsigned long size = 0;
> +	u16 *var_name16, *p;
> +	efi_guid_t guid;
> +	efi_status_t ret;
> +
> +	if (argc == 1)
> +		return CMD_RET_SUCCESS;
> +
> +	var_name = argv[1];
> +	if (argc == 2) {
> +		/* remove */
> +		value = NULL;
> +		size = 0;
> +	} else { /* set */
> +		argc -= 2;
> +		argv += 2;
> +
> +		for ( ; argc > 0; argc--, argv++)
> +			if (append_value(&value, &size, argv[0]) < 0) {
> +				ret = CMD_RET_FAILURE;
> +				goto out;
> +			}
> +	}
> +
> +	var_name16 = malloc((strlen(var_name) + 1) * 2);
> +	p = var_name16;
> +	utf8_utf16_strncpy(&p, var_name, strlen(var_name) + 1);
> +
> +	guid = efi_global_variable_guid;
> +	ret = efi_set_variable(var_name16, &guid,
> +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +			       EFI_VARIABLE_RUNTIME_ACCESS, size, value);
> +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> +out:
> +	return ret;
> +}
> +
> +static int do_efi_boot_add(int argc, char * const argv[])
> +{
> +	int id;
> +	char *endp;
> +	char var_name[9];
> +	u16 var_name16[9], *p;
> +	efi_guid_t guid;
> +	size_t label_len, label_len16;
> +	u16 *label;
> +	struct efi_device_path *device_path = NULL, *file_path = NULL;
> +	struct efi_load_option lo;
> +	void *data = NULL;
> +	unsigned long size;
> +	int ret;
> +
> +	if (argc < 6 || argc > 7)
> +		return CMD_RET_USAGE;
> +
> +	id = (int)simple_strtoul(argv[1], &endp, 0);
> +	if (*endp != '\0' || id > 0xffff)
> +		return CMD_RET_FAILURE;
> +
> +	sprintf(var_name, "Boot%04X", id);
> +	p = var_name16;
> +	utf8_utf16_strncpy(&p, var_name, 9);
> +
> +	guid = efi_global_variable_guid;
> +
> +	/* attributes */
> +	lo.attributes = 0x1; /* always ACTIVE */
> +
> +	/* label */
> +	label_len = strlen(argv[2]);
> +	label_len16 = utf8_utf16_strnlen(argv[2], label_len);
> +	label = malloc((label_len16 + 1) * sizeof(u16));
> +	if (!label)
> +		return CMD_RET_FAILURE;
> +	lo.label = label; /* label will be changed below */
> +	utf8_utf16_strncpy(&label, argv[2], label_len);
> +
> +	/* file path */
> +	ret = efi_dp_from_name(argv[3], argv[4], argv[5], &device_path,
> +			       &file_path);
> +	if (ret != EFI_SUCCESS) {
> +		ret = CMD_RET_FAILURE;
> +		goto out;
> +	}
> +	lo.file_path = file_path;
> +	lo.file_path_length = efi_dp_size(file_path)
> +				+ sizeof(struct efi_device_path); /* for END */
> +
> +	/* optional data */
> +	lo.optional_data = (u8 *)(argc == 6 ? "" : argv[6]);
> +
> +	size = efi_serialize_load_option(&lo, (u8 **)&data);
> +	if (!size) {
> +		ret = CMD_RET_FAILURE;
> +		goto out;
> +	}
> +
> +	ret = efi_set_variable(var_name16, &guid,
> +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +			       EFI_VARIABLE_RUNTIME_ACCESS, size, data);
> +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> +out:
> +	free(data);
> +#if 0 /* FIXME */

Eh, what? :)

> +	free(device_path);
> +	free(file_path);
> +#endif
> +	free(lo.label);
> +	return ret;
> +}
> +
> +static int do_efi_boot_rm(int argc, char * const argv[])
> +{
> +	efi_guid_t guid;
> +	int id, i;
> +	char *endp;
> +	char var_name[9];
> +	u16 var_name16[9];
> +	efi_status_t ret;
> +
> +	if (argc == 1)
> +		return CMD_RET_USAGE;
> +
> +	guid = efi_global_variable_guid;
> +	for (i = 1; i < argc; i++, argv++) {
> +		id = (int)simple_strtoul(argv[1], &endp, 0);
> +		if (*endp != '\0' || id > 0xffff)
> +			return CMD_RET_FAILURE;
> +
> +		sprintf(var_name, "Boot%04X", id);
> +		utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9);
> +
> +		ret = efi_set_variable(var_name16, &guid,
> +				       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				       EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL);
> +		if (ret) {
> +			printf("cannot remove Boot%04X", id);
> +			return CMD_RET_FAILURE;
> +		}
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static void show_efi_boot_opt_data(int id, void *data)
> +{
> +	struct efi_load_option lo;
> +	char *label, *p;
> +	size_t label_len16, label_len;
> +	u16 *dp_str;
> +
> +	efi_deserialize_load_option(&lo, data);
> +
> +	label_len16 = u16_strlen(lo.label);
> +	label_len = utf16_utf8_strnlen(lo.label, label_len16);
> +	label = malloc(label_len + 1);
> +	if (!label)
> +		return;
> +	p = label;
> +	utf16_utf8_strncpy(&p, lo.label, label_len16);
> +
> +	printf("Boot%04X:\n", id);
> +	printf("\tattributes: %c%c%c (0x%08x)\n",
> +	       /* ACTIVE */
> +	       lo.attributes & 0x1 ? 'A' : '-',
> +	       /* FORCE RECONNECT */
> +	       lo.attributes & 0x2 ? 'R' : '-',
> +	       /* HIDDEN */
> +	       lo.attributes & 0x8 ? 'H' : '-',
> +	       lo.attributes);
> +	printf("\tlabel: %s\n", label);
> +
> +	dp_str = efi_dp_str(lo.file_path);
> +	printf("\tfile_path: %ls\n", dp_str);
> +	efi_free_pool(dp_str);
> +
> +	printf("\tdata: %s\n", lo.optional_data);
> +
> +	free(label);
> +}
> +
> +static void show_efi_boot_opt(int id)
> +{
> +	char var_name[9];
> +	u16 var_name16[9], *p;
> +	efi_guid_t guid;
> +	void *data = NULL;
> +	unsigned long size;
> +	int ret;
> +
> +	sprintf(var_name, "Boot%04X", id);
> +	p = var_name16;
> +	utf8_utf16_strncpy(&p, var_name, 9);
> +	guid = efi_global_variable_guid;
> +
> +	size = 0;
> +	ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
> +	if (ret == (int)EFI_BUFFER_TOO_SMALL) {
> +		data = malloc(size);
> +		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
> +	}
> +	if (ret == EFI_SUCCESS) {
> +		show_efi_boot_opt_data(id, data);
> +		free(data);
> +	} else if (ret == EFI_NOT_FOUND) {
> +		printf("Boot%04X: not found\n", id);

Missing free?

> +	}
> +}
> +
> +static int do_efi_boot_dump(int argc, char * const argv[])
> +{
> +	char regex[256];
> +	char * const regexlist[] = {regex};
> +	char *variables = NULL, *boot, *value;
> +	int len;
> +	int id;
> +
> +	if (argc > 1)
> +		return CMD_RET_USAGE;
> +
> +	snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+");
> +
> +	len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
> +			&variables, 0, 1, regexlist);
> +
> +	if (!len)
> +		return CMD_RET_SUCCESS;
> +
> +	if (len < 0)
> +		return CMD_RET_FAILURE;
> +
> +	boot = variables;
> +	while (*boot) {
> +		value = strstr(boot, "Boot") + 4;
> +		id = (int)simple_strtoul(value, NULL, 16);
> +		show_efi_boot_opt(id);
> +		boot = strchr(boot, '\n');
> +		if (!*boot)
> +			break;
> +		boot++;
> +	}
> +	free(variables);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static int show_efi_boot_order(void)
> +{
> +	efi_guid_t guid;
> +	u16 *bootorder = NULL;
> +	unsigned long size;
> +	int num, i;
> +	char var_name[9];
> +	u16 var_name16[9], *p16;
> +	void *data;
> +	struct efi_load_option lo;
> +	char *label, *p;
> +	size_t label_len16, label_len;
> +	efi_status_t ret = CMD_RET_SUCCESS;
> +
> +	guid = efi_global_variable_guid;
> +	size = 0;
> +	ret = efi_get_variable(L"BootOrder", &guid, NULL, &size, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		bootorder = malloc(size);
> +		ret = efi_get_variable(L"BootOrder", &guid, NULL, &size,
> +				       bootorder);
> +	}
> +	if (ret != EFI_SUCCESS) {
> +		printf("BootOrder not defined\n");

Missing free(bootorder)

> +		return CMD_RET_SUCCESS;
> +	}
> +
> +	num = size / sizeof(u16);
> +	for (i = 0; i < num; i++) {
> +		sprintf(var_name, "Boot%04X", bootorder[i]);
> +		p16 = var_name16;
> +		utf8_utf16_strncpy(&p16, var_name, 9);
> +
> +		size = 0;
> +		ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
> +		if (ret != EFI_BUFFER_TOO_SMALL) {
> +			printf("%2d: Boot%04X: (not defined)\n",
> +			       i + 1, bootorder[i]);
> +			continue;
> +		}
> +
> +		data = malloc(size);
> +		if (!data) {
> +			ret = CMD_RET_FAILURE;
> +			goto out;
> +		}
> +		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
> +		if (ret != EFI_SUCCESS) {
> +			free(data);
> +			ret = CMD_RET_FAILURE;
> +			goto out;

Probably better to free(data) at out:, no?

> +		}
> +
> +		efi_deserialize_load_option(&lo, data);
> +
> +		label_len16 = u16_strlen(lo.label);
> +		label_len = utf16_utf8_strnlen(lo.label, label_len16);
> +		label = malloc(label_len + 1);
> +		if (!label) {
> +			free(data);
> +			ret = CMD_RET_FAILURE;
> +			goto out;
> +		}
> +		p = label;
> +		utf16_utf8_strncpy(&p, lo.label, label_len16);
> +		printf("%2d: Boot%04X: %s\n", i + 1, bootorder[i], label);
> +		free(label);
> +
> +		free(data);
> +	}
> +out:
> +	free(bootorder);
> +
> +	return ret;
> +}
> +
> +static int do_efi_boot_order(int argc, char * const argv[])
> +{
> +	u16 *bootorder = NULL;
> +	unsigned long size;
> +	int id, i;
> +	char *endp;
> +	efi_guid_t guid;
> +	efi_status_t ret;
> +
> +	if (argc == 1)
> +		return show_efi_boot_order();
> +
> +	argc--;
> +	argv++;
> +
> +	size = argc * sizeof(u16);
> +	bootorder = malloc(size);
> +	if (!bootorder)
> +		return CMD_RET_FAILURE;
> +
> +	for (i = 0; i < argc; i++) {
> +		id = (int)simple_strtoul(argv[i], &endp, 0);
> +		if (*endp != '\0' || id > 0xffff) {
> +			printf("invalid value: %s\n", argv[i]);
> +			ret = CMD_RET_FAILURE;
> +			goto out;
> +		}
> +
> +		bootorder[i] = (u16)id;
> +	}
> +
> +	guid = efi_global_variable_guid;
> +	ret = efi_set_variable(L"BootOrder", &guid,
> +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +			       EFI_VARIABLE_RUNTIME_ACCESS, size, bootorder);
> +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> +out:
> +	free(bootorder);
> +
> +	return ret;
> +}
> +
> +static int do_efi_boot_opt(int argc, char * const argv[])
> +{
> +	char *sub_command;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	argc--; argv++;
> +	sub_command = argv[0];
> +
> +	if (!strcmp(sub_command, "add"))
> +		return do_efi_boot_add(argc, argv);
> +	else if (!strcmp(sub_command, "rm"))
> +		return do_efi_boot_rm(argc, argv);
> +	else if (!strcmp(sub_command, "dump"))
> +		return do_efi_boot_dump(argc, argv);
> +	else if (!strcmp(sub_command, "order"))
> +		return do_efi_boot_order(argc, argv);
> +	else
> +		return CMD_RET_USAGE;
> +}
> +
> +/* Interpreter command to configure EFI environment */
> +static int do_efishell(cmd_tbl_t *cmdtp, int flag,
> +		       int argc, char * const argv[])
> +{
> +	char *command;
> +	efi_status_t r;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	argc--; argv++;
> +	command = argv[0];
> +
> +	/* Initialize EFI drivers */
> +	r = efi_init_obj_list();
> +	if (r != EFI_SUCCESS) {
> +		printf("Error: Cannot set up EFI drivers, r = %lu\n",
> +		       r & ~EFI_ERROR_MASK);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	if (!strcmp(command, "boot"))
> +		return do_efi_boot_opt(argc, argv);
> +	else if (!strcmp(command, "dumpvar") || !strcmp(command, "dmpstore"))
> +		return do_efi_dump_var(argc, argv);
> +	else if (!strcmp(command, "setvar"))
> +		return do_efi_set_var(argc, argv);
> +	else
> +		return CMD_RET_USAGE;
> +}
> +
> +#ifdef CONFIG_SYS_LONGHELP
> +static char efishell_help_text[] =
> +	"  - EFI Shell-like interface to configure EFI environment\n"
> +	"\n"
> +	"efishell boot add <bootid> <label> <interface> <device>[:<part>] <file path> <option>\n"
> +	"  - set uefi's BOOT variable\n"
> +	"efishell boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
> +	"  - set/display uefi boot order\n"
> +	"efishell boot dump\n"
> +	"  - display all uefi's BOOT variables\n"
> +	"efishell boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> +	"  - set/display uefi boot order\n"
> +	"\n"
> +	"efishell dumpvar [<name>]\n"
> +	"  - get uefi variable's value\n"
> +	"efishell setvar <name> [<value>]\n"
> +	"  - set/delete uefi variable's value\n"
> +	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n";
> +#endif
> +
> +U_BOOT_CMD(
> +	efishell, 10, 0, do_efishell,
> +	"Configure EFI environment",
> +	efishell_help_text
> +);
> diff --git a/cmd/efishell.h b/cmd/efishell.h
> new file mode 100644
> index 000000000000..6f70b402b26b
> --- /dev/null
> +++ b/cmd/efishell.h
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <efi.h>
> +
> +efi_status_t efi_init_obj_list(void);

Why isn't this in efi_loader.h? That's the subsystem that exports it, no?


Alex

On Mon, Dec 03, 2018 at 12:42:43AM +0100, Alexander Graf wrote:
> 
> 
> On 05.11.18 10:06, AKASHI Takahiro wrote:
> > Currently, there is no easy way to add or modify UEFI variables.
> > In particular, bootmgr supports BootOrder/BootXXXX variables, it is
> > quite hard to define them as u-boot variables because they are represented
> > in a complicated and encoded format.
> > 
> > The new command, efishell, helps address these issues and give us
> > more friendly interfaces:
> >  * efishell boot add: add BootXXXX variable
> >  * efishell boot rm: remove BootXXXX variable
> >  * efishell boot dump: display all BootXXXX variables
> >  * efishell boot order: set/display a boot order (BootOrder)
> >  * efishell setvar: set an UEFI variable (with limited functionality)
> >  * efishell dumpvar: display all UEFI variables
> > 
> > As the name suggests, this command basically provides a subset fo UEFI
> > shell commands with simplified functionality.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/Makefile   |   2 +-
> >  cmd/efishell.c | 673 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  cmd/efishell.h |   5 +
> >  3 files changed, 679 insertions(+), 1 deletion(-)
> >  create mode 100644 cmd/efishell.c
> >  create mode 100644 cmd/efishell.h
> > 
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index d9cdaf6064b8..bd531d505a8e 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -24,7 +24,7 @@ obj-$(CONFIG_CMD_BINOP) += binop.o
> >  obj-$(CONFIG_CMD_BLOCK_CACHE) += blkcache.o
> >  obj-$(CONFIG_CMD_BMP) += bmp.o
> >  obj-$(CONFIG_CMD_BOOTCOUNT) += bootcount.o
> > -obj-$(CONFIG_CMD_BOOTEFI) += bootefi.o
> > +obj-$(CONFIG_CMD_BOOTEFI) += bootefi.o efishell.o
> 
> Please create a separate command line option for efishell and make it
> disabled by default.

OK.

> I think it's a really really useful debugging aid, but in a normal
> environment people should only need to run efi binaries (plus bootmgr)
> and modify efi variables, no?

The ultimate goal would be to provide this command as a separate
application. In this case, however, it won't much different from
uefi shell itself :)

> 
> 
> >  obj-$(CONFIG_CMD_BOOTMENU) += bootmenu.o
> >  obj-$(CONFIG_CMD_BOOTSTAGE) += bootstage.o
> >  obj-$(CONFIG_CMD_BOOTZ) += bootz.o
> > diff --git a/cmd/efishell.c b/cmd/efishell.c
> > new file mode 100644
> > index 000000000000..abc8216c7bd6
> > --- /dev/null
> > +++ b/cmd/efishell.c
> > @@ -0,0 +1,673 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  EFI Shell-like command
> > + *
> > + *  Copyright (c) 2018 AKASHI Takahiro, Linaro Limited
> > + */
> > +
> > +#include <charset.h>
> > +#include <common.h>
> > +#include <command.h>
> > +#include <efi_loader.h>
> > +#include <environment.h>
> > +#include <errno.h>
> > +#include <exports.h>
> > +#include <hexdump.h>
> > +#include <malloc.h>
> > +#include <search.h>
> > +#include <linux/ctype.h>
> > +#include <asm/global_data.h>
> > +#include "efishell.h"
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> 
> Where in this file do you need gd?

OK.
# I thought this should always be declared in any file by default.

> > +
> > +static void dump_var_data(char *data, unsigned long len)
> > +{
> > +	char *start, *end, *p;
> > +	unsigned long pos, count;
> > +	char hex[3], line[9];
> > +	int i;
> > +
> > +	end = data + len;
> > +	for (start = data, pos = 0; start < end; start += count, pos += count) {
> > +		count = end - start;
> > +		if (count > 16)
> > +			count = 16;
> > +
> > +		/* count should be multiple of two */
> > +		printf("%08lx: ", pos);
> > +
> > +		/* in hex format */
> > +		p = start;
> > +		for (i = 0; i < count / 2; p += 2, i++)
> > +			printf(" %c%c", *p, *(p + 1));
> > +		for (; i < 8; i++)
> > +			printf("   ");
> > +
> > +		/* in character format */
> > +		p = start;
> > +		hex[2] = '\0';
> > +		for (i = 0; i < count / 2; i++) {
> > +			hex[0] = *p++;
> > +			hex[1] = *p++;
> > +			line[i] = simple_strtoul(hex, 0, 16);
> > +			if (line[i] < 0x20 || line[i] > 0x7f)
> > +				line[i] = '.';
> > +		}
> > +		line[i] = '\0';
> > +		printf("  %s\n", line);
> > +	}
> > +}
> > +
> > +/*
> > + * From efi_variable.c,
> > + *
> > + * Mapping between EFI variables and u-boot variables:
> > + *
> > + *   efi_$guid_$varname = {attributes}(type)value
> > + */
> > +static int do_efi_dump_var(int argc, char * const argv[])
> > +{
> > +	char regex[256];
> > +	char * const regexlist[] = {regex};
> > +	char *res = NULL, *start, *end;
> > +	int len;
> > +
> > +	if (argc > 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	if (argc == 2)
> > +		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_%s", argv[1]);
> > +	else
> > +		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");
> > +	debug("%s:%d grep uefi var %s\n", __func__, __LINE__, regex);
> > +
> > +	len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
> > +			&res, 0, 1, regexlist);
> > +
> > +	if (len < 0)
> > +		return CMD_RET_FAILURE;
> > +
> > +	if (len > 0) {
> > +		end = res;
> > +		while (true) {
> > +			/* variable name */
> > +			start = strchr(end, '_');
> > +			if (!start)
> > +				break;
> > +			start = strchr(++start, '_');
> > +			if (!start)
> > +				break;
> > +			end = strchr(++start, '=');
> > +			if (!end)
> > +				break;
> > +			printf("%.*s:", (int)(end - start), start);
> > +			end++;
> > +
> > +			/* value */
> > +			start = end;
> > +			end = strstr(start, "(blob)");
> > +			if (!end) {
> > +				putc('\n');
> > +				break;
> > +			}
> > +			end += 6;
> > +			printf(" %.*s\n", (int)(end - start), start);
> > +
> > +			start = end;
> > +			end = strchr(start, '\n');
> > +			if (!end)
> > +				break;
> > +			dump_var_data(start,  end - start);
> > +		}
> > +		free(res);
> > +
> > +		if (len < 2 && argc == 2)
> > +			printf("%s: not found\n", argv[1]);
> > +	}
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> > +static int append_value(char **bufp, unsigned long *sizep, char *data)
> > +{
> > +	char *tmp_buf = NULL, *new_buf = NULL, *value;
> > +	unsigned long len = 0;
> > +
> > +	if (!strncmp(data, "=0x", 2)) { /* hexadecimal number */
> > +		union {
> > +			u8 u8;
> > +			u16 u16;
> > +			u32 u32;
> > +			u64 u64;
> > +		} tmp_data;
> > +		unsigned long hex_value;
> > +		void *hex_ptr;
> > +
> > +		data += 3;
> > +		len = strlen(data);
> > +		if ((len & 0x1)) /* not multiple of two */
> > +			return -1;
> > +
> > +		len /= 2;
> > +		if (len > 8)
> > +			return -1;
> > +		else if (len > 4)
> > +			len = 8;
> > +		else if (len > 2)
> > +			len = 4;
> > +
> > +		/* convert hex hexadecimal number */
> > +		if (strict_strtoul(data, 16, &hex_value) < 0)
> > +			return -1;
> > +
> > +		tmp_buf = malloc(len);
> > +		if (!tmp_buf)
> > +			return -1;
> > +
> > +		if (len == 1) {
> > +			tmp_data.u8 = hex_value;
> > +			hex_ptr = &tmp_data.u8;
> > +		} else if (len == 2) {
> > +			tmp_data.u16 = hex_value;
> > +			hex_ptr = &tmp_data.u16;
> > +		} else if (len == 4) {
> > +			tmp_data.u32 = hex_value;
> > +			hex_ptr = &tmp_data.u32;
> > +		} else {
> > +			tmp_data.u64 = hex_value;
> > +			hex_ptr = &tmp_data.u64;
> > +		}
> > +		memcpy(tmp_buf, hex_ptr, len);
> > +		value = tmp_buf;
> > +
> > +	} else if (!strncmp(data, "=H", 2)) { /* hexadecimal-byte array */
> > +		data += 2;
> > +		len = strlen(data);
> > +		if (len & 0x1) /* not multiple of two */
> > +			return -1;
> > +
> > +		len /= 2;
> > +		tmp_buf = malloc(len);
> > +		if (!tmp_buf)
> > +			return -1;
> > +
> > +		if (hex2bin((u8 *)tmp_buf, data, len) < 0)
> > +			return -1;
> > +
> > +		value = tmp_buf;
> > +	} else { /* string */
> > +		if (!strncmp(data, "=\"", 2) || !strncmp(data, "=S\"", 3)) {
> > +			if (data[1] == '"')
> > +				data += 2;
> > +			else
> > +				data += 3;
> > +			value = data;
> > +			len = strlen(data) - 1;
> > +			if (data[len] != '"')
> > +				return -1;
> > +		} else {
> > +			value = data;
> > +			len = strlen(data);
> > +		}
> > +	}
> > +
> > +	new_buf = realloc(*bufp, *sizep + len);
> > +	if (!new_buf)
> > +		goto out;
> > +
> > +	memcpy(new_buf + *sizep, value, len);
> > +	*bufp = new_buf;
> > +	*sizep += len;
> > +
> > +out:
> > +	free(tmp_buf);
> > +
> > +	return 0;
> > +}
> > +
> > +static int do_efi_set_var(int argc, char * const argv[])
> > +{
> > +	char *var_name, *value = NULL;
> > +	unsigned long size = 0;
> > +	u16 *var_name16, *p;
> > +	efi_guid_t guid;
> > +	efi_status_t ret;
> > +
> > +	if (argc == 1)
> > +		return CMD_RET_SUCCESS;
> > +
> > +	var_name = argv[1];
> > +	if (argc == 2) {
> > +		/* remove */
> > +		value = NULL;
> > +		size = 0;
> > +	} else { /* set */
> > +		argc -= 2;
> > +		argv += 2;
> > +
> > +		for ( ; argc > 0; argc--, argv++)
> > +			if (append_value(&value, &size, argv[0]) < 0) {
> > +				ret = CMD_RET_FAILURE;
> > +				goto out;
> > +			}
> > +	}
> > +
> > +	var_name16 = malloc((strlen(var_name) + 1) * 2);
> > +	p = var_name16;
> > +	utf8_utf16_strncpy(&p, var_name, strlen(var_name) + 1);
> > +
> > +	guid = efi_global_variable_guid;
> > +	ret = efi_set_variable(var_name16, &guid,
> > +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +			       EFI_VARIABLE_RUNTIME_ACCESS, size, value);
> > +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int do_efi_boot_add(int argc, char * const argv[])
> > +{
> > +	int id;
> > +	char *endp;
> > +	char var_name[9];
> > +	u16 var_name16[9], *p;
> > +	efi_guid_t guid;
> > +	size_t label_len, label_len16;
> > +	u16 *label;
> > +	struct efi_device_path *device_path = NULL, *file_path = NULL;
> > +	struct efi_load_option lo;
> > +	void *data = NULL;
> > +	unsigned long size;
> > +	int ret;
> > +
> > +	if (argc < 6 || argc > 7)
> > +		return CMD_RET_USAGE;
> > +
> > +	id = (int)simple_strtoul(argv[1], &endp, 0);
> > +	if (*endp != '\0' || id > 0xffff)
> > +		return CMD_RET_FAILURE;
> > +
> > +	sprintf(var_name, "Boot%04X", id);
> > +	p = var_name16;
> > +	utf8_utf16_strncpy(&p, var_name, 9);
> > +
> > +	guid = efi_global_variable_guid;
> > +
> > +	/* attributes */
> > +	lo.attributes = 0x1; /* always ACTIVE */
> > +
> > +	/* label */
> > +	label_len = strlen(argv[2]);
> > +	label_len16 = utf8_utf16_strnlen(argv[2], label_len);
> > +	label = malloc((label_len16 + 1) * sizeof(u16));
> > +	if (!label)
> > +		return CMD_RET_FAILURE;
> > +	lo.label = label; /* label will be changed below */
> > +	utf8_utf16_strncpy(&label, argv[2], label_len);
> > +
> > +	/* file path */
> > +	ret = efi_dp_from_name(argv[3], argv[4], argv[5], &device_path,
> > +			       &file_path);
> > +	if (ret != EFI_SUCCESS) {
> > +		ret = CMD_RET_FAILURE;
> > +		goto out;
> > +	}
> > +	lo.file_path = file_path;
> > +	lo.file_path_length = efi_dp_size(file_path)
> > +				+ sizeof(struct efi_device_path); /* for END */
> > +
> > +	/* optional data */
> > +	lo.optional_data = (u8 *)(argc == 6 ? "" : argv[6]);
> > +
> > +	size = efi_serialize_load_option(&lo, (u8 **)&data);
> > +	if (!size) {
> > +		ret = CMD_RET_FAILURE;
> > +		goto out;
> > +	}
> > +
> > +	ret = efi_set_variable(var_name16, &guid,
> > +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +			       EFI_VARIABLE_RUNTIME_ACCESS, size, data);
> > +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> > +out:
> > +	free(data);
> > +#if 0 /* FIXME */
> 
> Eh, what? :)

Well, in the past, I saw u-boot hang-out if free()'s were enabled.
Now I found that we must free data with efi_free_pool() instead of free().

> > +	free(device_path);
> > +	free(file_path);
> > +#endif
> > +	free(lo.label);
> > +	return ret;
> > +}
> > +
> > +static int do_efi_boot_rm(int argc, char * const argv[])
> > +{
> > +	efi_guid_t guid;
> > +	int id, i;
> > +	char *endp;
> > +	char var_name[9];
> > +	u16 var_name16[9];
> > +	efi_status_t ret;
> > +
> > +	if (argc == 1)
> > +		return CMD_RET_USAGE;
> > +
> > +	guid = efi_global_variable_guid;
> > +	for (i = 1; i < argc; i++, argv++) {
> > +		id = (int)simple_strtoul(argv[1], &endp, 0);
> > +		if (*endp != '\0' || id > 0xffff)
> > +			return CMD_RET_FAILURE;
> > +
> > +		sprintf(var_name, "Boot%04X", id);
> > +		utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9);
> > +
> > +		ret = efi_set_variable(var_name16, &guid,
> > +				       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +				       EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL);
> > +		if (ret) {
> > +			printf("cannot remove Boot%04X", id);
> > +			return CMD_RET_FAILURE;
> > +		}
> > +	}
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> > +static void show_efi_boot_opt_data(int id, void *data)
> > +{
> > +	struct efi_load_option lo;
> > +	char *label, *p;
> > +	size_t label_len16, label_len;
> > +	u16 *dp_str;
> > +
> > +	efi_deserialize_load_option(&lo, data);
> > +
> > +	label_len16 = u16_strlen(lo.label);
> > +	label_len = utf16_utf8_strnlen(lo.label, label_len16);
> > +	label = malloc(label_len + 1);
> > +	if (!label)
> > +		return;
> > +	p = label;
> > +	utf16_utf8_strncpy(&p, lo.label, label_len16);
> > +
> > +	printf("Boot%04X:\n", id);
> > +	printf("\tattributes: %c%c%c (0x%08x)\n",
> > +	       /* ACTIVE */
> > +	       lo.attributes & 0x1 ? 'A' : '-',
> > +	       /* FORCE RECONNECT */
> > +	       lo.attributes & 0x2 ? 'R' : '-',
> > +	       /* HIDDEN */
> > +	       lo.attributes & 0x8 ? 'H' : '-',
> > +	       lo.attributes);
> > +	printf("\tlabel: %s\n", label);
> > +
> > +	dp_str = efi_dp_str(lo.file_path);
> > +	printf("\tfile_path: %ls\n", dp_str);
> > +	efi_free_pool(dp_str);
> > +
> > +	printf("\tdata: %s\n", lo.optional_data);
> > +
> > +	free(label);
> > +}
> > +
> > +static void show_efi_boot_opt(int id)
> > +{
> > +	char var_name[9];
> > +	u16 var_name16[9], *p;
> > +	efi_guid_t guid;
> > +	void *data = NULL;
> > +	unsigned long size;
> > +	int ret;
> > +
> > +	sprintf(var_name, "Boot%04X", id);
> > +	p = var_name16;
> > +	utf8_utf16_strncpy(&p, var_name, 9);
> > +	guid = efi_global_variable_guid;
> > +
> > +	size = 0;
> > +	ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
> > +	if (ret == (int)EFI_BUFFER_TOO_SMALL) {
> > +		data = malloc(size);
> > +		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
> > +	}
> > +	if (ret == EFI_SUCCESS) {
> > +		show_efi_boot_opt_data(id, data);
> > +		free(data);
> > +	} else if (ret == EFI_NOT_FOUND) {
> > +		printf("Boot%04X: not found\n", id);
> 
> Missing free?

Fix it.

> > +	}
> > +}
> > +
> > +static int do_efi_boot_dump(int argc, char * const argv[])
> > +{
> > +	char regex[256];
> > +	char * const regexlist[] = {regex};
> > +	char *variables = NULL, *boot, *value;
> > +	int len;
> > +	int id;
> > +
> > +	if (argc > 1)
> > +		return CMD_RET_USAGE;
> > +
> > +	snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+");
> > +
> > +	len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
> > +			&variables, 0, 1, regexlist);
> > +
> > +	if (!len)
> > +		return CMD_RET_SUCCESS;
> > +
> > +	if (len < 0)
> > +		return CMD_RET_FAILURE;
> > +
> > +	boot = variables;
> > +	while (*boot) {
> > +		value = strstr(boot, "Boot") + 4;
> > +		id = (int)simple_strtoul(value, NULL, 16);
> > +		show_efi_boot_opt(id);
> > +		boot = strchr(boot, '\n');
> > +		if (!*boot)
> > +			break;
> > +		boot++;
> > +	}
> > +	free(variables);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> > +static int show_efi_boot_order(void)
> > +{
> > +	efi_guid_t guid;
> > +	u16 *bootorder = NULL;
> > +	unsigned long size;
> > +	int num, i;
> > +	char var_name[9];
> > +	u16 var_name16[9], *p16;
> > +	void *data;
> > +	struct efi_load_option lo;
> > +	char *label, *p;
> > +	size_t label_len16, label_len;
> > +	efi_status_t ret = CMD_RET_SUCCESS;
> > +
> > +	guid = efi_global_variable_guid;
> > +	size = 0;
> > +	ret = efi_get_variable(L"BootOrder", &guid, NULL, &size, NULL);
> > +	if (ret == EFI_BUFFER_TOO_SMALL) {
> > +		bootorder = malloc(size);
> > +		ret = efi_get_variable(L"BootOrder", &guid, NULL, &size,
> > +				       bootorder);
> > +	}
> > +	if (ret != EFI_SUCCESS) {
> > +		printf("BootOrder not defined\n");
> 
> Missing free(bootorder)

OK. In addition, I will modify the code to check for EFI_NOT_FOUND.

> > +		return CMD_RET_SUCCESS;
> > +	}
> > +
> > +	num = size / sizeof(u16);
> > +	for (i = 0; i < num; i++) {
> > +		sprintf(var_name, "Boot%04X", bootorder[i]);
> > +		p16 = var_name16;
> > +		utf8_utf16_strncpy(&p16, var_name, 9);
> > +
> > +		size = 0;
> > +		ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
> > +		if (ret != EFI_BUFFER_TOO_SMALL) {
> > +			printf("%2d: Boot%04X: (not defined)\n",
> > +			       i + 1, bootorder[i]);
> > +			continue;
> > +		}
> > +
> > +		data = malloc(size);
> > +		if (!data) {
> > +			ret = CMD_RET_FAILURE;
> > +			goto out;
> > +		}
> > +		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
> > +		if (ret != EFI_SUCCESS) {
> > +			free(data);
> > +			ret = CMD_RET_FAILURE;
> > +			goto out;
> 
> Probably better to free(data) at out:, no?

It's possible, but 'data' is only allocated and used repeatedly in a loop,
so freeing it within a loop would look to be a good manner.

> > +		}
> > +
> > +		efi_deserialize_load_option(&lo, data);
> > +
> > +		label_len16 = u16_strlen(lo.label);
> > +		label_len = utf16_utf8_strnlen(lo.label, label_len16);
> > +		label = malloc(label_len + 1);
> > +		if (!label) {
> > +			free(data);
> > +			ret = CMD_RET_FAILURE;
> > +			goto out;
> > +		}
> > +		p = label;
> > +		utf16_utf8_strncpy(&p, lo.label, label_len16);
> > +		printf("%2d: Boot%04X: %s\n", i + 1, bootorder[i], label);
> > +		free(label);
> > +
> > +		free(data);
> > +	}
> > +out:
> > +	free(bootorder);
> > +
> > +	return ret;
> > +}
> > +
> > +static int do_efi_boot_order(int argc, char * const argv[])
> > +{
> > +	u16 *bootorder = NULL;
> > +	unsigned long size;
> > +	int id, i;
> > +	char *endp;
> > +	efi_guid_t guid;
> > +	efi_status_t ret;
> > +
> > +	if (argc == 1)
> > +		return show_efi_boot_order();
> > +
> > +	argc--;
> > +	argv++;
> > +
> > +	size = argc * sizeof(u16);
> > +	bootorder = malloc(size);
> > +	if (!bootorder)
> > +		return CMD_RET_FAILURE;
> > +
> > +	for (i = 0; i < argc; i++) {
> > +		id = (int)simple_strtoul(argv[i], &endp, 0);
> > +		if (*endp != '\0' || id > 0xffff) {
> > +			printf("invalid value: %s\n", argv[i]);
> > +			ret = CMD_RET_FAILURE;
> > +			goto out;
> > +		}
> > +
> > +		bootorder[i] = (u16)id;
> > +	}
> > +
> > +	guid = efi_global_variable_guid;
> > +	ret = efi_set_variable(L"BootOrder", &guid,
> > +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +			       EFI_VARIABLE_RUNTIME_ACCESS, size, bootorder);
> > +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> > +out:
> > +	free(bootorder);
> > +
> > +	return ret;
> > +}
> > +
> > +static int do_efi_boot_opt(int argc, char * const argv[])
> > +{
> > +	char *sub_command;
> > +
> > +	if (argc < 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	argc--; argv++;
> > +	sub_command = argv[0];
> > +
> > +	if (!strcmp(sub_command, "add"))
> > +		return do_efi_boot_add(argc, argv);
> > +	else if (!strcmp(sub_command, "rm"))
> > +		return do_efi_boot_rm(argc, argv);
> > +	else if (!strcmp(sub_command, "dump"))
> > +		return do_efi_boot_dump(argc, argv);
> > +	else if (!strcmp(sub_command, "order"))
> > +		return do_efi_boot_order(argc, argv);
> > +	else
> > +		return CMD_RET_USAGE;
> > +}
> > +
> > +/* Interpreter command to configure EFI environment */
> > +static int do_efishell(cmd_tbl_t *cmdtp, int flag,
> > +		       int argc, char * const argv[])
> > +{
> > +	char *command;
> > +	efi_status_t r;
> > +
> > +	if (argc < 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	argc--; argv++;
> > +	command = argv[0];
> > +
> > +	/* Initialize EFI drivers */
> > +	r = efi_init_obj_list();
> > +	if (r != EFI_SUCCESS) {
> > +		printf("Error: Cannot set up EFI drivers, r = %lu\n",
> > +		       r & ~EFI_ERROR_MASK);
> > +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	if (!strcmp(command, "boot"))
> > +		return do_efi_boot_opt(argc, argv);
> > +	else if (!strcmp(command, "dumpvar") || !strcmp(command, "dmpstore"))
> > +		return do_efi_dump_var(argc, argv);
> > +	else if (!strcmp(command, "setvar"))
> > +		return do_efi_set_var(argc, argv);
> > +	else
> > +		return CMD_RET_USAGE;
> > +}
> > +
> > +#ifdef CONFIG_SYS_LONGHELP
> > +static char efishell_help_text[] =
> > +	"  - EFI Shell-like interface to configure EFI environment\n"
> > +	"\n"
> > +	"efishell boot add <bootid> <label> <interface> <device>[:<part>] <file path> <option>\n"
> > +	"  - set uefi's BOOT variable\n"
> > +	"efishell boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
> > +	"  - set/display uefi boot order\n"
> > +	"efishell boot dump\n"
> > +	"  - display all uefi's BOOT variables\n"
> > +	"efishell boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> > +	"  - set/display uefi boot order\n"
> > +	"\n"
> > +	"efishell dumpvar [<name>]\n"
> > +	"  - get uefi variable's value\n"
> > +	"efishell setvar <name> [<value>]\n"
> > +	"  - set/delete uefi variable's value\n"
> > +	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n";
> > +#endif
> > +
> > +U_BOOT_CMD(
> > +	efishell, 10, 0, do_efishell,
> > +	"Configure EFI environment",
> > +	efishell_help_text
> > +);
> > diff --git a/cmd/efishell.h b/cmd/efishell.h
> > new file mode 100644
> > index 000000000000..6f70b402b26b
> > --- /dev/null
> > +++ b/cmd/efishell.h
> > @@ -0,0 +1,5 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <efi.h>
> > +
> > +efi_status_t efi_init_obj_list(void);
> 
> Why isn't this in efi_loader.h? That's the subsystem that exports it, no?

Agree. Moreover, I think of refactoring the code and moving efi_init_obj_list()
to a new file, lib/efi_loader/efi_setup.c, so that we can add more
features directly as part of the initialization code.
Features may include USB removable disk support for which I already posted
a patch set[1] and yet-coming capsule-on-disk support.

Actually, I have this refactoring patch in my local dev branch.
May I submit it as a separate one?

[1] https://lists.denx.de/pipermail/u-boot/2018-November/347493.html

Thanks,
-Takahiro Akashi


> 
> Alex

On 03.12.18 07:42, AKASHI Takahiro wrote:
> On Mon, Dec 03, 2018 at 12:42:43AM +0100, Alexander Graf wrote:
>>
>>
>> On 05.11.18 10:06, AKASHI Takahiro wrote:
>>> Currently, there is no easy way to add or modify UEFI variables.
>>> In particular, bootmgr supports BootOrder/BootXXXX variables, it is
>>> quite hard to define them as u-boot variables because they are represented
>>> in a complicated and encoded format.
>>>
>>> The new command, efishell, helps address these issues and give us
>>> more friendly interfaces:
>>>  * efishell boot add: add BootXXXX variable
>>>  * efishell boot rm: remove BootXXXX variable
>>>  * efishell boot dump: display all BootXXXX variables
>>>  * efishell boot order: set/display a boot order (BootOrder)
>>>  * efishell setvar: set an UEFI variable (with limited functionality)
>>>  * efishell dumpvar: display all UEFI variables
>>>
>>> As the name suggests, this command basically provides a subset fo UEFI
>>> shell commands with simplified functionality.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/Makefile   |   2 +-
>>>  cmd/efishell.c | 673 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  cmd/efishell.h |   5 +
>>>  3 files changed, 679 insertions(+), 1 deletion(-)
>>>  create mode 100644 cmd/efishell.c
>>>  create mode 100644 cmd/efishell.h
>>>
>>> diff --git a/cmd/Makefile b/cmd/Makefile
>>> index d9cdaf6064b8..bd531d505a8e 100644
>>> --- a/cmd/Makefile
>>> +++ b/cmd/Makefile
>>> @@ -24,7 +24,7 @@ obj-$(CONFIG_CMD_BINOP) += binop.o
>>>  obj-$(CONFIG_CMD_BLOCK_CACHE) += blkcache.o
>>>  obj-$(CONFIG_CMD_BMP) += bmp.o
>>>  obj-$(CONFIG_CMD_BOOTCOUNT) += bootcount.o
>>> -obj-$(CONFIG_CMD_BOOTEFI) += bootefi.o
>>> +obj-$(CONFIG_CMD_BOOTEFI) += bootefi.o efishell.o
>>
>> Please create a separate command line option for efishell and make it
>> disabled by default.
> 
> OK.
> 
>> I think it's a really really useful debugging aid, but in a normal
>> environment people should only need to run efi binaries (plus bootmgr)
>> and modify efi variables, no?
> 
> The ultimate goal would be to provide this command as a separate
> application. In this case, however, it won't much different from
> uefi shell itself :)
> 
>>
>>
>>>  obj-$(CONFIG_CMD_BOOTMENU) += bootmenu.o
>>>  obj-$(CONFIG_CMD_BOOTSTAGE) += bootstage.o
>>>  obj-$(CONFIG_CMD_BOOTZ) += bootz.o
>>> diff --git a/cmd/efishell.c b/cmd/efishell.c
>>> new file mode 100644
>>> index 000000000000..abc8216c7bd6
>>> --- /dev/null
>>> +++ b/cmd/efishell.c
>>> @@ -0,0 +1,673 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + *  EFI Shell-like command
>>> + *
>>> + *  Copyright (c) 2018 AKASHI Takahiro, Linaro Limited
>>> + */
>>> +
>>> +#include <charset.h>
>>> +#include <common.h>
>>> +#include <command.h>
>>> +#include <efi_loader.h>
>>> +#include <environment.h>
>>> +#include <errno.h>
>>> +#include <exports.h>
>>> +#include <hexdump.h>
>>> +#include <malloc.h>
>>> +#include <search.h>
>>> +#include <linux/ctype.h>
>>> +#include <asm/global_data.h>
>>> +#include "efishell.h"
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>
>> Where in this file do you need gd?
> 
> OK.
> # I thought this should always be declared in any file by default.

You only need to declare it when you need gd :). Most efi_loader code
should be able to live just fine without.

[...]

>>> +	/* optional data */
>>> +	lo.optional_data = (u8 *)(argc == 6 ? "" : argv[6]);
>>> +
>>> +	size = efi_serialize_load_option(&lo, (u8 **)&data);
>>> +	if (!size) {
>>> +		ret = CMD_RET_FAILURE;
>>> +		goto out;
>>> +	}
>>> +
>>> +	ret = efi_set_variable(var_name16, &guid,
>>> +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> +			       EFI_VARIABLE_RUNTIME_ACCESS, size, data);
>>> +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
>>> +out:
>>> +	free(data);
>>> +#if 0 /* FIXME */
>>
>> Eh, what? :)
> 
> Well, in the past, I saw u-boot hang-out if free()'s were enabled.
> Now I found that we must free data with efi_free_pool() instead of free().

It depends on how data was allocated, yes.

> 
>>> +	free(device_path);
>>> +	free(file_path);
>>> +#endif
>>> +	free(lo.label);
>>> +	return ret;
>>> +}
>>> +

[...]

>>> diff --git a/cmd/efishell.h b/cmd/efishell.h
>>> new file mode 100644
>>> index 000000000000..6f70b402b26b
>>> --- /dev/null
>>> +++ b/cmd/efishell.h
>>> @@ -0,0 +1,5 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +
>>> +#include <efi.h>
>>> +
>>> +efi_status_t efi_init_obj_list(void);
>>
>> Why isn't this in efi_loader.h? That's the subsystem that exports it, no?
> 
> Agree. Moreover, I think of refactoring the code and moving efi_init_obj_list()
> to a new file, lib/efi_loader/efi_setup.c, so that we can add more
> features directly as part of the initialization code.

Well, as Heinrich already indicated, the long term goal would be to have
UEFI handles implicitly generated for DM devices. So we wouldn't need to
maintain our own copy of the already existing object model.


Alex

> Features may include USB removable disk support for which I already posted
> a patch set[1] and yet-coming capsule-on-disk support.
> 
> Actually, I have this refactoring patch in my local dev branch.
> May I submit it as a separate one?
> 
> [1] https://lists.denx.de/pipermail/u-boot/2018-November/347493.html
> 
> Thanks,
> -Takahiro Akashi
> 
> 
>>
>> Alex