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

[AKASHI Takahiro]

On Fri, Apr 12, 2019 at 10:28:09PM +0200, Heinrich Schuchardt wrote:
> 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.

You also suggested initr_reloc_global_data(). No?

Anyhow, I think you ignored my concern:
> >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.

Since I think that we need discuss more, I will leave the code
unchanged in the next version.

-Takahiro Akashi

> 
> 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);
> >>>>>
> >>>>
> >>>
> >
>