[U-Boot] [RFC v2 08/11] cmd: bootefi: carve out bootmgr code from do_bootefi()

[Heinrich Schuchardt]

On 4/12/19 4:19 PM, AKASHI Takahiro wrote:
> On Fri, Apr 12, 2019 at 10:58:25AM +0200, Heinrich Schuchardt wrote:
>>
>>
>> On 4/12/19 9:06 AM, AKASHI Takahiro wrote:
>>> On Fri, Apr 12, 2019 at 07:55:16AM +0200, Heinrich Schuchardt wrote:
>>>> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
>>>>> This is a preparatory patch for reworking do_bootefi() in later patch.
>>>>> do_bootmgr_exec() is renamed to do_efibootmgr() as we put all the necessary
>>>>> code into this function.
>>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>>>> ---
>>>>>   cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++--------
>>>>>   1 file changed, 34 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>> index e9d4881997a1..94b5bdeed3f1 100644
>>>>> --- a/cmd/bootefi.c
>>>>> +++ b/cmd/bootefi.c
>>>>> @@ -309,22 +309,47 @@ err_add_protocol:
>>>>>   	return ret;
>>>>>   }
>>>>>
>>>>> -static int do_bootefi_bootmgr_exec(void)
>>>>> +/**
>>>>> + * do_efibootmgr() - execute EFI Boot Manager
>>>>> + *
>>>>> + * @fdt_opt:	string of fdt start address
>>>>> + * Return:	status code
>>>>> + *
>>>>> + * Execute EFI Boot Manager
>>>>> + */
>>>>> +static int do_efibootmgr(const char *fdt_opt)
>>>>>   {
>>>>>   	struct efi_device_path *device_path, *file_path;
>>>>>   	void *addr;
>>>>> -	efi_status_t r;
>>>>> +	efi_status_t ret;
>>>>> +
>>>>> +	/* Allow unaligned memory access */
>>>>> +	allow_unaligned();
>>>>> +
>>>>> +	switch_to_non_secure_mode();
>>>>> +
>>>>
>>>> Shouldn't we move these two call to efi_init_obj_list()?
>>>
>>> Given the fact that efi_init_obj_list() is called without invoking
>>> any UEFI binary at some places, I'm not sure that it is the right
>>> place where switch_to_non_secure_mode() be called.
>
> If this is UEFI(and arm)-specific, it should be placed just
> before setjmp() in efi_start_image().
>
> But I'm not sure whether we should call switch_to_non_secure_mode()
> even for a driver, which is logically part of U-Boot UEFI.

switch_to_not_secure_mode() is a weak function which is implemented only
for armv7 and armv8.

efi_init_obj_list() would be a safe place to call it.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>> I think this could even be done in initr_reloc_global_data().
>>
>> @Alex
>> What are your thoughts.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> +	/* Initialize EFI drivers */
>>>>> +	ret = efi_init_obj_list();
>>>>> +	if (ret != EFI_SUCCESS) {
>>>>> +		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
>>>>> +		       ret & ~EFI_ERROR_MASK);
>>>>> +		return CMD_RET_FAILURE;
>>>>> +	}
>>>>> +
>>>>> +	ret = efi_install_fdt(fdt_opt);
>>>>> +	if (ret != EFI_SUCCESS)
>>>>> +		return CMD_RET_FAILURE;
>>>>>
>>>>>   	addr = efi_bootmgr_load(&device_path, &file_path);
>>>>>   	if (!addr)
>>>>>   		return 1;
>>>>>
>>>>>   	printf("## Starting EFI application at %p ...\n", addr);
>>>>> -	r = do_bootefi_exec(addr, device_path, file_path);
>>>>> +	ret = do_bootefi_exec(addr, device_path, file_path);
>>>>>   	printf("## Application terminated, r = %lu\n",
>>>>> -	       r & ~EFI_ERROR_MASK);
>>>>> +	       ret & ~EFI_ERROR_MASK);
>>>>>
>>>>> -	if (r != EFI_SUCCESS)
>>>>> +	if (ret != EFI_SUCCESS)
>>>>>   		return 1;
>>>>>
>>>>>   	return 0;
>>>>> @@ -463,6 +488,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>>
>>>>>   	if (argc < 2)
>>>>>   		return CMD_RET_USAGE;
>>>>> +
>>>>> +	if (!strcmp(argv[1], "bootmgr"))
>>>>> +		return do_efibootmgr(argc > 2 ? argv[2] : NULL);
>>>>>   #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>>>>   	else if (!strcmp(argv[1], "selftest"))
>>>>>   		return do_efi_selftest(argc > 2 ? argv[2] : NULL);
>>>>> @@ -497,9 +525,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>>   		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
>>>>>   	} else
>>>>>   #endif
>>>>> -	if (!strcmp(argv[1], "bootmgr")) {
>>>>> -		return do_bootefi_bootmgr_exec();
>>>>> -	} else {
>>>>> +	{
>>>>>   		saddr = argv[1];
>>>>>
>>>>>   		addr = simple_strtoul(saddr, NULL, 16);
>>>>>
>>>>
>>>
>