[U-Boot] [RFC v2 09/11] cmd: bootefi: carve out do_boot_efi() from do_bootefi()

[Heinrich Schuchardt]

On 4/12/19 9:22 AM, AKASHI Takahiro wrote:
> On Fri, Apr 12, 2019 at 07:59:51AM +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.
>>> All the non-boot-manager-based (that is, bootefi <addr>) code is put
>>> into one function, do_boot_efi().
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>> ---
>>>  cmd/bootefi.c | 122 ++++++++++++++++++++++++++++----------------------
>>>  1 file changed, 68 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 94b5bdeed3f1..39a0712af6ad 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -355,6 +355,73 @@ static int do_efibootmgr(const char *fdt_opt)
>>>  	return 0;
>>>  }
>>>
>>> +/*
>>> + * do_boot_efi() - execute EFI binary from command line
>>> + *
>>> + * @image_opt:	string of image start address
>>> + * @fdt_opt:	string of fdt start address
>>> + * Return:	status code
>>> + *
>>> + * Set up memory image for the binary to be loaded, prepare
>>> + * device path and then call do_bootefi_exec() to execute it.
>>> + */
>>> +static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>>> +{
>>> +	unsigned long addr;
>>> +	char *saddr;
>>> +	efi_status_t ret;
>>> +	unsigned long fdt_addr;
>>> +
>>> +	/* Allow unaligned memory access */
>>> +	allow_unaligned();
>>> +
>>> +	switch_to_non_secure_mode();
>>
>> The two call above should be moved to efi_init_obj_list().
>
> Same comment as in patch#8/11
>
>>> +
>>> +	/* 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;
>>> +
>>> +#ifdef CONFIG_CMD_BOOTEFI_HELLO
>>> +	if (!strcmp(argv[1], "hello")) {
>>> +		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>>> +
>>> +		saddr = env_get("loadaddr");
>>> +		if (saddr)
>>> +			addr = simple_strtoul(saddr, NULL, 16);
>>> +		else
>>> +			addr = CONFIG_SYS_LOAD_ADDR;
>>> +		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
>>> +	} else
>>> +#endif
>>> +	{
>>> +		saddr = argv[1];
>>> +
>>> +		addr = simple_strtoul(saddr, NULL, 16);
>>> +		/* Check that a numeric value was passed */
>>> +		if (!addr && *saddr != '0')
>>> +			return CMD_RET_USAGE;
>>> +	}
>>> +
>>> +	printf("## Starting EFI application at %08lx ...\n", addr);
>>
>> Shouldn't this be debug() in efi_start_image()?
>
> Okay, and application -> image
> # but I'm not sure that this information is useful.

When debugging I would be more interested in the address to which the
image is loaded so that I have the offset at hand to load the text
symbols into gdb.

I am happy if you simply drop this entry address output.

Regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>>
>>> +	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
>>> +			      bootefi_image_path);
>>> +	printf("## Application terminated, r = %lu\n",
>>> +	       ret & ~EFI_ERROR_MASK);
>>> +
>>> +	if (ret != EFI_SUCCESS)
>>> +		return CMD_RET_FAILURE;
>>> +	else
>>> +		return CMD_RET_SUCCESS;
>>> +}
>>> +
>>>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>>  static efi_status_t bootefi_run_prepare(const char *load_options_path,
>>>  		struct efi_device_path *device_path,
>>> @@ -481,11 +548,6 @@ static int do_efi_selftest(const char *fdt_opt)
>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>  {
>>> -	unsigned long addr;
>>> -	char *saddr;
>>> -	efi_status_t r;
>>> -	unsigned long fdt_addr;
>>> -
>>>  	if (argc < 2)
>>>  		return CMD_RET_USAGE;
>>>
>>> @@ -496,55 +558,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>  		return do_efi_selftest(argc > 2 ? argv[2] : NULL);
>>>  #endif
>>>
>>> -	/* Allow unaligned memory access */
>>> -	allow_unaligned();
>>> -
>>> -	switch_to_non_secure_mode();
>>> -
>>> -	/* 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;
>>> -	}
>>> -
>>> -	r = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
>>> -	if (r != EFI_SUCCESS)
>>> -		return r;
>>> -
>>> -#ifdef CONFIG_CMD_BOOTEFI_HELLO
>>> -	if (!strcmp(argv[1], "hello")) {
>>> -		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>>> -
>>> -		saddr = env_get("loadaddr");
>>> -		if (saddr)
>>> -			addr = simple_strtoul(saddr, NULL, 16);
>>> -		else
>>> -			addr = CONFIG_SYS_LOAD_ADDR;
>>> -		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
>>> -	} else
>>> -#endif
>>> -	{
>>> -		saddr = argv[1];
>>> -
>>> -		addr = simple_strtoul(saddr, NULL, 16);
>>> -		/* Check that a numeric value was passed */
>>> -		if (!addr && *saddr != '0')
>>> -			return CMD_RET_USAGE;
>>> -
>>> -	}
>>> -
>>> -	printf("## Starting EFI application at %08lx ...\n", addr);
>>> -	r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
>>> -			    bootefi_image_path);
>>> -	printf("## Application terminated, r = %lu\n",
>>> -	       r & ~EFI_ERROR_MASK);
>>> -
>>> -	if (r != EFI_SUCCESS)
>>> -		return 1;
>>> -	else
>>> -		return 0;
>>> +	return do_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
>>>  }
>>>
>>>  #ifdef CONFIG_SYS_LONGHELP
>>>
>>
>