[U-Boot] [PATCH v2 4/5] cmd: bootefi: run an EFI application of a specific load option

[Heinrich Schuchardt]

On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
> On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
>>> With this patch applied, we will be able to selectively execute
>>> an EFI application by specifying a load option, say "1" for Boot0001,
>>> "2" for Boot0002 and so on.
>>>
>>>   => bootefi bootmgr <fdt addr> 1, or
>>>      bootefi bootmgr - 1
>>
>> You already introduced the support for BootNext. So is there a real benefit?
> 
> This is a convenient way of running EFI application directly,
> but I already removed this feature from the next version.

Please, remove 'run -e' instead because it cannot specify the device
tree needed for booting ARM boards.

> 
>>>
>>> Please note that BootXXXX need not be included in "BootOrder".
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>> ---
>>>  cmd/bootefi.c | 39 ++++++++++++++++++++++++++++-----------
>>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 3be01b49b589..241fd0f987ab 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare
>>>  
>>>  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>>>  
>>> -static int do_bootefi_bootmgr_exec(void)
>>> +static int do_bootefi_bootmgr_exec(int boot_id)
>>>  {
>>>  	struct efi_device_path *device_path, *file_path;
>>>  	void *addr;
>>>  	efi_status_t r;
>>>  
>>> -	addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER,
>>> -				&device_path, &file_path);
>>> +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
>>>  	if (!addr)
>>> -		return 1;
>>> +		return CMD_RET_FAILURE;
>>>  
>>>  	printf("## Starting EFI application at %p ...\n", addr);
>>>  	r = do_bootefi_exec(addr, device_path, file_path);
>>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void)
>>>  	       r & ~EFI_ERROR_MASK);
>>>  
>>>  	if (r != EFI_SUCCESS)
>>> -		return 1;
>>> +		return CMD_RET_FAILURE;
>>>  
>>> -	return 0;
>>> +	return CMD_RET_SUCCESS;
>>>  }
>>>  
>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>> @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>  	} else
>>>  #endif
>>>  	if (!strcmp(argv[1], "bootmgr")) {
>>> -		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
>>> -			return CMD_RET_FAILURE;
>>> +		char *fdtstr, *endp;
>>> +		int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;
>>> +
>>> +		if (argc > 2) {
>>> +			fdtstr = argv[2];
>>> +			 /* Special address "-" means no device tree */
>>> +			if (fdtstr[0] == '-')
>>> +				fdtstr = NULL;
>>> +
>>> +			r = efi_handle_fdt(fdtstr);
>>> +			if (r)
>>> +				return CMD_RET_FAILURE;
>>> +		}
>>> +
>>> +		if (argc > 3) {
>>> +			boot_id = (int)simple_strtoul(argv[3], &endp, 0);
>>> +			if ((argv[3] + strlen(argv[3]) != endp) ||
>>> +			    boot_id > 0xffff)
>>> +				return CMD_RET_USAGE;
>>> +		}
>>>  
>>> -		return do_bootefi_bootmgr_exec();
>>> +		return do_bootefi_bootmgr_exec(boot_id);
>>
>> Why not communicate via the BootNext variable?
> 
> I don't get your point.
> BootNext and BootOrder are both defined by UEFI specification.

Instead of changing the interface of do_bootefi_bootmgr_exec() you could
simply set BootNext. Then the boot manager would pick up the option from
the variable and finally delete the variable. This would result in less
code.

Best regards

Heinrich

> 
>>>  	} else {
>>>  		saddr = argv[1];
>>>  
>>> @@ -590,7 +607,7 @@ static char bootefi_help_text[] =
>>>  	"    Use environment variable efi_selftest to select a single test.\n"
>>>  	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
>>>  #endif
>>> -	"bootefi bootmgr [fdt addr]\n"
>>> +	"bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n"
>>>  	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
>>>  	"\n"
>>>  	"    If specified, the device tree located at <fdt address> gets\n"
>>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] =
>>>  #endif
>>>  
>>>  U_BOOT_CMD(
>>> -	bootefi, 3, 0, do_bootefi,
>>> +	bootefi, 5, 0, do_bootefi,
>>
>> Why 5?
> 
> For additional/optional '-' and <boot id>.
> But I removed this feature from bootefi.
> 
> Thanks,
> -Takahiro Akashi
> 
> 
>> Best regards
>>
>> Heinrich
>>
>>>  	"Boots an EFI payload from memory",
>>>  	bootefi_help_text
>>>  );
>>>
>>
>