[U-Boot] [PATCH v2 04/14] cmd: add efishell command

[Alexander Graf]

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