Re: [U-Boot] [PATCH v5 1/7] cmd: add efidebug command

On Tue, Jan 22, 2019 at 10:18:58AM +0100, Alexander Graf wrote:

On 22.01.19 02:02, AKASHI Takahiro wrote:

...

Alex,

On Mon, Jan 21, 2019 at 02:07:31PM +0100, Alexander Graf wrote:

...

On 01/21/2019 08:49 AM, 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, efidebug, helps address these issues and give us more friendly interfaces:

• efidebug boot add: add BootXXXX variable
• efidebug boot rm: remove BootXXXX variable
• efidebug boot dump: display all BootXXXX variables
• efidebug boot next: set BootNext variable
• efidebug boot order: set/display a boot order (BootOrder)
• efidebug setvar: set an UEFI variable (with limited functionality)
• efidebug dumpvar: display all UEFI variables

Please note that the file, efidebug.c, will be compiled under CONFIG_EFI_LOADER because some helper functions will be used to enable "env -e" command in a later patch whether or not the command is compiled in.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

MAINTAINERS | 1 + cmd/Kconfig | 10 + cmd/Makefile | 1 + cmd/efidebug.c | 755 ++++++++++++++++++++++++++++++++++++++++++++++ include/command.h | 6 + 5 files changed, 773 insertions(+) create mode 100644 cmd/efidebug.c

diff --git a/MAINTAINERS b/MAINTAINERS index ae825014bda9..301c5c69ea25 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -438,6 +438,7 @@ F: lib/efi*/ F: test/py/tests/test_efi* F: test/unicode_ut.c F: cmd/bootefi.c +F: cmd/efidebug.c F: tools/file2include.c FPGA diff --git a/cmd/Kconfig b/cmd/Kconfig index ea1a325eb301..d9cab3cc0c49 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1397,6 +1397,16 @@ config CMD_DISPLAY displayed on a simple board-specific display. Implement display_putc() to use it. +config CMD_EFIDEBUG

• bool "efidebug - display/customize UEFI environment"
• depends on EFI_LOADER
• default n
• help

•  Enable the 'efidebug' command which provides a subset of UEFI
  

•  shell utility with simplified functionality. It will be useful
  

•  particularly for managing boot parameters as  well as examining
  

•  various EFI status for debugging.
  

config CMD_LED bool "led" default y if LED diff --git a/cmd/Makefile b/cmd/Makefile index 15ae4d250f50..e48d34c394ee 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -51,6 +51,7 @@ obj-$(CONFIG_CMD_ECHO) += echo.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o obj-$(CONFIG_CMD_EEPROM) += eeprom.o obj-$(CONFIG_EFI_STUB) += efi.o +obj-$(CONFIG_EFI_LOADER) += efidebug.o obj-$(CONFIG_CMD_ELF) += elf.o obj-$(CONFIG_HUSH_PARSER) += exit.o obj-$(CONFIG_CMD_EXT4) += ext4.o diff --git a/cmd/efidebug.c b/cmd/efidebug.c new file mode 100644 index 000000000000..c54fb6cfa101 --- /dev/null +++ b/cmd/efidebug.c @@ -0,0 +1,755 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • UEFI 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>

+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] = (char)simple_strtoul(hex, 0, 16);
  

• 	if (line[i] < 0x20 || line[i] > 0x7f)
  

• 		line[i] = '.';
  

• }
  

• line[i] = '\0';
  

• printf("  %s\n", line);
  

• }

+}

Is this print_hex_dump() reimplemented?

Actually, no. A UEFI variable on u-boot is encoded as ascii representation of binary data. That means, for example,

    => env set -e PlatformLang en
    => env print -e
    PlatformLang: {boot,run}(blob)
    00000000:  65 6e                    en
    => env print
    ...
    efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_PlatformLang={boot,run}(blob)656e
    ...

the value of "PlatformLang" as a u-boot variable here is "656e", not "en." So if we want to use print_hex_dump(), we first have to convert that string to a binary. But then print_hex_dump() converts the binary to a string. It's just rediculuous, isn't it?

I actually think I would prefer that. This way we consolidate everything through a single API which means we then know that the results are always the same. If we implement parsing multiple times, there's a good chance we'll have a bug in one of them which gives us different results which then again makes debugging harder rather than easier.

Agree, but please note we have to add an apparently weird dependency of CONFIG_HEXDUMP to EFI_LOADER as do_efi_dump_var_ext() needs to be compiled in whether CMD_EFIDEBUG is enabled or not.

...

You might think that the value in this case should be {boot, run}(utf8)en ^^^^ It's possible, but it depends on a variable and currently my do_set_efi_var() doesn't support it anyway.

One more reason to use only a single path to funnel things through :).

So yes, can you maybe reuse RTS->get_variable() here? Same question on writing variables I suppose.

Well, we need GetNextVariableName API if we want to use GetVariable (for listing). I have held off merging my another patch for this. Is it time to do it?

-Takahiro Akashi

Alex